Allow setting table AM for partition roots

[huansong]

Reviewers: there are a bunch of CI failures. I'm fixing those now. Might have some additional changes to the PR. So no hurry on reviewing.

Adapted from patch submitted against PG 15: https://postgr.es/m/CAE-ML%2B9NycfS3Q9PhMVpgB2NN62HMa9OBCwNW60AiO8NgCCNXw%40mail.gmail.com

This enables specification of a table AM for partition roots at table creation time. Creating a partitioned table with a specific AM specified will enable subsequent children to inherit the AM from the parent.

For setting the access method of an existing partitioned table:

• If the ONLY clause is specified, it will set the access method at the root only. Existing children will not be affected.
• If the ONLY clause is NOT specified, it will set the access method at the root and for all existing children.

Conflicts resolved:

• Mostly conflicts due to PG12 vs PG15: signature changes, non-existing macros etc.

• utility.c: Remove explicit sabotage of the am for partition roots that was in place.

• psql: Had to fix describe table for partitioned AO/AOCO tables: partitioned tables can't have AO/AOCO specific reloptions (yet)

Tests adjusted:

• alter_table_aocs2: partition root has AM in catalog.
• partition: partition root has AM in catalog.
• dsp: partition root has AM, and the child should default to the parent's AM instead of default_table_access_method.

Dev pipeline: https://dev.ci.gpdb.pivotal.io/teams/main/pipelines/atsetam_part

[huansong]

the following commands and it crashed: create table foo (a int, b int) partition by range(b) (start (0) end (6) every (3)); alter table foo set access method ao_row; -- this sets the AM for the entire partitioning hierarchy as expected create table foox (like foo); alter table foo attach partition foox for values from (6) to (9); -- I'm not sure what should be the expectation for this one, but right now we keep the original AM of the attached partition as-is. alter table foo set access method ao_row; -- Then I try to again set AM for the entire partitioning hierarchy, and this crashes

@l-wang Great catch! Fixed - issue was that when altering table again with the existing AM, we pass down an AM oid=0 which is supposed to mean no-op, but we were incorrectly still trying to set that AM. Added a test case too.

[l-wang]

@huansong Could you also mention in the commit message about the expectations for ALTER TABLE ... EXCHANGE PARTITION ...? It looks like we keep the original storage type of the exchanged partitions, better to document it and/or add a test case for it.

[huansong]

Could you explain more on why do we need the revert commit? I don't mind it but just curious what hurdle it introduced for this PR.

It is not a must-have for this PR but it makes things easier and cleaner. W/o the change we'll have to check both pg_class and then pg_am to get the AM handler ID of the parent partition, despite that AM oid is all we need.

Could you also mention in the commit message about the expectations for ALTER TABLE ... EXCHANGE PARTITION ...? It looks like we keep the original storage type of the exchanged partitions, better to document it and/or add a test case for it.

Good point. The EXCHANGE PARTITION behavior stays the same as existing, which is that the AMs of the source and target tables are exchanged. This PR only addresses issues with setting AM for partition root, so EXCHANGE PARTITION of the child table is unaffected. But yes we can clarify that in the commit message.

[l-wang]

Could you also mention in the commit message about the expectations for ALTER TABLE ... EXCHANGE PARTITION ...? It looks like we keep the original storage type of the exchanged partitions, better to document it and/or add a test case for it.

Good point. The EXCHANGE PARTITION behavior stays the same as existing, which is that the AMs of the source and target tables are exchanged. This PR only addresses issues with setting AM for partition root, so EXCHANGE PARTITION of the child table is unaffected. But yes we can clarify that in the commit message.

Sorry my bad! Right this doesn't have much to do with EXCHANGE PARTITION, that was a typo I meant to say ALTER TABLE ... ATTACH PARTITION ...

[huansong]

Sorry my bad! Right this doesn't have much to do with EXCHANGE PARTITION, that was a typo I meant to say ALTER TABLE ... ATTACH PARTITION ...

No worries! I see. Yes ATTACH PARTITION is more relevant to this PR. The current behavior is that the attached partition will keep its original AM. I believe this is the desired behavior because the change of AM shouldn't happen outside of ALTER TABLE (i.e. altering the leaf table being attached). Imagine EXCHANGE PARTITION but just w/o a leaf partition to swap, the new leaf should keep its original AM.

Agreed that we should add commit message and test case. Done.

[huansong]

CI failure is an unrelated flake. Dev pipeline all passed: https://dev.ci.gpdb.pivotal.io/teams/main/pipelines/atsetam_part

[l-wang]

Could you explain more on why do we need the revert commit? I don't mind it but just curious what hurdle it introduced for this PR.

It is not a must-have for this PR but it makes things easier and cleaner. W/o the change we'll have to check both pg_class and then pg_am to get the AM handler ID of the parent partition, despite that AM oid is all we need.

Thanks for explaining. After discussing w/ @soumyadeep2007 and @huansong offline, I'm convinced reverting that commit makes our lives easier. Given upstream doesn't check table am handler id for heap am handler either, the previous commit was likely added for our own testing purposes only at the time of development.