Обсуждение: BUG #5988: CTINE duplicates constraints
The following bug has been logged online: Bug reference: 5988 Logged by: Marko Tiikkaja Email address: marko.tiikkaja@2ndquadrant.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.tiikkaja@2ndquadrant.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@sss.pgh.pa.us> wrote: > "Marko Tiikkaja" <marko.tiikkaja@2ndquadrant.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. =A0My 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=3D> 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. --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Apr 20, 2011 at 7:13 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Apr 20, 2011 at 7:13 PM, Tom Lane <tgl@sss.pgh.pa.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