pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
22+ messages / 7 participants
[nested] [flat]
Commits from this thread:
2023-02-24 d959523257 Disallow NULLS NOT DISTINCT indexes for primary keys
* BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 09:07 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 22+ messages in thread
From: PG Bug reporting form @ 2022-12-14 09:07 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: zedaardv@drizzle.com
The following bug has been logged on the website:
Bug reference: 17720
Logged by: reiner peterke
Email address: zedaardv@drizzle.com
PostgreSQL version: 15.1
Operating system: openSUSE Leap 15.4
Description:
I have a table
create table hamster(under integer, over text);
I create a unique index on the under column with nulls not distinct
create unique index uq_not_distinct on hamster (under) nulls not distinct;
i now create a primary key using the unique index
alter table hamster add constraint pk_hamster primary key using index
uq_not_distinct;
NOTICE: ALTER TABLE / ADD CONSTRAINT USING INDEX will rename index
"uq_not_distinct" to "pk_hamster"
table looks good
\d hamster
Table "moon.hamster"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
under | integer | | not null |
over | text | | |
Indexes:
"pk_hamster" PRIMARY KEY, btree (under) NULLS NOT DISTINCT
Do a pg_dump of the database.
the dump creates the code for a primary key that cannot be restored
pg_dump -p 5632 -Of tranquility.sql -d tranquility
on restore, I get the following error
psql:tranquility.sql:90: ERROR: syntax error at or near "NULLS"
LINE 2: ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT...
in the dump itself the create constraint command is
ALTER TABLE ONLY moon.hamster
ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT (under);
which does not work with the NULLS NOT DISTINCT string
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 12:54 David G. Johnston <david.g.johnston@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 22+ messages in thread
From: David G. Johnston @ 2022-12-14 12:54 UTC (permalink / raw)
To: zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Wednesday, December 14, 2022, PG Bug reporting form <
noreply@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 17720
> Logged by: reiner peterke
> Email address: zedaardv@drizzle.com
> PostgreSQL version: 15.1
> Operating system: openSUSE Leap 15.4
> Description:
>
> Do a pg_dump of the database.
> the dump creates the code for a primary key that cannot be restored
> pg_dump -p 5632 -Of tranquility.sql -d tranquility
> on restore, I get the following error
> psql:tranquility.sql:90: ERROR: syntax error at or near "NULLS"
> LINE 2: ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT...
> in the dump itself the create constraint command is
> ALTER TABLE ONLY moon.hamster
> ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT (under);
> which does not work with the NULLS NOT DISTINCT string
>
There is a decent chance that the fix here is to prohibit doing what you
did here - a PK cannot contain nulls in any of its columns so indeed
choosing an index that specifies how nulls behave is non-sensical. That
said, it also doesn’t hurt so long as the column itself is indeed not
null. But extending the syntax doesn’t seem that appealing.
David J.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 13:02 Daniel Gustafsson <daniel@yesql.se>
parent: David G. Johnston <david.g.johnston@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-14 13:02 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 14 Dec 2022, at 13:54, David G. Johnston <david.g.johnston@gmail.com> wrote:
>
> On Wednesday, December 14, 2022, PG Bug reporting form <noreply@postgresql.org <mailto:noreply@postgresql.org>> wrote:
> The following bug has been logged on the website:
>
> Bug reference: 17720
> Logged by: reiner peterke
> Email address: zedaardv@drizzle.com <mailto:zedaardv@drizzle.com>
> PostgreSQL version: 15.1
> Operating system: openSUSE Leap 15.4
> Description:
>
> Do a pg_dump of the database.
> the dump creates the code for a primary key that cannot be restored
> pg_dump -p 5632 -Of tranquility.sql -d tranquility
> on restore, I get the following error
> psql:tranquility.sql:90: ERROR: syntax error at or near "NULLS"
> LINE 2: ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT...
> in the dump itself the create constraint command is
> ALTER TABLE ONLY moon.hamster
> ADD CONSTRAINT pk_hamster PRIMARY KEY NULLS NOT DISTINCT (under);
> which does not work with the NULLS NOT DISTINCT string
>
> There is a decent chance that the fix here is to prohibit doing what you did here - a PK cannot contain nulls in any of its columns so indeed choosing an index that specifies how nulls behave is non-sensical. That said, it also doesn’t hurt so long as the column itself is indeed not null. But extending the syntax doesn’t seem that appealing.
Even if we prohibit this, there is still the case of all existing systems which
can't be dumped. I wonder if the solution is to teach pg_dump to not create
NULLS NOT DISTINCT primary key constraints? The simple attached fix creates a
valid PK constraint on the above schema.
--
Daniel Gustafsson https://vmware.com/
Attachments:
pg_dump_pk_nulls.diff (application/octet-stream, 589B) download
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 44d957c038..2d767cb286 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16416,7 +16416,7 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
{
appendPQExpBufferStr(q,
coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
- if (indxinfo->indnullsnotdistinct)
+ if (indxinfo->indnullsnotdistinct && coninfo->contype != 'p')
appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
appendPQExpBufferStr(q, " (");
for (k = 0; k < indxinfo->indnkeyattrs; k++)
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 15:37 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 3 replies; 22+ messages in thread
From: Tom Lane @ 2022-12-14 15:37 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> On 14 Dec 2022, at 13:54, David G. Johnston <david.g.johnston@gmail.com> wrote:
>> There is a decent chance that the fix here is to prohibit doing what you did here - a PK cannot contain nulls in any of its columns so indeed choosing an index that specifies how nulls behave is non-sensical. That said, it also doesn’t hurt so long as the column itself is indeed not null. But extending the syntax doesn’t seem that appealing.
> Even if we prohibit this, there is still the case of all existing systems which
> can't be dumped. I wonder if the solution is to teach pg_dump to not create
> NULLS NOT DISTINCT primary key constraints? The simple attached fix creates a
> valid PK constraint on the above schema.
It doesn't make sense for pg_dump to editorialize on a schema that
we otherwise consider valid; people would rightfully complain that
dump/restore changed things. So we need to do both things: prohibit
adopting such an index as a PK constraint (but I guess it's okay
for plain unique constraints?), and adjust pg_dump to compensate
for the legacy case where it was already done.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 15:51 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-14 15:51 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 14 Dec 2022, at 16:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 14 Dec 2022, at 13:54, David G. Johnston <david.g.johnston@gmail.com> wrote:
>>> There is a decent chance that the fix here is to prohibit doing what you did here - a PK cannot contain nulls in any of its columns so indeed choosing an index that specifies how nulls behave is non-sensical. That said, it also doesn’t hurt so long as the column itself is indeed not null. But extending the syntax doesn’t seem that appealing.
>
>> Even if we prohibit this, there is still the case of all existing systems which
>> can't be dumped. I wonder if the solution is to teach pg_dump to not create
>> NULLS NOT DISTINCT primary key constraints? The simple attached fix creates a
>> valid PK constraint on the above schema.
>
> It doesn't make sense for pg_dump to editorialize on a schema that
> we otherwise consider valid; people would rightfully complain that
> dump/restore changed things. So we need to do both things: prohibit
> adopting such an index as a PK constraint (but I guess it's okay
> for plain unique constraints?), and adjust pg_dump to compensate
> for the legacy case where it was already done.
Agreed, I'll expand the patch.
--
Daniel Gustafsson https://vmware.com/
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 15:59 David G. Johnston <david.g.johnston@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 22+ messages in thread
From: David G. Johnston @ 2022-12-14 15:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Wed, Dec 14, 2022 at 8:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
> > On 14 Dec 2022, at 13:54, David G. Johnston <david.g.johnston@gmail.com>
> wrote:
> >> There is a decent chance that the fix here is to prohibit doing what
> you did here - a PK cannot contain nulls in any of its columns so indeed
> choosing an index that specifies how nulls behave is non-sensical. That
> said, it also doesn’t hurt so long as the column itself is indeed not
> null. But extending the syntax doesn’t seem that appealing.
>
> > Even if we prohibit this, there is still the case of all existing
> systems which
> > can't be dumped. I wonder if the solution is to teach pg_dump to not
> create
> > NULLS NOT DISTINCT primary key constraints? The simple attached fix
> creates a
> > valid PK constraint on the above schema.
>
> It doesn't make sense for pg_dump to editorialize on a schema that
> we otherwise consider valid; people would rightfully complain that
> dump/restore changed things. So we need to do both things: prohibit
> adopting such an index as a PK constraint (but I guess it's okay
> for plain unique constraints?), and adjust pg_dump to compensate
> for the legacy case where it was already done.
>
>
The WHERE clause of CREATE INDEX doesn't pose an issue as we report "Cannot
create a primary key or unique constraint using such an index".
It is also not possible to specify an opclass in PRIMARY KEY, but since we
document that unique indexes are only currently implemented by B-tree, and
that is what you get from PRIMARY KEY, that is also not a problem
(presently - unless extension authors were to bypass this).
The remaining elements: INCLUDE, WITH, and TABLESPACE, all exist in both
formulations.
I am thinking now that the failure to include NULLS [NOT[ DISTINCT in the
CREATE TABLE syntax is an oversight that needs to be fixed. It just
doesn't make sense to have the two commands expose different features.
David J.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 16:08 Tom Lane <tgl@sss.pgh.pa.us>
parent: David G. Johnston <david.g.johnston@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2022-12-14 16:08 UTC (permalink / raw)
To: David G. Johnston <david.g.johnston@gmail.com>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; Daniel Gustafsson <daniel@yesql.se>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I am thinking now that the failure to include NULLS [NOT] DISTINCT in the
> CREATE TABLE syntax is an oversight that needs to be fixed. It just
> doesn't make sense to have the two commands expose different features.
It looks to me like it was pretty intentional, because both CREATE
and ALTER TABLE let you write UNIQUE NULLS NOT DISTINCT but not
PRIMARY KEY NULLS NOT DISTINCT. That doesn't seem like an oversight.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 16:34 David G. Johnston <david.g.johnston@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 22+ messages in thread
From: David G. Johnston @ 2022-12-14 16:34 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; Daniel Gustafsson <daniel@yesql.se>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Wed, Dec 14, 2022 at 9:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > I am thinking now that the failure to include NULLS [NOT] DISTINCT in the
> > CREATE TABLE syntax is an oversight that needs to be fixed. It just
> > doesn't make sense to have the two commands expose different features.
>
> It looks to me like it was pretty intentional, because both CREATE
> and ALTER TABLE let you write UNIQUE NULLS NOT DISTINCT but not
> PRIMARY KEY NULLS NOT DISTINCT. That doesn't seem like an oversight.
>
OK, that was me tunnel-visioned on the "index_parameters" syntax section
where INCLUDE, etc... are listed and not seeing it there.
So, don't document that PRIMARY KEY accepts the NULLS [NOT] DISTINCT but
make it do so anyway?
David J.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 22:30 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 1 reply; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-14 22:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 14 Dec 2022, at 16:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 14 Dec 2022, at 13:54, David G. Johnston <david.g.johnston@gmail.com> wrote:
>>> There is a decent chance that the fix here is to prohibit doing what you did here - a PK cannot contain nulls in any of its columns so indeed choosing an index that specifies how nulls behave is non-sensical. That said, it also doesn’t hurt so long as the column itself is indeed not null. But extending the syntax doesn’t seem that appealing.
>
>> Even if we prohibit this, there is still the case of all existing systems which
>> can't be dumped. I wonder if the solution is to teach pg_dump to not create
>> NULLS NOT DISTINCT primary key constraints? The simple attached fix creates a
>> valid PK constraint on the above schema.
>
> It doesn't make sense for pg_dump to editorialize on a schema that
> we otherwise consider valid; people would rightfully complain that
> dump/restore changed things. So we need to do both things: prohibit
> adopting such an index as a PK constraint (but I guess it's okay
> for plain unique constraints?), and adjust pg_dump to compensate
> for the legacy case where it was already done.
The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
constraints but allow them for unique constraints. Is this along the lines of
what you had in mind?
--
Daniel Gustafsson https://vmware.com/
Attachments:
v1-0001-Disallow-NULLS-NOT-DISTINCT-indexes-for-primary-k.patch (application/octet-stream, 3.6K) download
From 838b82bb33a14b499e2e6c26d661363100f2a76f Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 14 Dec 2022 23:23:58 +0100
Subject: [PATCH v1] Disallow NULLS NOT DISTINCT indexes for primary keys
A unique index which is created with non-distinct NULLS cannot be
used for backing a primary key constraint. Make sure to disallow
such table alterations and teach pg_dump to drop the non-distinct
NULLS clause on indexes where this has been set.
Bug: 17720
Reported-by: Reiner Peterke <zedaardv@drizzle.com>
Discussion: https://postgr.es/m/17720-dab8ee0fa85d316d@postgresql.org
---
src/backend/catalog/index.c | 7 +++++++
src/bin/pg_dump/pg_dump.c | 2 +-
src/test/regress/expected/create_index.out | 6 ++++++
src/test/regress/sql/create_index.sql | 6 ++++++
4 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 61f1d3926a..6b889c582f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -225,6 +225,13 @@ index_check_primary_key(Relation heapRel,
RelationGetRelationName(heapRel))));
}
+ if ((is_alter_table && indexInfo->ii_NullsNotDistinct))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("primary keys cannot use NULLS NOT DISTINCT indexes")));
+ }
+
/*
* Check that all of the attributes in a primary key are marked as not
* null. (We don't really expect to see that; it'd mean the parser messed
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 44d957c038..2d767cb286 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16416,7 +16416,7 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
{
appendPQExpBufferStr(q,
coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
- if (indxinfo->indnullsnotdistinct)
+ if (indxinfo->indnullsnotdistinct && coninfo->contype != 'p')
appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
appendPQExpBufferStr(q, " (");
for (k = 0; k < indxinfo->indnkeyattrs; k++)
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6cd57e3eaa..acfd9d1f4f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1595,6 +1595,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+ERROR: primary keys cannot use NULLS NOT DISTINCT indexes
+DROP TABLE cwi_test;
--
-- Check handling of indexes on system columns
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index a3738833b2..d49ce9f300 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -617,6 +617,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+DROP TABLE cwi_test;
+
--
-- Check handling of indexes on system columns
--
--
2.32.1 (Apple Git-133)
=
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-14 23:27 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 22+ messages in thread
From: Tom Lane @ 2022-12-14 23:27 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
> constraints but allow them for unique constraints. Is this along the lines of
> what you had in mind?
Needs more than zero comments in the code, and why bother testing
is_alter_table in index_check_primary_key? We're disallowing
this case across-the-board, no matter how you get to it.
I'll defer to Peter on whether this is in fact the right way to go,
or we should relax the syntax restriction as David suggests.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 13:18 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-15 13:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 15 Dec 2022, at 00:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
>> constraints but allow them for unique constraints. Is this along the lines of
>> what you had in mind?
>
> Needs more than zero comments in the code, and why bother testing
> is_alter_table in index_check_primary_key? We're disallowing
> this case across-the-board, no matter how you get to it.
That was an, admittedly poor, attempt at self-documenting code. Removed and
comments added in the attached.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v2-0001-Disallow-NULLS-NOT-DISTINCT-indexes-for-primary-k.patch (application/octet-stream, 4.1K) download
From c218517c97d4cf8f7ba078c7f9a615eba79a2228 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Wed, 14 Dec 2022 23:23:58 +0100
Subject: [PATCH v2] Disallow NULLS NOT DISTINCT indexes for primary keys
A unique index which is created with non-distinct NULLS cannot be
used for backing a primary key constraint. Make sure to disallow
such table alterations and teach pg_dump to drop the non-distinct
NULLS clause on indexes where this has been set.
Bug: 17720
Reported-by: Reiner Peterke <zedaardv@drizzle.com>
Discussion: https://postgr.es/m/17720-dab8ee0fa85d316d@postgresql.org
---
src/backend/catalog/index.c | 13 +++++++++++++
src/bin/pg_dump/pg_dump.c | 7 ++++++-
src/test/regress/expected/create_index.out | 6 ++++++
src/test/regress/sql/create_index.sql | 6 ++++++
4 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 61f1d3926a..6ff769911e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -225,6 +225,19 @@ index_check_primary_key(Relation heapRel,
RelationGetRelationName(heapRel))));
}
+ /*
+ * Indexes created with NULLS NOT DISTINCT cannot be used for primary key
+ * constraints. While there is no direct syntax to reach here, it can be
+ * done by creating a separate index and attaching it via ALTER TABLE ..
+ * USING INDEX.
+ */
+ if (indexInfo->ii_NullsNotDistinct)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("primary keys cannot use NULLS NOT DISTINCT indexes")));
+ }
+
/*
* Check that all of the attributes in a primary key are marked as not
* null. (We don't really expect to see that; it'd mean the parser messed
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 44d957c038..64a010a7ec 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16416,7 +16416,12 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
{
appendPQExpBufferStr(q,
coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
- if (indxinfo->indnullsnotdistinct)
+ /*
+ * PRIMARY KEY constraints should not be using NULLS NOT DISTINCT
+ * indexes. Being able to create this was fixed, but we need to
+ * make the index distinct in order to be able to restore the dump.
+ */
+ if (indxinfo->indnullsnotdistinct && coninfo->contype != 'p')
appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
appendPQExpBufferStr(q, " (");
for (k = 0; k < indxinfo->indnkeyattrs; k++)
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6cd57e3eaa..acfd9d1f4f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1595,6 +1595,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+ERROR: primary keys cannot use NULLS NOT DISTINCT indexes
+DROP TABLE cwi_test;
--
-- Check handling of indexes on system columns
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index a3738833b2..d49ce9f300 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -617,6 +617,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+DROP TABLE cwi_test;
+
--
-- Check handling of indexes on system columns
--
--
2.32.1 (Apple Git-133)
=
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 15:30 Vik Fearing <vik@postgresfriends.org>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 22+ messages in thread
From: Vik Fearing @ 2022-12-15 15:30 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 12/15/22 14:18, Daniel Gustafsson wrote:
>> On 15 Dec 2022, at 00:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>> The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
>>> constraints but allow them for unique constraints. Is this along the lines of
>>> what you had in mind?
>>
>> Needs more than zero comments in the code, and why bother testing
>> is_alter_table in index_check_primary_key? We're disallowing
>> this case across-the-board, no matter how you get to it.
>
> That was an, admittedly poor, attempt at self-documenting code. Removed and
> comments added in the attached.
[reviewing patch...]
If the point in adding a primary key USING INDEX is to avoid building an
index, then this restriction defeats that purpose. We have no ALTER
INDEX command to switch or drop the <unique null treatment>.
Since the primary key can't have any nulls anyway, I think we should
just flip this, perhaps with a NOTICE like the rename does.
--
Vik Fearing
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 15:54 Tom Lane <tgl@sss.pgh.pa.us>
parent: Vik Fearing <vik@postgresfriends.org>
0 siblings, 3 replies; 22+ messages in thread
From: Tom Lane @ 2022-12-15 15:54 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Vik Fearing <vik@postgresfriends.org> writes:
> If the point in adding a primary key USING INDEX is to avoid building an
> index, then this restriction defeats that purpose. We have no ALTER
> INDEX command to switch or drop the <unique null treatment>.
Well, if you want to build an index to then use as a primary key,
it's incumbent on you to make the index with the correct properties.
Do you expect the system to silently fix it for you if the index is
on the wrong columns?
I might have more sympathy for this argument if the correct <unique null
treatment> were non-default, but it is not. You'd have had to go out
of your way to make the index incompatible. That in turn suggests that
there's a mistake somewhere, so having the system automatically fix
it for you might just be masking something that would be better dealt
with manually.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 16:08 Vik Fearing <vik@postgresfriends.org>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 22+ messages in thread
From: Vik Fearing @ 2022-12-15 16:08 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 12/15/22 16:54, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> If the point in adding a primary key USING INDEX is to avoid building an
>> index, then this restriction defeats that purpose. We have no ALTER
>> INDEX command to switch or drop the <unique null treatment>.
>
> Well, if you want to build an index to then use as a primary key,
> it's incumbent on you to make the index with the correct properties.
This is assuming you initially created the index for the purpose of
making it the primary key, perhaps for concurrency.
> Do you expect the system to silently fix it for you if the index is
> on the wrong columns?
Of course not.
> I might have more sympathy for this argument if the correct <unique null
> treatment> were non-default, but it is not. You'd have had to go out
> of your way to make the index incompatible. That in turn suggests that
> there's a mistake somewhere, so having the system automatically fix
> it for you might just be masking something that would be better dealt
> with manually.
The index may have been originally created for something else. The only
way to deal with this manually is to drop and recreate the index, just
to remove something that has no effect anyway because the index contains
no nulls.
I am not going to die on this hill, but I think it is a shame to make
users go through that. A NOTICE saying "btw, we did that" should be
sufficient.
--
Vik Fearing
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 16:12 David G. Johnston <david.g.johnston@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 22+ messages in thread
From: David G. Johnston @ 2022-12-15 16:12 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Vik Fearing <vik@postgresfriends.org>; Daniel Gustafsson <daniel@yesql.se>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Thu, Dec 15, 2022 at 8:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
> > If the point in adding a primary key USING INDEX is to avoid building an
> > index, then this restriction defeats that purpose. We have no ALTER
> > INDEX command to switch or drop the <unique null treatment>.
>
> Well, if you want to build an index to then use as a primary key,
> it's incumbent on you to make the index with the correct properties.
> Do you expect the system to silently fix it for you if the index is
> on the wrong columns?
>
> I might have more sympathy for this argument if the correct <unique null
> treatment> were non-default, but it is not. You'd have had to go out
> of your way to make the index incompatible. That in turn suggests that
> there's a mistake somewhere, so having the system automatically fix
> it for you might just be masking something that would be better dealt
> with manually.
>
>
No matter what we do we are either masking the situation that a user has a
primary key backing index defined NULLS NOT DISTINCT which is a pointless,
but not invalid, definition, or causing an error to happen because we've
decided that we shouldn't be masking that inconsistent configuration.
Green-field, maybe we should have made it a "NULLS { NOT ALLOWED | [NOT]
DISTINCT }" option and been better at informing/prohibiting the user from
choosing an index that is not compatible with the constraint they want it
to support. But that isn't really something we make an effort to do
generally and it seems too late to get on that horse now.
Sleeping on this, I feel even more strongly that touching pg_dump now is
wrong. The configuration, while nonsensical, is technically valid. Which
means that the argument for causing existing pg_dumps to become broken is
basically zero. We need to simply synchronize PRIMARY KEY to accept this
configuration in the interest of dump/restore preservation.
David J.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-15 17:02 reiner peterke <zedaardv@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
2 siblings, 0 replies; 22+ messages in thread
From: reiner peterke @ 2022-12-15 17:02 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Vik Fearing <vik@postgresfriends.org>; Daniel Gustafsson <daniel@yesql.se>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On Thu, Dec 15, 2022 at 4:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
> > If the point in adding a primary key USING INDEX is to avoid building an
> > index, then this restriction defeats that purpose. We have no ALTER
> > INDEX command to switch or drop the <unique null treatment>.
>
> Well, if you want to build an index to then use as a primary key,
> it's incumbent on you to make the index with the correct properties.
> Do you expect the system to silently fix it for you if the index is
> on the wrong columns?
>
> I might have more sympathy for this argument if the correct <unique null
> treatment> were non-default, but it is not. You'd have had to go out
> of your way to make the index incompatible. That in turn suggests that
> there's a mistake somewhere, so having the system automatically fix
> it for you might just be masking something that would be better dealt
> with manually.
>
> regards, tom lane
>
>
> Can you explain the distinction?
If i build an index nulls distinct/nulls not distinct on a column that will
later be used in an
alter index add constraint ... using index
both indexes can have at least one null.
from that point using an index in the creation of a constraint should not
matter if the index could potentially have one or many nulls.
If the pre version 15 syntax is correct, then the post version 15 syntax
should also be correct.
If one is not valid, then both should not be valid.
Or am i missing something here?
Reiner
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-16 08:07 Peter Eisentraut <peter.eisentraut@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 22+ messages in thread
From: Peter Eisentraut @ 2022-12-16 08:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Daniel Gustafsson <daniel@yesql.se>; +Cc: David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 15.12.22 00:27, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
>> constraints but allow them for unique constraints. Is this along the lines of
>> what you had in mind?
>
> Needs more than zero comments in the code, and why bother testing
> is_alter_table in index_check_primary_key? We're disallowing
> this case across-the-board, no matter how you get to it.
>
> I'll defer to Peter on whether this is in fact the right way to go,
> or we should relax the syntax restriction as David suggests.
My first instinct was to just fix pg_dump to not dump syntax that can't
be loaded in.
It shouldn't matter what null treatment the underlying unique index has,
since the primary key can't have nulls anyway, so either type of index
should be acceptable. But then we'd need to think through a bunch of
possible ALTER behaviors. For example, if we just change pg_dump and
leave the index as is, a subsequent dump and restore would lose the
original null treatment flag. What if someone then wants to re-detach
the constraint from the index? (Does that exist now? Maybe not, but it
could.) What should happen then? This could all be worked out, I
think, but it would need more thought.
In short, I think preventing the ALTER command, as proposed in this
patch, seems like a good solution for the moment. Additional work to
enable some of this could follow later independently.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-16 16:03 Daniel Gustafsson <daniel@yesql.se>
parent: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-16 16:03 UTC (permalink / raw)
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 16 Dec 2022, at 09:07, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 15.12.22 00:27, Tom Lane wrote:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>> The attached prohibits the use of NULLS NOT DISTINCT for backing primary key
>>> constraints but allow them for unique constraints. Is this along the lines of
>>> what you had in mind?
>> Needs more than zero comments in the code, and why bother testing
>> is_alter_table in index_check_primary_key? We're disallowing
>> this case across-the-board, no matter how you get to it.
>> I'll defer to Peter on whether this is in fact the right way to go,
>> or we should relax the syntax restriction as David suggests.
>
> My first instinct was to just fix pg_dump to not dump syntax that can't be loaded in.
>
> It shouldn't matter what null treatment the underlying unique index has, since the primary key can't have nulls anyway, so either type of index should be acceptable. But then we'd need to think through a bunch of possible ALTER behaviors. For example, if we just change pg_dump and leave the index as is, a subsequent dump and restore would lose the original null treatment flag. What if someone then wants to re-detach the constraint from the index? (Does that exist now? Maybe not, but it could.) What should happen then? This could all be worked out, I think, but it would need more thought.
>
> In short, I think preventing the ALTER command, as proposed in this patch, seems like a good solution for the moment. Additional work to enable some of this could follow later independently.
The attached removes the change from pg_dump and only prohibits the ALTER TABLE
command for attaching the index. Since it will render dumps unable to be
restored I also added a check to pg_upgrade to cover the case.
--
Daniel Gustafsson https://vmware.com/
Attachments:
v3-0001-Disallow-NULLS-NOT-DISTINCT-indexes-for-primary-k.patch (application/octet-stream, 7.0K) download
From 6baea59b788b915c23a9c934cad6c0db5b1ff62e Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 16 Dec 2022 16:21:38 +0100
Subject: [PATCH v3] Disallow NULLS NOT DISTINCT indexes for primary keys
A unique index which is created with non-distinct NULLS cannot be
used for backing a primary key constraint. Make sure to disallow
such table alterations and add a check to pg_upgrade to warn when
finding these in an old cluster.
Bug: 17720
Reported-by: Reiner Peterke <zedaardv@drizzle.com>
Discussion: https://postgr.es/m/17720-dab8ee0fa85d316d@postgresql.org
---
src/backend/catalog/index.c | 13 +++
src/bin/pg_upgrade/check.c | 93 ++++++++++++++++++++++
src/test/regress/expected/create_index.out | 6 ++
src/test/regress/sql/create_index.sql | 6 ++
4 files changed, 118 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 61f1d3926a..6ff769911e 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -225,6 +225,19 @@ index_check_primary_key(Relation heapRel,
RelationGetRelationName(heapRel))));
}
+ /*
+ * Indexes created with NULLS NOT DISTINCT cannot be used for primary key
+ * constraints. While there is no direct syntax to reach here, it can be
+ * done by creating a separate index and attaching it via ALTER TABLE ..
+ * USING INDEX.
+ */
+ if (indexInfo->ii_NullsNotDistinct)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("primary keys cannot use NULLS NOT DISTINCT indexes")));
+ }
+
/*
* Check that all of the attributes in a primary key are marked as not
* null. (We don't really expect to see that; it'd mean the parser messed
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index d4a3691438..09828b09e7 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -33,6 +33,7 @@ static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
static void check_for_pg_role_prefix(ClusterInfo *cluster);
static void check_for_new_tablespace_dir(ClusterInfo *new_cluster);
static void check_for_user_defined_encoding_conversions(ClusterInfo *cluster);
+static void check_for_nulls_not_distinct_pk(ClusterInfo *cluster);
static char *get_canonical_locale_name(int category, const char *locale);
@@ -176,6 +177,13 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 903)
old_9_3_check_for_line_data_type_usage(&old_cluster);
+ /*
+ * Pre-PG 16 allowed indexes defined with NULLS NOT DISTINCT to be used
+ * for primary key constraints.
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1500)
+ check_for_nulls_not_distinct_pk(&old_cluster);
+
/*
* While not a check option, we do this now because this is the only time
* the old server is running.
@@ -1525,6 +1533,91 @@ check_for_user_defined_encoding_conversions(ClusterInfo *cluster)
check_ok();
}
+/*
+ * check_for_nulls_not_distinct_pk
+ *
+ * In versions before v16 it was possible to create a primary key constraint
+ * with non-distinct nulls by attaching the index via alter table:
+ *
+ * CREATE UNIQUE INDEX idx ON <rel> (<column>) NULLS NOT DISTINCT;
+ * ALTER TABLE <rel> ADD PRIMARY KEY USING INDEX idx;
+ *
+ * This schema will be dumped by pg_dump but cannot be restored, so make sure
+ * the old cluster doesn't have any such constraints.
+ */
+static void
+check_for_nulls_not_distinct_pk(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE *script = NULL;
+ char output_path[MAXPGPATH];
+
+ prep_status("Checking for primary keys with non-distinct null indexes");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "nulls_not_distinct_pk.txt");
+
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ PGresult *res;
+ bool db_used = false;
+ int ntups;
+ int i_relname;
+ int i_nspname;
+ DbInfo *active_db = &cluster->dbarr.dbs[dbnum];
+ PGconn *conn = connectToServer(cluster, active_db->db_name);
+
+ res = executeQueryOrDie(conn,
+ "SELECT n.nspname, cl.relname "
+ "FROM pg_catalog.pg_constraint cn "
+ "JOIN pg_catalog.pg_class cl ON "
+ " (cn.conrelid = cl.oid AND cn.contype = 'p') "
+ "JOIN pg_catalog.pg_namespace n ON "
+ " (cl.relnamespace = n.oid) "
+ "JOIN pg_catalog.pg_index i ON "
+ " (cn.conindid = i.indexrelid AND "
+ " i.indnullsnotdistinct) "
+ "WHERE cn.contype = 'p'");
+ ntups = PQntuples(res);
+ if (!ntups)
+ continue;
+ i_nspname = PQfnumber(res, "nspname");
+ i_relname = PQfnumber(res, "relname");
+ for (int rowno = 0; rowno < ntups; rowno++)
+ {
+ if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
+ pg_fatal("could not open file \"%s\": %s",
+ output_path, strerror(errno));
+ if (!db_used)
+ {
+ fprintf(script, "In database: %s\n", active_db->db_name);
+ db_used = true;
+ }
+
+ fprintf(script, " %s.%s\n",
+ PQgetvalue(res, rowno, i_nspname),
+ PQgetvalue(res, rowno, i_relname));
+ }
+
+ PQclear(res);
+
+ PQfinish(conn);
+ }
+
+ if (script)
+ {
+ fclose(script);
+ pg_log(PG_REPORT, "fatal");
+ pg_fatal("Your installation contains primary keys which use an index\n"
+ "defined with NULLS NOT DISTINCT which cannot be restored.\n"
+ "These constraints must be dropped and recreated before\n"
+ "upgrading. A list of the problematic tables is in the file:\n"
+ " %s", output_path);
+ }
+ else
+ check_ok();
+}
/*
* get_canonical_locale_name
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6cd57e3eaa..acfd9d1f4f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1595,6 +1595,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+ERROR: primary keys cannot use NULLS NOT DISTINCT indexes
+DROP TABLE cwi_test;
--
-- Check handling of indexes on system columns
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index a3738833b2..d49ce9f300 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -617,6 +617,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+DROP TABLE cwi_test;
+
--
-- Check handling of indexes on system columns
--
--
2.32.1 (Apple Git-133)
=
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-16 16:12 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2022-12-16 16:12 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> The attached removes the change from pg_dump and only prohibits the ALTER TABLE
> command for attaching the index. Since it will render dumps unable to be
> restored I also added a check to pg_upgrade to cover the case.
That doesn't seem like a great answer. I understood Peter to be
favoring both aspects of the previous patch.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-16 18:14 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Gustafsson @ 2022-12-16 18:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 16 Dec 2022, at 17:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> The attached removes the change from pg_dump and only prohibits the ALTER TABLE
>> command for attaching the index. Since it will render dumps unable to be
>> restored I also added a check to pg_upgrade to cover the case.
>
> That doesn't seem like a great answer. I understood Peter to be
> favoring both aspects of the previous patch.
Oh, ok, if so then I misunderstood. No worries, v2 is then the one to consider.
--
Daniel Gustafsson https://vmware.com/
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2022-12-16 18:19 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 22+ messages in thread
From: Tom Lane @ 2022-12-16 18:19 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Daniel Gustafsson <daniel@yesql.se> writes:
> Oh, ok, if so then I misunderstood. No worries, v2 is then the one to consider.
I'm good with v2.
regards, tom lane
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...'
@ 2023-02-23 21:07 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Gustafsson @ 2023-02-23 21:07 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>; David G. Johnston <david.g.johnston@gmail.com>; zedaardv@drizzle.com <zedaardv@drizzle.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
> On 16 Dec 2022, at 19:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Oh, ok, if so then I misunderstood. No worries, v2 is then the one to consider.
>
> I'm good with v2.
Returning to an old thread that got sidetracked by christmas et.al. Attached
is a rebased v4 based on the v2 patch above. I'll attach this to the CF to get
the full CFbot treatment and if all is green I'll push this.
--
Daniel Gustafsson
Attachments:
v4-0001-Disallow-NULLS-NOT-DISTINCT-indexes-for-primary-k.patch (application/octet-stream, 4.2K) download
From 8b31664b0e10c52b2eb0b1eaa00f6b1f0d235483 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Thu, 23 Feb 2023 21:27:35 +0100
Subject: [PATCH v4] Disallow NULLS NOT DISTINCT indexes for primary keys
A unique index which is created with non-distinct NULLS cannot be
used for backing a primary key constraint. Make sure to disallow
such table alterations and teach pg_dump to drop the non-distinct
NULLS clause on indexes where this has been set.
Bug: 17720
Reported-by: Reiner Peterke <zedaardv@drizzle.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/17720-dab8ee0fa85d316d@postgresql.org
---
src/backend/catalog/index.c | 13 +++++++++++++
src/bin/pg_dump/pg_dump.c | 7 ++++++-
src/test/regress/expected/create_index.out | 6 ++++++
src/test/regress/sql/create_index.sql | 6 ++++++
4 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 41b16cb89b..7777e7ec77 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -225,6 +225,19 @@ index_check_primary_key(Relation heapRel,
RelationGetRelationName(heapRel))));
}
+ /*
+ * Indexes created with NULLS NOT DISTINCT cannot be used for primary key
+ * constraints. While there is no direct syntax to reach here, it can be
+ * done by creating a separate index and attaching it via ALTER TABLE ..
+ * USING INDEX.
+ */
+ if (indexInfo->ii_NullsNotDistinct)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("primary keys cannot use NULLS NOT DISTINCT indexes")));
+ }
+
/*
* Check that all of the attributes in a primary key are marked as not
* null. (We don't really expect to see that; it'd mean the parser messed
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1a06eeaf6a..7a8df1e803 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16432,7 +16432,12 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo)
{
appendPQExpBufferStr(q,
coninfo->contype == 'p' ? "PRIMARY KEY" : "UNIQUE");
- if (indxinfo->indnullsnotdistinct)
+ /*
+ * PRIMARY KEY constraints should not be using NULLS NOT DISTINCT
+ * indexes. Being able to create this was fixed, but we need to
+ * make the index distinct in order to be able to restore the dump.
+ */
+ if (indxinfo->indnullsnotdistinct && coninfo->contype != 'p')
appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
appendPQExpBufferStr(q, " (");
for (k = 0; k < indxinfo->indnkeyattrs; k++)
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6cd57e3eaa..acfd9d1f4f 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1595,6 +1595,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
ERROR: ALTER TABLE / ADD CONSTRAINT USING INDEX is not supported on partitioned tables
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+ERROR: primary keys cannot use NULLS NOT DISTINCT indexes
+DROP TABLE cwi_test;
--
-- Check handling of indexes on system columns
--
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index a3738833b2..d49ce9f300 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -617,6 +617,12 @@ create unique index on cwi_test (a);
alter table cwi_test add primary key using index cwi_test_a_idx ;
DROP TABLE cwi_test;
+-- PRIMARY KEY constraint cannot be backed by a NULLS NOT DISTINCT index
+CREATE TABLE cwi_test(a int, b int);
+CREATE UNIQUE INDEX cwi_a_nnd ON cwi_test (a) NULLS NOT DISTINCT;
+ALTER TABLE cwi_test ADD PRIMARY KEY USING INDEX cwi_a_nnd;
+DROP TABLE cwi_test;
+
--
-- Check handling of indexes on system columns
--
--
2.32.1 (Apple Git-133)
=
end of thread, other threads:[~2023-02-23 21:07 UTC | newest]
Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14 09:07 BUG #17720: pg_dump creates a dump with primary key that cannot be restored, when specifying 'using index ...' PG Bug reporting form <noreply@postgresql.org>
2022-12-14 12:54 ` David G. Johnston <david.g.johnston@gmail.com>
2022-12-14 13:02 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-14 15:37 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-14 15:51 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-14 15:59 ` David G. Johnston <david.g.johnston@gmail.com>
2022-12-14 16:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-14 16:34 ` David G. Johnston <david.g.johnston@gmail.com>
2022-12-14 22:30 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-14 23:27 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-15 13:18 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-15 15:30 ` Vik Fearing <vik@postgresfriends.org>
2022-12-15 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-15 16:08 ` Vik Fearing <vik@postgresfriends.org>
2022-12-15 16:12 ` David G. Johnston <david.g.johnston@gmail.com>
2022-12-15 17:02 ` reiner peterke <zedaardv@gmail.com>
2022-12-16 08:07 ` Peter Eisentraut <peter.eisentraut@enterprisedb.com>
2022-12-16 16:03 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-16 16:12 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-12-16 18:14 ` Daniel Gustafsson <daniel@yesql.se>
2022-12-16 18:19 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-02-23 21:07 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox