On Mon, Apr 1, 2024 at 2:51 PM Bertrand Drouvot
<bertranddrouvot...@gmail.com> wrote:
>
> Hi,
>
> On Mon, Apr 01, 2024 at 06:05:34AM +0000, Zhijie Hou (Fujitsu) wrote:
> > On Monday, April 1, 2024 8:56 AM Zhijie Hou (Fujitsu)
> > <houzj.f...@fujitsu.com> wrote:
> > Attach the V4 patch which includes the optimization to skip the decoding if
> > the snapshot at the syncing restart_lsn is already serialized. It can avoid
> > most
> > of the duplicate decoding in my test, and I am doing some more tests
> > locally.
> >
>
> Thanks!
>
> 1 ===
>
> Same comment as in [1].
>
> In LogicalSlotAdvanceAndCheckReadynessForDecoding(), if we are synchronizing
> slots
> then I think that we can skip:
>
> + /*
> + * Wait for specified streaming replication standby servers
> (if any)
> + * to confirm receipt of WAL up to moveto lsn.
> + */
> + WaitForStandbyConfirmation(moveto);
>
> Indeed if we are dealing with synced slot then we know we're in
> RecoveryInProgress().
>
> Then there is no need to call WaitForStandbyConfirmation() as it could go
> until
> the RecoveryInProgress() in StandbySlotsHaveCaughtup() for nothing (as we
> already
> know it).
>
Won't it will normally return from the first check in
WaitForStandbyConfirmation() because standby_slot_names_config is not
set on standby?
> 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.
--
With Regards,
Amit Kapila.
