public inbox for [email protected]
help / color / mirror / Atom feedRe: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
8+ messages / 6 participants
[nested] [flat]
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 15:15 Robert Haas <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Robert Haas @ 2022-04-11 15:15 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Georgios <[email protected]>
On Mon, Apr 11, 2022 at 2:52 AM Michael Paquier <[email protected]> wrote:
> Since babbbb5 and the introduction of LZ4, I have reworked the way
> compression is controlled for pg_receivewal, with two options:
> - --compress-method, settable to "gzip", "none" or "lz4".
> - --compress, to pass down a compression level, where the allowed
> range is 1-9. If passing down 0, we'd get an error rather than
> implying no compression, contrary to what we did in ~14.
>
> I initially thought that this was fine as-is, but then Robert and
> others have worked on client/server compression for pg_basebackup,
> introducing a much better design with centralized APIs where one can
> use METHOD:DETAIL for as compression value, where DETAIL is a
> comma-separated list of keyword=value (keyword = "level" or
> "workers"), with centralized checks and an extensible design.
>
> This is something I think we had better fix before beta1, because now
> we have binaries that use an inconsistent set of options. So,
> attached is a patch set aimed at rework this option set from the
> ground, taking advantage of the recent work done by Robert and others
> for pg_basebackup:
+1 for this in general, but I think that naming like
"compression_algo" stinks. If you think "compression_algorithm" is too
long, I think you should use "algorithm" or "compression" or
"compression_method" or something.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h}
@ 2022-04-11 22:50 Michael Paquier <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2022-04-11 22:50 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Postgres hackers <[email protected]>; Georgios <[email protected]>
On Mon, Apr 11, 2022 at 11:15:46AM -0400, Robert Haas wrote:
> +1 for this in general, but I think that naming like
> "compression_algo" stinks. If you think "compression_algorithm" is too
> long, I think you should use "algorithm" or "compression" or
> "compression_method" or something.
Yes, I found "compression_algorithm" to be too long initially. For
walmethods.c and pg_receivewal.c, it may be better to just stick to
"algorithm" then, at least that's consistent with pg_basebackup.c.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
@ 2026-04-03 19:20 Tomas Vondra <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Tomas Vondra @ 2026-04-03 19:20 UTC (permalink / raw)
To: David Rowley <[email protected]>; [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi,
I'm working on adding information about prefetching for scans [1], which
includes BitmapHeapScan. I realized the instrumentation added by this
thread may not be quite right, resulting in missing instrumentation for
non-parallel-aware scans in a parallel query.
A better description / explanation of the issue is posted here [2]. I've
posted a proposed fix in the following message [3], in a patch:
v8-0002-Show-Bitmap-Heap-Scan-stats-for-non-parallel-awar.patch
I wonder if someone from this thread could review my analysis, and
confirm this is not intentional. I don't see it discussed in the thread,
so I assume no one noticed this behavior. I'd also appreciate a review
of the proposed fix, or suggestions for alternative fixes.
regards
[1]
https://www.postgresql.org/message-id/a177a6dd-240b-455a-8f25-aca0b1c08c6e%40vondra.me
[2]
https://www.postgresql.org/message-id/3bdbc70d-ad44-494a-8aab-868b5066fe8b%40vondra.me
[3]
https://www.postgresql.org/message-id/fce326bb-1210-4d48-8c97-bb3bca396eba%40vondra.me
--
Tomas Vondra
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
@ 2026-04-05 18:27 Melanie Plageman <[email protected]>
parent: Tomas Vondra <[email protected]>
0 siblings, 2 replies; 8+ messages in thread
From: Melanie Plageman @ 2026-04-05 18:27 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On Fri, Apr 3, 2026 at 3:20 PM Tomas Vondra <[email protected]> wrote:
>
> I'm working on adding information about prefetching for scans [1], which
> includes BitmapHeapScan. I realized the instrumentation added by this
> thread may not be quite right, resulting in missing instrumentation for
> non-parallel-aware scans in a parallel query.
>
> A better description / explanation of the issue is posted here [2]. I've
> posted a proposed fix in the following message [3], in a patch:
>
> v8-0002-Show-Bitmap-Heap-Scan-stats-for-non-parallel-awar.patch
>
> I wonder if someone from this thread could review my analysis, and
> confirm this is not intentional. I don't see it discussed in the thread,
> so I assume no one noticed this behavior. I'd also appreciate a review
> of the proposed fix, or suggestions for alternative fixes.
I can't imagine this was intentional.
I reviewed your approach and suggest we aim for even lower impact by
always allocating the ParallelBitmapHeapState. That means the DSM
layout won't differ such that pcxt->toc has to point to the
instrumentation in the parallel-oblivious case and the pstate in the
parallel-aware case. Attached is a patch that does this.
- Melanie
Attachments:
[text/x-patch] 0001-Allow-non-parallel-aware-bitmap-table-scans-to-share.patch (6.3K, ../../CAAKRu_a_c8HAtJ8Ynz-dU=Jb2PzheW0zWME6A1BB9jQ62DMZBg@mail.gmail.com/2-0001-Allow-non-parallel-aware-bitmap-table-scans-to-share.patch)
download | inline diff:
From 695215d4ada297ca31034c3c13f4d491f0c25a9a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Sun, 5 Apr 2026 14:10:33 -0400
Subject: [PATCH] Allow non-parallel-aware bitmap table scans to share
instrumentation in the DSM
EXPLAIN ANALYZE for non-parallel-aware bitmap table scans did not show
exact/lossy pages because the DSM where the stats would be read from
wasn't initialized. This affected queries like bitmap table scans on the
outer side of a parallel join or bitmap table scans with
debug_parallel_query=regress. Fix it by setting up the DSM if
instrumentation is needed even if the node is not parallel aware.
---
src/backend/commands/explain.c | 2 +-
src/backend/executor/execParallel.c | 18 ++++----
src/backend/executor/nodeBitmapHeapscan.c | 50 +++++++++++++++--------
3 files changed, 43 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e4b70166b0e..8275bb2af61 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -3949,7 +3949,7 @@ show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es)
}
/* Display stats for each parallel worker */
- if (planstate->pstate != NULL)
+ if (planstate->sinstrument != NULL)
{
for (int n = 0; n < planstate->sinstrument->num_workers; n++)
{
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index 755191b51ef..ce377a774f8 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -290,9 +290,9 @@ ExecParallelEstimate(PlanState *planstate, ExecParallelEstimateContext *e)
e->pcxt);
break;
case T_BitmapHeapScanState:
- if (planstate->plan->parallel_aware)
- ExecBitmapHeapEstimate((BitmapHeapScanState *) planstate,
- e->pcxt);
+ /* even when not parallel-aware, for EXPLAIN ANALYZE */
+ ExecBitmapHeapEstimate((BitmapHeapScanState *) planstate,
+ e->pcxt);
break;
case T_HashJoinState:
if (planstate->plan->parallel_aware)
@@ -522,9 +522,9 @@ ExecParallelInitializeDSM(PlanState *planstate,
d->pcxt);
break;
case T_BitmapHeapScanState:
- if (planstate->plan->parallel_aware)
- ExecBitmapHeapInitializeDSM((BitmapHeapScanState *) planstate,
- d->pcxt);
+ /* even when not parallel-aware, for EXPLAIN ANALYZE */
+ ExecBitmapHeapInitializeDSM((BitmapHeapScanState *) planstate,
+ d->pcxt);
break;
case T_HashJoinState:
if (planstate->plan->parallel_aware)
@@ -1400,9 +1400,9 @@ ExecParallelInitializeWorker(PlanState *planstate, ParallelWorkerContext *pwcxt)
pwcxt);
break;
case T_BitmapHeapScanState:
- if (planstate->plan->parallel_aware)
- ExecBitmapHeapInitializeWorker((BitmapHeapScanState *) planstate,
- pwcxt);
+ /* even when not parallel-aware, for EXPLAIN ANALYZE */
+ ExecBitmapHeapInitializeWorker((BitmapHeapScanState *) planstate,
+ pwcxt);
break;
case T_HashJoinState:
if (planstate->plan->parallel_aware)
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 73831aed451..7ae348ef1f1 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -103,9 +103,9 @@ BitmapTableScanSetup(BitmapHeapScanState *node)
{
TBMIterator tbmiterator = {0};
ParallelBitmapHeapState *pstate = node->pstate;
- dsa_area *dsa = node->ss.ps.state->es_query_dsa;
+ bool parallel_aware = node->ss.ps.plan->parallel_aware;
- if (!pstate)
+ if (!parallel_aware)
{
node->tbm = (TIDBitmap *) MultiExecProcNode(outerPlanState(node));
@@ -133,8 +133,9 @@ BitmapTableScanSetup(BitmapHeapScanState *node)
BitmapDoneInitializingSharedState(pstate);
}
- tbmiterator = tbm_begin_iterate(node->tbm, dsa,
- pstate ?
+ tbmiterator = tbm_begin_iterate(node->tbm,
+ node->ss.ps.state->es_query_dsa,
+ parallel_aware ?
pstate->tbmiterator :
InvalidDsaPointer);
@@ -497,6 +498,12 @@ ExecBitmapHeapEstimate(BitmapHeapScanState *node,
{
Size size;
+ if (!node->ss.ps.instrument && !node->ss.ps.plan->parallel_aware)
+ {
+ /* No DSM required by the scan */
+ return;
+ }
+
size = MAXALIGN(sizeof(ParallelBitmapHeapState));
/* account for instrumentation, if required */
@@ -522,13 +529,14 @@ ExecBitmapHeapInitializeDSM(BitmapHeapScanState *node,
{
ParallelBitmapHeapState *pstate;
SharedBitmapHeapInstrumentation *sinstrument = NULL;
- dsa_area *dsa = node->ss.ps.state->es_query_dsa;
char *ptr;
Size size;
- /* If there's no DSA, there are no workers; initialize nothing. */
- if (dsa == NULL)
+ if (!node->ss.ps.instrument && !node->ss.ps.plan->parallel_aware)
+ {
+ /* No DSM required by the scan */
return;
+ }
size = MAXALIGN(sizeof(ParallelBitmapHeapState));
if (node->ss.ps.instrument && pcxt->nworkers > 0)
@@ -543,13 +551,18 @@ ExecBitmapHeapInitializeDSM(BitmapHeapScanState *node,
if (node->ss.ps.instrument && pcxt->nworkers > 0)
sinstrument = (SharedBitmapHeapInstrumentation *) ptr;
- pstate->tbmiterator = 0;
-
- /* Initialize the mutex */
- SpinLockInit(&pstate->mutex);
- pstate->state = BM_INITIAL;
+ pstate->tbmiterator = InvalidDsaPointer;
- ConditionVariableInit(&pstate->cv);
+ /*
+ * Only initialize these fields when parallel-aware as they are used to
+ * coordinate TBM iteration amongst parallel workers.
+ */
+ if (node->ss.ps.plan->parallel_aware)
+ {
+ SpinLockInit(&pstate->mutex);
+ pstate->state = BM_INITIAL;
+ ConditionVariableInit(&pstate->cv);
+ }
if (sinstrument)
{
@@ -578,9 +591,8 @@ ExecBitmapHeapReInitializeDSM(BitmapHeapScanState *node,
ParallelBitmapHeapState *pstate = node->pstate;
dsa_area *dsa = node->ss.ps.state->es_query_dsa;
- /* If there's no DSA, there are no workers; do nothing. */
- if (dsa == NULL)
- return;
+ Assert(node->ss.ps.plan->parallel_aware);
+ Assert(dsa != NULL);
pstate->state = BM_INITIAL;
@@ -602,7 +614,11 @@ ExecBitmapHeapInitializeWorker(BitmapHeapScanState *node,
{
char *ptr;
- Assert(node->ss.ps.state->es_query_dsa != NULL);
+ if (!node->ss.ps.instrument && !node->ss.ps.plan->parallel_aware)
+ {
+ /* No DSM required by the scan */
+ return;
+ }
ptr = shm_toc_lookup(pwcxt->toc, node->ss.ps.plan->plan_node_id, false);
--
2.43.0
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
@ 2026-04-05 19:40 Tomas Vondra <[email protected]>
parent: Melanie Plageman <[email protected]>
1 sibling, 0 replies; 8+ messages in thread
From: Tomas Vondra @ 2026-04-05 19:40 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
On 4/5/26 20:27, Melanie Plageman wrote:
> On Fri, Apr 3, 2026 at 3:20 PM Tomas Vondra <[email protected]> wrote:
>>
>> I'm working on adding information about prefetching for scans [1], which
>> includes BitmapHeapScan. I realized the instrumentation added by this
>> thread may not be quite right, resulting in missing instrumentation for
>> non-parallel-aware scans in a parallel query.
>>
>> A better description / explanation of the issue is posted here [2]. I've
>> posted a proposed fix in the following message [3], in a patch:
>>
>> v8-0002-Show-Bitmap-Heap-Scan-stats-for-non-parallel-awar.patch
>>
>> I wonder if someone from this thread could review my analysis, and
>> confirm this is not intentional. I don't see it discussed in the thread,
>> so I assume no one noticed this behavior. I'd also appreciate a review
>> of the proposed fix, or suggestions for alternative fixes.
>
> I can't imagine this was intentional.
>
Indeed.
> I reviewed your approach and suggest we aim for even lower impact by
> always allocating the ParallelBitmapHeapState. That means the DSM
> layout won't differ such that pcxt->toc has to point to the
> instrumentation in the parallel-oblivious case and the pstate in the
> parallel-aware case. Attached is a patch that does this.
>
I like this approach - it's much simpler / less invasive. It did not
occur to me to use the parallel_aware in BitmapTableScanSetup.
regards
--
Tomas Vondra
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
@ 2026-04-16 06:49 David Geier <[email protected]>
parent: Melanie Plageman <[email protected]>
1 sibling, 0 replies; 8+ messages in thread
From: David Geier @ 2026-04-16 06:49 UTC (permalink / raw)
To: Melanie Plageman <[email protected]>; Tomas Vondra <[email protected]>; +Cc: David Rowley <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Hi Tomas!
On 05.04.2026 20:27, Melanie Plageman wrote:
> On Fri, Apr 3, 2026 at 3:20 PM Tomas Vondra <[email protected]> wrote:
>>
>> I'm working on adding information about prefetching for scans [1], which
>> includes BitmapHeapScan. I realized the instrumentation added by this
>> thread may not be quite right, resulting in missing instrumentation for
>> non-parallel-aware scans in a parallel query.
>>
>> A better description / explanation of the issue is posted here [2]. I've
>> posted a proposed fix in the following message [3], in a patch:
>>
>> v8-0002-Show-Bitmap-Heap-Scan-stats-for-non-parallel-awar.patch
I haven't spent a lot of time looking through the code and your patch
but I'm wondering why we're not rather missing a
ExecBitmapIndexScanInstrumentEstimate(), rather than calling
ExecBitmapIndexScanEstimate() also in the !parallel_aware case. All
other scans do it this way, so why do it differently for Bitmap Index Scan?
>> I wonder if someone from this thread could review my analysis, and
>> confirm this is not intentional. I don't see it discussed in the thread,
>> so I assume no one noticed this behavior. I'd also appreciate a review
>> of the proposed fix, or suggestions for alternative fixes.
>
> I can't imagine this was intentional.
Agreed.
--
David Geier
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix.
@ 2026-05-12 10:27 Antonin Houska <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Antonin Houska @ 2026-05-12 10:27 UTC (permalink / raw)
The fix: https://www.postgresql.org/message-id/77611.1778055944%40localhost
---
src/backend/access/transam/xact.c | 2 +
src/backend/commands/repack_worker.c | 2 +
src/test/isolation/isolationtester.c | 9 +-
.../expected/repack_running_xacts.out | 81 ++++++++++++
.../specs/repack_running_xacts.spec | 119 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/repack_running_xacts.out
create mode 100644 src/test/modules/injection_points/specs/repack_running_xacts.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5586fbe5b07..b63ee166028 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -2428,6 +2429,7 @@ CommitTransaction(void)
* must be done _before_ releasing locks we hold and _after_
* RecordTransactionCommit.
*/
+ INJECTION_POINT("before-end-transaction", NULL);
ProcArrayEndTransaction(MyProc, latestXid);
/*
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index c40f8c98e06..6835626d677 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -26,6 +26,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define REPL_PLUGIN_NAME "pgrepack"
@@ -233,6 +234,7 @@ repack_setup_logical_decoding(Oid relid)
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
*/
+ INJECTION_POINT("before-create-decoding-context", NULL);
ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME,
NIL,
true,
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 440c875b8ac..8f17ee412c9 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -216,15 +216,22 @@ main(int argc, char **argv)
* exactly expect concurrent use of test tables. However, autovacuum will
* occasionally take AccessExclusiveLock to truncate a table, and we must
* ignore that transient wait.
+ *
+ * If the session's backend is blocked, and if its background worker is
+ * waiting on an injection point, we assume that the injection point is
+ * the reason for the backend to be blocked. That's what we check in the
+ * second query of the UNION. XXX Should we use a separate query for that?
*/
initPQExpBuffer(&wait_query);
appendPQExpBufferStr(&wait_query,
+ "WITH blocking(res) AS ("
"SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
/* The spec syntax requires at least one session; assume that here. */
appendPQExpBufferStr(&wait_query, conns[1].backend_pid_str);
for (i = 2; i < nconns; i++)
appendPQExpBuffer(&wait_query, ",%s", conns[i].backend_pid_str);
- appendPQExpBufferStr(&wait_query, "}')");
+ appendPQExpBufferStr(&wait_query, "}') UNION "
+ "SELECT pg_catalog.pg_isolation_test_session_is_blocked(pid, '{}') FROM pg_stat_activity WHERE leader_pid=$1) SELECT bool_or(res) FROM blocking");
res = PQprepare(conns[0].conn, PREP_WAITING, wait_query.data, 0, NULL);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
diff --git a/src/test/modules/injection_points/expected/repack_running_xacts.out b/src/test/modules/injection_points/expected/repack_running_xacts.out
new file mode 100644
index 00000000000..271fe2b97cb
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_running_xacts.out
@@ -0,0 +1,81 @@
+Parsed test spec with 5 sessions
+
+starting permutation: repack s3_assign_xid wakeup_bcdc s4_changes s4_attach s4_commit s3_commit s5_assign_xid wakeup_bet s5_commit check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack:
+ REPACK (CONCURRENTLY) repack_test;
+ <waiting ...>
+step s3_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+
+step wakeup_bcdc:
+ SELECT injection_points_wakeup('before-create-decoding-context');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_changes:
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+
+step s4_attach:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s4_commit:
+ COMMIT;
+ <waiting ...>
+step s3_commit:
+ COMMIT;
+
+step s5_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+
+step wakeup_bet:
+ SELECT injection_points_wakeup('before-end-transaction');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step repack: <... completed>
+step s4_commit: <... completed>
+step s5_commit:
+ COMMIT;
+
+step check:
+ TABLE repack_test;
+
+i|j
+-+-
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_running_xacts.spec b/src/test/modules/injection_points/specs/repack_running_xacts.spec
new file mode 100644
index 00000000000..1f878514046
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_running_xacts.spec
@@ -0,0 +1,119 @@
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE repack_test(i int PRIMARY KEY, j int);
+ CREATE TABLE aux(i int);
+}
+
+teardown
+{
+ DROP TABLE repack_test;
+ DROP TABLE aux;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_attach('before-create-decoding-context', 'wait');
+}
+step repack
+{
+ REPACK (CONCURRENTLY) repack_test;
+}
+step check
+{
+ TABLE repack_test;
+}
+teardown
+{
+ SELECT injection_points_detach('before-create-decoding-context');
+}
+
+session s2
+step wakeup_bcdc
+{
+ SELECT injection_points_wakeup('before-create-decoding-context');
+}
+step wakeup_bet
+{
+ SELECT injection_points_wakeup('before-end-transaction');
+}
+
+session s3
+step s3_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+}
+step s3_commit
+{
+ COMMIT;
+}
+
+session s4
+step s4_changes
+{
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+}
+# Do not attach in the setup section, that would be too soon.
+step s4_attach
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+}
+step s4_commit
+{
+ COMMIT;
+}
+teardown
+{
+ SELECT injection_points_detach('before-end-transaction');
+}
+
+session s5
+step s5_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+}
+step s5_commit
+{
+ COMMIT;
+}
+
+permutation
+repack
+# Assign XID so that a running transaction prevents the snapshot builder from
+# reaching CONSISTENT state immediately. It will wait for this to complete
+# after having reached BUILDING_SNAPSHOT.
+s3_assign_xid
+# Let the decoding setup start.
+wakeup_bcdc
+# Likewise, the snapshot builder will wait for the s4's xact to complete after
+# having reached FULL_SNAPSHOT. This is the problematic transaction, so let it
+# do some changes.
+s4_changes
+# Attach to the 'before-end-transaction' injection point that s4 will need
+# during commit.
+s4_attach
+# Only write commit record for s4, but do not remove the xact from procarray
+# yet. Thus the snapshot builder still needs to wait.
+s4_commit
+# Let the snapshot builder proceed to FULL_SNAPSHOT.
+s3_commit
+# Start another transaction so that CONSISTENT is not reached "directly",
+# i.e. due to no running transaction. It's important here that builder->xmin
+# does not advance.
+s5_assign_xid
+# Remove s4 xact from procarray, and thus reach the CONSISTENT state. Since
+# the COMMIT appeared in WAL too early (i.e. when the snapshot builder state
+# did not allow decoding of COMMIT records yet), the snapshot builder will
+# consider s4 running. This is also due to returning from
+# SnapBuildProcessRunningXacts() too early, w/o advancing builder->xmin.
+wakeup_bet
+# s5 is not needed anymore
+s5_commit
+# Show that the data changes performed by s4 are lost.
+check
--
2.47.3
--=-=-=--
^ permalink raw reply [nested|flat] 8+ messages in thread
* [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix.
@ 2026-05-12 10:27 Antonin Houska <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Antonin Houska @ 2026-05-12 10:27 UTC (permalink / raw)
The fix: https://www.postgresql.org/message-id/77611.1778055944%40localhost
---
src/backend/access/transam/xact.c | 2 +
src/backend/commands/repack_worker.c | 2 +
src/test/isolation/isolationtester.c | 9 +-
.../expected/repack_running_xacts.out | 81 ++++++++++++
.../specs/repack_running_xacts.spec | 119 ++++++++++++++++++
5 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/injection_points/expected/repack_running_xacts.out
create mode 100644 src/test/modules/injection_points/specs/repack_running_xacts.spec
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5586fbe5b07..b63ee166028 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -65,6 +65,7 @@
#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/relmapper.h"
@@ -2428,6 +2429,7 @@ CommitTransaction(void)
* must be done _before_ releasing locks we hold and _after_
* RecordTransactionCommit.
*/
+ INJECTION_POINT("before-end-transaction", NULL);
ProcArrayEndTransaction(MyProc, latestXid);
/*
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index c40f8c98e06..6835626d677 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -26,6 +26,7 @@
#include "storage/ipc.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#define REPL_PLUGIN_NAME "pgrepack"
@@ -233,6 +234,7 @@ repack_setup_logical_decoding(Oid relid)
* Neither prepare_write nor do_write callback nor update_progress is
* useful for us.
*/
+ INJECTION_POINT("before-create-decoding-context", NULL);
ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME,
NIL,
true,
diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
index 440c875b8ac..8f17ee412c9 100644
--- a/src/test/isolation/isolationtester.c
+++ b/src/test/isolation/isolationtester.c
@@ -216,15 +216,22 @@ main(int argc, char **argv)
* exactly expect concurrent use of test tables. However, autovacuum will
* occasionally take AccessExclusiveLock to truncate a table, and we must
* ignore that transient wait.
+ *
+ * If the session's backend is blocked, and if its background worker is
+ * waiting on an injection point, we assume that the injection point is
+ * the reason for the backend to be blocked. That's what we check in the
+ * second query of the UNION. XXX Should we use a separate query for that?
*/
initPQExpBuffer(&wait_query);
appendPQExpBufferStr(&wait_query,
+ "WITH blocking(res) AS ("
"SELECT pg_catalog.pg_isolation_test_session_is_blocked($1, '{");
/* The spec syntax requires at least one session; assume that here. */
appendPQExpBufferStr(&wait_query, conns[1].backend_pid_str);
for (i = 2; i < nconns; i++)
appendPQExpBuffer(&wait_query, ",%s", conns[i].backend_pid_str);
- appendPQExpBufferStr(&wait_query, "}')");
+ appendPQExpBufferStr(&wait_query, "}') UNION "
+ "SELECT pg_catalog.pg_isolation_test_session_is_blocked(pid, '{}') FROM pg_stat_activity WHERE leader_pid=$1) SELECT bool_or(res) FROM blocking");
res = PQprepare(conns[0].conn, PREP_WAITING, wait_query.data, 0, NULL);
if (PQresultStatus(res) != PGRES_COMMAND_OK)
diff --git a/src/test/modules/injection_points/expected/repack_running_xacts.out b/src/test/modules/injection_points/expected/repack_running_xacts.out
new file mode 100644
index 00000000000..271fe2b97cb
--- /dev/null
+++ b/src/test/modules/injection_points/expected/repack_running_xacts.out
@@ -0,0 +1,81 @@
+Parsed test spec with 5 sessions
+
+starting permutation: repack s3_assign_xid wakeup_bcdc s4_changes s4_attach s4_commit s3_commit s5_assign_xid wakeup_bet s5_commit check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step repack:
+ REPACK (CONCURRENTLY) repack_test;
+ <waiting ...>
+step s3_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+
+step wakeup_bcdc:
+ SELECT injection_points_wakeup('before-create-decoding-context');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s4_changes:
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+
+step s4_attach:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s4_commit:
+ COMMIT;
+ <waiting ...>
+step s3_commit:
+ COMMIT;
+
+step s5_assign_xid:
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+
+step wakeup_bet:
+ SELECT injection_points_wakeup('before-end-transaction');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step repack: <... completed>
+step s4_commit: <... completed>
+step s5_commit:
+ COMMIT;
+
+step check:
+ TABLE repack_test;
+
+i|j
+-+-
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_running_xacts.spec b/src/test/modules/injection_points/specs/repack_running_xacts.spec
new file mode 100644
index 00000000000..1f878514046
--- /dev/null
+++ b/src/test/modules/injection_points/specs/repack_running_xacts.spec
@@ -0,0 +1,119 @@
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE repack_test(i int PRIMARY KEY, j int);
+ CREATE TABLE aux(i int);
+}
+
+teardown
+{
+ DROP TABLE repack_test;
+ DROP TABLE aux;
+ DROP EXTENSION injection_points;
+}
+
+session s1
+setup
+{
+ SELECT injection_points_attach('before-create-decoding-context', 'wait');
+}
+step repack
+{
+ REPACK (CONCURRENTLY) repack_test;
+}
+step check
+{
+ TABLE repack_test;
+}
+teardown
+{
+ SELECT injection_points_detach('before-create-decoding-context');
+}
+
+session s2
+step wakeup_bcdc
+{
+ SELECT injection_points_wakeup('before-create-decoding-context');
+}
+step wakeup_bet
+{
+ SELECT injection_points_wakeup('before-end-transaction');
+}
+
+session s3
+step s3_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (1);
+}
+step s3_commit
+{
+ COMMIT;
+}
+
+session s4
+step s4_changes
+{
+ BEGIN;
+ INSERT INTO repack_test(i, j) VALUES (1, 1);
+}
+# Do not attach in the setup section, that would be too soon.
+step s4_attach
+{
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('before-end-transaction', 'wait');
+}
+step s4_commit
+{
+ COMMIT;
+}
+teardown
+{
+ SELECT injection_points_detach('before-end-transaction');
+}
+
+session s5
+step s5_assign_xid
+{
+ BEGIN;
+ INSERT INTO aux VALUES (2);
+}
+step s5_commit
+{
+ COMMIT;
+}
+
+permutation
+repack
+# Assign XID so that a running transaction prevents the snapshot builder from
+# reaching CONSISTENT state immediately. It will wait for this to complete
+# after having reached BUILDING_SNAPSHOT.
+s3_assign_xid
+# Let the decoding setup start.
+wakeup_bcdc
+# Likewise, the snapshot builder will wait for the s4's xact to complete after
+# having reached FULL_SNAPSHOT. This is the problematic transaction, so let it
+# do some changes.
+s4_changes
+# Attach to the 'before-end-transaction' injection point that s4 will need
+# during commit.
+s4_attach
+# Only write commit record for s4, but do not remove the xact from procarray
+# yet. Thus the snapshot builder still needs to wait.
+s4_commit
+# Let the snapshot builder proceed to FULL_SNAPSHOT.
+s3_commit
+# Start another transaction so that CONSISTENT is not reached "directly",
+# i.e. due to no running transaction. It's important here that builder->xmin
+# does not advance.
+s5_assign_xid
+# Remove s4 xact from procarray, and thus reach the CONSISTENT state. Since
+# the COMMIT appeared in WAL too early (i.e. when the snapshot builder state
+# did not allow decoding of COMMIT records yet), the snapshot builder will
+# consider s4 running. This is also due to returning from
+# SnapBuildProcessRunningXacts() too early, w/o advancing builder->xmin.
+wakeup_bet
+# s5 is not needed anymore
+s5_commit
+# Show that the data changes performed by s4 are lost.
+check
--
2.47.3
--=-=-=--
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-05-12 10:27 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11 15:15 Re: Fixes for compression options of pg_receivewal and refactoring of backup_compression.{c,h} Robert Haas <[email protected]>
2022-04-11 22:50 ` Michael Paquier <[email protected]>
2026-04-03 19:20 Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE Tomas Vondra <[email protected]>
2026-04-05 18:27 ` Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE Melanie Plageman <[email protected]>
2026-04-05 19:40 ` Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE Tomas Vondra <[email protected]>
2026-04-16 06:49 ` Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE David Geier <[email protected]>
2026-05-12 10:27 [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix. Antonin Houska <[email protected]>
2026-05-12 10:27 [PATCH] Test to demonstrate bug in commit 0d3dba38c7 and to verify a fix. Antonin Houska <[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