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
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,
},
signature.asc
Description: PGP signature
