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