Horton
Menu

Re: Adding REPACK [concurrently]

Antonin Houska <ah@cybertec.at>

View thread
From: Antonin Houska <ah@cybertec.at>
To: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Pg Hackers <pgsql-hackers@lists.postgresql.org>, Robert Treat <rob@xzilla.net>
Date: 2026-02-16T07:47:08Z
Lists: pgsql-hackers 
Mihail Nikalayeu <mihailnikalayeu@gmail.com> wrote:

> Some feedback for v33.

Thanks again for your review!

> > else if (pg_strcasecmp(cmd, "REPACK") == 0)
> >      cmdtype = PROGRESS_COMMAND_REPACK;
> src/backend/utils/adt/pgstatfuncs.c:290
> 
> I think we need to add "CLUSTER" here too to avoid regression.

What kind of regression? There is no pg_stat_get_progress_info('CLUSTER') call
in system_views.sql.

> -----------
> 
> > 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.

> -----------
> 
> > /* Error queue. */
> > shm_mq       *error_mq;
> src/backend/commands/cluster.c:210.
> 
> Not used anywhere.

ok

> -----------
> 
> >   finish_heap_swap(old_table_oid, new_table_oid,
> >                is_system_catalog,
> >                false,       /* swap_toast_by_content */
> >                false, true, false,
> >                frozenXid, cutoffMulti,
> >                relpersistence);
> src/backend/commands/cluster.c
> 
> I think we should add comments for other boolean parameters.

Perhaps, it wouldn't hurt.

> -----------
> 
> > elog(ERROR, "Incomplete insert info.");
> > elog(ERROR, "Incomplete update info.");
> src/backend/replication/pgoutput_repack/pgoutput_repack.c:118,132
> 
> Should be non-capitalized?

ok

> -----------
> 
> > # Copyright (c) 2022-2024, PostgreSQL Global Development Group
> src/backend/replication/pgoutput_repack/meson.build
> 
> 2022-2026

ok

> -----------
> 
> > int         newxcnt = 0;
> src/backend/replication/logical/snapbuild.c:53
> 
> uint32 is better here.

This was already discussed:

https://www.postgresql.org/message-id/137668.1768235610%40localhost

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

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