Re: BUG #5988: CTINE duplicates constraints

On Wed, Apr 20, 2011 at 7:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Tiikkaja" <marko(dot)tiikkaja(at)2ndquadrant(dot)com> writes:
>> CREATE TABLE IF NOT EXISTS duplicates some constraints if the new table
>> isn't created:
>
> What this means is that the feature was implemented in the wrong place,
> ie the short-circuit is after rather than before parse_utilcmds.c splits
> up the CREATE TABLE command into multiple primitive actions.
>
> I have stated previously my opinion that this is a misconceived feature,
> and it's now apparent that the implementation is as poorly thought
> through as the definition.  My recommendation is to revert that patch
> altogether.
>
> (BTW, before anyone suggests that s/continue/break/ would fix it,
> I suggest experimenting with a table containing a serial column.)

IIRC, quite a few people voiced support for this feature, so I think
that ripping it out because you don't personally like it is not a good
solution. That is exactly the sort of thing that gives us a
reputation for parochialism. In fact, I believe that the person in
response to whose complaint I wrote this patch had exactly that
complaint.

Having said that, it's not altogether obvious to me what a good fix
for this problem would look like. The reason why the check isn't done
before transformCreateStmt() is because we don't decide on the
creation namespace until we get down to DefineRelation(). We could
duplicate that logic prior to calling transformCreateStmt(). That
would rely on getting the same answer both times, but apparently we're
relying on that anyway in the case of a table with a serial column;
else we'd be at risk of putting the sequence and table in different
schemas. To avoid information leakage, we'd also need to duplicate
the permissions-checking logic that immediately follows, which is a
little ugly... but maybe not too ugly, if we encapsulate the whole
thing in a function that gets called from multiple places, rather than
duplicating the code.

In fact, it looks like we already have a bit of information leakage here:

rhaas=> create table secret.foo (a serial);
NOTICE: CREATE TABLE will create implicit sequence "foo_a_seq2" for
serial column "foo.a"
ERROR: permission denied for schema secret

We can infer the existence of foo_a_seq and foo_a_seq1.

It seems like a better solution here would be to derive the creation
namespace before we break up the statement into subcommands, and pass
the OID around after that. Then we wouldn't be doing it multiple
times and hoping to get the same answer, and the permissions checking
would be happening at the beginning of the process instead of at some
point in the middle, which is really the root cause of the above
information leak.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company