Re: PATCH: Reducing lock strength of trigger and foreign key DDL

On 01/16/2015 03:01 PM, Andres Freund wrote:
> Hi,
>
>>       /*
>> -     * Grab an exclusive lock on the pk table, so that someone doesn't delete
>> -     * rows out from under us. (Although a lesser lock would do for that
>> -     * purpose, we'll need exclusive lock anyway to add triggers to the pk
>> -     * table; trying to start with a lesser lock will just create a risk of
>> -     * deadlock.)
>> +     * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't
>> +     * delete rows out from under us. Note that this does not create risks
>> +     * of deadlocks as triggers add added to the pk table using the same
>> +     * lock.
>>        */
>
> "add added" doesn't look intended. The rest of the sentence doesn't look
> entirely right either.

It was intended to be "are added", but the sentence is pretty awful 
anyway. I am not sure the sentence is really necessary anyway.

>>       /*
>>        * Triggers must be on tables or views, and there are additional
>> @@ -526,8 +526,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
>>        * can skip this for internally generated triggers, since the name
>>        * modification above should be sufficient.
>>        *
>> -     * NOTE that this is cool only because we have AccessExclusiveLock on the
>> -     * relation, so the trigger set won't be changing underneath us.
>> +     * NOTE that this is cool only because of the unique contraint.
>
> I fail to see what the unique constraint has to do with this? The
> previous comment refers to the fact that the AccessExclusiveLock is what
> prevents a race where another transaction adds a trigger with the same
> name already exists. Yes, the unique index would, as noted earlier in
> the comment, catch the error. But that's not the point of the
> check. Unless I miss something the comment is just as true if you
> replace the access exclusive with share row exlusive as it's also self
> conflicting.

Yeah, this must have been a remainder from the version where I only 
grabbed a ShareLock. The comment should be restored.

> Hm. Pushing the snapshot is supposed to make this fully mvcc? Idon't
> think that's actually sufficient because of the deparsing of the WHEN
> clause and of the function name.

Indeed. As Noah and I discussed previously in this thread we would need 
to do quite a bit of refactoring of ruleutils.c to make it fully MVCC. 
For this reason I opted to only lower the lock levels of ADD and ALTER 
TRIGGER, and not DROP TRIGGER. Neither of those require MVCC of then 
WHEN clause.

Andreas