Incorrect format in error message

Started by David Rowleyalmost 10 years ago5 messages

david.rowley@2ndquadrant.com

almost 10 years ago

1 attachment(s)

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

# create table t ();
# create unique index t_idx on t (ctid);
# alter table t replica identity using index t_idx;
ERROR: internal column 4294967295 in unique index "t_idx"

I was just going to submit a patch to change the %u to a %d, but this
error message should use ereport() instead of elog(), so I fixed that
up too and added the missing regression test.

I picked ERRCODE_INVALID_COLUMN_REFERENCE as I thought it was the most
suitable. I'm not sure if it's correct though.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

fix_replica_identity_error.difftext/plain; charset=US-ASCII; name=fix_replica_identity_error.diffDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 96dc923..36b5448 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11052,8 +11052,10 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
 
 		/* Of the system columns, only oid is indexable. */
 		if (attno <= 0 && attno != ObjectIdAttributeNumber)
-			elog(ERROR, "internal column %u in unique index \"%s\"",
-				 attno, RelationGetRelationName(indexRel));
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					 errmsg("index \"%s\" cannot be used as replica identity because column %d is a system column",
+							RelationGetRelationName(indexRel), attno)));
 
 		attr = rel->rd_att->attrs[attno - 1];
 		if (!attr->attnotnull)
diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out
index 60d9a42..47678db 100644
--- a/src/test/regress/expected/replica_identity.out
+++ b/src/test/regress/expected/replica_identity.out
@@ -9,6 +9,7 @@ CREATE TABLE test_replica_identity (
 CREATE TABLE test_replica_identity_othertable (id serial primary key);
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
+CREATE UNIQUE INDEX test_replica_indentity_syscol ON test_replica_identity (ctid);
 CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
 CREATE INDEX test_replica_identity_hash ON test_replica_identity USING hash (nonkey);
 WARNING:  hash indexes are not WAL-logged and their use is discouraged
@@ -40,6 +41,9 @@ SELECT relreplident FROM pg_class WHERE oid = 'pg_constraint'::regclass;
 -- fail, not unique
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_keyab;
 ERROR:  cannot use non-unique index "test_replica_identity_keyab" as replica identity
+-- fail, index can't contain system columns
+ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_indentity_syscol;
+ERROR:  index "test_replica_indentity_syscol" cannot be used as replica identity because column -1 is a system column
 -- fail, not a candidate key, nullable column
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_nonkey;
 ERROR:  index "test_replica_identity_nonkey" cannot be used as replica identity because column "nonkey" is nullable
@@ -91,6 +95,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 
@@ -121,6 +126,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 
@@ -169,6 +175,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 Replica Identity: FULL
diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql
index 20b6826..436851c 100644
--- a/src/test/regress/sql/replica_identity.sql
+++ b/src/test/regress/sql/replica_identity.sql
@@ -11,6 +11,7 @@ CREATE TABLE test_replica_identity_othertable (id serial primary key);
 
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
+CREATE UNIQUE INDEX test_replica_indentity_syscol ON test_replica_identity (ctid);
 CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
 CREATE INDEX test_replica_identity_hash ON test_replica_identity USING hash (nonkey);
 CREATE UNIQUE INDEX test_replica_identity_expr ON test_replica_identity (keya, keyb, (3));
@@ -28,6 +29,8 @@ SELECT relreplident FROM pg_class WHERE oid = 'pg_constraint'::regclass;
 
 -- fail, not unique
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_keyab;
+-- fail, index can't contain system columns
+ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_indentity_syscol;
 -- fail, not a candidate key, nullable column
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_nonkey;
 -- fail, hash indexes cannot do uniqueness

Tom Lane

tgl@sss.pgh.pa.us

almost 10 years ago

In reply to: David Rowley (#1)

Re: Incorrect format in error message

David Rowley <david.rowley@2ndquadrant.com> writes:

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

I think that's the tip of the iceberg :-(. For starters, the code
allows ObjectIdAttributeNumber without regard for the fact that the
next line will dump core on a negative attno. Really though, what
astonishes me about this example is that we allow indexes at all on
system columns other than OID. None of the other ones can possibly
have either a use-case or sensible semantics, can they? We certainly
would not stop to update indexes after changing xmax, for example.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Rowley

david.rowley@2ndquadrant.com

almost 10 years ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: Incorrect format in error message

On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <david.rowley@2ndquadrant.com> writes:

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

I think that's the tip of the iceberg :-(. For starters, the code
allows ObjectIdAttributeNumber without regard for the fact that the
next line will dump core on a negative attno. Really though, what
astonishes me about this example is that we allow indexes at all on
system columns other than OID. None of the other ones can possibly
have either a use-case or sensible semantics, can they? We certainly
would not stop to update indexes after changing xmax, for example.

ouch. Yeah that's not going to work very well. I guess nobody's tried
that with a unique index on an OID column yet then.

I've changed the patch around a little to fix the crash. I was a bit
worried as logical decoding obviously has not yet been tested with an
OID unique index as the replica indentity, so I gave it a quick test
to try to give myself a little piece of mind that this won't uncover
something else;

# create table t (a int) with oids;
CREATE TABLE
# create unique index t_oid_idx on t(oid);
CREATE INDEX
# alter table t replica identity using index t_oid_idx;
ALTER TABLE
# insert into t values(123);
INSERT 24593 1
# delete from t;

On the receive side:

BEGIN 606
table public.t: INSERT: oid[oid]:24593 a[integer]:123
COMMIT 606
BEGIN 607
table public.t: DELETE: oid[oid]:24593
COMMIT 607

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

replica_identity_crash_fix.patchapplication/octet-stream; name=replica_identity_crash_fix.patchDownload

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 96dc923..3ebe799 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11050,12 +11050,20 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
 		int16		attno = indexRel->rd_index->indkey.values[key];
 		Form_pg_attribute attr;
 
-		/* Of the system columns, only oid is indexable. */
-		if (attno <= 0 && attno != ObjectIdAttributeNumber)
-			elog(ERROR, "internal column %u in unique index \"%s\"",
-				 attno, RelationGetRelationName(indexRel));
+		/* Allow OIDs to be indexed. No need to check for nullabiliy here */
+		if (attno == ObjectIdAttributeNumber)
+			continue;
+
+		/* Reject any other system columns */
+		if (attno <= 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					 errmsg("index \"%s\" cannot be used as replica identity because column %d is a system column",
+							RelationGetRelationName(indexRel), attno)));
 
 		attr = rel->rd_att->attrs[attno - 1];
+
+		/* Must be a non-system column. Check it's not nullable */
 		if (!attr->attnotnull)
 			ereport(ERROR,
 					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out
index 60d9a42..47678db 100644
--- a/src/test/regress/expected/replica_identity.out
+++ b/src/test/regress/expected/replica_identity.out
@@ -9,6 +9,7 @@ CREATE TABLE test_replica_identity (
 CREATE TABLE test_replica_identity_othertable (id serial primary key);
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
+CREATE UNIQUE INDEX test_replica_indentity_syscol ON test_replica_identity (ctid);
 CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
 CREATE INDEX test_replica_identity_hash ON test_replica_identity USING hash (nonkey);
 WARNING:  hash indexes are not WAL-logged and their use is discouraged
@@ -40,6 +41,9 @@ SELECT relreplident FROM pg_class WHERE oid = 'pg_constraint'::regclass;
 -- fail, not unique
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_keyab;
 ERROR:  cannot use non-unique index "test_replica_identity_keyab" as replica identity
+-- fail, index can't contain system columns
+ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_indentity_syscol;
+ERROR:  index "test_replica_indentity_syscol" cannot be used as replica identity because column -1 is a system column
 -- fail, not a candidate key, nullable column
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_nonkey;
 ERROR:  index "test_replica_identity_nonkey" cannot be used as replica identity because column "nonkey" is nullable
@@ -91,6 +95,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 
@@ -121,6 +126,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 
@@ -169,6 +175,7 @@ Indexes:
     "test_replica_identity_partial" UNIQUE, btree (keya, keyb) WHERE keyb <> '3'::text
     "test_replica_identity_unique_defer" UNIQUE CONSTRAINT, btree (keya, keyb) DEFERRABLE
     "test_replica_identity_unique_nondefer" UNIQUE CONSTRAINT, btree (keya, keyb)
+    "test_replica_indentity_syscol" UNIQUE, btree (ctid)
     "test_replica_identity_hash" hash (nonkey)
     "test_replica_identity_keyab" btree (keya, keyb)
 Replica Identity: FULL
diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql
index 20b6826..436851c 100644
--- a/src/test/regress/sql/replica_identity.sql
+++ b/src/test/regress/sql/replica_identity.sql
@@ -11,6 +11,7 @@ CREATE TABLE test_replica_identity_othertable (id serial primary key);
 
 CREATE INDEX test_replica_identity_keyab ON test_replica_identity (keya, keyb);
 CREATE UNIQUE INDEX test_replica_identity_keyab_key ON test_replica_identity (keya, keyb);
+CREATE UNIQUE INDEX test_replica_indentity_syscol ON test_replica_identity (ctid);
 CREATE UNIQUE INDEX test_replica_identity_nonkey ON test_replica_identity (keya, nonkey);
 CREATE INDEX test_replica_identity_hash ON test_replica_identity USING hash (nonkey);
 CREATE UNIQUE INDEX test_replica_identity_expr ON test_replica_identity (keya, keyb, (3));
@@ -28,6 +29,8 @@ SELECT relreplident FROM pg_class WHERE oid = 'pg_constraint'::regclass;
 
 -- fail, not unique
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_keyab;
+-- fail, index can't contain system columns
+ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_indentity_syscol;
 -- fail, not a candidate key, nullable column
 ALTER TABLE test_replica_identity REPLICA IDENTITY USING INDEX test_replica_identity_nonkey;
 -- fail, hash indexes cannot do uniqueness

David Rowley

david.rowley@2ndquadrant.com

almost 10 years ago

In reply to: Tom Lane (#2)

Re: Incorrect format in error message

On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <david.rowley@2ndquadrant.com> writes:

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

Really though, what
astonishes me about this example is that we allow indexes at all on
system columns other than OID. None of the other ones can possibly
have either a use-case or sensible semantics, can they? We certainly
would not stop to update indexes after changing xmax, for example.

As for this part. I really don't see how we could disable this without
breaking pg_restore for database who have such indexes. My best
thought is to add some sort of warning during CREATE INDEX, like we do
for HASH indexes.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund

andres@anarazel.de

almost 10 years ago

In reply to: David Rowley (#4)

Re: Incorrect format in error message

On 2016-04-01 20:18:29 +1300, David Rowley wrote:

On 1 April 2016 at 17:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <david.rowley@2ndquadrant.com> writes:

The attached fixes an error message which is incorrectly using an
unsigned format specifier instead of a signed one.

Really though, what
astonishes me about this example is that we allow indexes at all on
system columns other than OID. None of the other ones can possibly
have either a use-case or sensible semantics, can they? We certainly
would not stop to update indexes after changing xmax, for example.

As for this part. I really don't see how we could disable this without
breaking pg_restore for database who have such indexes. My best
thought is to add some sort of warning during CREATE INDEX, like we do
for HASH indexes.

As they're currently already not working correctly as indexes, I don't
see throwing an error during pg_restore as being overly harmful.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers