Horton
Menu

Re: Adding REPACK [concurrently]

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Antonin Houska <ah@cybertec.at>
Cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Mihail Nikalayeu <mihailnikalayeu@gmail.com>, Matthias van de Meent <boekewurm+postgres@gmail.com>, Pg Hackers <pgsql-hackers@lists.postgresql.org>, Robert Treat <rob@xzilla.net>
Date: 2026-04-04T23:53:09Z
Lists: pgsql-hackers

Attachments

On 2026-Apr-04, Antonin Houska wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 
> > On 2026-Apr-04, Antonin Houska wrote:
> > 
> > > I agree that the global variable is not handy, but instead of modifying
> > > CreateInitDecodingContext(), how about adding a boolean returning callback to
> > > OutputPluginCallbacks? The point is that whether shared catalogs are needed
> > > during the decoding or not is actually property of the plugin.
> > 
> > Oh, yeah, that sounds good to me.
> 
> This is it. New callback was actually not needed, I just added a new flag to
> the OutputPluginOptions structure.

Thank you, I removed the previous one and picked up this one (it's 0001
here.)  The only potentially troublesome thing I see with it is this change:

    /*
     * Update range of interesting xids based on the running xacts
     * information. We don't increase ->xmax using it, because once we are in
     * a consistent state we can do that ourselves and much more efficiently
     * so, because we only need to do it for catalog transactions since we
     * only ever look at those.
     *
     * NB: We only increase xmax when a catalog modifying transaction commits
     * (see SnapBuildCommitTxn).  Because of this, xmax can be lower than
     * xmin, which looks odd but is correct and actually more efficient, since
     * we hit fast paths in heapam_visibility.c.
+    *
+    * If database specific transaction info was used during startup, the info
+    * for the whole cluster can make xmin go backwards. That would be bad
+    * because we might no longer have older XIDs in ->committed.
     */
-   builder->xmin = running->oldestRunningXid;
+   if (NormalTransactionIdFollows(running->oldestRunningXid, builder->xmin))
+       builder->xmin = running->oldestRunningXid;


I can't see any problem with advancing the ->xmin only when it goes
forward, but I wonder if it's possible to introduce any bugs this way.


This bit looks funny though:

    /*
     * Advance the xmin limit for the current replication slot, to allow
     * vacuum to clean up the tuples this slot has been protecting.
     *
     * The reorderbuffer might have an xmin among the currently running
     * snapshots; use it if so.  If not, we need only consider the snapshots
     * we'll produce later, which can't be less than the oldest running xid in
     * the record we're reading now.
     */
    xmin = ReorderBufferGetOldestXmin(builder->reorder);
-   if (xmin == InvalidTransactionId)
+   /*
+    * Like above, do not let slot xmin go backwards.
+    */
+   if (xmin == InvalidTransactionId && !db_specific)
        xmin = running->oldestRunningXid;

I probably need some sleep, but this doesn't make sense to me.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

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