postgres.git / summary / log / commit / refs
commit 2fa989e6a3407b9da625e1524c8694bc028e25ba
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Jul 29 15:53:49 2024 +0000
First released in: 18.0, 17.0
Reduce number of commands dumpTableSchema emits for binary upgrade.
Avoid issuing a separate SQL UPDATE command for each column when
directly manipulating pg_attribute contents in binary upgrade mode.
With the separate updates, we triggered a relcache invalidation with
each update. For a table with N columns, that causes O(N^2) relcache
bloat in txn_size mode because the table's newly-created relcache
entry can't be flushed till end of transaction. Reducing the number
of commands should make it marginally faster as well as avoiding that
problem.
While at it, likewise avoid issuing a separate UPDATE on pg_constraint
for each inherited constraint. This is less exciting, first because
inherited (non-partitioned) constraints are relatively rare, and
second because the backend has a good deal of trouble anyway with
restoring tables containing many such constraints, due to
MergeConstraintsIntoExisting being horribly inefficient. But it seems
more consistent to do it this way here too, and it surely can't hurt.
In passing, fix one place in dumpTableSchema that failed to use ONLY
in ALTER TABLE. That's not a live bug, but it's inconsistent.
Also avoid silently casting away const from string literals.
Per report from Justin Pryzby. Back-patch to v17 where txn_size mode
was introduced.
Discussion: https://postgr.es/m/ZqEND4ZcTDBmcv31@pryzbyj2023
src/bin/pg_dump/pg_dump.c | 134 ++++++++++++++++++++++++++++-----------
src/bin/pg_dump/t/002_pg_dump.pl | 2 +-
2 files changed, 99 insertions(+), 37 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index e3240708284..9202a43eb20 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15764,6 +15764,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
DumpOptions *dopt = fout->dopt;
PQExpBuffer q = createPQExpBuffer();
PQExpBuffer delq = createPQExpBuffer();
+ PQExpBuffer extra = createPQExpBuffer();
char *qrelname;
char *qualrelname;
int numParents;
@@ -15830,7 +15831,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
char *partkeydef = NULL;
char *ftoptions = NULL;
char *srvname = NULL;
- char *foreign = "";
+ const char *foreign = "";
/*
* Set reltypename, and collect any relkind-specific data that we
@@ -16188,51 +16189,98 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
tbinfo->relkind == RELKIND_FOREIGN_TABLE ||
tbinfo->relkind == RELKIND_PARTITIONED_TABLE))
{
+ bool firstitem;
+
+ /*
+ * Drop any dropped columns. Merge the pg_attribute manipulations
+ * into a single SQL command, so that we don't cause repeated
+ * relcache flushes on the target table. Otherwise we risk O(N^2)
+ * relcache bloat while dropping N columns.
+ */
+ resetPQExpBuffer(extra);
+ firstitem = true;
for (j = 0; j < tbinfo->numatts; j++)
{
if (tbinfo->attisdropped[j])
{
- appendPQExpBufferStr(q, "\n-- For binary upgrade, recreate dropped column.\n");
- appendPQExpBuffer(q, "UPDATE pg_catalog.pg_attribute\n"
- "SET attlen = %d, "
- "attalign = '%c', attbyval = false\n"
- "WHERE attname = ",
+ if (firstitem)
+ {
+ appendPQExpBufferStr(q, "\n-- For binary upgrade, recreate dropped columns.\n"
+ "UPDATE pg_catalog.pg_attribute\n"
+ "SET attlen = v.dlen, "
+ "attalign = v.dalign, "
+ "attbyval = false\n"
+ "FROM (VALUES ");
+ firstitem = false;
+ }
+ else
+ appendPQExpBufferStr(q, ",\n ");
+ appendPQExpBufferChar(q, '(');
+ appendStringLiteralAH(q, tbinfo->attnames[j], fout);
+ appendPQExpBuffer(q, ", %d, '%c')",
tbinfo->attlen[j],
tbinfo->attalign[j]);
- appendStringLiteralAH(q, tbinfo->attnames[j], fout);
- appendPQExpBufferStr(q, "\n AND attrelid = ");
- appendStringLiteralAH(q, qualrelname, fout);
- appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
-
- if (tbinfo->relkind == RELKIND_RELATION ||
- tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
- appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
- qualrelname);
- else
- appendPQExpBuffer(q, "ALTER FOREIGN TABLE ONLY %s ",
- qualrelname);
- appendPQExpBuffer(q, "DROP COLUMN %s;\n",
+ /* The ALTER ... DROP COLUMN commands must come after */
+ appendPQExpBuffer(extra, "ALTER %sTABLE ONLY %s ",
+ foreign, qualrelname);
+ appendPQExpBuffer(extra, "DROP COLUMN %s;\n",
fmtId(tbinfo->attnames[j]));
}
- else if (!tbinfo->attislocal[j])
+ }
+ if (!firstitem)
+ {
+ appendPQExpBufferStr(q, ") v(dname, dlen, dalign)\n"
+ "WHERE attrelid = ");
+ appendStringLiteralAH(q, qualrelname, fout);
+ appendPQExpBufferStr(q, "::pg_catalog.regclass\n"
+ " AND attname = v.dname;\n");
+ /* Now we can issue the actual DROP COLUMN commands */
+ appendBinaryPQExpBuffer(q, extra->data, extra->len);
+ }
+
+ /*
+ * Fix up inherited columns. As above, do the pg_attribute
+ * manipulations in a single SQL command.
+ */
+ firstitem = true;
+ for (j = 0; j < tbinfo->numatts; j++)
+ {
+ if (!tbinfo->attisdropped[j] &&
+ !tbinfo->attislocal[j])
{
- appendPQExpBufferStr(q, "\n-- For binary upgrade, recreate inherited column.\n");
- appendPQExpBufferStr(q, "UPDATE pg_catalog.pg_attribute\n"
- "SET attislocal = false\n"
- "WHERE attname = ");
+ if (firstitem)
+ {
+ appendPQExpBufferStr(q, "\n-- For binary upgrade, recreate inherited columns.\n");
+ appendPQExpBufferStr(q, "UPDATE pg_catalog.pg_attribute\n"
+ "SET attislocal = false\n"
+ "WHERE attrelid = ");
+ appendStringLiteralAH(q, qualrelname, fout);
+ appendPQExpBufferStr(q, "::pg_catalog.regclass\n"
+ " AND attname IN (");
+ firstitem = false;
+ }
+ else
+ appendPQExpBufferStr(q, ", ");
appendStringLiteralAH(q, tbinfo->attnames[j], fout);
- appendPQExpBufferStr(q, "\n AND attrelid = ");
- appendStringLiteralAH(q, qualrelname, fout);
- appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
}
}
+ if (!firstitem)
+ appendPQExpBufferStr(q, ");\n");
/*
* Add inherited CHECK constraints, if any.
*
* For partitions, they were already dumped, and conislocal
* doesn't need fixing.
+ *
+ * As above, issue only one direct manipulation of pg_constraint.
+ * Although it is tempting to merge the ALTER ADD CONSTRAINT
+ * commands into one as well, refrain for now due to concern about
+ * possible backend memory bloat if there are many such
+ * constraints.
*/
+ resetPQExpBuffer(extra);
+ firstitem = true;
for (k = 0; k < tbinfo->ncheck; k++)
{
ConstraintInfo *constr = &(tbinfo->checkexprs[k]);
@@ -16240,18 +16288,31 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
if (constr->separate || constr->conislocal || tbinfo->ispartition)
continue;
- appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inherited constraint.\n");
+ if (firstitem)
+ appendPQExpBufferStr(q, "\n-- For binary upgrade, set up inherited constraints.\n");
appendPQExpBuffer(q, "ALTER %sTABLE ONLY %s ADD CONSTRAINT %s %s;\n",
foreign, qualrelname,
fmtId(constr->dobj.name),
constr->condef);
- appendPQExpBufferStr(q, "UPDATE pg_catalog.pg_constraint\n"
- "SET conislocal = false\n"
- "WHERE contype = 'c' AND conname = ");
- appendStringLiteralAH(q, constr->dobj.name, fout);
- appendPQExpBufferStr(q, "\n AND conrelid = ");
- appendStringLiteralAH(q, qualrelname, fout);
- appendPQExpBufferStr(q, "::pg_catalog.regclass;\n");
+ /* Update pg_constraint after all the ALTER TABLEs */
+ if (firstitem)
+ {
+ appendPQExpBufferStr(extra, "UPDATE pg_catalog.pg_constraint\n"
+ "SET conislocal = false\n"
+ "WHERE contype = 'c' AND conrelid = ");
+ appendStringLiteralAH(extra, qualrelname, fout);
+ appendPQExpBufferStr(extra, "::pg_catalog.regclass\n");
+ appendPQExpBufferStr(extra, " AND conname IN (");
+ firstitem = false;
+ }
+ else
+ appendPQExpBufferStr(extra, ", ");
+ appendStringLiteralAH(extra, constr->dobj.name, fout);
+ }
+ if (!firstitem)
+ {
+ appendPQExpBufferStr(extra, ");\n");
+ appendBinaryPQExpBuffer(q, extra->data, extra->len);
}
if (numParents > 0 && !tbinfo->ispartition)
@@ -16438,7 +16499,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
if (tbinfo->relkind == RELKIND_FOREIGN_TABLE &&
tbinfo->attfdwoptions[j][0] != '\0')
appendPQExpBuffer(q,
- "ALTER FOREIGN TABLE %s ALTER COLUMN %s OPTIONS (\n"
+ "ALTER FOREIGN TABLE ONLY %s ALTER COLUMN %s OPTIONS (\n"
" %s\n"
");\n",
qualrelname,
@@ -16539,6 +16600,7 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo)
destroyPQExpBuffer(q);
destroyPQExpBuffer(delq);
+ destroyPQExpBuffer(extra);
free(qrelname);
free(qualrelname);
}
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index d3dd8784d64..5bcc2244d58 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -1154,7 +1154,7 @@ my %tests = (
'ALTER FOREIGN TABLE foreign_table ALTER COLUMN c1 OPTIONS' => {
regexp => qr/^
- \QALTER FOREIGN TABLE dump_test.foreign_table ALTER COLUMN c1 OPTIONS (\E\n
+ \QALTER FOREIGN TABLE ONLY dump_test.foreign_table ALTER COLUMN c1 OPTIONS (\E\n
\s+\Qcolumn_name 'col1'\E\n
\Q);\E\n
/xm,
[parent: 1e020258e53c]