Re: Synchronizing slots from primary to standby

On Tue, Dec 19, 2023 at 6:58 AM Peter Smith <smithpb2250(at)gmail(dot)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