On Tuesday, February 28, 2012 12:43:02 AM Andres Freund wrote:
> On Tuesday, February 28, 2012 12:30:36 AM Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > Sorry for letting this slide.
> > >
> > > Is it worth adding this bit to OpenIntoRel? Not sure if there is danger
> > > in allowing anyone to create shared tables
> > >
> > > /* In all cases disallow placing user relations in pg_global */
> > > if (tablespaceId == GLOBALTABLESPACE_OID)
> > >
> > > ereport(ERROR,
> > >
> > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > >
> > > errmsg("only shared relations can be placed in pg_global
> > >
> > > tablespace")));
> >
> > Ugh ... if that's currently allowed, we definitely need to fix it.
> > But I'm not sure OpenIntoRel is the right place. I'd have expected
> > the test to be at some lower level, like heap_create_with_catalog
> > or so.
>
> Its definitely allowed right now:
>
> test-upgrade=# CREATE TABLE foo TABLESPACE pg_global AS SELECT 1;
> SELECT 1
> Time: 354.097 ms
>
> The analogous check for the missing one in OpenIntoRel for plain relations
> is in defineRelation. heap_create_with_catalog only contains the inverse
> check:
>
> /*
> * Shared relations must be in pg_global (last-ditch check)
> */
> if (shared_relation && reltablespace != GLOBALTABLESPACE_OID)
> elog(ERROR, "shared relations must be placed in pg_global
> tablespace");
>
>
> Moving it there sounds like a good idea without any problem I can see right
> now. Want me to prepare a patch or is it just the same for you if you do it
> yourself?
Sorry to bother you with that dreary topic further, but this should probably
be fixed before the next set of stable releases.
The check cannot easily be moved to heap_create_with_catalog because e.g.
cluster.c's make_new_heap does heap_create_with_catalog for a temporary copy
of shared relations without being able to mark them as such (because they
don't have the right oid and thus IsSharedRelation would cry). So I think just
adding the same check to the ctas code as the normal DefineRelation contains
is the best way forward.
The attached patch applies from 8.3 to 9.1 (8.2 has conflicts but
thankfully...).
Andres