Re: Hot Standby WAL reply uses heavyweight session locks, but doesn't have enough infrastructure set up

On Sat, Jan 31, 2015 at 5:34 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2015-01-29 11:01:51 -0500, Robert Haas wrote:
>> On Wed, Jan 28, 2015 at 2:41 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > Andres Freund wrote:
>> >> 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.
>> > OK, I'm convinced with this part to remove the call of
>> > LockSharedObjectForSession that uses dontWait and replace it by a loop
>> > in ResolveRecoveryConflictWithDatabase.
>>
>> That seems right to me, too.
>
> It's slightly more complicated than that. The lock conflict should
> actually be resolved using ResolveRecoveryConflictWithLock()... That,
> combined with the race of connecting a actually already deleted database
> (see the XXXs I removed) seem to make the approach in here.
>
> Attached are two patches:
> 1) Infrastructure for attaching more kinds of locks on the startup
> process.
> 2) Use that infrastructure for database locks during replay.
>
> I'm not sure 2) alone would be sufficient justification for 1), but the
> nearby thread about basebackups also require similar infrastructure...

Some comments about patch 1:
- * No locking is required here because we already acquired
- * AccessExclusiveLock. Anybody trying to connect while we do this will
- * block during InitPostgres() and then disconnect when they see the
- * database has been removed.
+ * No locking is required here because we already acquired a
+ * AccessExclusiveLock on the database in dbase_redo().
Anybody trying to
+ * connect while we do this will block during InitPostgres() and then
+ * disconnect when they see the database has been removed.
*/
This change looks unnecessary, I'd rather let it as-is.

- "RecoveryLockList contains entry for lock no longer recorded by
lock manager: xid %u database %u relation %u",
- lock->xid, lock->dbOid, lock->relOid);
+ "RecoveryLockList contains entry for lock no longer recorded by
lock manager: xid %u",
+ lock->xid);
This patch is making the information provided less verbose, and I
think that it is useful to have some information not only about the
lock held, but as well about the database and the relation.
Also, ISTM that StandbyAcquireLock should still use a database OID and
a relation OID instead of a only LOCKTAG, and SET_LOCKTAG_RELATION
should be set in StandbyAcquireLock while
ResolveRecoveryConflictWithLock is extended only with the lock mode as
new argument. (Patch 2 adds many calls to SET_LOCKTAG_RELATION btw
justidying to keep he API changes minimal).

There are some typos in the commit message:
s/shanges/changes
s/exlusive/exclusive

In patch 2, isn't it necessary to bump XLOG_PAGE_MAGIC?
--
Michael