Horton
Menu

Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu@gmail.com>

View thread
From: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
To: Antonin Houska <ah@cybertec.at>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Pg Hackers <pgsql-hackers@lists.postgresql.org>, Robert Treat <rob@xzilla.net>
Date: 2026-01-18T21:52:00Z
Lists: pgsql-hackers 
Hello, Antonin!

Some comments for 0006:

> SnapBuildSnapshotForRepack(SnapBuild *builder)
Does it also "replays" previously processed WAL to the position that
snapshot is ready to use?
I am afraid we may see some non-yet processed parts of WAL leading to
duplicate insertion.

> first_block
What is the reason for that variable? It seems to be always the first block
of relation.
Also, what if we have a huge amount of empty space at the start. In that
case the first block will be the block of the first "filled" page. But
insert may (and will) fill empty pages before first_block - out of the
range.
So, I think we should delete it and always use 0 instead.

> DecodeMultiInsert
ItemPointerSet-related logic seems missing.

> tableScan = table_beginscan(OldHeap, SnapshotAny, 0, (ScanKey) NULL);
With SnapshotAny we are going to check the same tuple multiple times.
Better to let scan logic handle it (and change snapshots used by scan code).
See [0] for a way to reset snapshots during the scan.

> if (blkno >= range_end)
I don't think it is legal to switch a snapshot while holding the tuple.
Nothing is protecting it from being pruned\reused.
Snapshots need to be switched "between" pages. You may check how it is done
at [0].

> PopActiveSnapshot();
> InvalidateCatalogSnapshot();
I think it is a good idea to add here assert for MyProc->xmin and
MyProc->xid to be invalid. To ensure we really allow the horizon to advance.

> /* Set to request a snapshot. */
> bool snapshot_requested;
We know the end or region in advance, so it should be possible to filter
before writing changes to file.
So, it is some kind of "this is the end of region, create new file and
store everything before + create new snapshot for me".

> PopActiveSnapshot();
Sometimes without InvalidateCatalogSnapshot().

> PushActiveSnapshot(GetTransactionSnapshot());
GetLatestSnapshot() feels better here.

> * The individual builds might still be a problem, but that's a
> * separate issue.
Opening the index may create a catalog snapshot, so it needs to be
invalidated after.

> * TODO Can we somehow use the fact that the new heap is not yet
> * visible to other transaction, and thus cannot be vacuumed?
Snapshot resetting [0] may work here (without CIC, just as part of the scan
+ some code to ensure catalog snapshot is managed correctly).
Also, to correctly build a unique index - some tech from [0] is required
(building a unique index with multiple snapshots is a little bit tricky).
Or we may implement some super lightweight way - just SnapshotAny without
any visibility checks (just assume everything is ok since it copied from
another relation with the same index set).

> This approach introduces one limitation though: if the USING INDEX clause
is
> specified, an explicit sort is always used. Index scan wouldn't work
because
> it does not return the tuples sorted by CTID.

Technically we may just use keys (if they are comparable) as a way to
specify regions. Instead of number of pages to switch snapshot - number of
tuples or time.
But because we don't know the region end in advance - we have to keep all
the changes in file and filter only while applying.

> Assert(XLogRecPtrIsInvalid(shared->lsn_upto));
>  /* Initially we're expected to provide a snapshot and only that. */
>  Assert(shared->snapshot_requested &&
>    XLogRecPtrIsInvalid(shared->lsn_upto));

XLogRecPtrIsInvalid(shared->lsn_upto) assertion is duplicated here.

> range_end = repack_blocks_per_snapshot;
Should be repack_blocks_per_snapshot + ctx->first_block ? (but better to
remove the first_block at all).

> * XXX It might be worth Assert(CatalogSnapshot == NULL)
> * here, however that symbol is not external.
As said above - better assert for MyProc->xmin/xid + add
InvalidateCatalogSnapshot.

> extern Snapshot
> extern void

Some "externs" used in C files (not headers).

> ctx->block_ranges = NIL;
may be used with list_free?

>  * Remember here to which pages should applied to changes recorded in
given
>  * file.
"should apply"

>  of pages, so that VACUUM does not get block for too long
"blocked"

> there are no restriction on block
"there is" or "restrictions"

> repack_add_block_range
extra new-line after function.

> Is REPACKED (CONCURRENTLY) is being run by this backend?
REPACK and double "is"

[0]: https://commitfest.postgresql.org/patch/6401/

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Allow old WAL recycling during REPACK CONCURRENTLY

  2. Advance restart_lsn more eagerly in LogicalConfirmReceivedLocation

  3. Remove unnecessary signal handler change

  4. Improve REPACK (CONCURRENTLY) error messages some more

  5. Revert "Allow logical replication snapshots to be database-specific"

  6. Move REPACK (CONCURRENTLY) test out of stock regression tests

  7. REPACK: do not require REPLICATION or LOGIN

  8. Add missing initialization

  9. Simplify declaration of memcpy target

  10. Reserve replication slots specifically for REPACK

  11. doc: Add an example of REPACK (CONCURRENTLY)

  12. Allow logical replication snapshots to be database-specific

  13. Avoid different-size pointer-to-integer cast

  14. Fix valgrind failure

  15. Add CONCURRENTLY option to REPACK

  16. Rename cluster.c to repack.c (and corresponding .h)

  17. Allow index_create to suppress index_build progress reporting

  18. Make index_concurrently_create_copy more general

  19. Document the 'command' column of pg_stat_progress_repack

  20. Introduce the REPACK command

  21. Toggle logical decoding dynamically based on logical slot presence.

  22. Split vacuumdb to create vacuuming.c/h

  23. Remove ReorderBufferTupleBuf structure.

  24. Revert changes to CONCURRENTLY that "sped up" Xmin advance

  25. VACUUM: ignore indexing operations with CONCURRENTLY