Re: Replication slot stats misgivings

On Tue, Apr 27, 2021 at 9:48 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Apr 27, 2021 at 9:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 9:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Apr 27, 2021 at 11:31 AM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > > > 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.
> > >
> >
> > Sounds reasonable and we might add some stats in the future so that
> > will further increase the usage of shared memory.
> >
> > > 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.
> > >
> >
> > As the minimum size of logical_decoding_work_mem is 64KB, so in the
> > worst case, we will send stats after decoding that many changes. I
> > don't think it would impact too much considering that we need to spill
> > or stream those many changes.  If it concerns any users they can
> > always increase logical_decoding_work_mem. The default value is 64MB
> > at which point, I don't think it will matter sending the stats.
>
> Sounds good to me, I will rebase my previous patch and send a patch for this.
>

Attached patch has the changes to update statistics during
spill/stream which prevents the statistics from being lost during
interrupt.
Thoughts?

Regards,
Vignesh