Thread: Shouldn't CREATE TABLE LIKE copy the relhasoids property?
dst1 doesn't get an OID column:
regression=# create table src1 (f1 int) with oids;
CREATE TABLE
regression=# create table dst1 (like src1);
CREATE TABLE
regression=# \d+ src1 Table "public.src1"Column | Type | Modifiers | Storage | Stats target |
Description
--------+---------+-----------+---------+--------------+-------------f1 | integer | | plain |
|
Has OIDs: yes
regression=# \d+ dst1 Table "public.dst1"Column | Type | Modifiers | Storage | Stats target |
Description
--------+---------+-----------+---------+--------------+-------------f1 | integer | | plain |
|
If you don't find that problematic, how about this case?
regression=# create table src2 (f1 int, primary key(oid)) with oids;
CREATE TABLE
regression=# create table dst2 (like src2 including indexes);
ERROR: column "oid" named in key does not exist
regards, tom lane
On 01/14/2015 07:29 PM, Tom Lane wrote: > dst1 doesn't get an OID column: > > regression=# create table src1 (f1 int) with oids; > CREATE TABLE > regression=# create table dst1 (like src1); > CREATE TABLE > regression=# \d+ src1 > Table "public.src1" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > f1 | integer | | plain | | > Has OIDs: yes > > regression=# \d+ dst1 > Table "public.dst1" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > f1 | integer | | plain | | > > > If you don't find that problematic, how about this case? > > regression=# create table src2 (f1 int, primary key(oid)) with oids; > CREATE TABLE > regression=# create table dst2 (like src2 including indexes); > ERROR: column "oid" named in key does not exist > > I agree it's odd, and probably wrong, although it's been like that for a very long time, hasn't it? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 01/14/2015 07:29 PM, Tom Lane wrote:
>> If you don't find that problematic, how about this case?
>>
>> regression=# create table src2 (f1 int, primary key(oid)) with oids;
>> CREATE TABLE
>> regression=# create table dst2 (like src2 including indexes);
>> ERROR: column "oid" named in key does not exist
> I agree it's odd, and probably wrong, although it's been like that for a
> very long time, hasn't it?
Sure, LIKE has always behaved this way. It still seems wrong though.
As a reference point, creating a table that inherits from src1 or src2
will result in it having oids (even if you say WITHOUT OIDS).
regards, tom lane
On Wed, Jan 14, 2015 at 07:29:24PM -0500, Tom Lane wrote: > dst1 doesn't get an OID column: > > regression=# create table src1 (f1 int) with oids; > CREATE TABLE > regression=# create table dst1 (like src1); > CREATE TABLE > regression=# \d+ src1 > Table "public.src1" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > f1 | integer | | plain | | > Has OIDs: yes > > regression=# \d+ dst1 > Table "public.dst1" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > f1 | integer | | plain | | > > > If you don't find that problematic, how about this case? > > regression=# create table src2 (f1 int, primary key(oid)) with oids; > CREATE TABLE > regression=# create table dst2 (like src2 including indexes); > ERROR: column "oid" named in key does not exist I have developed the attached patch to fix this. The code was basically confused because setting cxt.hasoids had no effect, and the LIKE relation was never checked. The fix is to default cxt.hasoids to false, set it to true if the LIKE relation has oids, and add WITH OIDS to the CREATE TABLE statement, if necessary. It also honors WITH/WITHOUT OIDS specified literally in the CREATE TABLE clause because the first specification is honored, and we only append WITH OIDS if the LIKE table has oids. Should this be listed in the release notes as a backward-incompatibility? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
Bruce Momjian wrote: > Should this be listed in the release notes as a backward-incompatibility? Isn't this a backpatchable bug fix? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Apr 9, 2015 at 12:32:23PM -0300, Alvaro Herrera wrote: > Bruce Momjian wrote: > > > Should this be listed in the release notes as a backward-incompatibility? > > Isn't this a backpatchable bug fix? Uh, I don't think so. I think users are used to the existing behavior and changing it on them will cause more harm than good. Also, we have had zero field reports about this problem. The updated attached patch handles cases where the default_with_oids = true. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On Thu, Apr 9, 2015 at 5:33 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Thu, Apr 9, 2015 at 12:32:23PM -0300, Alvaro Herrera wrote: >> Bruce Momjian wrote: >> >> > Should this be listed in the release notes as a backward-incompatibility? >> >> Isn't this a backpatchable bug fix? > > Uh, I don't think so. I think users are used to the existing behavior > and changing it on them will cause more harm than good. Also, we have > had zero field reports about this problem. I agree. This should not be back-patched, but fixing it in master seems fine. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 9, 2015 at 05:33:20PM -0400, Bruce Momjian wrote: > On Thu, Apr 9, 2015 at 12:32:23PM -0300, Alvaro Herrera wrote: > > Bruce Momjian wrote: > > > > > Should this be listed in the release notes as a backward-incompatibility? > > > > Isn't this a backpatchable bug fix? > > Uh, I don't think so. I think users are used to the existing behavior > and changing it on them will cause more harm than good. Also, we have > had zero field reports about this problem. > > The updated attached patch handles cases where the default_with_oids = > true. Slightly improved patch applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian <bruce@momjian.us> wrote: > Slightly improved patch applied. Is it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Apr 20, 2015 at 05:04:14PM -0400, Robert Haas wrote: > On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian <bruce@momjian.us> wrote: > > Slightly improved patch applied. > > Is it? The patch has a slightly modified 'if' statement to check a constant before calling a function, and use elseif: < + if (!interpretOidsOption(stmt->options, true) && cxt.hasoids)---> + if (cxt.hasoids && !interpretOidsOption(stmt->options,true))47c57< + if (interpretOidsOption(stmt->options, true) && !cxt.hasoids)---> + else if (!cxt.hasoids && interpretOidsOption(stmt->options, true)) I realize the change is subtle. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Mon, Apr 20, 2015 at 5:41 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Mon, Apr 20, 2015 at 05:04:14PM -0400, Robert Haas wrote: >> On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian <bruce@momjian.us> wrote: >> > Slightly improved patch applied. >> >> Is it? > > The patch has a slightly modified 'if' statement to check a constant > before calling a function, and use elseif: > > < + if (!interpretOidsOption(stmt->options, true) && cxt.hasoids) > --- > > + if (cxt.hasoids && !interpretOidsOption(stmt->options, true)) > 47c57 > < + if (interpretOidsOption(stmt->options, true) && !cxt.hasoids) > --- > > + else if (!cxt.hasoids && interpretOidsOption(stmt->options, true)) > > I realize the change is subtle. What I meant was - I didn't see an attachment on that message. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Apr 21, 2015 at 05:36:41PM -0400, Robert Haas wrote: > On Mon, Apr 20, 2015 at 5:41 PM, Bruce Momjian <bruce@momjian.us> wrote: > > On Mon, Apr 20, 2015 at 05:04:14PM -0400, Robert Haas wrote: > >> On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian <bruce@momjian.us> wrote: > >> > Slightly improved patch applied. > >> > >> Is it? > > > > The patch has a slightly modified 'if' statement to check a constant > > before calling a function, and use elseif: > > > > < + if (!interpretOidsOption(stmt->options, true) && cxt.hasoids) > > --- > > > + if (cxt.hasoids && !interpretOidsOption(stmt->options, true)) > > 47c57 > > < + if (interpretOidsOption(stmt->options, true) && !cxt.hasoids) > > --- > > > + else if (!cxt.hasoids && interpretOidsOption(stmt->options, true)) > > > > I realize the change is subtle. > > What I meant was - I didn't see an attachment on that message. I didn't attach it as people have told me they can just as easily see the patch via git, and since it was so similar, I didn't repost it. Should I have? I can easily do that. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Wed, Apr 22, 2015 at 8:57 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Tue, Apr 21, 2015 at 05:36:41PM -0400, Robert Haas wrote: >> On Mon, Apr 20, 2015 at 5:41 PM, Bruce Momjian <bruce@momjian.us> wrote: >> > On Mon, Apr 20, 2015 at 05:04:14PM -0400, Robert Haas wrote: >> >> On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian <bruce@momjian.us> wrote: >> >> > Slightly improved patch applied. >> >> >> >> Is it? >> > >> > The patch has a slightly modified 'if' statement to check a constant >> > before calling a function, and use elseif: >> > >> > < + if (!interpretOidsOption(stmt->options, true) && cxt.hasoids) >> > --- >> > > + if (cxt.hasoids && !interpretOidsOption(stmt->options, true)) >> > 47c57 >> > < + if (interpretOidsOption(stmt->options, true) && !cxt.hasoids) >> > --- >> > > + else if (!cxt.hasoids && interpretOidsOption(stmt->options, true)) >> > >> > I realize the change is subtle. >> >> What I meant was - I didn't see an attachment on that message. > > I didn't attach it as people have told me they can just as easily see > the patch via git, and since it was so similar, I didn't repost it. > Should I have? I can easily do that. No, I just misread your email. I thought you said you had attached the patch; rereading it, I see that you said you had applied the patch. Silly me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 23, 2015 at 09:26:50AM -0400, Robert Haas wrote: > >> What I meant was - I didn't see an attachment on that message. > > > > I didn't attach it as people have told me they can just as easily see > > the patch via git, and since it was so similar, I didn't repost it. > > Should I have? I can easily do that. > > No, I just misread your email. I thought you said you had attached > the patch; rereading it, I see that you said you had applied the > patch. Silly me. You were not the only one confused --- I got a private email on the same topic. My only guess is that I normally say "attached" in that case so "applied" just looked too similar. I will try to mix it up in the future. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Robert Haas <robertmhaas@gmail.com> writes:
> No, I just misread your email. I thought you said you had attached
> the patch; rereading it, I see that you said you had applied the
> patch. Silly me.
The real problem with this patch is it's wrong. Specifically, it broke
the other case I mentioned in my original email:
regression=# create table src2 (f1 int, primary key(oid)) with oids;
ERROR: column "oid" named in key does not exist
LINE 1: create table src2 (f1 int, primary key(oid)) with oids; ^
That works in 9.4, and was still working in HEAD as of my original email.
I think the patch's logic for attaching made-up OIDS options is actually
backwards (it's adding TRUE where it should add FALSE and vice versa),
but in any case I do not like the dependence on default_with_oids that
was introduced by the patch. I am not sure there's any guarantee that
default_with_oids can't change between parsing and execution of a CREATE
TABLE command.
Apparently we need a few more regression tests in this area. In the
meantime I suggest reverting and rethinking the patch.
regards, tom lane
On Sat, Apr 25, 2015 at 06:15:25PM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > No, I just misread your email. I thought you said you had attached > > the patch; rereading it, I see that you said you had applied the > > patch. Silly me. > > The real problem with this patch is it's wrong. Specifically, it broke > the other case I mentioned in my original email: > > regression=# create table src2 (f1 int, primary key(oid)) with oids; > ERROR: column "oid" named in key does not exist > LINE 1: create table src2 (f1 int, primary key(oid)) with oids; > ^ Wow, thanks for seeing that mistake. I had things just fine, but then I decided to optimize it and forgot that this code is used in non-LIKE situations. Reverted. > That works in 9.4, and was still working in HEAD as of my original email. > I think the patch's logic for attaching made-up OIDS options is actually > backwards (it's adding TRUE where it should add FALSE and vice versa), > but in any case I do not like the dependence on default_with_oids that > was introduced by the patch. I am not sure there's any guarantee that > default_with_oids can't change between parsing and execution of a CREATE > TABLE command. I have changed the default value back to the function call as it should have been all along; patch attached. I will revisit this for 9.6 unless I hear otherwise. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> I have changed the default value back to the function call as it should
> have been all along; patch attached. I will revisit this for 9.6
> unless I hear otherwise.
I still don't like this patch one bit. I don't think that this code
should be modifying stmt->options that way. Also, you have not addressed
whether this is even the right semantics. In particular, currently
default_with_oids will force an OID column to exist regardless of whether
the LIKE-referenced table has them:
regression=# create table base (f1 int);
CREATE TABLE
regression=# set default_with_oids = true;
SET
regression=# create table likeit (like base);
CREATE TABLE
regression=# \d+ base Table "public.base"Column | Type | Modifiers | Storage | Stats target |
Description
--------+---------+-----------+---------+--------------+-------------f1 | integer | | plain |
|
regression=# \d+ likeit Table "public.likeit"Column | Type | Modifiers | Storage | Stats
target| Description
--------+---------+-----------+---------+--------------+-------------f1 | integer | | plain |
|
Has OIDs: yes
Another variant is "create table likeit (like base) with oids".
It's perhaps debatable whether it should act that way, but in the absence
of complaints from the field, I'm hesitant to change these cases. It
might be better if the effective behavior were "table gets OIDs if
default_with_oids = true or WITH OIDS is given or base table has OIDs".
Still another case that needs to be thought about is "create table likeit
(like base) without oids" where base does have OIDs. Probably the right
thing here is to let the WITHOUT OIDS spec override what we see in base.
regards, tom lane
On Saturday, April 25, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1
It's perhaps debatable whether it should act that way, but in the absence
of complaints from the field, I'm hesitant to change these cases. It
might be better if the effective behavior were "table gets OIDs if
default_with_oids = true or WITH OIDS is given or base table has OIDs".
Still another case that needs to be thought about is "create table likeit
(like base) without oids" where base does have OIDs. Probably the right
thing here is to let the WITHOUT OIDS spec override what we see in base.
Why are oids special in this manner? No other inherited column can be omitted from the child table. Though I guess unlike inherits there is no reason to mandate the final result be identical to the base table - though here is something to be said for pointing out the inconsistency and requiring the user to alter table if indeed they want to have the oid-ness changed.
David J.
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Saturday, April 25, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Still another case that needs to be thought about is "create table likeit
>> (like base) without oids" where base does have OIDs. Probably the right
>> thing here is to let the WITHOUT OIDS spec override what we see in base.
> Why are oids special in this manner? No other inherited column can be
> omitted from the child table.
Hm, good point; INHERITS will silently override such a specification:
regression=# create table base1 (f1 int) with oids;
CREATE TABLE
regression=# create table c2 () inherits (base1) without oids;
CREATE TABLE
regression=# \d+ c2 Table "public.c2"Column | Type | Modifiers | Storage | Stats target |
Description
--------+---------+-----------+---------+--------------+-------------f1 | integer | | plain |
|
Inherits: base1
Has OIDs: yes
> Though I guess unlike inherits there is no
> reason to mandate the final result be identical to the base table - though
> here is something to be said for pointing out the inconsistency and
> requiring the user to alter table if indeed they want to have the oid-ness
> changed.
Yeah, LIKE doesn't necessarily have to behave the same as INHERITS;
but probably we should follow that precedent unless we have a specific
argument not to. Which I don't.
regards, tom lane
On Sat, Apr 25, 2015 at 11:11:52PM -0400, Tom Lane wrote: > Hm, good point; INHERITS will silently override such a specification: > > regression=# create table base1 (f1 int) with oids; > CREATE TABLE > regression=# create table c2 () inherits (base1) without oids; > CREATE TABLE > regression=# \d+ c2 > Table "public.c2" > Column | Type | Modifiers | Storage | Stats target | Description > --------+---------+-----------+---------+--------------+------------- > f1 | integer | | plain | | > Inherits: base1 > Has OIDs: yes > > > Though I guess unlike inherits there is no > > reason to mandate the final result be identical to the base table - though > > here is something to be said for pointing out the inconsistency and > > requiring the user to alter table if indeed they want to have the oid-ness > > changed. > > Yeah, LIKE doesn't necessarily have to behave the same as INHERITS; > but probably we should follow that precedent unless we have a specific > argument not to. Which I don't. Agreed. Here is an attached patch for 9.6 which works for multiple LIKE'ed tables with multiple inheritance and index creation. I figured out why Tom's OID primary key test was failing so I now process the columns and LIKE first, then the constraints. There is also no longer a dependency on default_with_oids. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
Bruce Momjian wrote: > Agreed. Here is an attached patch for 9.6 which works for multiple > LIKE'ed tables with multiple inheritance and index creation. I figured > out why Tom's OID primary key test was failing so I now process the > columns and LIKE first, then the constraints. There is also no longer a > dependency on default_with_oids. It seems to me that waiting for 9.6 for what's arguably a bug fix is too much. It's not like this is a new feature. Why don't we just make sure it is as correct as possible and get it done for 9.5? It's not even in beta yet, nor feature freeze. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Apr 28, 2015 at 12:24:50AM -0300, Alvaro Herrera wrote: > Bruce Momjian wrote: > > > Agreed. Here is an attached patch for 9.6 which works for multiple > > LIKE'ed tables with multiple inheritance and index creation. I figured > > out why Tom's OID primary key test was failing so I now process the > > columns and LIKE first, then the constraints. There is also no longer a > > dependency on default_with_oids. > > It seems to me that waiting for 9.6 for what's arguably a bug fix is too > much. It's not like this is a new feature. Why don't we just make sure > it is as correct as possible and get it done for 9.5? It's not even in > beta yet, nor feature freeze. Well, I applied what I thought would work, but did not handle three cases: * checking of hasoids by index specifications * queries with multiple LIKE'ed tables * matching inheritance behavior I am unclear if I should be addressing such complex issues at this point in the development cycle. I can certainly apply this patch, but I need someone else to tell me it is good and should be applied. I am also thinking such review time would be better spent on patches submitted long before mine. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Tue, Apr 28, 2015 at 09:15:49AM -0400, Bruce Momjian wrote: > > It seems to me that waiting for 9.6 for what's arguably a bug fix is too > > much. It's not like this is a new feature. Why don't we just make sure > > it is as correct as possible and get it done for 9.5? It's not even in > > beta yet, nor feature freeze. > > Well, I applied what I thought would work, but did not handle three > cases: > > * checking of hasoids by index specifications > * queries with multiple LIKE'ed tables > * matching inheritance behavior > > I am unclear if I should be addressing such complex issues at this point > in the development cycle. I can certainly apply this patch, but I need > someone else to tell me it is good and should be applied. I am also > thinking such review time would be better spent on patches submitted > long before mine. I have added regression tests to the patch, attached. I have included Tom's test that doesn't directly use LIKE. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On Wed, Apr 29, 2015 at 03:48:04PM -0400, Bruce Momjian wrote: > On Tue, Apr 28, 2015 at 09:15:49AM -0400, Bruce Momjian wrote: > > > It seems to me that waiting for 9.6 for what's arguably a bug fix is too > > > much. It's not like this is a new feature. Why don't we just make sure > > > it is as correct as possible and get it done for 9.5? It's not even in > > > beta yet, nor feature freeze. > > > > Well, I applied what I thought would work, but did not handle three > > cases: > > > > * checking of hasoids by index specifications > > * queries with multiple LIKE'ed tables > > * matching inheritance behavior > > > > I am unclear if I should be addressing such complex issues at this point > > in the development cycle. I can certainly apply this patch, but I need > > someone else to tell me it is good and should be applied. I am also > > thinking such review time would be better spent on patches submitted > > long before mine. > > I have added regression tests to the patch, attached. I have included > Tom's test that doesn't directly use LIKE. Patch applied. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription +