Re: Report bytes and transactions actually sent downtream

On Tue, Sep 23, 2025 at 4:06 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Mon, Sep 22, 2025 at 10:44 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> 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-&gt;stats</literal> is
> initialized by the output plugin. Function
> <literal>ReorderBufferChangeSize</literal> may be used to find the size of
> filtered <literal>ReorderBufferChange</literal>.

Yes, this looks good.

>
> > 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?

I tested the flows with
a) logical replication slot and get-changes.
b) filtered data flows: pub-sub creation with row_filters, 'publish'
options. I tried to verify plugin fields as compared to total_wal*
fields.
c) reset flow.

While tests for a and c are present already. I don't see tests for b
anywhere when it comes to stats. Do you think we shall add a test for
filtered data using row-filter somewhere?

>
> Applied those suggestions in my repository. Do you have any further
> review comments?
>

No, I think that is all.

thanks
Shveta