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-02-28T15:16:00Z
Lists: pgsql-hackers 
Hello!

Some review comments:

------------

> attrs = palloc0_array(Datum, desc->natts);
> isnull = palloc0_array(bool, desc->natts);

It looks like there is a memory leak with those arrays.

------------

> # TOAST pointer, wich we need to update
typo

------------

> ident_idx = RelationGetReplicaIndex(rel);
> if (!OidIsValid(ident_idx) && OidIsValid(rel->rd_pkindex))

check_repack_concurrently_requirements uses rd_pkindex as fallback.

But rebuild_relation_finish_concurrent does not contain such logic:

> ident_idx_old = RelationGetReplicaIndex(OldHeap);

------------

> >
> > > ConditionVariablePrepareToSleep(&shared->cv);
> > >   for (;;)
> > >   {
> > >      bool      initialized;
> > >
> > >      SpinLockAcquire(&shared->mutex);
> > >      initialized = shared->initialized;
> > >      SpinLockRelease(&shared->mutex);
> > src/backend/commands/cluster.c:3663
> >
> > I think we should check GetBackgroundWorkerPid for worker status, to
> > not wait forever in case of some issue with the worker.

> ConditionVariableSleep() calls CHECK_FOR_INTERRUPTS(). That should process
> error messages from the worker.

Hm, yes, and RepackWorkerShutdown will detach the queue. But
ProcessRepackMessages does not react somehow to SHM_MQ_DETACHED - just
ignores. Or am I missing something?
And looks like it applies to all wait-loops related to repack.

------------

> build_identity_key
> ....
> n = ident_idx->indnatts;

Should we use indnkeyatts here?

------------

> build_identity_key
> ....
> entry->sk_collation = att->attcollation;

Should we use index collation (not heap) here?
entry->sk_collation =  ident_idx_rel->rd_indcollation[i];

------------

> SnapBuildInitialSnapshotForRepack

What is about to add defensive checks like SnapBuildInitialSnapshot does?

> if (!builder->committed.includes_all_transactions)
>    elog(ERROR, "cannot build an initial slot snapshot, not all transactions are monitored anymore");


Best regards,
Mikhail.

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