Re: Report bytes and transactions actually sent downtream

On Tue, Sep 30, 2025 at 12:22 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Sep 29, 2025 at 12:54:24PM +0530, Ashutosh Bapat wrote:
> > On Fri, Sep 26, 2025 at 10:28 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > > > >
> > > >
> > > > I don't think this is an issue. There is no way for the core to tell
> > > > whether the plugin will provide stats or not, unless it sets that
> > > > ctx->stats which happens in the startup callback. Till then it is
> > > > rightly providing the values accumulated so far. Once the decoding
> > > > starts, we know that the plugin is not providing any stats and we
> > > > don't display anything.
> > >
> > > Yeah, I got the technical reasons, but I think there's a valid user experience
> > > concern here: seeing statistics for a plugin that doesn't actually support
> > > statistics is misleading.
> > >
> >
> > 3. If the plugin starts supporting statistics and midway discontinues
> > its support, it already has a problem with backward compatibility.
> >
> > Practically it would 1 or 2, which are working fine.
> >
> > I don't think we will encounter case 3 practically. Do you have a
> > practical use case where a plugin would discontinue supporting stats?
>
> Not that I can think of currently. That looks unlikely but wanted to raise
> the point though. Maybe others see a use case and/or have a different point
> of view.
>
> > > What we need is a call to pgstat_report_replslot() to display stats that reflect
> > > the current plugin behavior. We can't just call pgstat_report_replslot()
> > > in say RestoreSlotFromDisk() because we really need the decoding to start.
> > >
> > > So one idea could be to set a flag (per slot) when pgstat_report_replslot()
> > > has been called (for good reasons) and check for this flag in
> > > pg_stat_get_replication_slot().
> > >
> > > If the flag is not set, then set the plugin fields to NULL.
> > > If the flag is set, then display their values (like now).
> >
> > This approach will have the same problem. Till
> > pgstat_report_replslot() is called, the old statistics will continue
> > to be shown.
>
> I don't think so because the flag would not be set.
>
> > > And we should document that the plugin stats are not available (i.e are NULL)
> > > until the decoding has valid stats to report after startup.
> >
> > The current documentation is " It is NULL when statistics is not
> > initialized or immediately after a reset or when not maintained by the
> > output plugin.". I think that covers all the cases.
>
> Do you think the doc covers the case we discussed above? i.e when a plugin
> discontinue supporting stats, it would display stats until the decoding actually
> starts.



Here's patchset addressing two issues:

Issue 1: A plugin supports stats in version X. It stopped supporting
the stats in version X + 1. It again started supporting stats in
version X + 2. Plugin stats will be accumulated when it was at version
X. When X + 1 is loaded, the stats will continue to report the stats
accumulated (by version X) till the first startup_call for that
replication slot happens. If the user knows (from documentation say)
that X + 1 does not support stats, seeing statistics will mislead
them. We don't know whether there's a practical need to do so. A
plugin which flip-flops on stats is breaking backward compatibility. I
have added a note in documentation for plugin authors, warning them
that this isn't expected. I don't think it's worth adding complexity
in code to support such a case unless we see a practical need for the
same.

Issue 2: Once X + 2 is loaded, further statistics are accumulated on
the top of statistics accumulated by version X. Attached patch fixes
issue 2 by zero'ing out the stats when the plugin does not report the
statistics.

The patchset also addresses your earlier review comments.
--
Best Wishes,
Ashutosh Bapat