On Monday, April 1, 2024 9:28 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
>
> On Mon, Apr 01, 2024 at 05:04:53PM +0530, Amit Kapila wrote:
> > On Mon, Apr 1, 2024 at 2:51 PM Bertrand Drouvot
>
> >
> > > 2 ===
> > >
> > > + {
> > > + if (SnapBuildSnapshotExists(remote_slot->restart_lsn))
> > > + {
> > >
> > > That could call SnapBuildSnapshotExists() multiple times for the
> > > same "restart_lsn" (for example in case of multiple remote slots to sync).
> > >
> > > What if the sync worker records the last lsn it asks for
> > > serialization (and serialized ? Then we could check that value first
> > > before deciding to call (or not)
> > > SnapBuildSnapshotExists() on it?
> > >
> > > It's not ideal because it would record "only the last one" but that
> > > would be simple enough for now (currently there is only one sync
> > > worker so that scenario is likely to happen).
> > >
> >
> > Yeah, we could do that but I am not sure how much it can help. I guess
> > we could do some tests to see if it helps.
>
> Yeah not sure either. I just think it can only help and shouldn't make things
> worst (but could avoid extra SnapBuildSnapshotExists() calls).
Thanks for the idea. I tried some tests based on Nisha's setup[1]. I tried to
advance the slots on the primary to the same restart_lsn before calling
sync_replication_slots(), and reduced the data generated by pgbench. The
SnapBuildSnapshotExists is still not noticeable in the profile. So, I feel we
could leave this as a further improvement once we encounter scenarios where
the duplicate SnapBuildSnapshotExists call becomes noticeable.
[1] https://www.postgresql.org/message-id/CALj2ACUeij5tFzJ1-cuoUh%2Bmhj33v%2BYgqD_gHYUpRdXSCSBbhw%40mail.gmail.com
Best Regards,
Hou zj
