On Fri, Sep 25, 2026 at 11:10 AM kedar anavardekar
<kedar.anavardekar@gmail.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
