public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v56 2/6] Add conditional lock feature to dshash
5+ messages / 5 participants
[nested] [flat]
* [PATCH v56 2/6] Add conditional lock feature to dshash
@ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw)
Dshash currently waits for lock unconditionally. It is inconvenient
when we want to avoid being blocked by other processes. This commit
adds alternative functions of dshash_find and dshash_find_or_insert
that allows immediate return on lock failure.
---
src/backend/lib/dshash.c | 98 +++++++++++++++++++++-------------------
src/include/lib/dshash.h | 3 ++
2 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index 29ad767618..f79b6de245 100644
--- a/src/backend/lib/dshash.c
+++ b/src/backend/lib/dshash.c
@@ -383,6 +383,10 @@ dshash_get_hash_table_handle(dshash_table *hash_table)
* the caller must take care to ensure that the entry is not left corrupted.
* The lock mode is either shared or exclusive depending on 'exclusive'.
*
+ * If found is not NULL, *found is set to true if the key is found in the hash
+ * table. If the key is not found, *found is set to false and a pointer to a
+ * newly created entry is returned.
+ *
* The caller must not lock a lock already.
*
* Note that the lock held is in fact an LWLock, so interrupts will be held on
@@ -392,36 +396,7 @@ dshash_get_hash_table_handle(dshash_table *hash_table)
void *
dshash_find(dshash_table *hash_table, const void *key, bool exclusive)
{
- dshash_hash hash;
- size_t partition;
- dshash_table_item *item;
-
- hash = hash_key(hash_table, key);
- partition = PARTITION_FOR_HASH(hash);
-
- Assert(hash_table->control->magic == DSHASH_MAGIC);
- Assert(!hash_table->find_locked);
-
- LWLockAcquire(PARTITION_LOCK(hash_table, partition),
- exclusive ? LW_EXCLUSIVE : LW_SHARED);
- ensure_valid_bucket_pointers(hash_table);
-
- /* Search the active bucket. */
- item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash));
-
- if (!item)
- {
- /* Not found. */
- LWLockRelease(PARTITION_LOCK(hash_table, partition));
- return NULL;
- }
- else
- {
- /* The caller will free the lock by calling dshash_release_lock. */
- hash_table->find_locked = true;
- hash_table->find_exclusively_locked = exclusive;
- return ENTRY_FROM_ITEM(item);
- }
+ return dshash_find_extended(hash_table, key, exclusive, false, false, NULL);
}
/*
@@ -439,31 +414,60 @@ dshash_find_or_insert(dshash_table *hash_table,
const void *key,
bool *found)
{
- dshash_hash hash;
- size_t partition_index;
- dshash_partition *partition;
+ return dshash_find_extended(hash_table, key, true, false, true, found);
+}
+
+
+/*
+ * Find the key in the hash table.
+ *
+ * "exclusive" is the lock mode in which the partition for the returned item
+ * is locked. If "nowait" is true, the function immediately returns if
+ * required lock was not acquired. "insert" indicates insert mode. In this
+ * mode new entry is inserted and set *found to false. *found is set to true if
+ * found. "found" must be non-null in this mode.
+ */
+void *
+dshash_find_extended(dshash_table *hash_table, const void *key,
+ bool exclusive, bool nowait, bool insert, bool *found)
+{
+ dshash_hash hash = hash_key(hash_table, key);
+ size_t partidx = PARTITION_FOR_HASH(hash);
+ dshash_partition *partition = &hash_table->control->partitions[partidx];
+ LWLockMode lockmode = exclusive ? LW_EXCLUSIVE : LW_SHARED;
dshash_table_item *item;
- hash = hash_key(hash_table, key);
- partition_index = PARTITION_FOR_HASH(hash);
- partition = &hash_table->control->partitions[partition_index];
-
- Assert(hash_table->control->magic == DSHASH_MAGIC);
- Assert(!hash_table->find_locked);
+ /* must be exclusive when insert allowed */
+ Assert(!insert || (exclusive && found != NULL));
restart:
- LWLockAcquire(PARTITION_LOCK(hash_table, partition_index),
- LW_EXCLUSIVE);
+ if (!nowait)
+ LWLockAcquire(PARTITION_LOCK(hash_table, partidx), lockmode);
+ else if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partidx),
+ lockmode))
+ return NULL;
+
ensure_valid_bucket_pointers(hash_table);
/* Search the active bucket. */
item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash));
if (item)
- *found = true;
+ {
+ if (found)
+ *found = true;
+ }
else
{
- *found = false;
+ if (found)
+ *found = false;
+
+ if (!insert)
+ {
+ /* The caller didn't told to add a new entry. */
+ LWLockRelease(PARTITION_LOCK(hash_table, partidx));
+ return NULL;
+ }
/* Check if we are getting too full. */
if (partition->count > MAX_COUNT_PER_PARTITION(hash_table))
@@ -479,7 +483,8 @@ restart:
* Give up our existing lock first, because resizing needs to
* reacquire all the locks in the right order to avoid deadlocks.
*/
- LWLockRelease(PARTITION_LOCK(hash_table, partition_index));
+ LWLockRelease(PARTITION_LOCK(hash_table, partidx));
+
resize(hash_table, hash_table->size_log2 + 1);
goto restart;
@@ -493,12 +498,13 @@ restart:
++partition->count;
}
- /* The caller must release the lock with dshash_release_lock. */
+ /* The caller will free the lock by calling dshash_release_lock. */
hash_table->find_locked = true;
- hash_table->find_exclusively_locked = true;
+ hash_table->find_exclusively_locked = exclusive;
return ENTRY_FROM_ITEM(item);
}
+
/*
* Remove an entry by key. Returns true if the key was found and the
* corresponding entry was removed.
diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h
index a6ea377173..5b8114d041 100644
--- a/src/include/lib/dshash.h
+++ b/src/include/lib/dshash.h
@@ -91,6 +91,9 @@ extern void *dshash_find(dshash_table *hash_table,
const void *key, bool exclusive);
extern void *dshash_find_or_insert(dshash_table *hash_table,
const void *key, bool *found);
+extern void *dshash_find_extended(dshash_table *hash_table, const void *key,
+ bool exclusive, bool nowait, bool insert,
+ bool *found);
extern bool dshash_delete_key(dshash_table *hash_table, const void *key);
extern void dshash_delete_entry(dshash_table *hash_table, void *entry);
extern void dshash_release_lock(dshash_table *hash_table, void *entry);
--
2.27.0
----Next_Part(Tue_Mar_16_10_27_55_2021_500)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v56-0003-Shared-memory-based-stats-collector.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-03-18 09:33 Alvaro Herrera <[email protected]>
2023-03-22 16:44 ` Re: Should we remove vacuum_defer_cleanup_age? Justin Pryzby <[email protected]>
2023-03-24 21:27 ` Re: Should we remove vacuum_defer_cleanup_age? Peter Geoghegan <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Alvaro Herrera @ 2023-03-18 09:33 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers
On 2023-Mar-17, Andres Freund wrote:
> I started writing a test for vacuum_defer_cleanup_age while working on the fix
> referenced above, but now I am wondering if said energy would be better spent
> removing vacuum_defer_cleanup_age alltogether.
+1 I agree it's not useful anymore.
> I don't think I have the cycles to push this through in the next weeks, but if
> we agree removing vacuum_defer_cleanup_age is a good idea, it seems like a
> good idea to mark it as deprecated in 16?
Hmm, for the time being, can we just "disable" it by disallowing to set
the GUC to a value different from 0? Then we can remove the code later
in the cycle at leisure.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
2023-03-18 09:33 Re: Should we remove vacuum_defer_cleanup_age? Alvaro Herrera <[email protected]>
@ 2023-03-22 16:44 ` Justin Pryzby <[email protected]>
2023-03-22 17:00 ` Re: Should we remove vacuum_defer_cleanup_age? Andres Freund <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Justin Pryzby @ 2023-03-22 16:44 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Sat, Mar 18, 2023 at 10:33:57AM +0100, Alvaro Herrera wrote:
> On 2023-Mar-17, Andres Freund wrote:
>
> > I started writing a test for vacuum_defer_cleanup_age while working on the fix
> > referenced above, but now I am wondering if said energy would be better spent
> > removing vacuum_defer_cleanup_age alltogether.
>
> +1 I agree it's not useful anymore.
>
> > I don't think I have the cycles to push this through in the next weeks, but if
> > we agree removing vacuum_defer_cleanup_age is a good idea, it seems like a
> > good idea to mark it as deprecated in 16?
>
> Hmm, for the time being, can we just "disable" it by disallowing to set
> the GUC to a value different from 0? Then we can remove the code later
> in the cycle at leisure.
It can be useful to do a "rolling transition", and it's something I do
often.
But I can't see why that would be useful here? It seems like something
that could be done after the feature freeze. It's removing a feature,
not adding one.
--
Justin
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
2023-03-18 09:33 Re: Should we remove vacuum_defer_cleanup_age? Alvaro Herrera <[email protected]>
2023-03-22 16:44 ` Re: Should we remove vacuum_defer_cleanup_age? Justin Pryzby <[email protected]>
@ 2023-03-22 17:00 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Andres Freund @ 2023-03-22 17:00 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
Hi,
On 2023-03-22 11:44:20 -0500, Justin Pryzby wrote:
> On Sat, Mar 18, 2023 at 10:33:57AM +0100, Alvaro Herrera wrote:
> > On 2023-Mar-17, Andres Freund wrote:
> >
> > > I started writing a test for vacuum_defer_cleanup_age while working on the fix
> > > referenced above, but now I am wondering if said energy would be better spent
> > > removing vacuum_defer_cleanup_age alltogether.
> >
> > +1 I agree it's not useful anymore.
> >
> > > I don't think I have the cycles to push this through in the next weeks, but if
> > > we agree removing vacuum_defer_cleanup_age is a good idea, it seems like a
> > > good idea to mark it as deprecated in 16?
> >
> > Hmm, for the time being, can we just "disable" it by disallowing to set
> > the GUC to a value different from 0? Then we can remove the code later
> > in the cycle at leisure.
>
> It can be useful to do a "rolling transition", and it's something I do
> often.
>
> But I can't see why that would be useful here? It seems like something
> that could be done after the feature freeze. It's removing a feature,
> not adding one.
It wasn't actually that much work to write a patch to remove
vacuum_defer_cleanup_age, see the attached.
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.
Greetings,
Andres Freund
Attachments:
[text/x-diff] v1-0001-Remove-vacuum_defer_cleanup_age.patch (15.1K, ../../[email protected]/2-v1-0001-Remove-vacuum_defer_cleanup_age.patch)
download | inline diff:
From 24b519015bb1922a9746eda2ebea8702ccdd486f Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Wed, 22 Mar 2023 09:56:39 -0700
Subject: [PATCH v1] Remove vacuum_defer_cleanup_age
Discussion: https://postgr.es/m/[email protected]
---
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 | 19 +---
8 files changed, 11 insertions(+), 165 deletions(-)
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 2effdea126f..4bc23adf516 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 9a73ae67d0b..accdb3d2530 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -37,7 +37,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 1c0583fe267..30cce4fea73 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2536,15 +2536,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 d06074b86f6..7c4e2ace4af 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -324,7 +324,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 481f93cea1b..eb0531dbf17 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4548,41 +4548,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..d3451fcd58b 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -945,7 +945,7 @@ primary_conninfo = 'host=192.168.1.50 port=5432 user=foo password=foopass'
</para>
<para>
Similarly, <xref linkend="guc-hot-standby-feedback"/>
- and <xref linkend="guc-vacuum-defer-cleanup-age"/> provide protection against
+ provides 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
@@ -1910,17 +1910,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 +2246,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 +2256,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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
2023-03-18 09:33 Re: Should we remove vacuum_defer_cleanup_age? Alvaro Herrera <[email protected]>
@ 2023-03-24 21:27 ` Peter Geoghegan <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Peter Geoghegan @ 2023-03-24 21:27 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
On Sat, Mar 18, 2023 at 2:34 AM Alvaro Herrera <[email protected]> wrote:
>
> On 2023-Mar-17, Andres Freund wrote:
>
> > I started writing a test for vacuum_defer_cleanup_age while working on the fix
> > referenced above, but now I am wondering if said energy would be better spent
> > removing vacuum_defer_cleanup_age alltogether.
>
> +1 I agree it's not useful anymore.
+1.
I am suspicious of most of the GUCs whose value is an XID age. It
strikes me as something that is convenient to the implementation, but
not to the user, since there are so many ways that XID age might be a
poor proxy for whatever it is that you really care about in each case.
A theoretical advantage of vacuum_defer_cleanup_age is that it allows
the user to control things in terms of the impact on the primary --
whereas hot_standby_feedback is a mechanism that controls things in
terms of the needs of the standby. In practice this is pretty useless,
but it seems like it might be possible to come up with some other new
mechanism that somehow does this in a way that's truly useful.
Something that allows the user to constrain how far we hold back
conflicts/vacuuming in terms of the *impact* on the primary.
It might be helpful to permit opportunistic cleanup by pruning and
index deletion at some point, but to throttle it when we know it would
violate some soft limit related to hot_standby_feedback. Maybe the
system could prevent the first few attempts at pruning when it
violates the soft limit, or make pruning prune somewhat less
aggressively where there is little advantage to it in terms of
space/tuples freed -- decide on what to do at the very last minute,
based on all available information at that late stage, with the full
context available. The system could be taught to be very patient at
first, when relatively few pruning operations have been attempted,
when the cost is basically still acceptable. But as more pruning
operations ran and clearly didn't free space that really should be
freed, we'd quickly lose patience.
The big idea here is to delay committing to any course of action for
as long as possible, so we wouldn't kill queries on standbys for very
little benefit on the primary, while at the same time avoiding ever
really failing to kill queries on standbys when the cost proved too
high on the primary. For this to have any chance of working it needs
to focus on the actual costs on the primary, and not some extremely
noisy proxy for that cost. The standby will have its query killed by
just one prune record affecting just one heap page, and delaying that
specific prune record is likely no big deal. It's preventing pruning
of tens of thousands of heap pages that we need to worry about.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-03-24 21:27 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 07:58 [PATCH v56 2/6] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]>
2023-03-18 09:33 Re: Should we remove vacuum_defer_cleanup_age? Alvaro Herrera <[email protected]>
2023-03-22 16:44 ` Re: Should we remove vacuum_defer_cleanup_age? Justin Pryzby <[email protected]>
2023-03-22 17:00 ` Re: Should we remove vacuum_defer_cleanup_age? Andres Freund <[email protected]>
2023-03-24 21:27 ` Re: Should we remove vacuum_defer_cleanup_age? Peter Geoghegan <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox