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

On Mon, Dec 8, 2025 at 9:12 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, 6 Dec 2025 at 20:36, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, Dec 5, 2025 at 10:39 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, Dec 4, 2025 at 8:05 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Wed, 3 Dec 2025 at 16:57, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > On Wed, Dec 3, 2025 at 9:49 AM shveta malik <shveta.malik@gmail.com> wrote:
> > > > > > >
> > > > > > > relid             | 16391
> > > > > > > schemaname        | public
> > > > > > > relname           | conf_tab
> > > > > > > conflict_type     | multiple_unique_conflicts
> > > > > > > remote_xid        | 761
> > > > > > > remote_commit_lsn | 0/01761400
> > > > > > > remote_commit_ts  | 2025-12-02 15:02:07.045935+00
> > > > > > > remote_origin     | pg_16406
> > > > > > > key_tuple         |
> > > > > > > remote_tuple      | {"a":2,"b":3,"c":4}
> > > > > > > local_conflicts   |
> > > > > > >
{"{\"xid\":\"773\",\"commit_ts\":\"2025-12-02T15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":2,\"b\":2,\"c\":2}}","{\"xid\":\"
> > > > > > >
773\",\"commit_ts\":\"2025-12-02T15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":3,\"b\":3,\"c\":3}}","{\"xid\":\"773\",\"commit_ts\":\"2025-12-02T
> > > > > > > 15:02:00.640253+00:00\",\"origin\":\"\",\"tuple\":{\"a\":4,\"b\":4,\"c\":4}}"}
> > > > > > >
> > > > > >
> > > > > > Thanks, it looks good. For the benefit of others, could you include a
> > > > > > brief note, perhaps in the commit message for now, describing how to
> > > > > > access or read this array column? We can remove it later.
> > > > >
> > > > > Thanks, okay, temporarily I have added in a commit message how we can
> > > > > fetch the data from the JSON array field.  In next version I will add
> > > > > a test to get the conflict stored in conflict log history table and
> > > > > fetch from it.
> > > >
> > > > I noticed that the table structure can get changed by the time the
> > > > conflict record is prepared. In ReportApplyConflict(), the code
> > > > currently prepares the conflict log tuple before deciding whether the
> > > > insertion will be immediate or deferred:
> > > > +       /* Insert conflict details to conflict log table. */
> > > > +       if (conflictlogrel)
> > > > +       {
> > > > +               /*
> > > > +                * Prepare the conflict log tuple. If the error level
> > > > is below ERROR,
> > > > +                * insert it immediately. Otherwise, defer the
> > > > insertion to a new
> > > > +                * transaction after the current one aborts, ensuring
> > > > the insertion of
> > > > +                * the log tuple is not rolled back.
> > > > +                */
> > > > +               prepare_conflict_log_tuple(estate,
> > > > +
> > > > relinfo->ri_RelationDesc,
> > > > +
> > > > conflictlogrel,
> > > > +                                                                  type,
> > > > +                                                                  searchslot,
> > > > +
> > > > conflicttuples,
> > > > +                                                                  remoteslot);
> > > > +               if (elevel < ERROR)
> > > > +                       InsertConflictLogTuple(conflictlogrel);
> > > > +
> > > > +               table_close(conflictlogrel, RowExclusiveLock);
> > > > +       }
> > > >
> > > > If the conflict history table defintion is changed just before
> > > > prepare_conflict_log_tuple, the tuple creation will crash:
> > > > Program received signal SIGSEGV, Segmentation fault.
> > > > 0x00005a342e01df4f in VARATT_CAN_MAKE_SHORT (PTR=0x4000) at
> > > > ../../../../src/include/varatt.h:419
> > > > 419 return VARATT_IS_4B_U(PTR) &&
> > > > (gdb) bt
> > > > #0  0x00005a342e01df4f in VARATT_CAN_MAKE_SHORT (PTR=0x4000) at
> > > > ../../../../src/include/varatt.h:419
> > > > #1  0x00005a342e01e5ed in heap_compute_data_size
> > > > (tupleDesc=0x7ab405e5dda8, values=0x7ffd7af3ad20,
> > > > isnull=0x7ffd7af3ad15) at heaptuple.c:239
> > > > #2  0x00005a342e0200dd in heap_form_tuple
> > > > (tupleDescriptor=0x7ab405e5dda8, values=0x7ffd7af3ad20,
> > > > isnull=0x7ffd7af3ad15) at heaptuple.c:1158
> > > > #3  0x00005a342e55e8c2 in prepare_conflict_log_tuple
> > > > (estate=0x5a3467944530, rel=0x7ab405e594e8,
> > > > conflictlogrel=0x7ab405e5da88, conflict_type=CT_INSERT_EXISTS,
> > > > searchslot=0x0,
> > > >     conflicttuples=0x5a3467942da0, remoteslot=0x5a346792e498) at conflict.c:936
> > > > #4  0x00005a342e55cea6 in ReportApplyConflict (estate=0x5a3467944530,
> > > > relinfo=0x5a346792e778, elevel=21, type=CT_INSERT_EXISTS,
> > > > searchslot=0x0, remoteslot=0x5a346792e498,
> > > >     conflicttuples=0x5a3467942da0) at conflict.c:168
> > > > #5  0x00005a342e348c35 in CheckAndReportConflict
> > > > (resultRelInfo=0x5a346792e778, estate=0x5a3467944530,
> > > > type=CT_INSERT_EXISTS, recheckIndexes=0x5a3467942648, searchslot=0x0,
> > > >     remoteslot=0x5a346792e498) at execReplication.c:793
> > > >
> > > > This can be reproduced by the following steps:
> > > > CREATE PUBLICATION pub;
> > > > CREATE SUBSCRIPTION sub ... WITH (conflict_log_table = 'conflict');
> > > > ALTER TABLE conflict RENAME TO conflict1:
> > > > CREATE TABLE conflict(c1 varchar, c2 varchar);
> > > > -- Cause a conflict, this will crash while trying to prepare the
> > > > conflicting tuple
> > >
> > > Yeah while it is allowed to drop or alter the conflict log table, it
> > > should not seg fault, IMHO error is acceptable as per the initial
> > > discussion, so I will look into this and tighten up the logic so that
> > > it will throw an error whenever it can not insert into the conflict
> > > log table.
> >
> > I was thinking about the solution that we need to do if table
> > definition is changed, one option is whenever we try to prepare the
> > tuple after acquiring the lock we can validate the table definition if
> > this doesn't qualify the standard conflict log table schema we can
> > ERROR out.  IMHO that should not be an issue as we are only doing this
> > in conflict logging.
>
> Should we emit a warning instead of error, to stay consistent with the
> other exception case where a warning is raised when the conflict log
> table does not exist?
> +       /* Conflict log table is dropped or not accessible. */
> +       if (conflictlogrel == NULL)
> +               ereport(WARNING,
> +                               (errcode(ERRCODE_UNDEFINED_TABLE),
> +                                errmsg("conflict log table \"%s.%s\"
> does not exist",
> +
> get_namespace_name(nspid), conflictlogtable)));

Yes this should be WARNING.

--
Regards,
Dilip Kumar
Google