On Tue, 16 Jun 2026 at 05:19, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Jun 8, 2026 at 9:39 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, 5 Jun 2026 at 07:59, Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > Hi Vignesh.
> > >
> > > Some review comments for the patch v45-0004.
> > >
> > > 4c.
> > > IMO there should be a separate function for handling the subscription
> > > footer/s, same as there is already a function
> > > addFooterToPublicationDesc.
> >
> > It is not required in this case as we don't have multiple footers from
> > different places to be added here.
> >
>
> Sure, it's not "required", but I think:
> A) Separating the footer code from the non-footer code makes it easier to read
> B) The 'describeSubscriptions' function is too long. This would make
> it 20 lines shorter.
> C) Consistent footer handling for pub/sub describes.
Ok, Let's keep it consistent
> //////
>
> More review comments for v50-0005
>
> ======
> src/bin/psql/describe.c
>
> 1.
> + /* Conflict log destination is supported in v19 and higher */
> + if (pset.sversion >= 190000)
>
> The CLT is targeting PG20, right? So, that comment ought to say "is
> supported in v20 and higher".
>
> Ideally, there should be some "TODO" reminder comments here to ensure
> the appropriate 190000's get replaced by 200000 as soon as the version
> number is bumped. Better to flag/comment all those places now, so that
> nothing gets missed later.
>
> (A similar review comment probably applies also to the pg_dump changes
> in the previous v50-0004 patch).
I felt it is better to mention it in the commit message of the patch
instead of mentioning it in the code.
The attached v51 version patch has the changes for the same.
The patch also addresses the comment from [1].
[1] - https://www.postgresql.org/message-id/CANhcyEUjc9TCcW1YAQVMTs6-huWBZoy%2BsVkz5C8b72os5p-f%2Bg%40mail.gmail.com
Regards,
Vignesh
