On 2026-Mar-11, Antonin Houska wrote: > I'm not sure it can be fixed nicely in the REPACK (CONCURRENTLY) patch. I > think the problem is that, in the current tree, VARSIZE_ANY() is used in such > a way that the compiler cannot check the "array bounds". The restore_tuple() > function is special in that it uses VARSIZE_ANY() to check a variable > allocated from the stack, so the compiler can check the size. > > I'm trying to fix that in a new diff 0002 - the point is that VARSIZE_ANY() > should not need to dereference a pointer to varattrib_4b, since the size > information is always located at the beginning of the structure. Maybe you > have better idea.
I have no immediate ideas on this. I offer the following rather trivial fixup diffs, which I think should be mostly be for 0002. Other trivial things I'd like to change, if you don't mind, - the name pgoutput_repack sounds wrong to me. I would rather say rpck_output, repack_output, repack_plugin, ... or something. I don't understand where the suffix "output" comes from in the name "pgoutput", but it sounds like arbitrary nonsense to me. - I would rename the routines in that file to also have the name "repack", probably as prefixes. One thing we need and is rather not trivial, is to go through the table AM interface rather than calling heapam.c routines directly. I'm working on this now and will present a patch later. Another thing I noticed while going over the code was that we seem to spill whole tuples even for things like the old tuple of an UPDATE and for DELETE, but unless I misunderstand how this works, these shouldn't be necessary: we just need the replica identity so that we can locate the tuple to operate on. Especially for tuples that contain large toasted attributes, this is likely important. It may make sense to use the TupleTableSlot interface rather than HeapTuple for everything. I'm not really sure about this though. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Most hackers will be perfectly comfortable conceptualizing users as entropy sources, so let's move on." (Nathaniel Smith) https://mail.gnu.org/archive/html/monotone-devel/2007-01/msg00080.html
>From 4c41bf4a74a9666ca5c7d991253205dd286d2f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Wed, 11 Mar 2026 15:36:14 +0100 Subject: [PATCH 1/9] rewrite infinite loop in our style --- src/backend/commands/cluster.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 8b5571374d0..da4919e497a 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -4062,8 +4062,14 @@ repack_worker_internal(dsm_segment *seg) */ InvalidateCatalogSnapshot(); - while (!decode_concurrent_changes(decoding_ctx, shared)) - ; + for (;;) + { + bool stop = decode_concurrent_changes(decoding_ctx, shared); + + if (stop) + break; + + } /* Cleanup. */ cleanup_logical_decoding(decoding_ctx); -- 2.47.3
>From 9fb2a3254cf53f6bf551fe0917a33f6252574484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:05:01 +0100 Subject: [PATCH 2/9] rename setup_logical_decoding to repack_setup_logical_decoding --- src/backend/commands/cluster.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index da4919e497a..db3980b84f5 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -274,7 +274,7 @@ static List *get_tables_to_repack_partitioned(RepackCommand cmd, static bool repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid); -static LogicalDecodingContext *setup_logical_decoding(Oid relid); +static LogicalDecodingContext *repack_setup_logical_decoding(Oid relid); static bool decode_concurrent_changes(LogicalDecodingContext *ctx, DecodingWorkerShared *shared); static void apply_concurrent_changes(BufFile *file, ChangeDest *dest); @@ -612,7 +612,7 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, { /* * Make sure we have no XID assigned, otherwise call of - * setup_logical_decoding() can cause a deadlock. + * repack_setup_logical_decoding() can cause a deadlock. * * The existence of transaction block actually does not imply that XID * was already assigned, but it very likely is. We might want to check @@ -2620,7 +2620,7 @@ change_useless_for_repack(XLogRecordBuffer *buf) * crash by restarting all the work from scratch). */ static LogicalDecodingContext * -setup_logical_decoding(Oid relid) +repack_setup_logical_decoding(Oid relid) { Relation rel; Oid toastrelid; @@ -4044,7 +4044,7 @@ repack_worker_internal(dsm_segment *seg) /* * Prepare to capture the concurrent data changes ourselves. */ - decoding_ctx = setup_logical_decoding(shared->relid); + decoding_ctx = repack_setup_logical_decoding(shared->relid); /* Announce that we're ready. */ SpinLockAcquire(&shared->mutex); -- 2.47.3
>From f5abd4559fee34d3bff58d46d92a351792665274 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:09:20 +0100 Subject: [PATCH 3/9] remove excess parens around ereport --- src/backend/commands/cluster.c | 78 ++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index db3980b84f5..af47354e382 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -992,10 +992,10 @@ check_repack_concurrently_requirements(Relation rel, Oid *ident_idx_p) /* Data changes in system relations are not logically decoded. */ if (IsCatalogRelation(rel)) ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot repack relation \"%s\"", - RelationGetRelationName(rel)), - errhint("REPACK CONCURRENTLY is not supported for catalog relations."))); + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot repack relation \"%s\"", + RelationGetRelationName(rel)), + errhint("REPACK CONCURRENTLY is not supported for catalog relations.")); /* * reorderbuffer.c does not seem to handle processing of TOAST relation @@ -1003,28 +1003,28 @@ check_repack_concurrently_requirements(Relation rel, Oid *ident_idx_p) */ if (IsToastRelation(rel)) ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot repack relation \"%s\"", - RelationGetRelationName(rel)), - errhint("REPACK CONCURRENTLY is not supported for TOAST relations, unless the main relation is repacked too."))); + errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot repack relation \"%s\"", + RelationGetRelationName(rel)), + errhint("REPACK CONCURRENTLY is not supported for TOAST relations, unless the main relation is repacked too.")); relpersistence = rel->rd_rel->relpersistence; if (relpersistence != RELPERSISTENCE_PERMANENT) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot repack relation \"%s\"", - RelationGetRelationName(rel)), - errhint("REPACK CONCURRENTLY is only allowed for permanent relations."))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot repack relation \"%s\"", + RelationGetRelationName(rel)), + errhint("REPACK CONCURRENTLY is only allowed for permanent relations.")); /* With NOTHING, WAL does not contain the old tuple. */ replident = rel->rd_rel->relreplident; if (replident == REPLICA_IDENTITY_NOTHING) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot repack relation \"%s\"", - RelationGetRelationName(rel)), - errhint("Relation \"%s\" has insufficient replication identity.", - RelationGetRelationName(rel)))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot repack relation \"%s\"", + RelationGetRelationName(rel)), + errhint("Relation \"%s\" has insufficient replication identity.", + RelationGetRelationName(rel))); /* * If the identity index is not set due to replica identity being, PK @@ -1035,11 +1035,11 @@ check_repack_concurrently_requirements(Relation rel, Oid *ident_idx_p) ident_idx = rel->rd_pkindex; if (!OidIsValid(ident_idx)) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("cannot process relation \"%s\"", - RelationGetRelationName(rel)), - errhint("Relation \"%s\" has no identity index.", - RelationGetRelationName(rel)))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("cannot process relation \"%s\"", + RelationGetRelationName(rel)), + errhint("Relation \"%s\" has no identity index.", + RelationGetRelationName(rel))); *ident_idx_p = ident_idx; } @@ -2793,20 +2793,21 @@ decode_concurrent_changes(LogicalDecodingContext *ctx, ReadLocalXLogPageNoWaitPrivate *priv; if (errm) - ereport(ERROR, (errmsg("%s", errm))); + ereport(ERROR, + errmsg("%s", errm)); /* * In the decoding loop we do not want to get blocked when there * is no more WAL available, otherwise the loop would become * uninterruptible. */ - priv = (ReadLocalXLogPageNoWaitPrivate *) - ctx->reader->private_data; + priv = (ReadLocalXLogPageNoWaitPrivate *) ctx->reader->private_data; if (priv->end_of_wal) /* Do not miss the end of WAL condition next time. */ priv->end_of_wal = false; else - ereport(ERROR, (errmsg("could not read WAL record"))); + ereport(ERROR, + errmsg("could not read WAL record")); } /* @@ -2852,7 +2853,8 @@ decode_concurrent_changes(LogicalDecodingContext *ctx, timeout); if (res != WAIT_LSN_RESULT_SUCCESS && res != WAIT_LSN_RESULT_TIMEOUT) - ereport(ERROR, (errmsg("waiting for WAL failed"))); + ereport(ERROR, + errmsg("waiting for WAL failed")); } } @@ -3050,7 +3052,8 @@ apply_concurrent_update(Relation rel, HeapTuple tup, HeapTuple tup_target, &tmfd, &lockmode, &update_indexes, false /* wal_logical */ ); if (res != TM_Ok) - ereport(ERROR, (errmsg("failed to apply concurrent UPDATE"))); + ereport(ERROR, + errmsg("failed to apply concurrent UPDATE")); ExecStoreHeapTuple(tup, index_slot, false); @@ -3091,7 +3094,8 @@ apply_concurrent_delete(Relation rel, HeapTuple tup_target) false /* wal_logical */ ); if (res != TM_Ok) - ereport(ERROR, (errmsg("failed to apply concurrent DELETE"))); + ereport(ERROR, + errmsg("failed to apply concurrent DELETE")); pgstat_progress_incr_param(PROGRESS_REPACK_HEAP_TUPLES_DELETED, 1); } @@ -3576,7 +3580,7 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap, * Should not happen, given our lock on the old relation. */ ereport(ERROR, - (errmsg("identity index missing on the new relation"))); + errmsg("identity index missing on the new relation")); /* Gather information to apply concurrent changes. */ chgdst.rel = NewHeap; @@ -3864,9 +3868,9 @@ start_decoding_worker(Oid relid) decoding_worker = palloc0_object(DecodingWorker); if (!RegisterDynamicBackgroundWorker(&bgw, &decoding_worker->handle)) ereport(ERROR, - (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), - errmsg("out of background worker slots"), - errhint("You might need to increase \"%s\".", "max_worker_processes"))); + errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED), + errmsg("out of background worker slots"), + errhint("You might need to increase \"%s\".", "max_worker_processes")); decoding_worker->seg = seg; decoding_worker->error_mqh = mqh; @@ -3921,8 +3925,8 @@ stop_decoding_worker(void) if (status == BGWH_POSTMASTER_DIED) ereport(FATAL, - (errcode(ERRCODE_ADMIN_SHUTDOWN), - errmsg("postmaster exited during REPACK command"))); + errcode(ERRCODE_ADMIN_SHUTDOWN), + errmsg("postmaster exited during REPACK command")); shm_mq_detach(decoding_worker->error_mqh); @@ -3979,8 +3983,8 @@ RepackWorkerMain(Datum main_arg) seg = dsm_attach(DatumGetUInt32(main_arg)); if (seg == NULL) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("could not map dynamic shared memory segment"))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("could not map dynamic shared memory segment")); shared = (DecodingWorkerShared *) dsm_segment_address(seg); -- 2.47.3
>From 5b6f2f0d062458ee19c956977b3ad7912bc13073 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:09:55 +0100 Subject: [PATCH 4/9] XLogRecPtrIsInvalid -> XLogRecPtrIsValid --- src/backend/commands/cluster.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index af47354e382..29440fb75cd 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -2814,7 +2814,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx, * Whether we could read new record or not, keep checking if * 'lsn_upto' was specified. */ - if (XLogRecPtrIsInvalid(lsn_upto)) + if (!XLogRecPtrIsValid(lsn_upto)) { SpinLockAcquire(&shared->mutex); lsn_upto = shared->lsn_upto; @@ -2822,7 +2822,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx, done = shared->done; SpinLockRelease(&shared->mutex); } - if (!XLogRecPtrIsInvalid(lsn_upto) && + if (XLogRecPtrIsValid(lsn_upto) && ctx->reader->EndRecPtr >= lsn_upto) break; @@ -2846,7 +2846,7 @@ decode_concurrent_changes(LogicalDecodingContext *ctx, * If lsn_upto is valid, WAL records having LSN lower than that * should already have been flushed to disk. */ - if (XLogRecPtrIsInvalid(lsn_upto)) + if (!XLogRecPtrIsValid(lsn_upto)) timeout = 100L; res = WaitForLSN(WAIT_LSN_TYPE_PRIMARY_FLUSH, ctx->reader->EndRecPtr + 1, @@ -4039,7 +4039,7 @@ repack_worker_internal(dsm_segment *seg) * anything in the shared memory until we have serialized the snapshot. */ SpinLockAcquire(&shared->mutex); - Assert(XLogRecPtrIsInvalid(shared->lsn_upto)); + Assert(!XLogRecPtrIsValid(shared->lsn_upto)); sfs = &shared->sfs; SpinLockRelease(&shared->mutex); -- 2.47.3
>From d46fdf192945d15655186db137000ff209a64afd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:10:47 +0100 Subject: [PATCH 5/9] document store_change somewhat more --- .../replication/pgoutput_repack/pgoutput_repack.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c index 90f3a8975b9..79fc611b9ff 100644 --- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c +++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c @@ -158,7 +158,14 @@ plugin_change(LogicalDecodingContext *ctx, ReorderBufferTXN *txn, } } -/* Store concurrent data change. */ +/* + * For each change affecting the table being repacked, we store enough + * information about each tuple in it, so that it can be replayed in the + * new copy of the table. + * + * XXX for DELETE and the UPDATE OLD tuples, we could store just the + * replication identity instead of the full tuple. + */ static void store_change(LogicalDecodingContext *ctx, Relation relation, ConcurrentChangeKind kind, HeapTuple tuple) -- 2.47.3
>From a2b69f30588c519927092f08d7e5f6c769751a1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:12:13 +0100 Subject: [PATCH 6/9] use 'int' instead of 'uint32' for the result of list_length --- src/backend/commands/cluster.c | 2 +- src/backend/replication/pgoutput_repack/pgoutput_repack.c | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 29440fb75cd..244843ee1cf 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -3113,7 +3113,7 @@ restore_tuple(BufFile *file, Relation relation, MemoryContext cxt) uint32 t_len; HeapTuple tup; MemoryContext oldcxt; - uint32 natt_ext; + int natt_ext; List *attrs_ext = NIL; oldcxt = MemoryContextSwitchTo(cxt); diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c index 79fc611b9ff..61cd7c52ae6 100644 --- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c +++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c @@ -175,7 +175,7 @@ store_change(LogicalDecodingContext *ctx, Relation relation, BufFile *file; char kind_byte = (char) kind; List *attrs_ext = NIL; - uint32 natt_ext; + int natt_ext; dstate = (RepackDecodingState *) ctx->output_writer_private; file = dstate->file; @@ -237,14 +237,12 @@ store_change(LogicalDecodingContext *ctx, Relation relation, Assert(VARATT_IS_EXTERNAL_ONDISK(varlena_pointer)); } } - natt_ext = list_length(attrs_ext); } - else - natt_ext = 0; /* Write the number of external attributes. */ + natt_ext = list_length(attrs_ext); BufFileWrite(file, &natt_ext, sizeof(natt_ext)); - /* ... and the attributes themselves, if there are some. */ + /* ... and the attributes themselves, if any */ foreach_ptr(varlena, attr_val, attrs_ext) { varlena *ext_val; -- 2.47.3
>From 58b1120059e89aa1e8a7c42a69654e6e311e0041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:13:49 +0100 Subject: [PATCH 7/9] no need for palloc0 here -- simple palloc is enough --- src/backend/replication/pgoutput_repack/pgoutput_repack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c index 61cd7c52ae6..73aa6f0589c 100644 --- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c +++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c @@ -197,8 +197,8 @@ store_change(LogicalDecodingContext *ctx, Relation relation, bool *isnull; desc = RelationGetDescr(relation); - attrs = palloc0_array(Datum, desc->natts); - isnull = palloc0_array(bool, desc->natts); + attrs = palloc_array(Datum, desc->natts); + isnull = palloc_array(bool, desc->natts); heap_deform_tuple(tuple, desc, attrs, isnull); -- 2.47.3
>From 92bbc5e7c2bf9b44913ee67048668354aa9f5030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:14:30 +0100 Subject: [PATCH 8/9] XXX devel comment: heap_deform_tuple -> slot_deform_tuple? --- src/backend/replication/pgoutput_repack/pgoutput_repack.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c index 73aa6f0589c..c77564c4024 100644 --- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c +++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c @@ -200,9 +200,14 @@ store_change(LogicalDecodingContext *ctx, Relation relation, attrs = palloc_array(Datum, desc->natts); isnull = palloc_array(bool, desc->natts); + /* + * XXX it might be better to use slot_deform_tuple here, for the case + * where atttributes near the end of the tuple don't need to undergo + * this procedure. + */ heap_deform_tuple(tuple, desc, attrs, isnull); - /* First, gather and count the "external indirect" attributes. */ + /* First, gather all the "external indirect" attributes. */ for (int i = 0; i < desc->natts; i++) { CompactAttribute *attr = TupleDescCompactAttr(desc, i); -- 2.47.3
>From 0d3de76c946d470010f32d81bbc5105308b05ed9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 12 Mar 2026 16:14:59 +0100 Subject: [PATCH 9/9] reuse existing variable by overwriting it --- src/backend/replication/pgoutput_repack/pgoutput_repack.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/backend/replication/pgoutput_repack/pgoutput_repack.c b/src/backend/replication/pgoutput_repack/pgoutput_repack.c index c77564c4024..67442d07ab1 100644 --- a/src/backend/replication/pgoutput_repack/pgoutput_repack.c +++ b/src/backend/replication/pgoutput_repack/pgoutput_repack.c @@ -250,12 +250,8 @@ store_change(LogicalDecodingContext *ctx, Relation relation, /* ... and the attributes themselves, if any */ foreach_ptr(varlena, attr_val, attrs_ext) { - varlena *ext_val; - Size ext_val_size; - - ext_val = detoast_external_attr(attr_val); - ext_val_size = VARSIZE_ANY(ext_val); - BufFileWrite(file, ext_val, ext_val_size); + attr_val = detoast_external_attr(attr_val); + BufFileWrite(file, attr_val, VARSIZE_ANY(attr_val)); } /* Finally write the tuple size ... */ -- 2.47.3
