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-30T04:15:22Z
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.
>
> In general, code still needs a lot more comments to make it readable
> and in some places, code formatting is also not as per PG standard so
> that needs to be improved.
> There are some other specific comments as listed below
>

Please see the latest patch-set (v14). Did some code-formatting, used
pg_indent as well.
Added more comments. Let me know specifically if some more comments or
formatting is needed.

> 1.
> @@ -925,7 +936,7 @@ ApplyLauncherRegister(void)
>   memset(&bgw, 0, sizeof(bgw));
>   bgw.bgw_flags = BGWORKER_SHMEM_ACCESS |
>   BGWORKER_BACKEND_DATABASE_CONNECTION;
> - bgw.bgw_start_time = BgWorkerStart_RecoveryFinished;
> + bgw.bgw_start_time = BgWorkerStart_ConsistentState;
>
> What is the reason for this change, can you add some comments?

Sure, done.

>
> 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.
>
> 3.
> + /*
> + * Remember the old dbids before we stop and cleanup this worker
> + * as these will be needed in order to relaunch the worker.
> + */
> + copied_dbcnt = worker->dbcount;
> + copied_dbids = (Oid *)palloc0(worker->dbcount * sizeof(Oid));
> +
> + for (i = 0; i < worker->dbcount; i++)
> + copied_dbids[i] = worker->dbids[i];
>
> probably we can just memcpy the memory containing the dbids.

Done.

>
> 4.
> + /*
> + * If worker is being reused, and there is vacancy in dbids array,
> + * just update dbids array and dbcount and we are done.
> + * But if dbids array is exhausted, stop the worker, reallocate
> + * dbids in dsm, relaunch the worker with same set of dbs as earlier
> + * plus the new db.
> + */
>
> Why do we need to relaunch the worker, can't we just use dsa_pointer
> to expand the shared memory whenever required?
>

Done.

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

Will revisit these as stated in [1].

[1]: https://www.postgresql.org/message-id/CAJpy0uDeWjJj%2BU8nn%2BHbnGWkfY%2Bn-Bbw_kuHqgphETJ1Lucy%2BQ%40mail.gmail.com

> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com

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