On Wed, Dec 25, 2024 at 5:29 PM Michael Paquier <[email protected]> wrote:
>
> On Mon, Dec 16, 2024 at 05:25:45PM +0800, jian he wrote:
> > i've split into 3 patches, feel free to merge them in any way.
> > v12-0001: add error position for ATPrepAlterColumnType.
>
> For this one, why don't you do the same for undefined columns and
> USING with generated columns at least? This looks half-baked.
>
I think I understand what you mean.
please check attached for changes within ATPrepAlterColumnType
> > v12-0002: add error position for these 3 functions:
> > transformColumnDefinition, transformAlterTableStmt,
> > transformTableConstraint.
>
> ERROR: column "c" of relation "itest4" does not exist
> +LINE 1: ALTER TABLE itest4 ALTER COLUMN c ADD GENERATED ALWAYS AS ID...
> + ^
>
> This one is kind of confusing? The part that matters for the error is
> the column that does not exist, not the ADD GENERATED.
>
I agree this is confusing.
while looking at 0002:
errmsg("conflicting NO INHERIT declarations for not-null constraints
on column \"%s\"", column->colname)
add parser_errposition
will not be very helpful. i think we need an errhint.
for example:
create table notnull_tbl_fail (a int primary key constraint foo not
null no inherit);
ERROR: conflicting NO INHERIT declarations for not-null constraints
on column "a"
the error message didn't explicitly say that the primary key imply a
not-null inherit constraint.
Maybe we can change to
errmsg("conflicting NO INHERIT declarations for not-null constraints
on column \"%s\"", column->colname),
errhint("specified primary key or identity sequence imply an inherited
not-null constraint will be created")
what do you think?
From a8055a5292a57e111790629d418fe1663e1ba8ec Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Fri, 27 Dec 2024 13:36:06 +0800
Subject: [PATCH v13 1/1] add error position for ATPrepAlterColumnType
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
within ATPrepAlterColumnType, for various
ereport(ERROR...), add parser_errposition for printing out the error position.
Author: Kirill Reshke <[email protected]>
Author: Jian He <[email protected]>
Reviewed-By: Michaël Paquier <[email protected]>
Reviewed-By: Álvaro Herrera <[email protected]>
discussion: https://postgr.es/m/CALdSSPhqfvKbDwqJaY=yEePi_aq61GmMpW88i6ZH7CMG_2Z4Cg@mail.gmail.com
---
src/backend/commands/tablecmds.c | 25 +++++++++++--------
src/test/regress/expected/alter_table.out | 14 +++++++++++
.../regress/expected/generated_stored.out | 2 ++
src/test/regress/expected/typed_table.out | 2 ++
4 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4937478262..f0acb008ea 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -13222,10 +13222,12 @@ ATPrepAlterColumnType(List **wqueue,
AclResult aclresult;
bool is_expr;
+ pstate->p_sourcetext = context->queryString;
if (rel->rd_rel->reloftype && !recursing)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("cannot alter column type of typed table")));
+ errmsg("cannot alter column type of typed table"),
+ parser_errposition(pstate, def->location)));
/* lookup the attribute so we can check inheritance status */
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
@@ -13233,7 +13235,8 @@ ATPrepAlterColumnType(List **wqueue,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
- colName, RelationGetRelationName(rel))));
+ colName, RelationGetRelationName(rel)),
+ parser_errposition(pstate, def->location)));
attTup = (Form_pg_attribute) GETSTRUCT(tuple);
attnum = attTup->attnum;
@@ -13241,8 +13244,8 @@ ATPrepAlterColumnType(List **wqueue,
if (attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot alter system column \"%s\"",
- colName)));
+ errmsg("cannot alter system column \"%s\"",colName),
+ parser_errposition(pstate, def->location)));
/*
* Cannot specify USING when altering type of a generated column, because
@@ -13252,7 +13255,8 @@ ATPrepAlterColumnType(List **wqueue,
ereport(ERROR,
(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
errmsg("cannot specify USING when altering type of generated column"),
- errdetail("Column \"%s\" is a generated column.", colName)));
+ errdetail("Column \"%s\" is a generated column.", colName),
+ parser_errposition(pstate, def->location)));
/*
* Don't alter inherited columns. At outer level, there had better not be
@@ -13262,8 +13266,8 @@ ATPrepAlterColumnType(List **wqueue,
if (attTup->attinhcount > 0 && !recursing)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("cannot alter inherited column \"%s\"",
- colName)));
+ errmsg("cannot alter inherited column \"%s\"", colName),
+ parser_errposition(pstate, def->location)));
/* Don't alter columns used in the partition key */
if (has_partition_attrs(rel,
@@ -13272,17 +13276,18 @@ ATPrepAlterColumnType(List **wqueue,
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot alter column \"%s\" because it is part of the partition key of relation \"%s\"",
- colName, RelationGetRelationName(rel))));
+ colName, RelationGetRelationName(rel)),
+ parser_errposition(pstate, def->location)));
/* Look up the target type */
- typenameTypeIdAndMod(NULL, typeName, &targettype, &targettypmod);
+ typenameTypeIdAndMod(pstate, typeName, &targettype, &targettypmod);
aclresult = object_aclcheck(TypeRelationId, targettype, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
aclcheck_error_type(aclresult, targettype);
/* And the collation */
- targetcollid = GetColumnDefCollation(NULL, def, targettype);
+ targetcollid = GetColumnDefCollation(pstate, def, targettype);
/* make sure datatype is legal for a column */
CheckAttributeType(colName, targettype, targetcollid,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 12852aa612..da74eabccc 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3416,10 +3416,16 @@ ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
-- Some error cases.
ALTER TABLE comment_test ALTER COLUMN xmin SET DATA TYPE x;
ERROR: cannot alter system column "xmin"
+LINE 1: ALTER TABLE comment_test ALTER COLUMN xmin SET DATA TYPE x;
+ ^
ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE x;
ERROR: type "x" does not exist
+LINE 1: ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE x;
+ ^
ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE int COLLATE "C";
ERROR: collations are not supported by type integer
+LINE 1: ...LE comment_test ALTER COLUMN id SET DATA TYPE int COLLATE "C...
+ ^
-- Check that the comments are intact.
SELECT col_description('comment_test'::regclass, 1) as comment;
comment
@@ -3885,10 +3891,14 @@ ALTER TABLE partitioned DROP COLUMN a;
ERROR: cannot drop column "a" because it is part of the partition key of relation "partitioned"
ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
ERROR: cannot alter column "a" because it is part of the partition key of relation "partitioned"
+LINE 1: ALTER TABLE partitioned ALTER COLUMN a TYPE char(5);
+ ^
ALTER TABLE partitioned DROP COLUMN b;
ERROR: cannot drop column "b" because it is part of the partition key of relation "partitioned"
ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
ERROR: cannot alter column "b" because it is part of the partition key of relation "partitioned"
+LINE 1: ALTER TABLE partitioned ALTER COLUMN b TYPE char(5);
+ ^
-- specifying storage parameters for partitioned tables is not supported
ALTER TABLE partitioned SET (fillfactor=100);
ERROR: cannot specify storage parameters for a partitioned table
@@ -4413,6 +4423,8 @@ ALTER TABLE part_2 RENAME COLUMN b to c;
ERROR: cannot rename inherited column "b"
ALTER TABLE part_2 ALTER COLUMN b TYPE text;
ERROR: cannot alter inherited column "b"
+LINE 1: ALTER TABLE part_2 ALTER COLUMN b TYPE text;
+ ^
-- cannot add NOT NULL or check constraints to *only* the parent, when
-- partitions exist
ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL;
@@ -4474,6 +4486,8 @@ ALTER TABLE list_parted2 DROP COLUMN b;
ERROR: cannot drop column "b" because it is part of the partition key of relation "part_5"
ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
ERROR: cannot alter column "b" because it is part of the partition key of relation "part_5"
+LINE 1: ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
+ ^
-- dropping non-partition key columns should be allowed on the parent table.
ALTER TABLE list_parted DROP COLUMN b;
SELECT * FROM list_parted;
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index 0d037d48ca..0dac2a7174 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1032,6 +1032,8 @@ SELECT * FROM gtest27;
ALTER TABLE gtest27 ALTER COLUMN x TYPE boolean USING x <> 0; -- error
ERROR: cannot specify USING when altering type of generated column
+LINE 1: ALTER TABLE gtest27 ALTER COLUMN x TYPE boolean USING x <> 0...
+ ^
DETAIL: Column "x" is a generated column.
ALTER TABLE gtest27 ALTER COLUMN x DROP DEFAULT; -- error
ERROR: column "x" of relation "gtest27" is a generated column
diff --git a/src/test/regress/expected/typed_table.out b/src/test/regress/expected/typed_table.out
index aa6150b853..885f085e15 100644
--- a/src/test/regress/expected/typed_table.out
+++ b/src/test/regress/expected/typed_table.out
@@ -38,6 +38,8 @@ ALTER TABLE persons RENAME COLUMN id TO num;
ERROR: cannot rename column of typed table
ALTER TABLE persons ALTER COLUMN name TYPE varchar;
ERROR: cannot alter column type of typed table
+LINE 1: ALTER TABLE persons ALTER COLUMN name TYPE varchar;
+ ^
CREATE TABLE stuff (id int);
ALTER TABLE persons INHERIT stuff;
ERROR: cannot change inheritance of typed table
--
2.34.1
