Re: Proposal: Conflict log history table for Logical Replication

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
>