Re: Proposal: Conflict log history table for Logical Replication

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.

> ======
> src/test/regress/expected/subscription.out
>
> Some general comments about the describe testing:
>
> 5.
> AFAICT there this patch is not being tested properly because there are
> no tests where the conflict_log_destination is "table" or "all".

That is not done intentionally as the description of subscriptions for
conflict log table oid will change and tests can fail. Instead it is
verified using select after create subscription

> ~~~
>
> 6.
> Even though the \dRs code was changed a lot, it seems there were no
> impacted tests because they were all using \dRs+ and never \dRs. So I
> think there needs to be more "describe" tests that are using *both*
> forms of this command.

There are tests for \dRs, since the existing format of \dRs was not
changed, except need not be updated.

> Also consider using expanded form \x for some of the \dRs and \dRs+
> tests, so that the expected output is easier to read.

Since no new tests were added, nothing done for this.

Rest of the comments were fixed. These were handled as part of the v47
version posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm2V3EuSKMaTqDvaiLQW3jwBX90aXTkMST1ft%3DuJ8J%2BR5A%40mail.gmail.com

Regards,
Vignesh