Re: Proposal: Conflict log history table for Logical Replication

On Fri, Sep 25, 2026 at 11:10 AM kedar anavardekar
<kedar(dot)anavardekar(at)gmail(dot)com> wrote:
>
> Hi Nisha,
> Thank you for the patch. I applied it locally, and it compiles successfully.
> I have a few suggestions for consideration for v75. Please treat these
> as optional review comments;
> you and the others can decide whether they are important enough to address.
> Otherwise, feel free to ignore them.
>

Thanks for the review.

>
> > Attached are the updated patches:
> > v75-001: Merged v74 patches 001 and 002 into a single patch.
> > v75-002: Includes the discussed fix, along with TAP tests covering
> > both cases. Also updated the documentation with an example.
> >
>
> 1. Documentation:
> In the example query, could the hard-coded table name:
> pg_conflict.pg_conflict_log_16392
> be replaced with a placeholder such as:
> pg_conflict_log_<subid>
> which is used at other places in doc.
>

The doc already defines the naming rule as pg_conflict_log_<subid> at
the start of the section. I think the examples can use the exact name,
like the hard-coded slot name in [1].
I’ll keep it as is for now and see what others think. Happy to change
it if needed.

> 2. Documentation:
> The documentation says that values larger than 1 kB are omitted.
> However, the implementation
> measures raw detoasted storage size rather than textual output size.
> This distinction may
> matter for custom types, arrays, compressed values, and JSON-like types.
>

Okay, I see it can be confusing. I have reworded the doc section added
patch 002 and added clarification for the 1kB limit.

> 3. Code:
> In:
> + return PointerGetDatum(cstring_to_text_with_len(result.data, result.len));
>
> `result.data` does not appear to be released after the text datum is created.
> Could this accumulate memory when multiple conflicts occur in one
> apply transaction?
>

Far point. ApplyMessageContext resets after each replication message,
so it wouldn't accumulate across a transaction.
But cstring_to_text_with_len() copies, so the buffer is dead
immediately and there's no reason to hold it. Added the pfree().

> 4. Code:
> The name `tuple_table_slot_to_indextup_json` was not immediately clear to me.
> Since the function serializes index key values rather than an index tuple,
> would something like `tuple_table_slot_to_index_key_json` be more descriptive?
>

Okay, function definition is changed, so renamed it as
"build_index_key_json" keeping consistent with neighbouring functions.

> 5. Code:
> Is the following variable necessary? It appears not to be used currently:
> + bool typisvarlena;
>

getTypeOutputInfo() has no NULL check for that out-parameter, hence it
requires &typisvarlena, even though we don't use the result.
~~~

Also added a XXX comment for the uncovered remaining case pointed out in [2].

Attached are the updated patches. Patch 001 has no change from the last version.

[1] https://www.postgresql.org/docs/devel/logical-replication-failover.html
[2] https://www.postgresql.org/message-id/CABdArM7LAtD5a8d%3DURTxxjXjJ4p6uhJVyGXq8QZBAbSPfaictw%40mail.gmail.com

--
Thanks,
Nisha