Re: Restrict copying of invalidated replication slots

On Sun, 23 Feb 2025 at 06:46, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for patch v2-0001.
>
> ======
> Commit message
>
> 1.
> Currently we can copy an invalidated logical and physical replication slot
> using functions 'pg_copy_logical_replication_slot' and
> 'pg_copy_physical_replication_slot' respectively.
> With this patch we will throw an error in such cases.
>
> /we can copy an invalidated logical and physical replication slot/we
> can copy invalidated logical and physical replication slots/
>
Updated the commit message

> ======
> doc/src/sgml/func.sgml
>
> pg_copy_physical_replication_slot:
>
> 2.
> - is omitted, the same value as the
> source slot is used.
> + is omitted, the same value as the source slot is used. Copy of an
> + invalidated physical replication slot in not allowed.
>
> Typo /in/is/
>
> Also, IMO you don't need to say "physical replication slot" because it
> is clear from the function's name.
>
> SUGGESTION
> Copy of an invalidated slot is not allowed.
>
Fixed

> ~~~
>
> pg_copy_logical_replication_slot:
>
> 3.
> + Copy of an invalidated logical replication slot in not allowed.
>
> Typo /in/is/
>
> Also, IMO you don't need to say "logical replication slot" because it
> is clear from the function's name.
>
> SUGGESTION
> Copy of an invalidated slot is not allowed.
>
>
Fixed

> ======
> src/backend/replication/slotfuncs.c
>
> copy_replication_slot:
>
> 4.
> + /* Check if source slot was invalidated while copying of slot */
> + SpinLockAcquire(&src->mutex);
> + first_slot_contents = *src;
> + SpinLockRelease(&src->mutex);
> +
> + src_isinvalidated = (first_slot_contents.data.invalidated != RS_INVAL_NONE);
> +
> + if (src_isinvalidated)
> + ereport(ERROR,
> + (errmsg("could not copy replication slot \"%s\"",
> + NameStr(*src_name)),
> + errdetail("The source replication slot was invalidated during the
> copy operation.")));
>
> 4a.
> We already know that it was not invalid the FIRST time we looked at
> it, so IMO we only need to confirm that the SECOND look gives the same
> answer. IOW, I thought the code should be like below. (AFAICT
> Sawada-san's review says the same as this).
>
> Also, I think it is better to say "became invalidated" instead of "was
> invalidated", to imply the state was known to be ok before the copy.
>
> SUGGESTION
>
> + /* Check if source slot became invalidated during the copy operation */
> + if (second_slot_contents.data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
>
> ~
>
> 4b.
> Unnecessary parentheses in the ereport.
>
> ~
>
> 4c.
> There seems some weird mix of tense "cannot copy" versus "could not
> copy" already in this file. But, maybe at least you can be consistent
> within the patch and always say "cannot".
>
Fixed.

I have addressed the above comments in v5 patch [1].

[1]: https://www.postgresql.org/message-id/CANhcyEUHp6cRfaKf0ZqHCppCqpqzmsf5swpbnYGyRU%2BN%2Bihi%3DQ%40mail.gmail.com

Thanks and Regards,
Shlok Kyal