Re: Proposal: Conflict log history table for Logical Replication

On Wed, 20 May 2026 at 16:12, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, May 20, 2026 at 3:05 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> >
> > Rest of the comments were fixed.
> > The attached v37 version patch has the changes for the same. Also
> > Peter's comments on the documentation patch from [1] and Shveta's
> > comments from [2] are addressed in the attached patch.
> >
> > [1] - https://www.postgresql.org/message-id/CAHut%2BPsrnU2BB1%2BM3c%2BDr5h62BLYfwBzhTg%3DBM7QtBoPwHYrKw%40mail.gmail.com
> > [2] - https://www.postgresql.org/message-id/CAJpy0uCX53c40xopqmHtWSWBmh78BqhLVGXa88fU42eOi6w%2BLQ%40mail.gmail.com
> >
>
> I have not yet looked at v37. But here are a few comments on v36-005,
> 006. I have merged them and reviewed together.
>
> 1)
> +#include "utils/fmgroids.h"
> +#include "utils/json.h"
>
> conflict.c compiles without above inclusions.
>
> 2)
> + bool log_dest_clt = false;
> + bool log_dest_logfile;
>
> A better and more clear name would be log_dest_table instead of
> log_dest_clt here.
>
> 3)
> @@ -6069,6 +6049,8 @@ DisableSubscriptionAndExit(void)
> */
> pgstat_report_subscription_error(MyLogicalRepWorker->subid);
>
> + ProcessPendingConflictLogTuple();
>
> It does not look obvious as in why we are trying to process
> conflict-tuple during disable-subscription? A comment will help here.
>
>
> 4)
> tuple_table_slot_to_indextup_json():
>
> + indexDesc = index_open(indexid, NoLock);
> +
> + build_index_datums_from_slot(estate, localrel, slot, indexDesc, values,
> + isnull);
> + tupdesc = RelationGetDescr(indexDesc);
> +
> + /* Bless the tupdesc so it can be looked up by row_to_json. */
> + BlessTupleDesc(tupdesc);
>
> We get the index's relcache pointer and pass it directly to
> BlessTupleDesc which internally changes it by assigning tdtypmod. Is
> this intentional i.e. do we want to change the relcache entry of index
> directly? Shouldn't we copy it (CreateTupleDescCopy) and then Bless
> it?
>
> 5)
> build_conflict_tupledesc() does 'CreateTemplateTupleDesc' and Bless it
> each time the conflict is raised. Since the tuple-descriptor here is
> not going to change, IMO, it will be better to create and bless it
> once and reuse it everytime. We can have a 'static' TupleDesc here.
> Thoughts?

Thanks for the comments, these comments are addressed in the v38
version attached.
Apart from this, the comments from [1], [2], [3], [4], [5], [6], [7],
and [8] are also addressed.

[1] - https://www.postgresql.org/message-id/CAJpy0uC43NTKheuLo%2BMsHG7Sfh-QWQM9QP-EVPL5LChiPfisJw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CANhcyEU8qr9%2BPMU2Kn0qqZakVptVvRsbRu3Ee2Q40YX9aivXww%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAJpy0uB19XxfF2Yj1w%3DC90iVBLMHb%3DDMBZ1h3rqzJhEbTSwtag%40mail.gmail.com
[4] - https://www.postgresql.org/message-id/CAHut%2BPvSaJAYwNUS9GnO6MCTfuPpVLdU1r8cZBf6gjGjvnbWpQ%40mail.gmail.com
[5] - https://www.postgresql.org/message-id/CAHut%2BPtUWTnUD8QpfmNpU8iU6Pg%2BE29nDALYAfMUudad8oYezw%40mail.gmail.com
[6] - https://www.postgresql.org/message-id/CAHut%2BPvW%3DFd-OSM6oe-9D3ycAG0qLfGEnaT%3DBUB%2BPMeUFeEAyQ%40mail.gmail.com
[7] - https://www.postgresql.org/message-id/CAHut%2BPu4ErbjstY86kWbKOepHn623Zp9MNiKW4DoMG3iVdG2fA%40mail.gmail.com
[8] - https://www.postgresql.org/message-id/CANhcyEUGoaSpJKDJaQfrQR6%2B-4%2B_PgQ%3D0DmZZztPAEheMkMw7w%40mail.gmail.com

Regards,
Vignesh