Re: hot_standby_feedback vs excludeVacuum and snapshots

On 2018-06-08 11:29:17 +0530, Amit Kapila wrote:
> On Fri, Jun 8, 2018 at 2:55 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > On 2018-06-07 14:19:18 -0700, Andres Freund wrote:
> > > We currently do acquire an xid when truncating the relation - but I
> > > think it'd somewhat fair to argue that that's somewhat of a bug. The
> > > reason a log is acquired is that we need to log AEL locks, and that
> > > currently means they have to be assigned to a transaction.
> > >
> > > Given that the truncation happens at the end of VACUUM and it *NEEDS* to
> > > be present on the standby - otherwise the locking stuff is useless - I
> > > don't think the fix commited in this thread is correct.
> > >
> > > Wonder if the right thing here wouldn't be to instead transiently
> > > acquire an AEL lock during replay when truncating a relation?
> >
>
> If we go that route, then don't we need to bother about when to
> release the lock which is right now done at the commit/abort of the
> transaction. In master, for vacuum, we do release the lock
> immediately after truncate, but I think that is not true generally.
> So, if we release the lock at a time other than commit/abort, we might
> end up releasing them at the different times in master and standby.

I don't think that'd be an actual problem. For TRUNCATE we'd just
temporarily hold two locks. One for the DDL command itself, and one for
the trunction. Same is true for VACUUM, except that only one of them is
an AEL.

> > Isn't the fact that vacuum truncation requires an AEL, and that the
> > change committed today excludes those transactions from running xacts
> > records, flat out broken?
> >
> > Look at:
> >
> > void
> > ProcArrayApplyRecoveryInfo(RunningTransactions running)
> > ...
> > /*
> > * Remove stale locks, if any.
> > *
> > * Locks are always assigned to the toplevel xid so we don't need to care
> > * about subxcnt/subxids (and by extension not about ->suboverflowed).
> > */
> > StandbyReleaseOldLocks(running->xcnt, running->xids);
> >
> > by excluding running transactions you have, as far as I can tell,
> > effectively removed the vacuum truncation AEL from the standby. Now that
> > only happens when a running xact record is logged, but as that happens
> > in the background...

> Yeah, that seems problematic. I think we can avoid that if before
> releasing the lock in StandbyReleaseOldLocks, we also have an
> additional check to see whether the transaction is committed or
> aborted as we do before adding it to KnownAssignedXids.

I think the right fix is to simply revert the change here, rather than
do unplanned open heart surgery. No convincing explanation has been
made why the change is necessary in the first place - the xid assignment
comes directly before vacuum finishes. That's much less than a lot of
other normal transactions will hold back concurrent vacuums / the
standby's reported horizon.

Greetings,

Andres Freund