Thread

Re: Proposal: Conflict log history table for Logical Replication

shveta malik <shveta.malik@gmail.com> — 2026-05-20T10:42:02Z

On Wed, May 20, 2026 at 3:05 PM vignesh C <vignesh21@gmail.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
Shveta