BUG #5988: CTINE duplicates constraints

The following bug has been logged online:

Bug reference: 5988
Logged by: Marko Tiikkaja
Email address: marko(dot)tiikkaja(at)2ndquadrant(dot)com
PostgreSQL version: git master
Operating system: Linux
Description: CTINE duplicates constraints
Details:

CREATE TABLE IF NOT EXISTS duplicates some constraints if the new table
isn't created:

=# create table foo(a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey"
for table "foo"
CREATE TABLE

=# create table if not exists foo(a int primary key);
NOTICE: relation "foo" already exists, skipping
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey1"
for table "foo"
CREATE TABLE

=# \d foo
Table "public.foo"
Column | Type | Modifiers
--------+---------+-----------
a | integer | not null
Indexes:
"foo_pkey" PRIMARY KEY, btree (a)
"foo_pkey1" PRIMARY KEY, btree (a)

It seems to do that at least for PRIMARY KEY and UNIQUE constraints.

"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.)

regards, tom lane

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Apr 20, 2011 at 7:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

> 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.

I will not stand in the way of someone else coming up with a less broken
implementation. But as you've noted, that seems to be a somewhat less
than trivial project. And time grows short. I don't think it's
unreasonable at all to pull this feature from 9.1 and let someone who
cares about it submit a rewritten patch for 9.2.

regards, tom lane

On Mon, Apr 25, 2011 at 11:06 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Apr 20, 2011 at 7:13 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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.
>
>> 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.
>
> I will not stand in the way of someone else coming up with a less broken
> implementation.  But as you've noted, that seems to be a somewhat less
> than trivial project.  And time grows short.  I don't think it's
> unreasonable at all to pull this feature from 9.1 and let someone who
> cares about it submit a rewritten patch for 9.2.

Well, I'd like to make at least some minimal effort to see whether we
can fix it before we give up on it completely. A little poking around
suggests that this actually isn't that hard. Attached please find a
proposed patch which fixes the Marko's original complaint, the related
problem with serial columns that you noted, and the information leak I
noted of in my previous reply. The guts of the change are in
transformCreateStmt(). It might eventually be worth doing some more
extensive refactoring in this area, but this seems good enough for
now.

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