Date: Tue, 26 Mar 2024 17:54:58 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Michael Paquier <michael@paquier.xyz>,
	Peter Eisentraut <peter@eisentraut.org>,
	Soumyadeep Chakraborty <soumyadeep2007@gmail.com>,
	Zhihong Yu <zyu@yugabyte.com>, pgsql-hackers@postgresql.org,
	Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com,
	Alexander Lakhin <exclusion@gmail.com>
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Message-ID: <ZgNSQraRJ4vWe6KI@pryzbyj2023>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202403261105.6o4ff4d6qagc@alvherre.pgsql>
 <202403211207.hfyirjweshbh@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/ZgNSQraRJ4vWe6KI%40pryzbyj2023>
Precedence: bulk

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