Re: CREATE LIKE INCLUDING COMMENTS and STORAGES

Here is a patch to implement the following items in our ToDo list:
* Add CREATE TABLE LIKE ... INCLUDING COMMENTS
* Have CREATE TABLE LIKE copy column storage parameters

The syntax is:
CREATE TABLE clone_table (LIKE template_table INCLUDING COMMENTS)
-- also copy comments on columns.
CREATE TABLE clone_table (LIKE template_table INCLUDING STORAGES)
-- also copy storage parameters on columns.

Also, storage parameters of inherited columns are inherited automatically.

There might be room for improvement:

* Should INCLUDING COMMENTS also copy comments on indexes?
It copies only comments on columns for now.

* Should we have additonal syntax to define storage parameters inline
of CREATE TABLE? For example,
CREATE TABLE tbl (col text STORAGE MAIN);
CREATE TABLE fails if there is a conflicted storage parameter for now.
ERROR: column "col" has a storage parameter conflict
DETAIL: MAIN versus EXTENDED
but there is no way to resolve the confliction unless we modify the
definitions of original tables. Meantime, we can overwrite DEFAULTs
to resolve conflictions by INCLUDING DEFAULTS.

* Should we have "INCLUDING ALL" as an abbreviated form?
Many INCLUDING options in CREATE LIKE seems to be messy:
CREATE TABLE clone_table (LIKE template_table
INCLUDING DEFAULTS
INCLUDING CONSTRAINTS
INCLUDING INDEXES
INCLUDING STORAGES
INCLUDING COMMENTS);

Comments welcome.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Mon, Sep 07, 2009 at 12:15:21PM +0900, Itagaki Takahiro wrote:
> Here is a patch to implement the following items in our ToDo list:
> * Add CREATE TABLE LIKE ... INCLUDING COMMENTS
> * Have CREATE TABLE LIKE copy column storage parameters
>
> The syntax is:
> CREATE TABLE clone_table (LIKE template_table INCLUDING COMMENTS)
> -- also copy comments on columns.
> CREATE TABLE clone_table (LIKE template_table INCLUDING STORAGES)

This should probably read INCLUDING STORAGE (singular) instead of
STORAGES.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david(at)fetter(dot)org> wrote:

> On Mon, Sep 07, 2009 at 12:15:21PM +0900, Itagaki Takahiro wrote:
> > Here is a patch to implement the following items in our ToDo list:
> > * Add CREATE TABLE LIKE ... INCLUDING COMMENTS
> > * Have CREATE TABLE LIKE copy column storage parameters
> >
> > The syntax is:
> > CREATE TABLE clone_table (LIKE template_table INCLUDING STORAGES)
>
> This should probably read INCLUDING STORAGE (singular) instead of
> STORAGES.

Thanks. I fixed it to INCLUDING STORAGE.

In addition, I modified INCLUDING COMMENTS to copy comments not only
on columns but also on constraints. However, comments on indexes are
not copied because copied indexes are named in DefineIndex();
We don't know names of new indexes when we build a command list.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
>
> David Fetter <david(at)fetter(dot)org> wrote:
>
> > On Mon, Sep 07, 2009 at 12:15:21PM +0900, Itagaki Takahiro wrote:
> > > Here is a patch to implement the following items in our ToDo list:
> > > * Add CREATE TABLE LIKE ... INCLUDING COMMENTS
> > > * Have CREATE TABLE LIKE copy column storage parameters
> > >
> > > The syntax is:
> > > CREATE TABLE clone_table (LIKE template_table INCLUDING STORAGES)
> >
> > This should probably read INCLUDING STORAGE (singular) instead of
> > STORAGES.
>
> Thanks. I fixed it to INCLUDING STORAGE.

This INCLUDING STORAGE is supposed to copy reloptions? In that case I
think this is still a misnomer; to me it sounds like it's copying the
underlying storage i.e. data, which would be very surprising. What
about "INCLUDING STORAGE OPTIONS"?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> This INCLUDING STORAGE is supposed to copy reloptions?

No. It copies only storage parameters of columns to control TOAST policy.
It might be good to have some features to copy reloptions with convenient
way, but it will be done in another patch.

> to me it sounds like it's copying the
> underlying storage i.e. data, which would be very surprising. What
> about "INCLUDING STORAGE OPTIONS"?

Hmm, but we have the following syntax already:
ALTER TABLE table ALTER COLUMN column SET STORAGE ...
Do you also think it should be "SET STORAGE OPTION ..." ?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

2009/9/9 Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>> This INCLUDING STORAGE is supposed to copy reloptions?
>
> No. It copies only storage parameters of columns to control TOAST policy.
> It might be good to have some features to copy reloptions with convenient
> way, but it will be done in another patch.
>
>> to me it sounds like it's copying the
>> underlying storage i.e. data, which would be very surprising.  What
>> about "INCLUDING STORAGE OPTIONS"?

It *would* be very surprising. An option to include data would
probably be called "INCLUDING DATA" =)

>
> Hmm, but we have the following syntax already:
>    ALTER TABLE table ALTER COLUMN column SET STORAGE ...
> Do you also think it should be "SET STORAGE OPTION ..." ?
>

Personally, I think INCLUDING STORAGE makes as much sense as you can
expect using just one word, and as Itagaki-san points out it
correlates well with the syntax for ALTER COLUMN.

Cheers,
BJ

2009/9/7 Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
> Here is a patch to implement the following items in our ToDo list:
>  * Add CREATE TABLE LIKE ... INCLUDING COMMENTS
>  * Have CREATE TABLE LIKE copy column storage parameters
>

Hello Itagaki-san,

I am doing an initial review of your patch. I applied the version
labelled 20090908 (applied with minor fuzz to HEAD). It compiled
cleanly and the feature appears to work as advertised.

I did a little bit of copy-editing on my way through (changes
attached) but the patch seems to be in very good shape. The
documentation is clearly worded, although I did add a cross-reference
in the bit about STORAGE. The regression tests seem to give a pretty
good coverage of both the success and failure modes.

In response to the questions you raised in your post:

>  * Should INCLUDING COMMENTS also copy comments on indexes?
>    It copies only comments on columns for now.

It probably should, but if this is difficult to work in, I don't see
anything wrong with leaving it out of this patch and making it a TODO.

>
>  * Should we have additonal syntax to define storage parameters inline
>    of CREATE TABLE? For example,
>        CREATE TABLE tbl (col text STORAGE MAIN);
>    CREATE TABLE fails if there is a conflicted storage parameter for now.
>        ERROR:  column "col" has a storage parameter conflict
>        DETAIL:  MAIN versus EXTENDED
>    but there is no way to resolve the confliction unless we modify the
>    definitions of original tables. Meantime, we can overwrite DEFAULTs
>    to resolve conflictions by INCLUDING DEFAULTS.

I think I'm failing to understand why this would be an issue. Why
would the user be specifying columns in the CREATE TABLE statement
that already exist in the table they are cloning?

>
>  * Should we have "INCLUDING ALL" as an abbreviated form?
>    Many INCLUDING options in CREATE LIKE seems to be messy:
>        CREATE TABLE clone_table (LIKE template_table
>            INCLUDING DEFAULTS
>            INCLUDING CONSTRAINTS
>            INCLUDING INDEXES
>            INCLUDING STORAGES
>            INCLUDING COMMENTS);
>

+1 for adding INCLUDING ALL. The grammar should also support
EXCLUDING ALL for symmetry, even though EXCLUDING ALL is the default
behaviour.

However I do think that this should be a separate patch ... add to TODO?

Cheers,
BJ

Brendan Jurd <direvus(at)gmail(dot)com> wrote:

> I am doing an initial review of your patch.

Thank you for reviewing.
I merged your fix and add INCLUDING ALL option to the new patch.
I changed InhRelation.options to be a bitmap of CreateStmtLikeOption.
INCLUDING just adds bits, and EXCLUDING drops bits.

Now this patch adds:
* CREATE TABLE LIKE ... INCLUDING COMMENTS (for columns and constraints)
* CREATE TABLE LIKE ... INCLUDING STORAGE
* CREATE TABLE LIKE ... INCLUDING ALL

> I think I'm failing to understand why this would be an issue. Why
> would the user be specifying columns in the CREATE TABLE statement
> that already exist in the table they are cloning?

Without inline-STORAGE syntax, we cannot resolve conflictions of
storage parameters unless we can define tables without STORAGE
and then re-add options with ALTER TABLE.

There might be ToDo items:
* Make INCLUDING COMMENTS also copy comments on indexes.
* Add syntax to define storage options inline like
CREATE TABLE tbl (col text STORAGE MAIN).

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

2009/9/28 Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
> Thank you for reviewing.
> I merged your fix and add INCLUDING ALL option to the new patch.
> I changed InhRelation.options to be a bitmap of CreateStmtLikeOption.
> INCLUDING just adds bits, and EXCLUDING drops bits.

I had two hunks fail trying to apply your new patch to the latest (git) HEAD:

patching file doc/src/sgml/ref/create_table.sgml
patching file src/backend/access/common/tupdesc.c
patching file src/backend/catalog/pg_constraint.c
patching file src/backend/commands/comment.c
patching file src/backend/commands/tablecmds.c
patching file src/backend/nodes/copyfuncs.c
patching file src/backend/nodes/equalfuncs.c
patching file src/backend/parser/gram.y
patching file src/backend/parser/parse_utilcmd.c
patching file src/bin/psql/sql_help.c
Hunk #1 FAILED at 3.
Hunk #2 FAILED at 1279.
2 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/sql_help.c.rej
patching file src/include/catalog/pg_constraint.h
patching file src/include/commands/comment.h
patching file src/include/nodes/parsenodes.h
patching file src/include/parser/kwlist.h
patching file src/test/regress/expected/inherit.out
patching file src/test/regress/sql/inherit.sql

I have attached the rejects file.

Cheers,
BJ

Brendan Jurd <direvus(at)gmail(dot)com> wrote:

> patching file src/bin/psql/sql_help.c
> Hunk #1 FAILED at 3.
> Hunk #2 FAILED at 1279.
> 2 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/sql_help.c.rej
>
> I have attached the rejects file.

Oops, sql_help.c is an automatic generated file. Please ignore the part.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

2009/9/28 Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
> Brendan Jurd <direvus(at)gmail(dot)com> wrote:
>> patching file src/bin/psql/sql_help.c
>> Hunk #1 FAILED at 3.
>> Hunk #2 FAILED at 1279.
>> 2 out of 2 hunks FAILED -- saving rejects to file src/bin/psql/sql_help.c.rej
>
> Oops, sql_help.c is an automatic generated file. Please ignore the part.
>
>

With the sql_help.c changes removed, the patch applied fine and
testing went well.

I noticed only the following in the new documentation in CREATE TABLE:

diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index 6417007..9ea8a49 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -299,7 +299,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ]
TABLE <replaceable class="PAR
</para>
<para>
<literal>INCLUDING ALL</literal> is an abbreviated form of
- <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING
INDEXES INCLUDING STORAGES INCLUDING COMMENTS</literal>.
+ <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING
INDEXES INCLUDING STORAGE INCLUDING COMMENTS</literal>.
</para>
<para>
Note also that unlike <literal>INHERITS</literal>, copied columns and

Aside from the bogus hunks in the patch, and this one typo, the patch
looks to be in excellent shape.

Cheers,
BJ

I removed hunks by sql_help.c and fix a typo in documentation.
An updated patch attached.

Brendan Jurd <direvus(at)gmail(dot)com> wrote:

> With the sql_help.c changes removed, the patch applied fine and
> testing went well.
>
> I noticed only the following in the new documentation in CREATE TABLE:
> - <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING INDEXES INCLUDING STORAGES INCLUDING COMMENTS</literal>.
> + <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING INDEXES INCLUDING STORAGE INCLUDING COMMENTS</literal>.
>
> Aside from the bogus hunks in the patch, and this one typo, the patch
> looks to be in excellent shape.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro escribió:
> I removed hunks by sql_help.c and fix a typo in documentation.
> An updated patch attached.

Hmm, so it works to specify LIKE t1 INCLUDING COMMENTS EXCLUDING COMMENTS?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:

> Hmm, so it works to specify LIKE t1 INCLUDING COMMENTS EXCLUDING COMMENTS?

Only last specifer is applied, which is the same behavior as of now.

EXCLUDING is typically useless because all of the default values are
EXCLUDING, but "INCLUDING ALL EXCLUDING xxx" are meaningful; it copies
all components except xxx.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
> I removed hunks by sql_help.c and fix a typo in documentation.
> An updated patch attached.
>
> Brendan Jurd <direvus(at)gmail(dot)com> wrote:
>
>
>> With the sql_help.c changes removed, the patch applied fine and
>> testing went well.
>>
>> I noticed only the following in the new documentation in CREATE TABLE:
>> - <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING INDEXES INCLUDING STORAGES INCLUDING COMMENTS</literal>.
>> + <literal>INCLUDING DEFAULTS INCLUDING CONSTRAINTS INCLUDING INDEXES INCLUDING STORAGE INCLUDING COMMENTS</literal>.
>>
>> Aside from the bogus hunks in the patch, and this one typo, the patch
>> looks to be in excellent shape.
>>

I'm reviewing this patch with a view to committing it, since the other
patch I was going to look at still seemed to be subject to some
discussion. In general it looks OK, but I'm wondering why we are not
copying comments on cloned indexes. I realize that might involve a bit
more code, but I think I'd feel happier if we cloned all the comments we
reasonably could from the outset. Is it really that hard to do?

cheers

andrew

Recently, I encountered a situation where the docs on (or impl?)
INCLUDING INDEXES and INCLUDING CONSTRAINTS are not clearly defined
for primary keys. Should it be noted in the docs that in this case, we
are referring to the technical implementation of a primary key, i.e. a
unique index and a not null constraint, thus both conditions are
required?

testdb=> CREATE TABLE foo (id int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo_pkey" for table "foo"
CREATE TABLE
testdb=> \d foo;
Table "public.foo"
Column | Type | Modifiers
--------+---------+-----------
id | integer | not null
Indexes:
"foo_pkey" PRIMARY KEY, btree (id)

testdb=> CREATE TABLE foo2 (LIKE FOO INCLUDING CONSTRAINTS EXCLUDING INDEXES);
CREATE TABLE
testdb=> \d foo2
Table "public.foo2"
Column | Type | Modifiers
--------+---------+-----------
id | integer | not null

testdb=> CREATE TABLE foo3 (LIKE FOO EXCLUDING CONSTRAINTS INCLUDING INDEXES);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"foo3_pkey" for table "foo3"
CREATE TABLE
testdb=> \d foo3;
Table "public.foo3"
Column | Type | Modifiers
--------+---------+-----------
id | integer | not null
Indexes:
"foo3_pkey" PRIMARY KEY, btree (id)

testdb=>

Regards,
Khee Chin.

Khee Chin <kheechin(at)gmail(dot)com> wrote:

> Recently, I encountered a situation where the docs on (or impl?)
> INCLUDING INDEXES and INCLUDING CONSTRAINTS are not clearly defined
> for primary keys. Should it be noted in the docs that in this case, we
> are referring to the technical implementation of a primary key, i.e. a
> unique index and a not null constraint, thus both conditions are
> required?

It might be a confusable feature, but it should be discussed separated
from this patch. IMO, almost all user will use "INCLUDING ALL"
if the syntax is added by the patch.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:

> I'm wondering why we are not
> copying comments on cloned indexes. I realize that might involve a bit
> more code, but I think I'd feel happier if we cloned all the comments we
> reasonably could from the outset. Is it really that hard to do?

I found it is not so difficult as I expected; patch attached. Now it copies
comments on indexes and columns of the indexes on INCLUDING COMMENTS.
Regression test and documentation are also adjusted. Please review around
chooseIndexName() and uses of it.

The codes becomes a bit complex and might be ugly because we will have some
duplicated codes; "pg_expression_%d" hacks and uses of ChooseRelationName()
are spread into index.c, indexcmds.c and parse_utilcmd.c.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
>> I'm wondering why we are not
>> copying comments on cloned indexes. I realize that might involve a bit
>> more code, but I think I'd feel happier if we cloned all the comments we
>> reasonably could from the outset. Is it really that hard to do?
>>
>
> I found it is not so difficult as I expected; patch attached. Now it copies
> comments on indexes and columns of the indexes on INCLUDING COMMENTS.
> Regression test and documentation are also adjusted. Please review around
> chooseIndexName() and uses of it.
>
> The codes becomes a bit complex and might be ugly because we will have some
> duplicated codes; "pg_expression_%d" hacks and uses of ChooseRelationName()
> are spread into index.c, indexcmds.c and parse_utilcmd.c.
>
>
>

I don't think that's a terrible tragedy - you haven't copied huge swags
of code. Committed with slight adjustments for bitrot etc.

cheers

andrew