On Fri, Sep 16, 2022 at 1:08 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 3 Aug 2022 at 11:07, Arne Roland <A.Roland@index.de> wrote:
> > Attached a rebased version of the patch.
> Firstly, I agree that we should fix the issue of join removals not
> working with partitioned tables.
Agreed, though the patch's changes to tests does not seem to have to
do with join removal? I don't really understand what the test changes
are all about. I wonder why the patch doesn't instead add the test
case that Arne showed in the file he attached with [1].
> I think the patch should be changed so that the existing list is used
> and we find another fix for the problems Alvaro fixed in 05fb5d661.
> Unfortunately, there was no discussion marked on that commit message,
> so it's not quite clear what the problem was. I'm unsure if there was
> anything other than CLUSTER that was broken. I see that cfdd03f45
> added CLUSTER for partitioned tables in v15. I think the patch would
> need to go over the usages of RelOptInfo.indexlist to make sure that
> we don't need to add any further conditions to skip their usage for
> partitioned tables.
>
> I wrote the attached patch as I wanted to see what would break if we
> did this. The only problem I got from running make check was in
> get_actual_variable_range(), so I just changed that so it returns
> false when the given rel is a partitioned table. I only quickly did
> the changes to get_relation_info() and didn't give much thought to
> what the can* bool flags should be set to. I just mostly skipped all
> that code because it was crashing on
> relation->rd_tableam->scan_bitmap_next_block due to the rd_tableam
> being NULL.
Yeah, it makes sense to just skip the portion of the code that reads
from rd_indam, as your patch does.
+ if (indexRelation->rd_rel->relkind != RELKIND_PARTITIONED_INDEX)
{
+ /* We copy just the fields we need, not all of rd_indam */
+ amroutine = indexRelation->rd_indam;
Maybe you're intending to add one before committing but there should
be a comment mentioning why the am* initializations are being skipped
over for partitioned index IndexOptInfos. I'd think that's because we
are not going to be building any paths using them for now.
The following portion of the top comment of get_relation_info()
perhaps needs an update.
* If inhparent is true, all we need to do is set up the attr arrays:
* the RelOptInfo actually represents the appendrel formed by an inheritance
* tree, and so the parent rel's physical size and index information isn't
* important for it.
*/
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
[1] https://www.postgresql.org/message-id/2641568c18de40e8b1528fc9d4d80127%40index.de
