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

On Sat, Sep 26, 2026 at 2:44 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Sat, Sep 26, 2026 at 6:34 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Sep 22, 2026 at 10:56 PM Nisha Moond <nisha.moond412@gmail.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