On Wed, Jan 19, 2022 at 1:50 PM Arne Roland <a.rol...@index.de> wrote:
> Hi, > > > I came up with a slightly more intuitive way to force the different > outcome for the fractional test, that does hardly change anything. > > > I'm not sure, whether the differentiation between fract_x and fract_t is > worth it, since there shouldn't be any difference, but as mentioned before > I added it for potential future clarity. > > > Thanks for your feedback again! > > > Regards > > Arne > > > ------------------------------ > *From:* Arne Roland > *Sent:* Wednesday, January 19, 2022 10:13:55 PM > *To:* Alvaro Herrera; Julien Rouhaud > *Cc:* pgsql-hackers > *Subject:* Re: missing indexes in indexlist with partitioned tables > > > Hi! > > > From: Alvaro Herrera <alvhe...@alvh.no-ip.org> > > [...] > > Hmm, these plan changes look valid to me. A left self-join using the > > primary key column? That looks optimizable all right. > > [...] > > What I still don't know is whether this patch is actually desirable or > > not. If the only cases it affects is self-joins, is there much actual > > value? > > This is not really about self joins. That was just the most simple > example, because otherwise we need a second table. > It's unique, it's not relevant whether it's the same table. In most cases > it won't. I was talking about join pruning. > > > I suspect that the author of partition-wise joins would want to change > > these queries so that whatever was being tested by these self-joins is > > tested by some other means (maybe just create an identical partitioned > > table via CREATE TABLE fract_t2 ... ; INSERT INTO fract_t2 SELECT FROM > > fract_t). But at the same time, the author of this patch should > > Your suggestion doesn't work, because with my patch we solve that case as > well. We solve the general join pruning case. If we make the index > non-unique however, we should be able to test the fractional case the > same way. > > > b) add some test cases to verify that his desired > > behavior is tested somewhere, not just in a test case that's > > incidentally broken by his patch. > > My patch already includes such a test, look at @@ -90,6 +90,13 @@ > src/test/regress/sql/partition_join.sql > Since the selfjoin part was confusing to you, it might be worthwhile to do > test that with two different tables. While I see no need to test in that > way, I will adjust the patch so. Just to make it more clear for people > looking at those tests in the future. > > a) make > > sure that the submitted patch updates these test results so that the > > test pass [...] > > Just for the record: I did run the tests, but I did miss that the commit > of Tomas fix for fractional optimization is already on the master. Please > note that this is a very new test case from a patch committed less than one > week ago. > > I'm glad Julien Rouhaud pointed out, that Tomas patch is committed it by > now. That was very helpful to me, as I can now integrate the two tests. > > @Álvaro Herrera: > If you want to help me, please don't put forward an abstract list of > responsibilities. If anything I obviously need practical help, on how I can > catch on recent changes quicker and without manual intervention. I don't > have a modified buildfarm animal running, that tries to apply my patch and > run regression tests for my patch on a daily basis. > Is there a simple way for me to check for that? > > I will probably integrate those two tests, since they can work of similar > structures without need to recreate the tables again and again. I have > clear understanding how that new test works. I have to attend a few calls > now, but I should be able to update the tests later. > > Regards > Arne > > Hi, - if (indexRelation->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) + if (inhparent && (!index->indisunique || indexRelation->rd_rel->relkind != RELKIND_PARTITIONED_INDEX)) The check on RELKIND_PARTITIONED_INDEX seems to negate what the comment above says: + * Don't add partitioned indexes to the indexlist Cheers
