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. > > 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 = ( >> 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 >> - (.*\n)* >> \QSET default_table_access_method = heap;\E >> + (.*\n)* >> + \QALTER TABLE dump_test.regress_pg_dump_table_am_parent >> SET ACCESS METHOD regress_table_am;\E >> (\n(?!SET[^;]+;)[^\n]*)* >> \n\QCREATE TABLE >> dump_test.regress_pg_dump_table_am_child_1 (\E >> (.*\n)* > > 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 > = regress_table_am line ... but it's not clear to me why we have the > "SET default_table_access_method = 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
From 009ba9bf376d87ab0aaf0a9e9df380cd801a9c2e Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@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_backup_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
+#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 *tableam);
+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, DumpId dumpId,
newToc->namespace = opts->namespace ? pg_strdup(opts->namespace) : NULL;
newToc->tablespace = opts->tablespace ? pg_strdup(opts->tablespace) : NULL;
newToc->tableam = opts->tableam ? pg_strdup(opts->tableam) : NULL;
+ newToc->relkind = opts->relkind;
newToc->owner = opts->owner ? pg_strdup(opts->owner) : NULL;
newToc->desc = pg_strdup(opts->description);
newToc->defn = 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");
@@ -2707,6 +2712,9 @@ ReadToc(ArchiveHandle *AH)
if (AH->version >= K_VERS_1_14)
te->tableam = ReadStr(AH);
+ if (AH->version >= K_VERS_1_16)
+ te->relkind = ReadInt(AH);
+
te->owner = ReadStr(AH);
is_supported = true;
if (AH->version < K_VERS_1_9)
@@ -3567,6 +3575,51 @@ _selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
AH->currTableAm = pg_strdup(want);
}
+/*
+ * 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 = AH->public.ropt;
+ const char *tableam = te->tableam;
+ PQExpBuffer cmd;
+
+ /* do nothing in --no-table-access-method mode */
+ if (ropt->noTableAm)
+ return;
+
+ if (!tableam)
+ return;
+
+ Assert(te->relkind == RELKIND_PARTITIONED_TABLE);
+
+ cmd = 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 = PQexec(AH->connection, cmd->data);
+
+ if (!res || PQresultStatus(res) != 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, bool isData)
{
RestoreOptions *ropt = AH->public.ropt;
- /* 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 != RELKIND_PARTITIONED_TABLE)
+ _selectTableAccessMethod(AH, te->tableam);
/* Emit header comment for item */
if (!AH->noTocComments)
@@ -3812,6 +3871,13 @@ _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData)
}
}
+ /*
+ * Select a partitioned table's default AM, once the table definition
+ * has been generated.
+ */
+ if (te->relkind == 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_backup_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 */
/* 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 *tbinfo)
.namespace = tbinfo->dobj.namespace->dobj.name,
.tablespace = tablespace,
.tableam = tableam,
+ .relkind = tbinfo->relkind,
.owner = tbinfo->rolname,
.description = reltypename,
.section = tbinfo->postponed_def ?
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.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 = (
CREATE TABLE dump_test.regress_pg_dump_table_am_parent (id int) PARTITION 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 => 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
+ (\n(?!SET[^;]+;)[^\n]*)*
+ \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
+ \QSET default_table_access_method = 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 = regress_table_am;\E
+ \QSET default_table_access_method = heap;\E
(\n(?!SET[^;]+;)[^\n]*)*
\n\QCREATE TABLE dump_test.regress_pg_dump_table_am_child_2 (\E
(.*\n)*/xm,
--
2.43.0
signature.asc
Description: PGP signature
