Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Thu, Jul 23, 2015 at 9:06 AM, Michael Paquier wrote: > Yeah, I think we should be able to define a collation in this case. > For example it is as well possible to pass a WITH clause with storage > parameters, though we do not document it in > table_constraint_using_index > (http://www.postgresql.org/docs/devel/static/sql-altertable.html). Mistake here, please ignore this remark. USING INDEX does not accept WITH storage_parameter but ADD CONSTRAINT without an index does and it is documented in CREATE TABLE. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Thu, Jul 23, 2015 at 5:47 AM, Gurjeet Singh wrote: > On Wed, Jul 22, 2015 at 7:34 AM, Robert Haas wrote: >> >> On Tue, Jul 21, 2015 at 8:34 PM, Michael Paquier >> wrote: >> > On Wed, Jul 22, 2015 at 1:23 AM, Robert Haas >> > wrote: >> >> Notice that the collation specifier is gone. Oops. >> > >> > As it is not possible to specify directly a constraint for a PRIMARY >> > KEY expression, what about switching dumpConstraint to have it use >> > first a CREATE INDEX query with the collation and then use ALTER TABLE >> > to attach the constraint to it? I am noticing that we already fetch >> > the index definition in indxinfo via pg_get_indexdef. Thoughts? >> >> I guess the questions on my mind is "Why is it that you can't do this >> directly when creating a primary key, but you can do it when turning >> an existing index into a primary key?" Sure. This does not seem to be complicated. >> If there's a good reason for prohibiting doing this when creating a >> primary key, then presumably it shouldn't be allowed when turning an >> index into a primary key either. If there's not, then why not extend >> the PRIMARY KEY syntax to allow it? Yeah, I think we should be able to define a collation in this case. For example it is as well possible to pass a WITH clause with storage parameters, though we do not document it in table_constraint_using_index (http://www.postgresql.org/docs/devel/static/sql-altertable.html). > +1. I think in the short term we can treat this as a bug, and "fix" the bug > by disallowing an index with attributes that cannot be present in an index > created by PRIMARY KEY constraint. The collation attribute on one of the > keys may be just one of many such attributes. Er, pushing such a patch on back-branches may break applications that do exactly what Robert did in his test case (using a unique index as primary key with a collation), no? I am not sure that this is acceptable. Taking the problem at its root by swtiching pg_dump to define an index and then use it looks a more solid approach on back branches. > In the long term, we may want to allow collation in primary key, but that > will be a feature ideally suited for a major version release. Yep. Definitely. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Tue, Jul 21, 2015 at 9:23 AM, Robert Haas wrote: > rhaas=# create unique index on foo (a collate "C"); > CREATE INDEX > rhaas=# alter table foo add primary key using index foo_a_idx; > ALTER TABLE > > Now dump and restore this database. Then: > > Notice that the collation specifier is gone. Oops. > The intent of the 'USING INDEX' feature was to work around the problem that PRIMARY KEY indexes cannot be reindexed concurrently to reduce bloat. Considering that the feature doesn't work [1] (at least as shown in example in the docs [2]) if there are any foreign keys referencing the table, there's scope for improvement. There was proposal to support reindexing the primary key index, but I am not sure where that stands. If that's already in, or soon to be in core, we may put a deprecation notice around USING INDEX. OTOH, if reindexing PKeys is too difficult, we may want to support index-replacement via a top-level ALTER TABLE subcommand that works in CONCURRENT mode, and not recommend the method shown in the docs (i.e deprecate the USING INDEX syntax). [1]: The DROP CONSTRAINT part of the command fails if there are any FKeys pointing to this table. [2]: http://www.postgresql.org/docs/9.4/static/sql-altertable.html -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Wed, Jul 22, 2015 at 7:34 AM, Robert Haas wrote: > On Tue, Jul 21, 2015 at 8:34 PM, Michael Paquier > wrote: > > On Wed, Jul 22, 2015 at 1:23 AM, Robert Haas > wrote: > >> Notice that the collation specifier is gone. Oops. > > > > As it is not possible to specify directly a constraint for a PRIMARY > > KEY expression, what about switching dumpConstraint to have it use > > first a CREATE INDEX query with the collation and then use ALTER TABLE > > to attach the constraint to it? I am noticing that we already fetch > > the index definition in indxinfo via pg_get_indexdef. Thoughts? > > I guess the questions on my mind is "Why is it that you can't do this > directly when creating a primary key, but you can do it when turning > an existing index into a primary key?" > > If there's a good reason for prohibiting doing this when creating a > primary key, then presumably it shouldn't be allowed when turning an > index into a primary key either. If there's not, then why not extend > the PRIMARY KEY syntax to allow it? > +1. I think in the short term we can treat this as a bug, and "fix" the bug by disallowing an index with attributes that cannot be present in an index created by PRIMARY KEY constraint. The collation attribute on one of the keys may be just one of many such attributes. In the long term, we may want to allow collation in primary key, but that will be a feature ideally suited for a major version release. Best regards, -- Gurjeet Singh http://gurjeet.singh.im/
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Tue, Jul 21, 2015 at 8:34 PM, Michael Paquier wrote: > On Wed, Jul 22, 2015 at 1:23 AM, Robert Haas wrote: >> Notice that the collation specifier is gone. Oops. > > As it is not possible to specify directly a constraint for a PRIMARY > KEY expression, what about switching dumpConstraint to have it use > first a CREATE INDEX query with the collation and then use ALTER TABLE > to attach the constraint to it? I am noticing that we already fetch > the index definition in indxinfo via pg_get_indexdef. Thoughts? I guess the questions on my mind is "Why is it that you can't do this directly when creating a primary key, but you can do it when turning an existing index into a primary key?" If there's a good reason for prohibiting doing this when creating a primary key, then presumably it shouldn't be allowed when turning an index into a primary key either. If there's not, then why not extend the PRIMARY KEY syntax to allow it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Wed, Jul 22, 2015 at 9:34 AM, Michael Paquier
wrote:
> On Wed, Jul 22, 2015 at 1:23 AM, Robert Haas wrote:
>> Notice that the collation specifier is gone. Oops.
>
> As it is not possible to specify directly a constraint for a PRIMARY
> KEY expression, what about switching dumpConstraint to have it use
> first a CREATE INDEX query with the collation and then use ALTER TABLE
> to attach the constraint to it? I am noticing that we already fetch
> the index definition in indxinfo via pg_get_indexdef. Thoughts?
And poking at that I have finished with the attached that adds a
CREATE INDEX query before ALTER TABLE ADD CONSTRAINT, to which a USING
INDEX is appended. Storage options as well as building the column list
becomes unnecessary because indexdef already provides everything what
is needed, so this patch makes dump rely more on what is on
backend-side.
--
Michael
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 0e036b8..7a1e6db 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -14515,7 +14515,6 @@ dumpConstraint(Archive *fout, DumpOptions *dopt, ConstraintInfo *coninfo)
{
/* Index-related constraint */
IndxInfo *indxinfo;
- int k;
indxinfo = (IndxInfo *) findObjectByDumpId(coninfo->conindex);
@@ -14527,6 +14526,10 @@ dumpConstraint(Archive *fout, DumpOptions *dopt, ConstraintInfo *coninfo)
binary_upgrade_set_pg_class_oids(fout, q,
indxinfo->dobj.catId.oid, true);
+ if (coninfo->contype == 'p' ||
+ coninfo->contype == 'u')
+ appendPQExpBuffer(q, "%s;\n", indxinfo->indexdef);
+
appendPQExpBuffer(q, "ALTER TABLE ONLY %s\n",
fmtId(tbinfo->dobj.name));
appendPQExpBuffer(q, "ADD CONSTRAINT %s ",
@@ -14534,31 +14537,21 @@ dumpConstraint(Archive *fout, DumpOptions *dopt, ConstraintInfo *coninfo)
if (coninfo->condef)
{
+ /*
+ * getIndexes sets a constraint definition only for exclusion
+ * constraints.
+ */
+ Assert(coninfo->contype == 'x');
+
/* pg_get_constraintdef should have provided everything */
appendPQExpBuffer(q, "%s;\n", coninfo->condef);
}
else
{
- appendPQExpBuffer(q, "%s (",
+ appendPQExpBuffer(q, "%s ",
coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
- for (k = 0; k < indxinfo->indnkeys; k++)
- {
-int indkey = (int) indxinfo->indkeys[k];
-const char *attname;
-
-if (indkey == InvalidAttrNumber)
- break;
-attname = getAttrName(indkey, tbinfo);
-
-appendPQExpBuffer(q, "%s%s",
- (k == 0) ? "" : ", ",
- fmtId(attname));
- }
-
- appendPQExpBufferChar(q, ')');
- if (indxinfo->options && strlen(indxinfo->options) > 0)
-appendPQExpBuffer(q, " WITH (%s)", indxinfo->options);
+ appendPQExpBuffer(q, "USING INDEX %s", indxinfo->dobj.name);
if (coninfo->condeferrable)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
On Wed, Jul 22, 2015 at 1:23 AM, Robert Haas wrote: > Notice that the collation specifier is gone. Oops. As it is not possible to specify directly a constraint for a PRIMARY KEY expression, what about switching dumpConstraint to have it use first a CREATE INDEX query with the collation and then use ALTER TABLE to attach the constraint to it? I am noticing that we already fetch the index definition in indxinfo via pg_get_indexdef. Thoughts? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ALTER TABLE .. ADD PRIMARY KEY .. USING INDEX has dump-restore hazard
Consider: rhaas=# create table foo (a text); CREATE TABLE rhaas=# create unique index on foo (a collate "C"); CREATE INDEX rhaas=# alter table foo add primary key using index foo_a_idx; ALTER TABLE rhaas=# \d foo Table "public.foo" Column | Type | Modifiers +--+--- a | text | not null Indexes: "foo_a_idx" PRIMARY KEY, btree (a COLLATE "C") Now dump and restore this database. Then: rhaas=# \d foo Table "public.foo" Column | Type | Modifiers +--+--- a | text | not null Indexes: "foo_a_idx" PRIMARY KEY, btree (a) Notice that the collation specifier is gone. Oops. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
