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

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.

>      /*
>       * 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.

> @@ -1272,8 +1271,7 @@ renametrig(RenameStmt *stmt)
>       * on tgrelid/tgname would complain anyway) and to ensure a trigger does
>       * exist with oldname.
>       *
> -     * 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 there is a unique constraint.
>       */

Same as above.

>      tgrel = heap_open(TriggerRelationId, RowExclusiveLock);
>  
> diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
> index dd748ac..8eeccf2 100644
> --- a/src/backend/utils/adt/ruleutils.c
> +++ b/src/backend/utils/adt/ruleutils.c
> @@ -699,7 +699,8 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
>      HeapTuple    ht_trig;
>      Form_pg_trigger trigrec;
>      StringInfoData buf;
> -    Relation    tgrel;
> +    Snapshot    snapshot = RegisterSnapshot(GetTransactionSnapshot());
> +    Relation    tgrel = heap_open(TriggerRelationId, AccessShareLock);
>      ScanKeyData skey[1];
>      SysScanDesc tgscan;
>      int            findx = 0;
> @@ -710,18 +711,18 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty)
>      /*
>       * Fetch the pg_trigger tuple by the Oid of the trigger
>       */
> -    tgrel = heap_open(TriggerRelationId, AccessShareLock);
> -
>      ScanKeyInit(&skey[0],
>                  ObjectIdAttributeNumber,
>                  BTEqualStrategyNumber, F_OIDEQ,
>                  ObjectIdGetDatum(trigid));
>  
>      tgscan = systable_beginscan(tgrel, TriggerOidIndexId, true,
> -                                NULL, 1, skey);
> +                                snapshot, 1, skey);
>  
>      ht_trig = systable_getnext(tgscan);
>  
> +    UnregisterSnapshot(snapshot);
> +
>      if (!HeapTupleIsValid(ht_trig))
>          elog(ERROR, "could not find tuple for trigger %u", trigid);
>

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.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services