On Wed, Dec 13, 2023 at 3:53 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Hi Shveta, here are some review comments for v45-0002. >
Thanks for the feedback. Addressed these in v48. Please find my comments on some. > ====== > doc/src/sgml/bgworker.sgml > > 1. > + <variablelist> > + <varlistentry> > + <term><literal>BgWorkerStart_PostmasterStart</literal></term> > + <listitem> > + <para> > + > <indexterm><primary>BgWorkerStart_PostmasterStart</primary></indexterm> > + Start as soon as postgres itself has finished its own initialization; > + processes requesting this are not eligible for database connections. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > + <term><literal>BgWorkerStart_ConsistentState</literal></term> > + <listitem> > + <para> > + > <indexterm><primary>BgWorkerStart_ConsistentState</primary></indexterm> > + Start as soon as a consistent state has been reached in a hot-standby, > + allowing processes to connect to databases and run read-only queries. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > + <term><literal>BgWorkerStart_RecoveryFinished</literal></term> > + <listitem> > + <para> > + > <indexterm><primary>BgWorkerStart_RecoveryFinished</primary></indexterm> > + Start as soon as the system has entered normal read-write state. Note > + that the <literal>BgWorkerStart_ConsistentState</literal> and > + <literal>BgWorkerStart_RecoveryFinished</literal> are equivalent > + in a server that's not a hot standby. > + </para> > + </listitem> > + </varlistentry> > + > + <varlistentry> > + <term><literal>BgWorkerStart_ConsistentState_HotStandby</literal></term> > + <listitem> > + <para> > + > <indexterm><primary>BgWorkerStart_ConsistentState_HotStandby</primary></indexterm> > + Same meaning as <literal>BgWorkerStart_ConsistentState</literal> but > + it is more strict in terms of the server i.e. start the worker only > + if it is hot-standby. > + </para> > + </listitem> > + </varlistentry> > + </variablelist> > > Maybe reorder these slightly, because I felt it is better if the > BgWorkerStart_ConsistentState_HotStandby comes next after > BgWorkerStart_ConsistentState, which it refers to > > For example:: > 1st.BgWorkerStart_PostmasterStart > 2nd.BgWorkerStart_ConsistentState > 3rd.BgWorkerStart_ConsistentState_HotStandby > 4th.BgWorkerStart_RecoveryFinished > > ====== > doc/src/sgml/config.sgml > > 2. > <varname>enable_syncslot</varname> = true > > Not sure, but I thought the "= true" part should be formatted too. > > SUGGESTION > <literal>enable_syncslot = true</literal> > > ====== > doc/src/sgml/logicaldecoding.sgml > > 3. > + <para> > + A logical replication slot on the primary can be synchronized to the hot > + standby by enabling the failover option during slot creation and setting > + <varname>enable_syncslot</varname> on the standby. For the > synchronization > + to work, it is mandatory to have a physical replication slot between the > + primary and the standby. It's highly recommended that the said physical > + replication slot is listed in <varname>standby_slot_names</varname> on > + the primary to prevent the subscriber from consuming changes faster than > + the hot standby. Additionally, <varname>hot_standby_feedback</varname> > + must be enabled on the standby for the slots synchronization to work. > + </para> > > I felt those parts that describe the mandatory GUCs should be kept together. > > SUGGESTION > For the synchronization to work, it is mandatory to have a physical > replication slot between the primary and the standby, and > <varname>hot_standby_feedback</varname> must be enabled on the > standby. > > It's also highly recommended that the said physical replication slot > is named in <varname>standby_slot_names</varname> list on the primary, > to prevent the subscriber from consuming changes faster than the hot > standby. > > ~~~ > > 4. (Chapter 49) > > By enabling synchronization of slots, logical replication can be > resumed after failover depending upon the > pg_replication_slots.sync_state for the synchronized slots on the > standby at the time of failover. Only slots that were in ready > sync_state ('r') on the standby before failover can be used for > logical replication after failover. However, the slots which were in > initiated sync_state ('i') and not sync-ready ('r') at the time of > failover will be dropped and logical replication for such slots can > not be resumed after failover. This applies to the case where a > logical subscription is disabled before failover and is enabled after > failover. If the synchronized slot due to disabled subscription could > not be made sync-ready ('r') on standby, then the subscription can not > be resumed after failover even when enabled. If the primary is idle, > then the synchronized slots on the standby may take a noticeable time > to reach the ready ('r') sync_state. This can be sped up by calling > the pg_log_standby_snapshot function on the primary. > > ~ > > Somehow, I still felt all that was too wordy/repetitive. Below is my > attempt to make it more concise. Thoughts? > > SUGGESTION > The ability to resume logical replication after failover depends upon > the pg_replication_slots.sync_state value for the synchronized slots > on the standby at the time of failover. Only slots that have attained > a "ready" sync_state ('r') on the standby before failover can be used > for logical replication after failover. Slots that have not yet > reached 'r' state (they are still 'i') will be dropped, therefore > logical replication for those slots cannot be resumed. For example, if > the synchronized slot could not become sync-ready on standby due to a > disabled subscription, then the subscription cannot be resumed after > failover even when it is enabled. > > If the primary is idle, the synchronized slots on the standby may take > a noticeable time to reach the ready ('r') sync_state. This can be > sped up by calling the pg_log_standby_snapshot function on the > primary. > > ====== > doc/src/sgml/system-views.sgml > > 5. > + <para> > + Defines slot synchronization state. This is meaningful on the physical > + standby which has configured <varname>enable_syncslot</varname> = true > + </para> > > As mentioned in the previous review comment ([1]#10) I thought it > might be good to include a hyperlink cross-reference to the > 'enable_syncslot' GUC. > > ~~~ > > 6. > + <para> > + The hot standby can have any of these sync_state for the slots but on a > + hot standby, the slots with state 'r' and 'i' can neither be used for > + logical decoding nor dropped by the user. The primary server will have > + sync_state as 'n' for all the slots. But if the standby is promoted to > + become the new primary server, sync_state can be seen 'r' as well. On > + this new primary server, slots with sync_state as 'r' and 'n' will > + behave the same. > + </para></entry> > > 6a. > /these sync_state for the slots/these sync_state values for the slots/ > > ~ > > 6b > Hm. I still felt (same as previous review [1]#12b) that there seems > too much information here. > > IIUC the sync_state is only meaningful on the standby. Sure, it might > have some values line 'n' or 'r' on the primary also, but those either > mean nothing ('n') or are leftover states from a previous failover > from a standby ('r'), which also means nothing. So can't we just say > it more succinctly like that? > > SUGGESTION > The sync_state has no meaning on the primary server; the primary > sync_state value is default 'n' for all slots but may (if leftover > from a promoted standby) also be 'r'. > > ====== > .../libpqwalreceiver/libpqwalreceiver.c > > 7. > static void > libpqrcv_alter_slot(WalReceiverConn *conn, const char *slotname, > - bool failover) > + bool failover) > > Still seems to be tampering with indentation that should only be in patch > 0001. > > ====== > src/backend/replication/logical/slotsync.c > > 8. wait_for_primary_slot_catchup > > The meaning of the boolean return of this function is still not > described by the function comment. > > ~~~ > > 9. > + * If passed, *wait_attempts_exceeded will be set to true only if this > + * function exits after exhausting its wait attempts. It will be false > + * in all the other cases like failure, remote-slot invalidation, primary > + * could catch up. > > The above already says when a return false happens, so it seems > overkill to give more information. > > SUGGESTION > If passed, *wait_attempts_exceeded will be set to true only if this > function exits due to exhausting its wait attempts. It will be false > in all the other cases. > > ~~~ > > 10. > > +static bool > +wait_for_primary_slot_catchup(WalReceiverConn *wrconn, RemoteSlot > *remote_slot, > + bool *wait_attempts_exceeded) > +{ > +#define WAIT_OUTPUT_COLUMN_COUNT 4 > +#define WORKER_PRIMARY_CATCHUP_WAIT_ATTEMPTS 5 > + > > 10a > Maybe the long constant name is too long. How about > WAIT_PRIMARY_CATCHUP_ATTEMPTS? > > ~~~ > > 10b. > IMO it is better to Assert the input value of this kind of side-effect > return parameter, to give a better understanding and to prevent future > accidents. > > SUGGESTION > Assert(wait_attempts_exceeded == NULL |} *wait_attempts_exceeded == false); > > ~~~ > > 11. synchronize_one_slot > > + ReplicationSlot *s; > + ReplicationSlot *slot; > + char sync_state = '\0'; > > 11a. > I don't think you need both 's' and 'slot' ReplicationSlot -- it looks > a bit odd. Can't you just reuse the one 'slot' variable? > > ~ > > 11b. > Also, maybe those assignment like > + slot = MyReplicationSlot; > > can have an explanatory comment like: > /* For convenience, we assign MyReplicationSlot to a shorter variable name. */ > I have changed it to slightly simpler one, if that is okay? > ~~~ > > 12. > +static void > +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot, > + bool *slot_updated) > +{ > + ReplicationSlot *s; > + ReplicationSlot *slot; > + char sync_state = '\0'; > > In my previous review [1]#33a I thought it was strange to assign the > sync_state (which is essentially an enum) to some meaningless value, > so I suggested it should be set to SYNCSLOT_STATE_NONE in the > declaration. The reply [2] was "No, that will change the flow. It > should stay uninitialized if the slot is not found." > > But I am not convinced there is any flow problem. Also, > SYNCSLOT_STATE_NONE seems the naturally correct default for something > with no state. It cannot be found and be SYNCSLOT_STATE_NONE at the > same time (that is reported as an ERROR "skipping sync of slot") so I > see no problem. > > The CURRENT code is like this: > > /* Slot created by the slot sync worker exists, sync it */ > if (sync_state) > { > Assert(sync_state == SYNCSLOT_STATE_READY || sync_state == > SYNCSLOT_STATE_INITIATED); > ... > } > /* Otherwise create the slot first. */ > else > { > ... > } > > AFAICT that could easily be changed to like below, with no change to > the logic, and it avoids setting strange values. > > SUGGESTION. > > if (sync_state == SYNCSLOT_STATE_NONE) > { > /* Slot not found. Create it. */ > .. > } > else > { > Assert(sync_state == SYNCSLOT_STATE_READY || sync_state == > SYNCSLOT_STATE_INITIATED); > ... > } > I have restructured the entire code here and thus initialization of sync_state is no longer needed. Please review now and let me know. > ~~~ > > 13. synchronize_one_slot > > +static void > +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot, > + bool *slot_updated) > > This *slot_updated parameter looks dubious. It is used in a loop from > the caller to mean that ANY slot was updated -- e.g. maybe it is true > or false on entry to this function. > > But, Instead of having some dependency between this function and the > caller, IMO it makes more sense if we would make this just a boolean > function in the first place (e.g. was updated? T/F) > > Then the caller can also be written more easily like: > > some_slot_updated |= synchronize_one_slot(wrconn, remote_slot); > > ~~~ > > 14. > + /* Search for the named slot */ > + if ((s = SearchNamedReplicationSlot(remote_slot->name, true))) > + { > + SpinLockAcquire(&s->mutex); > + sync_state = s->data.sync_state; > + SpinLockRelease(&s->mutex); > + > + /* User created slot with the same name exists, raise ERROR. */ > + if (sync_state == SYNCSLOT_STATE_NONE) > + { > + ereport(ERROR, > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("skipping sync of slot \"%s\" as it is a user created" > + " slot", remote_slot->name), > + errdetail("This slot has failover enabled on the primary and" > + " thus is sync candidate but user created slot with" > + " the same name already exists on the standby"))); > + } > + } > > > Extra curly brackets around the ereport are not needed. > > ~~~ > > 15. > + /* > + * Sanity check: With hot_standby_feedback enabled and > + * invalidations handled apropriately as above, this should never > + * happen. > + */ > + if (remote_slot->restart_lsn < slot->data.restart_lsn) > + { > + elog(ERROR, > + "not synchronizing local slot \"%s\" LSN(%X/%X)" > + " to remote slot's LSN(%X/%X) as synchronization " > + " would move it backwards", remote_slot->name, > + LSN_FORMAT_ARGS(slot->data.restart_lsn), > + LSN_FORMAT_ARGS(remote_slot->restart_lsn)); > + } > > 15a. > /apropriately/appropriately/ > > ~ > > 15b. > Extra curly brackets around the elog are not needed. > > ~~~ > > 16. synchronize_slots > > +static bool > +synchronize_slots(WalReceiverConn *wrconn) > +{ > +#define SLOTSYNC_COLUMN_COUNT 9 > + Oid slotRow[SLOTSYNC_COLUMN_COUNT] = {TEXTOID, TEXTOID, LSNOID, > + LSNOID, XIDOID, BOOLOID, BOOLOID, TEXTOID, INT2OID}; > + > + WalRcvExecResult *res; > + TupleTableSlot *tupslot; > + StringInfoData s; > + List *remote_slot_list = NIL; > + MemoryContext oldctx = CurrentMemoryContext; > + ListCell *lc; > + bool slot_updated = false; > > Suggest renaming 'slot_updated' to 'some_slot_updated' or > 'update_occurred' etc because the current name makes it look like it > applies to a single slot, but it doesn't. > > ~~~ > > 17. > + SpinLockAcquire(&WalRcv->mutex); > + if (!WalRcv || > + (WalRcv->slotname[0] == '\0') || > + XLogRecPtrIsInvalid(WalRcv->latestWalEnd)) > + { > + SpinLockRelease(&WalRcv->mutex); > + return slot_updated; > + } > + SpinLockRelease(&WalRcv->mutex); > > IMO "return false;" here is more clear than saying "return slot_updated;" > > ~~~ > > 18. > + appendStringInfo(&s, > + "SELECT slot_name, plugin, confirmed_flush_lsn," > + " restart_lsn, catalog_xmin, two_phase, failover," > + " database, pg_get_slot_invalidation_cause(slot_name)" > + " FROM pg_catalog.pg_replication_slots" > + " WHERE failover and sync_state != 'i'"); > > 18a. > /and/AND/ > > ~ > > 18b. > In the reply post (see [2]#32) Shveta said "I could not find quote_* > function for a character just like we have 'quote_literal_cstr' for > string". If you still want to use constant substitution instead of > just hardwired 'i' then why do even you need a quote_* function? I > thought the appendStringInfo uses a printf style format-string > internally, so I assumed it is possible to substitute the state char > directly using '%c'. > Since we have removed cascading standby support, this condition (sync_state != 'i') is no longer needed in the query. > ~~~ > > 19. > + > + > + > + /* We are done, free remote_slot_list elements */ > + list_free_deep(remote_slot_list); > + > + walrcv_clear_result(res); > + > + return slot_updated; > +} > > Excessive blank lines. > > ~~~ > > 20. validate_primary_slot > > + appendStringInfo(&cmd, > + "SELECT count(*) = 1 from pg_replication_slots " > + "WHERE slot_type='physical' and slot_name=%s", > + quote_literal_cstr(PrimarySlotName)); > > > /and/AND/ > > ~~~ > > 21. > + slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple); > + tuple_ok = tuplestore_gettupleslot(res->tuplestore, true, false, slot); > + Assert(tuple_ok); /* It must return one tuple */ > > IMO it's better to use all the var names the same across all > functions? So call this 'tupslot' like the other > MakeSingleTupleTableSlot result. > > ~~~ > > 22. validate_slotsync_parameters > > +/* > + * Checks if GUCs are set appropriately before starting slot sync worker > + * > + * The slot sync worker can not start if 'enable_syncslot' is off and > + * since 'enable_syncslot' is ON, check that the other GUC settings > + * (primary_slot_name, hot_standby_feedback, wal_level, primary_conninfo) > + * are compatible with slot synchronization. If not, raise ERROR. > + */ > +static void > +validate_slotsync_parameters(char **dbname) > +{ > > 22a. > The comment is quite verbose. IMO the 2nd para seems just unnecessary > detail of the 1st para. > > SUGGESTION > Check that all necessary GUCs for slot synchronization are set > appropriately. If not, raise an ERROR. > > ~~~ > > 22b. > IMO (and given what was said in the comment about enable_syncslot must > be on)the first statement of this function should be: > > /* Sanity check. */ > Assert(enable_syncslot); > > ~~~ > > 23. slotsync_reread_config > > + old_dbname = walrcv_get_dbname_from_conninfo(PrimaryConnInfo); > + Assert(old_dbname); > > (This is same comment as old review [1]#61) > > Hmm. I still don't see why this extraction of the dbname cannot be > deferred until later when you know the PrimaryConnInfo has changed, > otherwise, it might be redundant to do this. Shveta replied [2] that > "Once PrimaryConnInfo is changed, we can not get old-dbname.", but I'm > not so sure. Isn't this walrcv_get_dbname_from_conninfo just doing a > string search -- Why can't you defer this until you know > conninfoChanged is true, and then to get the old_dbname, you can just > pass the old_primary_conninfo. E.g. call like > walrcv_get_dbname_from_conninfo(old_primary_conninfo); Maybe I am > mistaken. > Sorry missed your point earlier that we can use old_primary_conninfo to extract dbname later. I have removed this re-validation now as we will restart the worker in case of a parameter change similar to the case of logical apply worker. So these changes are no longer needed. > ~~ > > 24. > + /* > + * Since we have initialized this worker with the old dbname, thus > + * exit if dbname changed. Let it get restarted and connect to the new > + * dbname specified. > + */ > + if (conninfoChanged && strcmp(old_dbname, new_dbname) != 0) > + ereport(ERROR, > + errmsg("exiting slot sync worker as dbname in " > + "primary_conninfo changed")); > > IIUC when the tablesync has to restart, it emits a LOG message before > it exits; but it's not an ERROR. So, shouldn't this be similar -- IMO > it is not an "error" for the user to wish to change the dbname. Maybe > this should be LOG followed by an explicit exit. If you agree, then it > might be better to encapsulate such logic in some little function: > > // pseudo-code > void slotsync_worker_restart(const char *msg) > { > ereport(LOG, msg... > exit(0); > } > we can not do proc_exit(0), as then postmaster will not restart it on clean-exit. I agree with your logic, but will have another argument in this function to accept 'exit code' from the caller. > ~~~ > > 25. ReplSlotSyncWorkerMain > > + for (;;) > + { > + int rc; > + long naptime = WORKER_DEFAULT_NAPTIME_MS; > + TimestampTz now; > + bool slot_updated; > + > + ProcessSlotSyncInterrupts(wrconn); > + > + slot_updated = synchronize_slots(wrconn); > > Here I think the 'slot_updated' should be renamed to the same name as > in #16 above (e.g. 'some_slot_updated' or 'any_slot_updated' or > 'update_occurred' etc). > > ~~~ > > 26. SlotSyncWorkerRegister > > + if (!enable_syncslot) > + { > + ereport(LOG, > + errmsg("skipping slots synchronization because enable_syncslot is " > + "disabled.")); > + return; > + } > > Instead of saying "because..." in the error message maybe keep the > message more terse and describe the "because" part in the errdetail > > SUGGESTION > errmsg("skipping slot synchronization") > errdetail("enable_syncslot is disabled.") > > > ====== > src/backend/replication/slot.c > > 27. > + * sync_state: Defines slot synchronization state. For user created slots, it > + * is SYNCSLOT_STATE_NONE and for the slots being synchronized on the > physical > + * standby, it is either SYNCSLOT_STATE_INITIATED or SYNCSLOT_STATE_READY > */ > void > ReplicationSlotCreate(const char *name, bool db_specific, > ReplicationSlotPersistency persistency, > - bool two_phase, bool failover) > + bool two_phase, bool failover, char sync_state) > > > 27a. > Why is this comment even mentioning SYNCSLOT_STATE_READY? IIUC it > doesn't make sense to ever call ReplicationSlotCreate directly setting > the 'r' state (e.g., bypassing 'i' ???) > > ~ > > 27b. > Indeed, IMO there should be Assert(sync_state == SYNCSLOT_STATE_NONE > || syncstate == SYNCSLOT_STATE_INITIATED); to guarantee this. > > ====== > src/include/replication/slot.h > > 28. > + /* > + * Is this a slot created by a sync-slot worker? > + * > + * Only relevant for logical slots on the physical standby. > + */ > + char sync_state; > + > > (probably I am repeating a previous thought here) > > The comment says the field is only relevant for standby, and that's > how I've been visualizing it, and why I had previously suggested even > renaming it to 'standby_sync_state'. However, replies are saying that > after failover these sync_states also have "some meaning for the > primary server". > > That's the part I have trouble understanding. IIUC the server states > are just either all 'n' (means nothing) or 'r' because they are just > leftover from the old standby state. So, does it *truly* have meaning > for the server? Or should those states somehow be removed/ignored on > the new primary? Anyway, the point is that if this field does have > meaning also on the primary (I doubt) then those details should be in > this comment. Otherwise "Only relevant ... on the standby" is too > misleading. > I have modified it currently, but I will give another thought on your suggestions here (and in earlier emails) and will let you know. > ====== > .../t/050_standby_failover_slots_sync.pl > We are working on CFbot failure fixes in this file and restructing the tests here. Thus I am keeping these test comments on hold and will address in next version. > 29. > +# Create table and publication on primary > +$primary->safe_psql('postgres', "CREATE TABLE tab_mypub3 (a int > PRIMARY KEY);"); > +$primary->safe_psql('postgres', "CREATE PUBLICATION mypub3 FOR TABLE > tab_mypub3;"); > + > > 29a. > /on primary/on the primary/ > > ~ > > 29b. > Consider to combine those DDL > > ~ > > 29c. > Perhaps for consistency, you should be calling this 'regress_mypub3'. > > ~~~ > > 30. > +# Create a subscriber node > +my $subscriber3 = PostgreSQL::Test::Cluster->new('subscriber3'); > +$subscriber3->init(allows_streaming => 'logical'); > +$subscriber3->start; > +$subscriber3->safe_psql('postgres', "CREATE TABLE tab_mypub3 (a int > PRIMARY KEY);"); > + > +# Create a subscription with failover = true & wait for sync to complete. > +$subscriber3->safe_psql('postgres', > + "CREATE SUBSCRIPTION mysub3 CONNECTION '$publisher_connstr' " > + . "PUBLICATION mypub3 WITH (slot_name = lsub3_slot, failover = true);"); > +$subscriber3->wait_for_subscription_sync; > > 30a > Consider combining those DDLs. > > ~ > > 30b. > Probably for consistency, you should be calling this 'regress_mysub3'. > > ~~~ > > 31. > +# Advance lsn on the primary > +$primary->safe_psql('postgres', > + "SELECT pg_log_standby_snapshot();"); > +$primary->safe_psql('postgres', > + "SELECT pg_log_standby_snapshot();"); > +$primary->safe_psql('postgres', > + "SELECT pg_log_standby_snapshot();"); > + > > Consider combining all those DDLs. > > ~~~ > > 32. > +# Truncate table on primary > +$primary->safe_psql('postgres', > + "TRUNCATE TABLE tab_mypub3;"); > + > +# Insert data on the primary > +$primary->safe_psql('postgres', > + "INSERT INTO tab_mypub3 SELECT generate_series(1, 10);"); > + > > Consider combining those DDLs. > > ~~~ > > 33. > +# Confirm that restart_lsn of lsub3_slot slot is synced to the standby > +$result = $standby3->safe_psql('postgres', > + qq[SELECT '$primary_lsn' >= restart_lsn from pg_replication_slots > WHERE slot_name = 'lsub3_slot';]); > +is($result, 't', 'restart_lsn of slot lsub3_slot synced to standby'); > > > Does "'$primary_lsn' >= restart_lsn" make sense here? NOTE, the sign > was '<=' in v43-0002 > > ~~~ > > 34. > +# Confirm that confirmed_flush_lsn of lsub3_slot slot is synced to the > standby > +$result = $standby3->safe_psql('postgres', > + qq[SELECT '$primary_lsn' >= confirmed_flush_lsn from > pg_replication_slots WHERE slot_name = 'lsub3_slot';]); > +is($result, 't', 'confirmed_flush_lsn of slot lsub3_slot synced to > the standby'); > > Does "'$primary_lsn' >= confirmed_flush_lsn" make sense here? NOTE, > the sign was '<=' in v43-0002 > > ~~~ > > 35. > +################################################## > +# Test that synchronized slot can neither be docoded nor dropped by the user > +################################################## > > 35a. > /docoded/decoded/ > > ~ > > 35b. > Please give explanation in the comment *why* those ops are not allowed > (e.g. because the hot_standby_feedback GUC does not have an accepted > value) > > ~~~ > > 36. > +################################################## > +# Create another slot which stays in sync_state as initiated ('i') > +################################################## > + > > Please explain the comment as to *why* it gets stuck in the initiated state. > > > ~~~ > > 37. > +################################################## > +# Promote the standby3 to primary. Confirm that: > +# a) the sync-ready('r') slot 'lsub3_slot' is retained on new primary > +# b) the initiated('i') slot 'logical_slot'is dropped on promotion > +# c) logical replication for mysub3 is resumed succesfully after failover > +################################################## > > > /'logical_slot'is/'logical_slot' is/ (missing space) > > /succesfully/successfully/ > > ~~~ > > 38. > +# Update subscription with new primary's connection info > +$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 DISABLE;"); > +$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 > CONNECTION '$standby3_conninfo';"); > +$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 ENABLE;"); > > > Consider combining all those DDLs. > > ~~~ > > 39. > + > +# Insert data on the new primary > +$standby3->safe_psql('postgres', > + "INSERT INTO tab_mypub3 SELECT generate_series(11, 20);"); > + > +# Confirm that data in tab_mypub3 replicated on subscriber > +is( $subscriber3->safe_psql('postgres', q{SELECT count(*) FROM tab_mypub3;}), > + "20", > + 'data replicated from new primary'); > > Shouldn't there be some wait_for_subscription_sync logic (or similar) > here just to ensure the subscriber3 had time to receive that data > before you immediately check that it had arrived? > > ====== > [1] My v43-0002 review. > https://www.postgresql.org/message-id/CAHut%2BPuuqEpDse5msENsVuK3rjTRN-QGS67rRCGVv%2BzcT-f0GA%40mail.gmail.com > [2] Replies to v43-0002 review. > https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com > > Kind Regards, > Peter Smith. > Fujitsu Australia