On Wed, Sep 24, 2025 at 8:11 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote: > > 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 <shveta.ma...@gmail.com> wrote: > > > > > > On Wed, Sep 24, 2025 at 12:47 PM Ashutosh Bapat > > > <ashutosh.bapat....@gmail.com> wrote: > > > > > > > > On Wed, Sep 24, 2025 at 10:12 AM shveta malik <shveta.ma...@gmail.com> > > > > 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 > > + <row> > + <entry role="catalog_table_entry"><para role="column_definition"> > + <structfield>plugin_filtered_bytes</structfield> <type>bigint</type> > + </para> > + <para> > + Amount of changes, from <structfield>total_wal_bytes</structfield>, > 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 > + <structfield>plugin</structfield>. > > I found "The count" somehow ambiguous. What about "This statistic" instead?
Existing fields use term "The counter". Changed "The count" to "The counter". > > === 2 > > + subtransactions. These transactions are subset of transctions sent to > > s/transctions/transactions Done. > > === 3 > > + the decoding plugin. Hence this count is expected to be lesser than > or > > s/be lesser/be less/? (not 100% sure) Less than is correct. Fixed. > > === 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. Do you see any problem in exposing ReorderBufferChangeSize(). It's a pretty small function and may be quite handy to output plugins otherwise as well. And we expose many ReorderBuffer related functions; so this isn't the first. If we were to do as you say, it will change other external facing APIs like change_cb(). Output plugins will need to change their code accordingly even when they don't want to support plugin statistics. Given that we have made maintaining plugin statistics optional, forcing API change does not make sense. For example, test_decoding which does not filter anything would unnecessarily have to change its code. I considered adding a field size to ReorderBufferChange itself. But that means we increase the amount of memory used in the reorder buffer, which seems to have become prime estate these days. So rejected that idea as well. Advantage of this change is that the minimal cost of calculating the size and maintaining the code change is incurred only when filtering happens, by the plugins which want to filter and maintain statistics. > > === 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. That's correct. Even output_plugin_private is freed when the decoding memory context is freed. Thanks for the review comments. I have addressed the comments in my repository and the changes will be included in the next set of patches. Do you have any further review comments? -- Best Wishes, Ashutosh Bapat
