Re: [HACKERS] make async slave to wait for lsn to be replayed

On Mon, Mar 18, 2024 at 5:17 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Mar 17, 2024 at 7:40 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Sat, Mar 16, 2024 at 5:05 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > > On Sat, Mar 16, 2024 at 4:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > In general, it seems this patch has been stuck for a long time on the
> > > > decision to choose an appropriate UI (syntax), and we thought of
> > > > moving it further so that the other parts of the patch can be
> > > > reviewed/discussed. So, I feel before pushing this we should see
> > > > comments from a few (at least two) other senior members who earlier
> > > > shared their opinion on the syntax. I know we don't have much time
> > > > left but OTOH pushing such a change (where we didn't have a consensus
> > > > on syntax) without much discussion at this point of time could lead to
> > > > discussions after commit.
> > >
> > > +1 to gain consensus first on the syntax changes. With this, we might
> > > be violating the SQL standard for explicit txn commands (I stand for
> > > correction about the SQL standard though).
> >
> > Thank you for your feedback.  Generally, I agree it's correct to get
> > consensus on syntax first.  And yes, this patch has been here since
> > 2016.  We didn't get consensus for syntax for 8 years.  Frankly
> > speaking, I don't see a reason why this shouldn't take another 8
> > years.  At the same time the ability to wait on standby given LSN is
> > replayed seems like pretty basic and simple functionality.  Thus, it's
> > quite frustrating it already took that long and still unclear when/how
> > this could be finished.
> >
> > My current attempt was to commit minimal implementation as less
> > invasive as possible.  A new clause for BEGIN doesn't require
> > additional keywords and doesn't introduce additional statements.  But
> > yes, this is still a new qual.  And, yes, Amit you're right that even
> > if I had committed that, there was still a high risk of further
> > debates and revert.
> >
> > Given my specsis about agreement over syntax, I'd like to check
> > another time if we could go without new syntax at all.  There was an
> > attempt to implement waiting for lsn as a function.  But function
> > holds a snapshot, which could prevent WAL records from being replayed.
> > Releasing a snapshot could break the parent query.  But now we have
> > procedures, which need a dedicated statement for the call and can even
> > control transactions.  Could we implement a waitlsn in a procedure
> > that:
> >
> > 1. First, check that it was called with non-atomic context (that is,
> > it's not called within a transaction). Trigger error if called with
> > atomic context.
> > 2. Release a snapshot to be able to wait without risk of WAL replay
> > stuck.  Procedure is still called within the snapshot.  It's a bit of
> > a hack to release a snapshot, but Vacuum statements already do so.
> >
>
> Can you please provide a bit more details with some example what is
> the existing problem with functions and how using procedures will
> resolve it? How will this this address the implicit transaction case
> or do we have any other workaround for those cases?

Please check [1] and [2] for the explanation of the problem with functions.

Also, please find a draft patch implementing the procedure.  The issue with the snapshot is addressed with the following lines.

We first ensure we're in a non-atomic context, then pop an active snapshot (tricky, but ExecuteVacuum() does the same).  Then we should have no active snapshot and it's safe to wait for lsn replay.

    if (context->atomic)
        ereport(ERROR,
                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                 errmsg("pg_wait_lsn() must be only called in non-atomic context")));

    if (ActiveSnapshotSet())
        PopActiveSnapshot();
    Assert(!ActiveSnapshotSet());

The function call could be added either before the BEGIN statement or before the implicit transaction.

CALL pg_wait_lsn('my_lsn', my_timeout); BEGIN;