On Tue, Dec 19, 2023 at 6:58 AM Peter Smith <smithpb2...@gmail.com> wrote: > > Here are some comments for the patch v49-0002. >
Thanks for reviewing. I have addressed these in v50. > (This is in addition to my review comments for v48-0002 [1]) > > ====== > src/backend/access/transam/xlogrecovery.c > > > 1. FinishWalRecovery > > + * > + * We do not update the sync_state from READY to NONE here, as any failed > + * update could leave some slots in the 'NONE' state, causing issues during > + * slot sync after restarting the server as a standby. While updating after > + * switching to the new timeline is an option, it does not simplify the > + * handling for both READY and NONE state slots. Therefore, we retain the > + * READY state slots after promotion as they can provide useful information > + * about their origin. > + */ > > Do you know if that wording is correct? e.g., If you were updating > from READY to NONE and there was a failed update, that would leave > some slots still in a READY state, right? So why does the comment say > "could leave some slots in the 'NONE' state"? > yes, it the comment is correct as stated in [1] [1]: https://www.postgresql.org/message-id/CAA4eK1LoJSbFJwa%3D97_5qHNAVfOkmfc40W_SFMVBbm6r0%3DPXHQ%40mail.gmail.com > ====== > src/backend/replication/slot.c > > 2. ReplicationSlotAlter > > + /* > + * Do not allow users to drop the slots which are currently being synced > + * from the primary to the standby. > + */ > + if (RecoveryInProgress() && > + MyReplicationSlot->data.sync_state != SYNCSLOT_STATE_NONE) > + ereport(ERROR, > + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), > + errmsg("cannot alter replication slot \"%s\"", name), > + errdetail("This slot is being synced from the primary server."))); > + > > The comment looks wrong -- should say "Do not allow users to alter..." > > ====== > > 3. > +################################################## > +# Test that synchronized slot can neither be decoded nor dropped by the user > +################################################## > + > > 3a, > /Test that synchronized slot/Test that a synchronized slot/ > > 3b. > Isn't there a missing test? Should this part also check that it cannot > ALTER the replication slot being synced? e.g. test for the new v49 > error message that was added in ReplicationSlotAlter() > > ~~~ > > 4. > +# Disable hot_standby_feedback > +$standby1->safe_psql('postgres', 'ALTER SYSTEM SET > hot_standby_feedback = off;'); > +$standby1->restart; > + > > Can there be a comment added to explain why you are doing the > 'hot_standby_feedback' toggle? > > ~~~ > > 5. > +################################################## > +# Promote the standby1 to primary. Confirm that: > +# a) the sync-ready('r') slot 'lsub1_slot' is retained on the new primary > +# b) the initiated('i') slot 'logical_slot' is dropped on promotion > +# c) logical replication for regress_mysub1 is resumed succesfully > after failover > +################################################## > > /succesfully/successfully/ > > ~~~ > > 6. > + > +# Confirm that data in tab_mypub3 replicated on subscriber > +is( $subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}), > + "$primary_row_count", > + 'data replicated from the new primary'); > > The comment is wrong -- it names a different table ('tab_mypub3' ?) to > what the SQL says. > > ====== > [1] My v48-0002 review comments. > https://www.postgresql.org/message-id/CAHut%2BPsyZQZ1A4XcKw-D%3DvcTg16pN9Dw0PzE8W_X7Yz_bv00rQ%40mail.gmail.com > > Kind Regards, > Peter Smith. > Fujitsu Australia
