Re: RangeVarGetRelid()

On Fri, Dec 09, 2011 at 03:43:19PM -0500, Robert Haas wrote:
> On Wed, Dec 7, 2011 at 8:42 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > It narrows the window for race conditions of that genesis, but isn't doing so
> > an anti-feature? ?Even if not, doing that _only_ in RemoveRelations() is odd.
>
> I dunno. I was just reluctant to change things without a clear reason
> for doing so, and "it's not what we do in other places" didn't seem
> like enough. Really, I'd like to *always* do
> AcceptInvalidationMessages() before a name lookup, but I'm not
> convinced it's cheap enough for that.

Okay.

> > I'd suggest limiting callback functions to checks that benefit greatly from
> > preceding the lock acquisition. ?In these cases, that's only the target object
> > ownership check; all other checks can wait for the lock. ?Writing correct
> > callback code is relatively tricky; we have no lock, so the available catalog
> > entries can change arbitrarily often as the function operates. ?It's worth the
> > trouble of navigating that hazard to keep the mob from freely locking all
> > objects. ?However, if the owner of "some_table" writes "ALTER VIEW some_table
> > RENAME TO foo", I see no commensurate benefit from reporting the relkind
> > mismatch before locking the relation. ?This would also permit sharing a
> > callback among almost all the call sites you've improved so far. ?Offhand, I
> > think only ReindexIndex() would retain a bespoke callback.
>
> No, I don't think so. REINDEX INDEX needs special heap locking and
> does not reject operations on system tables, REINDEX TABLE does not
> reject operations on system tables, LOCK TABLE has special permissions
> requirements and does not reject operations on system tables, DROP
> TABLE also has special permissions requirements, RENAME ATTRIBUTE is
> "normal" (i.e. must own relation, must not apply to system tables),
> and RENAME RELATION has an extra permission check. It might not be
> worth having separate callbacks *just* to check the relkind, but I
> don't think we have any instances of that in what's already committed
> or in this patch. It's an issue that is on my mind, though; and
> perhaps as I keep cranking through the call sites some things will
> turn up that can be merged.

I forgot that you fixed RemoveRelations() and LockTable() in your last patch.
In addition to ReindexIndex(), which I mentioned, those two do need their own
callbacks in any case.

It also seems my last explanation didn't convey the point. Yes, nearly every
command has a different set of permissions checks. However, we don't benefit
equally from performing each of those checks before acquiring a lock.
Consider renameatt(), which checks three things: you must own the relation,
the relation must be of a supported relkind, and the relation must not be a
typed table. To limit opportunities for denial of service, let's definitely
perform the ownership check before taking a lock. The other two checks can
wait until we hold that lock. The benefit of checking them early is to avoid
making a careless relation owner wait for a lock before discovering the
invalidity of his command. That's nice as far as it goes, but let's not
proliferate callbacks for such a third-order benefit.

> > This patch removes two of the three callers of CheckRelationOwnership(),
> > copying some of its logic to two other sites. ?I'd suggest fixing the third
> > caller (standard_ProcessUtility() for CREATE INDEX) in this same patch. ?That
> > way, we can either delete the function now or adjust it to permit continued
> > sharing of that code under the new regime.
>
> I had the same thought, but wasn't quite sure how to do it. That code
> seems to be making the rather optimistic assumption that you can look
> up the same name as many times as you want and get the same answer.
> CheckRelationOwnership() looks it up, and then transformIndexStmt()
> looks it up again, and then DefineIndex() looks it up again ... twice,
> in the case of a concurrent build. If someone renames a relation with
> the same name into a namespace earlier in our search path during all
> this, the chances of totally nutty behavior seem pretty good; what
> happens if we do phase 2 of the concurrent index build on a different
> relation than phase 1? So I didn't attempt to tackle the problem just
> because it looked to me like the code needed a little more TLC than I
> had time to give it, but maybe it deserves a second look.

Ah, fair enough.