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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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: yesregression=# \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: yesregression=# \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. +
Attachments:
like.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 1fc8c2c..405c347
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
*************** transformCreateStmt(CreateStmt *stmt, co
*** 222,228 ****
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
! cxt.hasoids = interpretOidsOption(stmt->options, true);
Assert(!stmt->ofTypename || !stmt->inhRelations); /* grammar enforces */
--- 222,228 ----
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
! cxt.hasoids = false;
Assert(!stmt->ofTypename || !stmt->inhRelations); /* grammar enforces */
*************** transformCreateStmt(CreateStmt *stmt, co
*** 281,286 ****
--- 281,290 ----
* Output results.
*/
stmt->tableElts = cxt.columns;
+ /* add WITH OIDS, if necessary */
+ if (!interpretOidsOption(stmt->options, true) && cxt.hasoids)
+ stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(TRUE)));
stmt->constraints = cxt.ckconstraints;
result = lappend(cxt.blist, stmt);
*************** transformTableLikeClause(CreateStmtConte
*** 849,854 ****
--- 853,860 ----
}
}
+ cxt->hasoids = relation->rd_rel->relhasoids;
+
/*
* Copy CHECK constraints if requested, being careful to adjust attribute
* numbers so they match the child.
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. +
Attachments:
like.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 1fc8c2c..6a72f15
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***************
*** 56,61 ****
--- 56,62 ----
#include "rewrite/rewriteManip.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
*************** transformCreateStmt(CreateStmt *stmt, co
*** 222,228 ****
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
! cxt.hasoids = interpretOidsOption(stmt->options, true);
Assert(!stmt->ofTypename || !stmt->inhRelations); /* grammar enforces */
--- 223,229 ----
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
! cxt.hasoids = default_with_oids;
Assert(!stmt->ofTypename || !stmt->inhRelations); /* grammar enforces */
*************** transformCreateStmt(CreateStmt *stmt, co
*** 281,286 ****
--- 282,298 ----
* Output results.
*/
stmt->tableElts = cxt.columns;
+ /*
+ * Add WITH/WITHOUT OIDS, if necessary. A literal statement-specified
+ * WITH/WITHOUT OIDS will still take precedence because the first
+ * matching "oids" in "options" is used.
+ */
+ if (!interpretOidsOption(stmt->options, true) && cxt.hasoids)
+ stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(TRUE)));
+ if (interpretOidsOption(stmt->options, true) && !cxt.hasoids)
+ stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(FALSE)));
stmt->constraints = cxt.ckconstraints;
result = lappend(cxt.blist, stmt);
*************** transformTableLikeClause(CreateStmtConte
*** 849,854 ****
--- 861,868 ----
}
}
+ cxt->hasoids = relation->rd_rel->relhasoids;
+
/*
* Copy CHECK constraints if requested, being careful to adjust attribute
* numbers so they match the child.
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. +
Attachments:
like.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 1fc8c2c..40fa9d6
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***************
*** 56,61 ****
--- 56,62 ----
#include "rewrite/rewriteManip.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
*************** transformCreateStmt(CreateStmt *stmt, co
*** 281,286 ****
--- 282,298 ----
* Output results.
*/
stmt->tableElts = cxt.columns;
+ /*
+ * Add WITH/WITHOUT OIDS, if necessary. A literal statement-specified
+ * WITH/WITHOUT OIDS will still take precedence because the first
+ * matching "oids" in "options" is used.
+ */
+ if (!interpretOidsOption(stmt->options, true) && cxt.hasoids)
+ stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(TRUE)));
+ else if (interpretOidsOption(stmt->options, true) && !cxt.hasoids)
+ stmt->options = lappend(stmt->options, makeDefElem("oids",
+ (Node *)makeInteger(FALSE)));
stmt->constraints = cxt.ckconstraints;
result = lappend(cxt.blist, stmt);
*************** transformTableLikeClause(CreateStmtConte
*** 849,854 ****
--- 861,868 ----
}
}
+ cxt->hasoids = relation->rd_rel->relhasoids;
+
/*
* Copy CHECK constraints if requested, being careful to adjust attribute
* numbers so they match the child.
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: yesThough 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. +
Attachments:
like.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 73924ae..d442bb0
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***************
*** 56,61 ****
--- 56,62 ----
#include "rewrite/rewriteManip.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
*************** transformCreateStmt(CreateStmt *stmt, co
*** 150,155 ****
--- 151,157 ----
Oid namespaceid;
Oid existing_relid;
ParseCallbackState pcbstate;
+ bool like_found = false;
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
*************** transformCreateStmt(CreateStmt *stmt, co
*** 242,248 ****
/*
* Run through each primary element in the table creation clause. Separate
! * column defs from constraints, and do preliminary analysis.
*/
foreach(elements, stmt->tableElts)
{
--- 244,253 ----
/*
* Run through each primary element in the table creation clause. Separate
! * column defs from constraints, and do preliminary analysis. We have to
! * process column-defining clauses first because it can control the
! * presence of columns which are referenced by columns referenced by
! * constraints.
*/
foreach(elements, stmt->tableElts)
{
*************** transformCreateStmt(CreateStmt *stmt, co
*** 254,267 ****
transformColumnDefinition(&cxt, (ColumnDef *) element);
break;
- case T_Constraint:
- transformTableConstraint(&cxt, (Constraint *) element);
- break;
-
case T_TableLikeClause:
transformTableLikeClause(&cxt, (TableLikeClause *) element);
break;
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(element));
--- 259,277 ----
transformColumnDefinition(&cxt, (ColumnDef *) element);
break;
case T_TableLikeClause:
+ if (!like_found)
+ {
+ cxt.hasoids = false;
+ like_found = true;
+ }
transformTableLikeClause(&cxt, (TableLikeClause *) element);
break;
+ case T_Constraint:
+ /* process later */
+ break;
+
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(element));
*************** transformCreateStmt(CreateStmt *stmt, co
*** 269,274 ****
--- 279,305 ----
}
}
+ if (like_found)
+ {
+ /*
+ * To match INHERITS, the existance of any LIKE table with OIDs
+ * causes the new table to have oids. For the same reason,
+ * WITH/WITHOUT OIDs is also ignored with LIKE. We prepend
+ * because the first oid option list entry is honored. Our
+ * prepended WITHOUT OIDS clause will be overridden if an
+ * inherited table has oids.
+ */
+ stmt->options = lcons(makeDefElem("oids",
+ (Node *)makeInteger(cxt.hasoids)), stmt->options);
+ }
+
+ foreach(elements, stmt->tableElts)
+ {
+ Node *element = lfirst(elements);
+
+ if (nodeTag(element) == T_Constraint)
+ transformTableConstraint(&cxt, (Constraint *) element);
+ }
/*
* transformIndexConstraints wants cxt.alist to contain only index
* statements, so transfer anything we already have into save_alist.
*************** transformTableLikeClause(CreateStmtConte
*** 860,865 ****
--- 891,899 ----
}
}
+ /* We use oids if at least one LIKE'ed table has oids. */
+ cxt->hasoids = cxt->hasoids || relation->rd_rel->relhasoids;
+
/*
* Copy CHECK constraints if requested, being careful to adjust attribute
* numbers so they match the child.
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
--
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 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. +
--
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 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 behaviorI 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. +
Attachments:
like.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
new file mode 100644
index 0a55db4..a76f957
*** a/src/backend/parser/parse_utilcmd.c
--- b/src/backend/parser/parse_utilcmd.c
***************
*** 56,61 ****
--- 56,62 ----
#include "rewrite/rewriteManip.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/guc.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"
*************** transformCreateStmt(CreateStmt *stmt, co
*** 150,155 ****
--- 151,157 ----
Oid namespaceid;
Oid existing_relid;
ParseCallbackState pcbstate;
+ bool like_found = false;
/*
* We must not scribble on the passed-in CreateStmt, so copy it. (This is
*************** transformCreateStmt(CreateStmt *stmt, co
*** 242,248 ****
/*
* Run through each primary element in the table creation clause. Separate
! * column defs from constraints, and do preliminary analysis.
*/
foreach(elements, stmt->tableElts)
{
--- 244,253 ----
/*
* Run through each primary element in the table creation clause. Separate
! * column defs from constraints, and do preliminary analysis. We have to
! * process column-defining clauses first because it can control the
! * presence of columns which are referenced by columns referenced by
! * constraints.
*/
foreach(elements, stmt->tableElts)
{
*************** transformCreateStmt(CreateStmt *stmt, co
*** 254,267 ****
transformColumnDefinition(&cxt, (ColumnDef *) element);
break;
- case T_Constraint:
- transformTableConstraint(&cxt, (Constraint *) element);
- break;
-
case T_TableLikeClause:
transformTableLikeClause(&cxt, (TableLikeClause *) element);
break;
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(element));
--- 259,277 ----
transformColumnDefinition(&cxt, (ColumnDef *) element);
break;
case T_TableLikeClause:
+ if (!like_found)
+ {
+ cxt.hasoids = false;
+ like_found = true;
+ }
transformTableLikeClause(&cxt, (TableLikeClause *) element);
break;
+ case T_Constraint:
+ /* process later */
+ break;
+
default:
elog(ERROR, "unrecognized node type: %d",
(int) nodeTag(element));
*************** transformCreateStmt(CreateStmt *stmt, co
*** 269,274 ****
--- 279,305 ----
}
}
+ if (like_found)
+ {
+ /*
+ * To match INHERITS, the existance of any LIKE table with OIDs
+ * causes the new table to have oids. For the same reason,
+ * WITH/WITHOUT OIDs is also ignored with LIKE. We prepend
+ * because the first oid option list entry is honored. Our
+ * prepended WITHOUT OIDS clause will be overridden if an
+ * inherited table has oids.
+ */
+ stmt->options = lcons(makeDefElem("oids",
+ (Node *)makeInteger(cxt.hasoids)), stmt->options);
+ }
+
+ foreach(elements, stmt->tableElts)
+ {
+ Node *element = lfirst(elements);
+
+ if (nodeTag(element) == T_Constraint)
+ transformTableConstraint(&cxt, (Constraint *) element);
+ }
/*
* transformIndexConstraints wants cxt.alist to contain only index
* statements, so transfer anything we already have into save_alist.
*************** transformTableLikeClause(CreateStmtConte
*** 860,865 ****
--- 891,899 ----
}
}
+ /* We use oids if at least one LIKE'ed table has oids. */
+ cxt->hasoids = cxt->hasoids || relation->rd_rel->relhasoids;
+
/*
* Copy CHECK constraints if requested, being careful to adjust attribute
* numbers so they match the child.
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
new file mode 100644
index 8ba7bbc..41ceb87
*** a/src/test/regress/expected/create_table.out
--- b/src/test/regress/expected/create_table.out
*************** ERROR: relation "as_select1" already ex
*** 250,252 ****
--- 250,255 ----
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
NOTICE: relation "as_select1" already exists, skipping
DROP TABLE as_select1;
+ -- check that the oid column is added before the primary key is checked
+ CREATE TABLE oid_pk (f1 INT, PRIMARY KEY(oid)) WITH OIDS;
+ DROP TABLE oid_pk;
diff --git a/src/test/regress/expected/create_table_like.out b/src/test/regress/expected/create_table_like.out
new file mode 100644
index a5fac7b..97edde1
*** a/src/test/regress/expected/create_table_like.out
--- b/src/test/regress/expected/create_table_like.out
*************** DROP TYPE ctlty1;
*** 228,230 ****
--- 228,257 ----
DROP VIEW ctlv1;
DROP TABLE IF EXISTS ctlt4, ctlt10, ctlt11, ctlt11a, ctlt12;
NOTICE: table "ctlt10" does not exist, skipping
+ /* LIKE WITH OIDS */
+ CREATE TABLE has_oid (x INTEGER) WITH OIDS;
+ CREATE TABLE no_oid (y INTEGER);
+ CREATE TABLE like_test (z INTEGER, LIKE has_oid);
+ SELECT oid FROM like_test;
+ oid
+ -----
+ (0 rows)
+
+ CREATE TABLE like_test2 (z INTEGER, LIKE no_oid);
+ SELECT oid FROM like_test2; -- fail
+ ERROR: column "oid" does not exist
+ LINE 1: SELECT oid FROM like_test2;
+ ^
+ CREATE TABLE like_test3 (z INTEGER, LIKE has_oid, LIKE no_oid);
+ SELECT oid FROM like_test3;
+ oid
+ -----
+ (0 rows)
+
+ CREATE TABLE like_test4 (z INTEGER, PRIMARY KEY(oid), LIKE has_oid);
+ SELECT oid FROM like_test4;
+ oid
+ -----
+ (0 rows)
+
+ DROP TABLE has_oid, no_oid, like_test, like_test2, like_test3, like_test4;
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
new file mode 100644
index 03bb5ff..78bdc8b
*** a/src/test/regress/sql/create_table.sql
--- b/src/test/regress/sql/create_table.sql
*************** CREATE TABLE as_select1 AS SELECT * FROM
*** 265,267 ****
--- 265,271 ----
CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class WHERE relkind = 'r';
DROP TABLE as_select1;
+
+ -- check that the oid column is added before the primary key is checked
+ CREATE TABLE oid_pk (f1 INT, PRIMARY KEY(oid)) WITH OIDS;
+ DROP TABLE oid_pk;
diff --git a/src/test/regress/sql/create_table_like.sql b/src/test/regress/sql/create_table_like.sql
new file mode 100644
index 2d017bc..6dded1f
*** a/src/test/regress/sql/create_table_like.sql
--- b/src/test/regress/sql/create_table_like.sql
*************** DROP SEQUENCE ctlseq1;
*** 119,121 ****
--- 119,134 ----
DROP TYPE ctlty1;
DROP VIEW ctlv1;
DROP TABLE IF EXISTS ctlt4, ctlt10, ctlt11, ctlt11a, ctlt12;
+
+ /* LIKE WITH OIDS */
+ CREATE TABLE has_oid (x INTEGER) WITH OIDS;
+ CREATE TABLE no_oid (y INTEGER);
+ CREATE TABLE like_test (z INTEGER, LIKE has_oid);
+ SELECT oid FROM like_test;
+ CREATE TABLE like_test2 (z INTEGER, LIKE no_oid);
+ SELECT oid FROM like_test2; -- fail
+ CREATE TABLE like_test3 (z INTEGER, LIKE has_oid, LIKE no_oid);
+ SELECT oid FROM like_test3;
+ CREATE TABLE like_test4 (z INTEGER, PRIMARY KEY(oid), LIKE has_oid);
+ SELECT oid FROM like_test4;
+ DROP TABLE has_oid, no_oid, like_test, like_test2, like_test3, like_test4;
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 behaviorI 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 +--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers