10000 Repair pg_upgrade's failure to preserve relfrozenxid for matviews. · MSPawanRanjith/postgres@66e203c · GitHub
[go: up one dir, main page]
Skip to content

Commit 66e203c

Browse files
tglsfdctglsfdc
committed
Repair pg_upgrade's failure to preserve relfrozenxid for matviews.
This oversight led to data corruption in matviews, manifesting as "could not access status of transaction" before our most recent releases, and "found xmin from before relfrozenxid" errors since then. The proximate cause of the problem seems to have been confusion between the task of preserving dropped-column status and the task of preserving frozenxid status. Those are required for distinct sets of relkinds, and the reasoning was entirely undocumented in the source code. In hopes of forestalling future errors of the same kind, try to improve the commentary in this area. In passing, also improve the remarkably unhelpful comments around pg_upgrade's set_frozenxids(). That's not actually buggy AFAICS, but good luck figuring out what it does from the old comments. Per report from Claudio Freire. It appears that bug #14852 from Alexey Ermakov is an earlier report of the same issue, and there may be other cases that we failed to identify at the time. Patch by me based on analysis by Andres Freund. The bug dates back to the introduction of matviews, so back-patch to all supported branches. Discussion: https://postgr.es/m/CAGTBQpbrY9CdRGGhyBZ9yqY4jWaGC85rUF4X+R7d-aim=mBNsw@mail.gmail.com Discussion: https://postgr.es/m/20171013115320.28049.86457@wrigleys.postgresql.org
1 parent 795f211 commit 66e203c

File tree

2 files changed

+47
-13
lines changed

2 files changed

+47
-13
lines changed

‎src/bin/pg_dump/pg_dump.c

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15688,6 +15688,13 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
1568815688
* column order. That also means we have to take care about setting
1568915689
* attislocal correctly, plus fix up any inherited CHECK constraints.
1569015690
* Analogously, we set up typed tables using ALTER TABLE / OF here.
15691+
*
15692+
* We process foreign tables here, even though they lack heap storage,
15693+
* because they can participate in inheritance relationships and we
15694+
* want this stuff to be consistent across the inheritance tree. We
15695+
* exclude indexes, toast tables, sequences and matviews, even though
15696+
* they have storage, because we don't support altering or dropping
15697+
* columns in them, nor can they be part of inheritance trees.
1569115698
*/
1569215699
if (dopt->binary_upgrade &&
1569315700
(tbinfo->relkind == RELKIND_RELATION ||
@@ -15777,7 +15784,19 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
1577715784
fmtId(tbinfo->dobj.name),
1577815785
tbinfo->reloftype);
1577915786
}
15787+
}
1578015788

15789+
/*
15790+
* In binary_upgrade mode, arrange to restore the old relfrozenxid and
15791+
* relminmxid of all vacuumable relations. (While vacuum.c processes
15792+
* TOAST tables semi-independently, here we see them only as children
15793+
* of other relations; so this "if" lacks RELKIND_TOASTVALUE, and the
15794+
* child toast table is handled below.)
15795+
*/
15796+
if (dopt->binary_upgrade &&
15797+
(tbinfo->relkind == RELKIND_RELATION ||
15798+
tbinfo->relkind == RELKIND_MATVIEW))
15799+
{
1578115800
appendPQExpBufferStr(q, "\n-- For binary upgrade, set heap's relfrozenxid and relminmxid\n");
1578215801
appendPQExpBuffer(q, "UPDATE pg_catalog.pg_class\n"
1578315802
"SET relfrozenxid = '%u', relminmxid = '%u'\n"
@@ -15788,7 +15807,10 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
1578815807

1578915808
if (tbinfo->toast_oid)
1579015809
{
15791-
/* We preserve the toast oids, so we can use it during restore */
15810+
/*
15811+
* The toast table will have the same OID at restore, so we
15812+
* can safely target it by OID.
15813+
*/
1579215814
appendPQExpBufferStr(q, "\n-- For binary upgrade, set toast's relfrozenxid and relminmxid\n");
1579315815
appendPQExpBuffer(q, "UPDATE pg_catalog.pg_class\n"
1579415816
"SET relfrozenxid = '%u', relminmxid = '%u'\n"
@@ -15802,7 +15824,8 @@ dumpTableSchema(Archive *fout, TableInfo *tbinfo)
1580215824
* In binary_upgrade mode, restore matviews' populated status by
1580315825
* poking pg_class directly. This is pretty ugly, but we can't use
1580415826
* REFRESH MATERIALIZED VIEW since it's possible that some underlying
15805-
* matview is not populated even though this matview is.
15827+
* matview is not populated even though this matview is; in any case,
15828+
* we want to transfer the matview's heap storage, not run REFRESH.
1580615829
*/
1580715830
if (dopt->binary_upgrade && tbinfo->relkind == RELKIND_MATVIEW &&
1580815831
tbinfo->relispopulated)

‎src/bin/pg_upgrade/pg_upgrade.c

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -271,13 +271,13 @@ static void
271271
prepare_new_databases(void)
272272
{
273273
/*
274-
* We set autovacuum_freeze_max_age to its maximum value so autovacuum
275-
* does not launch here and delete clog files, before the frozen xids are
276-
* set.
274+
* Before we restore anything, set frozenxids of initdb-created tables.
277275
*/
278-
279276
set_frozenxids(false);
280277

278+
/*
279+
* Now restore global objects (roles and tablespaces).
280+
*/
281281
prep_status("Restoring global objects in the new cluster");
282282

283283
/*
@@ -495,14 +495,25 @@ copy_clog_xlog_xid(void)
495495
/*
496496
* set_frozenxids()
497497
*
498-
* We have frozen all xids, so set datfrozenxid, relfrozenxid, and
499-
* relminmxid to be the old cluster's xid counter, which we just set
500-
* in the new cluster. User-table frozenxid and minmxid values will
501-
* be set by pg_dump --binary-upgrade, but objects not set by the pg_dump
502-
* must have proper frozen counters.
498+
* This is called on the new cluster before we restore anything, with
499+
* minmxid_only = false. Its purpose is to ensure that all initdb-created
500+
* vacuumable tables have relfrozenxid/relminmxid matching the old cluster's
501+
* xid/mxid counters. We also initialize the datfrozenxid/datminmxid of the
502+
* built-in databases to match.
503+
*
504+
* As we create user tables later, their relfrozenxid/relminmxid fields will
505+
* be restored properly by the binary-upgrade restore script. Likewise for
506+
* user-database datfrozenxid/datminmxid. However, if we're upgrading from a
507+
* pre-9.3 database, which does not store per-table or per-DB minmxid, then
508+
* the relminmxid/datminmxid values filled in by the restore script will just
509+
* be zeroes.
510+
*
511+
* Hence, with a pre-9.3 source database, a second call occurs after
512+
* everything is restored, with minmxid_only = true. This pass will
513+
* initialize all tables and databases, both those made by initdb and user
514+
* objects, with the desired minmxid value. frozenxid values are left alone.
503515
*/
504-
static
505-
void
516+
static void
506517
set_frozenxids(bool minmxid_only)
507518
{
508519
int dbnum;

0 commit comments

Comments
 (0)
0