On Mon, 17 Feb 2025 at 10:37, Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi. Some review comments for patch v1-0001.
>
> ======
> 1. DOCS?
>
> Shouldn't the documentation [1] for pg_copy_logical_replication_slot()
> and pg_copy_physical_replication_slot() be updated to mention this?
>
> ======
> src/backend/replication/slotfuncs.c
>
Updated the documentation
> 2.
> + /* We should not copy invalidated replication slots */
> + if (src_isinvalidated)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot copy an invalidated replication slot")));
> +
>
> 2a.
> The "we should not" sounds more like a recommendation than an error.
> Comment can just say the same as the as errmsg.
>
Fixed
> ~
>
> 2b.
> ereport does not need all these parentheses
>
Removed extra parentheses
> ~
>
> 2c.
> I felt the errmsg should include the name of the slot.
>
Added the slot name in error message
> ~~~
>
> 2d.
> AFAICT this code will emit the same error regardless of
> logical/physical slot so maybe you need to modify following to cater
> for both kinds of replication_slot:
> - the commit message
Fixed
> - docs
Fixed
> - test code
Currently a physical replication slot can only be invalidated for
"wal_removed". And logical replication slot can be invalidated for
"wal_removed", "rows_removed" , "wal_level_insufficient".
And for copying the slot invalidated for "wal_removed" throws error
"ERROR: cannot copy a replication slot that doesn't reserve WAL".
So, I have added test only for the case of logical replication slot.
>
> ======
> src/test/recovery/t/035_standby_logical_decoding.pl
>
> 3.
> +# Attempting to copy an invalidated slot
> +($result, $stdout, $stderr) = $node_standby->psql(
>
> /Attempting/Attempt/
>
Fixed
> ======
> [1] https://www.postgresql.org/docs/current/functions-admin.html
I have updated the changes in v2 patch [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEVJpb6%2Bhnk4MPVU3hZBYL%3DDS4v-PYBZOUoiivrN8Vd_Bw%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
