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@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

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