Re: missing indexes in indexlist with partitioned tables - Mailing list pgsql-hackers

On Sun, 2 Oct 2022 at 05:34, Arne Roland <A.Roland@index.de> wrote:
>
> On Tue, Sep 20, 2022 at 4:53 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > Arne sent me an off-list
> > message to say he's planning on working on the patch that uses the
> > existing field instead of the new one he originally added. Let's hold
> > off for that patch.
>
> I wouldn't say, I explicitly stated that. But I ended up doing something, that resulted in the attached patch. :)

I stand corrected. You said you'd think about it, not do it.  Anyway,
thanks for doing it :)

> Further feedback would be appreciated!

I had a quick look through the patch. Here are a few things that would
be good to adjust.  I understand that some of these things were how I
left them in the patch I sent. In my defence, I mostly did that very
quickly just so I could see if there was some issue with having the
partitioned indexes in indexlist. I didn't actually put much effort
into addressing the fine details of how that should be done.

* In the header comment in get_relation_info(), I don't think we need
to mention join removals explicitly. At a stretch, maybe mentioning
"unique proofs" might be ok, but "various optimizations" might be
better. If you mention "join removal", I fear that will just become
outdated too quickly as further optimisations are added.  Likewise for
the comment about "join pruning" you've added in the function body.
FWIW, we call these "join removals" anyway.

* I think we should put RelationGetNumberOfBlocks() back to what it
was and just ensure we don't call that for partitioned indexes in
get_relation_info(). (Yes, I know that was my change)

* I can't quite figure out why you're doing "DROP TABLE a CASCADE;" in
inherits.sql.  You've not changed anything else in that file. Did you
mean to do this in join.sql?

* The new test in join.sql. I understand that you've mostly copied the
test from another place in the file and adjusted it to use a
partitioned table. However, I don't really think you need to INSERT
any data into those tables. I also think that using the table name of
"a" is dangerous as it could conflict with another table by the same
name in a parallel run of the tests.  The non-partitioned version is a
TEMP table.  Also, it's slightly painful to look at the inconsistent
capitalisation of SQL keywords in those queries you've added, again, I
understand those are copied from above, but I see no need to duplicate
the inconsistencies.  Perhaps it's fine to copy the upper case
keywords in the DDL and keep all lowercase in the queries.  Also, I
think you probably should just add a single simple join removal test
for partitioned tables.  You're not exercising any code that the
non-partitioned case isn't by adding any additional tests.  All that I
think is worth testing here is ensuring nobody thinks that partitioned
tables can get away with an empty indexlist again.

* I had a bit of a 2nd thought on the test change in
partition_join.sql.  I know I added the "c" column so that join
removals didn't apply. I'm now thinking it's probably better to just
change the queries to use JOIN ON rather than JOIN USING. Also, apply
the correct alias to the ORDER BY.   This method should save from
slowing the test down due to the additional column. We have some
pretty slow buildfarm machines that this might actually make a
meaningful difference to.

Thanks again for working on this.

David