Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jun 8, 2026 at 9:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 5 Jun 2026 at 07:59, Peter Smith <smithpb2250(at)gmail(dot)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.

//////

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).

======
Kind Regards,
Peter Smith.
Fujitsu Australia