Thread: ERROR: "ft1" is of the wrong type.
Hello,
If I invoked a wrong ALTER TABLE command like this, I would see an
unexpected error.
=# ALTER TABLE <foreign table> ATTACH PARTITION ....
ERROR: "ft1" is of the wrong type
The cause is that ATWrongRelkidError doesn't handle ATT_TABLE |
ATT_ATT_PARTITIONED_INDEX.
After checking all callers of ATSimplePermissions, I found that;
The two below are no longer used.
ATT_TABLE | ATT_VIEW
ATT_TABLE | ATT_MATVIEW | ATT_INDEX
The four below are not handled.
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_PARTITIONED_INDEX:
ATT_INDEX:
The attached is just fixing that. I tried to make it generic but
didn't find a clean and translatable way.
Also I found that only three cases in the function are excecised by
make check.
ATT_TABLE : foreign_data, indexing checks
ATT_TABLE | ATT_FOREIGN_TABLE : alter_table
ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table
I'm not sure it's worth the trouble so the attached doesn't do
anything for that.
Versions back to PG11 have similar but different mistakes.
PG11, 12:
the two below are not used
ATT_TABLE | ATT_VIEW
ATT_TABLE | ATT_MATVIEW | ATT_INDEX
the two below are not handled
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_PARTITIONED_INDEX
PG13:
the two below are not used
ATT_TABLE | ATT_VIEW
ATT_TABLE | ATT_MATVIEW | ATT_INDEX
the three below are not handled
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_PARTITIONED_INDEX
PG10:
ATT_TABLE | ATT_VIEW is not used
(all values are handled)
So the attached are the patches for PG11, 12, 13 and master.
It seems that the case lines in the function are intended to be in the
ATT_*'s definition order, but some of the them are out of that
order. However, I didn't reorder existing lines in the attached. I
didn't check the value itself is correct for the callers.
regareds.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a149ca044c..cf572a7c58 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5063,9 +5063,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE:
msg = _("\"%s\" is not a table");
break;
- case ATT_TABLE | ATT_VIEW:
- msg = _("\"%s\" is not a table or view");
- break;
case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, view, or foreign table");
break;
@@ -5075,8 +5072,8 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW:
msg = _("\"%s\" is not a table or materialized view");
break;
- case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
- msg = _("\"%s\" is not a table, materialized view, or index");
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
@@ -5090,6 +5087,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6f4a3e70a4..6c15a4e034 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5350,9 +5350,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE:
msg = _("\"%s\" is not a table");
break;
- case ATT_TABLE | ATT_VIEW:
- msg = _("\"%s\" is not a table or view");
- break;
case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, view, or foreign table");
break;
@@ -5362,8 +5359,8 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW:
msg = _("\"%s\" is not a table or materialized view");
break;
- case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
- msg = _("\"%s\" is not a table, materialized view, or index");
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
@@ -5377,6 +5374,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index faba264135..fb49dbadc8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5627,9 +5627,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE:
msg = _("\"%s\" is not a table");
break;
- case ATT_TABLE | ATT_VIEW:
- msg = _("\"%s\" is not a table or view");
- break;
case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, view, or foreign table");
break;
@@ -5639,8 +5636,11 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW:
msg = _("\"%s\" is not a table or materialized view");
break;
- case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
- msg = _("\"%s\" is not a table, materialized view, or index");
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
+ break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, materialized view, index, partitioned index, or foreign table");
break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
@@ -5654,6 +5654,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 420991e315..2b30dee333 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5753,9 +5753,6 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE:
msg = _("\"%s\" is not a table");
break;
- case ATT_TABLE | ATT_VIEW:
- msg = _("\"%s\" is not a table or view");
- break;
case ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, view, or foreign table");
break;
@@ -5765,9 +5762,12 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW:
msg = _("\"%s\" is not a table or materialized view");
break;
- case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
- msg = _("\"%s\" is not a table, materialized view, or index");
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, materialized view, index, partitioned index, or foreign table");
+ break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
break;
@@ -5780,9 +5780,15 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
+ case ATT_INDEX:
+ msg = _("\"%s\" is not an index");
+ break;
case ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a foreign table");
break;
On Tue, Feb 16, 2021 at 06:14:15PM +0900, Kyotaro Horiguchi wrote: > The attached is just fixing that. I tried to make it generic but > didn't find a clean and translatable way. > > Also I found that only three cases in the function are excecised by > make check. > > ATT_TABLE : foreign_data, indexing checks > ATT_TABLE | ATT_FOREIGN_TABLE : alter_table > ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table > > I'm not sure it's worth the trouble so the attached doesn't do > anything for that. Each sentence needs to be completely separate, as the language translated to may tweak the punctuation of the set of objects listed, at least. But you know that already :) If you have seen cases where permission checks show up messages with an incorrect relkind mentioned, could you add some regression tests able to trigger the problematic cases you saw and to improve this coverage? -- Michael
Attachment
At Thu, 18 Feb 2021 16:27:23 +0900, Michael Paquier <michael@paquier.xyz> wrote in > On Tue, Feb 16, 2021 at 06:14:15PM +0900, Kyotaro Horiguchi wrote: > > The attached is just fixing that. I tried to make it generic but > > didn't find a clean and translatable way. > > > > Also I found that only three cases in the function are excecised by > > make check. > > > > ATT_TABLE : foreign_data, indexing checks > > ATT_TABLE | ATT_FOREIGN_TABLE : alter_table > > ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE : alter_table > > > > I'm not sure it's worth the trouble so the attached doesn't do > > anything for that. > > Each sentence needs to be completely separate, as the language > translated to may tweak the punctuation of the set of objects listed, > at least. But you know that already :) Yeah, I strongly feel that:p As you pointed, the puctuations and the article (for index and others) was that. > If you have seen cases where permission checks show up messages with > an incorrect relkind mentioned, could you add some regression tests > able to trigger the problematic cases you saw and to improve this > coverage? I can add some regression tests to cover all the live cases. That could reveal no-longer-used combinations. I'll do that. Thaks for the suggestion. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Thu, 18 Feb 2021 17:17:37 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> I can add some regression tests to cover all the live cases. That
> could reveal no-longer-used combinations.
The attached is that.
ATT_VIEW is used for "CREATE OR REPLACE view" and checked against
earlier in DefineVirtualRelation. But we can add a test to make sure
that is checked anywhere.
All other values can be exercised.
ATT_TABLE | ATT_MATVIEW
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX |
ATT_FOREIGN_TABLE
ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_PARTITIONED_INDEX
ATT_TABLE | ATT_VIEW | ATT_MATVIEW | ATT_INDEX
ATT_TABLE | ATT_VIEW | ATT_FOREIGN_TABLE:
ATT_FOREIGN_TABLE
These are provoked by the following commands respectively:
ALTER TABLE <view> CLUSTER ON
ALTER TABLE <view> SET TABLESPACE
ALTER TABLE <view> ALTER COLUMN <col> SET STATISTICS
ALTER TABLE <view> ALTER COLUMN <col> SET STORGE
ALTER TABLE <view> ALTER COLUMN <col> SET()
ALTER TABLE <view> ATTACH PARTITION
ALTER TABLE/INDEX <partidx> SET/RESET
ALTER TABLE <matview> ALTER <col> SET DEFAULT
ALTER TABLE/INDEX <pidx> ALTER COLLATION ..REFRESH VERSION
ALTER TABLE <view> OPTIONS ()
The following three errors are already excised.
ATT_TABLE
ATT_TABLE | ATT_FOREIGN_TABLE
ATT_TABLE | ATT_COMPOSITE_TYPE | ATT_FOREIGN_TABLE:
By the way, I find this as somewhat mystifying. I'm not sure it worth
fixing though..
ALTER MATERIALIZED VIEW mv1 ALTER COLUMN a SET DEFAULT 1;
ERROR: "mv1" is not a table, view, or foreign table
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 0ce6ee4622..4a367c9609 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -6,6 +6,7 @@ SET client_min_messages TO 'warning';
DROP ROLE IF EXISTS regress_alter_table_user1;
RESET client_min_messages;
CREATE USER regress_alter_table_user1;
+CREATE VIEW at_v1 AS SELECT 1 as a;
--
-- add attribute
--
@@ -120,6 +121,9 @@ ALTER INDEX attmp_idx ALTER COLUMN 4 SET STATISTICS 1000;
ERROR: column number 4 of relation "attmp_idx" does not exist
ALTER INDEX attmp_idx ALTER COLUMN 2 SET STATISTICS -1;
DROP TABLE attmp;
+-- test that the command correctly complains for the object of a wrong type
+ALTER TABLE at_v1 ALTER COLUMN a SET STATISTICS 0; -- ERROR
+ERROR: "at_v1" is not a table, materialized view, index, partitioned index, or foreign table
--
-- rename - check on both non-temp and temp tables
--
@@ -1186,6 +1190,11 @@ select * from def_test;
alter table def_test alter column c1 set default 'wrong_datatype';
ERROR: invalid input syntax for type integer: "wrong_datatype"
alter table def_test alter column c2 set default 20;
+-- set defaults to an incorrect object: this should fail
+create materialized view def_tmp_mv as select 1 as a;
+alter table def_tmp_mv alter a set default 0;
+ERROR: "def_tmp_mv" is not a table, view, or foreign table
+drop materialized view def_tmp_mv;
-- set defaults on a non-existent column: this should fail
alter table def_test alter column c3 set default 30;
ERROR: column "c3" of relation "def_test" does not exist
@@ -2076,6 +2085,9 @@ Indexes:
"at_part_2_a_idx" btree (a)
"at_part_2_b_idx" btree (b)
+-- check if the command correctly complains for the object of a wrong type
+alter table at_partitioned_a_idx set (dummy = 1); -- ERROR
+ERROR: "at_partitioned_a_idx" is not a table, view, materialized view, or index
drop table at_partitioned;
-- Alter column type when no table rewrite is required
-- Also check that comments are preserved
@@ -2212,6 +2224,9 @@ Indexes:
a | text | yes | a | external |
btree, for table "public.test_storage"
+-- test that SET STORAGE correctly complains for the object of a wrong type
+alter table at_v1 alter column a set storage plain; -- ERROR
+ERROR: "at_v1" is not a table, materialized view, or foreign table
-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
@@ -2684,6 +2699,9 @@ select * from my_locks order by 1;
(2 rows)
commit;
+-- test that the command corectly complains for the object of a wrong type
+alter table at_v1 alter column a set (dummy = 1);
+ERROR: "at_v1" is not a table, materialized view, index, or foreign table
begin; alter table alterlock alter column f2 set storage extended;
select * from my_locks order by 1;
relname | max_lockmode
@@ -4110,6 +4128,9 @@ ERROR: remainder for hash partition must be less than modulus
ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3, REMAINDER 2);
ERROR: every hash partition modulus must be a factor of the next larger modulus
DROP TABLE fail_part;
+-- check that attach partition correctly complains for the object of a wrong type
+ALTER TABLE at_v1 ATTACH PARTITION dummy default; -- ERROR
+ERROR: "at_v1" is not a table or partitioned index
--
-- DETACH PARTITION
--
@@ -4350,6 +4371,11 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- check that the command correctly complains for the object of a wrong type
+create table attmp();
+alter table attmp options (dummy '1');
+ERROR: "attmp" is not a foreign table
+drop table attmp;
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
-- non-null attribute values, and arrange to pass them through
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..b4d4163836 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -572,7 +572,12 @@ SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
(4 rows)
COMMIT;
+-- check that the command correctly complains for the object of a wrong type
+CREATE VIEW clstr_tst_view AS SELECT 1;
+ALTER TABLE clstr_tst_view CLUSTER ON x; -- ERROR
+ERROR: "clstr_tst_view" is not a table or materialized view
-- clean up
+DROP VIEW clstr_tst_view;
DROP TABLE clustertest;
DROP TABLE clstr_1;
DROP TABLE clstr_2;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index bc3752e923..981e626f53 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2148,6 +2148,13 @@ AND objid::regclass::text = 'icuidx17_part';
icuidx17_part | f
(1 row)
+-- Test that ALTER COLLATION REFRESH VERSION correctly complains for
+-- wrong object. We use ALTER TABLE, not ALTER INDEX since we are
+-- exercising ATWrongRelkindError here.
+CREATE VIEW failview AS SELECT 1 AS a;
+ALTER TABLE failview ALTER COLLATION a REFRESH VERSION; -- ERROR
+ERROR: "failview" is not an index
+DROP VIEW failview;
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index bd5fe60450..a507ad37be 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -72,6 +72,9 @@ ERROR: cannot change data type of view column "b" from integer to numeric
-- should work
CREATE OR REPLACE VIEW viewtest AS
SELECT a, b, 0 AS c FROM viewtest_tbl;
+-- check that the command correctly complains for the object of a wrong type
+CREATE OR REPLACE VIEW view_base_table AS SELECT 1 AS a;
+ERROR: "view_base_table" is not a view
DROP VIEW viewtest;
DROP TABLE viewtest_tbl;
-- tests for temporary views
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index b9e25820bc..ffa9287967 100644
--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -877,8 +877,10 @@ ERROR: column "no_column" of relation "ft1" does not exist
ALTER FOREIGN TABLE ft1 DROP COLUMN IF EXISTS no_column;
NOTICE: column "no_column" of relation "ft1" does not exist, skipping
ALTER FOREIGN TABLE ft1 DROP COLUMN c9;
+ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR (wrong object type)
+ERROR: "ft1" is not a table, materialized view, index, or partitioned index
ALTER FOREIGN TABLE ft1 SET SCHEMA foreign_schema;
-ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR
+ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR (not found)
ERROR: relation "ft1" does not exist
ALTER FOREIGN TABLE foreign_schema.ft1 RENAME c1 TO foreign_column_1;
ALTER FOREIGN TABLE foreign_schema.ft1 RENAME TO foreign_table_1;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 4cc55d8525..1b70458790 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -9,6 +9,8 @@ RESET client_min_messages;
CREATE USER regress_alter_table_user1;
+CREATE VIEW at_v1 AS SELECT 1 as a;
+
--
-- add attribute
--
@@ -158,6 +160,8 @@ ALTER INDEX attmp_idx ALTER COLUMN 2 SET STATISTICS -1;
DROP TABLE attmp;
+-- test that the command correctly complains for the object of a wrong type
+ALTER TABLE at_v1 ALTER COLUMN a SET STATISTICS 0; -- ERROR
--
-- rename - check on both non-temp and temp tables
@@ -916,6 +920,11 @@ select * from def_test;
alter table def_test alter column c1 set default 'wrong_datatype';
alter table def_test alter column c2 set default 20;
+-- set defaults to an incorrect object: this should fail
+create materialized view def_tmp_mv as select 1 as a;
+alter table def_tmp_mv alter a set default 0;
+drop materialized view def_tmp_mv;
+
-- set defaults on a non-existent column: this should fail
alter table def_test alter column c3 set default 30;
@@ -1409,6 +1418,10 @@ alter table at_partitioned attach partition at_part_2 for values from (1000) to
alter table at_partitioned alter column b type numeric using b::numeric;
\d at_part_1
\d at_part_2
+
+-- check if the command correctly complains for the object of a wrong type
+alter table at_partitioned_a_idx set (dummy = 1); -- ERROR
+
drop table at_partitioned;
-- Alter column type when no table rewrite is required
@@ -1500,6 +1513,9 @@ alter table test_storage alter column a set storage external;
\d+ test_storage
\d+ test_storage_idx
+-- test that SET STORAGE correctly complains for the object of a wrong type
+alter table at_v1 alter column a set storage plain; -- ERROR
+
-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
@@ -1721,6 +1737,9 @@ begin; alter table alterlock set (autovacuum_enabled = off, fillfactor = 80);
select * from my_locks order by 1;
commit;
+-- test that the command corectly complains for the object of a wrong type
+alter table at_v1 alter column a set (dummy = 1);
+
begin; alter table alterlock alter column f2 set storage extended;
select * from my_locks order by 1;
rollback;
@@ -2640,6 +2659,9 @@ ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 8, R
ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3, REMAINDER 2);
DROP TABLE fail_part;
+-- check that attach partition correctly complains for the object of a wrong type
+ALTER TABLE at_v1 ATTACH PARTITION dummy default; -- ERROR
+
--
-- DETACH PARTITION
--
@@ -2852,6 +2874,11 @@ drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- check that the command correctly complains for the object of a wrong type
+create table attmp();
+alter table attmp options (dummy '1');
+drop table attmp;
+
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..827a15b305 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -257,7 +257,12 @@ EXPLAIN (COSTS OFF) SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
SELECT * FROM clstr_expression WHERE -a = -3 ORDER BY -a, b;
COMMIT;
+-- check that the command correctly complains for the object of a wrong type
+CREATE VIEW clstr_tst_view AS SELECT 1;
+ALTER TABLE clstr_tst_view CLUSTER ON x; -- ERROR
+
-- clean up
+DROP VIEW clstr_tst_view;
DROP TABLE clustertest;
DROP TABLE clstr_1;
DROP TABLE clstr_2;
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 0de2ed8d85..583c671c34 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -875,6 +875,13 @@ SELECT objid::regclass, refobjversion = 'not a version' AS ver FROM pg_depend
WHERE refclassid = 'pg_collation'::regclass
AND objid::regclass::text = 'icuidx17_part';
+-- Test that ALTER COLLATION REFRESH VERSION correctly complains for
+-- wrong object. We use ALTER TABLE, not ALTER INDEX since we are
+-- exercising ATWrongRelkindError here.
+CREATE VIEW failview AS SELECT 1 AS a;
+ALTER TABLE failview ALTER COLLATION a REFRESH VERSION; -- ERROR
+DROP VIEW failview;
+
-- cleanup
RESET search_path;
SET client_min_messages TO warning;
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index fbd1313b9c..8fbbe05c56 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -77,6 +77,9 @@ CREATE OR REPLACE VIEW viewtest AS
CREATE OR REPLACE VIEW viewtest AS
SELECT a, b, 0 AS c FROM viewtest_tbl;
+-- check that the command correctly complains for the object of a wrong type
+CREATE OR REPLACE VIEW view_base_table AS SELECT 1 AS a;
+
DROP VIEW viewtest;
DROP TABLE viewtest_tbl;
diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql
index 73f9f621d8..e96aef5396 100644
--- a/src/test/regress/sql/foreign_data.sql
+++ b/src/test/regress/sql/foreign_data.sql
@@ -406,8 +406,9 @@ ALTER FOREIGN TABLE ft1 OPTIONS (DROP delimiter, SET quote '~', ADD escape '@');
ALTER FOREIGN TABLE ft1 DROP COLUMN no_column; -- ERROR
ALTER FOREIGN TABLE ft1 DROP COLUMN IF EXISTS no_column;
ALTER FOREIGN TABLE ft1 DROP COLUMN c9;
+ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR (wrong object type)
ALTER FOREIGN TABLE ft1 SET SCHEMA foreign_schema;
-ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR
+ALTER FOREIGN TABLE ft1 SET TABLESPACE ts; -- ERROR (not found)
ALTER FOREIGN TABLE foreign_schema.ft1 RENAME c1 TO foreign_column_1;
ALTER FOREIGN TABLE foreign_schema.ft1 RENAME TO foreign_table_1;
\d foreign_schema.foreign_table_1
Hi Horiguchi-san, On Fri, Feb 19, 2021 at 05:30:39PM +0900, Kyotaro Horiguchi wrote: > The attached is that. > > ATT_VIEW is used for "CREATE OR REPLACE view" and checked against > earlier in DefineVirtualRelation. But we can add a test to make sure > that is checked anywhere. My apologies for not coming back to this thread earlier. I have this thread in my backlog for some time now but I was not able to come back to it. That's too late for v14 but it could be possible to do something for v15. Could you add this patch to the next commit fest? That's fine to add my name as reviewer. Thanks, -- Michael
Attachment
At Thu, 22 Apr 2021 13:48:45 +0900, Michael Paquier <michael@paquier.xyz> wrote in > Hi Horiguchi-san, > > On Fri, Feb 19, 2021 at 05:30:39PM +0900, Kyotaro Horiguchi wrote: > > The attached is that. > > > > ATT_VIEW is used for "CREATE OR REPLACE view" and checked against > > earlier in DefineVirtualRelation. But we can add a test to make sure > > that is checked anywhere. > > My apologies for not coming back to this thread earlier. I have this > thread in my backlog for some time now but I was not able to come back > to it. That's too late for v14 but it could be possible to do > something for v15. Could you add this patch to the next commit fest? > That's fine to add my name as reviewer. Thank you for kindly telling me that, but please don't worry. I'll add it to the next CF, with specifying you as a reviewer as you told. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested I have tested it with various object types and getting a meaningful error.
At Tue, 29 Jun 2021 20:13:14 +0000, ahsan hadi <ahsan.hadi@gmail.com> wrote in > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: not tested > > I have tested it with various object types and getting a meaningful error. Thanks for looking this, Ahsan. However, Peter-E is proposing a change at a fundamental level, which looks more promising (disregarding backpatch burden). https://www.postgresql.org/message-id/01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Jun 30, 2021 at 5:56 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Tue, 29 Jun 2021 20:13:14 +0000, ahsan hadi <ahsan.hadi@gmail.com> wrote in
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: not tested
>
> I have tested it with various object types and getting a meaningful error.
Thanks for looking this, Ahsan.
However, Peter-E is proposing a change at a fundamental level, which
looks more promising (disregarding backpatch burden).
https://www.postgresql.org/message-id/01d4fd55-d4fe-5afc-446c-a7f99e043f3d@enterprisedb.com
Sure I will also take a look at this patch.
+1 for avoiding the backpatching burden.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.hadi@highgo.ca
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: ahsan.hadi@highgo.ca
On Wed, Jun 30, 2021 at 01:43:52PM +0500, Ahsan Hadi wrote: > Sure I will also take a look at this patch. > > +1 for avoiding the backpatching burden. From what I recall of this thread, nobody has really complained about this stuff either, so a backpatch would be off the table. I agree that what Peter E is proposing on the other thread is much more suitable in the long term, as there is no need to worry about multiple combinations of relkinds in error message, so such error strings become a no-brainer when more relkinds are added. -- Michael
Attachment
On 02.07.21 06:20, Michael Paquier wrote: > On Wed, Jun 30, 2021 at 01:43:52PM +0500, Ahsan Hadi wrote: >> Sure I will also take a look at this patch. >> >> +1 for avoiding the backpatching burden. > > From what I recall of this thread, nobody has really complained about > this stuff either, so a backpatch would be off the table. I agree > that what Peter E is proposing on the other thread is much more > suitable in the long term, as there is no need to worry about multiple > combinations of relkinds in error message, so such error strings > become a no-brainer when more relkinds are added. My patch is now committed. The issue that started this thread now behaves like this: ALTER TABLE ft1 ATTACH PARTITION ...; ERROR: ALTER action ATTACH PARTITION cannot be performed on relation "ft1" DETAIL: This operation is not supported for foreign tables. So, for PG15, this is taken care of. Backpatches under the old style for missing combinations would still be in scope, but there my comment on the proposed patches is that I would rather not remove apparently unused combinations from back branches.
At Thu, 8 Jul 2021 10:02:53 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in
> My patch is now committed. The issue that started this thread now behaves
> like this:
>
> ALTER TABLE ft1 ATTACH PARTITION ...;
> ERROR: ALTER action ATTACH PARTITION cannot be performed on relation "ft1"
> DETAIL: This operation is not supported for foreign tables.
>
> So, for PG15, this is taken care of.
Cool.
> Backpatches under the old style for missing combinations would still be in
> scope, but there my comment on the proposed patches is that I would rather not
> remove apparently unused combinations from back branches.
Sounds reasonable. So the attached are that for PG11-PG14. 11 and 12
shares the same patch.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From f04c1968c0305142ab81e80a193352e1037ec4ef Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 17:55:25 +0900
Subject: [PATCH v2] Add missing targets in ATWrongRelkindError
ATWrongRelkindError yields an ambiguous message ""tbl" is of the wrong
type" due the lack of some combinations of allowed_targets. Add the
missing items. We don't remove apparently unused entries in case
someone is using them.
---
src/backend/commands/tablecmds.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 97a9725df7..3432f9bdd3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6029,6 +6029,12 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
msg = _("\"%s\" is not a table, materialized view, or index");
break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
+ break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, materialized view, index, partitioned index, or foreign table");
+ break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
break;
@@ -6041,9 +6047,15 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
+ case ATT_INDEX:
+ msg = _("\"%s\" is not an index");
+ break;
case ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a foreign table");
break;
--
2.27.0
From b1fc39633ba1053a4933f89e70369a0cef966840 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 18:01:19 +0900
Subject: [PATCH v2] Add missing targets in ATWrongRelkindError
ATWrongRelkindError yields an ambiguous message ""tbl" is of the wrong
type" due the lack of some combinations of allowed_targets. Add the
missing items. We don't remove apparently unused entries in case
someone is using them.
---
src/backend/commands/tablecmds.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 135fa46981..1bf4ca6884 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5649,6 +5649,12 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
msg = _("\"%s\" is not a table, materialized view, or index");
break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
+ break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX | ATT_FOREIGN_TABLE:
+ msg = _("\"%s\" is not a table, materialized view, index, partitioned index, or foreign table");
+ break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
break;
@@ -5661,6 +5667,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
--
2.27.0
From 71f6da9183e6d41d2f787f6a591772956a2b53df Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Thu, 8 Jul 2021 18:03:17 +0900
Subject: [PATCH v2] Add missing targets in ATWrongRelkindError
ATWrongRelkindError yields an ambiguous message ""tbl" is of the wrong
type" due the lack of some combinations of allowed_targets. Add the
missing items. We don't remove apparently unused entries in case
someone is using them.
---
src/backend/commands/tablecmds.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 816ce8521f..8dc43195a5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5372,6 +5372,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX:
msg = _("\"%s\" is not a table, materialized view, or index");
break;
+ case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table, materialized view, index, or partitioned index");
+ break;
case ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, or foreign table");
break;
@@ -5384,6 +5387,9 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
case ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE:
msg = _("\"%s\" is not a table, materialized view, index, or foreign table");
break;
+ case ATT_TABLE | ATT_PARTITIONED_INDEX:
+ msg = _("\"%s\" is not a table or partitioned index");
+ break;
case ATT_VIEW:
msg = _("\"%s\" is not a view");
break;
--
2.27.0