Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip,

Thank you once again for the great work, please find some more comments on
v77 series of patches below,

1. build_local_conflicts_json_array() still uses row_to_json(). Can we
apply the same changes from v77-0002 in this place too ?

/*
* Builds the local conflicts JSON array column from the list of
* ConflictTupleInfo objects.
*
* Example output structure:
* [ { "xid": "1001", "commit_ts": "...", "origin": "..." }, ... ]
*/
static Datum
build_local_conflicts_json_array(List *conflicttuples)
{
.
.
.
.
/*
* Build the higher level JSON datum in format described in function
* header.
*/
json_datum = DirectFunctionCall1(row_to_json, datum);
.
.
.
}

2. Minor nit:

The commit message in v77-0001 states:

The JSON array uses the following structured format:
[ { "xid": "1001", "commit_ts": "2025-12-25 10:00:00+05:30", "origin":
"node_1",
"tuple": {"id": 1, "val": "old_data"} }, ... ]

In the code below however, there are only 3 fields: xid, commit_ts, and
origin. There is no "tuple" attribute,

/*
* 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}
};

Can we please consider changing the commit message in v77-0001 ?

Thank you,
Narayanan

On Tue, Sep 29, 2026 at 5:56 PM Narayanan Venkateswaran <
narayananvpostgres(at)gmail(dot)com> wrote:

> Hi Dilip,
>
> Thank you very much for the excellent work. I looked at the patch v77,
>
> The code decides replica_identity_full using the following logic (in
> conflict.c),
>
> if (!TupIsNull(searchslot))
> {
> Oid replica_index = GetRelationIdentityOrPK(rel);
>
> /*
> * If the table has a valid replica identity index, build the index
> * JSON datum from key value. Otherwise, in REPLICA IDENTITY FULL
> * cases, set replica_identity_full to true and leave replica_identity
> * NULL to avoid serializing full tuples that could exceed memory
> * allocation limits.
> */
> if (OidIsValid(replica_index))
> {
> values[attno++] = BoolGetDatum(false);
> values[attno++] = build_index_key_json(rel,
> replica_index,
> searchslot,
> &omitted);
> }
> else
> {
> values[attno++] = BoolGetDatum(true);
> nulls[attno++] = true;
> }
> }
> else
> {
> nulls[attno++] = true;
> nulls[attno++] = true;
> }
>
> In PostgreSQL catalogs (pg_class.relreplident), a table's replica can be
> one of four values:
>
> 'd' = REPLICA_IDENTITY_DEFAULT: Use PK index if one exists. If the table
> has no PK, it has no index and is NOT FULL.
> 'n' = REPLICA_IDENTITY_NOTHING: No replica identity.
> 'i' = REPLICA_IDENTITY_INDEX: Explicit unique index.
> 'f' = REPLICA_IDENTITY_FULL: The entire tuple is the identity.
>
> If a subscriber relation has REPLICA IDENTITY DEFAULT without a primary
> key (or REPLICA IDENTITY NOTHING) GetRelationIdentityOrPK() returns
> InvalidOid. In this case, the code sets replica_identity_full = true.
>
> However, the SGML docs update in the patch states that
> replica_identity_full "is NULL when replica identity information is not
> applicable".
>
> The only way replica_identity_full can ever be set to NULL is if the
> execution enters the outer else block: when TupIsNull(searchslot) is true
> (i.e., searchslot is NULL or empty). However, it looks like this slot
> contains the incoming row data sent by the publisher. It is always
> populated and never null.
>
> Because searchslot is never null, the outer else block is never executed.
> The code will never set replica_identity_full to NULL.
>
> I think it is better to explicitly check for REPLICA_IDENTITY_FULL in an
> else if block, something like the below,
>
> else if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
> {
> values[attno++] = BoolGetDatum(true);
> nulls[attno++] = true;
> }
>
> Thank you,
> Narayanan
>
> On Mon, Sep 28, 2026 at 9:03 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
>> On Sat, Sep 26, 2026 at 2:44 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com>
>> wrote:
>> >
>> > On Sat, Sep 26, 2026 at 6:34 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > >
>> > > On Tue, Sep 22, 2026 at 10:56 PM Nisha Moond <
>> nisha(dot)moond412(at)gmail(dot)com> wrote:
>> > > >
>> > > > A solution approach for both of these issues:
>> > > > ---------------------------------------------------------
>> > > > Case-B: when a column's type has a user-defined cast to json,
>> > > > row_to_json() uses that cast instead of the type's normal output
>> > > > function. The cast can return anything of any size no matter how
>> small
>> > > > the stored value is, so a 4-byte key can produce more than 1GB of
>> json
>> > > > and break apply. It applies inside arrays and composites too,
>> because
>> > > > row_to_json() walks into each element.
>> > > >
>> > > > The LOG case never had this problem, because it prints each key
>> column
>> > > > with the type's output function and a cast cannot replace that. So
>> we
>> > > > do the same: render each value with its output function and build
>> the
>> > > > json object ourselves. No cast is then reachable at any nesting
>> depth.
>> > > >
>> > > > The cost is that values become json strings instead of numbers,
>> > > > booleans or nested objects. With a int PRIMARY KEY, {"a":1} becomes
>> > > > {"a":"1"}, and a jsonb key becomes a quoted string.
>> > > > This does not reduce what can be queried. "->>" still returns a
>> value
>> > > > that can be cast back to the column’s type, so
>> > > > (replica_identity->>'a')::int can retrieve the key and be used to
>> find
>> > > > the row. Only querying inside a container key requires one extra
>> cast,
>> > > > e.g. (replica_identity->>'doc')::jsonb->>'x'.
>> > > >
>> > > > Case A: With case B handled, adding a per-column size cap before
>> > > > rendering becomes straightforward. 1kB looks like the right size.
>> > > > Roughly, the rendered value per key column is
>> > > > escape_json(typoutput(value)) plus small overhead, and:
>> > > > - escape_json expands at most 6x, since every byte below 0x20
>> becomes \u00XX
>> > > > - INDEX_MAX_KEYS bounds the column count at 32
>> > > > - the cap bounds each varlena input at 1kB
>> > > >
>> > > > Among built-in types only numeric produces output vastly larger than
>> > > > its storage: length(1e131071::numeric::text) is 131,072, so 10 bytes
>> > > > becomes 131kB. Repeating that inside a container (array, jsonb or
>> > > > multirange) gives at most about 11,000x per stored byte. See the
>> case
>> > > > at [1].
>> > > >
>> > > > So the worst case at 1kB is 1kB * 11,000 * 32 = ~360MB, a 3x margin
>> > > > below the 1GB limit. 2kB gives ~720MB, only 1.5x, which seems thin.
>> > > > 4kB exceeds 1GB outright. Hence 1kB.
>> > > >
>> > > > Fixed-length columns are not size-checked, as their output is
>> bounded
>> > > > by construction and adds only a few MB across 32 columns.
>> > > >
>> > >
>> > > The solution for these problems in the attached patch looks
>> reasonable to me.
>> > >
>> > > > What remains:
>> > > > Together these cover every built-in type, scalar and container
>> alike.
>> > > > The one case neither handles is a user-defined type whose own output
>> > > > function renders far more than its input; no cap on the input can
>> > > > detect that. Such a function has to be written in C, since a SQL
>> > > > function cannot return cstring, so it sits at the same level as
>> > > > replacing a built-in output function. IMO, that seems acceptable to
>> > > > leave for now.
>> > > >
>> > >
>> > > I have tried to verify this with the attached extension (written with
>> > > the help of AI). Here, I want to be precise with one thing that with
>> > > such a misbehaved out function, even by default replication will fail
>> > > because we use text format to transfer data and the same out function
>> > > will be invoked leading to an allocation failure. In binary format, it
>> > > will pass. So, in the attached patch, if remove "binary = true" from
>> > > the following statement:
>> > > + CONNECTION '$publisher_connstr application_name=$appname'
>> > > + PUBLICATION pub_bt WITH (binary = true, conflict_log_destination =
>> all)"
>> > >
>> > > the test will fail with publisher LOG printing:
>> > >
>> > > 2026-09-26 05:54:10.888 IST walsender[26238] sub_bt ERROR: string
>> > > buffer exceeds maximum allowed length (1073741823 bytes)
>> > > 2026-09-26 05:54:10.888 IST walsender[26238] sub_bt DETAIL: Cannot
>> > > enlarge string buffer containing 600000049 bytes by 600000000 more
>> > > bytes.
>> > > 2026-09-26 05:54:10.888 IST walsender[26238] sub_bt CONTEXT: slot
>> > > "sub_bt", output plugin "pgoutput", in the change callback, associated
>> > > LSN 0/018043D0
>> > > So, it is fine to leave this as is. Also, as such a function has to be
>> > > written in C which means one can write something to even crash the
>> > > backend (can read/write arbitrary memory) which is way worse than
>> > > allocation ERROR.
>> >
>> > Yeah that analogy makes sense to me.
>>
>> There is one issue with the patch, the problem is that for some data
>> type index operator class type is different than actual table column
>> type, and the original patch was using the tuple descriptor of the
>> index, that means the column type would be indexopclass type whereas
>> for fetching the datum value
>> build_index_key_json()->build_index_datums_from_slot()->FormIndexDatum()
>> and internally FormIndexDatum will fetch the tuple from heap
>> TupleTableSlot, that means we are using index op type to fetch tuple
>> from TupleTableSlot and it will not identify that type. Attached top
>> up patch fixes that by fetching the pg_attribute tuple from the table
>> tuple descriptor. Folded build_index_datums_from_slot() back into
>> build_index_value_desc(), which is now its only caller, and drop the
>> unused EState parameter from build_index_key_json() and
>> insert_conflict_log_tuple(). Also initialize omitted at the start of
>> build_index_key_json() and make minor improvements to the conflict log
>> documentation.
>>
>> --
>> Regards,
>> Dilip Kumar
>> Google
>>
>