Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 18 Apr 2024 09:42:08 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Justin Pryzby <pryzby@telsasoft.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	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: <ZiBsYJr9yC7rySHV@paquier.xyz>
References: <Zh-Ejsvm9YYZtJtS@paquier.xyz>
 <202404170831.imev4pwiztdo@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="VbQ+z/LAoipgu3Ey"
Content-Disposition: inline
In-Reply-To: <202404170831.imev4pwiztdo@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/ZiBsYJr9yC7rySHV%40paquier.xyz>
Precedence: bulk


--VbQ+z/LAoipgu3Ey
Content-Type: multipart/mixed; boundary="Ts57s6qIfZgEFZIK"
Content-Disposition: inline


--Ts57s6qIfZgEFZIK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 17, 2024 at 10:31:52AM +0200, Alvaro Herrera wrote:
> On 2024-Apr-17, Michael Paquier wrote:
>> Yeah, that would be easy enough to track but I was wondering about
>> adding the relkind instead.  Still, one thing that I found confusing
>> is the dump generated in this case, as it would mix the SET and the
>> ALTER TABLE commands so one reading the dumps may wonder why the SET
>> has no effect for a CREATE TABLE PARTITION OF without USING.  Perhaps
>> that's fine and I just worry too much ;)
>=20
> Hmm, maybe we should do a RESET of default_table_access_method before
> printing the CREATE TABLE to avoid the confusion.

A hard reset would make the business around currTableAM that decides
when to generate the SET default_table_access_method queries slightly
more complicated, while increasing the number of queries run on the
server.

>> The extra ALTER commands need to be generated after the object
>> definitions, so we'd need a new subroutine similar to
>> _selectTableAccessMethod() like a _selectTableAccessMethodNoStorage().
>> Or grouping both together is just simpler?
>=20
> I think there should be two routines, since _select* routines just print
> a SET command; maybe the new one would be _printAlterTableAM() or
> something like that.  Having _select() print an ALTER TABLE command
> depending on relkind (or the boolean flag) would be confusing, I think.

Fine by me to use two routines to generate the two different commands.
I am finishing with the attached for now, making dumps, restores and
upgrades work happily as far as I've tested.

I was also worrying about a need to dump the protocol version to be
able to track the relkind in the toc entries, but a45c78e3284b has
already done one.  The difference in AM handling between relations
without storage and relations with storage pushes the relkind logic
more within the internals of pg_backup_archiver.c.

What do you think?
--
Michael

--Ts57s6qIfZgEFZIK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Set-properly-table-AMs-of-partitioned-tables-in-p.patch"
Content-Transfer-Encoding: quoted-printable

=46rom ca3d0c1828336275d86a5c51ed975c0a43ca6af9 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 18 Apr 2024 09:37:20 +0900
Subject: [PATCH v2] Set properly table AMs of partitioned tables in pg_dump

---
 src/bin/pg_dump/pg_backup_archiver.c | 70 +++++++++++++++++++++++++++-
 src/bin/pg_dump/pg_backup_archiver.h |  5 +-
 src/bin/pg_dump/pg_dump.c            |  1 +
 src/bin/pg_dump/t/002_pg_dump.pl     |  6 +--
 4 files changed, 75 insertions(+), 7 deletions(-)

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_back=
up_archiver.c
index c7a6c918a6..f69f72d731 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -30,6 +30,7 @@
 #include <io.h>
 #endif
=20
+#include "catalog/pg_class_d.h"
 #include "common/string.h"
 #include "compress_io.h"
 #include "dumputils.h"
@@ -62,6 +63,8 @@ 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 _printTableAccessMethodNoStorage(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);
@@ -1222,6 +1225,7 @@ ArchiveEntry(Archive *AHX, CatalogId catalogId, DumpI=
d dumpId,
 	newToc->namespace =3D opts->namespace ? pg_strdup(opts->namespace) : NULL;
 	newToc->tablespace =3D opts->tablespace ? pg_strdup(opts->tablespace) : N=
ULL;
 	newToc->tableam =3D opts->tableam ? pg_strdup(opts->tableam) : NULL;
+	newToc->relkind =3D opts->relkind;
 	newToc->owner =3D opts->owner ? pg_strdup(opts->owner) : NULL;
 	newToc->desc =3D pg_strdup(opts->description);
 	newToc->defn =3D opts->createStmt ? pg_strdup(opts->createStmt) : NULL;
@@ -2602,6 +2606,7 @@ WriteToc(ArchiveHandle *AH)
 		WriteStr(AH, te->namespace);
 		WriteStr(AH, te->tablespace);
 		WriteStr(AH, te->tableam);
+		WriteInt(AH, te->relkind);
 		WriteStr(AH, te->owner);
 		WriteStr(AH, "false");
=20
@@ -2707,6 +2712,9 @@ ReadToc(ArchiveHandle *AH)
 		if (AH->version >=3D K_VERS_1_14)
 			te->tableam =3D ReadStr(AH);
=20
+		if (AH->version >=3D K_VERS_1_16)
+			te->relkind =3D ReadInt(AH);
+
 		te->owner =3D ReadStr(AH);
 		is_supported =3D true;
 		if (AH->version < K_VERS_1_9)
@@ -3567,6 +3575,51 @@ _selectTableAccessMethod(ArchiveHandle *AH, const ch=
ar *tableam)
 	AH->currTableAm =3D pg_strdup(want);
 }
=20
+/*
+ * Set the proper default table access method for a table without storage,
+ * like a partitioned one where a table access method may be set.
+ */
+static void
+_printTableAccessMethodNoStorage(ArchiveHandle *AH, TocEntry *te)
+{
+	RestoreOptions *ropt =3D AH->public.ropt;
+	const char *tableam =3D te->tableam;
+	PQExpBuffer cmd;
+
+	/* do nothing in --no-table-access-method mode */
+	if (ropt->noTableAm)
+		return;
+
+	if (!tableam)
+		return;
+
+	Assert(te->relkind =3D=3D RELKIND_PARTITIONED_TABLE);
+
+	cmd =3D createPQExpBuffer();
+
+	appendPQExpBufferStr(cmd, "ALTER TABLE ");
+	appendPQExpBuffer(cmd, "%s ", fmtQualifiedId(te->namespace, te->tag));
+	appendPQExpBuffer(cmd, "SET ACCESS METHOD %s;",
+					  fmtId(tableam));
+
+	if (RestoringToDB(AH))
+	{
+		PGresult   *res;
+
+		res =3D PQexec(AH->connection, cmd->data);
+
+		if (!res || PQresultStatus(res) !=3D PGRES_COMMAND_OK)
+			warn_or_exit_horribly(AH,
+								  "could not alter table access method: %s",
+								  PQerrorMessage(AH->connection));
+		PQclear(res);
+	}
+	else
+		ahprintf(AH, "%s\n\n", cmd->data);
+
+	destroyPQExpBuffer(cmd);
+}
+
 /*
  * Extract an object description for a TOC entry, and append it to buf.
  *
@@ -3673,11 +3726,17 @@ _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, tablespace and default AM as necessary.
+	 * The default access method for partitioned tables is handled
+	 * after generating the object definition, as it requires an ALTER
+	 * command rather than SET.
+	 */
 	_becomeOwner(AH, te);
 	_selectOutputSchema(AH, te->namespace);
 	_selectTablespace(AH, te->tablespace);
-	_selectTableAccessMethod(AH, te->tableam);
+	if (te->relkind !=3D RELKIND_PARTITIONED_TABLE)
+		_selectTableAccessMethod(AH, te->tableam);
=20
 	/* Emit header comment for item */
 	if (!AH->noTocComments)
@@ -3812,6 +3871,13 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool=
 isData)
 		}
 	}
=20
+	/*
+	 * Select a partitioned table's default AM, once the table definition
+	 * has been generated.
+	 */
+	if (te->relkind =3D=3D RELKIND_PARTITIONED_TABLE)
+		_printTableAccessMethodNoStorage(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.
diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_back=
up_archiver.h
index d6104a7196..ce5ed1dd39 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -69,7 +69,8 @@
 													 * compression_algorithm
 													 * in header */
 #define K_VERS_1_16 MAKE_ARCHIVE_VERSION(1, 16, 0)	/* BLOB METADATA entries
-													 * and multiple BLOBS */
+													 * and multiple BLOBS,
+													 * relkind */
=20
 /* Current archive version number (the format we can output) */
 #define K_VERS_MAJOR 1
@@ -353,6 +354,7 @@ struct _tocEntry
 	char	   *tablespace;		/* null if not in a tablespace; empty string
 								 * means use database default */
 	char	   *tableam;		/* table access method, only for TABLE tags */
+	char		relkind;		/* relation kind, only for TABLE tags */
 	char	   *owner;
 	char	   *desc;
 	char	   *defn;
@@ -393,6 +395,7 @@ typedef struct _archiveOpts
 	const char *namespace;
 	const char *tablespace;
 	const char *tableam;
+	char		relkind;
 	const char *owner;
 	const char *description;
 	teSection	section;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c52e961b30..6e167dda03 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16741,6 +16741,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbi=
nfo)
 								  .namespace =3D tbinfo->dobj.namespace->dobj.name,
 								  .tablespace =3D tablespace,
 								  .tableam =3D tableam,
+								  .relkind =3D tbinfo->relkind,
 								  .owner =3D tbinfo->rolname,
 								  .description =3D reltypename,
 								  .section =3D tbinfo->postponed_def ?
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..25b4082273 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4591,11 +4591,9 @@ 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
-			(.*\n)*
 			\QSET default_table_access_method =3D heap;\E
+			(.*\n)*
+			\QALTER TABLE dump_test.regress_pg_dump_table_am_parent SET ACCESS METH=
OD regress_table_am;\E
 			(\n(?!SET[^;]+;)[^\n]*)*
 			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
 			(.*\n)*
--=20
2.43.0


--Ts57s6qIfZgEFZIK--

--VbQ+z/LAoipgu3Ey
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmYgbGAACgkQnvQgOdby
QH3hOQ/9HRmGIN/e0DvZcRJUck02dyaBr2jLiYmMCViyDw//V3qQhaP5+mxbCXYR
Or1TUpt7RBM7LtC9LQkp5Wxh0PRF30A1qL2aPQTtkckRWiDtHtJEvWzx4MIoVxcH
H9es0ODO2s20X5p9HiX7IsrVUY8EwjXQjws2VoaS17qByIjdhqExWBOsPCZZmiir
8PUgbStJreA7KEe4jLnNnnYB0F5P9oeamCSapkSLp87CDsRbGlRX2BZBZGUecw3i
6xNY4/Z0mg7zpD/uL5kEUQ/gG58bKJMu+T0Un/QIl/xrmF7gmJ0pDhRjgCYD9ODy
7gntnZj5PWqXJMqgPLSFY1fgLqlynPTZH0hGKn2QJG8VLYQxQFX0R0HAAbvP6Jxf
LiPk1P9xr+mUa6Y/6y4K/iwGF2VKFPnv+rmMscy5vYoclu8+t7OTMaDAxknGrrr7
nIem7UDZTOlhblAPhejcNem60yrpZexb4JMf2hbNuZfgC3/xISG0TL21CcOY+vNP
plx0FGUYsHoDgpFi4qeTMmjWUJMN+RZFCz++utTtn8yWzpwl804DTSZyZYVERWVO
QqyrJmAHvuQwikV93NcPNjZ0mcQ4X2gb56XU3t5l+LmPK51LmPCJYnaJvdUJwS/b
hl9ZHfHGbwOrH4TYDefMJBug+Hn3XDXorFaguPLHIi7l67njzNU=
=BOfj
-----END PGP SIGNATURE-----

--VbQ+z/LAoipgu3Ey--