At Fri, 9 Jul 2021 11:03:56 +0900, Michael Paquier <mich...@paquier.xyz> wrote
in
> On Fri, Jul 09, 2021 at 10:44:13AM +0900, Kyotaro Horiguchi wrote:
> > Sounds reasonable. So the attached are that for PG11-PG14. 11 and 12
> > shares the same patch.
>
> How much do the regression tests published upthread in
> https://postgr.es/m/20210219.173039.609314751334535042.horikyota....@gmail.com
> apply here? Shouldn't we also have some regression tests for the new
> error cases you are adding? I agree that we'd better avoid removing
Mmm. Ok, I distributed the mother regression test into each version.
PG11, 12:
- ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
Added.
- ATT_TABLE | ATT_PARTITIONED_INDEX
This test doesn't detect the "is of the wrong type" issue.
The item is practically a dead one since the combination is caught
by transformPartitionCmd before visiting ATPrepCmd, which emits a
bit different error message for the test.
"\"%s\" is not a partitioned table or index"
ATPrepCmd emits an error that:
"\"%s\" is not a table or partitioned index"
Hmm.. somewhat funny. Actually ATT_TABLE is a bit off here but
there's no symbol ATT_PARTITIONED_TABLE. Theoretically the symbol
is needed but practically not. I don't think we need to do more
than that at least for these versions. (Or we don't even need to
add this item.)
PG13:
- ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
Same to PG12.
- ATT_TABLE | ATT_PARTITIONED_INDEX:
This version raches this item in ATPrepCmd because the commit
1281a5c907 moved the parse-transform phase to the ATExec stage,
which is visited after ATPrepCmd.
On the other hand, when the target relation is a regular table, the
error is missed by ATPrepCmd then the control reaches to the
Exec-stage. The error is finally aught by transformPartitionCmd.
Of course this works fine but doesn't seem clean, but it is
apparently a matter of the master branch.
- ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX |
ATT_FOREIGN_TABLE
Added and works as expected.
PG14:
- ATT_INDEX
I noticed that this combination has been reverted by the commit
ec48314708.
- ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX
- ATT_TABLE | ATT_PARTITIONED_INDEX:
- ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_PARTITIONED_INDEX |
ATT_FOREIGN_TABLE
Same as PG13.
So, PG14 and 13 share the same fix and test.
> error cases you are adding? I agree that we'd better avoid removing
> those entries, one argument in favor of not removing any entries being
> that this could have an impact on forks.
Ok. The attached are the two patchsets for PG14-13 and PG12-11
containing the fix and the regression test.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 80fa3ae43f5697a562b2ba460a50cdedf8d53e55 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 8 Jul 2021 17:55:25 +0900
Subject: [PATCH v3 1/2] 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 97a9725df7..d02e01cb20 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,6 +6047,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 3fc4f789006cb6f6239dda7e6f7c817f9da41812 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Fri, 9 Jul 2021 20:19:23 +0900
Subject: [PATCH v3 2/2] regress - Add missing targets in ATWrongRelkindError
---
src/test/regress/expected/alter_table.out | 7 +++++++
src/test/regress/expected/foreign_data.out | 4 +++-
src/test/regress/sql/alter_table.sql | 6 ++++++
src/test/regress/sql/foreign_data.sql | 3 ++-
4 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index f81bdf513b..b63b4d34b0 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
--
@@ -4111,6 +4115,9 @@ ALTER TABLE hash_parted ATTACH PARTITION fail_part FOR VALUES WITH (MODULUS 3, R
ERROR: every hash partition modulus must be a factor of the next larger modulus
DETAIL: The new modulus 3 is not a factor of 4, the modulus of existing partition "hpart_1".
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
--
diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out
index 5385f98a0f..0f56cae353 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 dc0200adcb..0c13478e3e 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -8,6 +8,7 @@ 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
@@ -158,6 +159,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
@@ -2640,6 +2643,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
--
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
--
2.27.0
>From 71f6da9183e6d41d2f787f6a591772956a2b53df Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 8 Jul 2021 18:03:17 +0900
Subject: [PATCH v3 1/2] 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
>From 18c3101d4f2dc95c917a19c82757bd6f4b81fa2a Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Fri, 9 Jul 2021 20:40:52 +0900
Subject: [PATCH v3 2/2] regress - Add missing targets in ATWrongRelkindError
---
src/test/regress/expected/alter_table.out | 4 ++++
src/test/regress/expected/foreign_data.out | 4 +++-
src/test/regress/sql/alter_table.sql | 4 ++++
src/test/regress/sql/foreign_data.sql | 3 ++-
4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 8d91e2d6c8..5f6d4ed7c3 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
--
@@ -3948,6 +3949,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 partitioned table or index
--
-- DETACH PARTITION
--
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 cd925ea8ce..a92cea7f87 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -8,6 +8,7 @@ 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
@@ -2593,6 +2594,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
--
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
--
2.27.0
