Shouldn't CREATE TABLE LIKE copy the relhasoids property?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Saturday, April 25, 2015, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

+1

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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