Re: Report bytes and transactions actually sent downtream

On Thu, Dec 18, 2025 at 11:52 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Dec 18, 2025 at 06:22:40PM +0530, Ashutosh Bapat wrote:
> > Hi Bertrand,
> >
> > On Wed, Dec 17, 2025 at 2:12 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > > What worries me is all those API changes:
> > >
> > > -typedef void (*LogicalDecodeChangeCB) (struct LogicalDecodingContext *ctx,
> > > +typedef bool (*LogicalDecodeChangeCB) (struct LogicalDecodingContext *ctx,
> > >
> > > Those changes will break existing third party logical decoding plugin, even ones
> > > that don't want the new statistics features.
> > >
> > > What about not changing those and just add a single new optional callback, say?
> > >
> > > typedef void (*LogicalDecodeReportStatsCB)(
> > > LogicalDecodingContext *ctx,
> > > ReorderBufferTXN *txn,
> > > bool *transaction_sent,
> > > size_t *bytes_filtered
> > > );
> > >
> > > This way:
> > >
> > > - Existing plugins can still work without modification
> > > - New or existing plugins can choose to provide statistics
> > >
> >
> > I think that it will bring back the same problems that the previous
> > design had or am I missing something?
>
> I think that my example was confusing due to "size_t *bytes_filtered". I think
> that what we could do is something like:
>
> "
> typedef void (*LogicalDecodeReportStatsCB)(
> LogicalDecodingContext *ctx,
> LogicalDecodeEventType event_type,
> bool *filtered,
> bool *txn_sent);
> "
>
> Note that there is no more size_t.
>

Thanks for the clarification. It fixes the problem of filteredBytes
divergence. Since the core is calling stats callback, the problem of
plugin not calling the function at appropriate places is also not
there. IIUC, it still has some problems from the previous solution and
some new problems as explained below.

> Then for, for example in change_cb_wrapper(), we could do:
>
> "
> ctx->callbacks.change_cb(ctx, txn, relation, change);
>
> if (ctx->callbacks.report_stats_cb)
> {
> bool filtered = false;
>
> ctx->callbacks.report_stats_cb(ctx, LOGICALDECODE_CHANGE,
> &filtered, NULL);
>
> if (filtered)
> cache->filteredBytes += ReorderBufferChangeSize(change);
> }
> "
>
> The plugin would need to "remember" that it filtered (so that it can
> reply to the callback). It could do that by adding say "last_event_filtered" to
> it's output_plugin_private structure.

Why does the core send NULL for the second parameter? Does the output
plugin have to take care of NULL references too?

I think the core will end up calling this or similar stanza at every
callback since it won't know when the output plugin will have
statistics to report. That's more complexity and wasted CPU cycles in
core.

>
> That's more work on the plugin side and we would probably need to provide some
> examples from our side.

Andres is objecting to this exact thing. IIUC, the code changes there
were far simpler than this proposal. Am I missing something?

My feeling is that the core will end up
>
> I think the pros are that:
>
> - plugins that don't want to report stats would have nothing to do (no breaking
> changes)

I don't think there will be an output plugin which wouldn't want to
take advantage of the statistics. The easier it is for them to adopt
the statistics, as is with my proposal, the better. With this proposal
output plugins have to do more work if they want to support
statistics. That itself will create a barrier for them to adopt the
statistics. We want the output plugins to support statistics so that
users can benefit. Let's make it easier for the output plugins to
implement them.

I feel this proposal makes both sides, the core and the output plugin
complex in pursuit of a goal which is not worth it.

This stil has the problem that you had raised. What if an output
plugin stops supporting statistics across versions?

--
Best Wishes,
Ashutosh Bapat