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
