On Wed, Oct 22, 2025 at 10:53 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Here are a few review comments for patch 0001:
>
> ======
> src/backend/catalog/pg_subscription.c
>
> GetSubscriptionRelations:
>
> 1.
> List *
> -GetSubscriptionRelations(Oid subid, bool not_ready)
> +GetSubscriptionRelations(Oid subid, bool otherwise return all the
> relations of the subscription, bool get_sequences,
> + bool not_ready)
>
> Now the function parameter means the (unchanged) function comment is
> not correct anymore.
>
> e.g. It still says "otherwise return all the relations of the
> subscription", but that does not account for the parameters indicating
> if you only want tables, or only want sequences.
>
I think the code for those parameters is quite clear. The previous
version had few comments but I found those unnecessary.
> src/backend/replication/logical/syncutils.c
>
> 10.
> bool
> FetchRelationStates(bool *started_tx)
>
> IIUC, you are using the terminology "relations" to apply to both
> tables or sequences; OTOH ", tables" is just tables. It seems this
> function is table-specific. Should it have a name change like
> FetchTableStates?
>
The patch 0002 has some changes related to sequences in this function.
So, I think the current naming is okay.
> ======
> src/test/subscription/t/036_sequences.pl
>
> 11.
> There are no test cases checking that ALTER commands will correctly
> add/remove sequences in the pg_subscription_rel?
>
The patch 0002 has tests related to ALTER commands. I don't think 0001
needs tests for all kinds of statements as both will be committed a
few days apart.
--
With Regards,
Amit Kapila.
