Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

From: Michael Paquier <[email-masked]>
Date: Mon, 15 Apr 2024 10:46:00 +0900

On Tue, Apr 02, 2024 at 01:06:06AM -0400, Tom Lane wrote:
> AFAICS, e2395cdbe posits that taking exclusive lock on pg_am in the
> middle of a bunch of concurrent regression scripts couldn't possibly
> cause any problems.  Really?

There is no need for a catalog here to trigger the failure, and it
would have happened as long as a foreign table is used.  The problem
introduced in 374c7a229042 fixed by e2395cdbe83a comes from a thinko
on my side, my apologies for that and the delay in replying.  Thanks
for the extra fix done in 13b3b62746ec, Alvaro.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 16 Apr 2024 14:14:21 +0900

> for the extra fix done in 13b3b62746ec, Alvaro.

While doing more tests with this feature, among other things, I've
spotted an incorrect behavior with dump/restore with the handling of
the GUC default_table_access_method when it comes to partitions.
Imagine the following in database "a":
CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id);
CREATE TABLE parent_tab_2 (id int) PARTITION BY RANGE (id) USING heap;
CREATE TABLE parent_tab_3 (id int) PARTITION BY RANGE (id);

This leads to the following in pg_class:
=# SELECT relname, relam FROM pg_class WHERE oid > 16000;
   relname    | relam 
--------------+-------
 parent_tab   |     0
 parent_tab_2 |     2
 parent_tab_3 |     0
(3 rows)

Now, let's do the following:
$ createdb b
$ pg_dump | psql b
$ psql b
=# SELECT relname, relam FROM pg_class WHERE oid > 16000;
   relname    | relam 
--------------+-------
 parent_tab   |     0
 parent_tab_2 |     0
 parent_tab_3 |     0
(3 rows)

And parent_tab_2 would now rely on the default GUC when creating new
partitions rather than enforce heap.

It seems to me that we are going to extend the GUC
default_table_access_method with a "default" mode to be able to force
relam to 0 and make a difference with the non-0 case, in the same way
as ALTER TABLE SET ACCESS METHOD DEFAULT.  The thing is that, like
tablespaces, we have to rely on a GUC and not a USING clause to be
able to handle --no-table-access-method.

An interesting point comes to what we should do for
default_table_access_method set to "default" when dealing with
something else than a partitioned table, where an error may be
adapted.  Still, I'm wondering if there are more flavors I lack
imagination for.  This requires more careful design work.

Perhaps somebody has a good idea?
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 16 Apr 2024 14:19:56 +0900

> Perhaps somebody has a good idea?

Actually, I've come up with an idea just after hitting the send
button: let's use an extra ALTER TABLE SET ACCESS METHOD rather than
rely on the GUC to set the AM of the partitioned table correctly.
This extra command should be optional, depending on
--no-table-access-method.  If a partitioned table has 0 as relam,
let's not add this extra ALTER TABLE at all.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 17 Apr 2024 14:31:47 +0900

> let's not add this extra ALTER TABLE at all.

I have explored this idea, and while this is tempting this faces a
couple of challenges:
1) Binary upgrades would fail because the table rewrite created by
ALTER TABLE SET ACCESS METHOD for relkinds with physical storage
expects heap_create_with_catalog to have a fixed OID, but the rewrite
would require extra steps to be able to handle that, and I am not
convinced that more binary_upgrade_set_next_heap_relfilenode() is a
good idea.
2) We could limit these extra ALTER TABLE commands to be generated for
partitioned tables.  This is kind of confusing as resulting dumps
would mix SET commands for default_table_access_method that would
affect tables with physical storage, while partitioned tables would
have their own extra ALTER TABLE commands.  Another issue that needs
more consideration is that TocEntrys don't hold any relkind
information so pg_backup_archiver.c cannot make a difference with
tables and partitioned tables to select if SET or ALTER TABLE should
be generated.

Several designs are possible, like:
- Mix SET and ALTER TABLE commands in the dumps to set the AM, SET for
tables and matviews, ALTER TABLE for relations without storage.  This
would bypass the binary upgrade problem with the fixed relid.
- Use only SET, requiring a new "default" value for
default_table_access_method that would force a partitioned table's
relam to be 0.  Be stricter with the "current" table AM tracked in
pg_dump's backup archiver.
- Use only ALTER TABLE commands, with extra binary upgrade tweaks to
force relation OIDs for the second heap_create_with_catalog() done
with the rewrite to update a relation's AM.

With all that in mind, it may be better to revert 374c7a229042 and
e2395cdbe83a from HEAD and reconsider how to tackle the dump issues in
v18 or newer versions as all of the approaches I can think of lead to
more complications of their own.

Please see attached a non-polished POC that switches dumps to use
ALTER TABLE, that I've used to detect the upgrade problems.

Thoughts or comments are welcome.
--
Michael
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index c7a6c918a6..93df260986 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -61,7 +61,7 @@ static void _becomeUser(ArchiveHandle *AH, const char *user);
 static void _becomeOwner(ArchiveHandle *AH, TocEntry *te);
 static void _selectOutputSchema(ArchiveHandle *AH, const char *schemaName);
 static void _selectTablespace(ArchiveHandle *AH, const char *tablespace);
-static void _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam);
+static void _selectTableAccessMethod(ArchiveHandle *AH, TocEntry *te);
 static void processEncodingEntry(ArchiveHandle *AH, TocEntry *te);
 static void processStdStringsEntry(ArchiveHandle *AH, TocEntry *te);
 static void processSearchPathEntry(ArchiveHandle *AH, TocEntry *te);
@@ -2373,7 +2373,6 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
 	AH->currUser = NULL;		/* unknown */
 	AH->currSchema = NULL;		/* ditto */
 	AH->currTablespace = NULL;	/* ditto */
-	AH->currTableAm = NULL;		/* ditto */
 
 	AH->toc = (TocEntry *) pg_malloc0(sizeof(TocEntry));
 
@@ -3356,9 +3355,6 @@ _reconnectToDB(ArchiveHandle *AH, const char *dbname)
 	free(AH->currSchema);
 	AH->currSchema = NULL;
 
-	free(AH->currTableAm);
-	AH->currTableAm = NULL;
-
 	free(AH->currTablespace);
 	AH->currTablespace = NULL;
 
@@ -3519,31 +3515,32 @@ _selectTablespace(ArchiveHandle *AH, const char *tablespace)
 }
 
 /*
- * Set the proper default_table_access_method value for the table.
+ * Set the proper default table access method for the table.
  */
 static void
-_selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
+_selectTableAccessMethod(ArchiveHandle *AH, TocEntry *te)
 {
 	RestoreOptions *ropt = AH->public.ropt;
+	const char *tableam = te->tableam;
 	PQExpBuffer cmd;
-	const char *want,
-			   *have;
 
 	/* do nothing in --no-table-access-method mode */
 	if (ropt->noTableAm)
 		return;
 
-	have = AH->currTableAm;
-	want = tableam;
-
-	if (!want)
-		return;
-
-	if (have && strcmp(want, have) == 0)
+	if (!tableam)
 		return;
 
 	cmd = createPQExpBuffer();
-	appendPQExpBuffer(cmd, "SET default_table_access_method = %s;", fmtId(want));
+
+	appendPQExpBufferStr(cmd, "ALTER ");
+	if (strcmp(te->desc, "MATERIALIZED VIEW") == 0)
+		appendPQExpBufferStr(cmd, "MATERIALIZED VIEW ");
+	else
+		appendPQExpBufferStr(cmd, "TABLE ");
+	appendPQExpBuffer(cmd, "%s ", fmtQualifiedId(te->namespace, te->tag));
+	appendPQExpBuffer(cmd, "SET ACCESS METHOD %s;",
+					  fmtId(tableam));
 
 	if (RestoringToDB(AH))
 	{
@@ -3553,7 +3550,7 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
 
 		if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
 			warn_or_exit_horribly(AH,
-								  "could not set default_table_access_method: %s",
+								  "could not set table access method: %s",
 								  PQerrorMessage(AH->connection));
 
 		PQclear(res);
@@ -3562,9 +3559,6 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
 		ahprintf(AH, "%s\n\n", cmd->data);
 
 	destroyPQExpBuffer(cmd);
-
-	free(AH->currTableAm);
-	AH->currTableAm = pg_strdup(want);
 }
 
 /*
@@ -3673,11 +3667,10 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
 {
 	RestoreOptions *ropt = AH->public.ropt;
 
-	/* Select owner, schema, tablespace and default AM as necessary */
+	/* Select owner, schema and tablespace as necessary */
 	_becomeOwner(AH, te);
 	_selectOutputSchema(AH, te->namespace);
 	_selectTablespace(AH, te->tablespace);
-	_selectTableAccessMethod(AH, te->tableam);
 
 	/* Emit header comment for item */
 	if (!AH->noTocComments)
@@ -3812,6 +3805,12 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
 		}
 	}
 
+	/*
+	 * Select the table's default AM, once the table definition has
+	 * been generated.
+	 */
+	_selectTableAccessMethod(AH, te);
+
 	/*
 	 * If it's an ACL entry, it might contain SET SESSION AUTHORIZATION
 	 * commands, so we can no longer assume we know the current auth setting.
@@ -4173,8 +4172,6 @@ restore_toc_entries_prefork(ArchiveHandle *AH, TocEntry *pending_list)
 	AH->currSchema = NULL;
 	free(AH->currTablespace);
 	AH->currTablespace = NULL;
-	free(AH->currTableAm);
-	AH->currTableAm = NULL;
 }
 
 /*
@@ -4910,7 +4907,6 @@ CloneArchive(ArchiveHandle *AH)
 	clone->connCancel = NULL;
 	clone->currUser = NULL;
 	clone->currSchema = NULL;
-	clone->currTableAm = NULL;
 	clone->currTablespace = NULL;
 
 	/* savedPassword must be local in case we change it while connecting */
@@ -4970,7 +4966,6 @@ DeCloneArchive(ArchiveHandle *AH)
 	free(AH->currUser);
 	free(AH->currSchema);
 	free(AH->currTablespace);
-	free(AH->currTableAm);
 	free(AH->savedPassword);
 
 	free(AH);
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index d6104a7196..d7409916d7 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -322,7 +322,6 @@ struct _archiveHandle
 	char	   *currUser;		/* current username, or NULL if unknown */
 	char	   *currSchema;		/* current schema, or NULL */
 	char	   *currTablespace; /* current tablespace, or NULL */
-	char	   *currTableAm;	/* current table access method, or NULL */
 
 	/* in --transaction-size mode, this counts objects emitted in cur xact */
 	int			txnCount;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 0c057fef94..c37054e7e7 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4539,11 +4539,8 @@ my %tests = (
 			CREATE TABLE dump_test.regress_pg_dump_table_am_1 (col1 int) USING regress_table_am;
 			CREATE TABLE dump_test.regress_pg_dump_table_am_2() USING heap;',
 		regexp => qr/^
-			\QSET default_table_access_method = regress_table_am;\E
-			(\n(?!SET[^;]+;)[^\n]*)*
-			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_1 (\E
-			\n\s+\Qcol1 integer\E
-			\n\);/xm,
+			\QALTER TABLE dump_test.regress_pg_dump_table_am_1 SET ACCESS METHOD regress_table_am;\E
+			\n/xm,
 		like => {
 			%full_runs, %dump_test_schema_runs, section_pre_data => 1,
 		},
@@ -4562,12 +4559,8 @@ my %tests = (
 				USING regress_table_am AS SELECT count(*) FROM pg_class;
 			CREATE MATERIALIZED VIEW dump_test.regress_pg_dump_matview_am_2 USING heap AS SELECT 1;',
 		regexp => qr/^
-			\QSET default_table_access_method = regress_table_am;\E
-			(\n(?!SET[^;]+;)[^\n]*)*
-			\QCREATE MATERIALIZED VIEW dump_test.regress_pg_dump_matview_am_1 AS\E
-			\n\s+\QSELECT count(*) AS count\E
-			\n\s+\QFROM pg_class\E
-			\n\s+\QWITH NO DATA;\E\n/xm,
+			\QALTER MATERIALIZED VIEW dump_test.regress_pg_dump_matview_am_1 SET ACCESS METHOD regress_table_am;\E
+			\n/xm,
 		like => {
 			%full_runs, %dump_test_schema_runs, section_pre_data => 1,
 		},
@@ -4591,18 +4584,12 @@ my %tests = (
 			CREATE TABLE dump_test.regress_pg_dump_table_am_child_2
 			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN (2);',
 		regexp => qr/^
-			\QSET default_table_access_method = regress_table_am;\E
-			(\n(?!SET[^;]+;)[^\n]*)*
-			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_parent (\E
+			\QALTER TABLE dump_test.regress_pg_dump_table_am_parent SET ACCESS METHOD regress_table_am;\E
 			(.*\n)*
-			\QSET default_table_access_method = heap;\E
-			(\n(?!SET[^;]+;)[^\n]*)*
-			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
+			\n\QALTER TABLE dump_test.regress_pg_dump_table_am_child_1 SET ACCESS METHOD heap;\E
 			(.*\n)*
-			\QSET default_table_access_method = regress_table_am;\E
-			(\n(?!SET[^;]+;)[^\n]*)*
-			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_2 (\E
-			(.*\n)*/xm,
+			\n\QALTER TABLE dump_test.regress_pg_dump_table_am_child_2 SET ACCESS METHOD regress_table_am;\E
+			\n/xm,
 		like => {
 			%full_runs, %dump_test_schema_runs, section_pre_data => 1,
 		},

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 17 Apr 2024 09:40:02 +0200

On 2024-Apr-17, Michael Paquier wrote:

> have their own extra ALTER TABLE commands.

Hmm, cannot we simply add a USING clause to the CREATE TABLE command for
partitioned tables?  That would override the
default_table_access_method, so it should give the correct result, no?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 17 Apr 2024 09:50:02 +0200

> default_table_access_method, so it should give the correct result, no?

Ah, upthread you noted that pg_restore-time --no-table-access-method
needs to be able to elide it, so a dump-time USING clause doesn't work.

I think it's easy enough to add a "bool ispartitioned" to TableInfo and
use an ALTER TABLE or rely on the GUC depending on that -- seems easy
enough.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/