Re: pg_upgrade failing for 200+ million Large Objects - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: pg_upgrade failing for 200+ million Large Objects |
Date | |
Msg-id | 1870579.1722033430@sss.pgh.pa.us Whole thread Raw |
In response to | Re: pg_upgrade failing for 200+ million Large Objects (Justin Pryzby <pryzby@telsasoft.com>) |
Responses |
Re: pg_upgrade failing for 200+ million Large Objects
|
List | pgsql-hackers |
Justin Pryzby <pryzby@telsasoft.com> writes: > On Fri, Jul 26, 2024 at 10:53:30PM +0300, Alexander Korotkov wrote: >> It would be nice to identify such cases and check which memory contexts are >> growing and why. > I reproduced the problem with this schema: > SELECT format('CREATE TABLE p(i int, %s) PARTITION BY RANGE(i)', array_to_string(a, ', ')) FROM (SELECT array_agg(format('i%sint', i))a FROM generate_series(1,999)i); > SELECT format('CREATE TABLE t%s PARTITION OF p FOR VALUES FROM (%s)TO(%s)', i,i,i+1) FROM generate_series(1,999)i; > This used over 4 GB of RAM. Interesting. This doesn't bloat particularly much in a regular pg_restore, even with --transaction-size=1000; but it does in pg_upgrade, as you say. I found that the bloat was occurring during these long sequences of UPDATE commands issued by pg_upgrade: -- For binary upgrade, recreate inherited column. UPDATE pg_catalog.pg_attribute SET attislocal = false WHERE attname = 'i' AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass; -- For binary upgrade, recreate inherited column. UPDATE pg_catalog.pg_attribute SET attislocal = false WHERE attname = 'i1' AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass; -- For binary upgrade, recreate inherited column. UPDATE pg_catalog.pg_attribute SET attislocal = false WHERE attname = 'i2' AND attrelid = '\"public\".\"t139\"'::pg_catalog.regclass; I think the problem is basically that each one of these commands causes a relcache inval, for which we can't reclaim space right away, so that we end up consuming O(N^2) cache space for an N-column inherited table. It's fairly easy to fix things so that this example doesn't cause that to happen: we just need to issue these updates as one command not N commands per table. See attached. However, I fear this should just be considered a draft, because the other code for binary upgrade in the immediate vicinity is just as aggressively stupid and unoptimized as this bit, and can probably also be driven to O(N^2) behavior with enough CHECK constraints etc. We've gone out of our way to make ALTER TABLE capable of handling many updates to a table's DDL in one command, but whoever wrote this code appears not to have read that memo, or at least to have believed that performance of pg_upgrade isn't of concern. > Note that there seemed to be no issue when I created 999 tables without > partitioning: > SELECT format('CREATE TABLE t%s(LIKE p)', i,i,i+1) FROM generate_series(1,999)i; Yeah, because then we don't need to play games with attislocal. regards, tom lane diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index b8b1888bd3..19f98bdf43 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -16094,6 +16094,12 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) tbinfo->relkind == RELKIND_FOREIGN_TABLE || tbinfo->relkind == RELKIND_PARTITIONED_TABLE)) { + bool firstinhcol = true; + + /* + * Drop any dropped columns. We don't really expect there to be a + * lot, else this code would be pretty inefficient. + */ for (j = 0; j < tbinfo->numatts; j++) { if (tbinfo->attisdropped[j]) @@ -16120,18 +16126,37 @@ dumpTableSchema(Archive *fout, const TableInfo *tbinfo) appendPQExpBuffer(q, "DROP COLUMN %s;\n", fmtId(tbinfo->attnames[j])); } - else if (!tbinfo->attislocal[j]) + } + + /* + * Fix up inherited columns. There could be a lot of these, so we + * do the operation in a single SQL command; otherwise, we risk + * O(N^2) relcache bloat thanks to repeatedly invalidating the + * table's relcache entry. + */ + 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 (firstinhcol) + { + 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 ("); + firstinhcol = 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 (!firstinhcol) + appendPQExpBufferStr(q, ");\n"); /* * Add inherited CHECK constraints, if any.
pgsql-hackers by date: