Thread: 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
Attachment
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@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David Fetter <david@fetter.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
Attachment
Itagaki Takahiro wrote: > > David Fetter <david@fetter.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@commandprompt.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.takahiro@oss.ntt.co.jp>: > Alvaro Herrera <alvherre@commandprompt.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.takahiro@oss.ntt.co.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
Attachment
Brendan Jurd <direvus@gmail.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
Attachment
2009/9/28 Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.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
Attachment
Brendan Jurd <direvus@gmail.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.takahiro@oss.ntt.co.jp>:
> Brendan Jurd <direvus@gmail.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@gmail.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
Attachment
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@commandprompt.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@gmail.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@gmail.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@dunslane.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
Attachment
Itagaki Takahiro wrote: > Andrew Dunstan <andrew@dunslane.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