Re: Report bytes and transactions actually sent downtream

Hi,

On Thu, Sep 25, 2025 at 10:16:35AM +0530, Ashutosh Bapat wrote:
> On Wed, Sep 24, 2025 at 8:11 PM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > === 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.

Right. I don't see a problem per say, just thinking that the less we expose
publicly to be used by extensions/plugins, the better.

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

Correct.

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

That's right.

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

Yes, anyway as it's unlikely that we have to fix a bug in a minor release that
would need a signature change to ReorderBufferChangeSize(), I think that's fine
as proposed.

> >
> > === 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.

Thanks!

> Do you have any further review comments?

Not right now. I'll give it another look by early next week the latest.

Regards,

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