Re: Replication slot stats misgivings

On Tue, Apr 27, 2021 at 11:31 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, Apr 26, 2021 at 8:42 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Apr 26, 2021 at 8:01 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Apr 23, 2021 at 6:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Apr 19, 2021 at 4:28 PM vignesh C <vignesh21(at)gmail(dot)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.

I agree that makes it easier to send slot stats during
ReplicationSlotRelease() but I'd prefer to avoid storing data that
doesn't need to be shared in the shared buffer if possible. And those
counters are not used by physical slots at all. If sending slot stats
every time we stream or spill changes doesn't affect the system much,
I think it's better than having slot stats in the shared memory.

Also, not sure it’s better but another idea would be to make the slot
stats a global variable like pgBufferUsage and use it during decoding.
Or we can set a proc-exit callback? But to be honest, I'm not sure
which approach we should go with. Those approaches have proc and cons.

Regards,

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