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.
