Re: missing indexes in indexlist with partitioned tables

On Wed, Jan 19, 2022 at 1:50 PM Arne Roland <A(dot)Roland(at)index(dot)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 <alvherre(at)alvh(dot)no-ip(dot)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