On 2015-01-27 16:23:53 +0900, Michael Paquier wrote: > On Tue, Jan 27, 2015 at 6:24 AM, Andres Freund <and...@2ndquadrant.com> wrote: > > Unfortunately that Assert()s when there's a lock conflict because > > e.g. another backend is currently connecting. That's because ProcSleep() > > does a enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout) - and > > there's no deadlock timeout (or lock timeout) handler registered.
> Yes, that could logically happen if there is a lock conflicting as > RowExclusiveLock or lower lock can be taken in recovery. I don't this specific lock (it's a object, not relation lock) can easily be taken directly by a user except during authentication. > > [...] > > afaics, that should work? Not pretty, but probably easier than starting > > to reason about the deadlock detector in the startup process. > Wouldn't it be cleaner to simply register a dedicated handler in > StartupXlog instead, obviously something else than DEADLOCK_TIMEOUT as > it is reserved for backend operations? For back-branches, we may even > consider using DEADLOCK_TIMEOUT.. What would that timeout handler actually do? Two problems: a) We really don't want the startup process be killed/error out as that likely means a postmaster restart. So the default deadlock detector strategy is something that's really useless for us. b) Pretty much all other (if not actually all other) heavyweight lock acquisitions in the startup process acquire locks using dontWait = true - which means that the deadlock detector isn't actually run. That's essentially fine because we simply kill everything in our way. C.f. StandbyAcquireAccessExclusiveLock() et al. There's a dedicated 'deadlock detector' like infrastructure around ResolveRecoveryConflictWithBufferPin(), but it deals with a class of deadlocks that's not handled in the deadlock detector anyway. I think this isn't particularly pretty, but it seems to be working well enough, and changing it would be pretty invasive. So keeping in line with all that code seems to be easier. > > We probably should also add a Assert(!InRecovery || sessionLock) to > > LockAcquireExtended() - these kind of problems are otherwise hard to > > find in a developer setting. > So this means that locks other than session ones cannot be taken while > a node is in recovery, but RowExclusiveLock can be taken while in > recovery. Don't we have a problem with this assertion then? Note that InRecovery doesn't mean what you probably think it means: /* * Are we doing recovery from XLOG? * * This is only ever true in the startup process; it should be read as meaning * "this process is replaying WAL records", rather than "the system is in * recovery mode". It should be examined primarily by functions that need * to act differently when called from a WAL redo function (e.g., to skip WAL * logging). To check whether the system is in recovery regardless of which * process you're running in, use RecoveryInProgress() but only after shared * memory startup and lock initialization. */ bool InRecovery = false; The assertion actually should be even stricter: Assert(InRecovery || (sessionLock && dontWait)); i.e. we never acquire a heavyweight lock in the startup process unless it's a session lock (as we don't have resource managers/a xact to track locks) and we don't wait (as we don't have the deadlock detector infrastructure set up). Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
