On Tue, Dec 19, 2023 at 6:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some comments for the patch v49-0002.
>
> (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"?
>
The comment is correct because after restart the server will start as
'standby', so 'READY' marked slots are okay but the slots that we
changed to 'NONE' would now appear as user-created slots which would
be wrong.
--
With Regards,
Amit Kapila.
