On Mon, Sep 22, 2025 at 10:44 AM shveta malik <[email protected]> wrote:
>
> Few trivial comments:
>
> 1)
> Currently the doc says:
>
> sentTxns is the number of transactions sent downstream by the output
> plugin. sentBytes is the amount of data, in bytes, sent downstream by
> the output plugin. OutputPluginWrite will update this counter if
> ctx->stats is initialized by the output plugin. filteredBytes is the
> size of changes, in bytes, that are filtered out by the output plugin.
> Function ReorderBufferChangeSize may be used to find the size of
> filtered ReorderBufferChange.
>
> Shall we rearrange it to:
>
> sentTxns is the number of transactions sent downstream by the output
> plugin. sentBytes is the amount of data, in bytes, sent downstream by
> the output plugin. filteredBytes is the size of changes, in bytes,
> that are filtered out by the output plugin. OutputPluginWrite will
> update these counters if ctx->stats is initialized by the output
> plugin.
> The function ReorderBufferChangeSize can be used to compute the size
> of a filtered ReorderBufferChange, i.e., the filteredBytes.
>
Only sentBytes is incremented by OutputPluginWrite(), so saying that
it will update counters is not correct. But I think you intend to keep
description of all the fields together followed by any additional
information. How about the following
<literal>sentTxns</literal> is the number of transactions sent downstream
by the output plugin. <literal>sentBytes</literal> is the amount of data,
in bytes, sent downstream by the output plugin.
<literal>filteredBytes</literal> is the size of changes, in bytes, that
are filtered out by the output plugin.
<function>OutputPluginWrite</function> will update
<literal>sentBytes</literal> if <literal>ctx->stats</literal> is
initialized by the output plugin. Function
<literal>ReorderBufferChangeSize</literal> may be used to find the size of
filtered <literal>ReorderBufferChange</literal>.
> 2)
> My preference will be to rename the fields 'total_txns' and
> 'total_bytes' in PgStat_StatReplSlotEntry to 'total_wal_txns' and
> 'total_wal_bytes' for better clarity. Additionally, upon rethinking,
> it seems better to me that plugin-related fields are also named as
> plugin_* to clearly indicate their association. OTOH, in
> OutputPluginStats, the field names are fine as is, since the structure
> name itself clearly indicates these are plugin-related fields.
> PgStat_StatReplSlotEntry lacks such context and thus using full
> descriptive names there would improve clarity.
Ok. Done.
>
> 3)
> LogicalOutputWrite:
> + if (ctx->stats)
> + ctx->stats->sentBytes += ctx->out->len + sizeof(XLogRecPtr) +
> sizeof(TransactionId);
> p->returned_rows++;
>
> A blank line after the new change will increase readability.
>
Ok.
> ~~
>
> In my testing, the patch works as expected. Thanks!
Thanks for testing. Can we include any of your tests in the patch? Are
the tests in patch enough?
Applied those suggestions in my repository. Do you have any further
review comments?
--
Best Wishes,
Ashutosh Bapat
