public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2 1/1] pg_rewind: Fetch small files according to new size.
7+ messages / 6 participants
[nested] [flat]
* [PATCH v2 1/1] pg_rewind: Fetch small files according to new size.
@ 2021-01-22 13:16 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Heikki Linnakangas @ 2021-01-22 13:16 UTC (permalink / raw)
There's a race condition if a file changes in the source system after we
have collected the file list. If the file becomes larger, we only fetched
up to its original size. That can easily result in a truncated file.
That's not a problem for relation files, files in pg_xact, etc. because
any actions on them will be replayed from the WAL. However, configuration
files are affected.
This commit mitigates the race condition by fetching small files in
whole, even if they have grown. This is not a full fix: we still believe
the original file size for files larger than 1 MB. That should be enough
for configuration files, and doing more than that would require bigger
changes to the chunking logic in in libpq_source.c.
That mitigates the race condition if the file is modified between the
original scan of files and copying the file, but there's still a race
condition if a file is changed while it's being copied. That's a much
smaller window, though, and pg_basebackup has the same issue.
I ran into this while playing with pg_auto_failover, which frequently
uses ALTER SYSTEM, which update postgresql.auto.conf. Often, pg_rewind
would fail, because the postgresql.auto.conf file changed concurrently
and a partial version of it was copied to the target. The partial
file would fail to parse, preventing the server from starting up.
Reviewed-by: Cary Huang
Discussion: https://www.postgresql.org/message-id/f67feb24-5833-88cb-1020-19a4a2b83ac7%40iki.fi
---
src/bin/pg_rewind/libpq_source.c | 32 +++++++++++++
src/bin/pg_rewind/local_source.c | 76 +++++++++++++++++++++++++++----
src/bin/pg_rewind/pg_rewind.c | 5 +-
src/bin/pg_rewind/rewind_source.h | 13 ++++++
4 files changed, 112 insertions(+), 14 deletions(-)
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 86d2adcaee9..ff16add16f5 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -63,6 +63,7 @@ static void process_queued_fetch_requests(libpq_source *src);
/* public interface functions */
static void libpq_traverse_files(rewind_source *source,
process_file_callback_t callback);
+static void libpq_queue_fetch_file(rewind_source *source, const char *path, size_t len);
static void libpq_queue_fetch_range(rewind_source *source, const char *path,
off_t off, size_t len);
static void libpq_finish_fetch(rewind_source *source);
@@ -88,6 +89,7 @@ init_libpq_source(PGconn *conn)
src->common.traverse_files = libpq_traverse_files;
src->common.fetch_file = libpq_fetch_file;
+ src->common.queue_fetch_file = libpq_queue_fetch_file;
src->common.queue_fetch_range = libpq_queue_fetch_range;
src->common.finish_fetch = libpq_finish_fetch;
src->common.get_current_wal_insert_lsn = libpq_get_current_wal_insert_lsn;
@@ -307,6 +309,36 @@ libpq_traverse_files(rewind_source *source, process_file_callback_t callback)
PQclear(res);
}
+/*
+ * Queue up a request to fetch a file from remote system.
+ */
+static void
+libpq_queue_fetch_file(rewind_source *source, const char *path, size_t len)
+{
+ /*
+ * Truncate the target file immediately, and queue a request to fetch it
+ * from the source. If the file is small, smaller than MAX_CHUNK_SIZE,
+ * request fetching a full-sized chunk anyway, so that if the file has
+ * become larger in the source system, after we scanned the source
+ * directory, we still fetch the whole file. This only works for files up
+ * to MAX_CHUNK_SIZE, but that's good enough for small configuration files
+ * and such that are changed every now and then, but not WAL-logged.
+ * For larger files, we fetch up to the original size.
+ *
+ * Even with that mechanism, there is an inherent race condition if the
+ * file is modified at the same instant that we're copying it, so that we
+ * might copy a torn version of the file with one half from the old
+ * version and another half from the new. But pg_basebackup has the same
+ * problem, and it hasn't been problem in practice.
+ *
+ * It might seem more natural to truncate the file later, when we receive
+ * it from the source server, but then we'd need to track which
+ * fetch-requests are for a whole file.
+ */
+ open_target_file(path, true);
+ libpq_queue_fetch_range(source, path, 0, Max(len, MAX_CHUNK_SIZE));
+}
+
/*
* Queue up a request to fetch a piece of a file from remote system.
*/
diff --git a/src/bin/pg_rewind/local_source.c b/src/bin/pg_rewind/local_source.c
index 9c3491c3fba..1899d1cc4ae 100644
--- a/src/bin/pg_rewind/local_source.c
+++ b/src/bin/pg_rewind/local_source.c
@@ -29,8 +29,10 @@ static void local_traverse_files(rewind_source *source,
process_file_callback_t callback);
static char *local_fetch_file(rewind_source *source, const char *path,
size_t *filesize);
-static void local_fetch_file_range(rewind_source *source, const char *path,
- off_t off, size_t len);
+static void local_queue_fetch_file(rewind_source *source, const char *path,
+ size_t len);
+static void local_queue_fetch_range(rewind_source *source, const char *path,
+ off_t off, size_t len);
static void local_finish_fetch(rewind_source *source);
static void local_destroy(rewind_source *source);
@@ -43,7 +45,8 @@ init_local_source(const char *datadir)
src->common.traverse_files = local_traverse_files;
src->common.fetch_file = local_fetch_file;
- src->common.queue_fetch_range = local_fetch_file_range;
+ src->common.queue_fetch_file = local_queue_fetch_file;
+ src->common.queue_fetch_range = local_queue_fetch_range;
src->common.finish_fetch = local_finish_fetch;
src->common.get_current_wal_insert_lsn = NULL;
src->common.destroy = local_destroy;
@@ -65,12 +68,65 @@ local_fetch_file(rewind_source *source, const char *path, size_t *filesize)
return slurpFile(((local_source *) source)->datadir, path, filesize);
}
+/*
+ * Copy a file from source to target.
+ *
+ * 'len' is the expected length of the file.
+ */
+static void
+local_queue_fetch_file(rewind_source *source, const char *path, size_t len)
+{
+ const char *datadir = ((local_source *) source)->datadir;
+ PGAlignedBlock buf;
+ char srcpath[MAXPGPATH];
+ int srcfd;
+ size_t written_len;
+
+ snprintf(srcpath, sizeof(srcpath), "%s/%s", datadir, path);
+
+ /* Open source file for reading. */
+ srcfd = open(srcpath, O_RDONLY | PG_BINARY, 0);
+ if (srcfd < 0)
+ pg_fatal("could not open source file \"%s\": %m",
+ srcpath);
+
+ /* Truncate and open the target file for writing. */
+ open_target_file(path, true);
+
+ written_len = 0;
+ for (;;)
+ {
+ ssize_t read_len;
+
+ read_len = read(srcfd, buf.data, sizeof(buf));
+
+ if (read_len < 0)
+ pg_fatal("could not read file \"%s\": %m", srcpath);
+ else if (read_len == 0)
+ break; /* EOF reached */
+
+ write_target_range(buf.data, written_len, read_len);
+ written_len += read_len;
+ }
+
+ /*
+ * A local source is not expected to change while we're rewinding, so check
+ * that the size of the file matches our earlier expectation.
+ */
+ if (written_len != len)
+ pg_fatal("size of source file \"%s\" changed concurrently: " UINT64_FORMAT " bytes expected, " UINT64_FORMAT " copied",
+ srcpath, len, written_len);
+
+ if (close(srcfd) != 0)
+ pg_fatal("could not close file \"%s\": %m", srcpath);
+}
+
/*
* Copy a file from source to target, starting at 'off', for 'len' bytes.
*/
static void
-local_fetch_file_range(rewind_source *source, const char *path, off_t off,
- size_t len)
+local_queue_fetch_range(rewind_source *source, const char *path, off_t off,
+ size_t len)
{
const char *datadir = ((local_source *) source)->datadir;
PGAlignedBlock buf;
@@ -94,14 +150,14 @@ local_fetch_file_range(rewind_source *source, const char *path, off_t off,
while (end - begin > 0)
{
ssize_t readlen;
- size_t len;
+ size_t thislen;
if (end - begin > sizeof(buf))
- len = sizeof(buf);
+ thislen = sizeof(buf);
else
- len = end - begin;
+ thislen = end - begin;
- readlen = read(srcfd, buf.data, len);
+ readlen = read(srcfd, buf.data, thislen);
if (readlen < 0)
pg_fatal("could not read file \"%s\": %m", srcpath);
@@ -120,7 +176,7 @@ static void
local_finish_fetch(rewind_source *source)
{
/*
- * Nothing to do, local_fetch_file_range() copies the ranges immediately.
+ * Nothing to do, local_queue_fetch_range() copies the ranges immediately.
*/
}
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 359a6a587cb..9030a1505e3 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -537,10 +537,7 @@ perform_rewind(filemap_t *filemap, rewind_source *source,
break;
case FILE_ACTION_COPY:
- /* Truncate the old file out of the way, if any */
- open_target_file(entry->path, true);
- source->queue_fetch_range(source, entry->path,
- 0, entry->source_size);
+ source->queue_fetch_file(source, entry->path, entry->source_size);
break;
case FILE_ACTION_TRUNCATE:
diff --git a/src/bin/pg_rewind/rewind_source.h b/src/bin/pg_rewind/rewind_source.h
index 2da92dbff94..799b7c120ea 100644
--- a/src/bin/pg_rewind/rewind_source.h
+++ b/src/bin/pg_rewind/rewind_source.h
@@ -47,6 +47,19 @@ typedef struct rewind_source
void (*queue_fetch_range) (struct rewind_source *, const char *path,
off_t offset, size_t len);
+ /*
+ * Like queue_fetch_range(), but requests replacing the whole local file
+ * from the source system. 'len' is the expected length of the file,
+ * although when the source is a live server, the file may change
+ * concurrently. The implementation is not obliged to copy more than 'len'
+ * bytes, even if the file is larger. However, to avoid copying a
+ * truncated version of the file, which can cause trouble if e.g. a
+ * configuration file is modified concurrently, the implementation should
+ * try to copy the whole file, even if it's larger than expected.
+ */
+ void (*queue_fetch_file) (struct rewind_source *, const char *path,
+ size_t len);
+
/*
* Execute all requests queued up with queue_fetch_range().
*/
--
2.29.2
--------------79EE74DA33DF3081397629A9--
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-11 16:33 Justin Pryzby <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Justin Pryzby @ 2023-04-11 16:33 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-11 18:20 Andres Freund <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Andres Freund @ 2023-04-11 18:20 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]>
Hi,
On 2023-04-11 11:33:01 -0500, Justin Pryzby wrote:
> 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.
Added the RMT.
We really should have a [email protected] 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
Attachments:
[text/x-diff] v2-0001-Remove-vacuum_defer_cleanup_age.patch (15.8K, ../../[email protected]/2-v2-0001-Remove-vacuum_defer_cleanup_age.patch)
download | inline diff:
From c51c9e450b1b1342c0f6ff32e7e360677ccbc2c6 Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
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 protected]>
Reviewed-by: Justin Pryzby <[email protected]>
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 | 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
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-13 03:34 Amit Kapila <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Amit Kapila @ 2023-04-13 03:34 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers
On Tue, Apr 11, 2023 at 11:50 PM Andres Freund <[email protected]> wrote:
>
> On 2023-04-11 11:33:01 -0500, Justin Pryzby wrote:
> > 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.
>
> Added the RMT.
>
> We really should have a [email protected] alias...
>
> Updated patch attached. I think we should either apply something like that
> 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.
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-13 15:32 Jonathan S. Katz <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan S. Katz @ 2023-04-13 15:32 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers
On 4/12/23 11:34 PM, Amit Kapila wrote:
> On Tue, Apr 11, 2023 at 11:50 PM Andres Freund <[email protected]> wrote:
>>
>> On 2023-04-11 11:33:01 -0500, Justin Pryzby wrote:
>>> 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.
>>
>> Added the RMT.
>>
>> We really should have a [email protected] alias...
(I had thought something as much -- will reach out to pginfra about options)
>> Updated patch attached. I think we should either apply something like that
>> 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?
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
Attachments:
[application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-13 16:16 Jonathan S. Katz <[email protected]>
parent: Jonathan S. Katz <[email protected]>
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan S. Katz @ 2023-04-13 16:16 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers
On 4/13/23 11:32 AM, Jonathan S. Katz wrote:
> On 4/12/23 11:34 PM, Amit Kapila wrote:
>> On Tue, Apr 11, 2023 at 11:50 PM Andres Freund <[email protected]>
>> +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?
>
> 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.
(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
Attachments:
[application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature)
download
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age?
@ 2023-04-14 03:06 Laurenz Albe <[email protected]>
parent: Jonathan S. Katz <[email protected]>
0 siblings, 0 replies; 7+ messages in thread
From: Laurenz Albe @ 2023-04-14 03:06 UTC (permalink / raw)
To: Jonathan S. Katz <[email protected]>; Amit Kapila <[email protected]>; Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers
On Thu, 2023-04-13 at 12:16 -0400, Jonathan S. Katz wrote:
> On 4/13/23 11:32 AM, Jonathan S. Katz wrote:
> > On 4/12/23 11:34 PM, Amit Kapila wrote:
> > > On Tue, Apr 11, 2023 at 11:50 PM Andres Freund <[email protected]>
>
> > > +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?
> >
> > 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.
>
> [RMT hat]
>
> +1 for removing.
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 protected]
I can't say if they have a good use case for that parameter or not.
Yours,
Laurenz Albe
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2023-04-14 03:06 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-22 13:16 [PATCH v2 1/1] pg_rewind: Fetch small files according to new size. Heikki Linnakangas <[email protected]>
2023-04-11 16:33 Re: Should we remove vacuum_defer_cleanup_age? Justin Pryzby <[email protected]>
2023-04-11 18:20 ` Re: Should we remove vacuum_defer_cleanup_age? Andres Freund <[email protected]>
2023-04-13 03:34 ` Re: Should we remove vacuum_defer_cleanup_age? Amit Kapila <[email protected]>
2023-04-13 15:32 ` Re: Should we remove vacuum_defer_cleanup_age? Jonathan S. Katz <[email protected]>
2023-04-13 16:16 ` Re: Should we remove vacuum_defer_cleanup_age? Jonathan S. Katz <[email protected]>
2023-04-14 03:06 ` Re: Should we remove vacuum_defer_cleanup_age? Laurenz Albe <[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