Re: Make relation_openrv atomic wrt DDL

On Thu, Jul 07, 2011 at 11:43:30AM -0400, Robert Haas wrote:
> On Wed, Jul 6, 2011 at 10:44 PM, Noah Misch <noah@2ndquadrant.com> wrote:
> > Attached.  I made the counter 64 bits wide, handled the nothing-found case per
> > your idea, and improved a few comments cosmetically.  I have not attempted to
> > improve the search_path interposition case.  We can recommend the workaround
> > above, and doing better looks like an excursion much larger than the one
> > represented by this patch.
> 
> I looked at this some more and started to get uncomfortable with the
> whole idea of having RangeVarLockRelid() be a wrapper around
> RangeVarGetRelid().  This hazard exists everywhere the latter function
> gets called, not just in relation_open().  So it doesn't seem right to
> fix the problem only in those places.

Yes; I wished to focus on the major case for this round, then address the
other callers later.  We can do it this way, though.

It does make long-term sense to expose only the lock-taking form, making
otherwise-unaffected callers say NoLock explicitly.

> So I went through and incorporated the logic proposed for
> RangeVarLockRelid() into RangeVarGetRelid() itself, and then went
> through and examined all the callers of RangeVarGetRelid().  There are
> some, such as has_table_privilege(), where it's really impractical to
> take any lock, first because we might have no privileges at all on
> that table and second because that could easily lead to a massive
> amount of locking for no particular good reason.  I believe Tom
> suggested that the right fix for these functions is to have them
> index-scan the system catalogs using the caller's MVCC snapshot, which
> would be right at least for pg_dump.  And there are other callers that
> cannot acquire the lock as part of RangeVarGetRelid() for a variety of
> other reasons.  However, having said that, there do appear to be a
> number of cases that are can be fixed fairly easily.
> 
> So here's a (heavily) updated patch that tries to do that, along with
> adding comments to the places where things still need more fixing.  In
> addition to the problems corrected by your last version, this fixes
> LOCK TABLE, ALTER SEQUENCE, ALTER TABLE .. RENAME, the whole-table
> variant of REINDEX, CREATE CONSTRAINT TRIGGER (which is flat-out wrong
> as it stands, since it acquires *no lock at all* on the table
> specified in the FROM clause, never mind the question of doing so
> atomically), CREATE RULE, and (partially) DROP TRIGGER and DROP RULE.

Looks basically sound; see a few code comments below.

> Regardless of exactly how we decide to proceed here, it strikes me
> that there is a heck of a lot more work that could stand to be done in
> this area...  :-(

Yes.  DDL-DDL concurrency is a much smaller practical concern, but it is a
quality-of-implementation hole.

> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c

> @@ -238,37 +246,121 @@ RangeVarGetRelid(const RangeVar *relation, bool failOK)
>      }
>  
>      /*
> -     * Some non-default relpersistence value may have been specified.  The
> -     * parser never generates such a RangeVar in simple DML, but it can happen
> -     * in contexts such as "CREATE TEMP TABLE foo (f1 int PRIMARY KEY)".  Such
> -     * a command will generate an added CREATE INDEX operation, which must be
> -     * careful to find the temp table, even when pg_temp is not first in the
> -     * search path.
> +     * If lockmode = NoLock, the caller is assumed to already hold some sort
> +     * of heavyweight lock on the target relation.  Otherwise, we're preceding
> +     * here with no lock at all, which means that any answers we get must be
> +     * viewed with a certain degree of suspicion.  In particular, any time we
> +     * AcceptInvalidationMessages(), the anwer might change.  We handle that
> +     * case by retrying the operation until either (1) no more invalidation
> +     * messages show up or (2) the answer doesn't change.

The third sentence is true for all lock levels.  The fourth sentence is true
for lock levels _except_ NoLock.

> +        /*
> +         * If no lock requested, we assume the caller knows what they're
> +         * doing.  They should have already acquired a heavyweight lock on
> +         * this relation earlier in the processing of this same statement,
> +         * so it wouldn't be appropriate to AcceptInvalidationMessages()
> +         * here, as that might pull the rug out from under them.
> +         */

What sort of rug-pullout do you have in mind?  Also, I don't think it matters
if the caller acquired the lock during this _statement_.  It just needs to
hold a lock, somehow.

> --- a/src/backend/commands/lockcmds.c
> +++ b/src/backend/commands/lockcmds.c

> @@ -69,67 +70,10 @@ LockTableCommand(LockStmt *lockstmt)
>   * "rv" is NULL and we should just silently ignore any dropped child rel.

This comment refers to a now-removed argument.

>   */
>  static void
> -LockTableRecurse(Oid reloid, RangeVar *rv,
> -                 LOCKMODE lockmode, bool nowait, bool recurse)
> +LockTableRecurse(Relation rel, LOCKMODE lockmode, bool nowait, bool recurse)

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -764,8 +764,15 @@ RemoveRelations(DropStmt *drop)
>           */
>          AcceptInvalidationMessages();
>  
> -        /* Look up the appropriate relation using namespace search */
> -        relOid = RangeVarGetRelid(rel, true);
> +        /*
> +         * Look up the appropriate relation using namespace search.
> +         *
> +         * XXX: Doing this without a lock is unsafe in the presence of
> +         * concurrent DDL, but acquiring a lock here might violate the rule
> +         * that a table must be locked before its corresponding index.
> +         * So, for now, we ignore the hazzard.

Spelling.

> --- a/src/backend/rewrite/rewriteDefine.c
> +++ b/src/backend/rewrite/rewriteDefine.c
> @@ -196,11 +196,15 @@ DefineRule(RuleStmt *stmt, const char *queryString)
>      Node       *whereClause;
>      Oid            relId;
>  
> -    /* Parse analysis ... */
> +    /* Parse analysis. */
>      transformRuleStmt(stmt, queryString, &actions, &whereClause);
>  
> -    /* ... find the relation ... */
> -    relId = RangeVarGetRelid(stmt->relation, false);
> +    /*
> +     * Find and lock the relation.  Lock level should match
> +     * DefineQueryRewrite.
> +     */
> +    relId = RangeVarGetRelid(stmt->relation, AccessExclusiveLock, false,
> +                             false);

Seems better to just pass the RangeVar to DefineQueryRewrite() ...

>  
>      /* ... and execute */
>      DefineQueryRewrite(stmt->rulename,
> @@ -235,17 +239,8 @@ DefineQueryRewrite(char *rulename,
>      Query       *query;
>      bool        RelisBecomingView = false;
>  
> -    /*
> -     * If we are installing an ON SELECT rule, we had better grab
> -     * AccessExclusiveLock to ensure no SELECTs are currently running on the
> -     * event relation.    For other types of rules, it is sufficient to grab
> -     * ShareRowExclusiveLock to lock out insert/update/delete actions and to
> -     * ensure that we lock out current CREATE RULE statements.
> -     */
> -    if (event_type == CMD_SELECT)
> -        event_relation = heap_open(event_relid, AccessExclusiveLock);
> -    else
> -        event_relation = heap_open(event_relid, ShareRowExclusiveLock);
> +    /* lock level should match the one used in DefineRule */
> +    event_relation = heap_open(event_relid, AccessExclusiveLock);

... also making it cleaner to preserve this optimization.

Incidentally, you've added in many places this pattern of commenting that a
lock level must match another lock level used elsewhere.  Would it be better
to migrate away from looking up a relation oid in one function and opening the
relation in another function, instead passing either a Relation or a RangeVar?
You did substitute passing a Relation in a couple of places.

Thanks,
nm