Re: Minimal logical decoding on standbys

On Mon, 24 Jun 2019 at 23:58, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>
> On Thu, 20 Jun 2019 at 00:31, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > > > Or else, do you think we can just increment the record pointer by
> > > > doing something like (lastReplayedEndRecPtr % XLOG_BLCKSZ) +
> > > > SizeOfXLogShortPHD() ?
> > >
> > > I found out that we can't do this, because we don't know whether the
> > > xlog header is SizeOfXLogShortPHD or SizeOfXLogLongPHD. In fact, in
> > > our context, it is SizeOfXLogLongPHD. So we indeed need the
> > > XLogReaderState handle.
> >
> > Well, we can determine whether a long or a short header is going to be
> > used, as that's solely dependent on the LSN:
>
> Discussion of this point (plus some more points) is in a separate
> reply. You can reply to my comments there :
> https://www.postgresql.org/message-id/CAJ3gD9f_HjQ6qP%3D%2B1jwzwy77fwcbT4-M3UvVsqpAzsY-jqM8nw%40mail.gmail.com
>

As you suggested, I have used XLogSegmentOffset() to know the header
size, and bumped the restart_lsn in ReplicationSlotReserveWal() rather
than DecodingContextFindStartpoint(). Like I mentioned in the above
link, I am not sure why it's not worth doing this like you said.

> >
> >
> > > - * That's not needed (or indeed helpful) for physical slots as they'll
> > > - * start replay at the last logged checkpoint anyway. Instead return
> > > - * the location of the last redo LSN. While that slightly increases
> > > - * the chance that we have to retry, it's where a base backup has to
> > > - * start replay at.
> > > + * None of this is needed (or indeed helpful) for physical slots as
> > > + * they'll start replay at the last logged checkpoint anyway. Instead
> > > + * return the location of the last redo LSN. While that slightly
> > > + * increases the chance that we have to retry, it's where a base backup
> > > + * has to start replay at.
> > > */
> > > +
> > > + restart_lsn =
> > > + (SlotIsPhysical(slot) ? GetRedoRecPtr() :
> > > + (RecoveryInProgress() ? GetXLogReplayRecPtr(NULL) :
> > > + GetXLogInsertRecPtr()));
> >
> > Please rewrite this to use normal if blocks.

Ok, done.

> > I'm also not convinced that
> > it's useful to have this if block, and then another if block that
> > basically tests the same conditions again.
>
> Will check and get back on this one.
Those conditions are not exactly same. restart_lsn is assigned three
different pointers depending upon three different conditions. And
LogStandbySnapshot() is to be done only for combination of two
specific conditions. So we need to have two different condition
blocks.
Also, it's better if we have the
"assign-slot-restart_lsn-under-spinlock" in a common code, rather than
repeating it in two different blocks.

We can do something like :

if (!RecoveryInProgress() && SlotIsLogical(slot))
{
restart_lsn = GetXLogInsertRecPtr();
/* Assign restart_lsn to slot restart_lsn under Spinlock */
/* Log standby snapshot and fsync to disk */
}
else
{
if (SlotIsPhysical(slot))
restart_lsn = GetRedoRecPtr();
else if (RecoveryInProgress())
restart_lsn = GetXLogReplayRecPtr(NULL);
else
restart_lsn = GetXLogInsertRecPtr();

/* Assign restart_lsn to slot restart_lsn under Spinlock */
}

But I think better/simpler thing would be to take out the
assign-slot-restart_lsn outside of the two condition blocks into a
common location, like this :

if (SlotIsPhysical(slot))
restart_lsn = GetRedoRecPtr();
else if (RecoveryInProgress())
restart_lsn = GetXLogReplayRecPtr(NULL);
else
restart_lsn = GetXLogInsertRecPtr();

/* Assign restart_lsn to slot restart_lsn under Spinlock */

if (!RecoveryInProgress() && SlotIsLogical(slot))
{
/ * Log standby snapshot and fsync to disk */
}

So in the updated patch (v10), I have done as above.