Re: Report bytes and transactions actually sent downtream

Hi,

On Wed, Sep 24, 2025 at 05:28:44PM +0530, Ashutosh Bapat wrote:
> On Wed, Sep 24, 2025 at 2:38 PM shveta malik  wrote:
> >
> > On Wed, Sep 24, 2025 at 12:47 PM Ashutosh Bapat
> >  wrote:
> > >
> > > On Wed, Sep 24, 2025 at 10:12 AM shveta malik  wrote:
> > > >
> > > > 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?
> > >
> > > Added a test in 028_row_filter. Please find it in the attached
> > > patchset.
> >
> > Test looks good.
> 
> Thanks. Added to three more files. I think we have covered all the
> cases where filtering can occur.
> 
> PFA patches.

Thanks for the new version!

A few random comments:

=== 1

+     
+      
+        plugin_filtered_bytes bigint
+       
+       
+        Amount of changes, from total_wal_bytes, filtered
+        out by the output plugin and not sent downstream. Please note that it
+        does not include the changes filtered before a change is sent to
+        the output plugin, e.g. the changes filtered by origin. The count is
+        maintained by the output plugin mentioned in
+        plugin.

I found "The count" somehow ambiguous. What about "This statistic" instead?

=== 2

+        subtransactions. These transactions are subset of transctions sent to

s/transctions/transactions

=== 3

+        the decoding plugin. Hence this count is expected to be lesser than or

s/be lesser/be less/? (not 100% sure)

=== 4

+extern Size ReorderBufferChangeSize(ReorderBufferChange *change);

Another approach could be to pass the change's size as an argument to the
callbacks? That would avoid to expose ReorderBufferChangeSize publicly.

=== 5

        ctx->output_plugin_private = data;
+       ctx->stats = palloc0(sizeof(OutputPluginStats));

I was wondering if we need to free this in pg_decode_shutdown, but it looks
like it's done through FreeDecodingContext() anyway.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com