Re: [UNVERIFIED SENDER] Re: pg_upgrade can result in early wraparound on databases with high transaction load

This patch has been applied back to 9.6 and will appear in the next
minor release.

---------------------------------------------------------------------------

On Tue, May 18, 2021 at 01:26:38PM +0200, Drouvot, Bertrand wrote:
> Hi,
>
> On 5/4/21 10:17 AM, Drouvot, Bertrand wrote:
>
>
> Hi,
>
> On 4/24/21 3:00 AM, Andres Freund wrote:
>
> Hi,
>
> On 2021-04-23 19:28:27 -0500, Justin Pryzby wrote:
>
> This (combination of) thread(s) seems relevant.
>
> Subject: pg_upgrade failing for 200+ million Large Objects
> https://www.postgresql.org/message-id/flat/12601596dbbc4c01b86b4ac4d2bd4d48%40EX13D05UWC001.ant.amazon.com
> https://www.postgresql.org/message-id/flat/a9f9376f1c3343a6bb319dce294e20ac%40EX13D05UWC001.ant.amazon.com
> https://www.postgresql.org/message-id/flat/cc089cc3-fc43-9904-fdba-d830d8222145%40enterprisedb.com#3eec85391c6076a4913e96a86fece75e
>
> Huh. Thanks for digging these up.
>
>
>
> Allows the user to provide a constant via pg_upgrade command-line, that
> overrides the 2 billion constant in pg_resetxlog [1] thereby increasing the
> (window of) Transaction IDs available for pg_upgrade to complete.
>
> That seems the entirely the wrong approach to me, buying further into
> the broken idea of inventing random wrong values for oldestXid.
>
> We drive important things like the emergency xid limits off oldestXid. On
> databases with tables that are older than ~147million xids (i.e. not even
> affected by the default autovacuum_freeze_max_age) the current constant leads
> to setting the oldestXid to a value *in the future*/wrapped around. Any
> different different constant (or pg_upgrade parameter) will do that too in
> other scenarios.
>
> As far as I can tell there is precisely *no* correct behaviour here other than
> exactly copying the oldestXid limit from the source database.
>
>
> Please find attached a patch proposal doing so: it adds a new (- u)
> parameter to pg_resetwal that allows to specify the oldest unfrozen XID to
> set.
> Then this new parameter is being used in pg_upgrade to copy the source
> Latest checkpoint's oldestXID.
>
> Questions:
>
> □ Should we keep the old behavior in case -x is being used without -u?
> (The proposed patch does not set an arbitrary oldestXID anymore in case
> -x is used.)
> □ Also shouldn't we ensure that the xid provided with -x or -u is >=
> FirstNormalTransactionId (Currently the only check is that it is # 0)?
>
>
> Copy/pasting Andres feedback (Thanks Andres for this feedback) on those
> questions from another thread [1].
>
> > I was also wondering if:
> >
> > * We should keep the old behavior in case pg_resetwal -x is being used
> > without -u?
 (The proposed patch does not set an arbitrary oldestXID
> > anymore in 
case -x is used)
>
> Andres: I don't think we should. I don't see anything in the old behaviour
> worth
> maintaining.
>
> > * We should ensure that the xid provided with -x or -u is
> > >=
FirstNormalTransactionId (Currently the only check is that it is
> > # 0)?
>
> Andres: Applying TransactionIdIsNormal() seems like a good idea.
>
> => I am attaching a new version that makes use of TransactionIdIsNormal()
> checks.
>
> Andres: I think it's important to verify that the xid provided with -x is
> within a reasonable range of the oldest xid.
>
> => What do you mean by "a reasonable range"?
>
> Thanks
>
> Bertrand
>
> [1]: https://www.postgresql.org/message-id/
> 20210517185646.pwe4klaufwmdhe2a%40alap3.anarazel.de
>
>
>
>

> src/bin/pg_resetwal/pg_resetwal.c | 65 ++++++++++++++++++++++-----------------
> src/bin/pg_upgrade/controldata.c | 17 +++++++++-
> src/bin/pg_upgrade/pg_upgrade.c | 6 ++++
> src/bin/pg_upgrade/pg_upgrade.h | 1 +
> 4 files changed, 60 insertions(+), 29 deletions(-)
> diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
> index 805dafef07..5e864760ed 100644
> --- a/src/bin/pg_resetwal/pg_resetwal.c
> +++ b/src/bin/pg_resetwal/pg_resetwal.c
> @@ -65,6 +65,7 @@ static bool guessed = false; /* T if we had to guess at any values */
> static const char *progname;
> static uint32 set_xid_epoch = (uint32) -1;
> static TransactionId set_xid = 0;
> +static TransactionId set_oldest_unfrozen_xid = 0;
> static TransactionId set_oldest_commit_ts_xid = 0;
> static TransactionId set_newest_commit_ts_xid = 0;
> static Oid set_oid = 0;
> @@ -102,6 +103,7 @@ main(int argc, char *argv[])
> {"next-oid", required_argument, NULL, 'o'},
> {"multixact-offset", required_argument, NULL, 'O'},
> {"next-transaction-id", required_argument, NULL, 'x'},
> + {"oldest-transaction-id", required_argument, NULL, 'u'},
> {"wal-segsize", required_argument, NULL, 1},
> {NULL, 0, NULL, 0}
> };
> @@ -135,7 +137,7 @@ main(int argc, char *argv[])
> }
>
>
> - while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:x:", long_options, NULL)) != -1)
> + while ((c = getopt_long(argc, argv, "c:D:e:fl:m:no:O:x:u:", long_options, NULL)) != -1)
> {
> switch (c)
> {
> @@ -176,9 +178,24 @@ main(int argc, char *argv[])
> fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
> exit(1);
> }
> - if (set_xid == 0)
> + if (!TransactionIdIsNormal(set_xid))
> {
> - pg_log_error("transaction ID (-x) must not be 0");
> + pg_log_error("transaction ID (-x) must be greater or equal to %u", FirstNormalTransactionId);
> + exit(1);
> + }
> + break;
> +
> + case 'u':
> + set_oldest_unfrozen_xid = strtoul(optarg, &endptr, 0);
> + if (endptr == optarg || *endptr != '\0')
> + {
> + pg_log_error("invalid argument for option %s", "-u");
> + fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
> + exit(1);
> + }
> + if (!TransactionIdIsNormal(set_oldest_unfrozen_xid))
> + {
> + pg_log_error("oldest unfrozen transaction ID (-u) must be greater or equal to %u", FirstNormalTransactionId);
> exit(1);
> }
> break;
> @@ -429,21 +446,12 @@ main(int argc, char *argv[])
> XidFromFullTransactionId(ControlFile.checkPointCopy.nextXid));
>
> if (set_xid != 0)
> - {
> ControlFile.checkPointCopy.nextXid =
> FullTransactionIdFromEpochAndXid(EpochFromFullTransactionId(ControlFile.checkPointCopy.nextXid),
> set_xid);
>
> - /*
> - * For the moment, just set oldestXid to a value that will force
> - * immediate autovacuum-for-wraparound. It's not clear whether adding
> - * user control of this is useful, so let's just do something that's
> - * reasonably safe. The magic constant here corresponds to the
> - * maximum allowed value of autovacuum_freeze_max_age.
> - */
> - ControlFile.checkPointCopy.oldestXid = set_xid - 2000000000;
> - if (ControlFile.checkPointCopy.oldestXid < FirstNormalTransactionId)
> - ControlFile.checkPointCopy.oldestXid += FirstNormalTransactionId;
> + if (set_oldest_unfrozen_xid != 0) {
> + ControlFile.checkPointCopy.oldestXid = set_oldest_unfrozen_xid;
> ControlFile.checkPointCopy.oldestXidDB = InvalidOid;
> }
>
> @@ -1209,20 +1217,21 @@ usage(void)
> printf(_("Usage:\n %s [OPTION]... DATADIR\n\n"), progname);
> printf(_("Options:\n"));
> printf(_(" -c, --commit-timestamp-ids=XID,XID\n"
> - " set oldest and newest transactions bearing\n"
> - " commit timestamp (zero means no change)\n"));
> - printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
> - printf(_(" -e, --epoch=XIDEPOCH set next transaction ID epoch\n"));
> - printf(_(" -f, --force force update to be done\n"));
> - printf(_(" -l, --next-wal-file=WALFILE set minimum starting location for new WAL\n"));
> - printf(_(" -m, --multixact-ids=MXID,MXID set next and oldest multitransaction ID\n"));
> - printf(_(" -n, --dry-run no update, just show what would be done\n"));
> - printf(_(" -o, --next-oid=OID set next OID\n"));
> - printf(_(" -O, --multixact-offset=OFFSET set next multitransaction offset\n"));
> - printf(_(" -V, --version output version information, then exit\n"));
> - printf(_(" -x, --next-transaction-id=XID set next transaction ID\n"));
> - printf(_(" --wal-segsize=SIZE size of WAL segments, in megabytes\n"));
> - printf(_(" -?, --help show this help, then exit\n"));
> + " set oldest and newest transactions bearing\n"
> + " commit timestamp (zero means no change)\n"));
> + printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
> + printf(_(" -e, --epoch=XIDEPOCH set next transaction ID epoch\n"));
> + printf(_(" -f, --force force update to be done\n"));
> + printf(_(" -l, --next-wal-file=WALFILE set minimum starting location for new WAL\n"));
> + printf(_(" -m, --multixact-ids=MXID,MXID set next and oldest multitransaction ID\n"));
> + printf(_(" -n, --dry-run no update, just show what would be done\n"));
> + printf(_(" -o, --next-oid=OID set next OID\n"));
> + printf(_(" -O, --multixact-offset=OFFSET set next multitransaction offset\n"));
> + printf(_(" -u, --oldest-transaction-id=XID set oldest unfrozen transaction ID\n"));
> + printf(_(" -V, --version output version information, then exit\n"));
> + printf(_(" -x, --next-transaction-id=XID set next transaction ID\n"));
> + printf(_(" --wal-segsize=SIZE size of WAL segments, in megabytes\n"));
> + printf(_(" -?, --help show this help, then exit\n"));
> printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
> printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
> }
> diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
> index 4f647cdf33..a4b6375403 100644
> --- a/src/bin/pg_upgrade/controldata.c
> +++ b/src/bin/pg_upgrade/controldata.c
> @@ -44,6 +44,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
> bool got_oid = false;
> bool got_multi = false;
> bool got_oldestmulti = false;
> + bool got_oldestxid = false;
> bool got_mxoff = false;
> bool got_nextxlogfile = false;
> bool got_float8_pass_by_value = false;
> @@ -312,6 +313,17 @@ get_control_data(ClusterInfo *cluster, bool live_check)
> cluster->controldata.chkpnt_nxtmulti = str2uint(p);
> got_multi = true;
> }
> + else if ((p = strstr(bufin, "Latest checkpoint's oldestXID:")) != NULL)
> + {
> + p = strchr(p, ':');
> +
> + if (p == NULL || strlen(p) <= 1)
> + pg_fatal("%d: controldata retrieval problem\n", __LINE__);
> +
> + p++; /* remove ':' char */
> + cluster->controldata.chkpnt_oldstxid = str2uint(p);
> + got_oldestxid = true;
> + }
> else if ((p = strstr(bufin, "Latest checkpoint's oldestMultiXid:")) != NULL)
> {
> p = strchr(p, ':');
> @@ -544,7 +556,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
>
> /* verify that we got all the mandatory pg_control data */
> if (!got_xid || !got_oid ||
> - !got_multi ||
> + !got_multi || !got_oldestxid ||
> (!got_oldestmulti &&
> cluster->controldata.cat_ver >= MULTIXACT_FORMATCHANGE_CAT_VER) ||
> !got_mxoff || (!live_check && !got_nextxlogfile) ||
> @@ -575,6 +587,9 @@ get_control_data(ClusterInfo *cluster, bool live_check)
> cluster->controldata.cat_ver >= MULTIXACT_FORMATCHANGE_CAT_VER)
> pg_log(PG_REPORT, " latest checkpoint oldest MultiXactId\n");
>
> + if (!got_oldestxid)
> + pg_log(PG_REPORT, " latest checkpoint oldestXID\n");
> +
> if (!got_mxoff)
> pg_log(PG_REPORT, " latest checkpoint next MultiXactOffset\n");
>
> diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c
> index e23b8ca88d..950ff24980 100644
> --- a/src/bin/pg_upgrade/pg_upgrade.c
> +++ b/src/bin/pg_upgrade/pg_upgrade.c
> @@ -473,6 +473,12 @@ copy_xact_xlog_xid(void)
> "\"%s/pg_resetwal\" -f -x %u \"%s\"",
> new_cluster.bindir, old_cluster.controldata.chkpnt_nxtxid,
> new_cluster.pgdata);
> + check_ok();
> + prep_status("Setting oldest XID for new cluster");
> + exec_prog(UTILITY_LOG_FILE, NULL, true, true,
> + "\"%s/pg_resetwal\" -f -u %u \"%s\"",
> + new_cluster.bindir, old_cluster.controldata.chkpnt_oldstxid,
> + new_cluster.pgdata);
> exec_prog(UTILITY_LOG_FILE, NULL, true, true,
> "\"%s/pg_resetwal\" -f -e %u \"%s\"",
> new_cluster.bindir, old_cluster.controldata.chkpnt_nxtepoch,
> diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
> index a5f71c5294..dd0204902c 100644
> --- a/src/bin/pg_upgrade/pg_upgrade.h
> +++ b/src/bin/pg_upgrade/pg_upgrade.h
> @@ -207,6 +207,7 @@ typedef struct
> uint32 chkpnt_nxtmulti;
> uint32 chkpnt_nxtmxoff;
> uint32 chkpnt_oldstMulti;
> + uint32 chkpnt_oldstxid;
> uint32 align;
> uint32 blocksz;
> uint32 largesz;

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.