Re: Replication slot stats misgivings

On Wed, Apr 14, 2021 at 12:09 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Apr 13, 2021 at 1:37 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, Apr 12, 2021 at 7:03 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > >
> > > The following test for the latest v8 patch seems to show different.
> > > total_bytes is 1808 whereas spill_bytes is 13200000. Am I missing
> > > something?
> > >
> > > postgres(1:85969)=# select pg_create_logical_replication_slot('s',
> > > 'test_decoding');
> > > pg_create_logical_replication_slot
> > > ------------------------------------
> > > (s,0/1884468)
> > > (1 row)
> > >
> > > postgres(1:85969)=# create table a (i int);
> > > CREATE TABLE
> > > postgres(1:85969)=# insert into a select generate_series(1, 100000);
> > > INSERT 0 100000
> > > postgres(1:85969)=# set logical_decoding_work_mem to 64;
> > > SET
> > > postgres(1:85969)=# select * from pg_stat_replication_slots ;
> > > slot_name | total_txns | total_bytes | spill_txns | spill_count |
> > > spill_bytes | stream_txns | stream_count | stream_bytes | stats_reset
> > > -----------+------------+-------------+------------+-------------+-------------+-------------+--------------+--------------+-------------
> > > s | 0 | 0 | 0 | 0 |
> > > 0 | 0 | 0 | 0 |
> > > (1 row)
> > >
> > > postgres(1:85969)=# select count(*) from
> > > pg_logical_slot_peek_changes('s', NULL, NULL);
> > > count
> > > --------
> > > 100004
> > > (1 row)
> > >
> > > postgres(1:85969)=# select * from pg_stat_replication_slots ;
> > > slot_name | total_txns | total_bytes | spill_txns | spill_count |
> > > spill_bytes | stream_txns | stream_count | stream_bytes | stats_reset
> > > -----------+------------+-------------+------------+-------------+-------------+-------------+--------------+--------------+-------------
> > > s | 2 | 1808 | 1 | 202 |
> > > 13200000 | 0 | 0 | 0 |
> > > (1 row)
> > >
> >
> > Thanks for identifying this issue, while spilling the transactions
> > reorder buffer changes gets released, we will not be able to get the
> > total size for spilled transactions from reorderbuffer size. I have
> > fixed it by including spilledbytes to totalbytes in case of spilled
> > transactions. Attached patch has the fix for this.
> > Thoughts?
> >
>
> I am not sure if that is the best way to fix it because sometimes we
> clear the serialized flag in which case it might not give the correct
> answer. Another way to fix it could be that before we try to restore a
> new set of changes, we update totalBytes counter. See, the attached
> patch atop your v6-0002-* patch.

I felt calculating totalbytes this way is better than depending on
spill_bytes. I have taken your changes. Attached patch includes the
changes suggested.
Thoughts?

Regards,
Vignesh