Subject: Re: Should we remove vacuum_defer_cleanup_age?

From: Justin Pryzby <[email-masked]>
Date: Tue, 11 Apr 2023 11:33:01 -0500

On Wed, Mar 22, 2023 at 10:00:48AM -0700, Andres Freund wrote:
> I don't know whether others think we should apply it this release, given the
> "late submission", but I tend to think it's not worth caring the complication
> of vacuum_defer_cleanup_age forward.

I don't see any utility in waiting; it just makes the process of
removing it take longer for no reason.

As long as it's done before the betas, it seems completely reasonable to
remove it for v16. 

-- 
Justin

---

From: Andres Freund <[email-masked]>
Date: Tue, 11 Apr 2023 11:20:10 -0700

Hi,

> As long as it's done before the betas, it seems completely reasonable to

Added the RMT.

We really should have a [email-masked] alias...

Updated patch attached. I think we should either apply something like that
patch, or at least add a <warning/> to the docs.

Greetings,

Andres Freund
>From c51c9e450b1b1342c0f6ff32e7e360677ccbc2c6 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email-masked]>
Date: Tue, 11 Apr 2023 10:21:36 -0700
Subject: [PATCH v2] Remove vacuum_defer_cleanup_age

This commit removes TransactionIdRetreatSafely(), as there are no users
anymore. There might be potential future users, hence noting that here.

Reviewed-by: Daniel Gustafsson <[email-masked]>
Reviewed-by: Justin Pryzby <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/include/storage/standby.h                 |   1 -
 src/backend/storage/ipc/procarray.c           | 105 ++----------------
 src/backend/storage/ipc/standby.c             |   1 -
 src/backend/utils/misc/guc_tables.c           |   9 --
 src/backend/utils/misc/postgresql.conf.sample |   1 -
 src/bin/pg_upgrade/server.c                   |   5 +-
 doc/src/sgml/config.sgml                      |  35 ------
 doc/src/sgml/high-availability.sgml           |  28 +----
 8 files changed, 15 insertions(+), 170 deletions(-)

diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 41f4dc372e6..e8f50569491 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -21,7 +21,6 @@
 #include "storage/standbydefs.h"
 
 /* User-settable GUC parameters */
-extern PGDLLIMPORT int vacuum_defer_cleanup_age;
 extern PGDLLIMPORT int max_standby_archive_delay;
 extern PGDLLIMPORT int max_standby_streaming_delay;
 extern PGDLLIMPORT bool log_recovery_conflict_waits;
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ea91ce355f2..106b184a3e6 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -367,9 +367,6 @@ static inline void ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId l
 static void ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid);
 static void MaintainLatestCompletedXid(TransactionId latestXid);
 static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
-static void TransactionIdRetreatSafely(TransactionId *xid,
-									   int retreat_by,
-									   FullTransactionId rel);
 
 static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
 												  TransactionId xid);
@@ -1709,10 +1706,7 @@ TransactionIdIsActive(TransactionId xid)
  * do about that --- data is only protected if the walsender runs continuously
  * while queries are executed on the standby.  (The Hot Standby code deals
  * with such cases by failing standby queries that needed to access
- * already-removed data, so there's no integrity bug.)  The computed values
- * are also adjusted with vacuum_defer_cleanup_age, so increasing that setting
- * on the fly is another easy way to make horizons move backwards, with no
- * consequences for data integrity.
+ * already-removed data, so there's no integrity bug.)
  *
  * Note: the approximate horizons (see definition of GlobalVisState) are
  * updated by the computations done here. That's currently required for
@@ -1877,50 +1871,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
 		/* temp relations cannot be accessed in recovery */
 	}
-	else
-	{
-		/*
-		 * Compute the cutoff XID by subtracting vacuum_defer_cleanup_age.
-		 *
-		 * vacuum_defer_cleanup_age provides some additional "slop" for the
-		 * benefit of hot standby queries on standby servers.  This is quick
-		 * and dirty, and perhaps not all that useful unless the primary has a
-		 * predictable transaction rate, but it offers some protection when
-		 * there's no walsender connection.  Note that we are assuming
-		 * vacuum_defer_cleanup_age isn't large enough to cause wraparound ---
-		 * so guc.c should limit it to no more than the xidStopLimit threshold
-		 * in varsup.c.  Also note that we intentionally don't apply
-		 * vacuum_defer_cleanup_age on standby servers.
-		 *
-		 * Need to use TransactionIdRetreatSafely() instead of open-coding the
-		 * subtraction, to prevent creating an xid before
-		 * FirstNormalTransactionId.
-		 */
-		Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
-											 h->shared_oldest_nonremovable));
-		Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
-											 h->data_oldest_nonremovable));
 
-		if (vacuum_defer_cleanup_age > 0)
-		{
-			TransactionIdRetreatSafely(&h->oldest_considered_running,
-									   vacuum_defer_cleanup_age,
-									   h->latest_completed);
-			TransactionIdRetreatSafely(&h->shared_oldest_nonremovable,
-									   vacuum_defer_cleanup_age,
-									   h->latest_completed);
-			TransactionIdRetreatSafely(&h->data_oldest_nonremovable,
-									   vacuum_defer_cleanup_age,
-									   h->latest_completed);
-			/* defer doesn't apply to temp relations */
-
-
-			Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
-												 h->shared_oldest_nonremovable));
-			Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
-												 h->data_oldest_nonremovable));
-		}
-	}
+	Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
+										 h->shared_oldest_nonremovable));
+	Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
+										 h->data_oldest_nonremovable));
 
 	/*
 	 * Check whether there are replication slots requiring an older xmin.
@@ -1947,8 +1902,8 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 						   h->slot_catalog_xmin);
 
 	/*
-	 * It's possible that slots / vacuum_defer_cleanup_age backed up the
-	 * horizons further than oldest_considered_running. Fix.
+	 * It's possible that slots backed up the horizons further than
+	 * oldest_considered_running. Fix.
 	 */
 	h->oldest_considered_running =
 		TransactionIdOlder(h->oldest_considered_running,
@@ -2490,15 +2445,9 @@ GetSnapshotData(Snapshot snapshot)
 		 */
 		oldestfxid = FullXidRelativeTo(latest_completed, oldestxid);
 
-		/* apply vacuum_defer_cleanup_age */
-		def_vis_xid_data = xmin;
-		TransactionIdRetreatSafely(&def_vis_xid_data,
-								   vacuum_defer_cleanup_age,
-								   oldestfxid);
-
 		/* Check whether there's a replication slot requiring an older xmin. */
 		def_vis_xid_data =
-			TransactionIdOlder(def_vis_xid_data, replication_slot_xmin);
+			TransactionIdOlder(xmin, replication_slot_xmin);
 
 		/*
 		 * Rows in non-shared, non-catalog tables possibly could be vacuumed
@@ -4320,44 +4269,6 @@ GlobalVisCheckRemovableXid(Relation rel, TransactionId xid)
 	return GlobalVisTestIsRemovableXid(state, xid);
 }
 
-/*
- * Safely retract *xid by retreat_by, store the result in *xid.
- *
- * Need to be careful to prevent *xid from retreating below
- * FirstNormalTransactionId during epoch 0. This is important to prevent
- * generating xids that cannot be converted to a FullTransactionId without
- * wrapping around.
- *
- * If retreat_by would lead to a too old xid, FirstNormalTransactionId is
- * returned instead.
- */
-static void
-TransactionIdRetreatSafely(TransactionId *xid, int retreat_by, FullTransactionId rel)
-{
-	TransactionId original_xid = *xid;
-	FullTransactionId fxid;
-	uint64		fxid_i;
-
-	Assert(TransactionIdIsNormal(original_xid));
-	Assert(retreat_by >= 0);	/* relevant GUCs are stored as ints */
-	AssertTransactionIdInAllowableRange(original_xid);
-
-	if (retreat_by == 0)
-		return;
-
-	fxid = FullXidRelativeTo(rel, original_xid);
-	fxid_i = U64FromFullTransactionId(fxid);
-
-	if ((fxid_i - FirstNormalTransactionId) <= retreat_by)
-		*xid = FirstNormalTransactionId;
-	else
-	{
-		*xid = TransactionIdRetreatedBy(original_xid, retreat_by);
-		Assert(TransactionIdIsNormal(*xid));
-		Assert(NormalTransactionIdPrecedes(*xid, original_xid));
-	}
-}
-
 /*
  * Convert a 32 bit transaction id into 64 bit transaction id, by assuming it
  * is within MaxTransactionId / 2 of XidFromFullTransactionId(rel).
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 63a72033f9a..ffe5e1563f5 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -38,7 +38,6 @@
 #include "utils/timestamp.h"
 
 /* User-settable GUC parameters */
-int			vacuum_defer_cleanup_age;
 int			max_standby_archive_delay = 30 * 1000;
 int			max_standby_streaming_delay = 30 * 1000;
 bool		log_recovery_conflict_waits = false;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1067537e74c..eb1666797a7 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2566,15 +2566,6 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
-	{
-		{"vacuum_defer_cleanup_age", PGC_SIGHUP, REPLICATION_PRIMARY,
-			gettext_noop("Number of transactions by which VACUUM and HOT cleanup should be deferred, if any."),
-			NULL
-		},
-		&vacuum_defer_cleanup_age,
-		0, 0, 1000000,			/* see ComputeXidHorizons */
-		NULL, NULL, NULL
-	},
 	{
 		{"vacuum_failsafe_age", PGC_USERSET, CLIENT_CONN_STATEMENT,
 			gettext_noop("Age at which VACUUM should trigger failsafe to avoid a wraparound outage."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e715aff3b81..c8e82d532ac 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -328,7 +328,6 @@
 				# method to choose sync standbys, number of sync standbys,
 				# and comma-separated list of application_name
 				# from standby(s); '*' = all
-#vacuum_defer_cleanup_age = 0	# number of xacts by which cleanup is delayed
 
 # - Standby Servers -
 
diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 820bddf3159..0bc3d2806b8 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -234,9 +234,6 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * we only modify the new cluster, so only use it there.  If there is a
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
-	 *
-	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
-	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
 			 "\"%s/pg_ctl\" -w -l \"%s/%s\" -D \"%s\" -o \"-p %d -b%s %s%s\" start",
@@ -244,7 +241,7 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 			 log_opts.logdir,
 			 SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster == &new_cluster) ?
-			 " -c synchronous_commit=off -c fsync=off -c full_page_writes=off -c vacuum_defer_cleanup_age=0" : "",
+			 " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
 			 cluster->pgopts ? cluster->pgopts : "", socket_string);
 
 	/*
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index f81c2045ec4..02ed461788e 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4597,41 +4597,6 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-vacuum-defer-cleanup-age" xreflabel="vacuum_defer_cleanup_age">
-      <term><varname>vacuum_defer_cleanup_age</varname> (<type>integer</type>)
-      <indexterm>
-       <primary><varname>vacuum_defer_cleanup_age</varname> configuration parameter</primary>
-      </indexterm>
-      </term>
-      <listitem>
-       <para>
-        Specifies the number of transactions by which <command>VACUUM</command> and
-        <link linkend="storage-hot"><acronym>HOT</acronym> updates</link>
-        will defer cleanup of dead row versions. The
-        default is zero transactions, meaning that dead row versions can be
-        removed as soon as possible, that is, as soon as they are no longer
-        visible to any open transaction.  You may wish to set this to a
-        non-zero value on a primary server that is supporting hot standby
-        servers, as described in <xref linkend="hot-standby"/>.  This allows
-        more time for queries on the standby to complete without incurring
-        conflicts due to early cleanup of rows.  However, since the value
-        is measured in terms of number of write transactions occurring on the
-        primary server, it is difficult to predict just how much additional
-        grace time will be made available to standby queries.
-        This parameter can only be set in the <filename>postgresql.conf</filename>
-        file or on the server command line.
-       </para>
-       <para>
-        You should also consider setting <varname>hot_standby_feedback</varname>
-        on standby server(s) as an alternative to using this parameter.
-       </para>
-       <para>
-        This does not prevent cleanup of dead rows which have reached the age
-        specified by <varname>old_snapshot_threshold</varname>.
-       </para>
-      </listitem>
-     </varlistentry>
-
      </variablelist>
     </sect2>
 
diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 9d0deaeeb80..cf61b2ed2a1 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -944,12 +944,11 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
     retained by replication slots.
    </para>
    <para>
-    Similarly, <xref linkend="guc-hot-standby-feedback"/>
-    and <xref linkend="guc-vacuum-defer-cleanup-age"/> provide protection against
-    relevant rows being removed by vacuum, but the former provides no
-    protection during any time period when the standby is not connected,
-    and the latter often needs to be set to a high value to provide adequate
-    protection.  Replication slots overcome these disadvantages.
+    Similarly, <xref linkend="guc-hot-standby-feedback"/> on its own, without
+    also using a replication slot, provides protection against relevant rows
+    being removed by vacuum, but provides no protection during any time period
+    when the standby is not connected.  Replication slots overcome these
+    disadvantages.
    </para>
    <sect3 id="streaming-replication-slots-manipulation">
     <title>Querying and Manipulating Replication Slots</title>
@@ -1910,17 +1909,6 @@ synchronous_standby_names = 'ANY 2 (s1, s2, s3)'
     by newly-arrived streaming WAL entries after reconnection.
    </para>
 
-   <para>
-    Another option is to increase <xref linkend="guc-vacuum-defer-cleanup-age"/>
-    on the primary server, so that dead rows will not be cleaned up as quickly
-    as they normally would be.  This will allow more time for queries to
-    execute before they are canceled on the standby, without having to set
-    a high <varname>max_standby_streaming_delay</varname>.  However it is
-    difficult to guarantee any specific execution-time window with this
-    approach, since <varname>vacuum_defer_cleanup_age</varname> is measured in
-    transactions executed on the primary server.
-   </para>
-
    <para>
     The number of query cancels and the reason for them can be viewed using
     the <structname>pg_stat_database_conflicts</structname> system view on the standby
@@ -2257,8 +2245,7 @@ HINT:  You can then restart the server after making the necessary configuration
    </para>
 
    <para>
-    On the primary, parameters <xref linkend="guc-wal-level"/> and
-    <xref linkend="guc-vacuum-defer-cleanup-age"/> can be used.
+    On the primary, the <xref linkend="guc-wal-level"/> parameter can be used.
     <xref linkend="guc-max-standby-archive-delay"/> and
     <xref linkend="guc-max-standby-streaming-delay"/> have no effect if set on
     the primary.
@@ -2268,9 +2255,6 @@ HINT:  You can then restart the server after making the necessary configuration
     On the standby, parameters <xref linkend="guc-hot-standby"/>,
     <xref linkend="guc-max-standby-archive-delay"/> and
     <xref linkend="guc-max-standby-streaming-delay"/> can be used.
-    <xref linkend="guc-vacuum-defer-cleanup-age"/> has no effect
-    as long as the server remains in standby mode, though it will
-    become relevant if the standby becomes primary.
    </para>
   </sect2>
 
-- 
2.38.0

---

From: Amit Kapila <[email-masked]>
Date: Thu, 13 Apr 2023 09:04:24 +0530

> patch, or at least add a <warning/> to the docs.

+1 to do one of the above. I think there is a good chance that
somebody might be doing more harm by using it so removing this
shouldn't be a problem. Personally, I have not heard of people using
it but OTOH it is difficult to predict so giving some time is also not
a bad idea.

Do others have any opinion/suggestion on this matter?

-- 
With Regards,
Amit Kapila.

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Thu, 13 Apr 2023 11:32:50 -0400

>> We really should have a [email-masked] alias...

(I had thought something as much -- will reach out to pginfra about options)

> Do others have any opinion/suggestion on this matter?

I need a bit more time to study this before formulating an opinion on 
whether we should remove it for v16. In any case, I'm not against 
documentation.

Jonathan

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Thu, 13 Apr 2023 12:16:38 -0400

On 4/13/23 11:32 AM, Jonathan S. Katz wrote:
>> On Tue, Apr 11, 2023 at 11:50 PM Andres Freund <[email-masked]> 

> whether we should remove it for v16. In any case, I'm not against 

(didn't need too much more time).

[RMT hat]

+1 for removing.

I looked at some data and it doesn't seem like vacuum_defer_cleanup_age 
is used in any significant way, whereas hot_standby_feedback is much 
more widely used. Given this, and all the problems + arguments made in 
the thread, we should just get rid of it for v16.

There are cases where we should deprecate before removing, but I don't 
think this one based upon usage and having a better alternative.

Per [1] it does sound like we can make some improvements to 
hot_standby_feedback, but those can wait to v17.

We should probably set $DATE to finish this, too. I don't think it's a 
rush, but we should give enough time before Beta 1.

Jonathan

[1] 
https://www.postgresql.org/message-id/20230317230930.nhsgk3qfk7f4axls%40awork3.anarazel.de

---

From: Laurenz Albe <[email-masked]>
Date: Fri, 14 Apr 2023 05:06:46 +0200

> > whether we should remove it for v16. In any case, I'm not against 

I am not against this in principle, but I know that there are people using
this parameter; see the discussion linked in

https://postgr.es/m/[email-masked]

I can't say if they have a good use case for that parameter or not.

Yours,
Laurenz Albe