Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 19 Apr 2024 10:41:30 +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: <ZiHLyg2RdObSTyn_@paquier.xyz>
References: <ZiBsYJr9yC7rySHV@paquier.xyz>
 <202404181617.fkow5ghqjcqs@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="UvPVjmdo/ZaC2jPH"
Content-Disposition: inline
In-Reply-To: <202404181617.fkow5ghqjcqs@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/ZiHLyg2RdObSTyn_%40paquier.xyz>
Precedence: bulk


--UvPVjmdo/ZaC2jPH
Content-Type: multipart/mixed; boundary="JT1JuCeO83rd7Kvp"
Content-Disposition: inline


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

On Thu, Apr 18, 2024 at 06:17:56PM +0200, Alvaro Herrera wrote:
> On 2024-Apr-18, Michael Paquier wrote:
>> 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.
>=20
> Hmm, does this mean that every dump taking since a45c78e3284b (April
> 1st) and before this commit will be unrestorable?  This doesn't worry me
> too much, because we aren't even in beta yet ... and I think we don't
> have a strict policy about it.

I've been scanning the history of K_VERS_1_* in the recent years, and
it does not seem that we have a case where we would have needed to
bump the version twice in the same release cycle.  Anyway, yes, any
dump taken since 1_16 has been bumped would fail to restore with this
patch in place.  For an unreleased not-yet-in-beta branch, why should
we care?  Things are not set in stone, like extensions.  If others
have comments about this point, feel free of course.

>> --- 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 M=
ETHOD regress_table_am;\E
>>  			(\n(?!SET[^;]+;)[^\n]*)*
>>  			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
>>  			(.*\n)*
>=20
> This looks strange -- why did you remove matching for the CREATE TABLE
> of the parent table?  That line should appear shortly before the ALTER
> TABLE SET ACCESS METHOD for the same table, shouldn't it?

Yeah, with the ALTER in place that did not seem that mandatory but I
don't mind keeping it, as well.

> Maybe your
> intention was to remove only the SET default_table_access_method
> =3D regress_table_am line ... but it's not clear to me why we have the
> "SET default_table_access_method =3D heap" line before the ALTER TABLE SET
> ACCESS METHOD.

This comes from the contents of the dump for
regress_pg_dump_table_am_2, that uses heap as table AM.  A SET is
issued for it before dumping regress_pg_dump_table_am_parent and its
partitions.  One trick that I can think of to make the output parsing
of the test more palatable is to switch the AMs used by the two
partitions, so as we finish with two SET queries before each partition
rather than one before the partitioned table.  See the attached for
the idea.
--
Michael

--JT1JuCeO83rd7Kvp
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 009ba9bf376d87ab0aaf0a9e9df380cd801a9c2e Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 19 Apr 2024 10:30:53 +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     | 12 ++---
 4 files changed, 79 insertions(+), 9 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 6d2f3fdef3..26e45e994f 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16744,6 +16744,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..7085053a2d 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -4587,19 +4587,19 @@ my %tests =3D (
 			CREATE TABLE dump_test.regress_pg_dump_table_am_parent (id int) PARTITI=
ON BY LIST (id);
 			ALTER TABLE dump_test.regress_pg_dump_table_am_parent SET ACCESS METHOD=
 regress_table_am;
 			CREATE TABLE dump_test.regress_pg_dump_table_am_child_1
-			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN =
(1) USING heap;
+			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN =
(1);
 			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);',
+			  PARTITION OF dump_test.regress_pg_dump_table_am_parent FOR VALUES IN =
(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_parent (\E
+			(\n(?!SET[^;]+;)[^\n]*)*
+			\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
+			\QSET default_table_access_method =3D regress_table_am;\E
 			(\n(?!SET[^;]+;)[^\n]*)*
 			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_1 (\E
 			(.*\n)*
-			\QSET default_table_access_method =3D regress_table_am;\E
+			\QSET default_table_access_method =3D heap;\E
 			(\n(?!SET[^;]+;)[^\n]*)*
 			\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_2 (\E
 			(.*\n)*/xm,
--=20
2.43.0


--JT1JuCeO83rd7Kvp--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmYhy8oACgkQnvQgOdby
QH1Hbw//V1SNdLh4dklQElX+vThc6WXiC7uk0MuikIieGBMHu/buCslzHlboXmsZ
Ty2p5obnZqp0frLkmsLVVWWXiBAX6xStC7TZ9P/X2lpFGaNh4eKNRlhdHN5Dr1lO
8S6sWeHh9DYt3CbIvljScuD4pMcrOvjuWHkQlLaJPm4qQvj66LFM2G/NtC5RUwXo
DyYYfP702GrjT7kvKb23+SmNnLleSkSHQvgPW1KsFkWKgyRt9AGvG8CzwNYBXaAu
m+dSTGgdipYU96/oUxyb3ylWl5LpuMdhjexQfJ8DOnpCBqnk87OCWkqLrAvvsbXE
QN5R4V+7ixyHOteXwUnbprVf52Ej/gfzEkgl1N0HamXOb6DnQJftpBQlp/kXVNP/
W82BgsgIoUjZn5zzuxC4tVdZy3Qj/toPVwPmph1v4B3nqe/CsbpN3t2rnn+z9pyE
J0a8ojCdC4koZrCX1UNtk5YWFAfTeh7SIMyskjy4paj90b24N3ZqSTB2QNq9+VoB
fmgKh27RFJUc9glbtDHZm/qGzY2DyOrc8qob3HvQsZWUsBXvPHEA+KAoaLanRjcu
QEuihGFwW2Bu5aXywE8F92/XTP+4gUxVaD6GDopTT29BrTeqr+vSB1I5e2RmP8VP
rY+6mKjFM2r8H/IhkoTNuDGWlWtbPNK/6sEYFUI9OPuxnnXVv5A=
=GXBZ
-----END PGP SIGNATURE-----

--UvPVjmdo/ZaC2jPH--