Re: Synchronizing slots from primary to standby

On Mon, Feb 5, 2024 at 8:26 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Feb 5, 2024 at 10:57 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Just noticed that doc/src/sgml/config.sgml still refers to enable_synclot instead of sync_replication_slots:
> >
> > The standbys corresponding to the physical replication slots in
> > <varname>standby_slot_names</varname> must configure
> > <literal>enable_syncslot = true</literal> so they can receive
> > failover logical slots changes from the primary.
>
> Thanks Ajin for pointing this out. Here are v78 patches, corrected there.
>
> Other changes are:
>
> 1) Rebased the patches as the v77-001 is now pushed.
> 2) Enabled executing pg_sync_replication_slots() on cascading-standby.
> 3) Rearranged the code around parameter validity checks. Changed
> function names and changed the way how dbname is extracted as
> suggested by Amit offlist.
> 4) Rearranged the code around check_primary_info(). Removed output args.
> 5) Few other trivial changes.
>

Thank you for updating the patch! Here are some comments:

---
Since Two processes (e.g. the slotsync worker and
pg_sync_replication_slots()) concurrently fetch and update the slot
information, there is a race condition where slot's
confirmed_flush_lsn goes backward. . We have the following check but
it doesn't prevent the slot's confirmed_flush_lsn from moving backward
if the restart_lsn does't change:

/*
* Sanity check: As long as the invalidations are handled
* appropriately as above, this should never happen.
*/
if (remote_slot->restart_lsn < slot->data.restart_lsn)
elog(ERROR,
"cannot synchronize local slot \"%s\" LSN(%X/%X)"
" to remote slot's LSN(%X/%X) as synchronization"
" would move it backwards", remote_slot->name,
LSN_FORMAT_ARGS(slot->data.restart_lsn),
LSN_FORMAT_ARGS(remote_slot->restart_lsn));

---
+ It is recommended that subscriptions are first disabled before promoting
f+ the standby and are enabled back after altering the connection string.

I think it's better to describe the reason why it's recommended to
disable subscriptions before the standby promotion.

---
+/* Slot sync worker objects */
+extern PGDLLIMPORT char *PrimaryConnInfo;
+extern PGDLLIMPORT char *PrimarySlotName;

These two variables are declared also in xlogrecovery.h. Is it
intentional? If so, I think it's better to write comments.

---
Global functions and variables used by the slotsync worker are
declared in logicalworker.h and worker_internal.h. But is it really
okay to make a dependency between the slotsync worker and logical
replication workers? IIUC the slotsync worker is conceptually a
separate feature from the logical replication. I think the slotsync
worker can have its own header file.

---
+ SELECT r.srsubid AS subid, CONCAT('pg_' || srsubid ||
'_sync_' || srrelid || '_' || ctl.system_identifier) AS slotname

and

+ SELECT (CASE WHEN r.srsubstate = 'f' THEN
pg_replication_origin_progress(CONCAT('pg_' || r.srsubid || '_' ||
r.srrelid), false)

If we use CONCAT function, we can replace '||' with ','.

---
+ Confirm that the standby server is not lagging behind the subscribers.
+ This step can be skipped if
+ <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>
+ has been correctly configured.

How can the user confirm if standby_slot_names is correctly configured?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com