Thread: CREATE TABLE LIKE INCLUDING INDEXES support
Greetings all,
I wrote this patch about a week ago to introduce myself to coding on
PostgreSQL. I wasn't entirely sure what the 'INCLUDING INDEXES' option
was meant to do, so I held off submitting it until I could get around to
asking about that and tweaking the documentation to reflect the patch.
By useful coincidence the thread "Auto creation of Partitions" had this
post in it, which made the intent of the option clear enough for me to
go ahead and see what people think of this.
Gregory Stark wrote:
> "NikhilS" <nikkhils@gmail.com> writes:
>
>
>> the intention is to use this information from the parent and make it a
>> property of the child table. This will avoid the step for the user having to
>> manually specify CREATE INDEX and the likes on all the children tables
>> one-by-one.
>>
>
> Missed the start of this thread. A while back I had intended to add WITH
> INDEXES to CREATE TABLE LIKE. That would let you create a tale LIKE parent
> WITH CONSTRAINTS WITH INDEXES and get pretty much a perfect table ready for
> adding to the inheritance structure.
>
>
>
So, that's what this patch does. When a table is created with 'CREATE
TABLE ... LIKE parent INCLUDING INDEXES' this iterates over the parent
table indexes looking for constraint indexes, and alters the
CreateStmtContext to include equivalent indexes on the child table.
This is probably a somewhat naive implementation, being a first attempt.
I wasn't sure what sort of lock to place on the parent indexes or what
tablespace the new indexes should be created in. Any help improving it
would be appreciated.
Thank you,
-Trevor Hardcastle
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.361
diff -c -r1.361 analyze.c
*** src/backend/parser/analyze.c 20 Feb 2007 17:32:16 -0000 1.361
--- src/backend/parser/analyze.c 7 Mar 2007 01:43:12 -0000
***************
*** 14,19 ****
--- 14,20 ----
#include "postgres.h"
#include "access/heapam.h"
+ #include "access/genam.h"
#include "catalog/heap.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
***************
*** 40,45 ****
--- 41,47 ----
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+ #include "utils/relcache.h"
#include "utils/syscache.h"
***************
*** 1345,1355 ****
}
}
- if (including_indexes)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("LIKE INCLUDING INDEXES is not implemented")));
-
/*
* Insert the copied attributes into the cxt for the new table
* definition.
--- 1347,1352 ----
***************
*** 1448,1453 ****
--- 1445,1519 ----
}
/*
+ * Clone constraint indexes if requested.
+ */
+ if (including_indexes && relation->rd_rel->relhasindex)
+ {
+ List *parent_index_list = RelationGetIndexList(relation);
+ ListCell *parent_index_scan;
+
+ foreach(parent_index_scan, parent_index_list)
+ {
+ Oid parent_index_oid = lfirst_oid(parent_index_scan);
+ Relation parent_index;
+
+ parent_index = index_open(parent_index_oid, AccessShareLock);
+
+ /*
+ * Create new unique or primary key indexes on the child.
+ */
+ if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
+ {
+ IndexInfo *parent_index_info;
+ Constraint *n = makeNode(Constraint);
+ AttrNumber parent_attno;
+
+ parent_index_info = BuildIndexInfo(parent_index);
+
+ if (parent_index->rd_index->indisprimary)
+ {
+ n->contype = CONSTR_PRIMARY;
+ }
+ else
+ {
+ n->contype = CONSTR_UNIQUE;
+ }
+ /* Let DefineIndex name it */
+ n->name = NULL;
+ n->raw_expr = NULL;
+ n->cooked_expr = NULL;
+
+ /*
+ * Search through the possible index keys, and append
+ * the names of simple columns to the new index key list.
+ */
+ for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
+ parent_attno++)
+ {
+ Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
+ char *attributeName = NameStr(attribute->attname);
+
+ /*
+ * Ignore dropped columns in the parent.
+ */
+ if (!attribute->attisdropped)
+ n->keys = lappend(n->keys,
+ makeString(attributeName));
+ }
+
+ /* Add the new index constraint to the create context */
+ cxt->ixconstraints = lappend(cxt->ixconstraints, n);
+
+ ereport(NOTICE,
+ (errmsg("Index \"%s\" cloned.",
+ RelationGetRelationName(parent_index))));
+ }
+
+ relation_close(parent_index, AccessShareLock);
+ }
+ }
+
+ /*
* Close the parent rel, but keep our AccessShareLock on it until xact
* commit. That will prevent someone else from deleting or ALTERing the
* parent before the child is committed.
Index: doc/src/sgml/ref/create_table.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
retrieving revision 1.107
diff -c -r1.107 create_table.sgml
*** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
--- doc/src/sgml/ref/create_table.sgml 7 Mar 2007 01:43:13 -0000
***************
*** 23,29 ****
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> ( [
{ <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable> [
DEFAULT<replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable>
! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... }
[, ... ]
] )
[ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
--- 23,29 ----
CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> ( [
{ <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable> [
DEFAULT<replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
| <replaceable>table_constraint</replaceable>
! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES } ]
...}
[, ... ]
] )
[ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
***************
*** 237,243 ****
</varlistentry>
<varlistentry>
! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS
}]</literal></term>
<listitem>
<para>
The <literal>LIKE</literal> clause specifies a table from which
--- 237,243 ----
</varlistentry>
<varlistentry>
! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING | INDEXES } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
<listitem>
<para>
The <literal>LIKE</literal> clause specifies a table from which
***************
*** 260,269 ****
<para>
Not-null constraints are always copied to the new table.
<literal>CHECK</literal> constraints will only be copied if
! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
! constraints will never be copied. Also, no distinction is made between
! column constraints and table constraints — when constraints are
! requested, all check constraints are copied.
</para>
<para>
Note also that unlike <literal>INHERITS</literal>, copied columns and
--- 260,271 ----
<para>
Not-null constraints are always copied to the new table.
<literal>CHECK</literal> constraints will only be copied if
! <literal>INCLUDING CONSTRAINTS</literal> is specified. UNIQUE and
! PRIMARY KEY constraints will only be copied if
! <literal>INCLUDING INDEXES</literal> is specified. Also, no
! distinction is made between column constraints and table constraints
! — when constraints are requested, all check constraints are
! copied.
</para>
<para>
Note also that unlike <literal>INHERITS</literal>, copied columns and
Index: src/test/regress/sql/inherit.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
retrieving revision 1.10
diff -c -r1.10 inherit.sql
*** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
--- src/test/regress/sql/inherit.sql 7 Mar 2007 01:43:13 -0000
***************
*** 155,160 ****
--- 155,164 ----
INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
+ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
+ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
+ DROP TABLE inhg;
-- Test changing the type of inherited columns
Index: src/test/regress/expected/inherit.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
retrieving revision 1.20
diff -c -r1.20 inherit.out
*** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
--- src/test/regress/expected/inherit.out 7 Mar 2007 01:43:14 -0000
***************
*** 633,638 ****
--- 633,645 ----
(2 rows)
DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
+ NOTICE: Index "inhx_pkey" cloned.
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
+ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
+ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
+ ERROR: duplicate key violates unique constraint "inhg_pkey"
+ DROP TABLE inhg;
-- Test changing the type of inherited columns
insert into d values('test','one','two','three');
alter table a alter column aa type integer using bit_length(aa);
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Trevor Hardcastle wrote:
> Greetings all,
>
> I wrote this patch about a week ago to introduce myself to coding on
> PostgreSQL. I wasn't entirely sure what the 'INCLUDING INDEXES' option
> was meant to do, so I held off submitting it until I could get around to
> asking about that and tweaking the documentation to reflect the patch.
> By useful coincidence the thread "Auto creation of Partitions" had this
> post in it, which made the intent of the option clear enough for me to
> go ahead and see what people think of this.
>
> Gregory Stark wrote:
> > "NikhilS" <nikkhils@gmail.com> writes:
> >
> >
> >> the intention is to use this information from the parent and make it a
> >> property of the child table. This will avoid the step for the user having to
> >> manually specify CREATE INDEX and the likes on all the children tables
> >> one-by-one.
> >>
> >
> > Missed the start of this thread. A while back I had intended to add WITH
> > INDEXES to CREATE TABLE LIKE. That would let you create a tale LIKE parent
> > WITH CONSTRAINTS WITH INDEXES and get pretty much a perfect table ready for
> > adding to the inheritance structure.
> >
> >
> >
> So, that's what this patch does. When a table is created with 'CREATE
> TABLE ... LIKE parent INCLUDING INDEXES' this iterates over the parent
> table indexes looking for constraint indexes, and alters the
> CreateStmtContext to include equivalent indexes on the child table.
>
> This is probably a somewhat naive implementation, being a first attempt.
> I wasn't sure what sort of lock to place on the parent indexes or what
> tablespace the new indexes should be created in. Any help improving it
> would be appreciated.
>
> Thank you,
> -Trevor Hardcastle
>
> Index: src/backend/parser/analyze.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
> retrieving revision 1.361
> diff -c -r1.361 analyze.c
> *** src/backend/parser/analyze.c 20 Feb 2007 17:32:16 -0000 1.361
> --- src/backend/parser/analyze.c 7 Mar 2007 01:43:12 -0000
> ***************
> *** 14,19 ****
> --- 14,20 ----
> #include "postgres.h"
>
> #include "access/heapam.h"
> + #include "access/genam.h"
> #include "catalog/heap.h"
> #include "catalog/index.h"
> #include "catalog/namespace.h"
> ***************
> *** 40,45 ****
> --- 41,47 ----
> #include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> + #include "utils/relcache.h"
> #include "utils/syscache.h"
>
>
> ***************
> *** 1345,1355 ****
> }
> }
>
> - if (including_indexes)
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("LIKE INCLUDING INDEXES is not implemented")));
> -
> /*
> * Insert the copied attributes into the cxt for the new table
> * definition.
> --- 1347,1352 ----
> ***************
> *** 1448,1453 ****
> --- 1445,1519 ----
> }
>
> /*
> + * Clone constraint indexes if requested.
> + */
> + if (including_indexes && relation->rd_rel->relhasindex)
> + {
> + List *parent_index_list = RelationGetIndexList(relation);
> + ListCell *parent_index_scan;
> +
> + foreach(parent_index_scan, parent_index_list)
> + {
> + Oid parent_index_oid = lfirst_oid(parent_index_scan);
> + Relation parent_index;
> +
> + parent_index = index_open(parent_index_oid, AccessShareLock);
> +
> + /*
> + * Create new unique or primary key indexes on the child.
> + */
> + if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
> + {
> + IndexInfo *parent_index_info;
> + Constraint *n = makeNode(Constraint);
> + AttrNumber parent_attno;
> +
> + parent_index_info = BuildIndexInfo(parent_index);
> +
> + if (parent_index->rd_index->indisprimary)
> + {
> + n->contype = CONSTR_PRIMARY;
> + }
> + else
> + {
> + n->contype = CONSTR_UNIQUE;
> + }
> + /* Let DefineIndex name it */
> + n->name = NULL;
> + n->raw_expr = NULL;
> + n->cooked_expr = NULL;
> +
> + /*
> + * Search through the possible index keys, and append
> + * the names of simple columns to the new index key list.
> + */
> + for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
> + parent_attno++)
> + {
> + Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
> + char *attributeName = NameStr(attribute->attname);
> +
> + /*
> + * Ignore dropped columns in the parent.
> + */
> + if (!attribute->attisdropped)
> + n->keys = lappend(n->keys,
> + makeString(attributeName));
> + }
> +
> + /* Add the new index constraint to the create context */
> + cxt->ixconstraints = lappend(cxt->ixconstraints, n);
> +
> + ereport(NOTICE,
> + (errmsg("Index \"%s\" cloned.",
> + RelationGetRelationName(parent_index))));
> + }
> +
> + relation_close(parent_index, AccessShareLock);
> + }
> + }
> +
> + /*
> * Close the parent rel, but keep our AccessShareLock on it until xact
> * commit. That will prevent someone else from deleting or ALTERing the
> * parent before the child is committed.
> Index: doc/src/sgml/ref/create_table.sgml
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
> retrieving revision 1.107
> diff -c -r1.107 create_table.sgml
> *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
> --- doc/src/sgml/ref/create_table.sgml 7 Mar 2007 01:43:13 -0000
> ***************
> *** 23,29 ****
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> --- 23,29 ----
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES }
]... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> ***************
> *** 237,243 ****
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> --- 237,243 ----
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING | INDEXES } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> ***************
> *** 260,269 ****
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
> ! constraints will never be copied. Also, no distinction is made between
> ! column constraints and table constraints — when constraints are
> ! requested, all check constraints are copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> --- 260,271 ----
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified. UNIQUE and
> ! PRIMARY KEY constraints will only be copied if
> ! <literal>INCLUDING INDEXES</literal> is specified. Also, no
> ! distinction is made between column constraints and table constraints
> ! — when constraints are requested, all check constraints are
> ! copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> Index: src/test/regress/sql/inherit.sql
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
> retrieving revision 1.10
> diff -c -r1.10 inherit.sql
> *** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
> --- src/test/regress/sql/inherit.sql 7 Mar 2007 01:43:13 -0000
> ***************
> *** 155,160 ****
> --- 155,164 ----
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + DROP TABLE inhg;
>
>
> -- Test changing the type of inherited columns
> Index: src/test/regress/expected/inherit.out
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
> retrieving revision 1.20
> diff -c -r1.20 inherit.out
> *** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
> --- src/test/regress/expected/inherit.out 7 Mar 2007 01:43:14 -0000
> ***************
> *** 633,638 ****
> --- 633,645 ----
> (2 rows)
>
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + NOTICE: Index "inhx_pkey" cloned.
> + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + ERROR: duplicate key violates unique constraint "inhg_pkey"
> + DROP TABLE inhg;
> -- Test changing the type of inherited columns
> insert into d values('test','one','two','three');
> alter table a alter column aa type integer using bit_length(aa);
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Uh, shouldn't CREATE TABLE LIKE INCLUDING CONSTRAINTS already be including
any indexes in the parent table?
---------------------------------------------------------------------------
Trevor Hardcastle wrote:
> Greetings all,
>
> I wrote this patch about a week ago to introduce myself to coding on
> PostgreSQL. I wasn't entirely sure what the 'INCLUDING INDEXES' option
> was meant to do, so I held off submitting it until I could get around to
> asking about that and tweaking the documentation to reflect the patch.
> By useful coincidence the thread "Auto creation of Partitions" had this
> post in it, which made the intent of the option clear enough for me to
> go ahead and see what people think of this.
>
> Gregory Stark wrote:
> > "NikhilS" <nikkhils@gmail.com> writes:
> >
> >
> >> the intention is to use this information from the parent and make it a
> >> property of the child table. This will avoid the step for the user having to
> >> manually specify CREATE INDEX and the likes on all the children tables
> >> one-by-one.
> >>
> >
> > Missed the start of this thread. A while back I had intended to add WITH
> > INDEXES to CREATE TABLE LIKE. That would let you create a tale LIKE parent
> > WITH CONSTRAINTS WITH INDEXES and get pretty much a perfect table ready for
> > adding to the inheritance structure.
> >
> >
> >
> So, that's what this patch does. When a table is created with 'CREATE
> TABLE ... LIKE parent INCLUDING INDEXES' this iterates over the parent
> table indexes looking for constraint indexes, and alters the
> CreateStmtContext to include equivalent indexes on the child table.
>
> This is probably a somewhat naive implementation, being a first attempt.
> I wasn't sure what sort of lock to place on the parent indexes or what
> tablespace the new indexes should be created in. Any help improving it
> would be appreciated.
>
> Thank you,
> -Trevor Hardcastle
>
> Index: src/backend/parser/analyze.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
> retrieving revision 1.361
> diff -c -r1.361 analyze.c
> *** src/backend/parser/analyze.c 20 Feb 2007 17:32:16 -0000 1.361
> --- src/backend/parser/analyze.c 7 Mar 2007 01:43:12 -0000
> ***************
> *** 14,19 ****
> --- 14,20 ----
> #include "postgres.h"
>
> #include "access/heapam.h"
> + #include "access/genam.h"
> #include "catalog/heap.h"
> #include "catalog/index.h"
> #include "catalog/namespace.h"
> ***************
> *** 40,45 ****
> --- 41,47 ----
> #include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> + #include "utils/relcache.h"
> #include "utils/syscache.h"
>
>
> ***************
> *** 1345,1355 ****
> }
> }
>
> - if (including_indexes)
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("LIKE INCLUDING INDEXES is not implemented")));
> -
> /*
> * Insert the copied attributes into the cxt for the new table
> * definition.
> --- 1347,1352 ----
> ***************
> *** 1448,1453 ****
> --- 1445,1519 ----
> }
>
> /*
> + * Clone constraint indexes if requested.
> + */
> + if (including_indexes && relation->rd_rel->relhasindex)
> + {
> + List *parent_index_list = RelationGetIndexList(relation);
> + ListCell *parent_index_scan;
> +
> + foreach(parent_index_scan, parent_index_list)
> + {
> + Oid parent_index_oid = lfirst_oid(parent_index_scan);
> + Relation parent_index;
> +
> + parent_index = index_open(parent_index_oid, AccessShareLock);
> +
> + /*
> + * Create new unique or primary key indexes on the child.
> + */
> + if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
> + {
> + IndexInfo *parent_index_info;
> + Constraint *n = makeNode(Constraint);
> + AttrNumber parent_attno;
> +
> + parent_index_info = BuildIndexInfo(parent_index);
> +
> + if (parent_index->rd_index->indisprimary)
> + {
> + n->contype = CONSTR_PRIMARY;
> + }
> + else
> + {
> + n->contype = CONSTR_UNIQUE;
> + }
> + /* Let DefineIndex name it */
> + n->name = NULL;
> + n->raw_expr = NULL;
> + n->cooked_expr = NULL;
> +
> + /*
> + * Search through the possible index keys, and append
> + * the names of simple columns to the new index key list.
> + */
> + for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
> + parent_attno++)
> + {
> + Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
> + char *attributeName = NameStr(attribute->attname);
> +
> + /*
> + * Ignore dropped columns in the parent.
> + */
> + if (!attribute->attisdropped)
> + n->keys = lappend(n->keys,
> + makeString(attributeName));
> + }
> +
> + /* Add the new index constraint to the create context */
> + cxt->ixconstraints = lappend(cxt->ixconstraints, n);
> +
> + ereport(NOTICE,
> + (errmsg("Index \"%s\" cloned.",
> + RelationGetRelationName(parent_index))));
> + }
> +
> + relation_close(parent_index, AccessShareLock);
> + }
> + }
> +
> + /*
> * Close the parent rel, but keep our AccessShareLock on it until xact
> * commit. That will prevent someone else from deleting or ALTERing the
> * parent before the child is committed.
> Index: doc/src/sgml/ref/create_table.sgml
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
> retrieving revision 1.107
> diff -c -r1.107 create_table.sgml
> *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
> --- doc/src/sgml/ref/create_table.sgml 7 Mar 2007 01:43:13 -0000
> ***************
> *** 23,29 ****
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> --- 23,29 ----
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES }
]... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> ***************
> *** 237,243 ****
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> --- 237,243 ----
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING | INDEXES } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> ***************
> *** 260,269 ****
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
> ! constraints will never be copied. Also, no distinction is made between
> ! column constraints and table constraints — when constraints are
> ! requested, all check constraints are copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> --- 260,271 ----
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified. UNIQUE and
> ! PRIMARY KEY constraints will only be copied if
> ! <literal>INCLUDING INDEXES</literal> is specified. Also, no
> ! distinction is made between column constraints and table constraints
> ! — when constraints are requested, all check constraints are
> ! copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> Index: src/test/regress/sql/inherit.sql
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
> retrieving revision 1.10
> diff -c -r1.10 inherit.sql
> *** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
> --- src/test/regress/sql/inherit.sql 7 Mar 2007 01:43:13 -0000
> ***************
> *** 155,160 ****
> --- 155,164 ----
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + DROP TABLE inhg;
>
>
> -- Test changing the type of inherited columns
> Index: src/test/regress/expected/inherit.out
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
> retrieving revision 1.20
> diff -c -r1.20 inherit.out
> *** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
> --- src/test/regress/expected/inherit.out 7 Mar 2007 01:43:14 -0000
> ***************
> *** 633,638 ****
> --- 633,645 ----
> (2 rows)
>
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + NOTICE: Index "inhx_pkey" cloned.
> + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + ERROR: duplicate key violates unique constraint "inhg_pkey"
> + DROP TABLE inhg;
> -- Test changing the type of inherited columns
> insert into d values('test','one','two','three');
> alter table a alter column aa type integer using bit_length(aa);
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
"Bruce Momjian" <bruce@momjian.us> writes: > Uh, shouldn't CREATE TABLE LIKE INCLUDING CONSTRAINTS already be including > any indexes in the parent table? You could argue it should for unique indexes since our unique indexes are how we implement unique constraints. But I see no particular reason to expect it to copy random other indexes. At least its name doesn't lead one to expect it to. I also thought it was sort of strange to have a command that otherwise is just copying definitions suddenly start building whole new objects. I think I was thinking it would be a long slow operation but I suppose creating an empty index isn't really noticeably slow. It could print a NOTICE similar to what's printed when you create a primary key or unique constraint. It does mean that users would be unable to create a partition, load data, then build indexes. Perhaps it would be nice to have an ALTER TABLE foo LIKE bar INCLUDING CONSTRAINTS as well. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > "Bruce Momjian" <bruce@momjian.us> writes: > > > Uh, shouldn't CREATE TABLE LIKE INCLUDING CONSTRAINTS already be including > > any indexes in the parent table? > > You could argue it should for unique indexes since our unique indexes are how > we implement unique constraints. But I see no particular reason to expect it > to copy random other indexes. At least its name doesn't lead one to expect it > to. > > I also thought it was sort of strange to have a command that otherwise is just > copying definitions suddenly start building whole new objects. I think I was > thinking it would be a long slow operation but I suppose creating an empty > index isn't really noticeably slow. It could print a NOTICE similar to what's > printed when you create a primary key or unique constraint. > > It does mean that users would be unable to create a partition, load data, then > build indexes. Perhaps it would be nice to have an ALTER TABLE foo LIKE bar > INCLUDING CONSTRAINTS as well. The patch already _only_ does constraint(unique) indexes: > So, that's what this patch does. When a table is created with 'CREATE > TABLE ... LIKE parent INCLUDING INDEXES' this iterates over the parent > table indexes looking for constraint indexes, and alters the > CreateStmtContext to include equivalent indexes on the child table. so I am just suggesting it do that always for INCLUDING CONSTRAINTS, with a notice as you suggest. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Added to TODO:
o Have WITH CONSTRAINTS also create constraint indexes
http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php
---------------------------------------------------------------------------
Trevor Hardcastle wrote:
> Greetings all,
>
> I wrote this patch about a week ago to introduce myself to coding on
> PostgreSQL. I wasn't entirely sure what the 'INCLUDING INDEXES' option
> was meant to do, so I held off submitting it until I could get around to
> asking about that and tweaking the documentation to reflect the patch.
> By useful coincidence the thread "Auto creation of Partitions" had this
> post in it, which made the intent of the option clear enough for me to
> go ahead and see what people think of this.
>
> Gregory Stark wrote:
> > "NikhilS" <nikkhils@gmail.com> writes:
> >
> >
> >> the intention is to use this information from the parent and make it a
> >> property of the child table. This will avoid the step for the user having to
> >> manually specify CREATE INDEX and the likes on all the children tables
> >> one-by-one.
> >>
> >
> > Missed the start of this thread. A while back I had intended to add WITH
> > INDEXES to CREATE TABLE LIKE. That would let you create a tale LIKE parent
> > WITH CONSTRAINTS WITH INDEXES and get pretty much a perfect table ready for
> > adding to the inheritance structure.
> >
> >
> >
> So, that's what this patch does. When a table is created with 'CREATE
> TABLE ... LIKE parent INCLUDING INDEXES' this iterates over the parent
> table indexes looking for constraint indexes, and alters the
> CreateStmtContext to include equivalent indexes on the child table.
>
> This is probably a somewhat naive implementation, being a first attempt.
> I wasn't sure what sort of lock to place on the parent indexes or what
> tablespace the new indexes should be created in. Any help improving it
> would be appreciated.
>
> Thank you,
> -Trevor Hardcastle
>
> Index: src/backend/parser/analyze.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
> retrieving revision 1.361
> diff -c -r1.361 analyze.c
> *** src/backend/parser/analyze.c 20 Feb 2007 17:32:16 -0000 1.361
> --- src/backend/parser/analyze.c 7 Mar 2007 01:43:12 -0000
> ***************
> *** 14,19 ****
> --- 14,20 ----
> #include "postgres.h"
>
> #include "access/heapam.h"
> + #include "access/genam.h"
> #include "catalog/heap.h"
> #include "catalog/index.h"
> #include "catalog/namespace.h"
> ***************
> *** 40,45 ****
> --- 41,47 ----
> #include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> + #include "utils/relcache.h"
> #include "utils/syscache.h"
>
>
> ***************
> *** 1345,1355 ****
> }
> }
>
> - if (including_indexes)
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("LIKE INCLUDING INDEXES is not implemented")));
> -
> /*
> * Insert the copied attributes into the cxt for the new table
> * definition.
> --- 1347,1352 ----
> ***************
> *** 1448,1453 ****
> --- 1445,1519 ----
> }
>
> /*
> + * Clone constraint indexes if requested.
> + */
> + if (including_indexes && relation->rd_rel->relhasindex)
> + {
> + List *parent_index_list = RelationGetIndexList(relation);
> + ListCell *parent_index_scan;
> +
> + foreach(parent_index_scan, parent_index_list)
> + {
> + Oid parent_index_oid = lfirst_oid(parent_index_scan);
> + Relation parent_index;
> +
> + parent_index = index_open(parent_index_oid, AccessShareLock);
> +
> + /*
> + * Create new unique or primary key indexes on the child.
> + */
> + if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
> + {
> + IndexInfo *parent_index_info;
> + Constraint *n = makeNode(Constraint);
> + AttrNumber parent_attno;
> +
> + parent_index_info = BuildIndexInfo(parent_index);
> +
> + if (parent_index->rd_index->indisprimary)
> + {
> + n->contype = CONSTR_PRIMARY;
> + }
> + else
> + {
> + n->contype = CONSTR_UNIQUE;
> + }
> + /* Let DefineIndex name it */
> + n->name = NULL;
> + n->raw_expr = NULL;
> + n->cooked_expr = NULL;
> +
> + /*
> + * Search through the possible index keys, and append
> + * the names of simple columns to the new index key list.
> + */
> + for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
> + parent_attno++)
> + {
> + Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
> + char *attributeName = NameStr(attribute->attname);
> +
> + /*
> + * Ignore dropped columns in the parent.
> + */
> + if (!attribute->attisdropped)
> + n->keys = lappend(n->keys,
> + makeString(attributeName));
> + }
> +
> + /* Add the new index constraint to the create context */
> + cxt->ixconstraints = lappend(cxt->ixconstraints, n);
> +
> + ereport(NOTICE,
> + (errmsg("Index \"%s\" cloned.",
> + RelationGetRelationName(parent_index))));
> + }
> +
> + relation_close(parent_index, AccessShareLock);
> + }
> + }
> +
> + /*
> * Close the parent rel, but keep our AccessShareLock on it until xact
> * commit. That will prevent someone else from deleting or ALTERing the
> * parent before the child is committed.
> Index: doc/src/sgml/ref/create_table.sgml
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
> retrieving revision 1.107
> diff -c -r1.107 create_table.sgml
> *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
> --- doc/src/sgml/ref/create_table.sgml 7 Mar 2007 01:43:13 -0000
> ***************
> *** 23,29 ****
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS } ] ... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> --- 23,29 ----
> CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } ] TABLE <replaceable class="PARAMETER">table_name</replaceable> (
[
> { <replaceable class="PARAMETER">column_name</replaceable> <replaceable class="PARAMETER">data_type</replaceable>
[DEFAULT <replaceable>default_expr</> ] [ <replaceable class="PARAMETER">column_constraint</replaceable> [ ... ] ]
> | <replaceable>table_constraint</replaceable>
> ! | LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS | CONSTRAINTS | INDEXES }
]... }
> [, ... ]
> ] )
> [ INHERITS ( <replaceable>parent_table</replaceable> [, ... ] ) ]
> ***************
> *** 237,243 ****
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> --- 237,243 ----
> </varlistentry>
>
> <varlistentry>
> ! <term><literal>LIKE <replaceable>parent_table</replaceable> [ { INCLUDING | EXCLUDING | INDEXES } { DEFAULTS |
CONSTRAINTS} ]</literal></term>
> <listitem>
> <para>
> The <literal>LIKE</literal> clause specifies a table from which
> ***************
> *** 260,269 ****
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
> ! constraints will never be copied. Also, no distinction is made between
> ! column constraints and table constraints — when constraints are
> ! requested, all check constraints are copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> --- 260,271 ----
> <para>
> Not-null constraints are always copied to the new table.
> <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified. UNIQUE and
> ! PRIMARY KEY constraints will only be copied if
> ! <literal>INCLUDING INDEXES</literal> is specified. Also, no
> ! distinction is made between column constraints and table constraints
> ! — when constraints are requested, all check constraints are
> ! copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> Index: src/test/regress/sql/inherit.sql
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
> retrieving revision 1.10
> diff -c -r1.10 inherit.sql
> *** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
> --- src/test/regress/sql/inherit.sql 7 Mar 2007 01:43:13 -0000
> ***************
> *** 155,160 ****
> --- 155,164 ----
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + DROP TABLE inhg;
>
>
> -- Test changing the type of inherited columns
> Index: src/test/regress/expected/inherit.out
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
> retrieving revision 1.20
> diff -c -r1.20 inherit.out
> *** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
> --- src/test/regress/expected/inherit.out 7 Mar 2007 01:43:14 -0000
> ***************
> *** 633,638 ****
> --- 633,645 ----
> (2 rows)
>
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Copies indexes */
> + NOTICE: Index "inhx_pkey" cloned.
> + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> + INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints are copied */
> + ERROR: duplicate key violates unique constraint "inhg_pkey"
> + DROP TABLE inhg;
> -- Test changing the type of inherited columns
> insert into d values('test','one','two','three');
> alter table a alter column aa type integer using bit_length(aa);
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Hi,
--
EnterpriseDB http://www.enterprisedb.com
On 4/10/07, Bruce Momjian <bruce@momjian.us> wrote:
Trevor's patch does add unique/primary indexes. This would mean that we have to remove the syntax support for "INCLUDING INDEXES" and just add code to the existing WITH CONSTRAINTs code path from his patch.
Is there something else and hence we have the above TODO?
Regards,
Nikhils
Added to TODO:
o Have WITH CONSTRAINTS also create constraint indexes
http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php
Trevor's patch does add unique/primary indexes. This would mean that we have to remove the syntax support for "INCLUDING INDEXES" and just add code to the existing WITH CONSTRAINTs code path from his patch.
Is there something else and hence we have the above TODO?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS wrote: > Hi, > > On 4/10/07, Bruce Momjian <bruce@momjian.us> wrote: > > > > > > Added to TODO: > > > > o Have WITH CONSTRAINTS also create constraint indexes > > > > http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php > > > Trevor's patch does add unique/primary indexes. This would mean that we have > to remove the syntax support for "INCLUDING INDEXES" and just add code to > the existing WITH CONSTRAINTs code path from his patch. That is all that is required. > Is there something else and hence we have the above TODO? If someone wants to work on this item and submit it, we can review it for 8.3, but if not, it waits until 8.4. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > NikhilS wrote: > >> Hi, >> >> On 4/10/07, Bruce Momjian <bruce@momjian.us> wrote: >> >>> Added to TODO: >>> >>> o Have WITH CONSTRAINTS also create constraint indexes >>> >>> http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php >>> >> Trevor's patch does add unique/primary indexes. This would mean that we have >> to remove the syntax support for "INCLUDING INDEXES" and just add code to >> the existing WITH CONSTRAINTs code path from his patch. >> > > That is all that is required. > > >> Is there something else and hence we have the above TODO? >> > > If someone wants to work on this item and submit it, we can review it > for 8.3, but if not, it waits until 8.4. > > I've updated my patch to merge the INDEXES behavior implemented into the CONSTRAINTS option, and restore the current error triggered when you try to use the INDEXES option. Attached is the updated patch. I didn't remove the INDEXES syntax, just undocumented it again and put the error it raised back in. It seems like an implementation of copying all of the indexes could still use that syntax. Thank you for all the comments, -Trevor Hardcastle Index: src/backend/parser/analyze.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v retrieving revision 1.362 diff -c -r1.362 analyze.c *** src/backend/parser/analyze.c 13 Mar 2007 00:33:41 -0000 1.362 --- src/backend/parser/analyze.c 12 Apr 2007 17:54:49 -0000 *************** *** 28,33 **** --- 28,34 ---- #include "postgres.h" #include "access/heapam.h" + #include "access/genam.h" #include "catalog/heap.h" #include "catalog/index.h" #include "catalog/namespace.h" *************** *** 54,59 **** --- 55,61 ---- #include "utils/acl.h" #include "utils/builtins.h" #include "utils/lsyscache.h" + #include "utils/relcache.h" #include "utils/syscache.h" *************** *** 1331,1338 **** } /* ! * Copy CHECK constraints if requested, being careful to adjust ! * attribute numbers */ if (including_constraints && tupleDesc->constr) { --- 1333,1340 ---- } /* ! * Copy CHECK based constraints if requested, being careful to adjust ! * attribute numbers. Also duplicate unique index constraints. */ if (including_constraints && tupleDesc->constr) { *************** *** 1355,1360 **** --- 1357,1431 ---- n->indexspace = NULL; cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n); } + + /* + * Clone constraint indexes if any exist. + */ + if (relation->rd_rel->relhasindex) + { + List *parent_index_list = RelationGetIndexList(relation); + ListCell *parent_index_scan; + + foreach(parent_index_scan, parent_index_list) + { + Oid parent_index_oid = lfirst_oid(parent_index_scan); + Relation parent_index; + + parent_index = index_open(parent_index_oid, AccessShareLock); + + /* + * Create new unique or primary key indexes on the child. + */ + if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary) + { + IndexInfo *parent_index_info; + Constraint *n = makeNode(Constraint); + AttrNumber parent_attno; + + parent_index_info = BuildIndexInfo(parent_index); + + if (parent_index->rd_index->indisprimary) + { + n->contype = CONSTR_PRIMARY; + } + else + { + n->contype = CONSTR_UNIQUE; + } + /* Let DefineIndex name it */ + n->name = NULL; + n->raw_expr = NULL; + n->cooked_expr = NULL; + + /* + * Search through the possible index keys, and append + * the names of simple columns to the new index key list. + */ + for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts; + parent_attno++) + { + Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1]; + char *attributeName = NameStr(attribute->attname); + + /* + * Ignore dropped columns in the parent. + */ + if (!attribute->attisdropped) + n->keys = lappend(n->keys, + makeString(attributeName)); + } + + /* Add the new index constraint to the create context */ + cxt->ixconstraints = lappend(cxt->ixconstraints, n); + + ereport(NOTICE, + (errmsg("Index \"%s\" cloned.", + RelationGetRelationName(parent_index)))); + } + + relation_close(parent_index, AccessShareLock); + } + } } /* Index: src/test/regress/sql/inherit.sql =================================================================== RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v retrieving revision 1.10 diff -c -r1.10 inherit.sql *** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10 --- src/test/regress/sql/inherit.sql 12 Apr 2007 17:54:49 -0000 *************** *** 151,160 **** DROP TABLE inhg; CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */ ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */ INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */ SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */ DROP TABLE inhg; -- Test changing the type of inherited columns --- 151,161 ---- DROP TABLE inhg; CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */ ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */ INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */ SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */ DROP TABLE inhg; + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */ -- Test changing the type of inherited columns Index: src/test/regress/expected/inherit.out =================================================================== RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v retrieving revision 1.20 diff -c -r1.20 inherit.out *** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20 --- src/test/regress/expected/inherit.out 12 Apr 2007 17:54:49 -0000 *************** *** 621,638 **** INSERT INTO inhg VALUES ('foo'); DROP TABLE inhg; CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */ INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */ ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */ INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */ ERROR: new row for relation "inhg" violates check constraint "foo" SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */ x | xx | y ---+------+--- x | text | y ! x | text | y ! (2 rows) DROP TABLE inhg; -- Test changing the type of inherited columns insert into d values('test','one','two','three'); alter table a alter column aa type integer using bit_length(aa); --- 621,642 ---- INSERT INTO inhg VALUES ('foo'); DROP TABLE inhg; CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */ + NOTICE: Index "inhx_pkey" cloned. + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg" INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */ ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */ ! ERROR: duplicate key violates unique constraint "inhg_pkey" INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */ ERROR: new row for relation "inhg" violates check constraint "foo" SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */ x | xx | y ---+------+--- x | text | y ! (1 row) DROP TABLE inhg; + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */ + ERROR: LIKE INCLUDING INDEXES is not implemented -- Test changing the type of inherited columns insert into d values('test','one','two','three'); alter table a alter column aa type integer using bit_length(aa); Index: doc/src/sgml/ref/create_table.sgml =================================================================== RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v retrieving revision 1.107 diff -c -r1.107 create_table.sgml *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107 --- doc/src/sgml/ref/create_table.sgml 12 Apr 2007 17:54:50 -0000 *************** *** 259,269 **** </para> <para> Not-null constraints are always copied to the new table. ! <literal>CHECK</literal> constraints will only be copied if ! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of ! constraints will never be copied. Also, no distinction is made between ! column constraints and table constraints — when constraints are ! requested, all check constraints are copied. </para> <para> Note also that unlike <literal>INHERITS</literal>, copied columns and --- 259,268 ---- </para> <para> Not-null constraints are always copied to the new table. ! <literal>CHECK, UNIQUE, and PRIMARY KEY</literal> constraints will only ! be copied if <literal>INCLUDING CONSTRAINTS</literal> is specified. Also, ! no distinction is made between column constraints and table constraints ! — when constraints are requested, all check constraints are copied. </para> <para> Note also that unlike <literal>INHERITS</literal>, copied columns and
Hi Trevor,
The above is not used anywhere else in the code and seems redundant.
DefineIndex will give out a message anyways for unique/primary keys. The above seems additional to it.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
+
+ parent_index_info = BuildIndexInfo(parent_index);
The above is not used anywhere else in the code and seems redundant.
+
+ ereport(NOTICE,
+ (errmsg("Index \"%s\" cloned.",
+ RelationGetRelationName(parent_index))));
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS wrote:
> Hi Trevor,
>
>
> +
> + parent_index_info =
> BuildIndexInfo(parent_index);
>
>
> The above is not used anywhere else in the code and seems redundant.
Yep, pulled that out.
>
> +
> + ereport(NOTICE,
> +
> (errmsg("Index \"%s\" cloned.",
> +
> RelationGetRelationName(parent_index))));
>
>
> DefineIndex will give out a message anyways for unique/primary keys.
> The above seems additional to it.
The original reason for this was the support for copying all indexes,
but it doesn't make much sense now. I've pulled it too.
Thanks for pointing those out. An updated patch is attached.
-Trevor Hardcastle
Index: src/backend/parser/analyze.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
retrieving revision 1.362
diff -c -r1.362 analyze.c
*** src/backend/parser/analyze.c 13 Mar 2007 00:33:41 -0000 1.362
--- src/backend/parser/analyze.c 13 Apr 2007 16:41:46 -0000
***************
*** 28,33 ****
--- 28,34 ----
#include "postgres.h"
#include "access/heapam.h"
+ #include "access/genam.h"
#include "catalog/heap.h"
#include "catalog/index.h"
#include "catalog/namespace.h"
***************
*** 54,59 ****
--- 55,61 ----
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
+ #include "utils/relcache.h"
#include "utils/syscache.h"
***************
*** 1331,1338 ****
}
/*
! * Copy CHECK constraints if requested, being careful to adjust
! * attribute numbers
*/
if (including_constraints && tupleDesc->constr)
{
--- 1333,1340 ----
}
/*
! * Copy CHECK based constraints if requested, being careful to adjust
! * attribute numbers. Also duplicate unique index constraints.
*/
if (including_constraints && tupleDesc->constr)
{
***************
*** 1355,1360 ****
--- 1357,1424 ----
n->indexspace = NULL;
cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n);
}
+
+ /*
+ * Clone constraint indexes if any exist.
+ */
+ if (relation->rd_rel->relhasindex)
+ {
+ List *parent_index_list = RelationGetIndexList(relation);
+ ListCell *parent_index_scan;
+
+ foreach(parent_index_scan, parent_index_list)
+ {
+ Oid parent_index_oid = lfirst_oid(parent_index_scan);
+ Relation parent_index;
+
+ parent_index = index_open(parent_index_oid, AccessShareLock);
+
+ /*
+ * Create new unique or primary key indexes on the child.
+ */
+ if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
+ {
+ Constraint *n = makeNode(Constraint);
+ AttrNumber parent_attno;
+
+ if (parent_index->rd_index->indisprimary)
+ {
+ n->contype = CONSTR_PRIMARY;
+ }
+ else
+ {
+ n->contype = CONSTR_UNIQUE;
+ }
+ /* Let DefineIndex name it */
+ n->name = NULL;
+ n->raw_expr = NULL;
+ n->cooked_expr = NULL;
+
+ /*
+ * Search through the possible index keys, and append
+ * the names of simple columns to the new index key list.
+ */
+ for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
+ parent_attno++)
+ {
+ Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
+ char *attributeName = NameStr(attribute->attname);
+
+ /*
+ * Ignore dropped columns in the parent.
+ */
+ if (!attribute->attisdropped)
+ n->keys = lappend(n->keys,
+ makeString(attributeName));
+ }
+
+ /* Add the new index constraint to the create context */
+ cxt->ixconstraints = lappend(cxt->ixconstraints, n);
+ }
+
+ relation_close(parent_index, AccessShareLock);
+ }
+ }
}
/*
Index: src/test/regress/sql/inherit.sql
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
retrieving revision 1.10
diff -c -r1.10 inherit.sql
*** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
--- src/test/regress/sql/inherit.sql 13 Apr 2007 16:41:46 -0000
***************
*** 151,160 ****
DROP TABLE inhg;
CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
DROP TABLE inhg;
-- Test changing the type of inherited columns
--- 151,161 ----
DROP TABLE inhg;
CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
-- Test changing the type of inherited columns
Index: src/test/regress/expected/inherit.out
===================================================================
RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
retrieving revision 1.20
diff -c -r1.20 inherit.out
*** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
--- src/test/regress/expected/inherit.out 13 Apr 2007 16:41:46 -0000
***************
*** 621,638 ****
INSERT INTO inhg VALUES ('foo');
DROP TABLE inhg;
CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
ERROR: new row for relation "inhg" violates check constraint "foo"
SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
x | xx | y
---+------+---
x | text | y
! x | text | y
! (2 rows)
DROP TABLE inhg;
-- Test changing the type of inherited columns
insert into d values('test','one','two','three');
alter table a alter column aa type integer using bit_length(aa);
--- 621,641 ----
INSERT INTO inhg VALUES ('foo');
DROP TABLE inhg;
CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
! ERROR: duplicate key violates unique constraint "inhg_pkey"
INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
ERROR: new row for relation "inhg" violates check constraint "foo"
SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
x | xx | y
---+------+---
x | text | y
! (1 row)
DROP TABLE inhg;
+ CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
+ ERROR: LIKE INCLUDING INDEXES is not implemented
-- Test changing the type of inherited columns
insert into d values('test','one','two','three');
alter table a alter column aa type integer using bit_length(aa);
Index: doc/src/sgml/ref/create_table.sgml
===================================================================
RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
retrieving revision 1.107
diff -c -r1.107 create_table.sgml
*** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
--- doc/src/sgml/ref/create_table.sgml 13 Apr 2007 16:41:46 -0000
***************
*** 259,269 ****
</para>
<para>
Not-null constraints are always copied to the new table.
! <literal>CHECK</literal> constraints will only be copied if
! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
! constraints will never be copied. Also, no distinction is made between
! column constraints and table constraints — when constraints are
! requested, all check constraints are copied.
</para>
<para>
Note also that unlike <literal>INHERITS</literal>, copied columns and
--- 259,268 ----
</para>
<para>
Not-null constraints are always copied to the new table.
! <literal>CHECK, UNIQUE, and PRIMARY KEY</literal> constraints will only
! be copied if <literal>INCLUDING CONSTRAINTS</literal> is specified. Also,
! no distinction is made between column constraints and table constraints
! — when constraints are requested, all check constraints are copied.
</para>
<para>
Note also that unlike <literal>INHERITS</literal>, copied columns and
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Trevor Hardcastle wrote:
> NikhilS wrote:
> > Hi Trevor,
> >
> >
> > +
> > + parent_index_info =
> > BuildIndexInfo(parent_index);
> >
> >
> > The above is not used anywhere else in the code and seems redundant.
> Yep, pulled that out.
> >
> > +
> > + ereport(NOTICE,
> > +
> > (errmsg("Index \"%s\" cloned.",
> > +
> > RelationGetRelationName(parent_index))));
> >
> >
> > DefineIndex will give out a message anyways for unique/primary keys.
> > The above seems additional to it.
> The original reason for this was the support for copying all indexes,
> but it doesn't make much sense now. I've pulled it too.
>
> Thanks for pointing those out. An updated patch is attached.
>
> -Trevor Hardcastle
>
> Index: src/backend/parser/analyze.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v
> retrieving revision 1.362
> diff -c -r1.362 analyze.c
> *** src/backend/parser/analyze.c 13 Mar 2007 00:33:41 -0000 1.362
> --- src/backend/parser/analyze.c 13 Apr 2007 16:41:46 -0000
> ***************
> *** 28,33 ****
> --- 28,34 ----
> #include "postgres.h"
>
> #include "access/heapam.h"
> + #include "access/genam.h"
> #include "catalog/heap.h"
> #include "catalog/index.h"
> #include "catalog/namespace.h"
> ***************
> *** 54,59 ****
> --- 55,61 ----
> #include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> + #include "utils/relcache.h"
> #include "utils/syscache.h"
>
>
> ***************
> *** 1331,1338 ****
> }
>
> /*
> ! * Copy CHECK constraints if requested, being careful to adjust
> ! * attribute numbers
> */
> if (including_constraints && tupleDesc->constr)
> {
> --- 1333,1340 ----
> }
>
> /*
> ! * Copy CHECK based constraints if requested, being careful to adjust
> ! * attribute numbers. Also duplicate unique index constraints.
> */
> if (including_constraints && tupleDesc->constr)
> {
> ***************
> *** 1355,1360 ****
> --- 1357,1424 ----
> n->indexspace = NULL;
> cxt->ckconstraints = lappend(cxt->ckconstraints, (Node *) n);
> }
> +
> + /*
> + * Clone constraint indexes if any exist.
> + */
> + if (relation->rd_rel->relhasindex)
> + {
> + List *parent_index_list = RelationGetIndexList(relation);
> + ListCell *parent_index_scan;
> +
> + foreach(parent_index_scan, parent_index_list)
> + {
> + Oid parent_index_oid = lfirst_oid(parent_index_scan);
> + Relation parent_index;
> +
> + parent_index = index_open(parent_index_oid, AccessShareLock);
> +
> + /*
> + * Create new unique or primary key indexes on the child.
> + */
> + if (parent_index->rd_index->indisunique || parent_index->rd_index->indisprimary)
> + {
> + Constraint *n = makeNode(Constraint);
> + AttrNumber parent_attno;
> +
> + if (parent_index->rd_index->indisprimary)
> + {
> + n->contype = CONSTR_PRIMARY;
> + }
> + else
> + {
> + n->contype = CONSTR_UNIQUE;
> + }
> + /* Let DefineIndex name it */
> + n->name = NULL;
> + n->raw_expr = NULL;
> + n->cooked_expr = NULL;
> +
> + /*
> + * Search through the possible index keys, and append
> + * the names of simple columns to the new index key list.
> + */
> + for (parent_attno = 1; parent_attno <= parent_index->rd_att->natts;
> + parent_attno++)
> + {
> + Form_pg_attribute attribute = parent_index->rd_att->attrs[parent_attno - 1];
> + char *attributeName = NameStr(attribute->attname);
> +
> + /*
> + * Ignore dropped columns in the parent.
> + */
> + if (!attribute->attisdropped)
> + n->keys = lappend(n->keys,
> + makeString(attributeName));
> + }
> +
> + /* Add the new index constraint to the create context */
> + cxt->ixconstraints = lappend(cxt->ixconstraints, n);
> + }
> +
> + relation_close(parent_index, AccessShareLock);
> + }
> + }
> }
>
> /*
> Index: src/test/regress/sql/inherit.sql
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/sql/inherit.sql,v
> retrieving revision 1.10
> diff -c -r1.10 inherit.sql
> *** src/test/regress/sql/inherit.sql 27 Jun 2006 03:43:20 -0000 1.10
> --- src/test/regress/sql/inherit.sql 13 Apr 2007 16:41:46 -0000
> ***************
> *** 151,160 ****
> DROP TABLE inhg;
> CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
> INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> DROP TABLE inhg;
>
>
> -- Test changing the type of inherited columns
> --- 151,161 ----
> DROP TABLE inhg;
> CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
> INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
>
>
> -- Test changing the type of inherited columns
> Index: src/test/regress/expected/inherit.out
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/src/test/regress/expected/inherit.out,v
> retrieving revision 1.20
> diff -c -r1.20 inherit.out
> *** src/test/regress/expected/inherit.out 27 Jun 2006 03:43:20 -0000 1.20
> --- src/test/regress/expected/inherit.out 13 Apr 2007 16:41:46 -0000
> ***************
> *** 621,638 ****
> INSERT INTO inhg VALUES ('foo');
> DROP TABLE inhg;
> CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
> INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds -- Unique constraints not copied */
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> ERROR: new row for relation "inhg" violates check constraint "foo"
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> x | xx | y
> ---+------+---
> x | text | y
> ! x | text | y
> ! (2 rows)
>
> DROP TABLE inhg;
> -- Test changing the type of inherited columns
> insert into d values('test','one','two','three');
> alter table a alter column aa type integer using bit_length(aa);
> --- 621,641 ----
> INSERT INTO inhg VALUES ('foo');
> DROP TABLE inhg;
> CREATE TABLE inhg (x text, LIKE inhx INCLUDING CONSTRAINTS, y text); /* Copies constraints */
> + NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "inhg_pkey" for table "inhg"
> INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Succeeds */
> ! INSERT INTO inhg VALUES ('x', 'text', 'y'); /* Fails -- Unique constraints copied */
> ! ERROR: duplicate key violates unique constraint "inhg_pkey"
> INSERT INTO inhg VALUES ('x', 'foo', 'y'); /* fails due to constraint */
> ERROR: new row for relation "inhg" violates check constraint "foo"
> SELECT * FROM inhg; /* Two records with three columns in order x=x, xx=text, y=y */
> x | xx | y
> ---+------+---
> x | text | y
> ! (1 row)
>
> DROP TABLE inhg;
> + CREATE TABLE inhg (x text, LIKE inhx INCLUDING INDEXES, y text); /* Unimplemented */
> + ERROR: LIKE INCLUDING INDEXES is not implemented
> -- Test changing the type of inherited columns
> insert into d values('test','one','two','three');
> alter table a alter column aa type integer using bit_length(aa);
> Index: doc/src/sgml/ref/create_table.sgml
> ===================================================================
> RCS file: /projects/cvsroot/pgsql/doc/src/sgml/ref/create_table.sgml,v
> retrieving revision 1.107
> diff -c -r1.107 create_table.sgml
> *** doc/src/sgml/ref/create_table.sgml 1 Feb 2007 00:28:18 -0000 1.107
> --- doc/src/sgml/ref/create_table.sgml 13 Apr 2007 16:41:46 -0000
> ***************
> *** 259,269 ****
> </para>
> <para>
> Not-null constraints are always copied to the new table.
> ! <literal>CHECK</literal> constraints will only be copied if
> ! <literal>INCLUDING CONSTRAINTS</literal> is specified; other types of
> ! constraints will never be copied. Also, no distinction is made between
> ! column constraints and table constraints — when constraints are
> ! requested, all check constraints are copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
> --- 259,268 ----
> </para>
> <para>
> Not-null constraints are always copied to the new table.
> ! <literal>CHECK, UNIQUE, and PRIMARY KEY</literal> constraints will only
> ! be copied if <literal>INCLUDING CONSTRAINTS</literal> is specified. Also,
> ! no distinction is made between column constraints and table constraints
> ! — when constraints are requested, all check constraints are copied.
> </para>
> <para>
> Note also that unlike <literal>INHERITS</literal>, copied columns and
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
This patch needs more work. How carefully did you test it? * the patch failed to copy index constraints if there was not also at least one CHECK constraint defined on the table * the patch is broken for expressional indexes, and silently omits copying the predicate that may be associated with an index. It also doesn't copy the index's amoptions (WITH clause), or the NULLS FIRST/etc. options that may be associated with any of the index's columns. In other words, copying column names does not suffice to duplicate the index constraint. Perhaps the right fix is to implement support for INCLUDING INDEXES, and then use that code to copy the definition of any constraint indexes in INCLUDING INDEXES? * should we copy invalid indexes? I suppose there's nothing wrong with copying them... * we should probably hold the AccessShareLock on copied indexes till end of xact, per the comments at the end of transformInhRelation() * index_open() should be matched with index_close(), not relation_close(). (In the current implementation, index_close() and relation_close() happen to be identical, so it still worked.) I've attached a revised version of the patch, but I didn't fix the second or third bullets in the list. -Neil
Attachment
Hi Neil,
Since this patch is only supposed to copy unique/primary indexes, I dont think we will ever have predicates associated to such indexes?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
* the patch is broken for expressional indexes, and silently omits
copying the predicate that may be associated with an index. It also
doesn't copy the index's amoptions (WITH clause), or the NULLS
FIRST/etc. options that may be associated with any of the index's
columns.
Since this patch is only supposed to copy unique/primary indexes, I dont think we will ever have predicates associated to such indexes?
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS <nikkhils@gmail.com> writes:
> Hi Neil,
>> * the patch is broken for expressional indexes, and silently omits
>> copying the predicate that may be associated with an index.
> Since this patch is only supposed to copy unique/primary indexes, I dont
> think we will ever have predicates associated to such indexes?
Huh? I would expect a clause "INCLUDING INDEXES" to mean copying *all*
indexes. A clause "INCLUDING CONSTRAINTS" would reasonably act as you
suggest, ie copy only indexes derived from SQL constraint clauses.
regards, tom lane
On Wed, 2007-02-05 at 17:09 +0530, NikhilS wrote: > Since this patch is only supposed to copy unique/primary indexes, I > dont think we will ever have predicates associated to such indexes? Nope: neilc=# create table t1 (a int, b int); CREATE TABLE neilc=# create unique index t1_a_idx on t1 ((a + b)) where (a > 5); CREATE INDEX -Neil
Hi,
--
EnterpriseDB http://www.enterprisedb.com
On 5/3/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
But this patch is not for the "INCLUDING INDEXES" case. As mentioned by Bruce earlier in this thread, this patch is for the following TODO:
o Have WITH CONSTRAINTS also create constraint indexes
http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php
Regards,
Nikhils
NikhilS <nikkhils@gmail.com> writes:
> Hi Neil,
>> * the patch is broken for expressional indexes, and silently omits
>> copying the predicate that may be associated with an index.
> Since this patch is only supposed to copy unique/primary indexes, I dont
> think we will ever have predicates associated to such indexes?
Huh? I would expect a clause "INCLUDING INDEXES" to mean copying *all*
indexes. A clause "INCLUDING CONSTRAINTS" would reasonably act as you
suggest, ie copy only indexes derived from SQL constraint clauses.
But this patch is not for the "INCLUDING INDEXES" case. As mentioned by Bruce earlier in this thread, this patch is for the following TODO:
o Have WITH CONSTRAINTS also create constraint indexes
http://archives.postgresql.org/pgsql-patches/2007-04/msg00149.php
Regards,
Nikhils
regards, tom lane
--
EnterpriseDB http://www.enterprisedb.com
NikhilS <nikkhils@gmail.com> writes:
> But this patch is not for the "INCLUDING INDEXES" case.
Pardon me for being misled by the thread title :-(
regards, tom lane
On Fri, 2007-27-04 at 18:59 -0400, Neil Conway wrote: > This patch needs more work. Has a revised version of this patch been submitted? -Neil
Hi,
I just now realized that even though we allow the above. We do not allow:
pg=# create table t1 (a int, b int, unique(a+b));
nor the where clause syntax.
Any specific reason for this behaviour?
If we want to pass such kinds of expr, predicate based constraints via the "LIKE .. INCLUDING CONSTRAINTS" statements, transformIndexConstraints as well as the Constraint structure might need modifications.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Nope:
neilc=# create table t1 (a int, b int);
CREATE TABLE
neilc=# create unique index t1_a_idx on t1 ((a + b)) where (a > 5);
CREATE INDEX
I just now realized that even though we allow the above. We do not allow:
pg=# create table t1 (a int, b int, unique(a+b));
nor the where clause syntax.
Any specific reason for this behaviour?
If we want to pass such kinds of expr, predicate based constraints via the "LIKE .. INCLUDING CONSTRAINTS" statements, transformIndexConstraints as well as the Constraint structure might need modifications.
Regards,
Nikhils
EnterpriseDB http://www.enterprisedb.com
NikhilS <nikkhils@gmail.com> writes:
> I just now realized that even though we allow the above. We do not allow:
> pg=# create table t1 (a int, b int, unique(a+b));
> Any specific reason for this behaviour?
It'd be contrary to SQL spec. The UNIQUE constraint takes a list of
column names, full stop.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > NikhilS <nikkhils@gmail.com> writes: >> I just now realized that even though we allow the above. We do not allow: > >> pg=# create table t1 (a int, b int, unique(a+b)); > >> Any specific reason for this behaviour? > > It'd be contrary to SQL spec. The UNIQUE constraint takes a list of > column names, full stop. Does the SQL spec actually specify what happens if you provide an non-compliant table definition like this? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes:
>> It'd be contrary to SQL spec. The UNIQUE constraint takes a list of
>> column names, full stop.
> Does the SQL spec actually specify what happens if you provide an
> non-compliant table definition like this?
It does not. We could accept expressions there, and pray that the SQL
committee never extends the spec syntax in a direction incompatible with
that. That seems like a pretty risky thing to do though.
[ thinks... ] I think actually the prior discussions about this have
suggested importing our CREATE INDEX argument-list syntax into the
UNIQUE/PRIMARY KEY syntax, lock stock and barrel. Between the optional
opclass and sort direction and the slightly klugy requirement to
parenthesize expressions, that seemed clearly at huge risk of putting us
behind the compatibility eight-ball. You could make an argument that
the only plausible way that the committee would extend the syntax would
be to allow plain expressions instead of column names, and thus that
allowing that but not our other extensions would be safe. You might be
right about that ... or not. The fact that the committee still hasn't
made this seemingly-obvious extension makes me wonder if they have plans
we don't know about. There are certainly a ton of bells and whistles
in SQL2003 that're far less useful than this would be.
In any case there will always be features in CREATE INDEX that we
daren't drop into the standard constraint syntax, so the argument
for importing just this one seems a bit weak.
BTW, has anyone checked to see if Oracle and/or DB2 allow expressions
here? I think it's unlikely the committee would do anything to break
the standards compliance of those guys, so we might be safe in following
their lead if there is one.
regards, tom lane
I wrote:
> Gregory Stark <stark@enterprisedb.com> writes:
>> Does the SQL spec actually specify what happens if you provide an
>> non-compliant table definition like this?
> It does not. We could accept expressions there, and pray that the SQL
> committee never extends the spec syntax in a direction incompatible with
> that. That seems like a pretty risky thing to do though.
[ remembering previous discussions more clearly... ] Actually there
is a concrete problem here: unique constraints are supposed to be
represented in the information_schema views, and there is no
spec-compliant way to do that for a constraint on something other than
a column. We'd have to guess at what the SQL committee would do about
that, and the odds of guessing exactly right don't seem encouraging.
regards, tom lane
On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote: > Has a revised version of this patch been submitted? In the absence of a revised patch, I can finish the feature myself, but I won't get the free cycles until after PGCon. I can commit to getting it done before the end of May, or else we can just push this to 8.4. -Neil
Neil Conway wrote: > On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote: > > Has a revised version of this patch been submitted? > > In the absence of a revised patch, I can finish the feature myself, but > I won't get the free cycles until after PGCon. I can commit to getting > it done before the end of May, or else we can just push this to 8.4. We will keep it in the queue then. We can always push it for 8.4 if it if we are near beta. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Hi,
Considering that a unique index is a unique constraint, then isn't allowing expressional unique indexes contradictory to the above?
It seems that "CREATE UNIQUE INDEX" currently does not pass isconstraint as true to DefineIndex, otherwise index_create() would have cribbed with:
"constraints cannot have index expressions" error.
Since this patch is going to consider creating unique/primary indexes assuming them to be constraints, IMHO we should be uniform about unique constraints semantics.
That might mean that we only support expressionless, non-predicate indexes via "INCLUDING CONSTRAINTS"?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
[ remembering previous discussions more clearly... ] Actually there
is a concrete problem here: unique constraints are supposed to be
represented in the information_schema views, and there is no
spec-compliant way to do that for a constraint on something other than
a column. We'd have to guess at what the SQL committee would do about
that, and the odds of guessing exactly right don't seem encouraging.
It seems that "CREATE UNIQUE INDEX" currently does not pass isconstraint as true to DefineIndex, otherwise index_create() would have cribbed with:
"constraints cannot have index expressions" error.
Since this patch is going to consider creating unique/primary indexes assuming them to be constraints, IMHO we should be uniform about unique constraints semantics.
That might mean that we only support expressionless, non-predicate indexes via "INCLUDING CONSTRAINTS"?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS <nikkhils@gmail.com> writes:
>> [ remembering previous discussions more clearly... ] Actually there
>> is a concrete problem here: unique constraints are supposed to be
>> represented in the information_schema views, and there is no
>> spec-compliant way to do that for a constraint on something other than
>> a column. We'd have to guess at what the SQL committee would do about
>> that, and the odds of guessing exactly right don't seem encouraging.
> Considering that a unique index is a unique constraint,
No, it isn't. You are confusing definition and implementation. The
spec requires us to do X, Y, and Z in response to the unique-constraint
syntax. It says nothing about what CREATE INDEX does.
> Since this patch is going to consider creating unique/primary indexes
> assuming them to be constraints,
If it does that it will be rejected. There is a difference here and
that difference has to be maintained.
The correct way to think about this is that a pg_constraint entry of
type "unique" or "primary key" has an associated index that is part of
its implementation (and therefore has an "internal" dependency on the
constraint). But they are far from being the same thing.
regression=# create table foo (f1 int unique);
NOTICE: CREATE TABLE / UNIQUE will create implicit index "foo_f1_key" for table "foo"
CREATE TABLE
regression=# drop index foo_f1_key;
ERROR: cannot drop index foo_f1_key because constraint foo_f1_key on table foo requires it
HINT: You can drop constraint foo_f1_key on table foo instead.
regression=# alter table foo drop constraint foo_f1_key;
ALTER TABLE
regression=#
regards, tom lane
Hi,
--
EnterpriseDB http://www.enterprisedb.com
> Since this patch is going to consider creating unique/primary indexes
> assuming them to be constraints,
If it does that it will be rejected. There is a difference here and
that difference has to be maintained.
The correct way to think about this is that a pg_constraint entry of
type "unique" or "primary key" has an associated index that is part of
its implementation (and therefore has an "internal" dependency on the
constraint). But they are far from being the same thing.
Thanks Tom, I understand the difference now. I have a working patch and will send it to Neil for review tommorrow.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Hi Neil,
EnterpriseDB http://www.enterprisedb.com
On 5/18/07, Neil Conway <neilc@samurai.com> wrote:
I had spent some time on this earlier so decided to complete and send the patch to you for review. This patch supports copying of expressions, predicates, opclass, amorder, reloptions etc. The test case also contains some more additions with this patch. Please let me know if there are any issues.
Also, if this patch is acceptable, I think the mechanism provided here can be used to support "INCLUDING INDEXES" case easily too.
Regards,
Nikhils
-- On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote:
> Has a revised version of this patch been submitted?
In the absence of a revised patch, I can finish the feature myself, but
I won't get the free cycles until after PGCon. I can commit to getting
it done before the end of May, or else we can just push this to 8.4.
I had spent some time on this earlier so decided to complete and send the patch to you for review. This patch supports copying of expressions, predicates, opclass, amorder, reloptions etc. The test case also contains some more additions with this patch. Please let me know if there are any issues.
Also, if this patch is acceptable, I think the mechanism provided here can be used to support "INCLUDING INDEXES" case easily too.
Regards,
Nikhils
EnterpriseDB http://www.enterprisedb.com
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
NikhilS wrote:
> Hi Neil,
>
> On 5/18/07, Neil Conway <neilc@samurai.com> wrote:
> >
> > On Mon, 2007-14-05 at 22:58 -0400, Neil Conway wrote:
> > > Has a revised version of this patch been submitted?
> >
> > In the absence of a revised patch, I can finish the feature myself, but
> > I won't get the free cycles until after PGCon. I can commit to getting
> > it done before the end of May, or else we can just push this to 8.4.
>
>
> I had spent some time on this earlier so decided to complete and send the
> patch to you for review. This patch supports copying of expressions,
> predicates, opclass, amorder, reloptions etc. The test case also contains
> some more additions with this patch. Please let me know if there are any
> issues.
>
> Also, if this patch is acceptable, I think the mechanism provided here can
> be used to support "INCLUDING INDEXES" case easily too.
>
> Regards,
> Nikhils
> --
> EnterpriseDB http://www.enterprisedb.com
[ Attachment, skipping... ]
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian escribió: > > Your patch has been added to the PostgreSQL unapplied patches list at: > > http://momjian.postgresql.org/cgi-bin/pgpatches > > It will be applied as soon as one of the PostgreSQL committers reviews > and approves it. I noticed that this patch uses names for some things (for example it gets the name of the access method), and then builds a IndexStmt which contains the name. I don't think this is a good idea. I think what should happen here is that the function to build indexes should be split in two: one to resolve the names and fill a structure with Oids of things, and another to get that structure and actually build the index or constraint. For example look into how GrantStmt is turned into InternalGrant, and the stuff in aclchk.c to work with that. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Hi,
The index creation happens after the new table (which is LIKE the parent) has been created, by appending the cxt.alist information to "extras_after". The entry point for the index creation is thus via ProcessUtility which expects an IndexStmt structure. That is why the current patch does all the Oid to name mapping exercise to populate the relevant fields in IndexStmt some of which are char pointers. The internal DefineIndex function also expects most of the fields to be in IndexStmt like format.
If we want to follow the above suggestion, as I understand it, we might have to devise a new structure to contain Oids and make ProcessUtility accept a new nodeTag. We will also not be able to use existing Index definition functions and this will lead to more coding IMHO. Do we want to go down this path? Or is there something else that has been suggested above and that I am missing completely?
Please let me know.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
I noticed that this patch uses names for some things (for example it
gets the name of the access method), and then builds a IndexStmt which
contains the name. I don't think this is a good idea. I think what
should happen here is that the function to build indexes should be split
in two: one to resolve the names and fill a structure with Oids of
things, and another to get that structure and actually build the index
or constraint. For example look into how GrantStmt is turned into
InternalGrant, and the stuff in aclchk.c to work with that.
If we want to follow the above suggestion, as I understand it, we might have to devise a new structure to contain Oids and make ProcessUtility accept a new nodeTag. We will also not be able to use existing Index definition functions and this will lead to more coding IMHO. Do we want to go down this path? Or is there something else that has been suggested above and that I am missing completely?
Please let me know.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Hi,
OTOH, we can populate a new structure with the relevant Oids, IndexInfo information from parent relation indexes and call index_create directly from within ProcessUtility. Guess, it should be cleaner than the current approach.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
The index creation happens after the new table (which is LIKE the parent) has been created, by appending the cxt.alist information to "extras_after". The entry point for the index creation is thus via ProcessUtility which expects an IndexStmt structure. That is why the current patch does all the Oid to name mapping exercise to populate the relevant fields in IndexStmt some of which are char pointers. The internal DefineIndex function also expects most of the fields to be in IndexStmt like format.
If we want to follow the above suggestion, as I understand it, we might have to devise a new structure to contain Oids and make ProcessUtility accept a new nodeTag. We will also not be able to use existing Index definition functions and this will lead to more coding IMHO. Do we want to go down this path? Or is there something else that has been suggested above and that I am missing completely?
OTOH, we can populate a new structure with the relevant Oids, IndexInfo information from parent relation indexes and call index_create directly from within ProcessUtility. Guess, it should be cleaner than the current approach.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Hi,
If so, I think we can introduce 2 Oid fields in the IndexStmt structure and store the Oids there. In DefineIndex we can use these Oids if they are not invalid.
IMHO, all this is less work and the bulk of the changes remain localized in mostly one or two functions as in the current patch.
Comments?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
On 5/23/07, NikhilS <nikkhils@gmail.com> wrote:
Sorry for the barrage of emails. But as I looked closely at the current patch there are only 2 fields (accessMethod and tableSpace) in IndexStmt structure that we populate by doing the conversion from OIDs to name. For the other fields, the current transformations will remain. Hi,The index creation happens after the new table (which is LIKE the parent) has been created, by appending the cxt.alist information to "extras_after". The entry point for the index creation is thus via ProcessUtility which expects an IndexStmt structure. That is why the current patch does all the Oid to name mapping exercise to populate the relevant fields in IndexStmt some of which are char pointers. The internal DefineIndex function also expects most of the fields to be in IndexStmt like format.
If we want to follow the above suggestion, as I understand it, we might have to devise a new structure to contain Oids and make ProcessUtility accept a new nodeTag. We will also not be able to use existing Index definition functions and this will lead to more coding IMHO. Do we want to go down this path? Or is there something else that has been suggested above and that I am missing completely?
OTOH, we can populate a new structure with the relevant Oids, IndexInfo information from parent relation indexes and call index_create directly from within ProcessUtility. Guess, it should be cleaner than the current approach.
If so, I think we can introduce 2 Oid fields in the IndexStmt structure and store the Oids there. In DefineIndex we can use these Oids if they are not invalid.
IMHO, all this is less work and the bulk of the changes remain localized in mostly one or two functions as in the current patch.
Comments?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS escribió: > Sorry for the barrage of emails. But as I looked closely at the current > patch there are only 2 fields (accessMethod and tableSpace) in IndexStmt > structure that we populate by doing the conversion from OIDs to name. For > the other fields, the current transformations will remain. > > If so, I think we can introduce 2 Oid fields in the IndexStmt structure and > store the Oids there. In DefineIndex we can use these Oids if they are not > invalid. Sounds reasonable. This is what we do for example in VacuumStmt. Make sure that the OIDs are set to Invalid in the parser. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
NikhilS <nikkhils@gmail.com> writes:
> If so, I think we can introduce 2 Oid fields in the IndexStmt structure and
> store the Oids there. In DefineIndex we can use these Oids if they are not
> invalid.
I think this is just make-work that causes the patch to complicate parts
of the system it didn't need to touch. The original suggestion was to
actively refactor existing code, which might or might not have been
worthwhile. But this isn't an appropriate substitute --- it's just
making the API uglier for no particular benefit.
regards, tom lane
Hi,
Considering that we will be generating this information from existing valid index information, I think converting the Oids to names is safe enough. Alvaro, do you think we should stick to the existing patch mechanism then considering that it avoids polluting the API?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
On 5/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I agree this will unnecessary add arguments to the DefineIndex API. If we stick to the patch's earlier way of converting the Oid to names for just these 2 arguments, we can avoid this IMO. NikhilS <nikkhils@gmail.com> writes:
> If so, I think we can introduce 2 Oid fields in the IndexStmt structure and
> store the Oids there. In DefineIndex we can use these Oids if they are not
> invalid.
I think this is just make-work that causes the patch to complicate parts
of the system it didn't need to touch. The original suggestion was to
actively refactor existing code, which might or might not have been
worthwhile. But this isn't an appropriate substitute --- it's just
making the API uglier for no particular benefit.
Considering that we will be generating this information from existing valid index information, I think converting the Oids to names is safe enough. Alvaro, do you think we should stick to the existing patch mechanism then considering that it avoids polluting the API?
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
NikhilS escribió: > On 5/23/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > >NikhilS <nikkhils@gmail.com> writes: > >> If so, I think we can introduce 2 Oid fields in the IndexStmt > >> structure and store the Oids there. In DefineIndex we can use these > >> Oids if they are not invalid. > > > >I think this is just make-work that causes the patch to complicate parts > >of the system it didn't need to touch. The original suggestion was to > >actively refactor existing code, which might or might not have been > >worthwhile. But this isn't an appropriate substitute --- it's just > >making the API uglier for no particular benefit. > > I agree this will unnecessary add arguments to the DefineIndex API. If we > stick to the patch's earlier way of converting the Oid to names for just > these 2 arguments, we can avoid this IMO. > > Considering that we will be generating this information from existing valid > index information, I think converting the Oids to names is safe enough. > Alvaro, do you think we should stick to the existing patch mechanism then > considering that it avoids polluting the API? Not sure. Is it possible that the schema is renamed while the operation is being executed? If it's not then this not a problem at all so the existing patch is fine. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Not sure. Is it possible that the schema is renamed while the operation
> is being executed? If it's not then this not a problem at all so the
> existing patch is fine.
There are hazards of that type in CREATE TABLE right now; it's hardly
fair to hold LIKE INCLUDING INDEXES to a higher standard.
regards, tom lane
Hi,
I doubt if accessMethod name will change. The tableSpace name can change, but the possibility is no worse to doing a [CREATE TABLE table_name ... TABLESPACE tablespace]. So this should be reasonably ok.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
> I agree this will unnecessary add arguments to the DefineIndex API. If we
> stick to the patch's earlier way of converting the Oid to names for just
> these 2 arguments, we can avoid this IMO.
>
> Considering that we will be generating this information from existing valid
> index information, I think converting the Oids to names is safe enough.
> Alvaro, do you think we should stick to the existing patch mechanism then
> considering that it avoids polluting the API?
Not sure. Is it possible that the schema is renamed while the operation
is being executed? If it's not then this not a problem at all so the
existing patch is fine.
I doubt if accessMethod name will change. The tableSpace name can change, but the possibility is no worse to doing a [CREATE TABLE table_name ... TABLESPACE tablespace]. So this should be reasonably ok.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
OK, so the patch is ready to be applied? --------------------------------------------------------------------------- NikhilS wrote: > Hi, > > > I agree this will unnecessary add arguments to the DefineIndex API. If we > > > stick to the patch's earlier way of converting the Oid to names for just > > > these 2 arguments, we can avoid this IMO. > > > > > > Considering that we will be generating this information from existing > > valid > > > index information, I think converting the Oids to names is safe enough. > > > Alvaro, do you think we should stick to the existing patch mechanism > > then > > > considering that it avoids polluting the API? > > > > Not sure. Is it possible that the schema is renamed while the operation > > is being executed? If it's not then this not a problem at all so the > > existing patch is fine. > > > I doubt if accessMethod name will change. The tableSpace name can change, > but the possibility is no worse to doing a [CREATE TABLE table_name ... > TABLESPACE tablespace]. So this should be reasonably ok. > > Regards, > Nikhils > -- > EnterpriseDB http://www.enterprisedb.com -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> OK, so the patch is ready to be applied?
Neil's still reviewing it, last I heard.
regards, tom lane
On Mon, 2007-21-05 at 12:23 +0530, NikhilS wrote:
> I had spent some time on this earlier so decided to complete and send
> the patch to you for review. This patch supports copying of
> expressions, predicates, opclass, amorder, reloptions etc. The test
> case also contains some more additions with this patch. Please let me
> know if there are any issues.
Attached is a revised version of this patch. Note that this pattern is
always unsafe:
ht_am = SearchSysCache(AMOID, ...);
if (!HeapTupleIsValid(ht_am))
elog(ERROR, "...");
amrec = (Form_pg_am) GETSTRUCT(ht_am);
index->accessMethod = NameStr(amrec->amname);
/* ... */
ReleaseSysCache(ht_am);
return index;
Before calling ReleaseSysCache(), all the data you need from the
syscache entry needs to be deep-copied to allow subsequent access, but
NameStr() doesn't do a deep-copy. Adding "-DFORCE_CATCACHE_RELEASE" is a
useful way to catch these kinds of problems (I wonder if this is worth
adding to the default CFLAGS when assertions are enabled?)
I also made a bunch of editorial changes, including moving the
varattnos_map_schema() call out of the loop in transformInhRelation().
BTW, comments like "This function is based on code from ruleutils.c"
would be helpful for reviewers (whether in the patch itself or just in
the email containing the patch).
There's still a few things I need to fix in the patch, but I'll apply a
revised version of the attached patch to HEAD tomorrow, barring any
objections.
-Neil
Attachment
Neil Conway <neilc@samurai.com> writes:
> Attached is a revised version of this patch.
This still seems to fundamentally misunderstand the difference between
an index and a constraint. IMHO it should not be examining pg_index
(or specifically, the index Relations) at all.
regards, tom lane
Hi,
On 6/3/07, Neil Conway <neilc@samurai.com> wrote:
On Mon, 2007-21-05 at 12:23 +0530, NikhilS wrote:
> I had spent some time on this earlier so decided to complete and send
> the patch to you for review. This patch supports copying of
> expressions, predicates, opclass, amorder, reloptions etc. The test
> case also contains some more additions with this patch. Please let me
> know if there are any issues.
Attached is a revised version of this patch. Note that this pattern is
always unsafe:
ht_am = SearchSysCache(AMOID, ...);
if (!HeapTupleIsValid(ht_am))
elog(ERROR, "...");
amrec = (Form_pg_am) GETSTRUCT(ht_am);
index->accessMethod = NameStr(amrec->amname);
/* ... */
ReleaseSysCache(ht_am);
return index;
Before calling ReleaseSysCache(), all the data you need from the
syscache entry needs to be deep-copied to allow subsequent access, but
NameStr() doesn't do a deep-copy. Adding "-DFORCE_CATCACHE_RELEASE" is a
useful way to catch these kinds of problems (I wonder if this is worth
adding to the default CFLAGS when assertions are enabled?)
I should have delved deep into the NameStr functionality myself for this. I too agree that adding the flag makes sense when assertions are enabled.
I also made a bunch of editorial changes, including moving the
varattnos_map_schema() call out of the loop in transformInhRelation().
BTW, comments like "This function is based on code from ruleutils.c"
would be helpful for reviewers (whether in the patch itself or just in
the email containing the patch).
Yes I had this in mind but somehow forgot to mention this when I posted the patch. I had done some changes in ruleutils.c specificly to take cognizance of this fact.
Regards,
Hi,
On 6/3/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Neil Conway <neilc@samurai.com> writes:
> Attached is a revised version of this patch.
This still seems to fundamentally misunderstand the difference between
an index and a constraint. IMHO it should not be examining pg_index
(or specifically, the index Relations) at all.
But as you had mentioned earlier, if we look at index entries as part of the implementation of "unique" or "primary key" pg_constraint entries, then examining pg_index is required, right?
Anyways, this patch and the functionality introduced herein will be useful in the "CREATE .. INCLUDING INDEXES" case too.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
EnterpriseDB http://www.enterprisedb.com
NikhilS <nikkhils@gmail.com> writes:
> On 6/3/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This still seems to fundamentally misunderstand the difference between
>> an index and a constraint. IMHO it should not be examining pg_index
>> (or specifically, the index Relations) at all.
> Anyways, this patch and the functionality introduced herein will be useful
> in the "CREATE .. INCLUDING INDEXES" case too.
No doubt. But those are different features and we shouldn't confuse
'em; in particular not put behavior into the INCLUDING CONSTRAINTS case
that shouldn't be there.
regards, tom lane
Hi,
> Anyways, this patch and the functionality introduced herein will be useful
> in the "CREATE .. INCLUDING INDEXES" case too.
No doubt. But those are different features and we shouldn't confuse
'em; in particular not put behavior into the INCLUDING CONSTRAINTS case
that shouldn't be there.
PFA, a patch which provides the "CREATE .. INCLUDING INDEXES" functionality. This patch uses the same functionality introduced by the earlier patches in this thread albeit for the "INCLUDING INDEXES" case.
For ease of review, this patch is based on the latest version which was posted by Neil.
Please let me know if there are any issues.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Attachment
On Tue, 2007-05-06 at 14:50 +0530, NikhilS wrote: > PFA, a patch which provides the "CREATE .. INCLUDING INDEXES" > functionality. This patch uses the same functionality introduced by > the earlier patches in this thread albeit for the "INCLUDING INDEXES" > case. Attached is a revised version of this patch. Sorry for taking so long to make progress on this: my new job been keeping my busy, and I've recently been ill. This version updates the patch to CVS HEAD and has various fixes and refactoring, including proper docs. I refactored get_opclass_name() into lsyscache.c, but then realized that this means that lsyscache.c will depend on commands/indexcmds.c (for GetDefaultOpClass()), which is arguably improper, so I'm tempted to revert and just duplicate the syscache lookups in both ruleutils.c and parse_utilcmd.c Nikhil: why are both "options" and "inhreloptions" necessary in IndexStmt? Won't at least one be NULL? BTW, I notice that include/defrem.h contains declarations for several different, marginally-related .c files (indexcmds.c, functioncmds.c, operatorcmds.c, aggregatecmds.c, opclasscmds.c, define.c). I'm inclined to separate these declarations into separate header files; any objections to doing that? -Neil
Attachment
Hi,
Yes, I too would vote for duplicating the lookups for the sake of simplicity.
Yes, in the CREATE..LIKE case, options will be NULL and in the normal CREATE..INDEX case inhreloptions will be NULL. Note that options is a List of DefElem entries, whereas inhreloptions is a char pointer.
The challenge was with converting the stored reloptions belonging to the parent index into some form which could be consumed by transformRelOptions(). It was difficult to convert it into a list of DefElem entries and hence I had to introduce inhreloptions.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
This version updates the patch to CVS HEAD and has various fixes and
refactoring, including proper docs. I refactored get_opclass_name() into
lsyscache.c, but then realized that this means that lsyscache.c will
depend on commands/indexcmds.c (for GetDefaultOpClass()), which is
arguably improper, so I'm tempted to revert and just duplicate the
syscache lookups in both ruleutils.c and parse_utilcmd.c
Yes, I too would vote for duplicating the lookups for the sake of simplicity.
Nikhil: why are both "options" and "inhreloptions" necessary in
IndexStmt? Won't at least one be NULL?
Yes, in the CREATE..LIKE case, options will be NULL and in the normal CREATE..INDEX case inhreloptions will be NULL. Note that options is a List of DefElem entries, whereas inhreloptions is a char pointer.
The challenge was with converting the stored reloptions belonging to the parent index into some form which could be consumed by transformRelOptions(). It was difficult to convert it into a list of DefElem entries and hence I had to introduce inhreloptions.
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
Neil Conway wrote: > On Tue, 2007-05-06 at 14:50 +0530, NikhilS wrote: > > PFA, a patch which provides the "CREATE .. INCLUDING INDEXES" > > functionality. This patch uses the same functionality introduced by > > the earlier patches in this thread albeit for the "INCLUDING INDEXES" > > case. > > Attached is a revised version of this patch. Sorry for taking so long to > make progress on this: my new job been keeping my busy, and I've > recently been ill. Illness only counts as an excuse if you _don't_ recover. If you recover, you weren't sick enough. ;-) LOL -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On 7/10/07, Bruce Momjian <bruce@momjian.us> wrote:
> Neil Conway wrote:
> > Attached is a revised version of this patch. Sorry for taking so long to
> > make progress on this: my new job been keeping my busy, and I've
> > recently been ill.
>
> Illness only counts as an excuse if you _don't_ recover. If you
> recover, you weren't sick enough. ;-) LOL
>
uh! that sounds like my boss talking!
--
regards,
Jaime Casanova
"Programming today is a race between software engineers striving to
build bigger and better idiot-proof programs and the universe trying
to produce bigger and better idiots.
So far, the universe is winning."
Richard Cook
On Tue, 2007-10-07 at 12:53 +0530, NikhilS wrote:
> Yes, in the CREATE..LIKE case, options will be NULL and in the normal
> CREATE..INDEX case inhreloptions will be NULL. Note that options is a
> List of DefElem entries, whereas inhreloptions is a char pointer.
Hmm, right -- ugly. I'll just stick with your approach.
BTW, I notice a problem with the patch as implemented:
# create table abc (a int, b int);
CREATE TABLE
# create index abc_a_idx on abc using hash (a);
CREATE INDEX
# create index abc_a_idx2 on abc (a);
CREATE INDEX
# create table abc2 (like abc including indexes);
CREATE TABLE
# \d abc2
Table "public.abc2"
Column | Type | Modifiers
--------+---------+-----------
a | integer |
b | integer |
Indexes:
"abc2_a_key" hash (a)
This occurs because transformIndexConstraints() eliminates "duplicate"
indexes from the index list by looking for two indexes with equal()
column lists. This makes some sense for the normal CREATE TABLE case,
but the above behavior is certainly objectionable -- when the access
method differs it is merely surprising, but when partial indexes are
involved it is surely a bug.
One way to fix this would be to check for duplicates by comparing all
the fields of the two IndexStmts, *except* the index name and "is PK?"
status. But that's ugly -- it seems much cleaner to keep index
definitions arising from LIKE ... INCLUDING INDEXES in a separate list
from the indexes derived from constraints.
Attached is a revised patch that does that. Barring any objections, I'll
apply this to HEAD tomorrow.
-Neil
Attachment
Hi,
> Attached is a revised patch that does that. Barring any objections, I'll
> apply this to HEAD tomorrow.
>
A minor correction below in your patch:
--- src/include/commands/defrem.h 16 Jul 2007 05:19:43 -0000
*************** extern void DefineIndex(RangeVar *heapRe
*** 26,31 ****
--- 26,32 ----
List *options,
+ char *inhreloptions,
Guess you want to use "src_options" here to be uniform with usages
elsewhere that you have replaced. You suppose "src_options" is much
more readable than "inhreloptions" or "inh_idxoptions"? Your call :).
Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com
On Mon, 2007-16-07 at 14:28 +0530, NikhilS wrote: > Guess you want to use "src_options" here to be uniform with usages > elsewhere that you have replaced. Thanks, good catch. > You suppose "src_options" is much more readable than "inhreloptions" > or "inh_idxoptions"? Your call :). Yeah, I'm not necessarily sure that it is. "inhreloptions" made me think of table inheritance, whereas LIKE in general is more of a "source" => "target" copying operation. But I'm not convinced about src_options, either ... suggestions/comments welcome. -Neil
Hi, > > You suppose "src_options" is much more readable than "inhreloptions" > > or "inh_idxoptions"? Your call :). > > Yeah, I'm not necessarily sure that it is. "inhreloptions" made me think > of table inheritance, whereas LIKE in general is more of a "source" => > "target" copying operation. But I'm not convinced about src_options, > either ... suggestions/comments welcome. > I agree, since its a LIKE operation and not inheritance as such, how about "src_idxoptions", just to make the reference to the source index clearer? To reiterate its a minor nitpick really from my side and can be ignored too. Regards, Nikhils -- EnterpriseDB http://www.enterprisedb.com
I've applied the latest version of the patch to HEAD. Thanks for the patches, Nikhil and Trevor -- we can take a look at improving INCLUDING CONSTRAINTS for 8.4. On Mon, 2007-16-07 at 15:50 +0530, NikhilS wrote: > I agree, since its a LIKE operation and not inheritance as such, how about > "src_idxoptions", just to make the reference to the source index > clearer? I decided to keep it as "src_options", but we can always change it later. -Neil
Can someone enligten me what the usecase for CREATE TABLE LIKE at this moment is? I read the doc and followed the discssion in this thread in pgsql-patches list but it was hard for me to figure out. -- Tatsuo Ishii SRA OSS, Inc. Japan > I've applied the latest version of the patch to HEAD. Thanks for the > patches, Nikhil and Trevor -- we can take a look at improving INCLUDING > CONSTRAINTS for 8.4. > > On Mon, 2007-16-07 at 15:50 +0530, NikhilS wrote: > > I agree, since its a LIKE operation and not inheritance as such, how about > > "src_idxoptions", just to make the reference to the source index > > clearer? > > I decided to keep it as "src_options", but we can always change it > later. > > -Neil > > > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Have you searched our list archives? > > http://archives.postgresql.org
"Tatsuo Ishii" <ishii@sraoss.co.jp> writes: > Can someone enligten me what the usecase for CREATE TABLE LIKE at this > moment is? One of the main use cases I envision is wanting to create new partitions suitable for being added to a partitioned table. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Fri, Jul 20, 2007 at 12:40:46PM +0100, Gregory Stark wrote: > "Tatsuo Ishii" <ishii@sraoss.co.jp> writes: > > > Can someone enligten me what the usecase for CREATE TABLE LIKE at this > > moment is? > > One of the main use cases I envision is wanting to create new partitions > suitable for being added to a partitioned table. Except that's normally done with CREATE TABLE INHERITS, which seems cleaner to me. I think the patch would be more useful if it added support for INHERITS, but I don't object to it as-is. -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
Attachment
"Jim C. Nasby" <decibel@decibel.org> writes: > On Fri, Jul 20, 2007 at 12:40:46PM +0100, Gregory Stark wrote: > >> One of the main use cases I envision is wanting to create new partitions >> suitable for being added to a partitioned table. > > Except that's normally done with CREATE TABLE INHERITS, which seems > cleaner to me. I think the patch would be more useful if it added > support for INHERITS, but I don't object to it as-is. Well there are two different approaches to creating a new partition. For some use cases you want to create a new empty partition directly in the table and start using it right away. For other use cases you want to create a table into which you need to load data, possibly even massage the data with updates and deletes or additional inserts, then add the partition with the data all ready directly into the master table. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com