Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Fri, Jan 9, 2026 at 4:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jan 8, 2026 at 12:30 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Wed, Jan 7, 2026 at 12:12 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > Hi Dilip,
> > > Please find a few comments on v19-001:
> > >
> >
> > Please find a few comments for v19-002's part I have reviewed so far:
> >
> > 1)
> > ReportApplyConflict:
> > + if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> > + if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
> >
> > We can use IsSet everywhere
>
> IMHO in subscriptioncmd.c we very frequently use the bitwise operation
> so it has IsSet declared, I am not really sure do we want to define
> IsSet in this function as we are using it just 2 places.
>
> > 2)
> > GetConflictLogTableInfo
> > This function gets dest and opens table, shall we rename to :
> > GetConflictLogDestAndTable
>
> Yeah make sense.
>
> > 3)
> > GetConflictLogTableInfo:
> > + *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;
> > +
> > + Assert(OidIsValid(conflictlogrelid));
> >
> > We don't need to fetch conflictlogrelid until after 'if (*log_dest ==
> > CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check.
>
> Done
>
> > 4)
> > GetConflictLogTableInfo:
> > + /* Conflict log table is dropped or not accessible. */
> > + if (conflictlogrel == NULL)
> > + ereport(WARNING,
> > + (errcode(ERRCODE_UNDEFINED_TABLE),
> > + errmsg("conflict log table with OID %u does not exist",
> > + conflictlogrelid)));
> >
> > Shall we replace it with elog(ERROR)? IMO, it should never happen and
> > if it happens, we should raise it as an internal error as we do for
> > various other cases.
>
> Right, now its internal table so we should make it elog(ERROR)
>
> >
> > 5)
> > ReportApplyConflict():
> >
> > Currently the function structure is:
> >
> > /* Insert to table if destination is 'table' or 'all' */
> > if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> >
> >   /* Decide what detail to show in server logs. */
> > if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
> > else <table-only: put reduced info in log>
> >
> >
> > It will be good to make it:
> >
> > /*
> >  * Insert to table if destination is 'table' or 'all' and
> >  * also log the error msg to serverlog
> >  */
> > if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
> > {...}
> > else <CONFLICT_LOG_DEST_LOG case>
> > {log complete detail}
>
> I did not understand this, I mean we can not put the "log complete
> detail" case in the else part, because we might have to log the
> complete detail along with the table if the destination is "all", are
> you suggesting something else?

Okay, I see it now. I missed the 'all' part earlier. Thanks for
pointing it out. The current implementation is good.

>
> >
> > 6)
> > tuple_table_slot_to_indextup_json:
> > + tuple = heap_form_tuple(tupdesc, values, isnull);
> >
> > Do we need to do: heap_freetuple at the end?
>
> Yeah better we do that, instead of assuming short lived memory context.
>
>
> --
> Regards,
> Dilip Kumar
> Google