Re: Replication slot stats misgivings

On Mon, May 3, 2021 at 2:27 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Apr 29, 2021 at 10:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Apr 28, 2021 at 7:43 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Apr 28, 2021 at 3:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > >
> > > > I am not sure if any of these alternatives are a good idea. What do
> > > > you think? Do you have any other ideas for this?
> > >
> > > I've been considering some ideas but don't come up with a good one
> > > yet. It’s just an idea and not tested but how about having
> > > CreateDecodingContext() register before_shmem_exit() callback with the
> > > decoding context to ensure that we send slot stats even on
> > > interruption. And FreeDecodingContext() cancels the callback.
> > >
> >
> > Is it a good idea to send stats while exiting and rely on the same? I
> > think before_shmem_exit is mostly used for the cleanup purpose so not
> > sure if we can rely on it for this purpose. I think we can't be sure
> > that in all cases we will send all stats, so maybe Vignesh's patch is
> > sufficient to cover the cases where we avoid losing it in cases where
> > we would have sent a large amount of data.
> >
>
> Sawada-San, any thoughts on this point?

before_shmem_exit is mostly used to the cleanup purpose but how about
on_shmem_exit()? pgstats relies on that to send stats at the
interruption. See pgstat_shutdown_hook().

That being said, I agree Vignesh' patch would cover most cases. If we
don't find any better solution, I think we can go with Vignesh's
patch.

> Apart from this, I think you
> have suggested somewhere in this thread to slightly update the
> description of stream_bytes. I would like to update the description of
> stream_bytes and total_bytes as below:
>
> stream_bytes
> Amount of transaction data decoded for streaming in-progress
> transactions to the decoding output plugin while decoding changes from
> WAL for this slot. This and other streaming counters for this slot can
> be used to tune logical_decoding_work_mem.
>
> total_bytes
> Amount of transaction data decoded for sending transactions to the
> decoding output plugin while decoding changes from WAL for this slot.
> Note that this includes data that is streamed and/or spilled.
>
> This update considers two points:
> a. we don't send this data across the network because plugin might
> decide to filter this data, ex. based on publications.
> b. not all of the decoded changes are sent to plugin, consider
> REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID,
> REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT, etc.

Looks good to me.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/