From fc449abc3dccc06b01783bcdb2c348c8ef670ccd Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 17 Dec 2019 17:44:28 -0500 Subject: [PATCH] Fix error reporting for index expressions of prohibited types. If CheckAttributeType() threw an error about the datatype of an index expression column, it would report an empty column name, which is pretty unhelpful and certainly not the intended behavior. I (tgl) evidently broke this in commit cfc5008a5, by not noticing that the column's attname was used above where I'd placed the assignment of it. In HEAD and v12, this is trivially fixable by moving up the assignment of attname. Before v12 the code is a bit more messy; to avoid doing substantial refactoring, I took the lazy way out and just put in two copies of the assignment code. Report and patch by Amit Langote. Back-patch to all supported branches. Discussion: https://postgr.es/m/CA+HiwqFA+BGyBFimjiYXXMa2Hc3fcL0+OJOyzUNjhU4NCa_XXw@mail.gmail.com --- src/backend/catalog/index.c | 24 +++++++++----- src/test/regress/expected/create_index.out | 37 ++++++++++++++++++++++ src/test/regress/sql/create_index.sql | 12 +++++++ 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index be363ce4dc..bc20d7c750 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -368,6 +368,14 @@ ConstructTupleDescriptor(Relation heapRelation, */ memcpy(to, from, ATTRIBUTE_FIXED_PART_SIZE); + /* + * Set the attribute name as specified by caller. + */ + if (colnames_item == NULL) /* shouldn't happen */ + elog(ERROR, "too few entries in colnames list"); + namestrcpy(&to->attname, (const char *) lfirst(colnames_item)); + colnames_item = lnext(colnames_item); + /* * Fix the stuff that should not be the same as the underlying * attr @@ -392,6 +400,14 @@ ConstructTupleDescriptor(Relation heapRelation, MemSet(to, 0, ATTRIBUTE_FIXED_PART_SIZE); + /* + * Set the attribute name as specified by caller. + */ + if (colnames_item == NULL) /* shouldn't happen */ + elog(ERROR, "too few entries in colnames list"); + namestrcpy(&to->attname, (const char *) lfirst(colnames_item)); + colnames_item = lnext(colnames_item); + if (indexpr_item == NULL) /* shouldn't happen */ elog(ERROR, "too few entries in indexprs list"); indexkey = (Node *) lfirst(indexpr_item); @@ -445,14 +461,6 @@ ConstructTupleDescriptor(Relation heapRelation, */ to->attrelid = InvalidOid; - /* - * Set the attribute name as specified by caller. - */ - if (colnames_item == NULL) /* shouldn't happen */ - elog(ERROR, "too few entries in colnames list"); - namestrcpy(&to->attname, (const char *) lfirst(colnames_item)); - colnames_item = lnext(colnames_item); - /* * Check the opclass and index AM to see if either provides a keytype * (overriding the attribute type). Opclass (if exists) takes diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 90b671f5b9..e7d7c7aee6 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2417,6 +2417,23 @@ ERROR: duplicate key value violates unique constraint "func_index_index" DETAIL: Key (textcat(f1, f2))=(ABCDEF) already exists. -- but this shouldn't: INSERT INTO func_index_heap VALUES('QWERTY'); +-- while we're here, see that the metadata looks sane +\d func_index_heap + Table "public.func_index_heap" + Column | Type | Collation | Nullable | Default +--------+------+-----------+----------+--------- + f1 | text | | | + f2 | text | | | +Indexes: + "func_index_index" UNIQUE, btree (textcat(f1, f2)) + +\d func_index_index + Index "public.func_index_index" + Column | Type | Key? | Definition +---------+------+------+----------------- + textcat | text | yes | textcat(f1, f2) +unique, btree, for table "public.func_index_heap" + -- -- Same test, expressional index -- @@ -2432,6 +2449,26 @@ ERROR: duplicate key value violates unique constraint "func_index_index" DETAIL: Key ((f1 || f2))=(ABCDEF) already exists. -- but this shouldn't: INSERT INTO func_index_heap VALUES('QWERTY'); +-- while we're here, see that the metadata looks sane +\d func_index_heap + Table "public.func_index_heap" + Column | Type | Collation | Nullable | Default +--------+------+-----------+----------+--------- + f1 | text | | | + f2 | text | | | +Indexes: + "func_index_index" UNIQUE, btree ((f1 || f2)) + +\d func_index_index + Index "public.func_index_index" + Column | Type | Key? | Definition +--------+------+------+------------ + expr | text | yes | (f1 || f2) +unique, btree, for table "public.func_index_heap" + +-- this should fail because of unsafe column type (anonymous record) +create index on func_index_heap ((f1 || f2), (row(f1, f2))); +ERROR: column "row" has pseudo-type record -- -- Test unique index with included columns -- diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql index 6eee92bf97..283c4e6c18 100644 --- a/src/test/regress/sql/create_index.sql +++ b/src/test/regress/sql/create_index.sql @@ -725,6 +725,10 @@ INSERT INTO func_index_heap VALUES('ABCD', 'EF'); -- but this shouldn't: INSERT INTO func_index_heap VALUES('QWERTY'); +-- while we're here, see that the metadata looks sane +\d func_index_heap +\d func_index_index + -- -- Same test, expressional index @@ -741,6 +745,14 @@ INSERT INTO func_index_heap VALUES('ABCD', 'EF'); -- but this shouldn't: INSERT INTO func_index_heap VALUES('QWERTY'); +-- while we're here, see that the metadata looks sane +\d func_index_heap +\d func_index_index + +-- this should fail because of unsafe column type (anonymous record) +create index on func_index_heap ((f1 || f2), (row(f1, f2))); + + -- -- Test unique index with included columns --