Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
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 || 

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Alvaro Herrera]

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 Herrerahttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

Robert Haas robertmh...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

On Sat, Apr 25, 2015 at 06:15:25PM -0400, Tom Lane wrote:
 Robert Haas robertmh...@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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
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.

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[David G. Johnston]

On Saturday, April 25, 2015, Tom Lane t...@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.

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

Bruce Momjian br...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

David G. Johnston david.g.johns...@gmail.com writes:
 On Saturday, April 25, 2015, Tom Lane t...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Robert Haas]

On Wed, Apr 22, 2015 at 8:57 PM, Bruce Momjian br...@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 br...@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 br...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

On Tue, Apr 21, 2015 at 05:36:41PM -0400, Robert Haas wrote:
 On Mon, Apr 20, 2015 at 5:41 PM, Bruce Momjian br...@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 br...@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  br...@momjian.ushttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Robert Haas]

On Mon, Apr 20, 2015 at 5:41 PM, Bruce Momjian br...@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 br...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Robert Haas]

On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian br...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

On Mon, Apr 20, 2015 at 05:04:14PM -0400, Robert Haas wrote:
 On Mon, Apr 20, 2015 at 4:11 PM, Bruce Momjian br...@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  br...@momjian.ushttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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  br...@momjian.ushttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Robert Haas]

On Thu, Apr 9, 2015 at 5:33 PM, Bruce Momjian br...@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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
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.

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Alvaro Herrera]

Bruce Momjian wrote:

 Should this be listed in the release notes as a backward-incompatibility?

Isn't this a backpatchable bug fix?

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Bruce Momjian]

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  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
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.

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Andrew Dunstan]

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

[HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

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

Re: [HACKERS] Shouldn't CREATE TABLE LIKE copy the relhasoids property?

[Tom Lane]

Andrew Dunstan and...@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