On Wed, May 20, 2026 at 3:05 PM vignesh C <[email protected]> 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
