Re: Proposal: Conflict log history table for Logical Replication

On Thu, Jul 2, 2026 at 1:39 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> I rebased the remaining patches on top of HEAD. So far, I have run
> pgindent and completed the doc merge for 0001. The 0002 and 0003 are
> just rebased.
>

Thanks, here are a few comments on the 001 patch:
1) conflict.h
#include "access/xlogdefs.h"
+#include "access/genam.h"
#include "datatype/timestamp.h"

We only need "Relation" here. Would it make sense to include
"relcache.h" instead to keep the include dependency chain narrower?

2) worker_internal.h
The comment above the new PARALLEL_TRANS_ERROR enum currently says:
/*
* The enum values must have the same order as the transaction state
* transitions.
*/

PARALLEL_TRANS_ERROR does not strictly fit the transition order since
it does not necessarily come after PARALLEL_TRANS_FINISHED.

Suggestion: add something like below to avoid any confusion:
"PARALLEL_TRANS_FINISHED and PARALLEL_TRANS_ERROR are mutually
exclusive terminal states."

3) 035_conflicts.pl
For the multiple_unique_conflicts tests, we are currently only
verifying that one key exists out of the three, and we are also not
checking whether all three conflicting rows are present in CLT.
How about using a query like below to verify all three keys instead to
provide stronger validation?

"SELECT (unnest(local_conflicts)->'tuple'->>'a')::int
FROM $clt WHERE conflict_type = 'multiple_unique_conflicts' ORDER BY 1;"

Attached a 001 top-up patch with the above suggestions applied. Please
consider them if you agree.

--
Thanks,
Nisha