Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 17 Apr 2024 14:31:47 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	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
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Message-ID: <Zh9ew9wnwDsGjl0z@paquier.xyz>
References: <202403281558.xk5pgf7ur3dq@alvherre.pgsql>
 <f3e61e27-19d0-5e40-3eb2-53282fa0532a@gmail.com>
 <ZglExpzL9w0MtBWf@pryzbyj2023>
 <1464385.1712034366@sss.pgh.pa.us>
 <ZhyG2JS2z6I6Kf7a@paquier.xyz>
 <Zh4JLSvvtQgBJZkZ@paquier.xyz>
 <Zh4KfLWnNDqyYzJa@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="in76JH08KGJysgX5"
Content-Disposition: inline
In-Reply-To: <Zh4KfLWnNDqyYzJa@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Zh9ew9wnwDsGjl0z%40paquier.xyz>
Precedence: bulk


--in76JH08KGJysgX5
Content-Type: multipart/mixed; boundary="2qpX4gu7APQEhqFs"
Content-Disposition: inline


--2qpX4gu7APQEhqFs
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 16, 2024 at 02:19:56PM +0900, Michael Paquier wrote:
> 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.

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

--2qpX4gu7APQEhqFs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="dump-partitions.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_back=
up_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 *us=
er);
 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 *tablea=
m);
+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 fm=
t,
 	AH->currUser =3D NULL;		/* unknown */
 	AH->currSchema =3D NULL;		/* ditto */
 	AH->currTablespace =3D NULL;	/* ditto */
-	AH->currTableAm =3D NULL;		/* ditto */
=20
 	AH->toc =3D (TocEntry *) pg_malloc0(sizeof(TocEntry));
=20
@@ -3356,9 +3355,6 @@ _reconnectToDB(ArchiveHandle *AH, const char *dbname)
 	free(AH->currSchema);
 	AH->currSchema =3D NULL;
=20
-	free(AH->currTableAm);
-	AH->currTableAm =3D NULL;
-
 	free(AH->currTablespace);
 	AH->currTablespace =3D NULL;
=20
@@ -3519,31 +3515,32 @@ _selectTablespace(ArchiveHandle *AH, const char *ta=
blespace)
 }
=20
 /*
- * 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 =3D AH->public.ropt;
+	const char *tableam =3D te->tableam;
 	PQExpBuffer cmd;
-	const char *want,
-			   *have;
=20
 	/* do nothing in --no-table-access-method mode */
 	if (ropt->noTableAm)
 		return;
=20
-	have =3D AH->currTableAm;
-	want =3D tableam;
-
-	if (!want)
-		return;
-
-	if (have && strcmp(want, have) =3D=3D 0)
+	if (!tableam)
 		return;
=20
 	cmd =3D createPQExpBuffer();
-	appendPQExpBuffer(cmd, "SET default_table_access_method =3D %s;", fmtId(w=
ant));
+
+	appendPQExpBufferStr(cmd, "ALTER ");
+	if (strcmp(te->desc, "MATERIALIZED VIEW") =3D=3D 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));
=20
 	if (RestoringToDB(AH))
 	{
@@ -3553,7 +3550,7 @@ _selectTableAccessMethod(ArchiveHandle *AH, const cha=
r *tableam)
=20
 		if (!res || PQresultStatus(res) !=3D 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));
=20
 		PQclear(res);
@@ -3562,9 +3559,6 @@ _selectTableAccessMethod(ArchiveHandle *AH, const cha=
r *tableam)
 		ahprintf(AH, "%s\n\n", cmd->data);
=20
 	destroyPQExpBuffer(cmd);
-
-	free(AH->currTableAm);
-	AH->currTableAm =3D pg_strdup(want);
 }
=20
 /*
@@ -3673,11 +3667,10 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, boo=
l isData)
 {
 	RestoreOptions *ropt =3D AH->public.ropt;
=20
-	/* 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);
=20
 	/* Emit header comment for item */
 	if (!AH->noTocComments)
@@ -3812,6 +3805,12 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool=
 isData)
 		}
 	}
=20
+	/*
+	 * 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, TocEnt=
ry *pending_list)
 	AH->currSchema =3D NULL;
 	free(AH->currTablespace);
 	AH->currTablespace =3D NULL;
-	free(AH->currTableAm);
-	AH->currTableAm =3D NULL;
 }
=20
 /*
@@ -4910,7 +4907,6 @@ CloneArchive(ArchiveHandle *AH)
 	clone->connCancel =3D NULL;
 	clone->currUser =3D NULL;
 	clone->currSchema =3D NULL;
-	clone->currTableAm =3D NULL;
 	clone->currTablespace =3D NULL;
=20
 	/* 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);
=20
 	free(AH);
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_back=
up_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 */
=20
 	/* 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_du=
mp.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 =3D (
 			CREATE TABLE dump_test.regress_pg_dump_table_am_1 (col1 int) USING regr=
ess_table_am;
 			CREATE TABLE dump_test.regress_pg_dump_table_am_2() USING heap;',
 		regexp =3D> qr/^
-			\QSET default_table_access_method =3D 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 re=
gress_table_am;\E
+			\n/xm,
 		like =3D> {
 			%full_runs, %dump_test_schema_runs, section_pre_data =3D> 1,
 		},
@@ -4562,12 +4559,8 @@ my %tests =3D (
 				USING regress_table_am AS SELECT count(*) FROM pg_class;
 			CREATE MATERIALIZED VIEW dump_test.regress_pg_dump_matview_am_2 USING h=
eap AS SELECT 1;',
 		regexp =3D> qr/^
-			\QSET default_table_access_method =3D 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 AC=
CESS METHOD regress_table_am;\E
+			\n/xm,
 		like =3D> {
 			%full_runs, %dump_test_schema_runs, section_pre_data =3D> 1,
 		},
@@ -4591,18 +4584,12 @@ my %tests =3D (
 			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 =3D> qr/^
-			\QSET default_table_access_method =3D 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 METH=
OD regress_table_am;\E
 			(.*\n)*
-			\QSET default_table_access_method =3D 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 M=
ETHOD heap;\E
 			(.*\n)*
-			\QSET default_table_access_method =3D 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 M=
ETHOD regress_table_am;\E
+			\n/xm,
 		like =3D> {
 			%full_runs, %dump_test_schema_runs, section_pre_data =3D> 1,
 		},

--2qpX4gu7APQEhqFs--

--in76JH08KGJysgX5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmYfXsMACgkQnvQgOdby
QH2FCg//ZVforNAJBvO2zQgSXtSHQF2H3FqNRVtwL3Fd71r+un6sgqsPbxiR7IUf
g0ZqjBB1KDbdQ16QONcNfLdLGbmdxPvr+7CJD+clKbUAsL/rdLepYRojXCbtUH+8
wvmv9X5+HbfyRhKdHNLhg5zacdH+OT+2Jb6tuM9eaUpsRdUqhOE/zT7SkmzIVKll
PAj304ZHtH8cMwE+1n9vDCjA5uNWbEZzBl3T44nW0B28ZWlcuFHauTiJB0kITRql
23XgOgPwLaUd8xeo180pOVuUewDCM9dx8yDRGtYtQiRjMQgLDqjCPXOx6rg6EboK
ya8uj0ue1DF6tc0DNnWdo6ktqrTXhfN2iOJ58uiUpjy45KEaUiKXlaG6QGlxMJDa
lPjfBhebWdGDF36p9gLI2VvaEvlbZmJy6Spop6QPnCqutFtWPVpA+D/3iTNan5Vz
/PzEzzidS9gtLN6VWiRYjx/E8CnBpIwB12rOA1dKGD3J54OAjv/BoEuUruskRj7D
mnO9CNMgUiiJHckcNz8vudx1a9KaUSQK574vuusN3ZJzRI4EJqDt+/bF3dzLd7gX
FfCmm9buF9s0wv5MWMe1yKJ3pD9m3Sbk3WIA4s1bTGC31Gick1rktOJvIj+1rSJj
MNYt8Zhuyp0+i5XTseMWoFoQLHBxjGtgpF9RRThK6ObF0G8QSWY=
=YgO0
-----END PGP SIGNATURE-----

--in76JH08KGJysgX5--