Thread: RangeVarGetRelid()
In commit 4240e429d0c2d889d0cda23c618f94e12c13ade7, we modified RangeVarGetRelid() so that it acquires a lock on the target relation "atomically" with respect to the name lookup. Since we lock OIDs, not names, that's not possible, strictly speaking, but the idea is that we detect whether any invalidation messages were processed between the time we did the lookup and the time we acquired the relation lock. If so, we double-check that the name still refers to the same object, and if not, we release the lock on the object we had locked previously and instead lock the new one. This is a good thing, because it means that if you, for example, start a transaction, drop a table, create a new one in its place, and commit the transaction, people who were blocked waiting for the table lock will correctly latch onto the new table instead of erroring out all over the place. This is obviously advantageous in production situations where switching out tables in this way has historically been quite difficult to do without user-visible disruption. The trouble with this mechanism, however, is that sometimes atomically looking up the relation and locking it is not what you want to do. For example, you might want to have a permission check in the middle, so that you don't even briefly lock a table on which the user has no permissions. Or, in the case of DROP INDEX, you might find it necessary to lock the heap before the index, as a result of our general rule that the heap locks should be acquired before index locks to reduce deadlocks. Right now, there's no good way to do this. Some code just opens the target relation with whatever lock is needed from the get-go, and we just hope the transaction will abort before it causes anyone else too much trouble. Other code looks up the relation OID without getting a lock, does its checks, and then gets the lock - but all of the places that take this approach uniformly lack the sort of retry loop that is present in RangeVarGetRelid() - and are therefore prone to latching onto a dropped relation, or maybe even checking permissions on the relation with OID 123 and then dropping the (eponymous) relation with OID 456. Although none of this seems like a huge problem in practice, it's awfully ugly, and it seems like it would be nice to clean it up. The trouble is, I'm not quite sure how to do that. It seems like permissions checks and lock-the-heap-for-this-index should be done in RangeVarGetRelid() just after the block that says "if (retry)" and just before the block that calls LockRelationOid(). That way, if we end up deciding we need to retry the name lookup, we'll retry all that other stuff as well, which is exactly right. The difficulty is that different callers have different needs for what should go in that space, to the degree that I'm a bit nervous about continuing to add arguments to that function to satisfy what everybody needs. Maybe we could make most of them Booleans and pass an "int flags" argument. Another option would be to add a "callback" argument to that function that would be called at that point with the relation, relId, and oldRelId as arguments. Alternatively, we could just resign ourselves to duplicating the loop in this function into each place in the code that has a special-purpose requirement, but the function is complex enough to make that a pretty unappealing prospect. Or we could just punt the whole thing and do nothing, but I don't like that either. Right now, for example, if user A is doing something with a table, and user B (who has no permissions) tries to use it, the pending request for an AccessExclusiveLock will block user C (who does have permissions) from doing anything until A's transaction commits or rolls back; only at that point do we realize that B has been naughty. I'd like to figure out some way to fix that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: > The trouble is, I'm not quite sure how to do that. It seems like > permissions checks and lock-the-heap-for-this-index should be done in > RangeVarGetRelid() just after the block that says "if (retry)" and > just before the block that calls LockRelationOid(). That way, if we > end up deciding we need to retry the name lookup, we'll retry all that > other stuff as well, which is exactly right. The difficulty is that > different callers have different needs for what should go in that > space, to the degree that I'm a bit nervous about continuing to add > arguments to that function to satisfy what everybody needs. Maybe we > could make most of them Booleans and pass an "int flags" argument. > Another option would be to add a "callback" argument to that function > that would be called at that point with the relation, relId, and > oldRelId as arguments. Alternatively, we could just resign ourselves > to duplicating the loop in this function into each place in the code > that has a special-purpose requirement, but the function is complex > enough to make that a pretty unappealing prospect. I'm for the callback. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: >> The trouble is, I'm not quite sure how to do that. It seems like >> permissions checks and lock-the-heap-for-this-index should be done in >> RangeVarGetRelid() just after the block that says "if (retry)" and >> just before the block that calls LockRelationOid(). That way, if we >> end up deciding we need to retry the name lookup, we'll retry all that >> other stuff as well, which is exactly right. The difficulty is that >> different callers have different needs for what should go in that >> space, to the degree that I'm a bit nervous about continuing to add >> arguments to that function to satisfy what everybody needs. Maybe we >> could make most of them Booleans and pass an "int flags" argument. >> Another option would be to add a "callback" argument to that function >> that would be called at that point with the relation, relId, and >> oldRelId as arguments. Alternatively, we could just resign ourselves >> to duplicating the loop in this function into each place in the code >> that has a special-purpose requirement, but the function is complex >> enough to make that a pretty unappealing prospect. > > I'm for the callback. I worked up the attached patch showing how this might work. For demonstration purposes, the attached patch modifies REINDEX INDEX and REINDEX TABLE to use this facility. It turns out that, in the current code, they have opposite problems, so this is a good example of how this gives you the best of both worlds. Suppose we do: rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE rhaas=# begin; BEGIN rhaas=# insert into foo values (1); INSERT 0 1 At this point, if we start up another session as non-privileged user, REINDEX INDEX foo_pkey will immediately fail with a permissions error (which is good), but REINDEX TABLE foo will queue up for a table lock (which is bad). Now suppose we set up like this: rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE rhaas=# begin; BEGIN rhaas=# drop table foo; DROP TABLE rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE If we open another session as superuser and say "REINDEX INDEX foo_pkey", and then commit the open transaction in the first session, it will fail: rhaas=# reindex index foo_pkey; ERROR: could not open relation with OID 16481 However, with the same setup, "REINDEX TABLE foo" will work. With the attached patch, both situations are handled correctly by both commands. Having now written the patch, I'm convinced that the callback is in fact the right choice. It requires only very minor reorganization of the existing code, which is always a plus. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: > On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: > >> The trouble is, I'm not quite sure how to do that. ?It seems like > >> permissions checks and lock-the-heap-for-this-index should be done in > >> RangeVarGetRelid() just after the block that says "if (retry)" and > >> just before the block that calls LockRelationOid(). ?That way, if we > >> end up deciding we need to retry the name lookup, we'll retry all that > >> other stuff as well, which is exactly right. ?The difficulty is that > >> different callers have different needs for what should go in that > >> space, to the degree that I'm a bit nervous about continuing to add > >> arguments to that function to satisfy what everybody needs. ?Maybe we > >> could make most of them Booleans and pass an "int flags" argument. > >> Another option would be to add a "callback" argument to that function > >> that would be called at that point with the relation, relId, and > >> oldRelId as arguments. ?Alternatively, we could just resign ourselves > >> to duplicating the loop in this function into each place in the code > >> that has a special-purpose requirement, but the function is complex > >> enough to make that a pretty unappealing prospect. > > > > I'm for the callback. > Having now written the patch, I'm convinced that the callback is in > fact the right choice. It requires only very minor reorganization of > the existing code, which is always a plus. +1 How about invoking the callback earlier, before "if (lockmode == NoLock)"? That way, a REINDEX TABLE that blocks against an ALTER TABLE OWNER TO will respect the new owner. Also, the callback will still get used in the NoLock case. Callbacks that would have preferred the old positioning can check relId == oldRelId to distinguish. > --- a/src/include/catalog/namespace.h > +++ b/src/include/catalog/namespace.h > @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath > bool addTemp; /* implicitly prepend temp schema? */ > } OverrideSearchPath; > > +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid relId, > + Oid oldRelId); > > -extern Oid RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, > - bool missing_ok, bool nowait); > +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ > + RangeVarGetRelidExtended(relation, lockmode, missing_ok, nowait, NULL) > + > +extern Oid RangeVarGetRelidExtended(const RangeVar *relation, > + LOCKMODE lockmode, bool missing_ok, bool nowait, > + RangeVarGetRelidCallback callback); I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. Shall we also default missing_ok=false and nowait=false for RangeVarGetRelid? Thanks, nm
On Thu, Nov 17, 2011 at 10:48 PM, Noah Misch <noah@leadboat.com> wrote: > On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: >> On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: >> > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: >> >> The trouble is, I'm not quite sure how to do that. ?It seems like >> >> permissions checks and lock-the-heap-for-this-index should be done in >> >> RangeVarGetRelid() just after the block that says "if (retry)" and >> >> just before the block that calls LockRelationOid(). ?That way, if we >> >> end up deciding we need to retry the name lookup, we'll retry all that >> >> other stuff as well, which is exactly right. ?The difficulty is that >> >> different callers have different needs for what should go in that >> >> space, to the degree that I'm a bit nervous about continuing to add >> >> arguments to that function to satisfy what everybody needs. ?Maybe we >> >> could make most of them Booleans and pass an "int flags" argument. >> >> Another option would be to add a "callback" argument to that function >> >> that would be called at that point with the relation, relId, and >> >> oldRelId as arguments. ?Alternatively, we could just resign ourselves >> >> to duplicating the loop in this function into each place in the code >> >> that has a special-purpose requirement, but the function is complex >> >> enough to make that a pretty unappealing prospect. >> > >> > I'm for the callback. > >> Having now written the patch, I'm convinced that the callback is in >> fact the right choice. It requires only very minor reorganization of >> the existing code, which is always a plus. > > +1 > > How about invoking the callback earlier, before "if (lockmode == NoLock)"? > That way, a REINDEX TABLE that blocks against an ALTER TABLE OWNER TO will > respect the new owner. Also, the callback will still get used in the NoLock > case. Callbacks that would have preferred the old positioning can check relId > == oldRelId to distinguish. Hmm, I guess that would work. >> --- a/src/include/catalog/namespace.h >> +++ b/src/include/catalog/namespace.h >> @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath >> bool addTemp; /* implicitly prepend temp schema? */ >> } OverrideSearchPath; >> >> +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid relId, >> + Oid oldRelId); >> >> -extern Oid RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, >> - bool missing_ok, bool nowait); >> +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ >> + RangeVarGetRelidExtended(relation, lockmode, missing_ok, nowait, NULL) >> + >> +extern Oid RangeVarGetRelidExtended(const RangeVar *relation, >> + LOCKMODE lockmode, bool missing_ok, bool nowait, >> + RangeVarGetRelidCallback callback); > > I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. Shall we > also default missing_ok=false and nowait=false for RangeVarGetRelid? I thought about that, but just did it this way for now to keep the patch small for review purposes. nowait = true is only used in one place, so it probably makes sense to default it to false in the non-extended version. But there are a number of callers who want missing_ok = true behavior, so I'm inclined not to think that one should be available in the non-extended version. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Nov 17, 2011 at 11:49:06PM -0500, Robert Haas wrote: > On Thu, Nov 17, 2011 at 10:48 PM, Noah Misch <noah@leadboat.com> wrote: > > On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: > >> --- a/src/include/catalog/namespace.h > >> +++ b/src/include/catalog/namespace.h > >> @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath > >> ? ? ? bool ? ? ? ? ? ?addTemp; ? ? ? ? ? ? ? ?/* implicitly prepend temp schema? */ > >> ?} OverrideSearchPath; > >> > >> +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid relId, > >> + ? ? Oid oldRelId); > >> > >> -extern Oid ? RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, > >> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?bool missing_ok, bool nowait); > >> +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ > >> + ? ? ? ? ? ? RangeVarGetRelidExtended(relation, lockmode, missing_ok, nowait, NULL) > >> + > >> +extern Oid ? RangeVarGetRelidExtended(const RangeVar *relation, > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?LOCKMODE lockmode, bool missing_ok, bool nowait, > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?RangeVarGetRelidCallback callback); > > > > I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. ?Shall we > > also default missing_ok=false and nowait=false for RangeVarGetRelid? > > I thought about that, but just did it this way for now to keep the > patch small for review purposes. Fair enough. > nowait = true is only used in one > place, so it probably makes sense to default it to false in the > non-extended version. But there are a number of callers who want > missing_ok = true behavior, so I'm inclined not to think that one > should be available in the non-extended version. [assuming the "not" in your last sentence was unintended] I count 1/25 callers overriding nowait and 3/25 overriding missing_ok. So, it's looking like a less-common override than the callback function will come to be.
On Fri, Nov 18, 2011 at 8:37 AM, Noah Misch <noah@leadboat.com> wrote: > I count 1/25 callers overriding nowait and 3/25 overriding missing_ok. So, it's > looking like a less-common override than the callback function will come to be. Yeah, you're probably right. However, I think there's another good reason not to use that signature: in 9.1, the function had a signature of (RangeVar *, bool). If in 9.2 it ends up with a signature of (RangeVar *, LOCKMODE), you won't get a compiler warning (at least not on my system) if you invoke it as RangeVarGetRelid(rel, true). You'll just get silently (and subtly) different behavior: an error if the relataion doesn't exist (instead of no error), and an AccessShareLock if it does (instead of no lock). I think we're best off making sure that any old-style usage of RangeVarGetRelid() that may be out there in third-party code fails to compile. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Nov 18, 2011 at 08:58:30AM -0500, Robert Haas wrote: > On Fri, Nov 18, 2011 at 8:37 AM, Noah Misch <noah@leadboat.com> wrote: > > I count 1/25 callers overriding nowait and 3/25 overriding missing_ok. ?So, it's > > looking like a less-common override than the callback function will come to be. > > Yeah, you're probably right. However, I think there's another good > reason not to use that signature: in 9.1, the function had a signature > of (RangeVar *, bool). If in 9.2 it ends up with a signature of > (RangeVar *, LOCKMODE), you won't get a compiler warning (at least not > on my system) if you invoke it as RangeVarGetRelid(rel, true). You'll > just get silently (and subtly) different behavior: an error if the > relataion doesn't exist (instead of no error), and an AccessShareLock > if it does (instead of no lock). I think we're best off making sure > that any old-style usage of RangeVarGetRelid() that may be out there > in third-party code fails to compile. Good call.
On Fri, Nov 18, 2011 at 9:12 AM, Noah Misch <noah@leadboat.com> wrote: > Good call. All right, here's an updated patch, and a couple of follow-on patches. I updated the main patch (rangevargetrelid-callback-v2.patch) per previous discussion. I also added a "callback_arg" argument to the RangeVarGetRelidExtended(), because it's a law of nature that all callback functions need such a thing, and this case proved to be no exception: the old version of the REINDEX INDEX callback was flaky, since it assumed that the index it locked during one iteration would still exist during the next iteration, which won't be true when the retry is caused by the index having been concurrently dropped. There were some other bugs as well, which I believe I've now fixed. fix-lock-table.patch applies over rangevargetrelid-callback-v2.patch and adjusts LOCK TABLE so that we never obtain a relation lock before validating that the object is a table and we have permission to lock it. fix-drop-relation.patch applies over that, and makes similar corrections to the logic for DROP TABLE/INDEX/SEQUENCE/VIEW/FOREIGN TABLE. This means that it's no longer possible to use these commands to launch a denial-of-service attack against a table you have no rights to. Sadly, there are a bunch of other commands that can be used the same way. I'd like to fix them all, but it's a decent amount of work, so I'm working through them one at a time. In the case of DROP, this also improves handling of concurrent DROP, DROP-and-CREATE, RENAME-and-CREATE, and similar situations. For example, in unpatched master: rhaas=# create table t (a int); CREATE TABLE rhaas=# begin; BEGIN rhaas=# drop table t; DROP TABLE rhaas=# create table t (b int); CREATE TABLE And then, from another session: rhaas=# drop table t; When the first session commits, you get something like this: ERROR: cache lookup failed for relation 16401 With these patches, that goes away: the concurrent DROP correctly sees the new relation and drops that one. Or, if the concurrent transaction just does a DROP rather than a DROP-and-CREATE, then you get an error - but instead of a somewhat incomprehensible complaint about a cache lookup having failed, you get the same error you would have gotten had the relation not existed in the first place: ERROR: table "t" does not exist -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Thu, Nov 24, 2011 at 10:54:50AM -0500, Robert Haas wrote:
> All right, here's an updated patch, and a couple of follow-on patches.
Your committed patch looks great overall. A few cosmetic points:
> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c
> @@ -309,6 +313,19 @@ RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, bool missing_ok,
> }
>
> /*
> + * Invoke caller-supplied callback, if any.
> + *
> + * This callback is a good place to check permissions: we haven't taken
> + * the table lock yet (and it's really best to check permissions before
> + * locking anything!), but we've gotten far enough to know what OID we
> + * think we should lock. Of course, concurrent DDL might things while
That last sentence needs a word around "might things".
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -767,15 +775,13 @@ RemoveRelations(DropStmt *drop)
> */
> AcceptInvalidationMessages();
The above call can go away, now.
> @@ -843,6 +804,74 @@ RemoveRelations(DropStmt *drop)
> }
>
> /*
> + * Before acquiring a table lock, check whether we have sufficient rights.
> + * In the case of DROP INDEX, also try to lock the table before the index.
> + */
> +static void
> +RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid oldRelOid,
> + void *arg)
> +{
> + HeapTuple tuple;
> + struct DropRelationCallbackState *state;
> + char relkind;
> + Form_pg_class classform;
> +
> + state = (struct DropRelationCallbackState *) arg;
> + relkind = state->relkind;
> +
> + /*
> + * If we previously locked some other index's heap, and the name we're
> + * looking up no longer refers to that relation, release the now-useless
> + * lock.
> + */
> + if (relOid != oldRelOid && OidIsValid(state->heapOid))
> + {
> + UnlockRelationOid(state->heapOid, AccessExclusiveLock);
> + state->heapOid = InvalidOid;
> + }
> +
> + /* Didn't find a relation, so need for locking or permission checks. */
That sentence needs a word around "so need".
Thanks,
nm
On Mon, Dec 5, 2011 at 2:09 AM, Noah Misch <noah@leadboat.com> wrote: > Your committed patch looks great overall. A few cosmetic points: Thanks for the review. > That last sentence needs a word around "might things". Fixed. >> AcceptInvalidationMessages(); > > The above call can go away, now. Doesn't that still protect us against namespace-shadowing issues? RangeVarGetRelid doesn't actually AcceptInvalidationMessages() at the top. > That sentence needs a word around "so need". Fixed. Attached please find a patch with some more fixes on this same general theme. This one tackles renaming of relations, columns, and triggers; and changing the schema of relations. In these cases, the current code does a permissions check before locking the table (which is good) and uses RangeVarGetRelid() to guard against "cache lookup failure" errors caused by concurrent DDL (also good). However, if the referent of the name changes during the lock wait, we don't recheck permissions; we just allow the rename or schema change on the basis that the user had permission to do it to the relation that formerly had that name. While this is pretty minor as security concerns go, it seems best to clean it up, so this patch does that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Tue, Dec 06, 2011 at 04:02:31PM -0500, Robert Haas wrote: > On Mon, Dec 5, 2011 at 2:09 AM, Noah Misch <noah@leadboat.com> wrote: > >> ? ? ? ? ? ? ? AcceptInvalidationMessages(); > > > > The above call can go away, now. > > Doesn't that still protect us against namespace-shadowing issues? > RangeVarGetRelid doesn't actually AcceptInvalidationMessages() at the > top. 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. FWIW, this is fairly independent of your latest patches -- I just happened to notice it due to the incidental quotation. > Attached please find a patch with some more fixes on this same general > theme. This one tackles renaming of relations, columns, and triggers; > and changing the schema of relations. In these cases, the current > code does a permissions check before locking the table (which is good) > and uses RangeVarGetRelid() to guard against "cache lookup failure" > errors caused by concurrent DDL (also good). However, if the referent > of the name changes during the lock wait, we don't recheck > permissions; we just allow the rename or schema change on the basis > that the user had permission to do it to the relation that formerly > had that name. While this is pretty minor as security concerns go, it > seems best to clean it up, so this patch does that. 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. 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 like how this patch reduces the arbitrary division of authorization checks between alter.c and tablecmds.c. Thanks, nm
On Wed, Dec 7, 2011 at 8:42 AM, Noah Misch <noah@leadboat.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. >> Attached please find a patch with some more fixes on this same general >> theme. This one tackles renaming of relations, columns, and triggers; >> and changing the schema of relations. In these cases, the current >> code does a permissions check before locking the table (which is good) >> and uses RangeVarGetRelid() to guard against "cache lookup failure" >> errors caused by concurrent DDL (also good). However, if the referent >> of the name changes during the lock wait, we don't recheck >> permissions; we just allow the rename or schema change on the basis >> that the user had permission to do it to the relation that formerly >> had that name. While this is pretty minor as security concerns go, it >> seems best to clean it up, so this patch does that. > > 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. It's amazing how many subtly different permissions requirements there are here, but none of them seem terribly ill-founded, and, in any case, changing them is a job for another day if it's to be done at all. > 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. > I like how this patch reduces the arbitrary division of authorization checks > between alter.c and tablecmds.c. Thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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@leadboat.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.
On Fri, Dec 9, 2011 at 5:41 PM, Noah Misch <noah@leadboat.com> wrote: > 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. I agree, but my point is that so far we have no callbacks that differ only in that detail. I accept that we'd probably want to avoid that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Dec 15, 2011 at 07:04:20PM -0500, Robert Haas wrote: > On Fri, Dec 9, 2011 at 5:41 PM, Noah Misch <noah@leadboat.com> wrote: > > 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. > > I agree, but my point is that so far we have no callbacks that differ > only in that detail. I accept that we'd probably want to avoid that. To illustrate what I had in mind, here's a version of your patch that has five callers sharing a callback. The patch is against d039fd51f79e, just prior to your recent commits.
Attachment
On Fri, Dec 16, 2011 at 10:32 AM, Noah Misch <noah@leadboat.com> wrote: >> I agree, but my point is that so far we have no callbacks that differ >> only in that detail. I accept that we'd probably want to avoid that. > > To illustrate what I had in mind, here's a version of your patch that has five > callers sharing a callback. The patch is against d039fd51f79e, just prior to > your recent commits. I don't think RenameRelation() ought to wait until we've taken the lock before doing this: + aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE); + if (aclresult != ACLCHECK_OK) + aclcheck_error(aclresult, ACL_KIND_NAMESPACE, + get_namespace_name(namespaceId)); I'm not sure how we can distinguished in a principled way between permissions checks that must be conducted before locking the relation and those that can be done afterwards. And I don't really want to make that distinction, anyhow. But I see your point, otherwise. What I'm tempted to do is define a new function RangeVarGetRelidCheckOwner(RangeVar *relation, LOCKMODE lockmode, bool system_tables_ok) that will arrange to look up the OID, check that you own it, optionally check that it's not a system table, and acquire the specified lock. Internally that will call RangeVarGetRelidExtended() with a callback; the callback arg can be used to pass through the system_tables_ok flag. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Dec 19, 2011 at 10:11:23AM -0500, Robert Haas wrote: > On Fri, Dec 16, 2011 at 10:32 AM, Noah Misch <noah@leadboat.com> wrote: > >> I agree, but my point is that so far we have no callbacks that differ > >> only in that detail. ?I accept that we'd probably want to avoid that. > > > > To illustrate what I had in mind, here's a version of your patch that has five > > callers sharing a callback. ?The patch is against d039fd51f79e, just prior to > > your recent commits. > > I don't think RenameRelation() ought to wait until we've taken the > lock before doing this: > > + aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE); > + if (aclresult != ACLCHECK_OK) > + aclcheck_error(aclresult, ACL_KIND_NAMESPACE, > + get_namespace_name(namespaceId)); > > I'm not sure how we can distinguished in a principled way between > permissions checks that must be conducted before locking the relation > and those that can be done afterwards. And I don't really want to > make that distinction, anyhow. Here's the rule I used: the pre-lock checks must prove that the user could, by some command working as-designed, have taken a lock at least as strong as the one we're about to acquire. How does that work out in practice? Relation owners can always take AccessExclusiveLock by way of a DROP command, so ownership is always sufficient as a pre-lock test. The above namespace check is no exception; the object owner has authority to take the AccessExclusiveLock independent of his authority to ultimately complete the rename. (Changes arising from the recent discussions about locking namespaces during DDL would complicate the situation in other ways.) > But I see your point, otherwise. What I'm tempted to do is define a > new function RangeVarGetRelidCheckOwner(RangeVar *relation, LOCKMODE > lockmode, bool system_tables_ok) that will arrange to look up the OID, > check that you own it, optionally check that it's not a system table, > and acquire the specified lock. Internally that will call > RangeVarGetRelidExtended() with a callback; the callback arg can be > used to pass through the system_tables_ok flag. Is the system catalog check special? Thanks, nm
On Mon, Dec 19, 2011 at 11:55 AM, Noah Misch <noah@leadboat.com> wrote: > Here's the rule I used: the pre-lock checks must prove that the user could, by > some command working as-designed, have taken a lock at least as strong as the > one we're about to acquire. How does that work out in practice? Relation > owners can always take AccessExclusiveLock by way of a DROP command, so > ownership is always sufficient as a pre-lock test. > > The above namespace check is no exception; the object owner has authority to > take the AccessExclusiveLock independent of his authority to ultimately > complete the rename. (Changes arising from the recent discussions about > locking namespaces during DDL would complicate the situation in other ways.) Yes: and I have it mind to work on that for the current release cycle, time permitting. It seems to me that there's no real value in consolidating this callback if there's a change coming down the pipe that would require us to split it right back out again. Also, while I see the point that it wouldn't be a security issue to defer this particular check until after the lock is taken, I still don't think it's a good idea. I basically don't see any point in splitting up the security checks across multiple functions. Our permissions-checking logic is already rather diffuse; I think that finding more reasons for some of it to be done in one function and some of it to be done in some other function is going in the wrong direction. It makes it easier for future people editing the code to rearrange things in ways that subtly break security without realizing it, and it's also one more obstacle for things like sepgsql which would like to get control whenever we do a security check. (Dimitri's recent comments about command triggers suggest that MAC isn't the only application of an option to override the default access control policy.) It's also not terribly consistent from a user perspective: hmm, if I don't have permission to do operation X because of reason A, then my command fails immediately; if I don't have permission because of reason B, then it waits for a lock and then fails. Some amount of inconsistency there is probably inevitable, because, for example, we won't know whether you have permission on an entire inheritance hierarchy until we start walking it. But I don't really see the point in going out of our way to add more of it. I think it's important to keep in mind that most of the code that is going into those callbacks is just being moved. We're not really ending up with any more code overall, except to the extent that there are a couple of lines of net increase for each callback because, well, it has a comment, and maybe a declaration, and some curly braces at the beginning and the end. The ownership check is two lines of code; it doesn't matter whether we duplicate that or not. In many cases, I think the callbacks are actually increasing readability, by pulling a bunch of related checks into their own function instead of cluttering up the main code path with them. I don't want to end up with a lot of needless code duplication, but I also don't feel any powerful need to squeeze the number of callback functions down to an absolute minimum just because I can. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Dec 19, 2011 at 01:43:18PM -0500, Robert Haas wrote: > On Mon, Dec 19, 2011 at 11:55 AM, Noah Misch <noah@leadboat.com> wrote: > > Here's the rule I used: the pre-lock checks must prove that the user could, by > > some command working as-designed, have taken a lock at least as strong as the > > one we're about to acquire. ?How does that work out in practice? ?Relation > > owners can always take AccessExclusiveLock by way of a DROP command, so > > ownership is always sufficient as a pre-lock test. > > > > The above namespace check is no exception; the object owner has authority to > > take the AccessExclusiveLock independent of his authority to ultimately > > complete the rename. ?(Changes arising from the recent discussions about > > locking namespaces during DDL would complicate the situation in other ways.) > > Yes: and I have it mind to work on that for the current release cycle, > time permitting. It seems to me that there's no real value in > consolidating this callback if there's a change coming down the pipe > that would require us to split it right back out again. > > Also, while I see the point that it wouldn't be a security issue to > defer this particular check until after the lock is taken, I still > don't think it's a good idea. I basically don't see any point in > splitting up the security checks across multiple functions. Our > permissions-checking logic is already rather diffuse; I think that > finding more reasons for some of it to be done in one function and > some of it to be done in some other function is going in the wrong > direction. It makes it easier for future people editing the code to > rearrange things in ways that subtly break security without realizing > it, and it's also one more obstacle for things like sepgsql which > would like to get control whenever we do a security check. (Dimitri's > recent comments about command triggers suggest that MAC isn't the only > application of an option to override the default access control > policy.) It's also not terribly consistent from a user perspective: > hmm, if I don't have permission to do operation X because of reason A, > then my command fails immediately; if I don't have permission because > of reason B, then it waits for a lock and then fails. Some amount of > inconsistency there is probably inevitable, because, for example, we > won't know whether you have permission on an entire inheritance > hierarchy until we start walking it. But I don't really see the point > in going out of our way to add more of it. > > I think it's important to keep in mind that most of the code that is > going into those callbacks is just being moved. We're not really > ending up with any more code overall, except to the extent that there > are a couple of lines of net increase for each callback because, well, > it has a comment, and maybe a declaration, and some curly braces at > the beginning and the end. The ownership check is two lines of code; > it doesn't matter whether we duplicate that or not. In many cases, I > think the callbacks are actually increasing readability, by pulling a > bunch of related checks into their own function instead of cluttering > up the main code path with them. I don't want to end up with a lot of > needless code duplication, but I also don't feel any powerful need to > squeeze the number of callback functions down to an absolute minimum > just because I can. I'm satisfied that you and I have a common understanding of the options and their respective merits. There's plenty of room for judgement in choosing which merits to seize at the expense of others. Your judgements here seem entirely reasonable. nm
On Mon, Dec 19, 2011 at 3:12 PM, Noah Misch <noah@leadboat.com> wrote: > I'm satisfied that you and I have a common understanding of the options and > their respective merits. There's plenty of room for judgement in choosing > which merits to seize at the expense of others. Your judgements here seem > entirely reasonable. Thanks. I'm not trying to be difficult. After staring at this for quite a while longer, it seemed to me that the logic for renaming a relation was similar enough to the logic for changing a schema that the two calbacks could reasonably be combined using a bit of conditional logic; and that, further, the same callback could be used, with a small amount of additional modification, for ALTER TABLE. Here's a patch to do that. I also notice that cluster() - which doesn't have a callback - has exactly the same needs as ReindexRelation() - which does. So that case can certainly share code; though I'm not quite sure what to call the shared callback, or which file to put it in. RangeVarCallbackForStorageRewrite? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Mon, Dec 19, 2011 at 11:52:54PM -0500, Robert Haas wrote:
> After staring at this for quite a while longer, it seemed to me that
> the logic for renaming a relation was similar enough to the logic for
> changing a schema that the two calbacks could reasonably be combined
> using a bit of conditional logic; and that, further, the same callback
> could be used, with a small amount of additional modification, for
> ALTER TABLE. Here's a patch to do that.
Nice.
> I also notice that cluster() - which doesn't have a callback - has
> exactly the same needs as ReindexRelation() - which does. So that
> case can certainly share code; though I'm not quite sure what to call
> the shared callback, or which file to put it in.
> RangeVarCallbackForStorageRewrite?
I'd put it in tablecmds.c and name it RangeVarCallbackOwnsTable.
A few things on the patch:
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -2560,90 +2500,26 @@ CheckTableNotInUse(Relation rel, const char *stmt)
> * Thanks to the magic of MVCC, an error anywhere along the way rolls back
> * the whole operation; we don't have to do anything special to clean up.
> *
> - * We lock the table as the first action, with an appropriate lock level
> + * The caller must lock the relation, with an appropriate lock level
> * for the subcommands requested. Any subcommand that needs to rewrite
> * tuples in the table forces the whole command to be executed with
> - * AccessExclusiveLock. If all subcommands do not require rewrite table
> - * then we may be able to use lower lock levels. We pass the lock level down
> + * AccessExclusiveLock (actually, that is currently required always, but
> + * we hope to relax it at some point). We pass the lock level down
> * so that we can apply it recursively to inherited tables. Note that the
> - * lock level we want as we recurse may well be higher than required for
> + * lock level we want as we recurse might well be higher than required for
> * that specific subcommand. So we pass down the overall lock requirement,
> * rather than reassess it at lower levels.
> */
> void
> -AlterTable(AlterTableStmt *stmt)
> +AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt)
> {
> Relation rel;
> - LOCKMODE lockmode = AlterTableGetLockLevel(stmt->cmds);
>
> - /*
> - * Acquire same level of lock as already acquired during parsing.
> - */
> - rel = relation_openrv(stmt->relation, lockmode);
> + /* Caller is required to provide an adequate lock. */
> + rel = relation_open(relid, NoLock);
>
> CheckTableNotInUse(rel, "ALTER TABLE");
>
> - /* Check relation type against type specified in the ALTER command */
> - switch (stmt->relkind)
> - {
> - case OBJECT_TABLE:
> -
> - /*
> - * For mostly-historical reasons, we allow ALTER TABLE to apply to
> - * almost all relation types.
> - */
> - if (rel->rd_rel->relkind == RELKIND_COMPOSITE_TYPE
> - || rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not a table",
> - RelationGetRelationName(rel))));
RangeVarCallbackForAlterRelation() does not preserve ALTER TABLE's refusal to
operate on foreign tables.
> - break;
> -
> - case OBJECT_INDEX:
> - if (rel->rd_rel->relkind != RELKIND_INDEX)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not an index",
> - RelationGetRelationName(rel))));
> - break;
> -
> - case OBJECT_SEQUENCE:
> - if (rel->rd_rel->relkind != RELKIND_SEQUENCE)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not a sequence",
> - RelationGetRelationName(rel))));
> - break;
> -
> - case OBJECT_TYPE:
> - if (rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not a composite type",
> - RelationGetRelationName(rel))));
> - break;
> -
> - case OBJECT_VIEW:
> - if (rel->rd_rel->relkind != RELKIND_VIEW)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not a view",
> - RelationGetRelationName(rel))));
> - break;
> -
> - case OBJECT_FOREIGN_TABLE:
> - if (rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
> - ereport(ERROR,
> - (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> - errmsg("\"%s\" is not a foreign table",
> - RelationGetRelationName(rel))));
> - break;
> -
> - default:
> - elog(ERROR, "unrecognized object type: %d", (int) stmt->relkind);
RangeVarCallbackForAlterRelation() does not preserve the check for unexpected
object types.
> - }
> -
> ATController(rel, stmt->cmds, interpretInhOption(stmt->relation->inhOpt),
> lockmode);
> }
> --- a/src/backend/tcop/utility.c
> +++ b/src/backend/tcop/utility.c
> @@ -699,12 +699,23 @@ standard_ProcessUtility(Node *parsetree,
>
> case T_AlterTableStmt:
> {
> + AlterTableStmt *atstmt = (AlterTableStmt *) parsetree;
> + Oid relid;
> List *stmts;
> ListCell *l;
> + LOCKMODE lockmode;
> +
> + /*
> + * Figure out lock mode, and acquire lock. This also does
> + * basic permissions checks, so that we won't wait for a lock
> + * on (for example) a relation on which we have no
> + * permissions.
> + */
> + lockmode = AlterTableGetLockLevel(atstmt->cmds);
> + relid = AlterTableLookupRelation(atstmt, lockmode);
>
> /* Run parse analysis ... */
> - stmts = transformAlterTableStmt((AlterTableStmt *) parsetree,
> - queryString);
> + stmts = transformAlterTableStmt(atstmt, queryString);
utility.c doesn't take locks for any other command; parse analysis usually
does that. To preserve that modularity, you could add a "bool toplevel"
argument to transformAlterTableStmt(). Pass true here, false in
ATPostAlterTypeParse(). If true, use AlterTableLookupRelation() to get full
security checks. Otherwise, just call relation_openrv() as now. Would that
be an improvement?
>
> /* ... and do it */
> foreach(l, stmts)
nm
On Tue, Dec 20, 2011 at 8:14 PM, Noah Misch <noah@leadboat.com> wrote: >> I also notice that cluster() - which doesn't have a callback - has >> exactly the same needs as ReindexRelation() - which does. So that >> case can certainly share code; though I'm not quite sure what to call >> the shared callback, or which file to put it in. >> RangeVarCallbackForStorageRewrite? > > I'd put it in tablecmds.c and name it RangeVarCallbackOwnsTable. OK. > RangeVarCallbackForAlterRelation() does not preserve ALTER TABLE's refusal to > operate on foreign tables. I should probably fix that, but I'm wondering if I ought to fix it by disallowing the use of ALTER TABLE on FOREIGN TABLEs for the other commands that share the callback as well. Allowing ALTER TABLE to apply to any relation type is mostly a legacy thing, I think, and any code that's new enough to know about foreign tables isn't old enough to know about the time when you had to use ALTER TABLE to rename views. > RangeVarCallbackForAlterRelation() does not preserve the check for unexpected > object types. I don't feel a strong need to retain that. > utility.c doesn't take locks for any other command; parse analysis usually > does that. To preserve that modularity, you could add a "bool toplevel" > argument to transformAlterTableStmt(). Pass true here, false in > ATPostAlterTypeParse(). If true, use AlterTableLookupRelation() to get full > security checks. Otherwise, just call relation_openrv() as now. Would that > be an improvement? Not sure. I feel that it's unwise to pass relation names all over the backend and assume that nothing will change meanwhile; no locking we do will prevent that, at least in the case of search path interposition. Ultimately I think this ought to be restructured somehow so that we look up each name ONCE and ever-after refer only to the resulting OID (except for error message text). But I'm not sure how to do that, and thought it might make sense to commit this much independently of such a refactoring. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Dec 21, 2011 at 03:16:39PM -0500, Robert Haas wrote: > On Tue, Dec 20, 2011 at 8:14 PM, Noah Misch <noah@leadboat.com> wrote: > > RangeVarCallbackForAlterRelation() does not preserve ALTER TABLE's refusal to > > operate on foreign tables. > > I should probably fix that, but I'm wondering if I ought to fix it by > disallowing the use of ALTER TABLE on FOREIGN TABLEs for the other > commands that share the callback as well. Allowing ALTER TABLE to > apply to any relation type is mostly a legacy thing, I think, and any > code that's new enough to know about foreign tables isn't old enough > to know about the time when you had to use ALTER TABLE to rename > views. Maybe. Now that we have a release with these semantics, I'd lean toward preserving the wart and being more careful next time. It's certainly a borderline case, though. > > RangeVarCallbackForAlterRelation() does not preserve the check for unexpected > > object types. > > I don't feel a strong need to retain that. Okay. > > utility.c doesn't take locks for any other command; parse analysis usually > > does that. ?To preserve that modularity, you could add a "bool toplevel" > > argument to transformAlterTableStmt(). ?Pass true here, false in > > ATPostAlterTypeParse(). ?If true, use AlterTableLookupRelation() to get full > > security checks. ?Otherwise, just call relation_openrv() as now. ?Would that > > be an improvement? > > Not sure. I feel that it's unwise to pass relation names all over the > backend and assume that nothing will change meanwhile; no locking we > do will prevent that, at least in the case of search path > interposition. Ultimately I think this ought to be restructured > somehow so that we look up each name ONCE and ever-after refer only to > the resulting OID (except for error message text). But I'm not sure > how to do that, and thought it might make sense to commit this much > independently of such a refactoring. I agree with all that, though my suggestion would not have increased the number of by-name lookups.