Re: Proposal: Conflict log history table for Logical Replication

On Fri, Jan 9, 2026 at 8:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Dilip.
>
> Here are some review comments for the v19-0002 (code only, not tests).
>
> ======
> src/backend/replication/logical/conflict.c
>
> 1.
> +/* Schema for the elements within the 'local_conflicts' JSON array */
> +static const ConflictLogColumnDef LocalConflictSchema[] =
> +{
> + { .attname = "xid",       .atttypid = XIDOID },
> + { .attname = "commit_ts", .atttypid = TIMESTAMPTZOID },
> + { .attname = "origin",    .atttypid = TEXTOID },
> + { .attname = "key",       .atttypid = JSONOID },
> + { .attname = "tuple",     .atttypid = JSONOID }
> +};
>
> Maybe this only needs to be used in this module, but I still thought
> LocalConflictSchema[] (and the MAX_LOCAL_CONFLICT_INFO_ATTRS) belongs
> more naturally alongside the other ConflictLogSchema[] (e.g. in
> conflict.h)

Since this is used only in conflict.c, my personal preference to keep
it in the .c file

> ~~~
>
> 2.
> +
> +#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
> + (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))
> +
>
> how about:
>
> #define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)

Make sense
> ~~~
>
> ReportApplyConflict:
>
> 3.
> + /* Insert to table if destination is 'table' or 'all' */
> + if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
>
> I think it is unnecessary to even mention 'all'. The bitmasks tell
> everything you need to know.
>
> SUGGESTION
> Insert to table if requested.

Done

> ~
>
> 4.
> There's a slightly convoluted if;if/else with these bitmasks here, but
> I think Shveta already suggested the same change as what I was
> thinking.

So now we have this, which is first handling the table case and
handling the LOG case separately, because irrespective of whether we
want to insert into the table or not we need to check what we need to
log?  What's your suggestion for this?

if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
{
 <Insert into table>
}

if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
{
<write full log>
}
else
{
<write minimal log>
}



> ~~~
>
> GetConflictLogTableInfo:
>
> 5.
> + *log_dest = GetLogDestination(MySubscription->conflictlogdest);
> + conflictlogrelid = MySubscription->conflictlogrelid;
> +
> + /* If destination is 'log' only, no table to open. */
> + if (*log_dest == CONFLICT_LOG_DEST_LOG)
> + return NULL;
>
> I think checking and mentioning 'log' here is not future-proof for
> when there might be more kinds of destinations. We just want to return
> when the user doesn't want a 'table'. Use the bitmasks to do that.
>
> SUGGESTION
> /* Quick exit if a conflict log table was not requested. */
> if ((*logdest & CONFLICT_LOG_DEST_TABLE) == 0)
>   return NULL;

Make sense

> ~~~
>
> build_conflict_tupledesc:
>
> 6.
> +static TupleDesc
> +build_conflict_tupledesc(void)
> +{
>
> Missing function comment. There used to be one (??).

Yeah it got lost in some of the refactoring.

> ~~~
>
> build_local_conflicts_json_array:
>
> 7.
> + json_datum_array = (Datum *) palloc(num_conflicts * sizeof(Datum));
> + json_null_array = (bool *) palloc0(num_conflicts * sizeof(bool));
>
> I may have already asked this same question in a previous review, but
> shouldn't these be using those new palloc_array and palloc0_array
> macros?

Done.

> ======
> src/include/replication/conflict.h
>
> 8.
> -/* Define the count using the array size */
>  #define MAX_CONFLICT_ATTR_NUM (sizeof(ConflictLogSchema) /
> sizeof(ConflictLogSchema[0]))
>
> This change appears to be in the wrong patch. AFAICT it belonged in 0001.

Fixed.

Will shared the updated patch after fixing other comments from Shveta
as well, thanks for the review.


--
Regards,
Dilip Kumar
Google