On Wed, Feb 7, 2024 at 9:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for v79-0001
Thanks for the feedback. Addressed the comments in v80 patch-set.
Please find my response inline for few.
> src/sgml/logicaldecoding.sgml
> 3.
> + <sect2 id="logicaldecoding-replication-slots-synchronization">
> + <title>Replication Slot Synchronization</title>
> + <para>
> + A logical replication slot on the primary can be synchronized to the hot
> + standby by enabling the <literal>failover</literal> option for the slot
> + and calling <function>pg_sync_replication_slots</function>
> + on the standby. The <literal>failover</literal> option of the slot
> + can be enabled either by enabling
> + <link linkend="sql-createsubscription-params-with-failover">
> + <literal>failover</literal></link>
> + option during subscription creation or by providing
> <literal>failover</literal>
> + parameter during
> + <link linkend="pg-create-logical-replication-slot">
> + <function>pg_create_logical_replication_slot</function></link>.
>
> IMO it will be better to slightly reword this (like was suggested for
> the Commit Message). I felt it is also better to refer/link to "CREATE
> SUBSCRIPTION" instead of saying "during subscription creation".
Regarding link to create-sub, the
'sql-createsubscription-params-with-failover' takes you to the
failover property of Create-Subscription page. Won't that suffice?
>
> 8.
> +/*
> + * Register the callback function to clean up the shared memory of slot
> + * synchronization.
> + */
> +void
> +SlotSyncInitialize(void)
> +{
> + before_shmem_exit(SlotSyncShmemExit, 0);
> +}
>
> This is only doing registration for cleanup of shmem stuff. So, does
> it really need it to be a separate function, or can this be registered
> within SlotSyncShmemInit() itself?
I think it makes more sense to call it from BaseInit() where we have
all such calls like InitTemporaryFileAccess(),
ReplicationSlotInitialize() etc which do similar callback
registrations using before_shmem_exit().
Attached the patches for v80. Overall changes are:
--Addressed comments by Peter (which I responded above) and Amit given
in [1] and [2].
--Also improved commit msg and comment around 'wal_level' as suggested
by Bertrand in [3].
[1]: https://www.postgresql.org/message-id/CAHut%2BPvtysbVd8tj2AADk%3DeNo0VY9Ov9wkBP-K%2B9tj1wRS4M4w%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1%2Bar0N1xXnZZ26BG1qO4LHRS8v3wnH9Pnz4BWmk6SDTHw%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/ZcHX4SXkqtGe27a6%40ip-10-97-1-34.eu-west-3.compute.internal
thanks
Shveta