Subject: RE: Synchronizing slots from primary to standby

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 27 Feb 2024 01:42:59 +0000

On Monday, February 26, 2024 1:19 PM Amit Kapila <[email-masked]> wrote:
> 
> On Fri, Feb 23, 2024 at 4:45 PM Bertrand Drouvot
> <[email-masked]> wrote:
> >
> > On Fri, Feb 23, 2024 at 09:46:00AM +0000, Zhijie Hou (Fujitsu) wrote:
> > >
> > > Besides, I'd like to clarify and discuss the behavior of standby_slot_names
> once.
> > >
> > > As it stands in the patch, If the slots specified in
> > > standby_slot_names are dropped or invalidated, the logical walsender
> > > will issue a WARNING and continue to replicate the changes. Another
> > > option for this could be to have the walsender pause until the slot
> > > in standby_slot_names is re-created or becomes valid again. Does anyone
> else have an opinion on this matter ?
> >
> > Good point, I'd vote for: the only reasons not to wait are:
> >
> > - slots mentioned in standby_slot_names exist and valid and do catch
> > up or
> > - standby_slot_names is empty
> >
> > The reason is that setting standby_slot_names to a non empty value
> > means that one wants the walsender to wait until the standby catchup.
> > The way to remove this intentional behavior should be by changing the
> > standby_slot_names value (not the existence or the state of the slot(s) it
> points too).
> >
> 
> It seems we already do wait for the case when there is an inactive slot as per the
> below code [1] in the patch. So, probably waiting in other cases is also okay and
> also as this parameter is a SIGHUP parameter, users should be easily able to
> change its value if required. Do you think it is a good idea to mention this in
> docs as well?
> 
> I think it is important to raise WARNING as the patch is doing in all the cases
> where the slot is not being processed so that users can be notified and they can
> take the required action.

Agreed. Here is the V99 patch which addressed the above.

This version also includes:
1. list_free the slot list when reloading the list due to GUC change.
2. Refactored the validate_standby_slots based on Shveta's suggestion.
3. Added errcode for the warnings as most of existing have errcodes.

Amit's latest comments[1] are pending, we will address that in next version.

[1] https://www.postgresql.org/message-id/CAA4eK1LJdmGATWG%3DxOD1CB9cogukk2cLNBGH8h-n-ZDJuwBdJg%40mail.gmail.com

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Tue, 27 Feb 2024 18:17:44 +1100

Here are some review comments for v99-0001

==========
0. GENERAL.

+#standby_slot_names = '' # streaming replication standby server slot names that
+ # logical walsender processes will wait for

IMO the GUC name is too generic. There is nothing in this name to
suggest it has anything to do with logical slot synchronization; that
meaning is only found in the accompanying comment -- it would be
better if the GUC name itself were more self-explanatory.

e.g. Maybe like 'wal_sender_sync_standby_slot_names' or
'wal_sender_standby_slot_names', 'wal_sender_wait_for_standby_slots',
or ...

(Of course, this has some impact across docs and comments and
variables in the patch).

==========
Commit Message

1.
A new parameter named standby_slot_names is introduced.

Maybe quote the GUC names here to make it more readable.

~~

2.
Additionally, The SQL functions pg_logical_slot_get_changes and
pg_replication_slot_advance are modified to wait for the replication slots
mentioned in standby_slot_names to catch up before returning the changes
to the user.

~

2a.
"pg_replication_slot_advance" is a typo? Did you mean
pg_logical_replication_slot_advance?

~

2b.
The "before returning the changes to the user" seems like it is
referring only to the first function.

Maybe needs slight rewording like:
/before returning the changes to the user./ before returning./

==========
doc/src/sgml/config.sgml

3. standby_slot_names

+       <para>
+        List of physical slots guarantees that logical replication slots with
+        failover enabled do not consume changes until those changes
are received
+        and flushed to corresponding physical standbys. If a logical
replication
+        connection is meant to switch to a physical standby after the
standby is
+        promoted, the physical replication slot for the standby
should be listed
+        here. Note that logical replication will not proceed if the slots
+        specified in the standby_slot_names do not exist or are invalidated.
+       </para>

The wording doesn't seem right. IMO this should be worded much like
how this GUC is described in guc_tables.c

e.g something a bit like:

Lists the streaming replication standby server slot names that logical
WAL sender processes will wait for. Logical WAL sender processes will
send decoded changes to plugins only after the specified replication
slots confirm receiving WAL. This guarantees that logical replication
slots with failover enabled do not consume changes until those changes
are received and flushed to corresponding physical standbys...

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

4. Section 48.2.3 Replication Slot Synchronization

+     It's also highly recommended that the said physical replication slot
+     is named in
+     <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>
+     list on the primary, to prevent the subscriber from consuming changes
+     faster than the hot standby. But once we configure it, then
certain latency
+     is expected in sending changes to logical subscribers due to wait on
+     physical replication slots in
+     <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>

4a.
/It's also highly/It is highly/

~

4b.

BEFORE
But once we configure it, then certain latency is expected in sending
changes to logical subscribers due to wait on physical replication
slots in <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>

SUGGESTION
Even when correctly configured, some latency is expected when sending
changes to logical subscribers due to the waiting on slots named in
standby_slot_names.

==========
.../replication/logical/logicalfuncs.c

5. pg_logical_slot_get_changes_guts

+ if (XLogRecPtrIsInvalid(upto_lsn))
+ wait_for_wal_lsn = end_of_wal;
+ else
+ wait_for_wal_lsn = Min(upto_lsn, end_of_wal);
+
+ /*
+ * Wait for specified streaming replication standby servers (if any)
+ * to confirm receipt of WAL up to wait_for_wal_lsn.
+ */
+ WaitForStandbyConfirmation(wait_for_wal_lsn);

Perhaps those statements all belong together with the comment up-front. e.g.

+ /*
+ * Wait for specified streaming replication standby servers (if any)
+ * to confirm receipt of WAL up to wait_for_wal_lsn.
+ */
+ if (XLogRecPtrIsInvalid(upto_lsn))
+ wait_for_wal_lsn = end_of_wal;
+ else
+ wait_for_wal_lsn = Min(upto_lsn, end_of_wal);
+ WaitForStandbyConfirmation(wait_for_wal_lsn);

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

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

6.
+static bool
+validate_standby_slots(char **newval)
+{
+ char    *rawname;
+ List    *elemlist;
+ ListCell   *lc;
+ bool ok;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(*newval);
+
+ /* Verify syntax and parse string into a list of identifiers */
+ ok = SplitIdentifierString(rawname, ',', &elemlist);
+
+ if (!ok)
+ {
+ GUC_check_errdetail("List syntax is invalid.");
+ }
+
+ /*
+ * If the replication slots' data have been initialized, verify if the
+ * specified slots exist and are logical slots.
+ */
+ else if (ReplicationSlotCtl)
+ {
+ foreach(lc, elemlist)

6a.
So, if the ReplicationSlotCtl is NULL, it is possible to return
ok=true without ever checking if the slots exist or are of the correct
kind. I am wondering what are the ramifications of that. -- e.g.
assuming names are OK when maybe they aren't OK at all. AFAICT this
works because it relies on getting subsequent WARNINGS when calling
FilterStandbySlots(). If that is correct then maybe the comment here
can be enhanced to say so.

Indeed, if it works like that, now I am wondering do we need this for
loop validation at all. e.g. it seems just a matter of timing whether
we get ERRORs validating the GUC here, or WARNINGS later in the
FilterStandbySlots. Maybe we don't need the double-checking and it is
enough to check in FilterStandbySlots?

~

6b.
AFAIK there are alternative foreach macros available now, so you
shouldn't need to declare the ListCell.

~~~

7. check_standby_slot_names

+bool
+check_standby_slot_names(char **newval, void **extra, GucSource source)
+{
+ if (strcmp(*newval, "") == 0)
+ return true;

Using strcmp seems like an overkill way to check for empty string.

SUGGESTION

if (*newval == '\0')
  return true;

~~~

8.
+ if (strcmp(*newval, "*") == 0)
+ {
+ GUC_check_errdetail("\"%s\" is not accepted for standby_slot_names",
+ *newval);
+ return false;
+ }

It seems overkill to use a format specifier when "*" is already the known value.

SUGGESTION
GUC_check_errdetail("Wildcard \"*\" is not accepted for standby_slot_names.");

~~~

9.
+ /* Now verify if the specified slots really exist and have correct type */
+ if (!validate_standby_slots(newval))
+ return false;

As in a prior comment, if ReplicationSlotCtl is NULL then it is not
always going to do exactly what that comment says it is doing...

~~~

10. assign_standby_slot_names

+ if (!SplitIdentifierString(standby_slot_names_cpy, ',', &standby_slots))
+ {
+ /* This should not happen if GUC checked check_standby_slot_names. */
+ elog(ERROR, "invalid list syntax");
+ }

I didn't see how it is possible to get here without having first
executed check_standby_slot_names. But, if it can happen, then maybe
describe the scenario in the comment.

~~~

11.
+ * Note that since we do not support syncing slots to cascading standbys, we
+ * return NIL if we are running in a standby to indicate that no standby slots
+ * need to be waited for, regardless of the copy flag value.

I didn't understand the relevance of even mentioning "regardless of
the copy flag value".

~~~

12. FilterStandbySlots

+ errhint("Consider starting standby associated with \"%s\" or amend
standby_slot_names.",
+ name));

This errhint should use a format substitution for the GUC
"standby_slot_names" for consistency with everything else.

~~~

13. WaitForStandbyConfirmation

+ /*
+ * We wait for the slots in the standby_slot_names to catch up, but we
+ * use a timeout (1s) so we can also check the if the
+ * standby_slot_names has been changed.
+ */
+ ConditionVariableTimedSleep(&WalSndCtl->wal_confirm_rcv_cv, 1000,
+ WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION);

Typo "the if the"

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

14. pg_physical_replication_slot_advance
+
+ PhysicalWakeupLogicalWalSnd();

Should this have a comment to say what it is for?

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

15.
+/*
+ * Wake up the logical walsender processes with failover enabled slots if the
+ * currently acquired physical slot is specified in standby_slot_names GUC.
+ */
+void
+PhysicalWakeupLogicalWalSnd(void)
+{
+ ListCell   *lc;
+ List    *standby_slots;
+
+ Assert(MyReplicationSlot && SlotIsPhysical(MyReplicationSlot));
+
+ standby_slots = GetStandbySlotList(false);
+
+ foreach(lc, standby_slots)
+ {
+ char    *name = lfirst(lc);
+
+ if (strcmp(name, NameStr(MyReplicationSlot->data.name)) == 0)
+ {
+ ConditionVariableBroadcast(&WalSndCtl->wal_confirm_rcv_cv);
+ return;
+ }
+ }
+}

15a.
There already exists another function called WalSndWakeup(bool
physical, bool logical), so I think this new one should use a similar
name pattern -- e.g. maybe like WalSndWakeupLogicalForSlotSync or ...

~

15b.
IIRC there are some new List macros you can use instead of needing to
declare the ListCell?

==========
.../utils/activity/wait_event_names.txt

16.
+WAIT_FOR_STANDBY_CONFIRMATION "Waiting for the WAL to be received by
physical standby."

Moving the 'the' will make this more consistent with all other
"Waiting for WAL..." names.

SUGGESTION
Waiting for WAL to be received by the physical standby.

==========
src/backend/utils/misc/guc_tables.c

17.
+ {
+ {"standby_slot_names", PGC_SIGHUP, REPLICATION_PRIMARY,
+ gettext_noop("Lists streaming replication standby server slot "
+ "names that logical WAL sender processes will wait for."),
+ gettext_noop("Decoded changes are sent out to plugins by logical "
+ "WAL sender processes only after specified "
+ "replication slots confirm receiving WAL."),
+ GUC_LIST_INPUT | GUC_LIST_QUOTE
+ },
+ &standby_slot_names,
+ "",
+ check_standby_slot_names, assign_standby_slot_names, NULL
+ },

The wording of the detail msg feels kind of backwards to me.

BEFORE
Decoded changes are sent out to plugins by logical WAL sender
processes only after specified replication slots confirm receiving
WAL.

SUGGESTION
Logical WAL sender processes will send decoded changes to plugins only
after the specified replication slots confirm receiving WAL.

==========
src/backend/utils/misc/postgresql.conf.sample

18.
+#standby_slot_names = '' # streaming replication standby server slot names that
+ # logical walsender processes will wait for

I'm not sure this is the best GUC name. See the general comment #0
above in this post.

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

==========
src/include/replication/walsender.h

==========
src/include/replication/walsender_private.h

==========
src/include/utils/guc_hooks.h

==========
src/test/recovery/t/006_logical_decoding.pl

19.
+# Pass failover=true (last-arg), it should not have any impact on advancing.

SUGGESTION
Passing failover=true (last arg) should not have any impact on advancing.

==========
.../t/040_standby_failover_slots_sync.pl

20.
+#
+# | ----> standby1 (primary_slot_name = sb1_slot)
+# | ----> standby2 (primary_slot_name = sb2_slot)
+# primary ----- |
+# | ----> subscriber1 (failover = true)
+# | ----> subscriber2 (failover = false)

In the diagram, the "--->" means a mixture of physical standbys and
logical pub/sub replication. Maybe it can be a bit clearer?

SUGGESTION

# primary (publisher)
#
#     (physical standbys)
#     | ----> standby1 (primary_slot_name = sb1_slot)
#     | ----> standby2 (primary_slot_name = sb2_slot)
#
#     (logical replication)
#     | ----> subscriber1 (failover = true, slot_name = lsub1_slot)
#     | ----> subscriber2 (failover = false, slot_name = lsub2_slot)

~~~

21.
+# Set up is configured in such a way that the logical slot of subscriber1 is
+# enabled failover, thus it will wait for the physical slot of
+# standby1(sb1_slot) to catch up before sending decoded changes to subscriber1.

/is enabled failover/is enabled for failover/

~~~

22.
+# Create another subscriber node without enabling failover, wait for sync to
+# complete
+my $subscriber2 = PostgreSQL::Test::Cluster->new('subscriber2');
+$subscriber2->init;
+$subscriber2->start;
+$subscriber2->safe_psql(
+ 'postgres', qq[
+ CREATE TABLE tab_int (a int PRIMARY KEY);
+ CREATE SUBSCRIPTION regress_mysub2 CONNECTION '$publisher_connstr'
PUBLICATION regress_mypub WITH (slot_name = lsub2_slot);
+]);
+
+$subscriber1->wait_for_subscription_sync;
+

Is this meant to wait for 'subscription2'?

~~~

23.
# Stop the standby associated with the specified physical replication slot so
# that the logical replication slot won't receive changes until the standby
# comes up.

Maybe this can give the values for better understanding:

SUGGESTION
Stop the standby associated with the specified physical replication
slot (sb1_slot) so that the logical replication slot (lsub1_slot)
won't receive changes until the standby comes up.

~~~

24.
+# Wait for the standby that's up and running gets the data from primary

SUGGESTION
Wait until the standby2 that's still running gets the data from the primary.

~~~

25.
+# Wait for the subscription that's up and running and is not enabled
for failover.
+# It gets the data from primary without waiting for any standbys.

SUGGESTION
Wait for subscription2 to get the data from the primary. This
subscription was not enabled for failover so it gets the data without
waiting for any standbys.

~~~

26.
+# The subscription that's up and running and is enabled for failover
+# doesn't get the data from primary and keeps waiting for the
+# standby specified in standby_slot_names.

SUGGESTION
The subscription1 was enabled for failover so it doesn't get the data
from primary and keeps waiting for the standby specified in
standby_slot_names (sb1_slot aka standby1).

~~~

27.
+# Start the standby specified in standby_slot_names and wait for it to catch
+# up with the primary.

SUGGESTION
Start the standby specified in standby_slot_names (sb1_slot aka
standby1) and wait for it to catch up with the primary.

~~~

28.
+# Now that the standby specified in standby_slot_names is up and running,
+# primary must send the decoded changes to subscription enabled for failover
+# While the standby was down, this subscriber didn't receive any data from
+# primary i.e. the primary didn't allow it to go ahead of standby.

SUGGESTION
Now that the standby specified in standby_slot_names is up and
running, the primary can send the decoded changes to the subscription
enabled for failover (i.e. subscription1). While the standby was down,
subscription1 didn't receive any data from the primary. i.e. the
primary didn't allow it to go ahead of standby.

~~~

29.
+# Stop the standby associated with the specified physical replication slot so
+# that the logical replication slot won't receive changes until the standby
+# slot's restart_lsn is advanced or the slot is removed from the
+# standby_slot_names list.
+$primary->safe_psql('postgres', "TRUNCATE tab_int;");
+$primary->wait_for_catchup('regress_mysub1');
+$standby1->stop;

Isn't this fragment more like the first step of the *next* TEST
instead of the last step of this one?

~~~

30.
+##################################################
+# Verify that when using pg_logical_slot_get_changes to consume changes from a
+# logical slot with failover enabled, it will also wait for the slots specified
+# in standby_slot_names to catch up.
+##################################################

AFAICT this test is checking only that the function cannot return
while waiting for the stopped standby, but it doesn't seem to check
that it *does* return when the stopped standby comes alive again.

~~~

31.
+$result =
+  $subscriber1->safe_psql('postgres', "SELECT count(*) = 0 FROM tab_int;");
+is($result, 't',
+ "subscriber1 doesn't get data as the sb1_slot doesn't catch up");

Do you think this fragment should have a comment?

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

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 27 Feb 2024 10:37:40 +0000

Hi,

> enough to check in FilterStandbySlots?

Good point, I have the feeling that it is enough to check in FilterStandbySlots().

Indeed, if the value is syntactically correct, then I think that its actual value
"really" matters when the logical decoding is starting/running, does it provide
additional benefits "before" that?

Regards,

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 27 Feb 2024 17:29:46 +0530

> Good point, I have the feeling that it is enough to check in FilterStandbySlots().

I think it is better if we get earlier in a case where the parameter
is changed and performed SIGHUP instead of waiting till we get to
logical decoding. So, there is merit in keeping these checks during
initial validation.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 27 Feb 2024 18:05:14 +0530

> Here are some review comments for v99-0001

It would be wrong and or misleading to append wal_sender to this GUC
name as this is used during SQL APIs as well. Also, adding wait sounds
more like a boolean. So, I don't see the proposed names any better
than the current one.

> always going to do exactly what that comment says it is doing...

It will do what the comment says when invoked as part of the SIGHUP
signal. I think the current comment is okay.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 28 Feb 2024 02:23:27 +0000

> Here are some review comments for v99-0001

Thanks for the comments!

> Maybe quote the GUC names here to make it more readable.

Added.

> mentioned in standby_slot_names to catch up before returning the changes to
> the user.
> 
> ~
> 
> 2a.

pg_logical_replication_slot_advance is not a user visible function. So the
pg_replication_slot_advance is correct.

> 
> ~
> 
> 2b.
> The "before returning the changes to the user" seems like it is referring only to
> the first function.
> 

Changed.

> The wording doesn't seem right. IMO this should be worded much like how this
> GUC is described in guc_tables.c
> 
> Lists the streaming replication standby server slot names that logical WAL
> sender processes will wait for. Logical WAL sender processes will send
> decoded changes to plugins only after the specified replication slots confirm
> receiving WAL. This guarantees that logical replication slots with failover
> enabled do not consume changes until those changes are received and flushed
> to corresponding physical standbys...

Changed.

> +     <link
> linkend="guc-standby-slot-names"><varname>standby_slot_names</varna
> me></link>
> +     <link
> linkend="guc-standby-slot-names"><varname>standby_slot_names</varna
> me>
> + </link>
> 
> 4a.
> But once we configure it, then certain latency is expected in sending changes
> to logical subscribers due to wait on physical replication slots in <link
> linkend="guc-standby-slot-names"><varname>standby_slot_names</varna
> me></link>
> 
> SUGGESTION

Changed.

Changed.

> + /* Need a modifiable copy of string */ rawname = pstrdup(*newval);
> +
> + /* Verify syntax and parse string into a list of identifiers */ ok =
> + SplitIdentifierString(rawname, ',', &elemlist);
> +
> + if (!ok)
> + {
> + GUC_check_errdetail("List syntax is invalid."); }
> +
> + /*
> So, if the ReplicationSlotCtl is NULL, it is possible to return ok=true without
> ever checking if the slots exist or are of the correct kind. I am wondering what
> are the ramifications of that. -- e.g.
> assuming names are OK when maybe they aren't OK at all. AFAICT this works
> because it relies on getting subsequent WARNINGS when calling
> FilterStandbySlots(). If that is correct then maybe the comment here can be
> enhanced to say so.
> 
> Indeed, if it works like that, now I am wondering do we need this for loop
> validation at all. e.g. it seems just a matter of timing whether we get ERRORs
> validating the GUC here, or WARNINGS later in the FilterStandbySlots. Maybe
> we don't need the double-checking and it is enough to check in
> FilterStandbySlots?

I think the check is OK so didn’t change this.

> 
> ~
> 
> 6b.
> AFAIK there are alternative foreach macros available now, so you shouldn't
> need to declare the ListCell.

Changed.

> +{  if (strcmp(*newval, "") == 0)  return true;
> 

Changed.

> + *newval); return false; }
> 

Changed.

> ~~~
> 
> 9.
> + /* Now verify if the specified slots really exist and have correct
> + type */ if (!validate_standby_slots(newval)) return false;
> 
> As in a prior comment, if ReplicationSlotCtl is NULL then it is not always going
> to do exactly what that comment says it is doing...

I think the comment is OK, one can check the detail in the
function definition if needed.

> + if (!SplitIdentifierString(standby_slot_names_cpy, ',',
> + &standby_slots)) {
> + elog(ERROR, "invalid list syntax"); }
> 
> I didn't see how it is possible to get here without having first executed
> check_standby_slot_names. But, if it can happen, then maybe describe the
> scenario in the comment.

This is sanity check which we don't expect to happen, which follows similar style of preprocessNamespacePath.

> 
> ~~~
> 
> 11.
> + * Note that since we do not support syncing slots to cascading
> + standbys, we
> + * return NIL if we are running in a standby to indicate that no
> + standby slots
> I didn't understand the relevance of even mentioning "regardless of the copy
> flag value".

Removed.

> This errhint should use a format substitution for the GUC "standby_slot_names"
> for consistency with everything else.

Changed.

> + WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION);

Changed.

Added.

> + * Wake up the logical walsender processes with failover enabled slots
> +if the
> + if (strcmp(name, NameStr(MyReplicationSlot->data.name)) == 0)  {
> +ConditionVariableBroadcast(&WalSndCtl->wal_confirm_rcv_cv);
> + return;
> + }
> + }
> +}
> 
> 15a.
> There already exists another function called WalSndWakeup(bool physical,
> bool logical), so I think this new one should use a similar name pattern -- e.g.
> maybe like WalSndWakeupLogicalForSlotSync or ...

WalSndWakeup is a general function for both physical and logical sender, but
our new function is specific to physical sender which is more similar to
PhysicalConfirmReceivedLocation/ PhysicalReplicationSlotNewXmin, so I think the
current name is ok.

> 
> ~
> 
> 15b.
> IIRC there are some new List macros you can use instead of needing to declare
> the ListCell?

Changed.

> Moving the 'the' will make this more consistent with all other "Waiting for
> WAL..." names.
> 
> SUGGESTION

Changed.

> + "replication slots confirm receiving WAL."), GUC_LIST_INPUT |
> + GUC_LIST_QUOTE }, &standby_slot_names, "", check_standby_slot_names,
> + assign_standby_slot_names, NULL },
> 
> Decoded changes are sent out to plugins by logical WAL sender processes
> only after specified replication slots confirm receiving WAL.
> 
> SUGGESTION
> Logical WAL sender processes will send decoded changes to plugins only after
> the specified replication slots confirm receiving WAL.

Changed.

> +#standby_slot_names = '' # streaming replication standby server slot
> +names that  # logical walsender processes will wait for
> 
> I'm not sure this is the best GUC name. See the general comment #0 above in
> this post.

As discussed, I didn’t change this.

Changed.

> +# | ----> standby1 (primary_slot_name = sb1_slot) # | ----> standby2
> +(primary_slot_name = sb2_slot) # primary ----- | # | ----> subscriber1
> +(failover = true) # | ----> subscriber2 (failover = false)
> 
> In the diagram, the "--->" means a mixture of physical standbys and logical
> pub/sub replication. Maybe it can be a bit clearer?
> 
> SUGGESTION
> 
> # primary (publisher)
> #
> #     (physical standbys)

I think one can distinguish it based on the 'standby' and 'subscriber' as well, because
'standby' normally refer to physical standby while the other refer to logical. 

> ~~~
> 
> 21.
> +# Set up is configured in such a way that the logical slot of
> +subscriber1 is # enabled failover, thus it will wait for the physical
> +slot of # standby1(sb1_slot) to catch up before sending decoded changes to
> subscriber1.
> 

Changed.

> 
> ~~~
> 
> 22.
> +# Create another subscriber node without enabling failover, wait for
> +sync to # complete my $subscriber2 =
> +PostgreSQL::Test::Cluster->new('subscriber2');
> +$subscriber2->init;
> +$subscriber2->start;
> + CREATE TABLE tab_int (a int PRIMARY KEY);  CREATE SUBSCRIPTION
> +regress_mysub2 CONNECTION '$publisher_connstr'

Yes, fixed.

> 
> ~~~
> 
> 23.
> # Stop the standby associated with the specified physical replication slot so #
> that the logical replication slot won't receive changes until the standby #
> comes up.
> 
> Stop the standby associated with the specified physical replication slot
> (sb1_slot) so that the logical replication slot (lsub1_slot) won't receive changes
> until the standby comes up.

Changed.

> Wait until the standby2 that's still running gets the data from the primary.

Changed.

> Wait for subscription2 to get the data from the primary. This subscription was
> not enabled for failover so it gets the data without waiting for any standbys.
> 

Changed.

> ~~~
> 
> 26.
> +# The subscription that's up and running and is enabled for failover #
> +doesn't get the data from primary and keeps waiting for the # standby
> +specified in standby_slot_names.
> 
> SUGGESTION
> The subscription1 was enabled for failover so it doesn't get the data from
> primary and keeps waiting for the standby specified in standby_slot_names
> (sb1_slot aka standby1).
> 

Changed.

> ~~~
> 
> 27.
> +# Start the standby specified in standby_slot_names and wait for it to
> +catch # up with the primary.
> 
> SUGGESTION

Changed.

> ~~~
> 
> 28.
> +# Now that the standby specified in standby_slot_names is up and
> +running, # primary must send the decoded changes to subscription
> +enabled for failover # While the standby was down, this subscriber
> +didn't receive any data from # primary i.e. the primary didn't allow it to go
> ahead of standby.
> 
> SUGGESTION
> Now that the standby specified in standby_slot_names is up and running, the
> primary can send the decoded changes to the subscription enabled for failover
> (i.e. subscription1). While the standby was down,
> subscription1 didn't receive any data from the primary. i.e. the primary didn't
> allow it to go ahead of standby.
> 

Changed.

> ~~~
> 
> 29.
> +# Stop the standby associated with the specified physical replication
> +slot so # that the logical replication slot won't receive changes until
> +the standby # slot's restart_lsn is advanced or the slot is removed
> +from the # standby_slot_names list.
> Isn't this fragment more like the first step of the *next* TEST instead of the last
> step of this one?
> 

Changed.

> +# Verify that when using pg_logical_slot_get_changes to consume changes
> +from a # logical slot with failover enabled, it will also wait for the
> +slots specified # in standby_slot_names to catch up.
> AFAICT this test is checking only that the function cannot return while waiting
> for the stopped standby, but it doesn't seem to check that it *does* return
> when the stopped standby comes alive again.
> 

Will think about this.

> ~~~
> 
> 31.
> +$result =
> +  $subscriber1->safe_psql('postgres', "SELECT count(*) = 0 FROM
> +tab_int;"); is($result, 't',

Added.

Attach the V100 patch set which addressed above comments.

Best Regards,
Hou zj

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 28 Feb 2024 11:35:33 +0000

Hi,

> Attach the V100 patch set which addressed above comments.

Thanks!

A few random comments:

1 ===

+       if (!ok)
+       {

What about to get rid of the brackets here?

2 ===

+
+       /*

remove the empty line above the comment?

3 ===

+check_standby_slot_names(char **newval, void **extra, GucSource source)
+{
+       if ((*newval)[0] == '\0')
+               return true;

I think "**newval == '\0'" is easier to read but that's a matter of taste and
check_synchronous_standby_names() is already using the same so it's a nit.

4 ===

Regarding the test, what about adding one to test the "new" behavior discussed
up-thread? (logical replication will wait if slot mentioned in standby_slot_names
is dropped and/or does not exist when the engine starts?)

Regards,

---

From: Peter Smith <[email-masked]>
Date: Thu, 29 Feb 2024 13:59:19 +1100

> On Tuesday, February 27, 2024 3:18 PM Peter Smith <[email-masked]> wrote:
...
> 'standby' normally refer to physical standby while the other refer to logical.

Ok, but shouldn't it at least include info about the logical slot
names associated with the subscribers (slot_name = lsub1_slot,
slot_name = lsub2_slot) like suggested above?

======

Here are some more review comments for v100-0001

======
doc/src/sgml/config.sgml

1.
+       <para>
+        Lists the streaming replication standby server slot names that logical
+        WAL sender processes will wait for. Logical WAL sender processes will
+        send decoded changes to plugins only after the specified replication
+        slots confirm receiving WAL. This guarantees that logical replication
+        slots with failover enabled do not consume changes until those changes
+        are received and flushed to corresponding physical standbys. If a
+        logical replication connection is meant to switch to a physical standby
+        after the standby is promoted, the physical replication slot for the
+        standby should be listed here. Note that logical replication will not
+        proceed if the slots specified in the standby_slot_names do
not exist or
+        are invalidated.
+       </para>

Is that note ("Note that logical replication will not proceed if the
slots specified in the standby_slot_names do not exist or are
invalidated") meant only for subscriptions marked for 'failover' or
any subscription? Maybe wording can be modified to help clarify it?

======

2.
+/*
+ * A helper function to validate slots specified in GUC standby_slot_names.
+ */
+static bool
+validate_standby_slots(char **newval)
+{
+ char    *rawname;
+ List    *elemlist;
+ bool ok;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(*newval);
+
+ /* Verify syntax and parse string into a list of identifiers */
+ ok = SplitIdentifierString(rawname, ',', &elemlist);
+
+ if (!ok)
+ {
+ GUC_check_errdetail("List syntax is invalid.");
+ }
+
+ /*
+ * If the replication slots' data have been initialized, verify if the
+ * specified slots exist and are logical slots.
+ */
+ else if (ReplicationSlotCtl)
+ {
+ foreach_ptr(char, name, elemlist)
+ {
+ ReplicationSlot *slot;
+
+ slot = SearchNamedReplicationSlot(name, true);
+
+ if (!slot)
+ {
+ GUC_check_errdetail("replication slot \"%s\" does not exist",
+ name);
+ ok = false;
+ break;
+ }
+
+ if (!SlotIsPhysical(slot))
+ {
+ GUC_check_errdetail("\"%s\" is not a physical replication slot",
+ name);
+ ok = false;
+ break;
+ }
+ }
+ }
+
+ pfree(rawname);
+ list_free(elemlist);
+ return ok;
+}

2a.
I didn't mention this previously because I thought this function was
not going to change anymore, but since Bertrand suggested some changes
[1], I will say IMO the { } are fine here for the single statement,
but I think it will be better to rearrange this code to be like below.
Having a 2nd NOP 'else' gives a much better place where you can put
your ReplicationSlotCtl comment.

if (!ok)
{
  GUC_check_errdetail("List syntax is invalid.");
}
else if (!ReplicationSlotCtl)
{
  <Insert big comment here about why it is OK to skip when
ReplicationSlotCtl is NULL>
}
else
{
  foreach_ptr ...
}

~

2b.
In any case even if don't refactor anything I still think you need to
extend that comment to explain how skipping ReplicationSlotCtl is NULL
is only OK because there will be some later checking also done in the
FilterStandbySlots() function to catch any config problems.

------
[1] https://www.postgresql.org/message-id/Zd8ahZXw82ieFxX/%40ip-10-97-1-34.eu-west-3.compute.internal

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 29 Feb 2024 03:16:15 +0000

Thanks for the comments!

> What about to get rid of the brackets here?

I personally prefer the current style.

> remove the empty line above the comment?

I feel it would be clean to have an empty line before the comments.

> check_synchronous_standby_names() is already using the same so it's a nit.

I don't have a strong opinion on this, so will change if others also feel so.

> 
> 4 ===
> 
> Regarding the test, what about adding one to test the "new" behavior
> discussed up-thread? (logical replication will wait if slot mentioned in
> standby_slot_names is dropped and/or does not exist when the engine starts?)

Will think about this.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Thu, 29 Feb 2024 14:43:30 +1100

On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila <[email-masked]> wrote:
> name as this is used during SQL APIs as well.

Fair enough, but the fact that some SQL functions might wait is also
not mentioned in the config file comment, nor in the documentation for
GUC  'standby_slot_names'. Seems like a docs omission?

> Also, adding wait sounds

Anyway, the point is that the current GUC name 'standby_slot_names' is
not ideal IMO because it doesn't have enough meaning by itself -- e.g.
you have to read the accompanying comment or documentation to have any
idea of its purpose.

My suggested GUC names were mostly just to get people thinking about
it. Maybe others can come up with better names.

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 29 Feb 2024 15:23:30 +0530

> > 'standby' normally refer to physical standby while the other refer to logical.

I think Peter's suggestion will make the setup clear.

> any subscription? Maybe wording can be modified to help clarify it?

I think it is implicit that here we are talking about failover slots.
I think clarifying again the same could be repetitive considering the
previous sentence: "This guarantees that logical replication slots
with failover enabled do not consume .." have mentioned it.

> ReplicationSlotCtl is NULL>

+1. This will make the code and reasoning to skip clear.

Few additional comments on the latest patch:
=================================
1.
 static XLogRecPtr
 WalSndWaitForWal(XLogRecPtr loc)
{
...
+ if (!XLogRecPtrIsInvalid(RecentFlushPtr) && loc <= RecentFlushPtr &&
+ (!replication_active || StandbyConfirmedFlush(loc, WARNING)))
+ {
+ /*
+ * Fast path to avoid acquiring the spinlock in case we already know
+ * we have enough WAL available and all the standby servers have
+ * confirmed receipt of WAL up to RecentFlushPtr. This is particularly
+ * interesting if we're far behind.
+ */
  return RecentFlushPtr;
+ }
...
...
+ * Wait for WALs to be flushed to disk only if the postmaster has not
+ * asked us to stop.
+ */
+ if (loc > RecentFlushPtr && !got_STOPPING)
+ wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
+
+ /*
+ * Check if the standby slots have caught up to the flushed position.
+ * It is good to wait up to RecentFlushPtr and then let it send the
+ * changes to logical subscribers one by one which are already covered
+ * in RecentFlushPtr without needing to wait on every change for
+ * standby confirmation. Note that after receiving the shutdown signal,
+ * an ERROR is reported if any slots are dropped, invalidated, or
+ * inactive. This measure is taken to prevent the walsender from
+ * waiting indefinitely.
+ */
+ else if (replication_active &&
+ !StandbyConfirmedFlush(RecentFlushPtr, WARNING))
+ {
+ wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
+ wait_for_standby = true;
+ }
+ else
+ {
+ /* Already caught up and doesn't need to wait for standby_slots. */
  break;
+ }
...
}

Can we try to move these checks into a separate function that returns
a boolean and has an out parameter as wait_event?

2. How about naming StandbyConfirmedFlush() as StandbySlotsAreCaughtup?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 29 Feb 2024 15:38:59 +0530

> you have to read the accompanying comment or documentation to have any

Yeah, one has to read the description but that is true for other
parameters like "temp_tablespaces". I don't have any better ideas but
open to suggestions.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 29 Feb 2024 16:47:29 +0530

> Few additional comments on the latest patch:

Some more comments:
==================
1.

Won't the second check need protection via ReplicationSlotControlLock?

2. In WaitForStandbyConfirmation(), we are anyway calling
StandbyConfirmedFlush() before the actual sleep in the loop, so does
calling it at the beginning of the function will serve any purpose? If
so, it is better to add some comments explaining the same.

3. Also do we need to perform the validation checks done in
StandbyConfirmedFlush() repeatedly when it is invoked in a loop? We
can probably separate those checks and perform them just once.

4.
+   *
+   * XXX: If needed, this can be attempted in future.

Remove this part of the comment.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Mar 2024 02:17:26 +0000

On Thursday, February 29, 2024 7:17 PM Amit Kapila <[email-masked]> wrote:
> > + (!replication_active || StandbyConfirmedFlush(loc, WARNING))) {
> > + /*
> > + * confirmed receipt of WAL up to RecentFlushPtr. This is
> > + particularly
> > + if (loc > RecentFlushPtr && !got_STOPPING) wait_event =
> > + WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
> > +
> > + /*
> > + * changes to logical subscribers one by one which are already
> > + covered
> > + * standby confirmation. Note that after receiving the shutdown
> > + signal,
> > + !StandbyConfirmedFlush(RecentFlushPtr, WARNING)) { wait_event =
> > + WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
> + GUC_check_errdetail("replication slot \"%s\" does not exist", name);
> + ok = false; break; }
> +
> + name); ok = false; break; } }
> 

Yes, added.

> StandbyConfirmedFlush() before the actual sleep in the loop, so does calling it
> at the beginning of the function will serve any purpose? If so, it is better to add
> some comments explaining the same.

It is used as a fast-path to avoid calling condition variable stuff, I think we can directly
put failover check and list check in the beginning instead of calling that function.

> StandbyConfirmedFlush() repeatedly when it is invoked in a loop? We can
> probably separate those checks and perform them just once.

I have removed slot.failover check from the StandbyConfirmedFlush
function, so that we can do it when necessary. I didn’t change the check
for standby_slot_names_list because the list could be changed in the loop
when reloading config.

Removed.

Attach the V102 patch set which addressed Amit and Shveta's comments.
Thanks Shveta for helping addressing the comments off-list.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Mar 2024 02:18:53 +0000

On Thursday, February 29, 2024 5:54 PM Amit Kapila <[email-masked]> wrote:
> logical

Changed.

Changed.

> Few additional comments on the latest patch:

Refactored.

> 2. How about naming StandbyConfirmedFlush() as StandbySlotsAreCaughtup?

I used a similar version based on the suggested name: StandbySlotsHaveCaughtup.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Mar 2024 03:42:23 +0000

> Thanks Shveta for helping addressing the comments off-list.

The cfbot reported a compile warning, here is the new version patch which fixed it,
Also removed some outdate comments in this version.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Fri, 1 Mar 2024 15:22:55 +1100

Here are some review comments for v102-0001.

======
doc/src/sgml/config.sgml

1.
+       <para>

Should this also mention the effect this GUC has on those 2 SQL
functions? E.g. Commit message says:

Additionally, The SQL functions pg_logical_slot_get_changes and
pg_replication_slot_advance are modified to wait for the replication
slots mentioned in 'standby_slot_names' to catch up before returning.

======

2. validate_standby_slots

+ else if (!ReplicationSlotCtl)
+ {
+ /*
+ * We cannot validate the replication slot if the replication slots'
+ * data has not been initialized. This is ok as we will validate the
+ * specified slot when waiting for them to catch up. See
+ * StandbySlotsHaveCaughtup for details.
+ */
+ }
+ else
+ {
+ /*
+ * If the replication slots' data have been initialized, verify if the
+ * specified slots exist and are logical slots.
+ */
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);

IMO that 2nd comment does not need to say "If the replication slots'
data have been initialized," because that is implicit from the
if/else.

~~~

3. GetStandbySlotList

+List *
+GetStandbySlotList(void)
+{
+ if (RecoveryInProgress())
+ return NIL;
+ else
+ return standby_slot_names_list;
+}

The 'else' is not needed. IMO is better without it, but YMMV.

~~~

4. StandbySlotsHaveCaughtup

+/*
+ * Return true if the slots specified in standby_slot_names have caught up to
+ * the given WAL location, false otherwise.
+ *
+ * The elevel parameter determines the error level used for logging messages
+ * related to slots that do not exist, are invalidated, or are inactive.
+ */
+bool
+StandbySlotsHaveCaughtup(XLogRecPtr wait_for_lsn, int elevel)

/determines/specifies/

~

5.
+ /*
+ * Don't need to wait for the standby to catch up if there is no value in
+ * standby_slot_names.
+ */
+ if (!standby_slot_names_list)
+ return true;
+
+ /*
+ * If we are on a standby server, we do not need to wait for the standby to
+ * catch up since we do not support syncing slots to cascading standbys.
+ */
+ if (RecoveryInProgress())
+ return true;
+
+ /*
+ * Return true if all the standbys have already caught up to the passed in
+ * WAL localtion.
+ */
+ if (!XLogRecPtrIsInvalid(standby_slot_oldest_flush_lsn) &&
+ standby_slot_oldest_flush_lsn >= wait_for_lsn)
+ return true;

5a.
I felt all these comments should be worded in a consistent way like
"Don't need to wait ..."

e.g.
1. Don't need to wait for the standbys to catch up if there is no
value in 'standby_slot_names'.
2. Don't need to wait for the standbys to catch up if we are on a
standby server, since we do not support syncing slots to cascading
standbys.
3. Don't need to wait for the standbys to catch up if they are already
beyond the specified WAL location.

~

5b.
typo
/WAL localtion/WAL location/

~~~

6.
+ if (!slot)
+ {
+ /*
+ * It may happen that the slot specified in standby_slot_names GUC
+ * value is dropped, so let's skip over it.
+ */
+ ereport(elevel,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("replication slot \"%s\" specified in parameter %s does not exist",
+    name, "standby_slot_names"));
+ continue;
+ }

Is "is dropped" strictly the only reason? IIUC another reason is the
slot maybe just did not even exist in the first place but it was not
detected before now because inititial validation was skipped.

~~~

7.
+ /*
+ * Return false if not all the standbys have caught up to the specified WAL
+ * location.
+ */
+ if (caught_up_slot_num != list_length(standby_slot_names_list))
+ return false;

Somehow it seems complicated to have a counter of the slots as you
process then compare that counter to the list_length to determine if
one of them was skipped.

Probably simpler just to set a 'skipped' flag set whenever you do 'continue'...

======

8.
+/*
+ * Returns true if there are not enough WALs to be read, or if not all standbys
+ * have caught up to the flushed position when failover_slot is true;
+ * otherwise, returns false.
+ *
+ * Set prioritize_stop to true to skip waiting for WALs if the shutdown signal
+ * is received.
+ *
+ * Set failover_slot to true if the current acquired slot is a failover enabled
+ * slot and we are streaming.
+ *
+ * If returning true, the function sets the appropriate wait event in
+ * wait_event; otherwise, wait_event is set to 0.
+ */
+static bool
+NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
+ bool prioritize_stop, bool failover_slot,
+ uint32 *wait_event)

8a.
/Set prioritize_stop to true/Specify prioritize_stop=true/

/Set failover_slot to true/Specify failover_slot=true/

~

8b.
Aren't the static function names typically snake_case?

~~~

9.
+ /*
+ * Check if we need to wait for WALs to be flushed to disk. We don't need
+ * to wait for WALs after receiving the shutdown signal unless the
+ * wait_for_wal_on_stop is true.
+ */
+ if (target_lsn > flushed_lsn && !(prioritize_stop && got_STOPPING))
+ *wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;

The comment says 'wait_for_wal_on_stop' but the code says 'prioritize_stop' (??)

~~~

10.
+ /*
+ * Check if we need to wait for WALs to be flushed to disk. We don't need
+ * to wait for WALs after receiving the shutdown signal unless the
+ * wait_for_wal_on_stop is true.
+ */
+ if (target_lsn > flushed_lsn && !(prioritize_stop && got_STOPPING))
+ *wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
+
+ /*
+ * Check if the standby slots have caught up to the flushed position. It is
+ * good to wait up to RecentFlushPtr and then let it send the changes to
+ * logical subscribers one by one which are already covered in
+ * RecentFlushPtr without needing to wait on every change for standby
+ * confirmation. Note that after receiving the shutdown signal, an ERROR is
+ * reported if any slots are dropped, invalidated, or inactive. This
+ * measure is taken to prevent the walsender from waiting indefinitely.
+ */
+ else if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn, elevel))
+ *wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
+ else
+ return false;
+
+ return true;

This if/else/else seems overly difficult to read. IMO it will be
easier if written like:

SUGGESTION
<comment>
if (target_lsn > flushed_lsn && !(prioritize_stop && got_STOPPING))
{
  *wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
  return true;
}

<comment>
if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn, elevel))
{
  *wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
  return true;
}

return false;

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

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 1 Mar 2024 15:10:57 +0900

> Also removed some outdate comments in this version.

Thank you for updating the patch!

I've reviewed the v102-0001 patch. Here are some comments:

---
I got a compiler warning:

walsender.c:1829:6: warning: variable 'wait_event' is used
uninitialized whenever '&&' condition is false
[-Wsometimes-uninitialized]
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1871:7: note: uninitialized use occurs here
                if (wait_event == WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL)
                    ^~~~~~~~~~
walsender.c:1829:6: note: remove the '&&' if its condition is always true
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1818:20: note: initialize the variable 'wait_event' to
silence this warning
        uint32          wait_event;
                                  ^
                                   = 0
1 warning generated.

---
+void
+assign_standby_slot_names(const char *newval, void *extra)
+{
+        List      *standby_slots;
+        MemoryContext oldcxt;
+        char      *standby_slot_names_cpy = extra;
+

Given that the newval and extra have the same data (standby_slot_names
value), why do we not use newval instead? I think that if we use
newval, we don't need to guc_strdup() in check_standby_slot_names(),
we might need to do list_copy_deep() instead, though. It's not clear
to me as there is no comment.

---
+
+        standby_slot_oldest_flush_lsn = min_restart_lsn;
+

IIUC we expect that standby_slot_oldest_flush_lsn never moves
backward. If so, I think it's better to have an assertion here.

---
Resetting standby_slot_names doesn't work:

=# alter system set standby_slot_names to '';
ERROR:  invalid value for parameter "standby_slot_names": """"
DETAIL:  replication slot "" does not exist

---
+        /*
+         * Switch to the memory context under which GUC variables are allocated
+         * (GUCMemoryContext).
+         */
+        oldcxt =
MemoryContextSwitchTo(GetMemoryChunkContext(standby_slot_names_cpy));
+        standby_slot_names_list = list_copy(standby_slots);
+        MemoryContextSwitchTo(oldcxt);

Why do we not explicitly switch to GUCMemoryContext?

---

Probably "standby_slot_names_list == NIL" is more consistent with
other places. The same can be applied in WaitForStandbyConfirmation().

---
+        /*
+         * Return true if all the standbys have already caught up to
the passed in
+         * WAL localtion.
+         */
+

s/localtion/location/

---
I was a bit surprised by the fact that standby_slot_names value is
handled in a different way than a similar parameter
synchronous_standby_names. For example, the following command doesn't
work unless there is a replication slot 'slot1, slot2':

=# alter system set standby_slot_names to 'slot1, slot2';
ERROR:  invalid value for parameter "standby_slot_names": ""slot1, slot2""
DETAIL:  replication slot "slot1, slot2" does not exist

Whereas "alter system set synchronous_standby_names to stb1, stb2;"
can correctly separate the string into 'stb1' and 'stb2'.

Probably it would be okay since this behavior of standby_slot_names is
the same as other GUC parameters that accept a comma-separated string.
But I was confused a bit the first time I used it.

---
+        /*
+         * "*" is not accepted as in that case primary will not be able to know
+         * for which all standbys to wait for. Even if we have physical slots
+         * info, there is no way to confirm whether there is any standby
+         * configured for the known physical slots.
+         */
+        if (strcmp(*newval, "*") == 0)
+        {
+                GUC_check_errdetail("\"*\" is not accepted for
standby_slot_names");
+                return false;
+        }

Why only '*' is checked aside from validate_standby_slots()? I think
that the doc doesn't mention anything about '*' and '*' cannot be used
as a replication slot name. So even if we don't have this check, it
might be no problem.

Regards,

-- 
Masahiko Sawada

---

From: Peter Smith <[email-masked]>
Date: Fri, 1 Mar 2024 17:21:46 +1100

On Fri, Mar 1, 2024 at 5:11 PM Masahiko Sawada <[email-masked]> wrote:
...
> as a replication slot name. So even if we don't have this check, it

Hi, a while ago I asked this same question. See [1 #28] for the response..

----------
[1] https://www.postgresql.org/message-id/OS0PR01MB571646B8186F6A06404BD50194BDA%40OS0PR01MB5716.jpnprd01.prod.outlook.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Fri, 1 Mar 2024 12:15:12 +0530

> Probably simpler just to set a 'skipped' flag set whenever you do 'continue'...

The other thing is do we need to continue when we find some slot that
can't be processed? If not, then we can simply set the boolean flag,
break the loop, and return false if the boolean is set after releasing
the LWLock. The other way is we simply release lock whenever we need
to skip the slot and just return false.

-- 
With Regards,
Amit Kapila.

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 1 Mar 2024 07:15:45 +0000

Hi,

> slots mentioned in 'standby_slot_names' to catch up before returning.

I think that's also true for all the ones that rely on
pg_logical_slot_get_changes_guts(), means:

- pg_logical_slot_get_changes
- pg_logical_slot_peek_changes
- pg_logical_slot_get_binary_changes
- pg_logical_slot_peek_binary_changes

Not sure it's worth to mention the "binary" ones though as their doc mention
they behave as their "non binary" counterpart.

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Mar 2024 07:21:18 +0000

On Friday, March 1, 2024 2:11 PM Masahiko Sawada <[email-masked]> wrote:
> 
> On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> > The cfbot reported a compile warning, here is the new version patch
> > which fixed it, Also removed some outdate comments in this version.
> >
> 

Thanks for the comments !

> walsender.c:1829:6: warning: variable 'wait_event' is used uninitialized
> whenever '&&' condition is false [-Wsometimes-uninitialized]
> walsender.c:1818:20: note: initialize the variable 'wait_event' to silence this
> warning
>         uint32          wait_event;
>                                   ^
>                                    = 0
> 1 warning generated.

Thanks for reporting, it was fixed in V102_2.

> 
> ---
> +void
> +assign_standby_slot_names(const char *newval, void *extra) {
> +        List      *standby_slots;
> +        MemoryContext oldcxt;

I think SplitIdentifierString will modify the passed in string, so we'd better
not pass the newval to it, otherwise the stored guc string(standby_slot_names)
will be changed. I can see we are doing similar thing in other GUC check/assign
function as well. (check_wal_consistency_checking/
assign_wal_consistency_checking, check_createrole_self_grant/
assign_createrole_self_grant ...).

> Why do we not explicitly switch to GUCMemoryContext?

I think it's because the GUCMemoryContext is not exposed.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 1 Mar 2024 14:51:42 +0530

> But I was confused a bit the first time I used it.

I think it is better to keep the behavior in this regard the same as
'synchronous_standby_names' because both have similarities w.r.t
replication.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 2 Mar 2024 03:51:18 +0000

On Friday, March 1, 2024 12:23 PM Peter Smith <[email-masked]> wrote:
> Should this also mention the effect this GUC has on those 2 SQL functions? E.g.
> Commit message says:
> 
> mentioned in 'standby_slot_names' to catch up before returning.

Added.

> data have been initialized," because that is implicit from the if/else.

Removed.

Removed.

> + * Return true if the slots specified in standby_slot_names have caught
> +up to
> + * The elevel parameter determines the error level used for logging
> +messages

Changed.

> 
> ~
> 
> 5.
> + /*
> + * Don't need to wait for the standby to catch up if there is no value
> + in
> + * standby_slot_names.
> + */
> + * If we are on a standby server, we do not need to wait for the
> + standby to
> + * Return true if all the standbys have already caught up to the passed
> + in
> + * WAL localtion.
> + */
> + standby_slot_oldest_flush_lsn >= wait_for_lsn) return true;
> 
> 
> 5a.
> I felt all these comments should be worded in a consistent way like "Don't need
> to wait ..."
> 
> e.g.
> 1. Don't need to wait for the standbys to catch up if there is no value in
> 'standby_slot_names'.
> 2. Don't need to wait for the standbys to catch up if we are on a standby server,
> since we do not support syncing slots to cascading standbys.
> 3. Don't need to wait for the standbys to catch up if they are already beyond
> the specified WAL location.

Changed.

> /WAL localtion/WAL location/

Fixed.

> Is "is dropped" strictly the only reason? IIUC another reason is the slot maybe
> just did not even exist in the first place but it was not detected before now
> because inititial validation was skipped.

Changed the comment.

> 
> ~~~
> 
> 7.
> + /*
> + * Return false if not all the standbys have caught up to the specified
> + WAL
> + * location.
> + */
> Somehow it seems complicated to have a counter of the slots as you process
> then compare that counter to the list_length to determine if one of them was
> skipped.
> 

I prefer the current style because we need to set skipped =true in
multiple places which doesn't seem better to me.

> + * Returns true if there are not enough WALs to be read, or if not all
> +standbys
> + * Set prioritize_stop to true to skip waiting for WALs if the shutdown
> +signal
> + * is received.
> + *
> + * Set failover_slot to true if the current acquired slot is a failover
> +enabled
> +NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,  bool
> +prioritize_stop, bool failover_slot,
> + uint32 *wait_event)
> 
> 8a.

This function has been refactored a bit.

> Aren't the static function names typically snake_case?

I think the current name style is more consistent with the other functions in walsender.c.

> 
> ~~~
> 
> 9.
> + /*
> + * Check if we need to wait for WALs to be flushed to disk. We don't
> + need

This has been removed in last version.

> ~~~
> 
> 10.
> + /*
> + * Check if we need to wait for WALs to be flushed to disk. We don't
> + need
> + It is
> + * good to wait up to RecentFlushPtr and then let it send the changes
> + to
> + * confirmation. Note that after receiving the shutdown signal, an
> + ERROR is
> + else if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn,
> + elevel)) *wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
> + else
> + return false;
> +
> + return true;
> 
> This if/else/else seems overly difficult to read. IMO it will be easier if written
> like:
> 
> SUGGESTION
> <comment>
> if (target_lsn > flushed_lsn && !(prioritize_stop && got_STOPPING)) {
> if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn, elevel)) {

Changed.

Attach the V103 patch set which addressed above comments and
Sawada-san's comment[1].

Apart from the comments, the code in WalSndWaitForWal was refactored
a bit to make it neater. Thanks Shveta for helping writing the code and doc.

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

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sat, 2 Mar 2024 03:51:51 +0000

On Thursday, February 29, 2024 11:16 AM Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> > discussed up-thread? (logical replication will wait if slot mentioned
> > in standby_slot_names is dropped and/or does not exist when the engine
> > starts?)
> 
> Will think about this.

I think we could add a test to check the warning message for dropped slot, but
since similar wait/warning functionality has been tested, I prefer to leave
this for now and can consider it again after the main patch and tests are
stabilized considering the previous experience of BF instability with this
feature.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Sat, 2 Mar 2024 16:24:48 +0530

> a bit to make it neater. Thanks Shveta for helping writing the code and doc.

A few more comments:
==================
1.
+# Wait until the primary server logs a warning indicating that it is waiting
+# for the sb1_slot to catch up.
+$primary->wait_for_log(
+ qr/replication slot \"sb1_slot\" specified in parameter
standby_slot_names does not have active_pid/,
+ $offset);

Shouldn't we wait for such a LOG even in the first test as well which
involves two standbys and two logical subscribers?

2.
+##################################################
+# Test that logical replication will wait for the user-created inactive
+# physical slot to catch up until we remove the slot from standby_slot_names.
+##################################################

I don't see anything different tested in this test from what we
already tested in the first test involving two standbys and two
logical subscribers. Can you please clarify if I am missing something?

3.
Note that after receiving the shutdown signal, an ERROR
+ * is reported if any slots are dropped, invalidated, or inactive. This
+ * measure is taken to prevent the walsender from waiting indefinitely.
+ */
+ if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))

Isn't this part of the comment should be moved inside NeedToWaitForStandby()?

4.
+ /*
+ * Update our idea of the currently flushed position only if we are
+ * not waiting for standbys to catch up, otherwise the standby would
+ * have to catch up to a newer WAL location in each cycle.
+ */
+ if (wait_event != WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION)
+ {

This functionality (in function WalSndWaitForWal()) seems to ensure
that we first wait for the required WAL to be flushed and then wait
for standbys. If true, we should cover that point in the comments here
or somewhere in the function WalSndWaitForWal().

Apart from this, I have made a few modifications in the comments.

-- 
With Regards,
Amit Kapila.
diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl
index 5e4dd6c0ab..16483e52a9 100644
--- a/src/test/recovery/t/040_standby_failover_slots_sync.pl
+++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl
@@ -505,8 +505,9 @@ ok( $standby1->poll_query_until(
 	'restart_lsn and confirmed_flush_lsn of slot lsub1_slot synced to standby');
 
 ##################################################
-# Test primary disallowing specified logical replication slots getting ahead of
-# specified physical replication slots. It uses the following set up:
+# Test that logical failover replication slots wait for the specified
+# physical replication slots to  receive the changes first. It uses the
+# following set up:
 #
 #				(physical standbys)
 #				| ----> standby1 (primary_slot_name = sb1_slot)
@@ -518,9 +519,9 @@ ok( $standby1->poll_query_until(
 #
 # standby_slot_names = 'sb1_slot'
 #
-# Set up is configured in such a way that the logical slot of subscriber1 is
-# enabled for failover, thus it will wait for the physical slot of
-# standby1(sb1_slot) to catch up before sending decoded changes to subscriber1.
+# The setup is configured in such a way that the logical slot of subscriber1 is
+# enabled for failover, and thus the subscriber1 will wait for the physical
+# slot of standby1(sb1_slot) to catch up before receiving the decoded changes.
 ##################################################
 
 $backup_name = 'backup3';
@@ -543,8 +544,8 @@ primary_slot_name = 'sb2_slot'
 $standby2->start;
 $primary->wait_for_replay_catchup($standby2);
 
-# Configure primary to disallow any logical slots that enabled failover from
-# getting ahead of specified physical replication slot (sb1_slot).
+# Configure primary to disallow any logical slots that have enabled failover
+# from getting ahead of the specified physical replication slot (sb1_slot).
 $primary->append_conf(
 	'postgresql.conf', qq(
 standby_slot_names = 'sb1_slot'

---

From: Peter Smith <[email-masked]>
Date: Sun, 3 Mar 2024 10:46:59 +1100

...
> > data have been initialized," because that is implicit from the if/else.

I only meant to suggest removing the 1st part, not the entire comment.
I thought it is still useful to have a comment like:

/* Check that the specified slots exist and are logical slots.*/

======

And, here are some review comments for v103-0001.

======
Commit message

1.
Additionally, The SQL functions pg_logical_slot_get_changes,
pg_logical_slot_peek_changes and pg_replication_slot_advance are modified
to wait for the replication slots mentioned in 'standby_slot_names' to
catch up before returning.

~

(use the same wording as previous in this message)

/mentioned in/specified in/

======
doc/src/sgml/config.sgml

2.
+        Additionally, when using the replication management functions
+        <link linkend="pg-replication-slot-advance">
+        <function>pg_replication_slot_advance</function></link>,
+        <link linkend="pg-logical-slot-get-changes">
+        <function>pg_logical_slot_get_changes</function></link>, and
+        <link linkend="pg-logical-slot-peek-changes">
+        <function>pg_logical_slot_peek_changes</function></link>,
+        with failover enabled logical slots, the functions will wait for the
+        physical slots specified in <varname>standby_slot_names</varname> to
+        confirm WAL receipt before proceeding.
+       </para>

It says "the ... functions" twice so maybe reword it slightly.

BEFORE
Additionally, when using the replication management functions
pg_replication_slot_advance, pg_logical_slot_get_changes, and
pg_logical_slot_peek_changes, with failover enabled logical slots, the
functions will wait for the physical slots specified in
standby_slot_names to confirm WAL receipt before proceeding.

SUGGESTION
Additionally, the replication management functions
pg_replication_slot_advance, pg_logical_slot_get_changes, and
pg_logical_slot_peek_changes, when used with failover enabled logical
slots, will wait for the physical slots specified in
standby_slot_names to confirm WAL receipt before proceeding.

(Actually the "will wait ... before proceeding" is also a bit tricky,
so below is another possible rewording)

SUGGESTION #2
Additionally, the replication management functions
pg_replication_slot_advance, pg_logical_slot_get_changes, and
pg_logical_slot_peek_changes, when used with failover enabled logical
slots, will block until all physical slots specified in
standby_slot_names have confirmed WAL receipt.

~~~

3.
+       <note>
+        <para>
+         Value <literal>*</literal> is not accepted as it is inappropriate to
+         block logical replication for physical slots that either lack
+         associated standbys or have standbys associated that are not enabled
+         for replication slot synchronization. (see
+         <xref linkend="logicaldecoding-replication-slots-synchronization"/>).
+        </para>
+       </note>

Why does the document need to provide an excuse/reason for the rule?
You could just say something like:

SUGGESTION
The slots must be named explicitly. For example, specifying wildcard
values like <literal>*</literal> is not permitted.

======
doc/src/sgml/func.sgml

4.
@@ -28150,7 +28150,7 @@ postgres=# SELECT '0/0'::pg_lsn +
pd.segment_number * ps.setting::int + :offset
       </row>

       <row>
-       <entry role="func_table_entry"><para role="func_signature">
+       <entry id="pg-logical-slot-get-changes"
role="func_table_entry"><para role="func_signature">
         <indexterm>
          <primary>pg_logical_slot_get_changes</primary>
         </indexterm>
@@ -28177,7 +28177,7 @@ postgres=# SELECT '0/0'::pg_lsn +
pd.segment_number * ps.setting::int + :offset
       </row>

       <row>
-       <entry role="func_table_entry"><para role="func_signature">
+       <entry id="pg-logical-slot-peek-changes"
role="func_table_entry"><para role="func_signature">
         <indexterm>
          <primary>pg_logical_slot_peek_changes</primary>
         </indexterm>
@@ -28232,7 +28232,7 @@ postgres=# SELECT '0/0'::pg_lsn +
pd.segment_number * ps.setting::int + :offset
       </row>

       <row>
-       <entry role="func_table_entry"><para role="func_signature">
+       <entry id="pg-replication-slot-advance"
role="func_table_entry"><para role="func_signature">
         <indexterm>
          <primary>pg_replication_slot_advance</primary>
         </indexterm>

Should these 3 functions say something about how their behaviour is
affected by 'standby_slot_names' and give a link back to the GUC
'standby_slot_names' docs?

======

5. StandbySlotsHaveCaughtup

+ if (!slot)
+ {
+ /*
+ * If the provided slot does not exist, report a message and exit
+ * the loop. It is possible for a user to specify a slot name in
+ * standby_slot_names that does not exist just before the server
+ * startup. The GUC check_hook(validate_standby_slots) cannot
+ * validate such a slot during startup as the ReplicationSlotCtl
+ * shared memory is not initialized at that time. It is also
+ * possible for a user to drop the slot in standby_slot_names
+ * afterwards.
+ */

5a.
Minor rewording to make this code comment more similar to the next one:

SUGGESTION
If a slot name provided in standby_slot_names does not exist, report a
message and exit the loop. A user can specify a slot name that does
not exist just before the server startup...

~

5b.
+ /*
+ * If a logical slot name is provided in standby_slot_names,
+ * report a message and exit the loop. Similar to the non-existent
+ * case, it is possible for a user to specify a logical slot name
+ * in standby_slot_names before the server startup, or drop an
+ * existing physical slot and recreate a logical slot with the
+ * same name.
+ */

/it is possible for a user to specify/a user can specify/

~~~

6. WaitForStandbyConfirmation

+ /*
+ * We wait for the slots in the standby_slot_names to catch up, but we
+ * use a timeout (1s) so we can also check if the standby_slot_names
+ * has been changed.
+ */

Remove some of the "we".

SUGGESTION
Wait for the slots in the standby_slot_names to catch up, but use a
timeout (1s) so we can also check if the standby_slot_names has been
changed.

======

7. NeedToWaitForStandby

+/*
+ * Returns true if not all standbys have caught up to the flushed position
+ * (flushed_lsn) when failover_slot is true; otherwise, returns false.
+ *
+ * If returning true, the function sets the appropriate wait event in
+ * wait_event; otherwise, wait_event is set to 0.
+ */

The function comment refers to 'failover_slot' but IMO needs to be
worded differently because 'failover_slot' is not a parameter anymore.

~~~

8. NeedToWaitForWal

+/*
+ * Returns true if we need to wait for WALs to be flushed to disk, or if not
+ * all standbys have caught up to the flushed position (flushed_lsn) when
+ * failover_slot is true; otherwise, returns false.
+ *
+ * If returning true, the function sets the appropriate wait event in
+ * wait_event; otherwise, wait_event is set to 0.
+ */
+static bool
+NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
+ uint32 *wait_event)

Same as above -- Now that 'failover_slot' is not a function parameter,
I thought this should be reworded.

~~~

9. NeedToWaitForWal

+ /*
+ * Check if the standby slots have caught up to the flushed position. It
+ * is good to wait up to flushed position and then let it send the changes
+ * to logical subscribers one by one which are already covered in flushed
+ * position without needing to wait on every change for standby
+ * confirmation. Note that after receiving the shutdown signal, an ERROR
+ * is reported if any slots are dropped, invalidated, or inactive. This
+ * measure is taken to prevent the walsender from waiting indefinitely.
+ */
+ if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))
+ return true;

I felt it was confusing things for this function to also call to the
other one -- it seems an overlapping/muddling of the purpose of these.
I think it will be easier to understand if the calling code just calls
to one or both of these functions as required.

~~~

10.

- WalSndWait(wakeEvents, sleeptime, WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL);
+ WalSndWait(wakeEvents, sleeptime, wait_event);

Tracing the assignments of the 'wait_event' is a bit tricky... IIUC it
should not be 0 when we got to this point, so maybe it is better to
put Assert(wait_event) before this call?

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

---

From: Amit Kapila <[email-masked]>
Date: Sun, 3 Mar 2024 09:26:02 +0530

> values like <literal>*</literal> is not permitted.

I would like to document the reason somewhere, if not in docs, then
let's write a comment for the same in code.

> to one or both of these functions as required.

I felt otherwise because the caller has to call these functions at
more than one place which makes the caller's code difficult to follow.
It is better to encapsulate the computation of wait_event.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sun, 3 Mar 2024 07:51:38 +0000

On Sunday, March 3, 2024 7:47 AM Peter Smith <[email-masked]> wrote:
> 
> On Sat, Mar 2, 2024 at 2:51 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> > > + * data has not been initialized. This is ok as we will validate
> > > + the
> > > + * If the replication slots' data have been initialized, verify if
> > > + the

OK, I misunderstood. Fixed.

> And, here are some review comments for v103-0001.

Thanks for the comments.

> to wait for the replication slots mentioned in 'standby_slot_names' to catch up
> before returning.
> 
> ~
> 

Changed.

> pg_logical_slot_peek_changes, with failover enabled logical slots, the functions
> will wait for the physical slots specified in standby_slot_names to confirm WAL
> receipt before proceeding.
> 
> SUGGESTION
> pg_logical_slot_peek_changes, when used with failover enabled logical slots,
> will wait for the physical slots specified in standby_slot_names to confirm WAL
> receipt before proceeding.
> 
> (Actually the "will wait ... before proceeding" is also a bit tricky, so below is
> another possible rewording)
> 
> SUGGESTION #2
> pg_logical_slot_peek_changes, when used with failover enabled logical slots,
> will block until all physical slots specified in standby_slot_names have
> confirmed WAL receipt.

I prefer the #2 version.

> +         <xref
> linkend="logicaldecoding-replication-slots-synchronization"/>).
> +        </para>
> +       </note>
> 
> The slots must be named explicitly. For example, specifying wildcard values like
> <literal>*</literal> is not permitted.

As suggested by Amit, I moved this to code comments.

> Should these 3 functions say something about how their behaviour is affected
> by 'standby_slot_names' and give a link back to the GUC 'standby_slot_names'
> docs?

I added the info for pg_logical_slot_get_changes() and
pg_replication_slot_advance(). The pg_logical_slot_peek_changes() function
clarifies that it behaves just like pg_logical_slot_get_changes(), so I didn’t
touch it.

> If a slot name provided in standby_slot_names does not exist, report a message
> and exit the loop. A user can specify a slot name that does not exist just before
> the server startup...

Changed.

Changed.

> Wait for the slots in the standby_slot_names to catch up, but use a timeout (1s)
> so we can also check if the standby_slot_names has been changed.

Changed.

> + * Returns true if not all standbys have caught up to the flushed
> +position
> The function comment refers to 'failover_slot' but IMO needs to be worded
> differently because 'failover_slot' is not a parameter anymore.

Changed.

> 
> ~~~
> 
> 8. NeedToWaitForWal
> 
> +/*
> + * Returns true if we need to wait for WALs to be flushed to disk, or
> +if not
> + * all standbys have caught up to the flushed position (flushed_lsn)
> +when
> Same as above -- Now that 'failover_slot' is not a function parameter, I thought
> this should be reworded.

Changed.

> + It
> + * is good to wait up to flushed position and then let it send the
> + changes
> + * to logical subscribers one by one which are already covered in
> + flushed
> + ERROR
> + if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event)) return
> + true;
> 
> I felt it was confusing things for this function to also call to the other one -- it
> seems an overlapping/muddling of the purpose of these.
> I think it will be easier to understand if the calling code just calls to one or both
> of these functions as required.

Same as Amit, I didn't change this.

> Tracing the assignments of the 'wait_event' is a bit tricky... IIUC it should not be
> 0 when we got to this point, so maybe it is better to put Assert(wait_event)
> before this call?

Added.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Sun, 3 Mar 2024 07:56:32 +0000

On Saturday, March 2, 2024 6:55 PM Amit Kapila <[email-masked]> wrote:
> 
> On Sat, Mar 2, 2024 at 9:21 AM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> > Apart from the comments, the code in WalSndWaitForWal was refactored a
> > bit to make it neater. Thanks Shveta for helping writing the code and doc.
> >
> 
> A few more comments:

Thanks for the comments.

> ==================
> 1.
> +# Wait until the primary server logs a warning indicating that it is
> +waiting # for the sb1_slot to catch up.
> +$primary->wait_for_log(
> Shouldn't we wait for such a LOG even in the first test as well which involves two
> standbys and two logical subscribers?

Yes, we should. Added.

> I don't see anything different tested in this test from what we already tested in
> the first test involving two standbys and two logical subscribers. Can you
> please clarify if I am missing something?

I think the intention was to test that the wait loop is ended due to GUC config
reload, while the first test is for the case when the loop is ended due to
restart_lsn movement. But it seems we tested the config reload with xx_get_changes() as
well, so I can remove it if you agree.

> Isn't this part of the comment should be moved inside

Moved.

> This functionality (in function WalSndWaitForWal()) seems to ensure that we
> first wait for the required WAL to be flushed and then wait for standbys. If true,
> we should cover that point in the comments here or somewhere in the function
> WalSndWaitForWal().
> 

Thanks. I have reviewed and merged them.

Here is the V104 patch which addressed above and Peter's comments.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Mon, 4 Mar 2024 12:27:29 +1100

...
> It is better to encapsulate the computation of wait_event.

You may have misinterpreted my review comment because I didn't say
anything about changing the encapsulation of the computation of the
wait_event.

I only wrote it is better IMO for the functions to stick to just one
job each according to their function name. E.g.:
- NeedToWaitForStandby should *only* check if we need to wait for standbys
- NeedToWaitForWal should *only* check if we need to wait for WAL
flush; i.e. this shouldn't be also calling NeedToWaitForStandby().

Also, AFAICT the caller changes should not be difficult. Indeed, these
changes will make the code aligned properly with what the comments are
saying:

BEFORE
/*
 * Fast path to avoid acquiring the spinlock in case we already know we
 * have enough WAL available and all the standby servers have confirmed
 * receipt of WAL up to RecentFlushPtr. This is particularly interesting
 * if we're far behind.
 */
if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event))
  return RecentFlushPtr;

SUGGESTED
...
if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event) &&
  !NeedToWaitForStandby(loc, RecentFlushPtr, &wait_event)
  return RecentFlushPtr;

~~~

BEFORE
/*
 * Exit the loop if already caught up and doesn't need to wait for
 * standby slots.
 */
if (!wait_for_standby_at_stop &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event))
  break;

SUGGESTED
...
if (!wait_for_standby_at_stop &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event) &&
  !NeedToWaitForStandby(loc, RecentFlushPtr, &wait_event))
  break;

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

---

From: Peter Smith <[email-masked]>
Date: Mon, 4 Mar 2024 12:55:21 +1100

> As suggested by Amit, I moved this to code comments.

Was the total removal of this note deliberate? I only suggested
removing the *reason* for the rule, not the entire rule. Otherwise,
the user won't know to avoid doing this until they try it and get an
error.

> Same as Amit, I didn't change this.

AFAICT my previous review comment was misinterpreted. Please see [1]
for more details.

~~~~

Here are some more review comments for v104-00001

======
Commit message

1.
Additionally, The SQL functions pg_logical_slot_get_changes,
pg_logical_slot_peek_changes and pg_replication_slot_advance are modified
to wait for the replication slots specified in 'standby_slot_names' to
catch up before returning.

~

Maybe that should be expressed using similar wording as the docs...

SUGGESTION
Additionally, The SQL functions ... are modified. Now, when used with
failover-enabled logical slots, these functions will block until all
physical slots specified in 'standby_slot_names' have confirmed WAL
receipt.

======
doc/src/sgml/config.sgml

2.
+        <function>pg_logical_slot_peek_changes</function></link>,
+        when used with failover enabled logical slots, will block until all
+        physical slots specified in <varname>standby_slot_names</varname> have
+        confirmed WAL receipt.

/failover enabled logical slots/failover-enabled logical slots/

======
doc/src/sgml/func.sgml

3.
+        The function may be blocked if the specified slot is a failover enabled
+        slot and <link
linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>
+        is configured.
        </para></entry>

/a failover enabled slot//a failover-enabled slot/

~~~

4.
+        slot may return to an earlier position. The function may be blocked if
+        the specified slot is a failover enabled slot and
+        <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>
+        is configured.

/a failover enabled slot//a failover-enabled slot/

======

5.
+/*
+ * Wait for physical standbys to confirm receiving the given lsn.
+ *
+ * Used by logical decoding SQL functions that acquired failover enabled slot.
+ * It waits for physical standbys corresponding to the physical slots specified
+ * in the standby_slot_names GUC.
+ */
+void
+WaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)

/failover enabled slot/failover-enabled slot/

~~~

6.
+ /*
+ * Don't need to wait for the standby to catch up if the current acquired
+ * slot is not a failover enabled slot, or there is no value in
+ * standby_slot_names.
+ */

/failover enabled slot/failover-enabled slot/

======

7.
+
+ /*
+ * Wake up logical walsenders holding failover enabled slots after
+ * updating the restart_lsn of the physical slot.
+ */
+ PhysicalWakeupLogicalWalSnd();

/failover enabled slots/failover-enabled slots/

======

8.
+/*

/failover enabled slots/failover-enabled slots/

9.
+/*
+ * Returns true if not all standbys have caught up to the flushed position
+ * (flushed_lsn) when the current acquired slot is a failover enabled logical
+ * slot and we are streaming; otherwise, returns false.
+ *
+ * If returning true, the function sets the appropriate wait event in
+ * wait_event; otherwise, wait_event is set to 0.
+ */
+static bool
+NeedToWaitForStandby(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
+ uint32 *wait_event)

9a.
/failover enabled logical slot/failover-enabled logical slot/

~

9b.
Probably that function name should be plural.

/NeedToWaitForStandby/NeedToWaitForStandbys/

~~~

10.
+/*
+ * Returns true if we need to wait for WALs to be flushed to disk, or if not
+ * all standbys have caught up to the flushed position (flushed_lsn) when the
+ * current acquired slot is a failover enabled logical slot and we are
+ * streaming; otherwise, returns false.
+ *
+ * If returning true, the function sets the appropriate wait event in
+ * wait_event; otherwise, wait_event is set to 0.
+ */
+static bool
+NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
+ uint32 *wait_event)

/failover enabled logical slot/failover-enabled logical slot/

~~~

11. WalSndWaitForWal

+ /*
+ * Within the loop, we wait for the necessary WALs to be flushed to
+ * disk first, followed by waiting for standbys to catch up if there
+ * are enought WALs or upon receiving the shutdown signal. To avoid
+ * the scenario where standbys need to catch up to a newer WAL
+ * location in each iteration, we update our idea of the currently
+ * flushed position only if we are not waiting for standbys to catch
+ * up.
+ */

typo

/enought/enough/

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

Kind Regards,
Peter Smith.
Fujitsu Austalia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 4 Mar 2024 09:08:20 +0530

> anything about changing the encapsulation of the computation of the

No, I have understood it in the same way as you have outlined in this
email and liked the way the current patch has it.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 4 Mar 2024 09:19:13 +0530

> /failover enabled logical slots/failover-enabled logical slots/

How about just saying logical failover slots at this and other places?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 4 Mar 2024 14:57:44 +1100

> How about just saying logical failover slots at this and other places?

Yes, that wording works too.

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

---

From: Peter Smith <[email-masked]>
Date: Mon, 4 Mar 2024 15:05:12 +1100

> email and liked the way the current patch has it.

OK, if the code will remain as-is wouldn't it be better to anyway
change the function name to indicate what it really does?

e.g.  NeedToWaitForWal --> NeedToWaitForWalFlushOrStandbys

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

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Mar 2024 08:57:34 +0000

Hi,

> parameters like "temp_tablespaces". I don't have any better ideas but

What about "non_lagging_standby_slots"?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Mon, 4 Mar 2024 15:21:52 +0530

> e.g.  NeedToWaitForWal --> NeedToWaitForWalFlushOrStandbys

This seems too long. I would prefer the current name NeedToWaitForWal
as waiting for WAL means waiting to flush the WAL and waiting to
replicate it to standby. On similar lines, the variable name
standby_slot_oldest_flush_lsn looks too long. How about
ss_oldest_flush_lsn (where ss indicates standy_slots)?

Apart from this, I have made minor modifications in the attached.

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index aa477015bd..1530e7720c 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2390,8 +2390,8 @@ validate_standby_slots(char **newval)
 	{
 		/*
 		 * We cannot validate the replication slot if the replication slots'
-		 * data has not been initialized. This is ok as we will validate the
-		 * specified slot when waiting for them to catch up. See
+		 * data has not been initialized. This is ok as we will anyway validate
+		 * the specified slot when waiting for them to catch up. See
 		 * StandbySlotsHaveCaughtup for details.
 		 */
 	}
@@ -2473,8 +2473,7 @@ assign_standby_slot_names(const char *newval, void *extra)
 	char	   *standby_slot_names_cpy = extra;
 
 	/*
-	 * The standby slots may have changed, so we need to recompute the oldest
-	 * LSN.
+	 * The standby slots may have changed, so we must recompute the oldest LSN.
 	 */
 	standby_slot_oldest_flush_lsn = InvalidXLogRecPtr;
 
@@ -2664,6 +2663,7 @@ StandbySlotsHaveCaughtup(XLogRecPtr wait_for_lsn, int elevel)
 	if (caught_up_slot_num != list_length(standby_slot_names_list))
 		return false;
 
+	/* The standby_slot_oldest_flush_lsn must not retreat. */
 	Assert(XLogRecPtrIsInvalid(standby_slot_oldest_flush_lsn) ||
 		   min_restart_lsn >= standby_slot_oldest_flush_lsn);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index b71408d533..580f9dabd3 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1805,10 +1805,10 @@ NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
 
 	/*
 	 * Check if the standby slots have caught up to the flushed position. It
-	 * is good to wait up to flushed position and then let it send the changes
-	 * to logical subscribers one by one which are already covered in flushed
-	 * position without needing to wait on every change for standby
-	 * confirmation.
+	 * is good to wait up to the flushed position and then let the WalSender
+	 * send the changes to logical subscribers one by one which are already
+	 * covered by the flushed position without needing to wait on every change
+	 * for standby confirmation.
 	 */
 	if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))
 		return true;

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Mar 2024 11:22:16 +0000

Hi,

Thanks!

A few more random comments:

1 ===

s/blocked/waiting/ ?

2 ===

s/StandbySlotsHaveCaughtup/StandbySlotsHaveCaughtup()/ ?

3 ===

remove "really"?

4 ===

+       /*
+        * Don't need to wait for the standbys to catch up if there is no value in
+        * standby_slot_names.
+        */
+       if (standby_slot_names_list == NIL)
+               return true;
+
+       /*
+        * Don't need to wait for the standbys to catch up if we are on a standby
+        * server, since we do not support syncing slots to cascading standbys.
+        */
+       if (RecoveryInProgress())
+               return true;
+
+       /*
+        * Don't need to wait for the standbys to catch up if they are already
+        * beyond the specified WAL location.
+        */
+       if (!XLogRecPtrIsInvalid(standby_slot_oldest_flush_lsn) &&
+               standby_slot_oldest_flush_lsn >= wait_for_lsn)
+               return true;

What about using OR conditions instead?

5 ===

+static bool

Not a big deal but does it need to return a bool? (I mean it all depends of
the *wait_event value). Is it for better code readability in the caller?

6 ===

+static bool

Same questions as for NeedToWaitForStandby().

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 4 Mar 2024 13:26:13 +0000

Thanks for the comments!

> 
> 1 ===
> 
> +        The function may be blocked if the specified slot is a failover
> + enabled
> 
> s/blocked/waiting/ ?

Changed.

Changed.

> 
> 3 ===
> 
> +       /* Now verify if the specified slots really exist and have
> + correct type */
> 
> remove "really"?

Changed.

> Not a big deal but does it need to return a bool? (I mean it all depends of the
> *wait_event value). Is it for better code readability in the caller?
> 
> 6 ===
> 
> +static bool

I also feel the current style looks a bit cleaner, so didn’t change these.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 4 Mar 2024 13:26:47 +0000

On Monday, March 4, 2024 9:55 AM Peter Smith <[email-masked]> wrote:
> 
> On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> > > +         associated standbys or have standbys associated that are
> > > + not
> > > enabled
> Was the total removal of this note deliberate? I only suggested removing the
> *reason* for the rule, not the entire rule. Otherwise, the user won't know to
> avoid doing this until they try it and get an error.

OK, Added.

> > > + * is reported if any slots are dropped, invalidated, or inactive.
> > > + This
> > > I think it will be easier to understand if the calling code just
> > > calls to one or both of these functions as required.
> >
> AFAICT my previous review comment was misinterpreted. Please see [1] for
> more details.
> 
> ~~~~
> 

Thanks!

> to wait for the replication slots specified in 'standby_slot_names' to catch up
> before returning.
> 
> ~
> 
> failover-enabled logical slots, these functions will block until all physical slots
> specified in 'standby_slot_names' have confirmed WAL receipt.

Changed.

> +        physical slots specified in
> <varname>standby_slot_names</varname> have
> +        confirmed WAL receipt.
> 

Changed. Note that for this comment and remaining comments, 
I used the later version we agreed(logical failover slot).

Changed.

Changed.

> + * It waits for physical standbys corresponding to the physical slots
> +specified

Changed.

> 
> ~~~
> 
> 6.
> + /*
> + * Don't need to wait for the standby to catch up if the current
> + acquired

Changed.

Changed.

Changed.

> + * (flushed_lsn) when the current acquired slot is a failover enabled
> +logical

Changed.

> +when the

Changed.

Fixed.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 4 Mar 2024 13:28:04 +0000

On Monday, March 4, 2024 5:52 PM Amit Kapila <[email-masked]> wrote:
> This seems too long. I would prefer the current name NeedToWaitForWal as
> waiting for WAL means waiting to flush the WAL and waiting to replicate it to
> standby. On similar lines, the variable name standby_slot_oldest_flush_lsn looks
> too long. How about ss_oldest_flush_lsn (where ss indicates standy_slots)?
> 

Thanks, I have merged it.

Attach the V105 patch set which addressed Peter, Amit and Bertrand's comments.

This version also includes the following changes:
* We found a string matching issue for query_until() and fixed it.
* Removed one un-used parameter from NeedToWaitForStandbys.
* Disable the sub before testing the pg_logical_slot_get_changes in 040.pl, this is to prevent
This test from catching the warning from another walsender.
* Ran pgindent.

Best Regards,
Hou zj

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 4 Mar 2024 15:43:58 +0000

Hi,

On Mon, Mar 04, 2024 at 01:28:04PM +0000, Zhijie Hou (Fujitsu) wrote:
> Attach the V105 patch set

Thanks!

Sorry I missed those during the previous review:

1 ===

Commit message: "these functions will block until"

s/block/wait/ ?

2 ===

+        when used with logical failover slots, will block until all

s/block/wait/ ?

It seems those are the 2 remaining "block" that could deserve the proposed
above change.

3 ===

+               invalidated = slot->data.invalidated != RS_INVAL_NONE;
+               inactive = slot->active_pid == 0;

invalidated = (slot->data.invalidated != RS_INVAL_NONE);
inactive = (slot->active_pid == 0);

instead?

I think it's easier to read and it looks like this is the way it's written in
other places (at least the few I checked).

Regards,

---

From: Peter Smith <[email-masked]>
Date: Tue, 5 Mar 2024 11:40:27 +1100

Here are some review comments for v105-0001

==========
doc/src/sgml/config.sgml

1.
+       <para>
+        The standbys corresponding to the physical replication slots in
+        <varname>standby_slot_names</varname> must configure
+        <literal>sync_replication_slots = true</literal> so they can receive
+        logical failover slots changes from the primary.
+       </para>

/slots changes/slot changes/

======
doc/src/sgml/func.sgml

2.
+        The function may be waiting if the specified slot is a logical failover
+        slot and <link
linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>
+        is configured.

I know this has been through multiple versions already, but this
latest wording "may be waiting..." doesn't seem very good to me.

How about one of these?

* The function may not be able to return immediately if the specified
slot is a logical failover slot and standby_slot_names is configured.

* The function return might be blocked if the specified slot is a
logical failover slot and standby_slot_names is configured.

* If the specified slot is a logical failover slot then the function
will block until all physical slots specified in standby_slot_names
have confirmed WAL receipt.

* If the specified slot is a logical failover slot then the function
will not return until all physical slots specified in
standby_slot_names have confirmed WAL receipt.

~~~

3.
+        slot may return to an earlier position. The function may be waiting if
+        the specified slot is a logical failover slot and
+        <link linkend="guc-standby-slot-names"><varname>standby_slot_names</varname></link>

Same as previous review comment #2

======

4. WaitForStandbyConfirmation

+ * Used by logical decoding SQL functions that acquired logical failover slot.

IIUC it doesn't work like that. pg_logical_slot_get_changes_guts()
calls here unconditionally (i.e. the SQL functions don't even check if
they are failover slots before calling this) so the comment seems
misleading/redundant.

======

5. NeedToWaitForWal

+ /*
+ if (NeedToWaitForStandbys(flushed_lsn, wait_event))
+ return true;
+
+ *wait_event = 0;
+ return false;
+}
+

5a.
The comment (or part of it?) seems misplaced because it is talking
WalSender sending changes, but that is not happening in this function.

Also, isn't what this is saying already described by the other comment
in the caller? e.g.:

+ /*
+ * Within the loop, we wait for the necessary WALs to be flushed to
+ * disk first, followed by waiting for standbys to catch up if there
+ * are enough WALs or upon receiving the shutdown signal. To avoid the
+ * scenario where standbys need to catch up to a newer WAL location in
+ * each iteration, we update our idea of the currently flushed
+ * position only if we are not waiting for standbys to catch up.
+ */

~

5b.
Most of the code is unnecessary. AFAICT all this is exactly same as just 1 line:

return NeedToWaitForStandbys(flushed_lsn, wait_event);

~~~

6. WalSndWaitForWal

+ /*
+ * Within the loop, we wait for the necessary WALs to be flushed to
+ * disk first, followed by waiting for standbys to catch up if there
+ * are enough WALs or upon receiving the shutdown signal. To avoid the
+ * scenario where standbys need to catch up to a newer WAL location in
+ * each iteration, we update our idea of the currently flushed
+ * position only if we are not waiting for standbys to catch up.
+ */

Regarding that 1st sentence: maybe this logic used to be done
explicitly "within the loop" but IIUC this logic is now hidden inside
NeedToWaitForWal() so the comment should mention that.

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 5 Mar 2024 08:54:50 +0530

> What about "non_lagging_standby_slots"?

I still prefer the current one as that at least resembles with
existing synchronous_standby_names. I think we can change the GUC name
if we get an agreement on a better name before release. At this stage,
let's move with the current one.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 5 Mar 2024 09:15:20 +0530

> WalSender sending changes, but that is not happening in this function.

I don't think so. This is invoked only by walsender and a static
function. I don't see any other better place to mention this.

> Also, isn't what this is saying already described by the other comment

Oh no, here we are explaining the wait order.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 5 Mar 2024 12:05:22 +0530

> Oh no, here we are explaining the wait order.

I think there is a scope of improvement here. The comment inside
NeedToWaitForWal() which states that we need to wait here for standbys
on flush-position(and not on each change) should be outside of this
function. It is too embedded. And the comment which states the order
of wait (first flush and then standbys confirmation) should be outside
the for-loop in WalSndWaitForWal(), but yes we do need both the
comments. Attached a patch (.txt) for comments improvement, please
merge if appropriate.

thanks
Shveta
From e03332839dd804f3bc38937e677ba87ac10f981b Mon Sep 17 00:00:00 2001
From: Shveta Malik <[email-masked]>
Date: Tue, 5 Mar 2024 11:29:52 +0530
Subject: [PATCH v2] Comments improvement.

---
 src/backend/replication/walsender.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 96f44ba85d..6a082b42eb 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1799,13 +1799,6 @@ NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
 		return true;
 	}
 
-	/*
-	 * Check if the standby slots have caught up to the flushed position. It
-	 * is good to wait up to the flushed position and then let the WalSender
-	 * send the changes to logical subscribers one by one which are already
-	 * covered by the flushed position without needing to wait on every change
-	 * for standby confirmation.
-	 */
 	if (NeedToWaitForStandbys(flushed_lsn, wait_event))
 		return true;
 
@@ -1818,7 +1811,10 @@ NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
  *
  * If the walsender holds a logical slot that has enabled failover, we also
  * wait for all the specified streaming replication standby servers to
- * confirm receipt of WAL up to RecentFlushPtr.
+ * confirm receipt of WAL up to RecentFlushPtr. It's beneficial to wait
+ * here for the confirmation from standbys up to RecentFlushPtr rather
+ * than waiting in ReorderBufferCommit() before transmitting each
+ * change to logical subscribers, which is already covered in RecentFlushPtr.
  *
  * Returns end LSN of flushed WAL.  Normally this will be >= loc, but if we
  * detect a shutdown request (either from postmaster or client) we will return
@@ -1847,6 +1843,12 @@ WalSndWaitForWal(XLogRecPtr loc)
 	else
 		RecentFlushPtr = GetXLogReplayRecPtr(NULL);
 
+	/*
+	 * In the loop, we wait for the necessary WALs to be flushed to disk
+	 * initially, and then we wait for standbys to catch up. Upon receiving
+	 * the shutdown signal, we immediately transition to waiting for standbys
+	 * to catch up.
+	 */
 	for (;;)
 	{
 		bool		wait_for_standby_at_stop = false;
@@ -1877,12 +1879,10 @@ WalSndWaitForWal(XLogRecPtr loc)
 			XLogBackgroundFlush();
 
 		/*
-		 * Within the loop, we wait for the necessary WALs to be flushed to
-		 * disk first, followed by waiting for standbys to catch up if there
-		 * are enough WALs or upon receiving the shutdown signal. To avoid the
-		 * scenario where standbys need to catch up to a newer WAL location in
-		 * each iteration, we update our idea of the currently flushed
-		 * position only if we are not waiting for standbys to catch up.
+		 * Update our idea of the currently flushed position, but do it only
+		 * if we haven't started waiting for standbys. This is done to prevent
+		 * standbys from needing to catch up to a more recent WAL location
+		 * with each iteration.
 		 */
 		if (wait_event != WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION)
 		{
-- 
2.34.1

---

From: Nisha Moond <[email-masked]>
Date: Tue, 5 Mar 2024 12:15:15 +0530

I did performance tests for the v99 patch w.r.t. wait time analysis.
As this patch is introducing a wait for standby before sending changes
to a subscriber, at the primary node, logged time at the start and end
of the XLogSendLogical() call (which eventually calls
WalSndWaitForWal()) and calculated total time taken by this function
during the load run.

For load, ran pgbench for 15 minutes:
Creating tables: pgbench -p 5833 postgres -qis 2
Running benchmark: pgbench postgres -p 5833 -c 10 -j 3 -T 900 -P 20

Machine details:
11th Gen Intel(R) Core(TM) i9-11950H @ 2.60GHz 32GB RAM
OS - Windows 10 Enterprise

Test setup:
Primary node -->
      -> One physical standby node
      -> One subscriber node having only one subscription with failover=true

-- the slot-sync relevant parameters are set to default (OFF) for all
the tests i.e.
        hot_standby_feedback = off
        sync_replication_slots = false

-- addition configuration on each instance is:
shared_buffers = 6GB
max_worker_processes = 32
max_parallel_maintenance_workers = 24
max_parallel_workers = 32
synchronous_commit = off
checkpoint_timeout = 1d
max_wal_size = 24GB
min_wal_size = 15GB
autovacuum = off

To review the wait time impact with and without patch, compared three
cases (did two runs for each case)-
(1) HEAD code:
        time taken in run 1 = 103.935631 seconds
        time taken in run 2 = 104.832186 seconds

(2) HEAD code + v99 patch ('standby_slot_names' is not set):
        time taken in run 1 = 104.076343 seconds
        time taken in run 2 = 103.116226 seconds

(3) HEAD code + v99 patch + a valid 'standby_slot_names' is set:
        time taken in run 1 = 103.871012 seconds
        time taken in run 2 = 103.793524 seconds

The time consumption of XLogSendLogical() is almost same in all the
cases and no performance degradation is observed.

--
Thanks,
Nisha

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 5 Mar 2024 07:15:29 +0000

> Sorry I missed those during the previous review:

No problem, thanks for the comments!

> It seems those are the 2 remaining "block" that could deserve the proposed

I prefer using 'block' here. And it seems others also suggest
to change the 'wait'[1].

> invalidated = (slot->data.invalidated != RS_INVAL_NONE); inactive =
> (slot->active_pid == 0);
> 
> instead?
> 
> I think it's easier to read and it looks like this is the way it's written in other
> places (at least the few I checked).

I think the current code is consistent with other similar code in slot.c.
(grep "data.invalidated != RS_INVAL_NONE").

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

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 5 Mar 2024 07:21:24 +0000

On Tuesday, March 5, 2024 2:35 PM shveta malik <[email-masked]> wrote:
> > > + * Check if the standby slots have caught up to the flushed
> > > + position. It
> > > + * is good to wait up to the flushed position and then let the
> > > + WalSender
> > > + * send the changes to logical subscribers one by one which are
> > > + already
> > > + * covered by the flushed position without needing to wait on every
> > > + change
> > > + if (NeedToWaitForStandbys(flushed_lsn, wait_event)) return true;
> > > +
> > > + *wait_event = 0;
> > > + return false;
> > > +}
> > > +
> > >
> > > 5a.
> > > Also, isn't what this is saying already described by the other
> > > comment in the caller? e.g.:
> > >
> >
> NeedToWaitForWal() which states that we need to wait here for standbys on
> flush-position(and not on each change) should be outside of this function. It is
> too embedded. And the comment which states the order of wait (first flush and
> then standbys confirmation) should be outside the for-loop in
> WalSndWaitForWal(), but yes we do need both the comments. Attached a
> patch (.txt) for comments improvement, please merge if appropriate.

Thanks, I have slightly modified the top-up patch and merged it.

Attach the V106 patch which addressed above and Peter's comments[1].

Best Regards,
Hou zj

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 6 Mar 2024 10:30:17 +0900

> assign_createrole_self_grant ...).

Why does it have to be a List in the first place? In earlier version
patches, we used to copy the list and delete the element until it
became empty, while waiting for physical wal senders. But we now just
refer to each slot name in the list. The current code assumes that
stnadby_slot_names_cpy is allocated in GUCMemoryContext but once it
changes, it will silently get broken. I think we can check and assign
standby_slot_names in a similar way to check/assign_temp_tablespaces
and check/assign_synchronous_standby_names.

Regards,

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 6 Mar 2024 03:03:44 +0000

On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada <[email-masked]> wrote:

Hi,

> On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu) <[email-masked]>
> wrote:
> >
> > > Given that the newval and extra have the same data
> > > (standby_slot_names value), why do we not use newval instead? I
> > > think that if we use newval, we don't need to guc_strdup() in
> > > check_standby_slot_names(), we might need to do list_copy_deep()
> > > instead, though. It's not clear to me as there is no comment.
> >
> > I think SplitIdentifierString will modify the passed in string, so
> > we'd better not pass the newval to it, otherwise the stored guc
> > string(standby_slot_names) will be changed. I can see we are doing
> > similar thing in other GUC check/assign function as well.
> > (check_wal_consistency_checking/ assign_wal_consistency_checking,
> > check_createrole_self_grant/ assign_createrole_self_grant ...).
> 
> Why does it have to be a List in the first place? 

I thought the List type is convenient to use here, as we have existing list
build function(SplitIdentifierString), and have convenient list macro to loop
the list(foreach_ptr) which can save some codes.

> In earlier version patches, we
> used to copy the list and delete the element until it became empty, while
> waiting for physical wal senders. But we now just refer to each slot name in the
> list. The current code assumes that stnadby_slot_names_cpy is allocated in
> GUCMemoryContext but once it changes, it will silently get broken. I think we
> can check and assign standby_slot_names in a similar way to
> check/assign_temp_tablespaces and
> check/assign_synchronous_standby_names.

Yes, we could do follow it by allocating an array and copy each slot name into
it, but it also requires some codes to build and scan the array. So, is it possible
to expose the GucMemorycontext or have an API like guc_copy_list instead ?
If we don't want to touch the guc api, I am ok with using an array as well.

Best Regards,
Hou zj

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 6 Mar 2024 15:37:00 +0900

> Hi, a while ago I asked this same question. See [1 #28] for the response..

Thanks. Quoting the response from the email:

SplitIdentifierString() does not give error for '*' and '*' can be considered
as valid value which if accepted can mislead user that all the standbys's slots
are now considered, which is not the case here. So we want to explicitly call
out this case i.e. '*' is not accepted as valid value for standby_slot_names.

IIUC we're concerned with a case like where the user confused
standby_slot_names values with synchronous_standby_names values. Which
means we would need to keep thath check consistent with available
values of synchronous_standby_names. For example, if we support a
regexp for synchronous_standby_names, we will have to update the check
so we disallow other special characters. Also, if we add a new
replication-related parameter that accepts other special characters as
the value in the future, will we want to raise an error also for such
values in check_standby_slot_names()?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 6 Mar 2024 14:23:08 +0530

On Wed, Mar 6, 2024 at 12:07 PM Masahiko Sawada <[email-masked]> wrote:
> values of synchronous_standby_names.
>

Both have different formats to specify. For example, for
synchronous_standby_names we have the following kind of syntax to
specify:
[FIRST] num_sync ( standby_name [, ...] )
ANY num_sync ( standby_name [, ...] )
standby_name [, ...]

I don't think we can have a common check for both of them as the
specifications are different. In fact, I don't think we need a special
check for '*'. The user will anyway get a WARNING at a later point
that the replication slot with that name doesn't exist.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 6 Mar 2024 13:13:07 +0000

On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> I thought the List type is convenient to use here, as we have existing list build
> function(SplitIdentifierString), and have convenient list macro to loop the
> list(foreach_ptr) which can save some codes.
> 
> > used to copy the list and delete the element until it became empty,
> > while waiting for physical wal senders. But we now just refer to each
> > slot name in the list. The current code assumes that
> Yes, we could do follow it by allocating an array and copy each slot name into it,
> but it also requires some codes to build and scan the array. So, is it possible to
> expose the GucMemorycontext or have an API like guc_copy_list instead ?

I rethink about this and realize that it's not good to do the memory allocation
in assign hook function. As the "src/backend/utils/misc/README" said, we'd better
do that in check hook function and pass it via extra to assign hook function. And thus
array is a good choice in this case rather than a List which cannot be passed to *extra.

Here is the V107 patch set which parse and cache the standby slot names in an
array instead of a List.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 6 Mar 2024 13:24:28 +0000

On Wednesday, March 6, 2024 9:13 PM Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> > I thought the List type is convenient to use here, as we have existing
> > list build function(SplitIdentifierString), and have convenient list
> > macro to loop the
> > > while waiting for physical wal senders. But we now just refer to
> > > each slot name in the list. The current code assumes that
> > > changes, it will silently get broken. I think we can check and
> > > assign standby_slot_names in a similar way to
> > Yes, we could do follow it by allocating an array and copy each slot
> > name into it, but it also requires some codes to build and scan the
> > array. So, is it possible to expose the GucMemorycontext or have an API like
> guc_copy_list instead ?
> I rethink about this and realize that it's not good to do the memory allocation in
> assign hook function. As the "src/backend/utils/misc/README" said, we'd
> better do that in check hook function and pass it via extra to assign hook
> function. And thus array is a good choice in this case rather than a List which
> cannot be passed to *extra.
> 

The patch needs to be rebased due to recent commit.

Attach the V107_2 path set. There are no code changes in this version.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Thu, 7 Mar 2024 13:04:51 +1100

Here are some review comments for v107-0001

======

1.
+/*
+ * Struct for the configuration of standby_slot_names.
+ *
+ * Note: this must be a flat representation that can be held in a single chunk
+ * of guc_malloc'd memory, so that it can be stored as the "extra" data for the
+ * standby_slot_names GUC.
+ */
+typedef struct
+{
+ int slot_num;
+
+ /* slot_names contains nmembers consecutive nul-terminated C strings */
+ char slot_names[FLEXIBLE_ARRAY_MEMBER];
+} StandbySlotConfigData;
+

1a.
To avoid any ambiguity this 1st field is somehow a slot ID number, I
felt a better name would be 'nslotnames' or even just 'n' or 'count',

~

1b.
(fix typo)

SUGGESTION for the 2nd field comment
slot_names is a chunk of 'n' X consecutive null-terminated C strings

~

1c.
A more explanatory name for this typedef maybe is 'StandbySlotNamesConfigData' ?

~~~

2.
+/* This is parsed and cached configuration for standby_slot_names */
+static StandbySlotConfigData *standby_slot_config;

2a.
/This is parsed and cached configuration for .../This is the parsed
and cached configuration for .../

~

2b.
Similar to above -- since this only has name information maybe it is
more correct to call it 'standby_slot_names_config'?

~~~

3.
+/*
+ * A helper function to validate slots specified in GUC standby_slot_names.
+ *
+ * The rawname will be parsed, and the parsed result will be saved into
+ * *elemlist.
+ */
+static bool
+validate_standby_slots(char *rawname, List **elemlist)

/and the parsed result/and the result/

~~~

4. check_standby_slot_names

/copy of string/copy of the GUC string/

~~~

5.
+assign_standby_slot_names(const char *newval, void *extra)
+{
+ /*
+ * The standby slots may have changed, so we must recompute the oldest
+ * LSN.
+ */
+ ss_oldest_flush_lsn = InvalidXLogRecPtr;
+
+ standby_slot_config = (StandbySlotConfigData *) extra;
+}

To avoid leaking don't we need to somewhere take care to free any
memory used by a previous value (if any) of this
'standby_slot_config'?

~~~

6. AcquiredStandbySlot

+/*
+ * Return true if the currently acquired slot is specified in
+ * standby_slot_names GUC; otherwise, return false.
+ */
+bool
+AcquiredStandbySlot(void)
+{
+ const char *name;
+
+ /* Return false if there is no value in standby_slot_names */
+ if (standby_slot_config == NULL)
+ return false;
+
+ name = standby_slot_config->slot_names;
+ for (int i = 0; i < standby_slot_config->slot_num; i++)
+ {
+ if (strcmp(name, NameStr(MyReplicationSlot->data.name)) == 0)
+ return true;
+
+ name += strlen(name) + 1;
+ }
+
+ return false;
+}

6a.
Just checking "(standby_slot_config == NULL)" doesn't seem enough to
me, because IIUIC it is possible when 'standby_slot_names' has no
value then maybe standby_slot_config is not NULL but
standby_slot_config->slot_num is 0.

~

6b.
IMO this function would be tidier written such that the
MyReplicationSlot->data.name is passed as a parameter. Then you can
name the function more naturally like:

IsSlotInStandbySlotNames(const char *slot_name)

~

6c.
IMO the body of the function will be tidier if written so there are
only 2 returns instead of 3 like

SUGGESTION:
if (...)
{
  for (...)
  {
     ...
return true;
  }
}
return false;

~~~

7.
+ /*
+ * Don't need to wait for the standbys to catch up if there is no value in
+ * standby_slot_names.
+ */
+ if (standby_slot_config == NULL)
+ return true;

(similar to a previous review comment)

This check doesn't seem enough because IIUIC it is possible when
'standby_slot_names' has no value then maybe standby_slot_config is
not NULL but standby_slot_config->slot_num is 0.

~~~

8. WaitForStandbyConfirmation

+ /*
+ * Don't need to wait for the standby to catch up if the current acquired
+ * slot is not a logical failover slot, or there is no value in
+ * standby_slot_names.
+ */
+ if (!MyReplicationSlot->data.failover || !standby_slot_config)
+ return;

(similar to a previous review comment)

IIUIC it is possible that when 'standby_slot_names' has no value, then
standby_slot_config is not NULL but standby_slot_config->slot_num is
0. So shouldn't that be checked too?

Perhaps it is convenient to encapsulate this check using some macro:
#define StandbySlotNamesHasNoValue() (standby_slot_config = NULL ||
standby_slot_config->slot_num == 0)

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

---

From: shveta malik <[email-masked]>
Date: Thu, 7 Mar 2024 08:37:01 +0530

> Attach the V107_2 path set. There are no code changes in this version.

 The patch needed to be rebased due to a recent commit. Attached
v107_3, there are no code changes in this version.

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 7 Mar 2024 12:19:11 +0900

On Wed, Mar 6, 2024 at 5:53 PM Amit Kapila <[email-masked]> wrote:
> check for '*'.

I think so too.

> The user will anyway get a WARNING at a later point

Right.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 7 Mar 2024 10:16:03 +0530

> felt a better name would be 'nslotnames' or even just 'n' or 'count',

We can probably just add a comment above slot_num and that should be
sufficient but I am fine with 'nslotnames' as well, in anycase let's
add a comment for the same.

> IsSlotInStandbySlotNames(const char *slot_name)

+1. How about naming it as SlotExistsinStandbySlotNames(char
*slot_name) and pass the slot_name from MyReplicationSlot? Otherwise,
we need an Assert for MyReplicationSlot in this function.

Also, can we add a comment like below before the loop:
+ /*
+ * XXX: We are not expecting this list to be long so a linear search
+ * shouldn't hurt but if that turns out not to be true then we can cache
+ * this information for each WalSender as well.
+ */

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 7 Mar 2024 11:24:53 +0530

On Thu, Mar 7, 2024 at 8:37 AM shveta malik <[email-masked]> wrote:

I thought about whether we can make standby_slot_names as USERSET
instead of SIGHUP and it doesn't sound like a good idea as that can
lead to inconsistent standby replicas even after configuring the
correct value of standby_slot_names. One can set a different or ''
(empty) value for a particular session and consume all changes from
the slot without waiting for the standby to acknowledge the change.
Also, it would be difficult for users to ensure whether the standby is
always ahead of subscribers. Does anyone think differently?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 7 Mar 2024 06:28:26 +0000

Thanks for the comments.

> + * Note: this must be a flat representation that can be held in a
> +single chunk
> + * of guc_malloc'd memory, so that it can be stored as the "extra" data
> +for the
> + /* slot_names contains nmembers consecutive nul-terminated C strings
> +*/  char slot_names[FLEXIBLE_ARRAY_MEMBER];
> To avoid any ambiguity this 1st field is somehow a slot ID number, I felt a better
> name would be 'nslotnames' or even just 'n' or 'count',

Changed to 'nslotnames'.

Changed.

Changed.

> /This is parsed and cached configuration for .../This is the parsed and cached
> configuration for .../

Changed.

> 
> ~
> 
> 2b.
> Similar to above -- since this only has name information maybe it is more
> correct to call it 'standby_slot_names_config'?
> 

Changed.

Changed.

Changed.

> + standby_slot_config = (StandbySlotConfigData *) extra; }
> 
> To avoid leaking don't we need to somewhere take care to free any memory
> used by a previous value (if any) of this 'standby_slot_config'?
> 

The memory of extra is maintained by the GUC mechanism. It will be
automatically freed when the associated GUC setting is no longer of interest.
See src/backend/utils/misc/README for details.

> + /* Return false if there is no value in standby_slot_names */ if
> + (standby_slot_config == NULL) return false;
> +
> + name = standby_slot_config->slot_names; for (int i = 0; i <
> + standby_slot_config->slot_num; i++) { if (strcmp(name,
> + NameStr(MyReplicationSlot->data.name)) == 0) return true;
> +
> Just checking "(standby_slot_config == NULL)" doesn't seem enough to me,
> because IIUIC it is possible when 'standby_slot_names' has no value then
> maybe standby_slot_config is not NULL but standby_slot_config->slot_num is
> 0.

The standby_slot_config will always be NULL if there is no value in it.

While checking, I did find a rare case that if there are only some white space
in the standby_slot_names, then slot_num will be 0, and have fixed it so that
standby_slot_config will always be NULL if there is no meaning value in guc.

> IsSlotInStandbySlotNames(const char *slot_name)

Changed it to SlotExistsInStandbySlotNames.

> 
> ~
> 
> 6c.
> IMO the body of the function will be tidier if written so there are only 2 returns
> instead of 3 like
> 
> SUGGESTION:
> if (...)
> {
>   for (...)
>   {
>      ...
> return true;
>   }
> }
> return false;

> 
> ~~~
> 
> 7.
> + /*
> + * Don't need to wait for the standbys to catch up if there is no value
> + in
> + * standby_slot_names.
> + */
> 'standby_slot_names' has no value then maybe standby_slot_config is not NULL
> but standby_slot_config->slot_num is 0.

Same as above.

> + if (!MyReplicationSlot->data.failover || !standby_slot_config) return;
> 
> standby_slot_config is not NULL but standby_slot_config->slot_num is 0. So
> shouldn't that be checked too?
> 

Same as above, I think we can avoid checking slot_num.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 7 Mar 2024 06:30:09 +0000

Added.

> we need an Assert for MyReplicationSlot in this function.

Changed as suggested.

Added.

Attach the V108 patch set which addressed above and Peter's comments.
I also removed the check for "*" in guc check hook.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Fri, 8 Mar 2024 09:03:01 +0530

> I also removed the check for "*" in guc check hook.

Pushed with minor modifications. I'll keep an eye on BF.

BTW, one thing that we should try to evaluate a bit more is the
traversal of slots in StandbySlotsHaveCaughtup() where we verify if
all the slots mentioned in standby_slot_names have received the
required WAL. Even if the standby_slot_names list is short the total
number of slots can be much larger which can lead to an increase in
CPU usage during traversal. There is an optimization that allows to
cache ss_oldest_flush_lsn and ensures that we don't need to traverse
the slots each time so it may not hit frequently but still there is a
chance. I see it is possible to further optimize this area by caching
the position of each slot mentioned in standby_slot_names in
replication_slots array but not sure whether it is worth.

-- 
With Regards,
Amit Kapila.

---

From: Ajin Cherian <[email-masked]>
Date: Fri, 8 Mar 2024 15:26:35 +1100

> replication_slots array but not sure whether it is worth.
I tried to test this by configuring a large number of logical slots while
making sure the standby slots are at the end of the array and checking if
there was any performance hit in logical replication from these searches.

Setup:
1. 1 primary server configured with 3 servers in the standby_slot_names, 1
extra logical slot (not configured for failover) + 1 logical subscriber
configures as failover + 3 physical standbys(all configured to sync logical
slots)

2. 1 primary server configured with 3 servers in the standby_slot_names,
100 extra logical slot (not configured for failover) + 1 logical subscriber
configures as failover + 3 physical standbys(all configured to sync logical
slots)

3. 1 primary server configured with 3 servers in the standby_slot_names,
500 extra logical slot (not configured for failover) + 1 logical subscriber
configures as failover + 3 physical standbys(all configured to sync logical
slots)

In the three setups, 3 standby_slot_names are compared with a list of 2,101
and 501 slots respectively.

I ran a pgbench for 15 minutes for all 3 setups:

Case 1: Average TPS - 8.143399 TPS
Case 2: Average TPS - 8.187462 TPS
Case 3: Average TPS - 8.190611 TPS

I see no degradation in the performance, the differences in performance are
well within the run to run variations seen.

Nisha also did some performance tests to record the lag introduced by the
large number of slots traversal in StandbySlotsHaveCaughtup(). The tests
logged time at the start and end of the XLogSendLogical() call (which
eventually calls WalSndWaitForWal() --> StandbySlotsHaveCaughtup())  and
calculated total time taken by this function during the load run for
different total slots count.

Setup:
--one primary with 3 standbys and one subscriber with one active
subscription
--hot_standby_feedback=off and sync_replication_slots=false
--made sure the standby slots remain at the end
ReplicationSlotCtl->replication_slots array to measure performance of worst
case scenario for standby slot search in StandbySlotsHaveCaughtup()

pgbench for 15 min was run. Here is the data:

Case1 : with 1 logical slot, standby_slot_names having 3 slots
Run1: 626.141642 secs
Run2: 631.930254 secs

Case2 : with 100 logical slots,  standby_slot_names having 3 slots
Run1: 629.38332 secs
Run2: 630.548432 secs

Case3 : with 500 logical slots,  standby_slot_names having 3 slots
Run1: 629.910829 secs
Run2: 627.924183 secs

There was no degradation in performance seen.

Thanks Nisha for helping with the testing.

regards,
Ajin Cherian
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Fri, 8 Mar 2024 10:39:21 +0530

On Fri, Mar 8, 2024 at 9:56 AM Ajin Cherian <[email-masked]> wrote:
> I tried to test this by configuring a large number of logical slots while making sure the standby slots are at the end of the array and checking if there was any performance hit in logical replication from these searches.
>

Thanks  Ajin and Nisha.

We also plan:
1) Redoing XLogSendLogical time-log related test with
'sync_replication_slots' enabled.
2) pg_recvlogical test to monitor lag in StandbySlotsHaveCaughtup()
for a large number of slots.
3) Profiling to see if StandbySlotsHaveCaughtup() is noticeable in the
report when there are a large number of slots to traverse.

thanks
Shveta