RE: Synchronizing slots from primary to standby

On Tuesday, November 21, 2023 1:39 PM Peter Smith <smithpb2250@gmail.com> wrote:

Hi,

Thanks for the comments.

> 
> ======
> doc/src/sgml/catalogs.sgml
> 
> 6.
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>subfailoverstate</structfield> <type>char</type>
> +      </para>
> +      <para>
> +       State codes for failover mode:
> +       <literal>d</literal> = disabled,
> +       <literal>p</literal> = pending enablement,
> +       <literal>e</literal> = enabled
> +      </para></entry>
> +     </row>
> +
> 
> This attribute is very similar to the 'subtwophasestate' so IMO it would be
> better to be adjacent to that one in the docs.
> 
> (probably this means putting it in the same order in the catalog also, assuming
> that is allowed)

It's allowed, but I think the functionality of two fields are different and I didn’t find
the correlation between two fields except for the type of value. So I didn't change the order.

> 
> ~~~
> 
> 12. AlterSubscription
> 
> + if (sub->failoverstate == LOGICALREP_FAILOVER_STATE_ENABLED &&
> + opts.copy_data) ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
> when failover is enabled"),
> + errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh =
> false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.")));
> 
> ~
> 
> 12b.
> AFAIK when there are messages like this that differ only by non-translatable
> things ("failover" option) then that non-translatable thing should be extracted
> as a parameter so the messages are common.
> And, don't forget to add a /* translator: %s is a subscription option like
> 'failover' */ comment.
> 
> SUGGESTION like:
> errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
> when %s is enabled", "two_phase") errmsg("ALTER SUBSCRIPTION with refresh
> and copy_data is not allowed when %s is enabled", "failover")

I am not sure about changing the existing message here, I feel you can start a
separate thread to change the twophase related messages, and we can change accordingly
if it's accepted.

> 
> ======
> .../libpqwalreceiver/libpqwalreceiver.c
> 
> 15. libpqrcv_create_slot
> 
> + if (failover)
> + {
> + appendStringInfoString(&cmd, "FAILOVER"); if (use_new_options_syntax)
> + appendStringInfoString(&cmd, ", "); else appendStringInfoChar(&cmd, '
> + '); }
> 
> 15a.
> Isn't failover a new option that is unsupported pre-PG17? Why is it necessary to
> support an old-style syntax for something that was not supported on old
> servers? (I'm confused).
> 
> ~
> 
> 15b.
> Also IIRC, this FAILOVER wasn't not listed in the old-style syntax of
> doc/src/sgml/protocol.sgml. Was that deliberate?

We don't support FAILOVER for old-style syntax and pre-PG17,
libpqrcv_create_slot is only building the replication command string and we
will add failover in the string so that the publisher will report errors if it
doesn't support these options ,the same is true for two_phase.


> ~~~
> 
> 24. ReplicationSlotAlter
> +/*
> + * Change the definition of the slot identified by the passed in name.
> + */
> +void
> +ReplicationSlotAlter(const char *name, bool failover)
> 
> /the definition/the failover state/

I kept this as it's a general function but we only
support changing failover state for now.


> ~~~
> 
> 28. check_standby_slot_names
> 
> +bool
> +check_standby_slot_names(char **newval, void **extra, GucSource source)
> +{  if (strcmp(*newval, "") == 0)  return true;
> +
> + /*
> + * "*" 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("\"%s\" is not accepted for standby_slot_names",
> + *newval); return false; }
> +
> + /* Now verify if the specified slots really exist and have correct
> + type */ if (!validate_standby_slots(newval)) return false;
> +
> + *extra = guc_strdup(ERROR, *newval);
> +
> + return true;
> +}
> 
> Is it really necessary to have a special test for the special value "*" which you are
> going to reject? I don't see why this should be any different from checking for
> other values like "." or "$" or "?" etc.
> Why not just let validate_standby_slots() handle all of these?

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.

> 
> ~~~
> 
> 29. assign_standby_slot_names
> 
> + /* No value is specified for standby_slot_names. */ if
> + (standby_slot_names_cpy == NULL) return;
> 
> Is this possible? IIUC the check_standby_slot_names() did:
> *extra = guc_strdup(ERROR, *newval);
> 
> Maybe this code also needs a similar elog and comment like already in this
> function:
> /* This should not happen if GUC checked check_standby_slot_names. */

This case is possible, standby_slot_names_cpy(e.g. extra pointer) is NULL if no
value("") is specified for the GUC.(see the code in check_standby_slot_names).

> ~
> 
> 30. assign_standby_slot_names
> 
> + char    *standby_slot_names_cpy = extra;
> 
> IIUC, the 'extra' was unconditionally guc_strdup()'ed in the check hook, so
> should we also free it here before leaving this function?

No, as mentioned in src/backend/utils/misc/README, the space of extra
will be automatically freed when the associated GUC setting is no longer of interest.

> 
> ~~~
> 
> 31. GetStandbySlotList
> 
> +/*
> + * Return a copy of standby_slot_names_list if the copy flag is set to
> +true,
> + * otherwise return the original list.
> + */
> +List *
> +GetStandbySlotList(bool copy)
> +{
> + if (copy)
> + return list_copy(standby_slot_names_list);
> + else
> + return standby_slot_names_list;
> +}
> 
> Why is this better than just exposing the standby_slot_names_list. The caller
> can make a copy or not.
> e.g. why is calling GetStandbySlotList(true) better than just doing
> list_copy(standby_slot_names_list)?

I think either way is fine, but I prefer not to add one global
variable if possible.

> 
> ~~~
> 
> 34. WalSndFilterStandbySlots
> 
> + /* Log warning if no active_pid for this physical slot */ if
> + (slot->active_pid == 0) ereport(WARNING,
> 
> Other nearby code is guarding the slot in case it was NULL, so why not here? Is
> it a potential NPE?

I think it will not pass the check for restart_lsn before the active_pid if slot is NULL.

> 
> ~~~
> 
> 35.
> + /*
> + * If logical slot name is given in standby_slot_names, give WARNING
> + * and skip it. Since it is harmless, so WARNING should be enough, no
> + * need to error-out.
> + */
> + else if (SlotIsLogical(slot))
> + warningfmt = _("cannot have logical replication slot \"%s\" in
> parameter \"%s\", ignoring");
> 
> Is this possible? Doesn't the function 'validate_standby_slots' called by the GUC
> hook prevent specifying logical slots in the GUC? Maybe this warning should be
> changed to Assert?

I think user could drop the logical slot and recreate a physical slot with the same name
without changing the GUC.

> 
> ~~~
> 
> 36.
> + /*
> + * Reaching here indicates that either the slot has passed the
> + * wait_for_lsn or there is an issue with the slot that requires a
> + * warning to be reported.
> + */
> + if (warningfmt)
> + ereport(WARNING, errmsg(warningfmt, name, "standby_slot_names"));
> +
> + standby_slots_cpy = foreach_delete_current(standby_slots_cpy, lc);
> 
> If something was wrong with the slot that required a warning, is it really OK to
> remove this slot from the list? This seems contrary to the function comment
> which only talks about removing slots that have caught up.

I think it's OK to remove slots if it's invalidated, dropped, or was
changed to logical one as we don't need to wait for these slots to catch up anymore.

> ~~~
> 
> 41.
>   /*
> - * Fast path to avoid acquiring the spinlock in case we already know we
> - * have enough WAL available. This is particularly interesting if we're
> - * far behind.
> + * Check if all the standby servers have confirmed receipt of WAL upto
> + * RecentFlushPtr if we already know we have enough WAL available.
> + *
> + * Note that we cannot directly return without checking the status of
> + * standby servers because the standby_slot_names may have changed,
> + which
> + * means there could be new standby slots in the list that have not yet
> + * caught up to the RecentFlushPtr.
>   */
>   if (RecentFlushPtr != InvalidXLogRecPtr &&
>   loc <= RecentFlushPtr)
> - return RecentFlushPtr;
> + {
> + WalSndFilterStandbySlots(RecentFlushPtr, &standby_slots);
> 
> 41b.
> IMO there is some missing information in this comment because it wasn't clear
> to me that calling WalSndFilterStandbySlots was going to side-efect that list to
> give it a different meaning. e.g. it seems it no longer means "standby slots" but
> instead means something like "standby slots that are not caught up". Perhaps
> that local variable can have a name that helps to convey that better?

I am not sure about this, WalSndFilterStandbySlots already indicates it will
filter the slot list which seems clear to me. But if you have better ideas, we can
adjust in next version.

> 
> ~~~
> 
> 44.
> + if (wait_for_standby)
> + ConditionVariablePrepareToSleep(&WalSndCtl->wal_confirm_rcv_cv);
> + else if (MyWalSnd->kind == REPLICATION_KIND_PHYSICAL)
>   ConditionVariablePrepareToSleep(&WalSndCtl->wal_flush_cv);
>   else if (MyWalSnd->kind == REPLICATION_KIND_LOGICAL)
>   ConditionVariablePrepareToSleep(&WalSndCtl->wal_replay_cv);
> ~
> 
> A walsender is either physical or logical, but here the 'wait_for_standby' flag
> overrides everything. Is it OK for this to be if/else/else or should this code call
> for wal_confirm_rcv_cv AND the other one?

No, we cannot prepare to sleep twice(see the comment in
ConditionVariablePrepareToSleep()).

> ======
> src/include/catalog/pg_subscription.h
> 
> 54.
> /*
>  * two_phase tri-state values. See comments atop worker.c to know more
> about
>  * these states.
>  */
> #define LOGICALREP_TWOPHASE_STATE_DISABLED 'd'
> #define LOGICALREP_TWOPHASE_STATE_PENDING 'p'
> #define LOGICALREP_TWOPHASE_STATE_ENABLED 'e'
> 
> #define LOGICALREP_FAILOVER_STATE_DISABLED 'd'
> #define LOGICALREP_FAILOVER_STATE_PENDING 'p'
> #define LOGICALREP_FAILOVER_STATE_ENABLED 'e'
> 
> ~
> 
> 54a.
> There should either be another comment (like the 'two_phase tri-state'
> one) added for the FAILOVER states or that existing comment should be
> expanded so that it also mentions the 'failover' tri-states.
> 
> ~
> 
> 54b.
> Idea: If you are willing to change the constant names (not the values) of the
> current tri-states then now both the 'two_phase' and 'failover'
> could share them -- I also think this might give the ability to create macros (if
> wanted) or to share more code instead of always handling failover and
> two_phase separately.
> 
> SUGGESTION
> #define LOGICALREP_TRISTATE_DISABLED 'd'
> #define LOGICALREP_TRISTATE_PENDING 'p'
> #define LOGICALREP_TRISTATE_ENABLED 'e'

I am not sure about the idea, but if others also prefer this then
we can adjust the code.


~~~ 
On Wednesday, November 22, 2023 3:42 PM Peter Smith <smithpb2250@gmail.com> wrote:
> 6.
> +# 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.
> +$result = $subscriber1->safe_psql('postgres',
> + "SELECT count(*) = 0 FROM tab_int;");
> +is($result, 't', "subscriber1 doesn't get data from primary until
> standby1 acknowledges changes");
> 
> Might it be better to write as "SELECT count(*) = $primary_row_count FROM
> tab_int;" and expect it to return false?

Ensuring the number is 0 looks better to me.

Attach the V38 patch set which addressed all comments in [1][2]
except for the ones that mentioned above.

[1] https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD55bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPuEGX5kr0xh06yv8ndoAQvDNedoec1OqOq3GMxDN6p%3D9A%40mail.gmail.com

Best Regards,
Hou zj