At Wed, 10 Jun 2020 15:53:53 +0900, Michael Paquier <michael@paquier.xyz> wrote in
> On Tue, Jun 09, 2020 at 09:01:13PM +0300, Alexey Kondratov wrote:
> > Yes, there was a ReplicationSlotsComputeRequiredLSN() call inside
> > pg_physical_replication_slot_advance() in the v5 of the patch:
> >
> > But later it has been missed and we have not noticed that.
> >
> > I think that adding it back as per attached will be enough.
Sure.
> [ scratches head... ]
> Indeed, this part gets wrong and we would have to likely rely on a WAL
> sender to do this calculation once a new flush location is received,
> but that may not happen in some cases. It feels more natural to do
> that in the location where the slot is marked as dirty, and there
> is no need to move around an extra check to see if the slot has
> actually been advanced or not. Or we could just call the routine once
> any advancing is attempted? That would be unnecessary if no advancing
> is done.
We don't call the function so frequently. I don't think it can be a
problem to update replicationSlotMinLSN every time trying advancing.
> > > I find it really depressing how much obviously untested stuff gets
> > > added in this area.
> >
> > Prior to this patch pg_replication_slot_advance was not being tested
> > at all.
> > Unfortunately, added tests appeared to be not enough to cover all
> > cases. It
> > seems that the whole machinery of WAL holding and trimming is worth
> > to be
> > tested more thoroughly.
>
> I think that it would be interesting if we had a SQL representation of
> the contents of XLogCtlData (wanted that a couple of times). Now we
> are actually limited to use a checkpoint and check that past segments
> are getting recycled by looking at the contents of pg_wal. Doing that
> here does not cause the existing tests to be much more expensive as we
> only need one extra call to pg_switch_wal(), mostly. Please see the
> attached.
The test in the patch looks fine to me and worked well for me.
Using smaller wal_segment_size (1(MB) worked for me) reduces the cost
of the check, but I'm not sure it's worth doing.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center