Re: Minimal logical decoding on standbys

On Thu, 23 May 2019 at 23:18, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-23 23:08:55 +0530, Amit Khandekar wrote:
> > On Thu, 23 May 2019 at 21:29, Andres Freund <andres@anarazel.de> wrote:
> > > On 2019-05-23 17:39:21 +0530, Amit Khandekar wrote:
> > > > On Tue, 21 May 2019 at 21:49, Andres Freund <andres@anarazel.de> wrote:
> > > > Yeah, I agree we should add such checks to minimize the possibility of
> > > > reading logical records from a master that has insufficient wal_level.
> > > > So to summarize :
> > > > a. CheckLogicalDecodingRequirements() : Add Controlfile wal_level checks
> > > > b. Call this function call in CreateInitDecodingContext() as well.
> > > > c. While decoding XLOG_PARAMETER_CHANGE record, emit recovery conflict
> > > > error if there is an existing logical slot.
> > > >
> > > > This made me think more of the race conditions. For instance, in
> > > > pg_create_logical_replication_slot(), just after
> > > > CheckLogicalDecodingRequirements and before actually creating the
> > > > slot, suppose concurrently Controlfile->wal_level is changed from
> > > > logical to replica.  So suppose a new slot does get created. Later the
> > > > slot is read, so in pg_logical_slot_get_changes_guts(),
> > > > CheckLogicalDecodingRequirements() is called where it checks
> > > > ControlFile->wal_level value. But just before it does that,
> > > > ControlFile->wal_level concurrently changes back to logical, because
> > > > of replay of another param-change record. So this logical reader will
> > > > think that the wal_level is sufficient, and will proceed to read the
> > > > records, but those records are *before* the wal_level change, so these
> > > > records don't have logical data.
> > >
> > > I don't think that's an actual problem, because there's no decoding
> > > before the slot exists and CreateInitDecodingContext() has determined
> > > the start LSN. And by that point the slot exists, slo
> > > XLOG_PARAMETER_CHANGE replay can error out.
> >
> > So between the start lsn and the lsn for
> > parameter-change(logical=>replica) record, there can be some records ,
> > and these don't have logical data. So the slot created will read from
> > the start lsn, and proceed to read these records, before reading the
> > parameter-change record.
>
> I don't think that's possible. By the time CreateInitDecodingContext()
> is called, the slot *already* exists (but in a state that'll cause it to
> be throw away on error). But the restart point has not yet been
> determined. Thus, if there is a XLOG_PARAMETER_CHANGE with a wal_level
> change it can error out. And to handle the race of wal_level changing
> between CheckLogicalDecodingRequirements() and the slot creation, we
> recheck in CreateInitDecodingContext().

ok, got it now. I was concerned that there might be some such cases
unhandled because we are not using locks to handle such concurrency
conditions. But as you have explained, the checks we are adding will
avoid this race condition.

>
> Think we might nee dto change ReplicationSlotReserveWal() to use the
> replay, rather than the redo pointer for logical slots though.

Not thought of this; will get back.

Working on the patch now ....

> > Are you saying we want to error out when the postgres replays the
> > param change record and there is existing logical slot ? I thought you
> > were suggesting earlier that it's the decoder.c code which should
> > error out when reading the param-change record.
>
> Yes, that's what I'm saying. See this portion of my previous email on
> the topic:
Yeah, thanks for pointing that.
>
> On 2019-05-21 09:19:37 -0700, Andres Freund wrote:
> > So I suspect we need conflict handling in xlog_redo's
> > XLOG_PARAMETER_CHANGE case. If we there check against existing logical
> > slots, we ought to be safe.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company