At Sat, 4 Aug 2018 14:09:18 +1200, Thomas Munro <thomas.mu...@enterprisedb.com>
wrote in <CAEepm=1gwba8AKKb6BPp5iTdVxf=dex1qhphxgdrvt76zvt...@mail.gmail.com>
> On Mon, Jul 9, 2018 at 6:51 AM, Noah Misch <n...@leadboat.com> wrote:
> > On Fri, Jul 06, 2018 at 04:32:56PM +0100, Simon Riggs wrote:
> >> On 6 July 2018 at 03:30, Thomas Munro <thomas.mu...@enterprisedb.com>
> >> wrote:
> >> > On Thu, Jul 5, 2018 at 8:27 AM, Noah Misch <n...@leadboat.com> wrote:
> >> >>> However, 49bff5300d527 also introduced a similar bug where
> >> >>> subtransaction
> >> >>> commit would fail to release an AccessExclusiveLock, leaving the
> >> >>> lock to
> >> >>> be removed sometimes early and sometimes late. This commit fixes
> >> >>> that bug also. Backpatch to PG10 needed.
> >> >>
> >> >> Subtransaction commit is too early to release an arbitrary
> >> >> AccessExclusiveLock. The primary releases every AccessExclusiveLock at
> >> >> top-level transaction commit, top-level transaction abort, or
> >> >> subtransaction
> >> >> abort. CommitSubTransaction() doesn't do that; it transfers locks to
> >> >> the
> >> >> parent sub(xact). Standby nodes can't safely remove an arbitrary lock
> >> >> earlier
> >> >> than the primary would.
> >> >
> >> > But we don't release locks acquired by committing subxacts until the
> >> > top level xact commits. Perhaps that's what the git commit message
> >> > meant by "early", as opposed to "late" meaning when
> >> > StandbyReleaseOldLocks() next runs because an XLOG_RUNNING_XACTS
> >> > record is replayed?
> >>
> >> Locks held by subtransactions were not released at the correct timing
> >> of top-level commit; they are now.
> >
> > I read the above-quoted commit message as saying that the commit expands the
> > set of locks released when replaying subtransaction commit. The only lock
> > that should ever be released at subxact commit is the subxact XID lock. If
> > that continues to be the only lock released at subxact commit, good.
>
> You can still see these "late" lock releases on 10, since the above
> quoted commit (15378c1a) hasn't been back-patched yet:
>
> CREATE TABLE foo ();
>
> BEGIN; SAVEPOINT s; LOCK foo; COMMIT;
>
> An AccessExclusiveLock is held on the standby until the next
> XLOG_RUNNING_XACTS comes along, up to LOG_SNAPSHOT_INTERVAL_MS = 15
> seconds later.
>
> Does anyone know why StandbyReleaseLocks() releases all locks if
> passed InvalidTransactionId? When would that happen?
AFAICS, it used to be used at shutdown time since hot standby was
introduced by efc16ea520 from
ShutdownRecoveryTransactionEnvironment/StandbyReleaseAllLocks.
c172b7b02e (Jan 23 2012) modified StandbyReleaseAllLocks not to
call StandbyReleaseLocks with InvalidTransactionId and the
feature became useless, and now it is.
So I think the feature has been obsolete for a long time.
As a similar thing, the following commands leaves AEL even though
the savepoint is rollbacked.
BEGIN; SAVEPOINT s; LOCK foo; CHECKPOINT; ROLLBACK TO SAVEPOINT s;
This is because the checkpoint issues XLOG_STANDBY_LOCK on foo
with the top-transaciton XID.
Every checkpoint issues it for all existent locks so
RecoveryLockList(s) can be bloated with the same lock entries and
increases lock counts. Although it doesn't seem common to sustain
AELs for a long time so that the length harms, I don't think such
duplication is good. Patch attached.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index f9c12e603b..9de46e0bea 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -632,6 +632,7 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
xl_standby_lock *newlock;
LOCKTAG locktag;
bool found;
+ ListCell *lc;
/* Already processed? */
if (!TransactionIdIsValid(xid) ||
@@ -653,6 +654,20 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
entry->locks = NIL;
}
+ /*
+ * multple lock for the same object can be given by XLOG_STANDBY_LOCK logs.
+ * we need no more than one of them.
+ */
+ foreach (lc, entry->locks)
+ {
+ xl_standby_lock *l = (xl_standby_lock *) lfirst(lc);
+
+ Assert(l->xid == xid);
+
+ if (l->dbOid == dbOid && l->relOid == relOid)
+ return;
+ }
+
newlock = palloc(sizeof(xl_standby_lock));
newlock->xid = xid;
newlock->dbOid = dbOid;
