Horton

Re: Synchronizing slots from primary to standby

shveta malik <shveta.malik@gmail.com>

View thread View on postgr.es
From: shveta malik <shveta.malik@gmail.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, Ajin Cherian <itsajin@gmail.com>, shveta malik <shveta.malik@gmail.com>
Date: 2023-08-25T05:39:01Z
Lists: pgsql-hackers 
On Wed, Aug 23, 2023 at 4:21 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Aug 23, 2023 at 3:38 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> I have reviewed the v12-0002 patch and I have some comments. I see the
> latest version posted sometime back and if any of this comment is
> already fixed in this version then feel free to ignore that.
>

Thanks for the feedback. Please find my comments on a few. I will work on rest.


> 2.
> ApplyLauncherShmemInit(void)
>  {
>   bool found;
> + bool foundSlotSync;
>
> Is there any specific reason to launch the sync worker from the
> logical launcher instead of making this independent?
> I mean in the future if we plan to sync physical slots as well then it
> wouldn't be an expandable design.

When we started working on this, it was reusing logical-apply worker
infra, so I separated it from logical-apply worker but let it be
managed by a replication launcher considering that only logical slots
needed to be synced. I think this needs more thought and I would like
to know from others as well before concluding anything here.


> 5.
>
> +static bool
> +WaitForSlotSyncWorkerAttach(SlotSyncWorker *worker,
> +    uint16 generation,
> +    BackgroundWorkerHandle *handle)
>
> this function is an exact duplicate of WaitForReplicationWorkerAttach
> except for the LWlock, why don't we use the same function by passing
> the LWLock as a parameter
>

Here workers (first argument) are different. We can always pass
LWLock, but since workers are different, in order to merge the common
functionality, we need to have some common worker structure between
the two workers (apply and sync-slot) and pass that to functions which
need to be merged (similar to NodeTag used in Insert/CreateStmt etc).
But changing LogicalRepWorker() would mean changing
applyworker/table-sync worker/parallel-apply-worker files. Since there
are only two such functions which you pointed out (attach and
wait_for_attach), I prefered to keep the functions as is until we
conclude on where slot-sync worker functionality actually fits in. I
can revisit these comments then. Or if you see any better way to do
it, kindly let me know.

> 6.
> +/*
> + * Attach Slot-sync worker to worker-slot assigned by launcher.
> + */
> +void
> +slotsync_worker_attach(int slot)
>
> this is also very similar to the logicalrep_worker_attach function.
>
> Please check other similar functions and reuse them wherever possible
>
> Also, why this function is not registering the cleanup function on shmmem_exit?
>

It is doing it in ReplSlotSyncMain() since we have dsm-seg there.
Please see this:

        /* Primary initialization is complete. Now, attach to our slot. */
        slotsync_worker_attach(worker_slot);
        before_shmem_exit(slotsync_worker_detach, PointerGetDatum(seg));

thanks
Shveta

Commits

  1. Doc: Add the new section "Logical Replication Failover".

  2. Fix the review comments and a bug in the slot sync code.

  3. Fix a test in failover slots regression test.

  4. Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync.

  5. Split XLogCtl->LogwrtResult into separate struct members

  6. Ensure that the sync slots reach a consistent state after promotion without losing data.

  7. Introduce a new GUC 'standby_slot_names'.

  8. Fix BF failure introduced by commit b3f6b14cf4.

  9. Fixups for commit 93db6cbda0.

  10. Fix BF failure in commit 93db6cbda0.

  11. Add a new slot sync worker to synchronize logical slots.

  12. Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48.

  13. Disable autovacuum on primary in 040_standby_failover_slots_sync test.

  14. Fix the incorrect format specifier used in commit 7a424ece48.

  15. Change the LOG level in 040_standby_failover_slots_sync.pl to DEBUG2.

  16. Add more LOG and DEBUG messages for slot synchronization.

  17. Another try to fix BF failure introduced in commit ddd5f4f54a.

  18. Fix BF introduced in commit ddd5f4f54a.

  19. Add a slot synchronization function.

  20. Clean-ups for 776621a5e4 and 7329240437.

  21. Enhance libpqrcv APIs to support slot synchronization.

  22. Add a failover option to subscriptions.

  23. Allow setting failover property in the replication command.

  24. Allow to enable failover property for replication slots via SQL API.

  25. Add macros for looping through a List without a ListCell.

  26. Track conflict_reason in pg_replication_slots.

  27. Allow upgrades to preserve the full subscription's state.

  28. Add support for incremental backup.

  29. Simplify some logic in CreateReplicationSlot()

  30. Fix uninitialized access to InitialRunningXacts during decoding after ERROR.

  31. Flexible options for CREATE_REPLICATION_SLOT.

  32. Empty search_path in logical replication apply worker and walsender.

  33. Fix oldest xmin and LSN computation across repslots after advancing

  34. Require C99 (and thus MSCV 2013 upwards).