Обсуждение: Timeline following for logical slots

On 04/03/16 17:08, Craig Ringer wrote:
> I'd really appreciate some review of the logic there by people who know
> timelines well and preferably know the xlogreader. It's really just one
> function and 2/3 comments; the code is simple but the reasoning leading
> to it is not.
>

I think this will have to be work for committer at this point. I can't 
find any flaws in the logic myself so I unless somebody protests I am 
going to mark this as ready for committer.

> I've also attached an updated version of the tests posted a few days
> ago.  The tests depend on the remaining patches from the TAP
> enhancements tree so it's easiest to just get the whole tree from
> https://github.com/2ndQuadrant/postgres/tree/dev/logical-decoding-timeline-following
> (subject to regular rebases and force pushes, do not use as a base).
>
> The tests now include a test module that exposes some slots guts to SQL
> to allow the client to sync slot state from master to replica(s) without
> needing failover slots and the use of extra WAL as transport. It's very
> much for-testing-only.
>
> The new test module is used by a second round of tests to demonstrate
> the practicality of failover of a logical replication client to a
> physical replica using a base backup taken by pg_basebackup and without
> the presence of failover slots. I won't pretend it's pretty.
>

Well for testing purposes it's quite fine I think. The TAP framework 
enhancements needed for this are now in and it works correctly against 
current master.

--   Petr Jelinek                  http://www.2ndQuadrant.com/  PostgreSQL Development, 24x7 Support, Training &
Services

Petr Jelinek wrote:
> On 04/03/16 17:08, Craig Ringer wrote:
> >I'd really appreciate some review of the logic there by people who know
> >timelines well and preferably know the xlogreader. It's really just one
> >function and 2/3 comments; the code is simple but the reasoning leading
> >to it is not.
>
> I think this will have to be work for committer at this point. I can't find
> any flaws in the logic myself so I unless somebody protests I am going to
> mark this as ready for committer.

Great, thanks.  I've studied this to the point where I'm confident that
it makes sense, so I'm about to push it.  I didn't change any logic,
only updated comments here and there, both in the patch and in some
preexisting code.  I also changed the "List *timelineHistory" to be
#ifndef FRONTEND, which seems cleaner than having it and insist that it
couldn't be used.

Also, hopefully you don't have any future callers for the functions I
marked static (I checked the failover slots series and couldn't see
anything).  I will push this early tomorrow.

One thing I'm not quite sure about is why this is said to apply to
"logical slots" and not just to replication slots in general.  In what
sense does it not apply to physical replication slots?

(I noticed that we have this new line,
-        randAccess = true;
+        randAccess = true;      /* allow readPageTLI to go backwards */
in which now the comment is an identical copy of an identical line
elsewhere; however, randAccess doesn't actually affect readPageTLI in
any way, so AFAICS both comments are now wrong.)

> Well for testing purposes it's quite fine I think. The TAP framework
> enhancements needed for this are now in and it works correctly against
> current master.

Certainly the new src/test/recovery/t/006whatever.pl file is going to be
part of the commit.  What I'm not so sure about is
src/test/modules/decoding_failover: is there any reason we don't just
put the new functions in contrib/test_decoding?

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-03-14 20:10:58 -0300, Alvaro Herrera wrote:
> Great, thanks.  I've studied this to the point where I'm confident that
> it makes sense, so I'm about to push it.  I didn't change any logic,
> only updated comments here and there, both in the patch and in some
> preexisting code.  I also changed the "List *timelineHistory" to be
> #ifndef FRONTEND, which seems cleaner than having it and insist that it
> couldn't be used.

Could you perhaps wait till tomorrow? I'd like to do a pass over it.

Thanks

Andres

Hi,


On 2016-03-14 20:10:58 -0300, Alvaro Herrera wrote:
> diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
> index fcb0872..7b60b8f 100644
> --- a/src/backend/access/transam/xlogreader.c
> +++ b/src/backend/access/transam/xlogreader.c
> @@ -10,9 +10,11 @@
>   *
>   * NOTES
>   *        See xlogreader.h for more notes on this facility.
> + *
> + *        This file is compiled as both front-end and backend code, so it
> + *        may not use ereport, server-defined static variables, etc.
>   *-------------------------------------------------------------------------
>   */
> -

Huh?

>  #include "postgres.h"
>  
>  #include "access/transam.h"
> @@ -116,6 +118,11 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
>          return NULL;
>      }
>  
> +#ifndef FRONTEND
> +    /* Will be loaded on first read */
> +    state->timelineHistory = NIL;
> +#endif
> +
>      return state;
>  }
>  
> @@ -135,6 +142,10 @@ XLogReaderFree(XLogReaderState *state)
>      pfree(state->errormsg_buf);
>      if (state->readRecordBuf)
>          pfree(state->readRecordBuf);
> +#ifndef FRONTEND
> +    if (state->timelineHistory)
> +        list_free_deep(state->timelineHistory);
> +#endif

Hm. So we don't support timelines following for frontend code, although
it'd be rather helpful for pg_xlogdump. And possibly pg_rewind.


>      pfree(state->readBuf);
>      pfree(state);
>  }
> @@ -208,10 +219,11 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
>  
>      if (RecPtr == InvalidXLogRecPtr)
>      {
> +        /* No explicit start point; read the record after the one we just read */
>          RecPtr = state->EndRecPtr;
>  
>          if (state->ReadRecPtr == InvalidXLogRecPtr)
> -            randAccess = true;
> +            randAccess = true;    /* allow readPageTLI to go backwards */

randAccess is doing more than that, so I'm doubtful that comment is an
improvment.


>          /*
>           * RecPtr is pointing to end+1 of the previous WAL record.  If we're
> @@ -223,6 +235,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
>      else
>      {
>          /*
> +         * Caller supplied a position to start at.
> +         *
>           * In this case, the passed-in record pointer should already be
>           * pointing to a valid record starting position.
>           */
> @@ -309,8 +323,10 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
>          /* XXX: more validation should be done here */
>          if (total_len < SizeOfXLogRecord)
>          {
> -            report_invalid_record(state, "invalid record length at %X/%X",
> -                                  (uint32) (RecPtr >> 32), (uint32) RecPtr);
> +            report_invalid_record(state,
> +                        "invalid record length at %X/%X: wanted %lu, got %u",
> +                                  (uint32) (RecPtr >> 32), (uint32) RecPtr,
> +                                  SizeOfXLogRecord, total_len);
>              goto err;
>          }
>          gotheader = false;
> @@ -466,9 +482,7 @@ err:
>       * Invalidate the xlog page we've cached. We might read from a different
>       * source after failure.
>       */
> -    state->readSegNo = 0;
> -    state->readOff = 0;
> -    state->readLen = 0;
> +    XLogReaderInvalCache(state);

I don't think that "cache" is the right way to describe this.


>  #include <unistd.h>
>  
> -#include "miscadmin.h"
> -

spurious change imo.



>  /*
> - * TODO: This is duplicate code with pg_xlogdump, similar to walsender.c, but
> - * we currently don't have the infrastructure (elog!) to share it.
> + * Read 'count' bytes from WAL into 'buf', starting at location 'startptr'
> + * in timeline 'tli'.
> + *
> + * Will open, and keep open, one WAL segment stored in the static file
> + * descriptor 'sendFile'. This means if XLogRead is used once, there will
> + * always be one descriptor left open until the process ends, but never
> + * more than one.
> + *
> + * XXX This is very similar to pg_xlogdump's XLogDumpXLogRead and to XLogRead
> + * in walsender.c but for small differences (such as lack of elog() in
> + * frontend).  Probably these should be merged at some point.
>   */
>  static void
>  XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
> @@ -648,8 +657,12 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
>      XLogRecPtr    recptr;
>      Size        nbytes;
>  
> +    /*
> +     * Cached state across calls.
> +     */

One line?


>      static int    sendFile = -1;
>      static XLogSegNo sendSegNo = 0;
> +    static TimeLineID sendTLI = 0;
>      static uint32 sendOff = 0;
>  
>      p = buf;
> @@ -664,11 +677,12 @@ XLogRead(char *buf, TimeLineID tli, XLogRecPtr startptr, Size count)
>  
>          startoff = recptr % XLogSegSize;
>  
> -        if (sendFile < 0 || !XLByteInSeg(recptr, sendSegNo))
> +        /* Do we need to open a new xlog segment? */
> +        if (sendFile < 0 || !XLByteInSeg(recptr, sendSegNo) ||
> +            sendTLI != tli)
>          {

s/open a new/open a different/?  New imo has connotations that we don't
really want here.


>              char        path[MAXPGPATH];
>  
> -            /* Switch to another logfile segment */
>              if (sendFile >= 0)
>                  close(sendFile);

E.g. you could just have moved the above comment.


>          /* Need to seek in the file? */
>          if (sendOff != startoff)
>          {
>              if (lseek(sendFile, (off_t) startoff, SEEK_SET) < 0)
> -            {
> -                char        path[MAXPGPATH];
> -
> -                XLogFilePath(path, tli, sendSegNo);
> -
>                  ereport(ERROR,
>                          (errcode_for_file_access(),
>                    errmsg("could not seek in log segment %s to offset %u: %m",
> -                         path, startoff)));
> -            }
> +                         XLogFileNameP(tli, sendSegNo), startoff)));
>              sendOff = startoff;
>          }

Not a serious issue, more a general remark: I'm doubtful that going for
palloc in error situations is good practice. This will be allocated in
the current memory context; without access to the emergency error
reserves.


I'm also getting the feeling that the patch is bordering on doing some
relatively random cleanups mixed in with architectural changes. Makes
things a bit harder to review.


> +static void
> +XLogReadDetermineTimeline(XLogReaderState *state)
> +{
> +    /* Read the history on first time through */
> +    if (state->timelineHistory == NIL)
> +        state->timelineHistory = readTimeLineHistory(ThisTimeLineID);
> +
> +    /*
> +     * Are we reading the record immediately following the one we read last
> +     * time?  If not, then don't use the cached timeline info.
> +     */
> +    if (state->currRecPtr != state->EndRecPtr)
> +    {
> +        state->currTLI = 0;
> +        state->currTLIValidUntil = InvalidXLogRecPtr;
> +    }


Hm. So we grow essentially a second version of the last end position and
the randAccess stuff in XLogReadRecord().


> +    if (state->currTLI == 0)
> +    {
> +        /*
> +         * Something changed; work out what timeline this record is on. We
> +         * might read it from the segment on this TLI or, if the segment is
> +         * also contained by newer timelines, the copy from a newer TLI.
> +         */
> +        state->currTLI = tliOfPointInHistory(state->currRecPtr,
> +                                             state->timelineHistory);
> +
> +        /*
> +         * Look for the most recent timeline that's on the same xlog segment
> +         * as this record, since that's the only one we can assume is still
> +         * readable.
> +         */
> +        while (state->currTLI != ThisTimeLineID &&
> +               state->currTLIValidUntil == InvalidXLogRecPtr)
> +        {
> +            XLogRecPtr    tliSwitch;
> +            TimeLineID    nextTLI;
> +
> +            tliSwitch = tliSwitchPoint(state->currTLI, state->timelineHistory,
> +                                       &nextTLI);
> +
> +            /* round ValidUntil down to start of seg containing the switch */
> +            state->currTLIValidUntil =
> +                ((tliSwitch / XLogSegSize) * XLogSegSize);
> +
> +            if (state->currRecPtr >= state->currTLIValidUntil)
> +            {
> +                /*
> +                 * The new currTLI ends on this WAL segment so check the next
> +                 * TLI to see if it's the last one on the segment.
> +                 *
> +                 * If that's the current TLI we'll stop searching.

I don't really understand how we're stopping searching here?

> +                 */
> +                state->currTLI = nextTLI;
> +                state->currTLIValidUntil = InvalidXLogRecPtr;
> +            }
> +        }
> +}


XLogReadDetermineTimeline() doesn't sit quite right with me, I do wonder
whether there's not a simpler way to write this.


> +/*
> + * XLogPageReadCB callback for reading local xlog files
>   *
>   * Public because it would likely be very helpful for someone writing another
>   * output method outside walsender, e.g. in a bgworker.
>   *
> - * TODO: The walsender has it's own version of this, but it relies on the
> + * TODO: The walsender has its own version of this, but it relies on the
>   * walsender's latch being set whenever WAL is flushed. No such infrastructure
>   * exists for normal backends, so we have to do a check/sleep/repeat style of
>   * loop for now.
> @@ -754,46 +897,88 @@ int
>  read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
>      int reqLen, XLogRecPtr targetRecPtr, char *cur_page, TimeLineID *pageTLI)
>  {
> -    XLogRecPtr    flushptr,
> +    XLogRecPtr    read_upto,
>                  loc;
>      int            count;
>  
>      loc = targetPagePtr + reqLen;
> +
> +    /* Make sure enough xlog is available... */
>      while (1)
>      {
>          /*
> -         * TODO: we're going to have to do something more intelligent about
> -         * timelines on standbys. Use readTimeLineHistory() and
> -         * tliOfPointInHistory() to get the proper LSN? For now we'll catch
> -         * that case earlier, but the code and TODO is left in here for when
> -         * that changes.
> +         * Check which timeline to get the record from.
> +         *
> +         * We have to do it each time through the loop because if we're in
> +         * recovery as a cascading standby, the current timeline might've
> +         * become historical.
>           */
> -        if (!RecoveryInProgress())
> +        XLogReadDetermineTimeline(state);
> +
> +        if (state->currTLI == ThisTimeLineID)
>          {
> -            *pageTLI = ThisTimeLineID;
> -            flushptr = GetFlushRecPtr();
> +            /*
> +             * We're reading from the current timeline so we might have to
> +             * wait for the desired record to be generated (or, for a standby,
> +             * received & replayed)
> +             */
> +            if (!RecoveryInProgress())
> +            {
> +                *pageTLI = ThisTimeLineID;
> +                read_upto = GetFlushRecPtr();
> +            }
> +            else
> +                read_upto = GetXLogReplayRecPtr(pageTLI);
> +
> +            if (loc <= read_upto)
> +                break;
> +
> +            CHECK_FOR_INTERRUPTS();
> +            pg_usleep(1000L);
>          }
>          else
> -            flushptr = GetXLogReplayRecPtr(pageTLI);
> +        {
> +            /*
> +             * We're on a historical timeline, so limit reading to the switch
> +             * point where we moved to the next timeline.
> +             */
> +            read_upto = state->currTLIValidUntil;

Hm. Is it ok to not check GetFlushRecPtr/GetXLogReplayRecPtr() here? If
so, how come?

> -        if (loc <= flushptr)
> +            /*
> +             * Setting pageTLI to our wanted record's TLI is slightly wrong;
> +             * the page might begin on an older timeline if it contains a
> +             * timeline switch, since its xlog segment will have been copied
> +             * from the prior timeline. This is pretty harmless though, as
> +             * nothing cares so long as the timeline doesn't go backwards.  We
> +             * should read the page header instead; FIXME someday.
> +             */
> +            *pageTLI = state->currTLI;
> +
> +            /* No need to wait on a historical timeline */
>              break;
> -
> -        CHECK_FOR_INTERRUPTS();
> -        pg_usleep(1000L);
> +        }
>      }
>  
> -    /* more than one block available */
> -    if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
> +    if (targetPagePtr + XLOG_BLCKSZ <= read_upto)
> +    {
> +        /*
> +         * more than one block available; read only that block, have caller
> +         * come back if they need more.
> +         */
>          count = XLOG_BLCKSZ;
> -    /* not enough data there */
> -    else if (targetPagePtr + reqLen > flushptr)
> +    }
> +    else if (targetPagePtr + reqLen > read_upto)
> +    {
> +        /* not enough data there */
>          return -1;
> -    /* part of the page available */
> +    }
>      else
> -        count = flushptr - targetPagePtr;
> +    {
> +        /* enough bytes available to satisfy the request */
> +        count = read_upto - targetPagePtr;
> +    }
>  
> -    XLogRead(cur_page, *pageTLI, targetPagePtr, XLOG_BLCKSZ);
> +    XLogRead(cur_page, *pageTLI, targetPagePtr, count);

When are we reading less than a page? That should afaik never be required.


> +        /*
> +         * We start reading xlog from the restart lsn, even though in
> +         * CreateDecodingContext we set the snapshot builder up using the
> +         * slot's candidate_restart_lsn. This means we might read xlog we
> +         * don't actually decode rows from, but the snapshot builder might
> +         * need it to get to a consistent point. The point we start returning
> +         * data to *users* at is the candidate restart lsn from the decoding
> +         * context.
> +         */

Uh? Where are we using candidate_restart_lsn that way? I seriously doubt
it is - candidate_restart_lsn is about a potential future restart_lsn,
which we can set once we get reception confirmation from the client.


> @@ -299,6 +312,18 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
>              CHECK_FOR_INTERRUPTS();
>          }
>  
> +        /* Make sure timeline lookups use the start of the next record */
> +        startptr = ctx->reader->EndRecPtr;

Huh? startptr isn't used after this, so I'm not sure what this even
means?

> +        /*
> +         * The XLogReader will read a page past the valid end of WAL because
> +         * it doesn't know about timelines. When we switch timelines and ask
> +         * it for the first page on the new timeline it will think it has it
> +         * cached, but it'll have the old partial page and say it can't find
> +         * the next record. So flush the cache.
> +         */
> +        XLogReaderInvalCache(ctx->reader);
> +

dito.


> diff --git a/src/test/modules/decoding_failover/decoding_failover.c
b/src/test/modules/decoding_failover/decoding_failover.c
> new file mode 100644
> index 0000000..669e6c4
> --- /dev/null
> +++ b/src/test/modules/decoding_failover/decoding_failover.c

> +
> +/*
> + * Create a new logical slot, with invalid LSN and xid, directly. This does not
> + * use the snapshot builder or logical decoding machinery. It's only intended
> + * for creating a slot on a replica that mirrors the state of a slot on an
> + * upstream master.
> + *
> + * You should immediately decoding_failover_advance_logical_slot(...) it
> + * after creation.
> + */

Uh. I doubt we want this, even if it's formally located in
src/test/modules.  These comments make it appear not to be only intended
for that, and I have serious doubts about the validity of the concept as
is.


This seems to need some more polishing.


Greetings,

Andres Freund

Andres Freund wrote:

> Could you perhaps wait till tomorrow? I'd like to do a pass over it.

Sure thing.  I'll wait for your review.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Craig already replied to most points.  I think we can sum up as "we've
done the best we can with what we have, maybe somebody can figure how to
do better in the future but for now this works."

Andres Freund wrote:

> On 2016-03-14 20:10:58 -0300, Alvaro Herrera wrote:

> > +#ifndef FRONTEND
> > +    if (state->timelineHistory)
> > +        list_free_deep(state->timelineHistory);
> > +#endif
> 
> Hm. So we don't support timelines following for frontend code, although
> it'd be rather helpful for pg_xlogdump. And possibly pg_rewind.

Exactly.  I had the same comment, but the argument that we would need to
rewrite a lot of timeline.c won me.  That can wait for a future patch;
we certainly don't want to be rewriting these things during the last CF.

> >          if (state->ReadRecPtr == InvalidXLogRecPtr)
> > -            randAccess = true;
> > +            randAccess = true;    /* allow readPageTLI to go backwards */
> 
> randAccess is doing more than that, so I'm doubtful that comment is an
> improvment.

As I said, it's only a copy of what's already a few lines above.  I
would be happier removing both, and instead adding an explanatory
comment about that variable elsewhere.

> >  #include <unistd.h>
> >  
> > -#include "miscadmin.h"
> > -
> 
> spurious change imo.

Yeah, in a way this is cleanup after previous patches that have messed
up things somewhat.  I wouldn't have posted it unless I was close to
committing this ... I think it'd be better to push these cleanup changes
separately.

> >          /* Need to seek in the file? */
> >          if (sendOff != startoff)
> >          {
> >              if (lseek(sendFile, (off_t) startoff, SEEK_SET) < 0)
> > -            {
> > -                char        path[MAXPGPATH];
> > -
> > -                XLogFilePath(path, tli, sendSegNo);
> > -
> >                  ereport(ERROR,
> >                          (errcode_for_file_access(),
> >                    errmsg("could not seek in log segment %s to offset %u: %m",
> > -                         path, startoff)));
> > -            }
> > +                         XLogFileNameP(tli, sendSegNo), startoff)));
> >              sendOff = startoff;
> >          }
> 
> Not a serious issue, more a general remark: I'm doubtful that going for
> palloc in error situations is good practice. This will be allocated in
> the current memory context; without access to the emergency error
> reserves.

Yeah, this code was copied from elsewhere in 422a55a68784f but there is
already another version of this routine in walsender which is using
XLogFileNameP instead of the stack-allocated one.  I just made this one
more similar to the walsender's (which is in backend environment just
like this one) instead of continuing to use the frontend-limited
version (which is where this one was copied from).

I'm having a hard time getting concerned about using palloc there,
considering that every single call of XLogFileNameP occurs in an error
message.  If we want to reject this one, surely we should change all the
others too.

> I'm also getting the feeling that the patch is bordering on doing some
> relatively random cleanups mixed in with architectural changes. Makes
> things a bit harder to review.

Yes.

> > +static void
> > +XLogReadDetermineTimeline(XLogReaderState *state)
> > +{
> > +    /* Read the history on first time through */
> > +    if (state->timelineHistory == NIL)
> > +        state->timelineHistory = readTimeLineHistory(ThisTimeLineID);
> > +
> > +    /*
> > +     * Are we reading the record immediately following the one we read last
> > +     * time?  If not, then don't use the cached timeline info.
> > +     */
> > +    if (state->currRecPtr != state->EndRecPtr)
> > +    {
> > +        state->currTLI = 0;
> > +        state->currTLIValidUntil = InvalidXLogRecPtr;
> > +    }
> 
> Hm. So we grow essentially a second version of the last end position and
> the randAccess stuff in XLogReadRecord().

Craig's argument against that seems reasonable to me.

> XLogReadDetermineTimeline() doesn't sit quite right with me, I do wonder
> whether there's not a simpler way to write this.

Feel free to suggest something :-)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-03-15 21:04:12 +0800, Craig Ringer wrote:
> Thanks very much for the review.

Are you planning to update the patch?

- Andres

Craig Ringer wrote:

> It removes the questionable cleanups, fixes the randAccess comment (IMO),

I pushed cleanup separately, including the addition of a few comments
that were documenting the original code.  I actually made a mistake in
extracting those, so there's one comment that's actually bogus in that
commit (the candidate_restart_lsn one); it's fixed in the second commit.
Sorry about that.  I also introduced XLogReaderInvalReadState here,
called XLogReaderInvalCache originally, since Andres objected to the
"cache" terminology (though you will note that the word "cache" was used
in the original code too.)

>  changes the "open a new xlog segment" wording and explains why we don't
> need GetFlushRecPtr/GetXLogReplayRecPtr on replay of a historical timeline.
> I removed the change that did less than a whole page at the XLogRead call
> and instead added a comment explaining it. Fixed the brainfrart with
> candidate_restart_lsn, removed the outdated startptr comment and update and
> the unnecessary XLogReaderInvalCache call after it.
> 
> Also renamed src/test/modules/decoding_failover to
> src/test/modules/test_slot_timelines . Best name I could come up with that
> wasn't 1000chars long.
> 
> Passes main 'check', contrib/test_decoding check,
> src/test/modules/test_slot_timelines check and src/test/recovery check.
> 
> Available attached or at
> https://github.com/2ndQuadrant/postgres/tree/dev/logical-decoding-timeline-following

And pushed this too.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

On 2016-03-31 08:52:34 +0800, Craig Ringer wrote:
> On 31 March 2016 at 07:15, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> 
> 
> > > Available attached or at
> > >
> > https://github.com/2ndQuadrant/postgres/tree/dev/logical-decoding-timeline-following
> >
> > And pushed this too.
> >
> 
> Much appreciated. Marked as committed at
> https://commitfest.postgresql.org/9/568/ .
> 
> This gives us an option for failover of logical replication in 9.6, even if
> it's a bit cumbersome and complex for the client, in case failover slots
> don't make the cut. And, of course, it's a pre-req for failover slots,
> which I'll rebase on top of it shortly.

FWIW, I think it's dangerous to use this that way. If people manipulate
slots that way we'll have hellishly to debug issues. The test code needs
a big disclaimer to never ever be used in production, and we should
"disclaim any warranty" if somebody does that. To the point of not
fixing issues around it in back branches.

> Andres, I tried to address your comments as best I could. The main one that
> I think stayed open was about the loop that finds the last timeline on a
> segment. If you think that's better done by directly scanning the List* of
> timeline history entries I'm happy to prep a follow-up.

Have to look again.

+        * We start reading xlog from the restart lsn, even though in
+        * CreateDecodingContext we set the snapshot builder up using the
+        * slot's confirmed_flush. This means we might read xlog we don't
+        * actually decode rows from, but the snapshot builder might need it
+        * to get to a consistent point. The point we start returning data to
+        * *users* at is the confirmed_flush lsn set up in the decoding
+        * context.
+        */
still seems pretty misleading - and pretty much unrelated to the
callsite.


Greetings,

Andres Freund

Hi,

On 2016-04-01 13:16:18 +0800, Craig Ringer wrote:
> I think it's pretty unsafe from SQL, to be sure.
> 
> Unless failover slots get in to 9.6 we'll need to do exactly that from
> internal C stuff in pglogical to support following physical failover,

I know. And this makes me scared shitless.


I'll refuse to debug anything related to decoding, when this "feature"
has been used. And I think there'll be plenty; if you notice the issues
that is.


The more I think about it, the more I think we should rip this out
again, until we have the actual feature is in. With proper review.


> However: It's safe for the slot state on the replica to be somewhat behind
> the local replay from the master, so the slot state on the replica is older
> than what it would've been at an equivalent time on the master... so long
> as it's not so far behind that the replica has replayed vacuum activity
> that removes rows still required by the slot's declared catalog_xmin. Even
> then it should actually be OK in practice because the failing-over client
> will always have replayed past that point on the master (otherwise
> catalog_xmin couldn't have advanced on the master), so it'll always ask to
> start replay at a point at or after the lsn where the catalog_xmin was
> advanced to its most recent value on the old master before failover. It's
> only safe for walsender based decoding though, since there's no way to
> specify the startpoint for sql-level decoding.

This whole logic fails entirely flats on its face by the fact that even
if you specify a startpoint, we read older WAL and process the
records. The startpoint filters every transaction that commits earlier
than the "startpoint". But with a long-running transaction you still
will have old changes being processed, which need the corresponding
catalog state.


> My only real worry there is whether invalidations are a concern - if
> internal replay starts at the restart_lsn and replays over part of history
> where the catalog entries have been vacuumed before it starts collecting
> rows for return to the decoding client at the requested decoding
> startpoint, can that cause problems with invalidation processing?

Yes.


> It's also OK for the slot state to be *ahead* of the replica's replay
> position, i.e. from the future. restart_lsn and catalog_xmin will be higher
> than they were on the master at the same point in time, but that doesn't
> matter at all, since the client will always be requesting to start from
> that point or later, having replayed up to there on the old master before
> failover.

I don't think that's true. See my point above about startpoint.



> > Andres, I tried to address your comments as best I could. The main one
> > that
> > > I think stayed open was about the loop that finds the last timeline on a
> > > segment. If you think that's better done by directly scanning the List*
> > of
> > > timeline history entries I'm happy to prep a follow-up.
> >
> > Have to look again.
> >
> > +        * We start reading xlog from the restart lsn, even though in
> > +        * CreateDecodingContext we set the snapshot builder up using the
> > +        * slot's confirmed_flush. This means we might read xlog we don't
> > +        * actually decode rows from, but the snapshot builder might need
> > it
> > +        * to get to a consistent point. The point we start returning data
> > to
> > +        * *users* at is the confirmed_flush lsn set up in the decoding
> > +        * context.
> > +        */
> > still seems pretty misleading - and pretty much unrelated to the
> > callsite.
> >
> 
> 
> I think it's pretty confusing that the function uses's the slot's
> restart_lsn and never refers to confirmed_flush which is where it actually
> starts decoding from as far as the user's concerned. It took me a while to
> figure out what was going on there.

That's a fundamental misunderstanding on your part (perhaps created by
imprecise docs). The startpoint isn't about where to start
decoding. It's about skip calling the output plugin of a transaction if
it *commits* before the startpoint. We almost always will *decode* rows
from before the startpoint.  And that's pretty much unavoidable: The
consumer of a decoding slot only really can do anything with commit LSNs
wrt replay progress: But for a remembered progress LSN there can be a
lot of in-progress xacts (about which said client doesn't know
anything); and they start *earlier* than the commit LSN of the just
replayed xact. So we have to be able to re-process their state and send
it out.


> I think the comment would be less necessary, and could be moved up to the
> CreateDecodingContext call, if we passed the slot's confirmed_flush
> explicitly to CreateDecodingContext instead of passing InvalidXLogRecPtr
> and having the CreateDecodingContext call infer the start point. That way
> what's going on would be a lot clearer when reading the code.

How would that solve anything? We still need to process the old records,
hence the xlogreader needs to instructed to read them. Hence
logicalfuncs.c needs to know about it.


> You're thinking from the perspective of someone who knows this code
> intimately.

Maybe. But that's not addressed by adding half-true comments to places
they only belong to peripherally. And not to all the relevant places,
since you've not added the same comment to StartLogicalReplication().


> Speaking of which, did you see the proposed README I sent for
> src/backend/replication/logical ?

I skimmed it. But given we have a CF full of patches, some submitted
over a year ago, it seems unfair to spend time on a patch submitted a
few days ago.


Greetings,

Andres Freund

Hi,

On 2016-04-01 08:46:01 +0200, Andres Freund wrote:
> That's a fundamental misunderstanding on your part (perhaps created by
> imprecise docs).

> > Speaking of which, did you see the proposed README I sent for
> > src/backend/replication/logical ?
> 
> I skimmed it. But given we have a CF full of patches, some submitted
> over a year ago, it seems unfair to spend time on a patch submitted a
> few days ago.

For that purpos

WRT design readme, it might be interesting to look at 0009 in
http://archives.postgresql.org/message-id/20140127162006.GA25670%40awork2.anarazel.de

That's not up2date obviously, but it still might help.

Andres

On 2016-04-04 14:24:52 +0800, Craig Ringer wrote:
> I don't feel like I've grasped this properly yet. I think it's referring to
> the pg_logical/snapshots/ serialization, the use of which allows us to
> avoid doing extra work in SnapBuildFindSnapshot(...), but doesn't seem to
> be crucial for correct function. After all, decoding still restarts at the
> restart_lsn and feeds relevant xact info into the snapshot builder,
> accumulates invalidation information, etc.

restart_lsn is set to the last known point where a) all changes for
ongoing transactions are available b) we can re-build visiblity
information when we start reading from there.

As we essentially can only start determining visibility information
whenever processing a xl_running_xacts record. Building visibility
information means that there has to be a xl_running_xacts to start
from. To build full visibility information we also have to wait till we
have seen all in-progress transactions finish. So we dump visibility
information every now and then, so we can re-use the information we'd
already assembled.

Greetings,

Andres Freund

On 2016-04-04 14:18:53 +0800, Craig Ringer wrote:
> I want to go over the rest of your reply in more detail, but regarding this
> and the original comment, I'm going by:
> 
>     if (start_lsn == InvalidXLogRecPtr)
>     {
>         /* continue from last position */
>         start_lsn = slot->data.confirmed_flush;
>     }
>     ....
>     ctx = StartupDecodingContext(output_plugin_options,
>                                  start_lsn, InvalidTransactionId,
>                                  read_page, prepare_write, do_write);
> 
> ...  in CreateDecodingContext(), which in turn does ...
> 
>     ctx->snapshot_builder =
>         AllocateSnapshotBuilder(ctx->reorder, xmin_horizon, start_lsn);
> 
> ... in StartupDecodingContext(...).
> 
> The snapshot builder controls when the snapshot builder returns
> SNAPBUILD_CONSISTENT, therefore when we start returning decoded commits to
> the client. Right?

> We pass InvalidXLogRecPtr as the start_lsn
> in pg_logical_slot_get_changes_guts(...).
> 
> So, when I wrote that:
> 
>         /*
>          * We start reading xlog from the restart lsn, even though in
>          * CreateDecodingContext we set the snapshot builder up using the
>          * slot's confirmed_flush. This means we might read xlog we don't
>          * actually decode rows from, but the snapshot builder might need it
>          * to get to a consistent point. The point we start returning data
> to
>          * *users* at is the confirmed_flush lsn set up in the decoding
>          * context.
>          */
> 
> I don't see what's wrong there at all.

They're independent. What the snapshot builder gets as the start
position isn't the same as where we start reading WAL.


> We do "start reading xlog from the slot's restart_lsn". That's what gets
> passed to set up the xlogreader. That's where we read the first xlog
> records from.
> 
> In CreateDecodingContext we _do_ "set the snapshot builder up using the
> slot's confirmed_flush" [unless overridden by explicit argument to
> CreateDecodingContext, which isn't given here].

Yes. But again, that hasn't really got anything to do with where we read
WAL from. We always have to read older WAL than were confirmed_flush is
set to; to assemble all the changes from transactions that are
in-progress at the point of confirmed_flush.


> This is presumably what you're taking issue with:
> 
>          * This means we might read xlog we don't
>          * actually decode rows from, but the snapshot builder might need it
>          * to get to a consistent point.
> 
> and would be better worded as something like:
> 
> This means we will read and decode xlog from before the point we actually
> start returning decoded commits to the client, but the snapshot builder may
> need this additional xlog to get to a consistent point.

> right?

No. The snapshot builder really hasn't gotten that much to do with
things. The fundamental thing we have to do is re-assemble all
in-progress transactions (as in reorderbuffer.c, not snapbuild.c).


> I think
> 
>             The point we start returning data to
>          * *users* at is the confirmed_flush lsn set up in the decoding
>          * context.
> 
> is still right.
> 
> What I was proposing instead is, in pg_logical_slot_get_changes_guts,
> changing:
> 
>         ctx = CreateDecodingContext(InvalidXLogRecPtr,
>                                     options,
>                                     logical_read_local_xlog_page,
>                                     LogicalOutputPrepareWrite,
>                                     LogicalOutputWrite);
> 
> so that instead of InvalidXLogRecPtr we explicitly pass
> 
>    MyReplicationSlot->data.confirmed_flush;
> 
> thus making it way easier to see what's going on without having to chase
> way deeper to figure out why data isn't returned from the restart_lsn.
> Where, y'know, you'd expect decoding to restart from... and where it does
> restart from, just not where it resumes sending output to the client from.

I just don't buy this. It's absolutely fundamental to understand that
the commit LSN isn't the point where all the data of transactions is
stored.


Andres

On 2016-04-04 14:36:29 +0800, Craig Ringer wrote:
> On 4 April 2016 at 14:30, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2016-04-04 14:24:52 +0800, Craig Ringer wrote:
> > > I don't feel like I've grasped this properly yet. I think it's referring
> > to
> > > the pg_logical/snapshots/ serialization, the use of which allows us to
> > > avoid doing extra work in SnapBuildFindSnapshot(...), but doesn't seem to
> > > be crucial for correct function. After all, decoding still restarts at
> > the
> > > restart_lsn and feeds relevant xact info into the snapshot builder,
> > > accumulates invalidation information, etc.
> >
> > restart_lsn is set to the last known point where a) all changes for
> > ongoing transactions are available b) we can re-build visiblity
> > information when we start reading from there.
> >
> > As we essentially can only start determining visibility information
> > whenever processing a xl_running_xacts record. Building visibility
> > information means that there has to be a xl_running_xacts to start
> > from. To build full visibility information we also have to wait till we
> > have seen all in-progress transactions finish. So we dump visibility
> > information every now and then, so we can re-use the information we'd
> > already assembled.
> >
> 
> OK, makes sense. And still resume decoding from restart_lsn, despite having
> that visibility information stashed, because we also have to rebuild the
> information on invalidations for running xacts, which is not stored
> persistently anywhere as decoding progresses. So for now at least it's an
> optimisation to store the visibility info, since we still have go go back
> and decode for invalidations anyway. Right?

Not really no. The important point isn't invalidation or anything. It's
that we don't have the content & metadata of the transactions
themselves.  Yes, we also do re-read invalidations, but that's just a
side effect.

On 2016-04-04 17:50:02 +0800, Craig Ringer wrote:
> To rephrase per my understanding: The client only specifies the point it
> wants to start seeing decoded commits. Decoding starts from the slot's
> restart_lsn, and that's the point from which the accumulation of reorder
> buffer contents begins, the snapshot building process begins, and where
> accumulation of relcache invalidation information begins. At restart_lsn no
> xact that is to be emitted to the client may yet be in progress. Decoding,
s/yet/already/
> whether or not the xacts will be fed to the output plugin callbacks,
> requires access to the system catalogs. Therefore catalog_xmin reported by
> the slot must be >= the real effective catalog_xmin of the heap and valid
> at the restart_lsn, not just the confirmed flush point or the point the
> client specifies to resume fetching changes from.

Hm. Maybe I'm misunderstanding you here, but doesn't it have to be <=?

> On the original copy of the slot on the pre-failover master the restart_lsn
> would've been further ahead, as would the catalog_xmin. So catalog rows
> have been purged.
+may


> So it's necessary to ensure that the slot's restart_lsn and catalog_xmin
> are advanced in a timely, consistent manner on the replica's copy of the
> slot at a point where no vacuum changes to the catalog that could remove
> needed tuples have been replayed.

Right.


> The only way I can think of to do that really reliably right now, without
> full failover slots, is to use the newly committed pluggable WAL mechanism
> and add a hook to SaveSlotToPath() so slot info can be captured, injected
> in WAL, and replayed on the replica.

I personally think the primary answer is to use separate slots on
different machines. Failover slots can be an extension to that at some
point, but I think they're a secondary goal.


> It'd also be necessary to move
> CheckPointReplicationSlots() out of CheckPointGuts()  to the start of a
> checkpoint/restartpoint when WAL writing is still permitted, like the
> failover slots patch does.

Ugh. That makes me rather wary.


> Basically, failover slots as a plugin using a hook, without the
> additions to base backup commands and the backup label.

I'm going to be *VERY* hard to convince that adding a hook inside
checkpointing code is acceptable.


> I'd really hate 9.6 to go out with - still - no way to use logical decoding
> in a basic, bog-standard HA/failover environment. It overwhelmingly limits
> their utility and it's becoming a major drag on practical use of the
> feature. That's a difficulty given that the failover slots patch isn't
> especially trivial and you've shown that lazy sync of slot state is not
> sufficient.

I think the right way to do this is to focus on failover for logical
rep, with separate slots. The whole idea of integrating this physical
rep imo makes this a *lot* more complex than necessary. Not all that
many people are going to want to physical rep and logical rep.


> The restart_lsn from the newer copy of the slot is, as you said, a point we
> know we can reconstruct visibility info.

We can on the master. There's absolutely no guarantee that the
associated serialized snapshot is present on the standby.


Andres

On 2016-04-04 22:59:41 +0800, Craig Ringer wrote:
> Assuming that here you mean separate slots on different machines
> replicating via physical rep:

No,  I don't.

> We don't currently allow the creation of a logical slot on a standby. Nor
> replay from it, even to advance it without receiving the decoded
> changes.

Yes. I know.


> > I think the right way to do this is to focus on failover for logical
> > rep, with separate slots. The whole idea of integrating this physical
> > rep imo makes this a *lot* more complex than necessary. Not all that
> > many people are going to want to physical rep and logical rep.

> If you're saying we should focus on failover between nodes that're
> themselves connected using logical replication rather than physical
> replication, I really have to strongly disagree.
> 
> TL;DR for book-length below: We're a long, long way from being able to
> deliver even vaguely decent logical rep based failover.

I don't buy that.


> * Robust sequence decoding and replication. If you were following the later
> parts of that discussion you will've seen how fun that's going to be, but
> it's the simplest of all of the problems.

Unconvinced. People used londiste and slony for years without that, and
it's not even remotely at the top of the list of problems with either.


> * Logical decoding and sending of in-progress xacts, so the logical client
> can already be most of the way through receiving a big xact when it
> commits. Without this we have a huge lag spike whenever a big xact happens,
> since we must first finish decoding it in to a reorder buffer and can only
> then *begin* to send it to the client. During which time no later xacts may
> be decoded or replayed to the client. If you're running that rare thing,
> the perfect pure OLTP system, you won't care... but good luck finding one
> in the real world.

So? If you're using logical rep, you've always have that.


> * Either parallel apply on the client side or at least buffering of
> in-progress xacts on the client side so they can be safely flushed to disk
> and confirmed, allowing receive to continue while replay is done on the
> client. Otherwise sync rep is completely impractical... and there's no
> shortage of systems out there that can't afford to lose any transactions.
> Or at least have some crucial transactions they can't lose.

That's more or less trivial to implement. In an extension.


> * Robust, seamless DDL replication, so things don't just break randomly.
> This makes the other points above look nice and simple by comparison.

We're talking about a system which involves logical decoding. Whether
you have failover via physical rep or not, doesn't have anything to do
with this point.


> Physical rep *works*. Robustly. Reliably. With decent performance. It's
> proven. It supports sync rep. I'm confident telling people to use it.

Sure? And nothing prevents you from using it.


> I don't think there's any realistic way we're going to get there for
> logical rep in 9.6+n for n<2 unless a whole lot more people get on board
> and work on it. Even then.

I think the primary problem here is that you're focusing on things that
just are not very interesting for the majority of users, and which thus
won't get very enthusastic help.  The way to make progress is to get
something basic in, and then iterate from there.  Instead you're
focussing on the fringes; which nobody cares about, because the basics
aren't there.

FWIW, I plan to aggressively work on in-core (9.7) logical rep starting
in a few weeks. If we can coordinate on that end, I'm very happy, if not
then not.

> Right now we can deliver logical failover for DBs that:
> (b) don't do DDL, ever, or only do some limited DDL via direct admin
> commands where they can call some kind of helper function to queue and
> apply the DDL;
> (c) don't do big transactions or don't care about unbounded lag;
> (d) don't need synchronous replication or don't care about possibly large
> delays before commit is confirmed;
> (e) only manage role creation (among other things) via very strict
> processes that can be absolutely guaranteed to run on all nodes

And just about nothing of that has to do with any of the recent patches
send towards logical rep. I agree about the problems, but you should
attack them, instead of building way too complicated workarounds which
barely anybody will find interesting.


> ... which in my view isn't a great many databases.

Meh^2. Slony and londiste have these problems. And the reason they're
not used isn't primarily those, but that they're external and waaayyy to
complicated to use.


> Physical rep has *none* of those problems. (Sure, it has others, but we're
> used to them).

So?


-Andres

On Mon, Apr 4, 2016 at 10:59 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> To allow logical rep and failover to be a reasonable substitute for physical
> rep and failover IMO *need*:
>
> * Robust sequence decoding and replication. If you were following the later
> parts of that discussion you will've seen how fun that's going to be, but
> it's the simplest of all of the problems.
>
> * Logical decoding and sending of in-progress xacts, so the logical client
> can already be most of the way through receiving a big xact when it commits.
> Without this we have a huge lag spike whenever a big xact happens, since we
> must first finish decoding it in to a reorder buffer and can only then
> *begin* to send it to the client. During which time no later xacts may be
> decoded or replayed to the client. If you're running that rare thing, the
> perfect pure OLTP system, you won't care... but good luck finding one in the
> real world.
>
> * Either parallel apply on the client side or at least buffering of
> in-progress xacts on the client side so they can be safely flushed to disk
> and confirmed, allowing receive to continue while replay is done on the
> client. Otherwise sync rep is completely impractical... and there's no
> shortage of systems out there that can't afford to lose any transactions. Or
> at least have some crucial transactions they can't lose.
>
> * Robust, seamless DDL replication, so things don't just break randomly.
> This makes the other points above look nice and simple by comparison.
> Logical decoding of 2PC xacts with DDL would help here, as would the ability
> to transparently convert an xact into a prepare-xact on client commit and
> hold the client waiting while we replicate it, confirm the successful
> prepare on the replica, then commit prepared on the upstream.
>
> * oh, and some way to handle replication of shared catalog changes like
> pg_authid, so the above DDL replication doesn't just randomly break if it
> happens to refer to a global object that doesn't exist on the downstream.

In general, I think we'd be a lot better off if we got some kind of
logical replication into core first and then worked on lifting these
types of limitations afterwards.  If I had to pick an order in which
to do the things you list, I'd focus first on the one you list second:
being able to stream and begin applying transactions before they've
committed is a really big deal for large transactions, and lots of
people have some large transactions.  DDL replication is nice, but
realistically, there are a lot of people who simply don't change their
schema all that often, and who could (and might even prefer to) manage
that process in other ways - e.g. change nodes one by one while they
are off-line, then bring them on-line.

I don't necessarily disagree with your statement that we'd need all of
this stuff to make logical replication a substitute for physical
replication as far as failover is concerned.  But I don't think that's
the most important goal, and even to the extent that it is the goal, I
don't think we need to meet every need before we can consider
ourselves to have met some needs.  I don't think that we need every
physical replication feature plus some before logical replication can
start to be useful to PostgreSQL users generally.  We do, however,
need the functionality to be accessible to people who are using only
the PostgreSQL core distribution.  The thing that is going to get
people excited about making logical replication better is getting to a
point where they can use it at all - and that is not going to be true
as long as you can't use it without having to download something from
an external website.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04/04/16 22:00, Robert Haas wrote:
> On Mon, Apr 4, 2016 at 10:59 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> To allow logical rep and failover to be a reasonable substitute for physical
>> rep and failover IMO *need*:
>>
>> * Robust sequence decoding and replication. If you were following the later
>> parts of that discussion you will've seen how fun that's going to be, but
>> it's the simplest of all of the problems.
>>
>> * Logical decoding and sending of in-progress xacts, so the logical client
>> can already be most of the way through receiving a big xact when it commits.
>> Without this we have a huge lag spike whenever a big xact happens, since we
>> must first finish decoding it in to a reorder buffer and can only then
>> *begin* to send it to the client. During which time no later xacts may be
>> decoded or replayed to the client. If you're running that rare thing, the
>> perfect pure OLTP system, you won't care... but good luck finding one in the
>> real world.
>>
>> * Either parallel apply on the client side or at least buffering of
>> in-progress xacts on the client side so they can be safely flushed to disk
>> and confirmed, allowing receive to continue while replay is done on the
>> client. Otherwise sync rep is completely impractical... and there's no
>> shortage of systems out there that can't afford to lose any transactions. Or
>> at least have some crucial transactions they can't lose.
>>
>> * Robust, seamless DDL replication, so things don't just break randomly.
>> This makes the other points above look nice and simple by comparison.
>> Logical decoding of 2PC xacts with DDL would help here, as would the ability
>> to transparently convert an xact into a prepare-xact on client commit and
>> hold the client waiting while we replicate it, confirm the successful
>> prepare on the replica, then commit prepared on the upstream.
>>
>> * oh, and some way to handle replication of shared catalog changes like
>> pg_authid, so the above DDL replication doesn't just randomly break if it
>> happens to refer to a global object that doesn't exist on the downstream.
>
> In general, I think we'd be a lot better off if we got some kind of
> logical replication into core first and then worked on lifting these
> types of limitations afterwards.  If I had to pick an order in which
> to do the things you list, I'd focus first on the one you list second:
> being able to stream and begin applying transactions before they've
> committed is a really big deal for large transactions, and lots of
> people have some large transactions.  DDL replication is nice, but
> realistically, there are a lot of people who simply don't change their
> schema all that often, and who could (and might even prefer to) manage
> that process in other ways - e.g. change nodes one by one while they
> are off-line, then bring them on-line.
>
> I don't necessarily disagree with your statement that we'd need all of
> this stuff to make logical replication a substitute for physical
> replication as far as failover is concerned.  But I don't think that's
> the most important goal, and even to the extent that it is the goal, I
> don't think we need to meet every need before we can consider
> ourselves to have met some needs.  I don't think that we need every
> physical replication feature plus some before logical replication can
> start to be useful to PostgreSQL users generally.  We do, however,
> need the functionality to be accessible to people who are using only
> the PostgreSQL core distribution.  The thing that is going to get
> people excited about making logical replication better is getting to a
> point where they can use it at all - and that is not going to be true
> as long as you can't use it without having to download something from
> an external website.
>

I agree with you and I think Craig does as well. IMHO he merely pointed 
this out to show that we at the moment need physical replication as HA 
solution for logical decoding because currently there is no way to have 
reasonable continuation of service when the original master dies. And 
waiting until we have logical that's on par with physical for failover 
will take long time as it needs the above.

--   Petr Jelinek                  http://www.2ndQuadrant.com/  PostgreSQL Development, 24x7 Support, Training &
Services

On 04/04/16 17:15, Andres Freund wrote:
>
>> * Robust sequence decoding and replication. If you were following the later
>> parts of that discussion you will've seen how fun that's going to be, but
>> it's the simplest of all of the problems.
>
> Unconvinced. People used londiste and slony for years without that, and
> it's not even remotely at the top of the list of problems with either.
>

Londiste and Slony also support physical failover unlike logical 
decoding which is the main point of this discussion, lets not forget that.

>
>> * Robust, seamless DDL replication, so things don't just break randomly.
>> This makes the other points above look nice and simple by comparison.
>
> We're talking about a system which involves logical decoding. Whether
> you have failover via physical rep or not, doesn't have anything to do
> with this point.
>

It is if you are insisting on using logical rep as solution for failover.

>
>> I don't think there's any realistic way we're going to get there for
>> logical rep in 9.6+n for n<2 unless a whole lot more people get on board
>> and work on it. Even then.
>
> I think the primary problem here is that you're focusing on things that
> just are not very interesting for the majority of users, and which thus
> won't get very enthusastic help.  The way to make progress is to get
> something basic in, and then iterate from there.  Instead you're
> focussing on the fringes; which nobody cares about, because the basics
> aren't there.

Craig is focusing on solving failover for logical slots which is very 
damn basic issue with logical decoding right now no matter if we have 
logical replication in core or not. I personally don't think it's ok by 
any means to not be able to continue logical decoding on failover event 
2 versions after logical decoding was introduced.

I also don't buy your argument that it's unsafe to use timeline 
following on logical decoding on replica. You can always keep master 
from moving too far ahead by other means (even if you just use dummy 
slot which is only used for this purpose, yes ugly I know). If we got 
failover slots into 9.6 it would be better but that does not look 
realistic at this point. I don't think that current design for failover 
slots is best possible - I think failover slots should be created on 
replica and send their status up to the master which would then take 
them into account when calculating oldest needed catalog xmin and lsn 
(simple way of doing that would be to add this to feedback protocol and 
let physical slot to keep the xmin/lsn as well), but that does not mean 
timeline following isn't good thing on it's own (not to mention that 
iterative development is a thing).

--   Petr Jelinek                  http://www.2ndQuadrant.com/  PostgreSQL Development, 24x7 Support, Training &
Services

On 2016-04-05 05:53:53 +0200, Petr Jelinek wrote:
> On 04/04/16 17:15, Andres Freund wrote:
> >
> >>* Robust sequence decoding and replication. If you were following the later
> >>parts of that discussion you will've seen how fun that's going to be, but
> >>it's the simplest of all of the problems.
> >
> >Unconvinced. People used londiste and slony for years without that, and
> >it's not even remotely at the top of the list of problems with either.
> >
> 
> Londiste and Slony also support physical failover unlike logical decoding
> which is the main point of this discussion, lets not forget that.

Sure. But that level of failover isn't all that hard to implement.

I just want to reiterate: I'm not against failover slots per-se, or
against timeline following[1], or something like that. I think it's just
getting the priorities backwards. Until there's basic features
available, you're going to have a hard time fighting for more advanced
features.

[1]: That said, I don't agree with the approach chosen so far, but
that's just a matter of discussion.


> >
> >>* Robust, seamless DDL replication, so things don't just break randomly.
> >>This makes the other points above look nice and simple by comparison.
> >
> >We're talking about a system which involves logical decoding. Whether
> >you have failover via physical rep or not, doesn't have anything to do
> >with this point.

> It is if you are insisting on using logical rep as solution for failover.

How on earth does this follow? If your replicas don't do DDL
propagation, doing so on the primary -> new primary, it surely isn't a
prerequisite.  I agree DDL rep is pretty crucial, but this argument here
isn't that.


> I also don't buy your argument that it's unsafe to use timeline following on
> logical decoding on replica.

I'm not saying that generally. I'm saying that you can't realistically
do it safely with just the timeline following patch, as committed.


> You can always keep master from moving too far
> ahead by other means (even if you just use dummy slot which is only used for
> this purpose, yes ugly I know).

Yes, that somewhat works. And I think this is actually kinda the design
"failover" slots should take instead.


> If we got failover slots into 9.6 it would
> be better but that does not look realistic at this point. I don't think that
> current design for failover slots is best possible - I think failover slots
> should be created on replica and send their status up to the master which
> would then take them into account when calculating oldest needed catalog
> xmin and lsn (simple way of doing that would be to add this to feedback
> protocol and let physical slot to keep the xmin/lsn as well)

Yes, that's not too far away from what I'm thinking of. If we do it
right that also solves the important problems for decoding on a standby.


> but that does not mean timeline following isn't good thing on it's own
> (not to mention that iterative development is a thing).

Yes, I'm not saying that. The reason it scares me is that it's not a
particularly carefully reviewed patch, and the intended usage as
described by Craig.

We most definitely need timeline following, independent of failover
slots. Most importantly for decoding on a standby.

Greetings,

Andres Freund

On Mon, Apr 4, 2016 at 1:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I don't necessarily disagree with your statement that we'd need all of
> this stuff to make logical replication a substitute for physical
> replication as far as failover is concerned.  But I don't think that's
> the most important goal, and even to the extent that it is the goal, I
> don't think we need to meet every need before we can consider
> ourselves to have met some needs.  I don't think that we need every
> physical replication feature plus some before logical replication can
> start to be useful to PostgreSQL users generally.  We do, however,
> need the functionality to be accessible to people who are using only
> the PostgreSQL core distribution.  The thing that is going to get
> people excited about making logical replication better is getting to a
> point where they can use it at all - and that is not going to be true
> as long as you can't use it without having to download something from
> an external website.

I rather agree that an in-core system that solved some of the basic
problems would be a huge step forward, and would motivate people to
work on the harder problems. It's surprising to me that we don't seem
to be much closer to that then we were when 9.4 was released.

-- 
Peter Geoghegan

On 2016-04-05 15:51:00 +0800, Craig Ringer wrote:
> Review and test responses have been pretty underwhelming for pglogical, and
> quite a bit seem to have boiled down to "this should live as an extension,
> we don't need it in core". It often feels like we can't win: if we seek to
> get it into core we're told it's not wanted/needed, but if we try to focus
> on solving issues in core to make it work better and let it live as an
> extension we're told we shouldn't bother until it's in core.

I think partially that's because it's hard to see the goal from those
threads. Leading the intro email with "after applying use these three
steps to replicate a database" or something might help.

I also want to add that so far, to my knowledge, the feedback hasn't
fully been addressed. It's a bit hard to see progress at that pace.


> Do you want to get a logical replication system into core that doesn't work
> properly with lots of the other features in PostgreSQL? That's historically
> not how we've done things here, and sometimes massive amounts of work have
> been required to make new feature X work with obscure/awkward existing
> feature Y.

I think that's a strawman. We have done actual iterative development
where the basic feature came at an early stage a lot of times. Most
impressively FDWs.

And even if we decide that feature X has to be supported, having an
otherwise close-to-committable patch series, goes a *LONG* way to
motivate people.


> Still, I don't really want to block work on making logical decoding more
> real-world usable on inclusion of a logical replication system for
> PostgreSQL, especially one that'll be lucky to get in for 9.7 at the
> earliest.

My impression is that unless you *NOW* press very hard to get it into
core, there's no way to get it into 9.7. Unless you start aggressively
at some point, it'll never get in.

Greetings,

Andres Freund

On Tue, Apr 5, 2016 at 3:51 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> First, I'd like to remind everyone that logical decoding is useful for more
> than replication. You can consume the change stream for audit
> logging/archival, to feed into a different DBMS, etc etc. This is not just
> about replicating from one PostgreSQL to another, though to be sure that'll
> be the main use in the near term.
>
> The Zalando guys at least are already using it for other things, and
> interest in the json support suggests they're not alone.

I have not forgotten any of that, nor do I consider it unimportant.

> Right now if you're doing any kind of logical deocoding from a master server
> that fails over to a standby the client just dies. The slot vanishes. You're
> stuffed. Gee, I hope you didn't need all that nice consistent ordering,
> because you're going to have to start from scratch and somehow reconcile the
> data in the new master with what you've already received ... and haven't.

Right, but right now you probably *aren't* do doing any kind of
logical decoding from a master server to a standby, because there's
squat in the core distribution that could make use of that capability.
So you never get as far as discovering this problem.

> I don't understand why it seems to be considered OK for logical slots to
> just vanish on failover. The only other things I can think of where that's
> considered OK are unlogged tables (because that's the point and we have
> failover-safe ones too) and the old hash indexes nobody's quite willing to
> remove yet.

First, it wasn't until 9.3 that physical standbys could follow
timeline switches, but that doesn't mean that streaming replication
was useless in 9.0 - 9.2, or that warm standby was useless in earlier
versions.  Logical decoding isn't useless without that capability
either.  Would it be nice if we did have that capability?  Of course.

Second, I'm not sure whether it was a good design decision to make
logical slots a special kind of object that sit off to the side,
neither configuration (like postgresql.conf) nor WAL-protected data
(like pg_clog and the data files themselves), but it was certainly a
very deliberate decision.  I sort of expected them to be WAL-logged,
but Andres argued (not unconvincingly) that we'd want to have slots on
standbys, and making them WAL-logged would preclude that.  So I don't
really think that this is much like hash indexes, which just never got
properly finished.  It's more like unlogged tables, where a deliberate
design decision to lose data was made in order to meet some other
goal.

>> realistically, there are a lot of people who simply don't change their
>> schema all that often, and who could (and might even prefer to) manage
>> that process in other ways - e.g. change nodes one by one while they
>> are off-line, then bring them on-line.
>
> Yeah, it's a bit more complex than that. Schema changes *must* be made at a
> specific point in replay. You can't generally just ALTER TABLE ... DROP
> COLUMN on the master then do the same thing on the replica. The replica
> probably still has un-replayed changes from the master that have the
> now-dropped column in their change stream, but now it can't apply them to
> the new table structure on the downstream. This particular case can be
> worked around, but column type changes, addition of non-null columns etc
> cannot.

There are certainly problem cases, but I'm not sure they really arise
that much in practice.  If you retype a column from text to integer,
you probably aren't storing anything in it other than integers, in
which case it is not necessarily the case that you are locked into
applying that change at a particular point in the change stream.  If
you are storing non-integers in a text column and relying on a USING
clause to make them look like integers during the conversion, then,
yes, that has to be done at a precise point in the change stream.  But
that's a pretty strange thing to do, and your application is most
likely going to get confused anyway, so you are probably taking a
maintenance window for the changeover anyway - in which case, there's
not really a big problem.  You can run the same change at the same
time on both servers while nothing else is happening.

> Also, a growing portion of the world uses generated schemas and migrations
> and can't easily pipe that through some arbitrary function we provide. They
> may not be too keen on stopping writes to their entire database as an
> alternative either (and we don't provide a true read-only mode for them to
> use if they did want to). I know it's fashionable around here to just write
> them off as idiots for using ORMs and so on, but they're rather widespread
> idiots who're just as likely to be interested in geographically distributed
> selective replication, change stream extraction, etc. This has been a
> persistent problem with people who want to use BDR, too.

It's not my intent to write anyone off as an idiot.

> Review and test responses have been pretty underwhelming for pglogical, and
> quite a bit seem to have boiled down to "this should live as an extension,
> we don't need it in core". It often feels like we can't win: if we seek to
> get it into core we're told it's not wanted/needed, but if we try to focus
> on solving issues in core to make it work better and let it live as an
> extension we're told we shouldn't bother until it's in core.

To be honest, I was shocked that pglogical and pglogical_output didn't
go into this release.  I assumed that you and other folks at
2ndQuadrant were going to make a big push to get that done.  I did
take a brief look at one of them - pglogical, I think - a week or two
ago but there were unaddressed review comments that had been pending
for months and there were a lot of fairly obvious things that needed
to be done before it could be seriously considered as a core
submission.  Like, for example, rewriting the documentation heavily
and making it look like the rest of our docs, and putting it in SGML
format.  The code seemed to need quite a bit of cleanup, too.  Now,
logical replication is a sufficiently important feature that if the
only way it's going to get into core is if I work on it myself, or get
other people at EnterpriseDB to do so, then I'll try to make that
happen.  But I was assuming that that was your/2ndQuadrant's patch,
that you were going to get it in shape, and that me poking my nose
into it wasn't going to be particularly welcome.  Maybe I've misread
the whole dynamic here.

> Do you want to get a logical replication system into core that doesn't work
> properly with lots of the other features in PostgreSQL? That's historically
> not how we've done things here, and sometimes massive amounts of work have
> been required to make new feature X work with obscure/awkward existing
> feature Y. Inheritance comes strongly to mind as an exciting challenge for
> many new features to support properly. Arguments are usually raised that
> reference things like the mess created by MySQL's storage engines and how
> we're better off making everything work right once and for all.

So, let me be clear, here.  You have every right to decide which
feature you want to work on, and if, at the current time, failover
slots is that thing and if, at the current time, in-core logical
replication solution is not that thing, then that is entirely up to
you.  I don't have any right to tell you what to work on, and I'm not
trying to tell you what to work on.  I'm giving you my opinion on what
I would work on if I were going to do some work related to logical
replication - nothing more.

That being said, if we get a logical replication system into core that
doesn't do DDL, doesn't do multi-master, doesn't know squat about
sequences, and rolls over and dies if a timeline switch happens, I
would consider that a huge step forward and I think a lot of other
people would, too.  We have a long history of building features
incrementally.  Parallel query in 9.6 doesn't parallelize every query
that can be parallelized, postgres_fdw has atrocious performance on
simple queries like SELECT count(*) FROM ft, and autovacuum existed
for a long time before it was turned on by default.  Some people fail
to get patches committed because they set their expectations too low,
and we come back and say "well, that's nice, but we really need a
little more here in order to consider this feature complete".  But
plenty of people also make the opposite mistake, of thinking that they
have to fix everything at once in order to have anything worthwhile,
and that's just as much of a trap as the other thing.  Even with lots
of limitations, built-in logical replication could still be good
enough to be used long enough to manage a major version upgrade -
people who have this problem today are using Slony, and I believe that
even a really basic version of logical rep could have significant
advantages over Slony in terms of both performance and ease of
configuration.

> Still, I don't really want to block work on making logical decoding more
> real-world usable on inclusion of a logical replication system for
> PostgreSQL, especially one that'll be lucky to get in for 9.7 at the
> earliest.

Well, as noted above, I think the main thing we need to figure out is
who is going to do the work.  The main thing blocking other people
from working on it is the belief that we are just waiting for you or
someone else at 2ndQuadrant to land the necessary patches, but if you
aren't really working on that in a focused way, then somebody else can
step up.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/04/16 03:29, Robert Haas wrote:
>
>> I don't understand why it seems to be considered OK for logical slots to
>> just vanish on failover. The only other things I can think of where that's
>> considered OK are unlogged tables (because that's the point and we have
>> failover-safe ones too) and the old hash indexes nobody's quite willing to
>> remove yet.
>
> First, it wasn't until 9.3 that physical standbys could follow
> timeline switches, but that doesn't mean that streaming replication
> was useless in 9.0 - 9.2, or that warm standby was useless in earlier
> versions.  Logical decoding isn't useless without that capability
> either.  Would it be nice if we did have that capability?  Of course.
>

Except that in 9.0 - 9.2 there was working workaround for that, there is 
no such thing for logical decoding.

> Second, I'm not sure whether it was a good design decision to make
> logical slots a special kind of object that sit off to the side,
> neither configuration (like postgresql.conf) nor WAL-protected data
> (like pg_clog and the data files themselves), but it was certainly a
> very deliberate decision.  I sort of expected them to be WAL-logged,
> but Andres argued (not unconvincingly) that we'd want to have slots on
> standbys, and making them WAL-logged would preclude that.

I do think it was good design decision. We just need to make them 
failoverable bit differently and the failover slots patch IMHO isn't the 
right way either as I said in another reply in this thread.

>
>> Review and test responses have been pretty underwhelming for pglogical, and
>> quite a bit seem to have boiled down to "this should live as an extension,
>> we don't need it in core". It often feels like we can't win: if we seek to
>> get it into core we're told it's not wanted/needed, but if we try to focus
>> on solving issues in core to make it work better and let it live as an
>> extension we're told we shouldn't bother until it's in core.
>
> To be honest, I was shocked that pglogical and pglogical_output didn't
> go into this release.  I assumed that you and other folks at
> 2ndQuadrant were going to make a big push to get that done.  I did
> take a brief look at one of them - pglogical, I think - a week or two
> ago but there were unaddressed review comments that had been pending
> for months and there were a lot of fairly obvious things that needed
> to be done before it could be seriously considered as a core
> submission.  Like, for example, rewriting the documentation heavily
> and making it look like the rest of our docs, and putting it in SGML
> format.  The code seemed to need quite a bit of cleanup, too.  Now,
> logical replication is a sufficiently important feature that if the
> only way it's going to get into core is if I work on it myself, or get
> other people at EnterpriseDB to do so, then I'll try to make that
> happen.  But I was assuming that that was your/2ndQuadrant's patch,
> that you were going to get it in shape, and that me poking my nose
> into it wasn't going to be particularly welcome.  Maybe I've misread
> the whole dynamic here.

I guess you did, me and I think Craig as well hoped for some feedback on 
the general ideas in terms of protocol, node setup (I mean catalogs) and 
general architecture from the community. That didn't really happen. And 
without any of that happening I didn't feel confident trying to get it 
right within last month of dev cycle. Especially given the size of the 
patch and the fact we also had other patches that we worked on and had 
realistically higher chance of getting in. Not sure how Craig feels 
about it. Converting documentation, renaming some params in function 
names etc (those unaddressed comments) seemed like secondary to me.

(As a side note I was also 2 weeks without proper working laptop around 
FOSDEM time which had effect on my responses to -hackers about the 
topic, especially to Steve Singer who did good job of reviewing the 
usability at the time, but even if I had it it would not saved the patch)

In general I think project of this size requires more attention from 
committer to help shepherding it and neither Craig or me are that. I am 
glad that Andres said he plans to give some time in next cycle to 
logical replication because that should be big help.

>
> That being said, if we get a logical replication system into core that
> doesn't do DDL, doesn't do multi-master, doesn't know squat about
> sequences, and rolls over and dies if a timeline switch happens, I
> would consider that a huge step forward and I think a lot of other
> people would, too.

I agree with the exception of working HA. I would consider it very sad 
if we got logical replication in core without having any provision for 
continuity of service. Doing that is relatively trivial in comparison to 
the logical replication itself however.

--   Petr Jelinek                  http://www.2ndQuadrant.com/  PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Apr 5, 2016 at 10:34 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> Right, but right now you probably *aren't* do doing any kind of
>> logical decoding from a master server to a standby, because there's
>> squat in the core distribution that could make use of that capability.
>> So you never get as far as discovering this problem.
>
> OK, I'm starting to understand where this is going.
>
> pglogical and pglogical_output are irrelevant so long as they haven't yet
> landed in core. There is no logical replication worth considering, therefore
> anything that improves logical replication is not worth the effort because
> there's really nothing to improve. Why bother with enhancements like
> timeline following when nothing uses it.
>
> Right?

"Irrelevant" is a strong word, and I wouldn't personally go that far.
But they would be a lot *more* relevant if they landed in core, at
least IMHO.

>> Second, I'm not sure whether it was a good design decision to make
>> logical slots a special kind of object that sit off to the side,
>> neither configuration (like postgresql.conf) nor WAL-protected data
>> (like pg_clog and the data files themselves), but it was certainly a
>> very deliberate decision.  I sort of expected them to be WAL-logged,
>> but Andres argued (not unconvincingly) that we'd want to have slots on
>> standbys, and making them WAL-logged would preclude that.
>
> Yeah. I understand the reasons for that decision. Per an earlier reply I
> think we can avoid making them WAL-logged so they can be used on standbys
> and still achieve usable failover support on physical replicas.

I think at one point we may have discussed doing this via additional
side-channel protocol messages.  Is that what you are thinking about
now, or something else?

>> If you retype a column from text to integer,
>> you probably aren't storing anything in it other than integers, in
>> which case it is not necessarily the case that you are locked into
>> applying that change at a particular point in the change stream.
>
> ... if you're replicating in text format, not send/recv format, yeah. You
> get away with it in that particular case. But that *only* works where you're
> using text format and the types are convertable via text representation.
>
> It's also just one example. Adding a NOT NULL column bites you, hard, for
> example.
>
> Asking users to do this themselves is incredibly fragile and requires much
> more knowledge of PostgreSQL innards than I think is realistic. It's part of
> why Slony-I is difficult too.
>
> Sure, you can avoid having DDL replication if you're really, really careful
> and know exactly what you're doing. I don't think it's a pre-req for getting
> logical replication into core, but I do think it's a pretty strong
> requirement for taking the use of logical replication for HA and failover
> seriously for general use.

Well, we can agree to disagree on this.  I don't think that it's all
that difficult to figure out how to change your schema in a
step-by-step way that allows logical replication to keep working while
the nodes are out of sync, but you don't have to agree and that's
fine.  I'm not objecting to eventually adding that feature to core.  I
do think it's a bad idea to be polishing that sort of thing before
getting some more basic facility into core.

>> To be honest, I was shocked that pglogical and pglogical_output didn't
>> go into this release.  I assumed that you and other folks at
>> 2ndQuadrant were going to make a big push to get that done.  I did
>> take a brief look at one of them - pglogical, I think - a week or two
>> ago but there were unaddressed review comments that had been pending
>> for months and there were a lot of fairly obvious things that needed
>> to be done before it could be seriously considered as a core
>> submission.  Like, for example, rewriting the documentation heavily
>> and making it look like the rest of our docs, and putting it in SGML
>> format.  The code seemed to need quite a bit of cleanup, too.
>
> Yes, I agree, and I think those comments are largely fair. Both Petr and I
> had more limited time to work on them than either of us had expected, so
> timely follow-up was a challenge. We also got them ready for initial
> submission later in the cycle than we'd hoped. That said, there was also
> very little review response or interest for a long time after they were
> published... and by the time there was I think both Petr and I were bogged a
> bit in other project work.
>
> In retrospect I think the separate submission of the output plugin was a
> mistake too, at least when called pglogical_output; in part because of the
> name it was completely ignored as meaningless without the downstream and
> written off as useful for nothing except logical replication. So no momentum
> was built by the time the downstream was published.

While I acknowledge that a logical output plugin has applications
beyond replication, I think replication is the preeminent one by a
considerable margin.  Some people want it for other purposes, and we
should facilitate that.  But that number of people is dwarfed by the
number who would use a seamless logical replication facility if we had
one available.  And so that's the thing I think we should focus on
making work.

If I were doing that, I think I would attack it from a considerably
different direction than what has so far been proposed.  I would try
to put the stuff in core, not contrib, and I would arrange to control
it using DDL, not function calls.  For version one, I would cut all of
the stuff that allows data to be sent in any format other than text,
and just use in/outfuncs all the time.

> I think getting more people involved is useful in a number of ways. Not just
> time and resources, but also getting more agreement on the direction things
> should be going in and making sure that more people have a direct hand in
> the core and are happy with it. Working from a common repo where interested
> people can push directly would be an interesting start.
>
> I'll need to discuss this inside 2ndQ and follow up as there are things
> involved that I can't make determinations for myself.

Understood.

>> We have a long history of building features
>> incrementally.  Parallel query in 9.6 doesn't parallelize every query
>> that can be parallelized
>
> Right. But it doesn't just arbitrarily break on what it doesn't understand.
> That's the difference here.

I do generally think that logical decoding relies too much on trying
to set up situations where it will never fail, and I've said from the
beginning that it should make more provision to cope with failure
rather than just trying to avoid it.  If logical decoding never
breaks, great.  But the approach I would favor is to set things up so
that it automatically reclones if there is a replication break, and
then as an optimization project, try to eliminate those cases one by
one.

> It's a big job, and there are lots of differing viewpoints on it.
>
> It does seem like having 2ndQ submit a patch, wait for feedback, be told
> "oh, don't do it that way, come back with <something else>", then wait for
> us to respond and address the issues isn't working. Certainly isn't going to
> get us there in a timely manner.

That isn't really my perception of how things have gone, but I admit I
may not have been following it as closely as I should have done.  I'd
be happy to talk with you about this in person if you're going to be
at PGCONF.US or PGCon in Ottawa; or there's always Skype.  I don't see
any reason why we can't all put our heads together and agree on a
direction that we can all live with.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/04/16 17:32, Robert Haas wrote:
>>> Second, I'm not sure whether it was a good design decision to make
>>> logical slots a special kind of object that sit off to the side,
>>> neither configuration (like postgresql.conf) nor WAL-protected data
>>> (like pg_clog and the data files themselves), but it was certainly a
>>> very deliberate decision.  I sort of expected them to be WAL-logged,
>>> but Andres argued (not unconvincingly) that we'd want to have slots on
>>> standbys, and making them WAL-logged would preclude that.
>>
>> Yeah. I understand the reasons for that decision. Per an earlier reply I
>> think we can avoid making them WAL-logged so they can be used on standbys
>> and still achieve usable failover support on physical replicas.
>
> I think at one point we may have discussed doing this via additional
> side-channel protocol messages.  Is that what you are thinking about
> now, or something else?
>

I think the most promising idea was to use pull model instead of push 
model for slot updates and using feedback to tell master how far the 
oldest slot on standby is. This has the additional advantage of solving 
the biggest problem with decoding on standby (keeping old enough catalog 
xid and lsn). And also solves your concern of propagating through whole 
cascade as this can be done in more controllable way (from bottom up in 
the replication chain).

> For version one, I would cut all of
> the stuff that allows data to be sent in any format other than text,
> and just use in/outfuncs all the time.

Agreed here, I think doing the binary transfer for base types, etc is 
just optimization, not necessity. This is one of the reasons why I 
wanted to get bit broader feedback on the protocol - to get some kind of 
consensus about what we want now, what we might want in the future and 
how to handle getting from now to future without too much complexity or 
breakage. For example currently we have flags for every protocol message 
which I am not sure are completely necessary there, OTOH we probably 
don't want to bump protocol version with every new version of Postgres 
either.

>
> I do generally think that logical decoding relies too much on trying
> to set up situations where it will never fail, and I've said from the
> beginning that it should make more provision to cope with failure
> rather than just trying to avoid it.  If logical decoding never
> breaks, great.  But the approach I would favor is to set things up so
> that it automatically reclones if there is a replication break, and
> then as an optimization project, try to eliminate those cases one by
> one.
>

Well that really depends. I've seen way too many cases where people use 
logical replication as transport mechanism, rather than replication 
where the destination is same as source, and in those scenarios there is 
often no way to "reclone" either because the historical data are no 
longer on the source or because the data on the target were already 
updated after they've been replicated. But in general the idea of 
recovering from error rather than being hell bent on preventing it is 
something I am pushing as well. For example it should be easier to look 
at what's in replication queue and remove things from there if needed.

--   Petr Jelinek                  http://www.2ndQuadrant.com/  PostgreSQL Development, 24x7 Support, Training &
Services

On Sun, Apr 17, 2016 at 10:01:36PM +0800, Craig Ringer wrote:
> I made an unfortunate oversight in the logical decoding timeline following
> code: it doesn't work for logical decoding from the walsender, because I
> left the glue code required out of the final cut of the patch.

> Subject: [PATCH] Enable logical timeline following in the walsender
> 
> ---
>  src/backend/access/transam/xlogutils.c |  7 +++----
>  src/backend/replication/walsender.c    | 11 +++++++----
>  src/include/access/xlogreader.h        |  3 +++
>  src/include/access/xlogutils.h         |  2 ++
>  4 files changed, 15 insertions(+), 8 deletions(-)

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.

Noah Misch wrote:

> The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
> since you committed the patch believed to have created it, you own this open
> item.  If that responsibility lies elsewhere, please let us know whose
> responsibility it is to fix this.  Since new open items may be discovered at
> any time and I want to plan to have them all fixed well in advance of the ship
> date, I will appreciate your efforts toward speedy resolution.  Please
> present, within 72 hours, a plan to fix the defect within seven days of this
> message.  Thanks.

I plan to get Craig's patch applied on Monday 25th, giving me some more
time for study.
.
-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Craig Ringer wrote:

> -    /* oldest LSN that the client has acked receipt for */
> +    /*
> +     * oldest LSN that the client has acked receipt for
> +     *
> +     * Decoding will start calling output plugin callbacks for commits
> +     * after this LSN unless a different start point is specified by
> +     * the client.
> +     */
>      XLogRecPtr    confirmed_flush;

How about this wording?
/* * Oldest LSN that the client has acked receipt for.  This is used as * the start_lsn point in case the client
doesn'tspecify one, and also * as a safety measure to back off in case the client specifies a * start_lsn that's
furtherin the future than this value. */XLogRecPtr    confirmed_flush;
 

> -    /* oldest LSN that might be required by this replication slot */
> +    /*
> +     * oldest LSN that might be required by this replication slot.
> +     *
> +     * Points to the LSN of the most recent xl_running_xacts record where
> +     * no transaction that commits after confirmed_flush is already in
> +     * progress. At this point all xacts committing after confirmed_flush
> +     * can be read entirely into reorder buffers and all visibility
> +     * information can be reconstructed.
> +     */
>      XLogRecPtr    restart_lsn;

I'm unsure about this one.  Why are we speaking of reorder buffers here,
if this is generically about replication slots?  Is it that slots used
by physical replication do not *need* a restart_lsn?  I think the
comment should be worded in a way that's not specific to logical
decoding; or, if the restart_lsn field *is* specific to logical
decoding, then the comment should state as much.


> diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
> index 5af6751..5f74941 100644
> --- a/src/backend/replication/logical/logicalfuncs.c
> +++ b/src/backend/replication/logical/logicalfuncs.c
> @@ -236,8 +236,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
>      PG_TRY();
>      {
>          /*
> -         * Passing InvalidXLogRecPtr here causes decoding to start returning
> -         * commited xacts to the client at the slot's confirmed_flush.
> +         * Passing InvalidXLogRecPtr here causes logical decoding to
> +         * start calling the output plugin for transactions that commit
> +         * at or after the slot's confirmed_flush. This filters commits
> +         * out from the client but they're still decoded.
>           */
>          ctx = CreateDecodingContext(InvalidXLogRecPtr,
>                                      options,

I this it's weird to have the parameter explained in the callsite rather
than in the comment about CreateDecodingContext.  I think this patch
needs to apply to logical.c, not logicalfuncs.

> @@ -262,11 +264,9 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
>          ctx->output_writer_private = p;
>  
>          /*
> -         * We start reading xlog from the restart lsn, even though we won't
> -         * start returning decoded data to the user until the point set up in
> -         * CreateDecodingContext. The restart_lsn is far enough back that we'll
> -         * see the beginning of any xact we're expected to return to the client
> -         * so we can assemble a complete reorder buffer for it.
> +         * Reading and decoding of WAL must start at restart_lsn so that the
> +         * entirety of each of the xacts that commit after confimed_lsn can be
> +         * accumulated into reorder buffers.
>           */
>          startptr = MyReplicationSlot->data.restart_lsn;

This one looks fine to me, except typo s/confimed/confirmed/

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-04-26 17:22:49 -0300, Alvaro Herrera wrote:
> > -    /* oldest LSN that might be required by this replication slot */
> > +    /*
> > +     * oldest LSN that might be required by this replication slot.
> > +     *
> > +     * Points to the LSN of the most recent xl_running_xacts record where
> > +     * no transaction that commits after confirmed_flush is already in
> > +     * progress. At this point all xacts committing after confirmed_flush
> > +     * can be read entirely into reorder buffers and all visibility
> > +     * information can be reconstructed.
> > +     */
> >      XLogRecPtr    restart_lsn;
> 
> I'm unsure about this one.  Why are we speaking of reorder buffers here,
> if this is generically about replication slots?  Is it that slots used
> by physical replication do not *need* a restart_lsn?

It's used in physical slots as well, so I agree.  Also I think the
xl_running_xacts bit is going into way to much detail; it could very
well just be shutdown checkpoint or other similar locations.

> > diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
> > index 5af6751..5f74941 100644
> > --- a/src/backend/replication/logical/logicalfuncs.c
> > +++ b/src/backend/replication/logical/logicalfuncs.c
> > @@ -236,8 +236,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
> >      PG_TRY();
> >      {
> >          /*
> > -         * Passing InvalidXLogRecPtr here causes decoding to start returning
> > -         * commited xacts to the client at the slot's confirmed_flush.
> > +         * Passing InvalidXLogRecPtr here causes logical decoding to
> > +         * start calling the output plugin for transactions that commit
> > +         * at or after the slot's confirmed_flush. This filters commits
> > +         * out from the client but they're still decoded.
> >           */
> >          ctx = CreateDecodingContext(InvalidXLogRecPtr,
> >                                      options,
> 
> I this it's weird to have the parameter explained in the callsite rather
> than in the comment about CreateDecodingContext.  I think this patch
> needs to apply to logical.c, not logicalfuncs.

I still think the entire comment ought to be removed.


- Andres

Craig Ringer wrote:

> The reason the new src/test/recovery/ tests don't notice this is that using
> pg_recvlogical from the TAP tests is currently pretty awkward.
> pg_recvlogical has no way to specify a stop-point or return when there's no
> immediately pending data like the SQL interface does. So you have to read
> from it until you figure it's not going to return anything more then kill
> it and look at what it did return and hope you don't lose anything in
> buffering.I don't much like relying on timeouts as part of normal
> successful results since they can upset some of the older and slower
> buildfarm members. I'd rather be able to pass a --stoppos= or a --n-xacts=
> option, but it was a bit too late to add those.

Considering that pg_recvlogical was introduced mostly as a way to test
logical decoding features, I think this is a serious oversight and we
should patch it.  I suppose we could leave it for 9.7, thought I admit I
would prefer it to introduce it in 9.6.  Now everyone throwing stones at
me in 3 ... 2 ...

> Per the separate mail I sent to hackers, xlogreader is currently pretty
> much timeline-agnostic. The timeline tracking code needs to know when the
> xlogreader does a random read and do a fresh lookup so its state is part of
> the XLogReaderState struct. But the walsender's xlogreader page read
> callback doesn't use the xlogreader's state, and it can't because it's also
> used for physical replication, which communicates the timeline to read from
> with the page read function via globals.

The globals there are a disaster and I welcome efforts to get rid of
them.

> So for now, I just set the globals before calling the page read
> function:
> 
> +       XLogReadDetermineTimeline(state);
> +       sendTimeLineIsHistoric = state->currTLI == ThisTimeLineID;
> +       sendTimeLine = state->currTLI;
> +       sendTimeLineValidUpto = state->currTLIValidUntil;
> +       sendTimeLineNextTLI = state->nextTLI;

Ok, this is pretty ugly but seems acceptable.

> Per separate mail, I'd quite like to tackle the level of duplication in
> timeline following logic in 9.7 and maybe see if the _three_ separate read
> xlog page functions can be unified at the same time. But that sure isn't
> 9.6 material.

Agreed.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:
> Noah Misch wrote:
> 
> > The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
> > since you committed the patch believed to have created it, you own this open
> > item.  If that responsibility lies elsewhere, please let us know whose
> > responsibility it is to fix this.  Since new open items may be discovered at
> > any time and I want to plan to have them all fixed well in advance of the ship
> > date, I will appreciate your efforts toward speedy resolution.  Please
> > present, within 72 hours, a plan to fix the defect within seven days of this
> > message.  Thanks.
> 
> I plan to get Craig's patch applied on Monday 25th, giving me some more
> time for study.

I failed to meet this deadline, for which I apologize.  This week is
going to be hectic around here, so my new deadline is to get these two
patches applied on Friday 29th.  Ok?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Apr 26, 2016 at 05:37:40PM -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> > Noah Misch wrote:
> > > The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
> > > since you committed the patch believed to have created it, you own this open
> > > item.  If that responsibility lies elsewhere, please let us know whose
> > > responsibility it is to fix this.  Since new open items may be discovered at
> > > any time and I want to plan to have them all fixed well in advance of the ship
> > > date, I will appreciate your efforts toward speedy resolution.  Please
> > > present, within 72 hours, a plan to fix the defect within seven days of this
> > > message.  Thanks.
> > 
> > I plan to get Craig's patch applied on Monday 25th, giving me some more
> > time for study.
> 
> I failed to meet this deadline, for which I apologize.  This week is
> going to be hectic around here, so my new deadline is to get these two
> patches applied on Friday 29th.  Ok?

Okay; thanks for this notification.

On Tue, Apr 26, 2016 at 4:35 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Craig Ringer wrote:
>> The reason the new src/test/recovery/ tests don't notice this is that using
>> pg_recvlogical from the TAP tests is currently pretty awkward.
>> pg_recvlogical has no way to specify a stop-point or return when there's no
>> immediately pending data like the SQL interface does. So you have to read
>> from it until you figure it's not going to return anything more then kill
>> it and look at what it did return and hope you don't lose anything in
>> buffering.I don't much like relying on timeouts as part of normal
>> successful results since they can upset some of the older and slower
>> buildfarm members. I'd rather be able to pass a --stoppos= or a --n-xacts=
>> option, but it was a bit too late to add those.
>
> Considering that pg_recvlogical was introduced mostly as a way to test
> logical decoding features, I think this is a serious oversight and we
> should patch it.  I suppose we could leave it for 9.7, thought I admit I
> would prefer it to introduce it in 9.6.  Now everyone throwing stones at
> me in 3 ... 2 ...

If that's a small and relatively contained change, I don't object to
the idea of you making it, provided it's done pretty soon, and
definitely before beta1.  If it's going to require substantial
refactoring or can't be done quickly, then, yes, I object.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2016-04-27 14:21:53 -0400, Robert Haas wrote:
> > Considering that pg_recvlogical was introduced mostly as a way to test
> > logical decoding features, I think this is a serious oversight and we
> > should patch it.  I suppose we could leave it for 9.7, thought I admit I
> > would prefer it to introduce it in 9.6.  Now everyone throwing stones at
> > me in 3 ... 2 ...
> 
> If that's a small and relatively contained change, I don't object to
> the idea of you making it, provided it's done pretty soon, and
> definitely before beta1.  If it's going to require substantial
> refactoring or can't be done quickly, then, yes, I object.

I don't object either, I was looking for the feature myself a number of
times (for tap tests in my case).

It requires a some amount of thinking about what the limit applies to
though. "messages sent by server", Bytes? TCP messages? xids? Time?

- Andres

On Tue, Apr 26, 2016 at 4:37 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Alvaro Herrera wrote:
>> Noah Misch wrote:
>>
>> > The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
>> > since you committed the patch believed to have created it, you own this open
>> > item.  If that responsibility lies elsewhere, please let us know whose
>> > responsibility it is to fix this.  Since new open items may be discovered at
>> > any time and I want to plan to have them all fixed well in advance of the ship
>> > date, I will appreciate your efforts toward speedy resolution.  Please
>> > present, within 72 hours, a plan to fix the defect within seven days of this
>> > message.  Thanks.
>>
>> I plan to get Craig's patch applied on Monday 25th, giving me some more
>> time for study.
>
> I failed to meet this deadline, for which I apologize.  This week is
> going to be hectic around here, so my new deadline is to get these two
> patches applied on Friday 29th.  Ok?

Not that you don't know this already, but you have also failed to meet
this deadline.  Also, this open item has now been promoted to the much
sought-after status of "oldest as-yet-unaddressed PostgreSQL 9.6 open
item".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Tue, Apr 26, 2016 at 4:37 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > I failed to meet this deadline, for which I apologize.  This week is
> > going to be hectic around here, so my new deadline is to get these two
> > patches applied on Friday 29th.  Ok?
> 
> Not that you don't know this already, but you have also failed to meet
> this deadline.  Also, this open item has now been promoted to the much
> sought-after status of "oldest as-yet-unaddressed PostgreSQL 9.6 open
> item".

Yay.  I thought I was going to have Friday completely available, but
that ended up not happening.

I'm working on this exclusively now until I get the item closed.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I pushed a fix to some comments, including the ones being discussed in
this subthread, which should hopefully close things here.

I'm now going to go over Craig's pg_recvlogical changes and the proposed
for that problem.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-05-02 16:12:53 -0300, Alvaro Herrera wrote:
> I pushed a fix to some comments, including the ones being discussed in
> this subthread, which should hopefully close things here.
> 
> I'm now going to go over Craig's pg_recvlogical changes and the proposed
> for that problem.


-   /* oldest LSN that the client has acked receipt for */
+   /*
+    * Oldest LSN that the client has acked receipt for.  This is used as the
+    * start_lsn point in case the client doesn't specify one, and also as a
+    * safety measure to back off in case the client specifies a start_lsn
+    * that's further in the future than this value.
+    */   XLogRecPtr  confirmed_flush;

This is the wrong way round. confirmed_flush is used if the client's
start_lsn is further in the *past* than this value.

Andres Freund wrote:

> -   /* oldest LSN that the client has acked receipt for */
> +   /*
> +    * Oldest LSN that the client has acked receipt for.  This is used as the
> +    * start_lsn point in case the client doesn't specify one, and also as a
> +    * safety measure to back off in case the client specifies a start_lsn
> +    * that's further in the future than this value.
> +    */
>     XLogRecPtr  confirmed_flush;
> 
> This is the wrong way round. confirmed_flush is used if the client's
> start_lsn is further in the *past* than this value.

Bah.  Funnily enough I got this one right in the comment for
CreateDecodingContext.

Thanks for reading through the commit.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Craig Ringer wrote:

> With this patch pg_recvlogical takes a new --endpos LSN argument, and will
> exit if either:
>
> * it receives an XLogData message with dataStart >= endpos; or
> * it receives a keepalive with walEnd >= endpos
>
> The latter allows it to work without needing a dummy transaction to make it
> see a data message after endpos. If there's nothing to read on the socket
> until a keepalive we know that the server has nothing to send us, and if
> walend has passed endpos we know nothing can have committed before endpos.

Here's a slightly revised version of this patch, for later
consideration.

Changes:
- added -E as short form of --endpos (consistent with -I as --startpos)
- refactored some repetitive code in two auxilliary functions
- allow --endpos to work with --create-slot.
- revert some unrelated changes, such as message additions in verbose
  mode and changes to existing messages
- documentation reworked.

I didn't spot any bugs in Craig's patch, but it needs more testing.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I don't like reverting patches, but this patch is making me more and
more uncomfortable.  We have two open items, one of which requires
writing new test code that doesn't exist yet; and we have the
pg_recvlogical changes that were approved post-feature freeze, but that
I now have second thoughts about pushing right away.
Craig has also commented on some followup patch to change this whole
area in 9.7.

Per
https://www.postgresql.org/message-id/20160503165812.GA29604%40alvherre.pgsql
I think the best course of action is to revert the whole feature and
revisit for 9.7.

Here's a proposed revert patch.  Many minor changes (mostly comment
additions) that were applied as part of this series are kept intact.
The test_slot_timeline test module and corresponding recovery test
script are also reverted.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera wrote:

> Here's a proposed revert patch.  Many minor changes (mostly comment
> additions) that were applied as part of this series are kept intact.
> The test_slot_timeline test module and corresponding recovery test
> script are also reverted.

Pushed.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services