Re: Replication slot stats misgivings

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.

Regards,
Vignesh