public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mihail Nikalayeu <[email protected]>
To: Antonin Houska <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Robert Treat <[email protected]>
Subject: Re: Adding REPACK [concurrently]
Date: Sun, 1 Feb 2026 20:46:31 +0100
Message-ID: <CADzfLwUukiGOPoUkDgf6oEB-Y0TnNy6UFUN4obnU-AN5W1N=sw@mail.gmail.com> (raw)
In-Reply-To: <57210.1769801636@localhost>
References: <[email protected]>
<11247.1767609087@localhost>
<11558.1767609632@localhost>
<141054.1767891540@localhost>
<CADzfLwU-OmxW3t3AoQo9=K7uq4G1yZ-txcetzW3jbcVxV_pJew@mail.gmail.com>
<137668.1768235610@localhost>
<CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
<CADzfLwXJ+4s1tJuG9injcxAUP3urj9D6dUAPOCaX33UeiUxrRQ@mail.gmail.com>
<74802.1769071060@localhost>
<CADzfLwVZ_DeU_3avD=G4ZHFJJgZ0EOFzxnmWxwyB23zsS-uxjA@mail.gmail.com>
<CADzfLwUEH5+LjCN+6kRfSsXwuou8rKXyVV42Wi-O_TG0360Kug@mail.gmail.com>
<3901.1769412880@localhost>
<88003.1769511456@localhost>
<CADzfLwXdaJh4awQstc2PpBz=EBBc6tMA50wYLqMoEtY5B+WUnA@mail.gmail.com>
<57210.1769801636@localhost>
Hello!
PART 1:
--------------
Something still wrong with 0006, check:
'pgbench: error: client 12 script 0 aborted in command 2 query 0: ERROR:
attempted to overwrite invisible tuple
https://cirrus-ci.com/task/6385612527239168?logs=test_world#L300
But it is hard to reproduce - happened once.
--------------
Also, once I got
[16:25:18.641] # at /tmp/cirrus-ci-build/contrib/amcheck/t/
007_repack_concurrently.pl line 57.
[16:25:18.641] # 'pgbench: error: client 6 script 0
aborted in command 2 query 0: ERROR: relation 21856 deleted while still in
use
https://cirrus-ci.com/task/4686014242881536?logs=test_world#L384
It was the PROC_IN_REPACK version (see below), but I think it is not
related to it. But I'm not 100% sure.
PART 2:
> I'm considering a special kind of relation whose catalog entries remain
in the
> catalog cache and are never written to the catalog tables. (Unlike
temporary
> relation, it'd be WAL logged so that REPACK can be replayed on standby.)
I think it is too complicated, especially including replication logic.
Approach with catalog-only xid is much simpler, it was even committed (yes,
reverted but because of another reason).
Essentially we have two issues:
1) make sure catalog entities are not dropped because the vacuum
2) make sure data in new table is not vacuumed also
For the first PROC_IN_REPACK is enough.
For second - depends if MVCC-safe (original xmin/xmax) are preserved. If
yes - looks like nothing more needed.
If not - just prevent the vacuum from touching the table (but, looks like
it is done already, because lock is held on NewHeap until commit).
And additionally reset snapshots during the index building itself, but it
is scope of another patch.
I have implemented PROC_IN_REPACK POC in the attached patch.
Also, I am still not sure if MVCC-safe implementation is worth
its complexity compared with "relcheckxmin"approach [0].
[0]:
https://www.postgresql.org/message-id/CADzfLwUEH5%2BLjCN%2B6kRfSsXwuou8rKXyVV42Wi-O_TG0360Kug%40mail...
Best regards,
Mikhail.
Attachments:
[application/octet-stream] vX-0001-Handle-VACUUM-interaction-with-REPACK-CONCURRENTL.patch (6.8K, 3-vX-0001-Handle-VACUUM-interaction-with-REPACK-CONCURRENTL.patch)
download | inline diff:
From 2edaf6d9a427c9f683989bd8eb1ed4da9579541a Mon Sep 17 00:00:00 2001
From: Mikhail Nikalayeu <[email protected]>
Date: Sun, 1 Feb 2026 14:21:41 +0100
Subject: [PATCH vX] Handle VACUUM interaction with REPACK CONCURRENTLY and
separate catalog/data horizons, based on d9d076222f5
---
src/backend/commands/cluster.c | 32 ++++++++++++++++++++++++-
src/backend/storage/ipc/procarray.c | 36 +++++++++++++++++++++++------
src/include/storage/proc.h | 6 +++--
3 files changed, 64 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 403bdcda32e..5afd6e8088f 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -292,7 +292,7 @@ static void export_snapshot(Snapshot snapshot,
DecodingWorkerShared *shared);
static void ProcessRepackMessage(StringInfo msg);
static const char *RepackCommandAsString(RepackCommand cmd);
-
+static void set_in_repack_procflags(void);
#define REPL_PLUGIN_NAME "pgoutput_repack"
@@ -355,6 +355,14 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location));
}
+ if ((params.options & CLUOPT_CONCURRENT) != 0)
+ {
+ InvalidateCatalogSnapshot();
+ PopActiveSnapshot();
+ set_in_repack_procflags();
+ PushActiveSnapshot(GetTransactionSnapshot());
+ }
+
/*
* Determine the lock mode expected by cluster_rel().
*
@@ -509,6 +517,12 @@ ExecRepack(ParseState *pstate, RepackStmt *stmt, bool isTopLevel)
continue;
}
+ if ((params.options & CLUOPT_CONCURRENT) != 0)
+ {
+ InvalidateCatalogSnapshot();
+ set_in_repack_procflags();
+ }
+
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
@@ -4357,3 +4371,19 @@ ProcessRepackMessage(StringInfo msg)
}
}
}
+
+static void
+set_in_repack_procflags(void)
+{
+ /*
+ * This should only be called before installing xid or xmin in MyProc;
+ * otherwise, concurrent processes could see an Xmin that moves backwards.
+ */
+ Assert(MyProc->xid == InvalidTransactionId &&
+ MyProc->xmin == InvalidTransactionId);
+
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->statusFlags |= PROC_IN_REPACK;
+ ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+ LWLockRelease(ProcArrayLock);
+}
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 6be565155ab..9cede06338a 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1642,7 +1642,12 @@ TransactionIdIsInProgress(TransactionId xid)
* relations that's not required, since only backends in my own database could
* ever see the tuples in them. Also, we can ignore concurrently running lazy
* VACUUMs because (a) they must be working on other tables, and (b) they
- * don't need to do snapshot-based lookups.
+ * don't need to do snapshot-based lookups. Similarly, for the non-catalog
+ * horizon, we can ignore REPACK CONCURRENTLY for the
+ * same reasons and because they can't run in transaction blocks. (They are
+ * not possible to ignore for catalogs, because REPACK do some catalog
+ * operations.) Do note that this means that REPACK must use a lock level
+ * that conflicts with VACUUM.
*
* This also computes a horizon used to truncate pg_subtrans. For that
* backends in all databases have to be considered, and concurrently running
@@ -1689,9 +1694,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
bool in_recovery = RecoveryInProgress();
TransactionId *other_xids = ProcGlobal->xids;
- /* inferred after ProcArrayLock is released */
- h->catalog_oldest_nonremovable = InvalidTransactionId;
-
LWLockAcquire(ProcArrayLock, LW_SHARED);
h->latest_completed = TransamVariables->latestCompletedXid;
@@ -1711,6 +1713,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
h->oldest_considered_running = initial;
h->shared_oldest_nonremovable = initial;
+ h->catalog_oldest_nonremovable = initial;
h->data_oldest_nonremovable = initial;
/*
@@ -1809,11 +1812,26 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
(statusFlags & PROC_AFFECTS_ALL_HORIZONS) ||
in_recovery)
{
- h->data_oldest_nonremovable =
- TransactionIdOlder(h->data_oldest_nonremovable, xmin);
+ /*
+ * We can ignore this backend if it's running REPACK
+ * CONCURRENTLY - but
+ * only on vacuums of user-defined tables.
+ */
+ if (!(statusFlags & PROC_IN_REPACK))
+ h->data_oldest_nonremovable =
+ TransactionIdOlder(h->data_oldest_nonremovable, xmin);
+
+ /* Catalog tables need to consider all backends in this db */
+ h->catalog_oldest_nonremovable =
+ TransactionIdOlder(h->catalog_oldest_nonremovable, xmin);
+
}
}
+ /* catalog horizon should never be later than data */
+ Assert(TransactionIdPrecedesOrEquals(h->catalog_oldest_nonremovable,
+ h->data_oldest_nonremovable));
+
/*
* If in recovery fetch oldest xid in KnownAssignedXids, will be applied
* after lock is released.
@@ -1835,6 +1853,8 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
TransactionIdOlder(h->shared_oldest_nonremovable, kaxmin);
h->data_oldest_nonremovable =
TransactionIdOlder(h->data_oldest_nonremovable, kaxmin);
+ h->catalog_oldest_nonremovable =
+ TransactionIdOlder(h->catalog_oldest_nonremovable, kaxmin);
/* temp relations cannot be accessed in recovery */
}
@@ -1862,7 +1882,9 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
h->shared_oldest_nonremovable =
TransactionIdOlder(h->shared_oldest_nonremovable,
h->slot_catalog_xmin);
- h->catalog_oldest_nonremovable = h->data_oldest_nonremovable;
+ h->catalog_oldest_nonremovable =
+ TransactionIdOlder(h->catalog_oldest_nonremovable,
+ h->slot_xmin);
h->catalog_oldest_nonremovable =
TransactionIdOlder(h->catalog_oldest_nonremovable,
h->slot_catalog_xmin);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 039bc8353be..5396680d2b9 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -66,16 +66,18 @@ struct XidCache
#define PROC_AFFECTS_ALL_HORIZONS 0x20 /* this proc's xmin must be
* included in vacuum horizons
* in all databases */
+#define PROC_IN_REPACK 0x40
+
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
- (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND)
+ (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND | PROC_IN_REPACK)
/*
* Xmin-related flags. Make sure any flags that affect how the process' Xmin
* value is interpreted by VACUUM are included here.
*/
-#define PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC)
+#define PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_IN_REPACK)
/*
* We allow a limited number of "weak" relation locks (AccessShareLock,
--
2.43.0
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Adding REPACK [concurrently]
In-Reply-To: <CADzfLwUukiGOPoUkDgf6oEB-Y0TnNy6UFUN4obnU-AN5W1N=sw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox