Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Thu, Mar 21, 2024 at 01:07:01PM +0100, Alvaro Herrera wrote:
> Given that Michaël is temporarily gone, I propose to push the attached
> tomorrow.

Thanks.

On Tue, Mar 26, 2024 at 12:05:47PM +0100, Alvaro Herrera wrote:
> On 2024-Mar-26, Alexander Lakhin wrote:
>
> > Hello Alvaro,
> >
> > 21.03.2024 15:07, Alvaro Herrera wrote:
> > > Given that Michaël is temporarily gone, I propose to push the attached
> > > tomorrow.
> >
> > Please look at a new anomaly introduced with 374c7a229.
> > Starting from that commit, the following erroneous query:
> > CREATE FOREIGN TABLE fp PARTITION OF pg_am DEFAULT SERVER x;
> >
> > triggers an assertion failure:
> > TRAP: failed Assert("relation->rd_rel->relam == InvalidOid"), File: "relcache.c", Line: 1219, PID: 3706301
>
> Hmm, yeah, we're setting relam for relations that shouldn't have it.
> I propose the attached.

Looks right. That's how I originally wrote it, except for the
"stmt->accessMethod != NULL" case.

I prefered my way - the grammar should refuse to set stmt->accessMethod
for inappropriate relkinds. And you could assert that.

I also prefered to set "accessMethodId = InvalidOid" once, rather than
twice.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8a02c5b05b6..050be89728f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -962,18 +962,21 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* case of a partitioned table) the parent's, if it has one.
*/
if (stmt->accessMethod != NULL)
- accessMethodId = get_table_am_oid(stmt->accessMethod, false);
- else if (stmt->partbound)
{
- Assert(list_length(inheritOids) == 1);
- accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ Assert(RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE);
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
}
- else
- accessMethodId = InvalidOid;
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ if (stmt->partbound)
+ {
+ Assert(list_length(inheritOids) == 1);
+ accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ }

- /* still nothing? use the default */
- if (RELKIND_HAS_TABLE_AM(relkind) && !OidIsValid(accessMethodId))
- accessMethodId = get_table_am_oid(default_table_access_method, false);
+ if (RELKIND_HAS_TABLE_AM(relkind) && !OidIsValid(accessMethodId))
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
+ }

/*
* Create the relation. Inherited defaults and constraints are passed in