Re: Report bytes and transactions actually sent downtream - Mailing list pgsql-hackers

Hi Shveta, Bertrand,

Replying to both of your review comments together.

On Thu, Sep 18, 2025 at 10:52 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Aug 27, 2025 at 7:14 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Jul 24, 2025 at 12:24:26PM +0530, Ashutosh Bapat wrote:
> > > Here's the next patch which considers all the discussion so far. It
> > > adds four fields to pg_stat_replication_slots.
> > >     - plugin - name of the output plugin
> >
> > Is this one needed? (we could get it with a join on pg_replication_slots)
> >
>
> In my opinion, when there are other plugin_* fields present, including
> the plugin name directly here seems like a better approach. So, +1 for
> the plugin field.

Yeah. I think so too.

>
> > >     - plugin_filtered_bytes - reports the amount of changes filtered
> > > out by the output plugin
> > >     - plugin_sent_txns - the amount of transactions sent downstream by
> > > the output plugin
> > >     - plugin_sent_bytes - the amount of data sent downstream by the
> > > outputplugin.
> > >
> > > There are some points up for a discussion:
> > > 1. pg_stat_reset_replication_slot() zeroes out the statistics entry by
> > > calling pgstat_reset() or pgstat_reset_of_kind() which don't know
> > > about the contents of the entry. So
> > > PgStat_StatReplSlotEntry::plugin_has_stats is set to false and plugin
> > > stats are reported as NULL, instead of zero, immediately after reset.
> > > This is the same case when the stats is queried immediately after the
> > > statistics is initialized and before any stats are reported. We could
> > > instead make it report
> > > zero, if we save the plugin_has_stats and restore it after reset. But
> > > doing that in pgstat_reset_of_kind() seems like an extra overhead + we
> > > will need to write a function to find all replication slot entries.
>
> I tried to think of an approach where we can differentiate between the
> cases 'not initialized' and 'reset' ones with the values. Say instead
> of plugin_has_stats, if we have plugin_stats_status, then we can
> maintain status like -1(not initialized), 0(reset). But this too will
> complicate the code further. Personally, I’m okay with NULL values
> appearing even after a reset, especially since the documentation
> explains this clearly.

Ok. Great.

>
> > Could we store plugin_has_stats in ReplicationSlotPersistentData instead? That
> > way it would not be reset. We would need to access ReplicationSlotPersistentData
> > in pg_stat_get_replication_slot though.
>
> > Also would that make sense to expose plugin_has_stats in pg_replication_slots?
>

A plugin may change its decision to support the stats across versions,
we won't be able to tell when it changes that decision and thus
reflect it accurately in ReplicationSlotPersistentData. Doing it in
startup gives the opportunity to the plugin to change it as often as
it wants OR even based on some plugin specific configurations. Further
ReplicationSlotPersistentData is maintained by the core. It will not
be a good place to store something plugin specific.

> >
> > > 2. There's also a bit of asymmetry in the way sent_bytes is handled.
> > > The code which actually sends the logical changes to the downstream is
> > > part of the core code
> > > but the format of the change and hence the number of bytes sent is
> > > decided by the plugin. It's a stat related to plugin but maintained by
> > > the core code. The patch implements it as a plugin stat (so the
> > > corresponding column has "plugin" prefix
> >
> > The way it is done makes sense to me.

Great.

> >
> > > 3. The names of new columns have the prefix "plugin_" but the internal
> > > variables tracking those don't for the sake of brevity. If you prefer
> > > to have the same prefix for the internal variables, I can change that.
> >
>
> I am okay either way.
>
> > Just my taste: I do prefer when they match.

I don't see a strong preference to change what's there in the patch.
Let's wait for more reviews.

>
> Few comments:
>
> 1)
> postgres=# select slot_name,
> total_bytes,plugin_filtered_bytes,plugin_sent_bytes  from
> pg_stat_replication_slots order by slot_name;
>  slot_name | total_bytes | plugin_filtered_bytes | plugin_sent_bytes
> -----------+-------------+-----------------------+-------------------
>  slot1     |      800636 |                793188 |               211
>  sub1      |      401496 |                132712 |             84041
>  sub2      |      401496 |                396184 |               674
>  sub3      |      401496 |                145912 |             79959
> (4 rows)
>
> Currently it looks quite confusing. 'total_bytes' gives a sense that
> it has to be a sum of filtered and sent. But they are no way like
> that. In the thread earlier there was a proposal to change the name to
> reordered_txns, reordered_bytes. That looks better to me. It will give
> clarity without even someone digging into docs.

I also agree with that. But that will break backward compatibility. Do
you think other columns like spill_* and stream_* should also be
renamed with the prefix "reordered"?

>
> 2)
> Tried to verify all filtered data tests, seems to work well. Also  I
> tried tracking the usage of OutputPluginWrite() to see if there is any
> other place where data needs to be considered as filtered-data.
> Encountered this:
>
> send_relation_and_attrs has:
>                 if (!logicalrep_should_publish_column(att, columns,
>
>                    include_gencols_type))
>                         continue;
>                 if (att->atttypid < FirstGenbkiObjectId)
>                         continue;
>
> But I don't think it needs to be considered as filtered data. This is
> mostly schema related info. But I wanted to confirm once. Thoughts?

Yeah. It's part of metadata which in turn is sent only when needed.
It's not part of, say, transaction changes. So it can't be considered
as filtering.

>
> 3)
> +-- total_txns may vary based on the background activity but sent_txns should
> +-- always be 1 since the background transactions are always skipped. Filtered
> +-- bytes would be set only when there's a change that was passed to the plugin
> +-- but was filtered out. Depending upon the background transactions, filtered
> +-- bytes may or may not be zero.
> +SELECT slot_name, spill_txns = 0 AS spill_txns, spill_count = 0 AS
> spill_count, total_txns > 0 AS total_txns, total_bytes > 0 AS
> total_bytes, plugin_sent_txns, plugin_sent_bytes > 0 AS sent_bytes,
> plugin_filtered_bytes >= 0 AS filtered_bytes FROM
> pg_stat_replication_slots ORDER BY slot_name;
>
> In comment either we can say plugin_sent_txns instead of sent_txns or
> in the query we can fetch plugin_sent_txns AS  sent_txns, so that we
> can relate comment and query.
>

Used plugin_sent_txns in the comment as well as query.

>
> 4)
> +      <literal>sentTxns</literal> is the number of transactions sent downstream
> +      by the output plugin. <literal>sentBytes</literal> is the amount of data
> +      sent downstream by the output plugin.
> +      <function>OutputPluginWrite</function> is expected to update this counter
> +      if <literal>ctx->stats</literal> is initialized by the output plugin.
> +      <literal>filteredBytes</literal> is the size of changes in bytes that are
> +      filtered out by the output plugin. Function
> +      <literal>ReorderBufferChangeSize</literal> may be used to find
> the size of
> +      filtered <literal>ReorderBufferChange</literal>.
> +     </para>
>
> Either we can mention units as 'bytes' for both filteredBytes and
> sentBytes or for none. Currently filteredBytes says 'in bytes' while
> sentBytes does not.

Used 'in bytes' in both the places.

Thanks for your review. I will include these changes in the next set of patches.

--
Best Wishes,
Ashutosh Bapat