Re: Replication slot stats misgivings

On Mon, Apr 26, 2021 at 8:42 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 26, 2021 at 8:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Apr 23, 2021 at 6:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Apr 19, 2021 at 4:28 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > I have made the changes to update the replication statistics at
> > > > replication slot release. Please find the patch attached for the same.
> > > > Thoughts?
> > > >
> > >
> > > Thanks, the changes look mostly good to me. The slot stats need to be
> > > initialized in RestoreSlotFromDisk and ReplicationSlotCreate, not in
> > > StartupDecodingContext. Apart from that, I have moved the declaration
> > > of UpdateDecodingStats from slot.h back to logical.h. I have also
> > > added/edited a few comments. Please check and let me know what do you
> > > think of the attached?
> >
> > The patch moves slot stats to the ReplicationSlot data that is on the
> > shared memory. If we have a space to store the statistics in the
> > shared memory can we simply accumulate the stats there and make them
> > persistent without using the stats collector?
> >
>
> But for that, we need to write to file at every commit/abort/prepare
> (decode of commit) which I think will incur significant overhead.
> Also, we try to write after few commits then there is a danger of
> losing them and still there could be a visible overhead for small
> transactions.
>

I preferred not to persist this information to file, let's have stats
collector handle the stats persisting.

> > And I think there is
> > also a risk to increase shared memory when we want to add other
> > statistics in the future.
> >
>
> Yeah, so do you think it is not a good idea to store stats in
> ReplicationSlot? Actually storing them in a slot makes it easier to
> send them during ReplicationSlotRelease which is quite helpful if the
> replication is interrupted due to some reason. Or the other idea was
> that we send stats every time we stream or spill changes.

We use around 64 bytes of shared memory to store the statistics
information per slot, I'm not sure if this is a lot of memory. If this
memory is fine, then I felt the approach to store stats seems fine. If
that memory is too much then we could use the other approach to update
stats when we stream or spill the changes as suggested by Amit.

Regards,
Vignesh