Subject: Re: Synchronizing slots from primary to standby

From: shveta malik <[email-masked]>
Date: Fri, 5 Jan 2024 08:59:31 +0530

On Thu, Jan 4, 2024 at 7:24 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> Hi,
>
> On Thu, Jan 04, 2024 at 10:27:31AM +0530, shveta malik wrote:
> > On Thu, Jan 4, 2024 at 9:18 AM shveta malik <[email-masked]> wrote:
> > >
> > > On Wed, Jan 3, 2024 at 6:33 PM Zhijie Hou (Fujitsu)
> > > <[email-masked]> wrote:
> > > >
> > > > On Tuesday, January 2, 2024 6:32 PM shveta malik <[email-masked]> wrote:
> > > > > On Fri, Dec 29, 2023 at 10:25 AM Amit Kapila <[email-masked]>
> > > > >
> > > > > The topup patch has also changed app_name to
> > > > > {cluster_name}_slotsyncworker so that we do not confuse between walreceiver
> > > > > and slotsyncworker entry.
> > > > >
> > > > > Please note that there is no change in rest of the patches, changes are in
> > > > > additional 0004 patch alone.
> > > >
> > > > Attach the V56 patch set which supports ALTER SUBSCRIPTION SET (failover).
> > > > This is useful when user want to refresh the publication tables, they can now alter the
> > > > failover option to false and then execute the refresh command.
> > > >
> > > > Best Regards,
> > > > Hou zj
> > >
> > > The patches no longer apply to HEAD due to a recent commit 007693f. I
> > > am working on rebasing and will post the new patches soon
> > >
> > > thanks
> > > Shveta
> >
> > Commit 007693f has changed 'conflicting' to 'conflict_reason', so
> > adjusted the code around that in the slotsync worker.
> >
> > Also removed function 'pg_get_slot_invalidation_cause' as now
> > conflict_reason tells the same.
> >
> > PFA rebased patches with above changes.
> >
>
> Thanks!
>
> Looking at 0004:
>
> 1 ====
>
> -libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
> -                                const char *appname, char **err)
> +libpqrcv_connect(const char *conninfo, bool replication, bool logical,
> +                                bool must_use_password, const char *appname, char **err)
>
> What about adjusting the preceding comment a bit to describe what the new replication
> parameter is for?
>
> 2 ====
>
> +       /* We can not have logical w/o replication */
>
> what about replacing w/o by without?
>
> 3 ===
>
> +       if(!replication)
> +               Assert(!logical);
> +
> +       if (replication)
>         {
>
> what about using "if () else" instead (to avoid unnecessary test)?
>
>
> Having said that the patch seems a reasonable way to implement non-replication
> connection in slotsync worker.
>
> 4 ===
>
> Looking closer, the only place where walrcv_connect() is called with replication
> set to false and logical set to false is in ReplSlotSyncWorkerMain().
>
> That does make sense, but what do you think about creating dedicated libpqslotsyncwrkr_connect
> and slotsyncwrkr_connect (instead of using the libpqrcv_connect / walrcv_connect ones)?
>
> That way we could make use of slotsyncwrkr_connect() in ReplSlotSyncWorkerMain()
> as I think it's confusing to use "rcv" functions while the process using them is
> not of backend type walreceiver.
>
> I'm not sure that worth the extra complexity though, what do you think?

I gave it a thought earlier, but then I was not sure even if I create
a new function w/o "rcv" in it then where should it be placed as the
existing file name itself is libpq'walreceiver'.c. Shall we be
creating a new file then? But it does not seem good to create a new
setup (new file, function pointers  other stuff) around 1 function.
And thus reusing the same function with 'replication' (new arg) felt
like a better choice than other options. If in future, there is any
other module trying to do the same, then it can use current
walrcv_connect() with rep=false. If I make it specific to slot-sync
worker, then it will not be reusable by other modules (if needed).

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Fri, 5 Jan 2024 10:00:53 +0530

> worker, then it will not be reusable by other modules (if needed).

I agree that the benefit of creating a new API is not very clear. How
about adjusting the description in the file header of
libpqwalreceiver.c. I think apart from walreceiver, it is now also
used by logical replication workers and with this patch by the
slotsync worker as well.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 5 Jan 2024 08:15:46 +0000

Hi,

> > setup (new file, function pointers  other stuff) around 1 function.

Yeah...

> > worker, then it will not be reusable by other modules (if needed).

Yeah good point, it would need to be more generic.

> I agree that the benefit of creating a new API is not very clear.

Yeah, that would be more for cosmetic purpose (and avoid using a WalReceiverConn
while a PGconn could/should suffice).

> How
> libpqwalreceiver.c. 

Agree, that seems to be a better option (not sure that building the new API is
worth the extra work).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Dilip Kumar <[email-masked]>
Date: Fri, 5 Jan 2024 16:25:00 +0530

On Fri, Jan 5, 2024 at 8:59 AM shveta malik <[email-masked]> wrote:
I was going the the patch set again, I have a question.  The below
comments say that we keep the failover option as PENDING until we have
done the initial table sync which seems fine.  But what happens if we
add a new table to the publication and refresh the subscription? In
such a case does this go back to the PENDING state or something else?

+ * As a result, we enable the failover option for the main slot only after the
+ * initial sync is complete. The failover option is implemented as a tri-state
+ * with values DISABLED, PENDING, and ENABLED. The state transition process
+ * between these values is the same as the two_phase option (see TWO_PHASE
+ * TRANSACTIONS for details).

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

---

From: Amit Kapila <[email-masked]>
Date: Fri, 5 Jan 2024 17:45:44 +0530

> such a case does this go back to the PENDING state or something else?

At this stage, such an operation is prohibited. Users need to disable
the failover option first, then perform the above operation, and after
that failover option can be re-enabled.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 8 Jan 2024 11:39:39 +0530

> that failover option can be re-enabled.

Okay, that makes sense to me.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 9 Jan 2024 12:14:04 +0000

On Monday, January 8, 2024 2:10 PM Dilip Kumar <[email-masked]> wrote:
> > > comments say that we keep the failover option as PENDING until we
> > > have done the initial table sync which seems fine.  But what happens
> > > if we add a new table to the publication and refresh the
> > > subscription? In such a case does this go back to the PENDING state or
> something else?
> > >
> >

During the off-list discussion, Sawada-san proposed one idea which can release the
restriction for table sync: instead of relying on the latest WAL position, we
can utilize the remote restart_lsn to reserve the WAL when creating a new
synced slot on the standby. This approach eliminates the need to wait for the
primary server to catch up, thus improving the speed of synced slot creation on
the standby in most scenarios.

By using this approach, the limitation that prevents users from performing
table sync during failover can be eliminated. In previous versions, this
restriction existed because table sync slots were often incompletely
synchronized to the standby(the slots on primary could not catch up the synced
slot). And with this approach, the table sync slots can be efficiently
synced to the standby in most cases.

However, there could still be rare cases that the WAL around remote restart_lsn
has been removed on standby, we will try to reserve the last remaining wal in
this case and mark the slot as temporary, these temp slots will be converted to
persistent once the remote restart_lsn catches up.

We think this idea is promising and here is the V58 patch set which tries to
address the idea, the summary of changes for each patch is as follows:

V58-0001

1) Enables failover for table sync slot.
2) Removes the restriction on table sync when failover is enabled.
3) Removes tristate handling for failover state.
4) Renames failoverstate to failover.
5) Address Peter's comments[1].

V58-0002
1) Add the document about how to resume logical replication after failover.
2) Don't sync temporary from primary server anymore.
3) Fix one spinlock miss.
4) Fix one CFbot warning.
5) Fixes a bug where last_update_time is not initialized.
6) Reserves WAL based on the remote restart_lsn.
7) Improves and adjusts the tests.
8) remove the separate function wait_for_primary_slot_catchup() and integrate
  its logic of marking the slot as ready into the main loop.
9) remove the 'i' state of sync_state. The slots that need to wait for the
  primary to catch up will be marked as TEMPORARY, and they will be converted
  to PERSISTENT once the remote restart_lsn catches up.

Thanks Shveta for working on 1) to 4).

V58-0003
Rebases the tests.

V58-0004:
Address Bertrand comments[2]. Thanks Shveta for working on this.

TODO: Add documents to guide user the way to identity if the table sync slot
and the main slot is READY that the logical replication can be resumed by
subscribing to the new primary.

[1] https://www.postgresql.org/message-id/CAHut%2BPvbbPz1%3DT4bzY0_GotUK460Eih41Twjt%3DczJ1z2J8SGEw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/ZZa4pLFCe2mAks1m%40ip-10-97-1-34.eu-west-3.compute.internal

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Tue, 9 Jan 2024 18:39:43 +0530

> V58-0002

+static bool
+synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot)
{
...
+ /* Slot ready for sync, so sync it. */
+ else
+ {
+ /*
+ * Sanity check: With hot_standby_feedback enabled and
+ * invalidations 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));
...
}

I was thinking about the above code in the patch and as far as I can
think this can only occur if the same name slot is re-created with
prior restart_lsn after the existing slot is dropped. Normally, the
newly created slot (with the same name) will have higher restart_lsn
but one can mimic it by copying some older slot by using
pg_copy_logical_replication_slot().

I don't think as mentioned in comments even if hot_standby_feedback is
temporarily set to off, the above shouldn't happen. It can only lead
to invalidated slots on standby.

To close the above race, I could think of the following ways:
1. Drop and re-create the slot.
2. Emit LOG/WARNING in this case and once remote_slot's LSN moves
ahead of local_slot's LSN then we can update it; but as mentioned in
your previous comment, we need to update all other fields as well. If
we follow this then we probably need to have a check for catalog_xmin
as well.

Now, related to this the other case which needs some handling is what
if the remote_slot's restart_lsn is greater than local_slot's
restart_lsn but it is a re-created slot with the same name. In that
case, I think the other properties like 'two_phase', 'plugin' could be
different. So, is simply copying those sufficient or do we need to do
something else as well?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Wed, 10 Jan 2024 12:29:21 +1100

Here are some review comments for the patch v58-0001

======
doc/src/sgml/catalogs.sgml

1.
+      <para>
+       If true, the associated replication slots (i.e. the main slot and the
+       table sync slots) in the upstream database are enabled to be
+       synchronized to the physical standbys.
+      </para></entry>

It seems the other single-sentence descriptions on this page have no
period (.) so for consistency maybe you should remove it here also.

======
src/backend/commands/subscriptioncmds.c

2. AlterSubscription

+ /*
+ * Do not allow changing the failover state if the
+ * subscription is enabled. This is because the failover
+ * state of the slot on the publisher cannot be modified if
+ * the slot is currently being acquired by the apply
+ * worker.
+ */

/being acquired/acquired/

~~~

3.
values[Anum_pg_subscription_subfailover - 1] =
BoolGetDatum(opts.failover);
replaces[Anum_pg_subscription_subfailover - 1] = true;

/*
* The failover state of the slot should be changed after
* the catalog update is completed.
*/
set_failover = true;
AFAICT you don't need to introduce a new variable 'set_failover'.
Instead, you can test like:

BEFORE
if (set_failover)

AFTER
if (replaces[Anum_pg_subscription_subfailover - 1])

======
src/backend/replication/logical/tablesync.c

4.
  walrcv_create_slot(LogRepWorkerWalRcvConn,
     slotname, false /* permanent */ , false /* two_phase */ ,
+    MySubscription->failover /* failover */ ,
     CRS_USE_SNAPSHOT, origin_startpos);

The "/* failover */ comment is unnecessary now that you pass the
boolean field with the same descriptive name.

======
src/include/catalog/pg_subscription.h

5. CATALOG

+ bool subfailover; /* True if the associated replication slots
+ * (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * the upstream database are enabled to be
+ * synchronized to the physical standbys. */
+

The wording of the comment is broken (it says "are enabled" 2x).

SUGGESTION
True if the associated replication slots (i.e. the main slot and the
table sync slots) in the upstream database are enabled to be
synchronized to the physical standbys.

~~~

6. Subscription

+ bool failover; /* Indicates if the associated replication
+ * slots (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * to be synchronized to the physical
+ * standbys. */

This comment can say "True if...", so it will be the same as the
earlier CATALOG comment for 'subfailover'.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

---

From: Dilip Kumar <[email-masked]>
Date: Wed, 10 Jan 2024 11:56:08 +0530

On Tue, Jan 9, 2024 at 5:44 PM Zhijie Hou (Fujitsu)
<[email-masked]> wrote:
comments on 0002

1.
+/* Worker's nap time in case of regular activity on the primary server */
+#define WORKER_DEFAULT_NAPTIME_MS                   10L /* 10 ms */
+
+/* Worker's nap time in case of no-activity on the primary server */
+#define WORKER_INACTIVITY_NAPTIME_MS                10000L /* 10 sec */

Instead of directly switching between 10ms to 10s shouldn't we
increase the nap time gradually?  I mean it can go beyond 10 sec as
well but instead of directly switching
from 10ms to 10 sec we can increase it every time with some multiplier
and keep a max limit up to which it can grow.  Although we can reset
back to 10ms directly as soon as
we observe some activity.

2.
SlotSyncWorkerCtxStruct add this to typedefs. list file

3.
+/*
+ * Update local slot metadata as per remote_slot's positions
+ */
+static void
+local_slot_update(RemoteSlot *remote_slot)
+{
+ Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);
+
+ LogicalConfirmReceivedLocation(remote_slot->confirmed_lsn);
+ LogicalIncreaseXminForSlot(remote_slot->confirmed_lsn,
+    remote_slot->catalog_xmin);
+ LogicalIncreaseRestartDecodingForSlot(remote_slot->confirmed_lsn,
+   remote_slot->restart_lsn);
+}

IIUC on the standby we just want to overwrite what we get from primary
no? If so why we are using those APIs that are meant for the actual
decoding slots where it needs to take certain logical decisions
instead of mere overwriting?

4.
+/*
+ * Helper function for drop_obsolete_slots()
+ *
+ * Drops synced slot identified by the passed in name.
+ */
+static void
+drop_synced_slots_internal(const char *name, bool nowait)

Suggestion to add one line to explain no wait in the header

5.
+/*
+ * Helper function to check if local_slot is present in remote_slots list.
+ *
+ * It also checks if logical slot is locally invalidated i.e. invalidated on
+ * the standby but valid on the primary server. If found so, it sets
+ * locally_invalidated to true.
+ */

Instead of saying "but valid on the primary server" better to mention
it in the remote_slots list, because here this function is just
checking the remote_slots list regardless of whether the list came
from.  Mentioning primary
seems like it might fetch directly from the primary in this function
so this is a bit confusing.

6.
+/*
+ * Check that all necessary GUCs for slot synchronization are set
+ * appropriately. If not, raise an ERROR.
+ */
+static void
+validate_slotsync_parameters(char **dbname)

The function name just says 'validate_slotsync_parameters' but it also
gets the dbname so I think it better we change the name accordingly
also instead of passing dbname as a parameter just return it directly.
There
is no need to pass this extra parameter and make the function return void.

7.
+ tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ tuple_ok = tuplestore_gettupleslot(res->tuplestore, true, false, tupslot);
+ Assert(tuple_ok); /* It must return one tuple */

Comments say 'It must return one tuple' but asserting just for at
least one tuple shouldn't we enhance assert so that it checks that we
got exactly one tuple?

8.
/* No need to check further, return that we are cascading standby */
+ *am_cascading_standby = true;

we are not returning immediately we are just setting
am_cascading_standby to true so adjust comments accordingly

9.
+ /* No need to check further, return that we are cascading standby */
+ *am_cascading_standby = true;
+ }
+ else
+ {
+ /* We are a normal standby. */

Single-line comments do not follow the uniform pattern for the full
stop, either use a full stop for all single-line comments or none, at
least follow the same rule in a file or nearby comments.

10.
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_slot_name"));

Why we are using the constant string "primary_slot_name" as a variable
in this error formatting?

11.
+ /*
+ * Hot_standby_feedback must be enabled to cooperate with the physical
+ * replication slot, which allows informing the primary about the xmin and
+ * catalog_xmin values on the standby.

I do not like capitalizing the first letter of the
'hot_standby_feedback' which is a GUC parameter

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 10 Jan 2024 12:23:14 +0000

On Wednesday, January 10, 2024 2:26 PM Dilip Kumar <[email-masked]> wrote:
> 
> On Tue, Jan 9, 2024 at 5:44 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> comments on 0002

Thanks for the comments !

> Instead of directly switching between 10ms to 10s shouldn't we increase the
> nap time gradually?  I mean it can go beyond 10 sec as well but instead of
> directly switching from 10ms to 10 sec we can increase it every time with some
> multiplier and keep a max limit up to which it can grow.  Although we can reset
> back to 10ms directly as soon as we observe some activity.

Agreed. I changed the strategy similar to what we do in the walsummarizer.

> + * Update local slot metadata as per remote_slot's positions  */ static
> +void local_slot_update(RemoteSlot *remote_slot) {
> +Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);
> +
> IIUC on the standby we just want to overwrite what we get from primary no? If
> so why we are using those APIs that are meant for the actual decoding slots
> where it needs to take certain logical decisions instead of mere overwriting?

I think we don't have a strong reason to use these APIs, but it was convenient to
use these APIs as they can take care of updating the slots info and will call
functions like, ReplicationSlotsComputeRequiredXmin,
ReplicationSlotsComputeRequiredLSN internally. Or do you prefer directly overwriting
the fields and call these manually ?

> Suggestion to add one line to explain no wait in the header

The 'nowait' flag is not necessary now, so removed.

> + * It also checks if logical slot is locally invalidated i.e.
> +invalidated on
> Instead of saying "but valid on the primary server" better to mention it in the
> remote_slots list, because here this function is just checking the remote_slots
> list regardless of whether the list came from.  Mentioning primary seems like it
> might fetch directly from the primary in this function so this is a bit confusing.

Adjusted.

> The function name just says 'validate_slotsync_parameters' but it also gets the
> dbname so I think it better we change the name accordingly also instead of
> passing dbname as a parameter just return it directly.
> There

Renamed.

> 
> 7.
> + tupslot = MakeSingleTupleTableSlot(res->tupledesc,
> + &TTSOpsMinimalTuple); tuple_ok =
> + tuplestore_gettupleslot(res->tuplestore, true, false, tupslot);
> Comments say 'It must return one tuple' but asserting just for at least one tuple
> shouldn't we enhance assert so that it checks that we got exactly one tuple?

Changed to use tuplestore_tuple_count.

> we are not returning immediately we are just setting am_cascading_standby to
> true so adjust comments accordingly

Adjusted.

> + *am_cascading_standby = true; } else {
> Single-line comments do not follow the uniform pattern for the full stop, either
> use a full stop for all single-line comments or none, at least follow the same rule
> in a file or nearby comments.

Adjusted.

> Why we are using the constant string "primary_slot_name" as a variable in this
> error formatting?

It was suggested to make it friendly to the translator, as the GUC doesn't needs to be translated
and it can avoid adding multiple similar message to be translated.

> + * replication slot, which allows informing the primary about the xmin
> + and
> I do not like capitalizing the first letter of the 'hot_standby_feedback' which is a
> GUC parameter

Changed.

Here is the V59 patch set which addressed above comments and comments from Peter[1].

[1] https://www.postgresql.org/message-id/CAHut%2BPu34_dYj9MnV6n3cPsssEx57YaO6Pg0d9mDryQZX2Mx3g%40mail.gmail.com

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Thu, 11 Jan 2024 12:58:17 +1100

Here are some review comments for patch v58-0002

(FYI - I quickly checked with the latest v59-0002 and AFAIK all these
review comments below are still relevant)

======
Commit message

1.
If a logical slot is invalidated on the primary, slot on the standby is also
invalidated.

~

/slot on the standby/then that slot on the standby/

======
doc/src/sgml/logicaldecoding.sgml

2.
In order to resume logical replication after failover from the synced
logical slots, it is required that 'conninfo' in subscriptions are
altered to point to the new primary server using ALTER SUBSCRIPTION
... CONNECTION. It is recommended that subscriptions are first
disabled before promoting the standby and are enabled back once these
are altered as above after failover.

~

Minor rewording mainly to reduce a long sentence.

SUGGESTION
To resume logical replication after failover from the synced logical
slots, the subscription's 'conninfo' must be altered to point to the
new primary server. This is done using ALTER SUBSCRIPTION ...
CONNECTION. It is recommended that subscriptions are first disabled
before promoting the standby and are enabled back after altering the
connection string.

======
doc/src/sgml/system-views.sgml

3.
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>synced</structfield> <type>bool</type>
+      </para>
+      <para>
+      True if this logical slot was synced from a primary server.
+      </para>
+      <para>

SUGGESTION
True if this is a logical slot that was synced from a primary server.

======
src/backend/access/transam/xlogrecovery.c

4.
+ /*
+ * Shutdown the slot sync workers to prevent potential conflicts between
+ * user processes and slotsync workers after a promotion.
+ *
+ * We do not update the 'synced' column from true to false here, as any
+ * failed update could leave some slot's 'synced' column as false. This
+ * could cause issues during slot sync after restarting the server as a
+ * standby. While updating after switching to the new timeline is an
+ * option, it does not simplify the handling for 'synced' column.
+ * Therefore, we retain the 'synced' column as true after promotion as they
+ * can provide useful information about their origin.
+ */

Minor comment wording changes.

BEFORE
...any failed update could leave some slot's 'synced' column as false.
SUGGESTION
...any failed update could leave 'synced' column false for some slots.

~

BEFORE
Therefore, we retain the 'synced' column as true after promotion as
they can provide useful information about their origin.
SUGGESTION
Therefore, we retain the 'synced' column as true after promotion as it
may provide useful information about the slot origin.

======
src/backend/replication/logical/slotsync.c

5.
+ * While creating the slot on physical standby, if the local restart_lsn and/or
+ * local catalog_xmin is ahead of those on the remote then the worker cannot
+ * create the local slot in sync with the primary server because that would
+ * mean moving the local slot backwards and the standby might not have WALs
+ * retained for old LSN. In this case, the worker will mark the slot as
+ * RS_TEMPORARY. Once the primary server catches up, it will move the slot to
+ * RS_PERSISTENT and will perform the sync periodically.

/will move the slot to RS_PERSISTENT/will mark the slot as RS_PERSISTENT/

~~~

6. drop_synced_slots_internal
+/*
+ * Helper function for drop_obsolete_slots()
+ *
+ * Drops synced slot identified by the passed in name.
+ */
+static void
+drop_synced_slots_internal(const char *name, bool nowait)
+{
+ Assert(MyReplicationSlot == NULL);
+
+ ReplicationSlotAcquire(name, nowait);
+
+ Assert(MyReplicationSlot->data.synced);
+
+ ReplicationSlotDropAcquired();
+}

IMO you don't need this function. AFAICT it is only called from one
place and does not result in fewer lines of code.

~~~

7. get_local_synced_slots

+ /* Check if it is logical synchronized slot */
+ if (s->in_use && SlotIsLogical(s) && s->data.synced)
+ {
+ local_slots = lappend(local_slots, s);
+ }

Do you need to check SlotIsLogical(s) here? I thought s->data.synced
can never be true for physical slots. I felt you could write this like
blelow:

if (s->in_use s->data.synced)
{
  Assert(SlotIsLogical(s));
  local_slots = lappend(local_slots, s);
}

~~~

8. check_sync_slot_on_remote

+static bool
+check_sync_slot_on_remote(ReplicationSlot *local_slot, List *remote_slots,
+   bool *locally_invalidated)
+{
+ ListCell   *lc;
+
+ foreach(lc, remote_slots)
+ {
+ RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);

I think you can use the new style foreach_ptr list macros here.

~~~

9. drop_obsolete_slots

+drop_obsolete_slots(List *remote_slot_list)
+{
+ List    *local_slots = NIL;
+ ListCell   *lc;
+
+ local_slots = get_local_synced_slots();
+
+ foreach(lc, local_slots)
+ {
+ ReplicationSlot *local_slot = (ReplicationSlot *) lfirst(lc);

I think you can use the new style foreach_ptr list macros here.

~~~

10. reserve_wal_for_slot

+ Assert(slot != NULL);
+ Assert(slot->data.restart_lsn == InvalidXLogRecPtr);

You can use the macro XLogRecPtrIsInvalid(lot->data.restart_lsn)

~~~

11. update_and_persist_slot

+/*
+ * Update the LSNs and persist the slot for further syncs if the remote
+ * restart_lsn and catalog_xmin have caught up with the local ones. Otherwise,
+ * persist the slot and return.
+ *
+ * Return true if the slot is marked READY, otherwise false.
+ */
+static bool
+update_and_persist_slot(RemoteSlot *remote_slot)

11a.
The comment says "Otherwise, persist the slot and return" but there is
a return false which doesn't seem to persist anything so it seems
contrary to the comment.

~

11b.
"slot is marked READY" -- IIUC the synced states no longer exist in
v58 so this comment maybe should not be referring to READY anymore. Or
maybe there just needs to be more explanation about the difference
between 'synced' and the state you call "READY".

~~~

12. synchronize_one_slot

+ * The slot is created as a temporary slot and stays in same state until the
+ * initialization is complete. The initialization is considered to be completed
+ * once the remote_slot catches up with locally reserved position and local
+ * slot is updated. The slot is then persisted.

I think this comment is related to the "READY" mentioned by
update_and_persist_slot. Still, perhaps the terminology needs to be
made consistent across all these comments -- e.g. "considered to be
completed" versus "READY" versus "sync-ready" etc.

~~~

13.
+ ReplicationSlotCreate(remote_slot->name, true, RS_TEMPORARY,
+   remote_slot->two_phase,
+   remote_slot->failover,
+   true);

This review comment is similar to elsewhere in this post. Consider
commenting on the new parameter like "true /* synced */"

~~~

14. synchronize_slots

+ /*
+ * It is possible to get null values for LSN and Xmin if slot is
+ * invalidated on the primary server, so handle accordingly.
+ */
+ remote_slot->confirmed_lsn = !slot_attisnull(tupslot, 3) ?
+ DatumGetLSN(slot_getattr(tupslot, 3, &isnull)) :
+ InvalidXLogRecPtr;
+
+ remote_slot->restart_lsn = !slot_attisnull(tupslot, 4) ?
+ DatumGetLSN(slot_getattr(tupslot, 4, &isnull)) :
+ InvalidXLogRecPtr;
+
+ remote_slot->catalog_xmin = !slot_attisnull(tupslot, 5) ?
+ DatumGetTransactionId(slot_getattr(tupslot, 5, &isnull)) :
+ InvalidTransactionId;

Isn't this the same functionality as the older v51 code that was
written differently? I felt the old style (without ignoring the
'isnull') was more readable.

v51
+ remote_slot->confirmed_lsn = DatumGetLSN(slot_getattr(tupslot, 3, &isnull));
+ if (isnull)
+ remote_slot->confirmed_lsn = InvalidXLogRecPtr;

v58
+ remote_slot->confirmed_lsn = !slot_attisnull(tupslot, 3) ?
+ DatumGetLSN(slot_getattr(tupslot, 3, &isnull)) :
+ InvalidXLogRecPtr;

If you prefer a ternary, it might be cleaner to do it like:

Datum d;
...
d = slot_getattr(tupslot, 3, &isnull);
remote_slot->confirmed_lsn = isnull ? InvalidXLogRecPtr : DatumGetLSN(d);
...

~~~

15.
+
+ /* Drop local slots that no longer need to be synced. */
+ drop_obsolete_slots(remote_slot_list);
+
+ /* Now sync the slots locally */
+ foreach(lc, remote_slot_list)
+ {
+ RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
+
+ some_slot_updated |= synchronize_one_slot(wrconn, remote_slot);
+ }

Here you can use the new list macro like foreach_ptr.

~~~

16. ReplSlotSyncWorkerMain

+ wrconn = walrcv_connect(PrimaryConnInfo, true, false,
+ cluster_name[0] ? cluster_name : "slotsyncworker",
+ &err);
+ if (wrconn == NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not connect to the primary server: %s", err));

Typically, I saw other PG code doing "if (!wrconn)" instead of "if
(wrconn == NULL)"

======
src/backend/replication/slotfuncs.c

17. create_physical_replication_slot

  ReplicationSlotCreate(name, false,
    temporary ? RS_TEMPORARY : RS_PERSISTENT, false,
-   false);
+   false, false);

IMO passing parameters like "false, false, false" becomes a bit
difficult to understand from the caller's POV so it might be good to
comment on the parameter like:

ReplicationSlotCreate(name, false,
  temporary ? RS_TEMPORARY : RS_PERSISTENT, false,
  false, false /* synced */);

(there are a few other places like this where the same review comment applies)

~~~

18. create_logical_replication_slot

  ReplicationSlotCreate(name, true,
    temporary ? RS_TEMPORARY : RS_EPHEMERAL, two_phase,
-   failover);
+   failover, false);

Same as above. Maybe comment on the parameter like "false /* synced */"

~~~

19. pg_get_replication_slots

  case RS_INVAL_WAL_REMOVED:
- values[i++] = CStringGetTextDatum("wal_removed");
+ values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_REMOVED_TEXT);
  break;

  case RS_INVAL_HORIZON:
- values[i++] = CStringGetTextDatum("rows_removed");
+ values[i++] = CStringGetTextDatum(SLOT_INVAL_HORIZON_TEXT);
  break;

  case RS_INVAL_WAL_LEVEL:
- values[i++] = CStringGetTextDatum("wal_level_insufficient");
+ values[i++] = CStringGetTextDatum(SLOT_INVAL_WAL_LEVEL_TEXT);
  break;

IMO this code and the #defines that it uses can be written and pushed
as an independent patch.

======
src/backend/replication/walsender.c

20. CreateReplicationSlot

  ReplicationSlotCreate(cmd->slotname, false,
    cmd->temporary ? RS_TEMPORARY : RS_PERSISTENT,
-   false, false);
+   false, false, false);

Consider commenting the parameter like "false /* synced */"

~~~

21.
  ReplicationSlotCreate(cmd->slotname, true,
    cmd->temporary ? RS_TEMPORARY : RS_EPHEMERAL,
-   two_phase, failover);
+   two_phase, failover, false);

Consider commenting the parameter like "false /* synced */"

======
src/include/replication/slot.h

22.
+/*
+ * The possible values for 'conflict_reason' returned in
+ * pg_get_replication_slots.
+ */
+#define SLOT_INVAL_WAL_REMOVED_TEXT "wal_removed"
+#define SLOT_INVAL_HORIZON_TEXT     "rows_removed"
+#define SLOT_INVAL_WAL_LEVEL_TEXT   "wal_level_insufficient"

IMO these #defines and also the code in pg_get_replication_slots()
that uses them can be written and pushed as an independent patch.

======
.../t/050_standby_failover_slots_sync.pl

23.
+# Wait for the standby to start sync
+$standby1->start;

But there is no waiting here? Maybe the comment should say like "Start
the standby so that slot syncing can begin"

~~~

24.
+# Wait for the standby to finish sync
+my $offset = -s $standby1->logfile;
+$standby1->wait_for_log(
+ qr/LOG: ( [A-Z0-9]+:)? newly locally created slot \"lsub1_slot\" is
sync-ready now/,
+ $offset);

SUGGESTION
# Wait for the standby to finish slot syncing

~~~

25.
+# Confirm that logical failover slot is created on the standby and is sync
+# ready.
+is($standby1->safe_psql('postgres',
+ q{SELECT failover, synced FROM pg_replication_slots WHERE slot_name
= 'lsub1_slot';}),
+ "t|t",
+ 'logical slot has failover as true and synced as true on standby');

SUGGESTION
# Confirm that the logical failover slot is created on the standby and
is flagged as 'synced'

~~~

26.
+$subscriber1->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE tab_int (a int PRIMARY KEY);
+ ALTER SUBSCRIPTION regress_mysub1 REFRESH PUBLICATION;
+]);
+
+$subscriber1->wait_for_subscription_sync;

Add a comment like

# Subscribe to the new table data and wait for it to arrive

~~~

27.
+# Disable hot_standby_feedback temporarily to stop slot sync worker otherwise
+# the concerned testing scenarios here may be interrupted by different error:
+# 'ERROR:  replication slot is active for PID ..'
+
+$standby1->safe_psql('postgres', 'ALTER SYSTEM SET
hot_standby_feedback = off;');
+$standby1->restart;

Remove the blank line.

~~~

28.
+is($standby1->safe_psql('postgres',
+ q{SELECT slot_name FROM pg_replication_slots WHERE slot_name =
'lsub1_slot';}),
+ 'lsub1_slot',
+ 'synced slot retained on the new primary');

There should be some comment like:

SUGGESTION
# Confirm the synced slot 'lsub1_slot' is retained on the new primary

~~~

29.
+# Confirm that data in tab_int replicated on subscriber
+is( $subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}),
+ "20",
+ 'data replicated from the new primary');

/replicated on subscriber/replicated on the subscriber/

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 11 Jan 2024 07:49:47 +0000

Hi,

> the fields and call these manually ?

I'd vote for using the APIs as I think it will be harder to maintain if we are
not using them (means ensure the "direct" overwriting still makes sense over time).

FWIW, pg_failover_slots also rely on those APIs from what I can see.

Regards,

---

From: shveta malik <[email-masked]>
Date: Thu, 11 Jan 2024 15:03:52 +0530

> not using them (means ensure the "direct" overwriting still makes sense over time).
+1

PFA v60 which addresses:

1) Peter's comment in [1]
2) Peter's off list suggestion to convert sleep_quanta to sleep_ms and
simplify the logic in wait_for_slot_activity()

[1]: https://www.postgresql.org/message-id/CAHut%2BPtJAAPghc4GPt0k%3DjeMz1qu4H7mnaDifOHsVsMqi-qOLA%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Thu, 11 Jan 2024 15:12:38 +0530

> Here are some review comments for patch v58-0002

Thank You for the feedback. These are addressed in v60. Please find my
response inline for a few.

> If you prefer a ternary, it might be cleaner to do it like:

We got a CFBot failure, where the v51's way was crashing in a 32-bit
env, because there a Datum for int64 is regarded as a pointer and thus
it resulted in NULL pointer access if slot_getattr() returned NULL.
Please see DatumGetInt64().

> remote_slot->confirmed_lsn = isnull ? InvalidXLogRecPtr : DatumGetLSN(d);

Okay, I see. This can also be done. I kind of missed this line
earlier, I can consider it in the next version.

> IMO this code and the #defines that it uses can be written and pushed

Okay, let me review this one and #22 which mentions the same.

---

From: Dilip Kumar <[email-masked]>
Date: Thu, 11 Jan 2024 15:42:08 +0530

On Wed, Jan 10, 2024 at 5:53 PM Zhijie Hou (Fujitsu)
<[email-masked]> wrote:

> the fields and call these manually ?

I might be missing something but do you want to call
ReplicationSlotsComputeRequiredXmin() kind of functions in standby?  I
mean those will ultimately update the catalog xmin and replication
xmin in Procarray and that prevents Vacuum from cleaning up some of
the required xids.  But on standby, those shared memory parameters are
not used IIUC.

In my opinion on standby, we just need to update the values in the
local slots and whatever we get from remote slots without taking all
the logical decisions in the hope that they will all fall into a
particular path, for example, if you see LogicalIncreaseXminForSlot(),
it is doing following steps of operations as shown below[1].  These
all make sense when you are doing candidate-based updation where we
first mark the candidates and then update the candidate to real value
once you get the confirmation for the LSN.  Now following all this
logic looks completely weird unless this can fall in a different path
I feel it will do some duplicate steps as well.  For example in
local_slot_update(), first you call LogicalConfirmReceivedLocation()
which will set the 'data.confirmed_flush' and then you will call
LogicalIncreaseXminForSlot() which will set the 'updated_xmin = true;'
and will again call LogicalConfirmReceivedLocation().  I don't think
this is the correct way of reusing the function unless you need to go
through those paths and I am missing something.

[1]
LogicalIncreaseXminForSlot()
{
   if (TransactionIdPrecedesOrEquals(xmin, slot->data.catalog_xmin))
  {
  }
  else if (current_lsn <= slot->data.confirmed_flush)
  {
  }
 else if (slot->candidate_xmin_lsn == InvalidXLogRecPtr)
 {
 }

if (updated_xmin)
  LogicalConfirmReceivedLocation(slot->data.confirmed_flush);
}

---

From: Amit Kapila <[email-masked]>
Date: Thu, 11 Jan 2024 16:22:56 +0530

> we follow this then we probably need to have a check for catalog_xmin

The second point as mentioned is slightly misleading, so let me try to
rephrase it once again: Emit LOG/WARNING in this case and once
remote_slot's LSN moves ahead of local_slot's LSN then we can update
it; additionally, we need to update all other fields like two_phase as
well. If we follow this then we probably need to have a check for
catalog_xmin as well along remote_slot's restart_lsn.

> different. So, is simply copying those sufficient or do we need to do

Bertrand, Dilip, Sawada-San, and others, please share your opinion on
this problem as I think it is important to handle this race condition.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 11 Jan 2024 17:01:15 +0530

> the required xids.  But on standby, those shared memory parameters are

These xmins are required for logical slots as we allow logical
decoding on standby (see GetOldestSafeDecodingTransactionId()). We
also invalidate such slots if the required rows are removed on
standby. Similarly, we need ReplicationSlotsComputeRequiredLSN() to
avoid getting the required WAL removed.

> and will again call LogicalConfirmReceivedLocation().
>

In case (else if (slot->candidate_xmin_lsn == InvalidXLogRecPtr)),
even the updated_xmin is not getting set to true which means there is
a chance that we will never update the required xmin values.

  I don't think
> through those paths and I am missing something.

I agree with this conclusion and also think that we should directly
update the required fields and call functions like
ReplicationSlotsComputeRequiredLSN() wherever required.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 11 Jan 2024 15:41:47 +0000

Hi,

> > to invalidated slots on standby.

I also think so.

> > we follow this then we probably need to have a check for catalog_xmin

IIUC, this would be a sync slot (so not usable until promotion) that could
not be used anyway (invalidated), so I'll vote for drop / re-create then.

> > different. So, is simply copying those sufficient or do we need to do

I'm not sure to follow here. If the remote slot is re-created then it would
be also dropped / re-created locally, or am I missing something?

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 12 Jan 2024 03:46:00 +0000

On Thursday, January 11, 2024 11:42 PM Bertrand Drouvot <[email-masked]> wrote:

Hi,
 
> > > +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot
> > > +*remote_slot)
> > > {
> > > ...
> > > + /* Slot ready for sync, so sync it. */ else {
> > > + /*
> > > + if (remote_slot->restart_lsn < slot->data.restart_lsn) elog(ERROR,
> > > I don't think as mentioned in comments even if hot_standby_feedback
> > > is temporarily set to off, the above shouldn't happen. It can only
> > > lead to invalidated slots on standby.
> 
> I also think so.
> 
> > >
> > > your previous comment, we need to update all other fields as well.
> > > If we follow this then we probably need to have a check for
> > > catalog_xmin as well.
> 
> IIUC, this would be a sync slot (so not usable until promotion) that could not be
> used anyway (invalidated), so I'll vote for drop / re-create then.

Such race can happen when user drop and re-create the same failover slot on
primary as well. For example, user dropped one failover slot and them
immediately created a new one by copying from an old slot(using
pg_copy_logical_replication_slot). Then the slotsync worker will find the
restart_lsn of this slot go backwards.

The steps:
----
SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'pgoutput', false, false, true);
SELECT 'init' FROM pg_create_logical_replication_slot('test', 'pgoutput', false, false, true);

- Advance the restart_lsn of 'test' slot
CREATE TABLE test2(a int);
INSERT INTO test2 SELECT generate_series(1,10000,1);
SELECT slot_name FROM pg_replication_slot_advance('test', pg_current_wal_lsn());

- re-create the test slot but based on the old isolation_slot.
SELECT pg_drop_replication_slot('test');
SELECT 'copy' FROM pg_copy_logical_replication_slot('isolation_slot', 'test');

Then the restart_lsn of 'test' slot will go backwards.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 12 Jan 2024 11:11:09 +0530

> not be used anyway (invalidated), so I'll vote for drop / re-create then.

The one more drawback I see is that in such a case (where the slot
could have dropped on primary) is that it is not advisable to keep it
on standby. So, I also think we should drop and re-create the slots in
this case unless I am missing something here.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 12 Jan 2024 06:54:09 +0000

Hi,

> Then the restart_lsn of 'test' slot will go backwards.

Yeah, that's right.

BTW, I think it's worth to add those "corner cases" in the TAP tests related to
the sync slot feature (the more coverage the better).

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 12 Jan 2024 21:00:06 +0900

> this problem as I think it is important to handle this race condition.

Is there any good use case of copying a failover slot in the first
place? If it's not a normal use case and we can probably live without
it, why not always disable failover during the copy? FYI we always
disable two_phase on copied slots. It seems to me that copying a
failover slot could lead to problems, as long as we synchronize slots
based on their names. IIUC without the copy, this pass should never
happen.

Regards,

-- 
Masahiko Sawada

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 12 Jan 2024 14:21:14 +0000

On Friday, January 12, 2024 8:00 PM Masahiko Sawada <[email-masked]> wrote:

Hi,

> > > Now, related to this the other case which needs some handling is
> > > what if the remote_slot's restart_lsn is greater than local_slot's
> > > case, I think the other properties like 'two_phase', 'plugin' could
> > > be different. So, is simply copying those sufficient or do we need
> > > to do something else as well?
> > >
> >
> Is there any good use case of copying a failover slot in the first place? If it's not
> a normal use case and we can probably live without it, why not always disable
> failover during the copy? FYI we always disable two_phase on copied slots. It
> seems to me that copying a failover slot could lead to problems, as long as we
> synchronize slots based on their names. IIUC without the copy, this pass should
> never happen.

Thanks for the suggestion. I also don't have a use case for this.
Attach the V61 patch set that addresses this suggestion. And here is the
summary of the changes made in each patch.

V61-0001
1. Reverts the changes in copy_replication_slot.

V61-0002

1. Adds the documents for the steps that user needs to follow to ensure the
   standby is ready for failover
2. Directly update the fields restart_lsn/confirmed_flush/catalog_xmin instead
   of using APIs like LogicalConfirmReceivedLocation
3. Updates all the fields(two_phase, failover, plugin) when syncing the slots
4. fixes CFbot failures.
5. Some code style adjustment. (pending comments in last version)
6. Remove some unnecessary Assert and variable assignment (off-list comments from Peter)

Thanks Shveta for working on 4 and 5.

V61-0003
1. Some documents update related to standby_slot_names and the steps for
   failover.

V61-0004
- No change.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Tue, 16 Jan 2024 12:27:26 +1100

Here are some review comments for patch v61-0002

======
doc/src/sgml/logical-replication.sgml

1.
+  <sect2 id="logical-replication-failover-examples">
+   <title>Examples: logical replication failover</title>

The current documentation structure (after the patch is applied) looks
like this:

30.1. Publication
30.2. Subscription
    30.2.1. Replication Slot Management
    30.2.2. Examples: Set Up Logical Replication
    30.2.3. Examples: Deferred Replication Slot Creation
    30.2.4. Examples: logical replication failover

I don't think it is ideal.

Firstly, I think this new section is not just "Examples:"; it is more
like instructions for steps to check if a successful failover is
possible. IMO call it something like "Logical Replication Failover" or
"Replication Slot Failover".

Secondly, I don't think this new section strictly belongs underneath
the "Subscription" section anymore because IMO it is just as much
about the promotion of the publications. Now that you are adding this
new (2nd) section about slots, I think the whole structure of this
document should be changed like below:

SUGGESTION #1 (make a new section 30.3 just for slot-related topics)

30.1. Publication
30.2. Subscription
    30.2.1. Examples: Set Up Logical Replication
30.3. Logical Replication Slots
    30.3.1. Replication Slot Management
    30.3.2. Examples: Deferred Replication Slot Creation
    30.3.3. Logical Replication Failover

~

SUGGESTION #2 (keep the existing structure, but give the failover its
own new section 30.3)

30.1. Publication
30.2. Subscription
    30.2.1. Replication Slot Management
    30.2.2. Examples: Set Up Logical Replication
    30.2.3. Examples: Deferred Replication Slot Creation
30.3 Logical Replication Failover

~

SUGGESTION #2a (and maybe later you can extract some of the failover
examples further)

30.1. Publication
30.2. Subscription
    30.2.1. Replication Slot Management
    30.2.2. Examples: Set Up Logical Replication
    30.2.3. Examples: Deferred Replication Slot Creation
30.3 Logical Replication Failover
    30.3.1. Examples: Checking if failover ready

~~~

2.
+   <para>
+    In a logical replication setup, if the publisher server is also the primary
+    server of the streaming replication, the logical slots on the
primary server
+    can be synchronized to the standby server by specifying
<literal>failover = true</literal>
+    when creating the subscription. Enabling failover ensures a seamless
+    transition of the subscription to the promoted standby, allowing it to
+    subscribe to the new primary server without any data loss.
+   </para>

I was initially confused by the wording. How about like below:

SUGGESTION
When the publisher server is the primary server of a streaming
replication, the logical slots on that primary server can be
synchronized to the standby server by specifying <literal>failover =
true</literal> when creating subscriptions for those publications.
Enabling failover ensures a seamless transition of those subscriptions
after the standby is promoted. They can continue subscribing to
publications now on the new primary server without any data loss.

~~~

3.
+   <para>
+    However, the replication slots are copied asynchronously, which
means it's necessary
+    to confirm that replication slots have been synced to the standby server
+    before the failover happens. Additionally, to ensure a successful failover,
+    the standby server must not lag behind the subscriber. To confirm
+    that the standby server is ready for failover, follow these steps:
+   </para>

Minor rewording

SUGGESTION
Because the slot synchronization logic copies asynchronously, it is
necessary to confirm that replication slots have been synced to the
standby server before the failover happens. Furthermore, to ensure a
successful failover, the standby server must not be lagging behind the
subscriber. To confirm that the standby server is indeed ready for
failover, follow these 2 steps:

~~~

4.
The instructions said "follow these steps", so the next parts should
be rendered as 2 "steps" (using <procedure> markup?)

SUGGESTION (show as steps 1,2  and also some minor rewording of the
step heading)

1. Confirm that all the necessary logical replication slots have been
synced to the standby server.
2. Confirm that the standby server is not lagging behind the subscribers.

~~~

5.
+   <para>
+    Check if all the necessary logical replication slots have been synced to
+    the standby server.
+   </para>

SUGGESTION
Confirm that all the necessary logical replication slots have been
synced to the standby server.

~~~

6.
+     <listitem>
+      <para>
+       On logical subscriber, fetch the slot names that should be synced to the
+       standby that we plan to promote.

SUGGESTION
Firstly, on the subscriber node, use the following SQL to identify the
slot names that should be...

~~~

7.
+<programlisting>
+test_sub=# SELECT
+               array_agg(slotname) AS slots
+           FROM
+           ((
+               SELECT r.srsubid AS subid, CONCAT('pg_' || srsubid ||
'_sync_' || srrelid || '_' || ctl.system_identifier) AS slotname
+               FROM pg_control_system() ctl, pg_subscription_rel r,
pg_subscription s
+               WHERE r.srsubstate = 'f' AND s.oid = r.srsubid AND s.subfailover
+           ) UNION (
+               SELECT oid AS subid, subslotname as slotname
+               FROM pg_subscription
+               WHERE subfailover
+           ));

7a
Maybe this ought to include "pg_catalog" schemas?

~

7b.
For consistency, maybe it is better to use a table alias "FROM
pg_subscription s" in the UNION also

~~~

8.
+     <listitem>
+      <para>
+       Check that the logical replication slots exist on the standby server.

SUGGESTION
Next, check that the logical replication slots identified above exist
on the standby server.

~~~

9.
+<programlisting>
+test_standby=# SELECT bool_and(synced AND NOT temporary AND
conflict_reason IS NULL) AS failover_ready
+               FROM pg_replication_slots
+               WHERE slot_name in ('slots');
+ failover_ready
+----------------
+ t

9a.
Maybe this ought to include "pg_catalog" schemas?

~

9b.
IIUC that 'slots' reference is supposed to be those names that were
found in the prior step. If so, then that point needs to be made
clear, and anyway in this case 'slots' is not compatible with the
'sub' name returned by your first SQL.

~~~

10.
+     <listitem>
+      <para>
+       Query the last replayed WAL on the logical subscriber.

SUGGESTION
Firstly, on the subscriber node check the last replayed WAL.

~~~

11.
+<programlisting>
+test_sub=# SELECT
+               MAX(remote_lsn) AS remote_lsn_on_subscriber
+           FROM
+           ((
+               SELECT (CASE WHEN r.srsubstate = 'f' THEN
pg_replication_origin_progress(CONCAT('pg_' || r.srsubid || '_' ||
r.srrelid), false)
+                           WHEN r.srsubstate = 's' THEN r.srsublsn
END) as remote_lsn
+               FROM pg_subscription_rel r, pg_subscription s
+               WHERE r.srsubstate IN ('f', 's') AND s.oid = r.srsubid
AND s.subfailover
+           ) UNION (
+               SELECT pg_replication_origin_progress(CONCAT('pg_' ||
s.oid), false) AS remote_lsn
+               FROM pg_subscription s
+               WHERE subfailover
+           ));

11a.
Maybe this ought to include "pg_catalog" schemas?

~

11b.
/WHERE subfailover/WHERE s.subfailover/

~~~

12.
+     <listitem>
+      <para>
+       On the standby server, check that the last-received WAL location
+       is ahead of the replayed WAL location on the subscriber.

SUGGESTION
Next, on the standby server check that the last-received WAL location
is ahead of the replayed WAL location on the subscriber identified
above.

~~~

13.
+</programlisting></para>
+     </listitem>
+     <listitem>
+      <para>
+       On the standby server, check that the last-received WAL location
+       is ahead of the replayed WAL location on the subscriber.
+<programlisting>
+test_standby=# SELECT pg_last_wal_receive_lsn() >=
'remote_lsn_on_subscriber'::pg_lsn AS failover_ready;
+ failover_ready
+----------------
+ t

IIUC the 'remote_lsn_on_subscriber' is supposed to represent the
substitution of the value found in the subscriber server. In this
example maybe it would be:
SELECT pg_last_wal_receive_lsn() >= '0/3000388'::pg_lsn AS failover_ready;

maybe that point can be made more clearly.

~~~

14.
+   <para>
+    If the result (failover_ready) of both above steps is true, it means it is
+    okay to subscribe to the standby server.
+   </para>

14a.
failover_ready should be rendered as literal.

~

14b.
Does this say what you intended, or did you mean something more like
"the standby can be promoted and existing subscriptions will be able
to continue without data loss"

======

15. local_slot_update

+/*
+ * Try to update local slot metadata based on the data from the remote slot.
+ *
+ * Return false if the data of the remote slot is the same as the local slot.
+ * Otherwise, return true.
+ */

There's not really any "try to" here; it either does it if needed or
doesn't do it because it's not needed.

SUGGESTION
If necessary, update local slot metadata based on the data from the remote slot.

If no update was needed (the data of the remote slot is the same as
the local slot)
return false, otherwise true.

~~~

16.
+ bool updated_xmin;
+ bool updated_restart;
+ Oid dbid;
+ ReplicationSlot *slot = MyReplicationSlot;
+
+ Assert(slot->data.invalidated == RS_INVAL_NONE);
+
+ updated_xmin = (remote_slot->catalog_xmin != slot->data.catalog_xmin);
+ updated_restart = (remote_slot->restart_lsn != slot->data.restart_lsn);
+ dbid = get_database_oid(remote_slot->database, false);
+
+ if (namestrcmp(&slot->data.plugin, remote_slot->plugin) == 0 &&
+ slot->data.database == dbid && !updated_restart && !updated_xmin &&
+ remote_slot->two_phase == slot->data.two_phase &&
+ remote_slot->failover == slot->data.failover &&
+ remote_slot->confirmed_lsn == slot->data.confirmed_flush)
+ return false;

It seems a bit strange to have boolean flags for some of the
differences (updated_xmin, updated_restart) but not for the others. I
expected it should be for all (e.g. updated_twophase,
updated_failover, ...) or none of them.

~~~

17. synchronize_one_slot

+ slot_updated = local_slot_update(remote_slot);
+
+ /* Make sure the slot changes persist across server restart */
+ if (slot_updated)
+ {
+ ReplicationSlotMarkDirty();
+ ReplicationSlotSave();
+ }

IMO this code would be simpler if written like below because then
'slot_updated' is only ever assigned when true instead of maybe
overwriting the default again with false:

SUGGESTION
/* Make sure the slot changes persist across server restart */
if (local_slot_update(remote_slot))
{
  slot_updated = true;
  ReplicationSlotMarkDirty();
  ReplicationSlotSave();
}

======
src/backend/replication/slot.c

18. ReplicationSlotPersist - TEMPORARY v EPHEMERAL

I noticed this ReplicationSlotPersist() from v59-0002 was reverted:

- * Convert a slot that's marked as RS_EPHEMERAL to a RS_PERSISTENT slot,
- * guaranteeing it will be there after an eventual crash.
+ * Convert a slot that's marked as RS_EPHEMERAL or RS_TEMPORARY to a
+ * RS_PERSISTENT slot, guaranteeing it will be there after an eventual crash.

AFAIK in v61 you are still calling this function with RS_TEMPORARY
which is now contrary to the current function comment if you don't
change it to also mention RS_TEMPORARY.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 16 Jan 2024 12:00:29 +0000

> Here are some review comments for patch v61-0002

Thanks for the comments.

> 30.3 Logical Replication Failover

I used this version for now as I am sure about changing other section.

> publications now on the new primary server without any data loss.

Changed as suggested.

> failover, follow these 2 steps:

Changed as suggested.

> 2. Confirm that the standby server is not lagging behind the subscribers.

Changed as suggested.

> synced to the standby server.

Changed as suggested.

> slot names that should be...

Changed as suggested.

> Maybe this ought to include "pg_catalog" schemas?

After searching other query examples, I think most of them don’t add this for
either function or system table. So, I didn’t add this.

> pg_subscription s" in the UNION also

Added.

> Next, check that the logical replication slots identified above exist

Changed as suggested.

> +               WHERE slot_name in ('slots');

Same as above.

> 'sub' name returned by your first SQL.

Changed as suggested.

> Firstly, on the subscriber node check the last replayed WAL.
Changed as suggested.

> Maybe this ought to include "pg_catalog" schemas?

Same as above.

> is ahead of the replayed WAL location on the subscriber identified

Changed as suggested.

> 'remote_lsn_on_subscriber'::pg_lsn AS failover_ready;

I have changed it to use the actual LSN got in last step.

> failover_ready should be rendered as literal.

Added.

> to continue without data loss"

I used the later part of your suggestion as I think promotion
depends not only on logical replication part.

Best Regards,
Hou zj