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-05T18:56:16Z
Lists: pgsql-hackers 
Hi,

So I've been trying to understand the "Introduce an option to make
logical replication database specific." patch and I have to confess I
just cannot.

As far as I can read, the point is that if we reach
SnapBuildProcessRunningXacts() when db_specific is true (which means
standby_decode is called in an output plugin that has set
need_shared_catalogs to false), _and_ we've not reached consistent state
yet, then we'll call LogStandbySnapshot with our DB oid to emit a new
xl_running_xacts message.

So the WAL-decoding process emits WAL.  I don't know if in normal
conditions logical decoding processes emit WAL.  If this is exceptional,
I think we should add a comment.

Now, this additional WAL message will be processed by all other
processes decoding WAL.  Perhaps it will ignored by most of them.  But
most importantly, it will also reach back to ourselves, at which point
we can hopefully use it to see that we might have reached consistent
state within our database.  Then we know our snapshot is ready to be
used.

Is this correct?


I think the reason it's safe to skip a lot of the processing caused by
this additional process, is that xl_running_xacts messages are also
emitted in other places in a non-database specific manner.  So all the
other placecs that are emitting that message continue to exist and
cause logical-decoders operate in the same way as before.

I think we should sprinkle lots of comments in several places about
this.  For example, I propose that standby_redo() should have something
like

  * If 'dbid' is valid, only gather transactions running in that database.
+ * Such records should not be the only ones emitted, because this has
+ * potentially dangerous side-effects which makes some places ignore them:
+ *
+ * 1. SnapBuildProcessRunningXacts will skip computing the xmin and restart
+ * point from its input record if the record's xmin is older that the
+ * snapbuilder's current xmin; this should normally be fine because that
+ * information will be updated from other xl_running_xacts records.
+ * 2. standby_redo will likewise skip processing such a record
  *
(are there other things that should be mentioned?)


Also, LogStandbySnapshot() should have a comment explaining that passing
a valid dboid is a weird corner case which is to be used with care, and
that functions X Y and Z are going to ignore snapshots carrying a valid
dbid.

Why do we call SnapBuildFindSnapshot() to do this, instead of doing it
directly in SnapBuildProcessRunningXacts?  Seems like it would be more
straightforward.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.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