hi.
ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
i changed the above function, so the above related function errors may
print out error position.
reason for change mainly because these functions have
`typenameType(NULL, typeName, &targettypmod);`
we want to pass not NULL pstate (typenameType(pstate, typeName, &targettypmod);)
why do we want printout error position
1. it can quickly locate DDL command error positions, beginner friendly.
2. in the thread `CREATE SCHEMA ... CREATE DOMAIN support`, case like:
CREATE SCHEMA regress_schema_2
create domain ss1 as ss
create domain ss as text;
ERROR: type "ss" does not exist
obviously the error is not helpful at all.
As you can see, in cases like a single DDL, multiple sub DDL within
it, error position is quite important
I also added some tests for DefineDomain.
added parser_errposition for many places in DefineDomain.
the attached patch (based on Kirill Reshke 's v2 patch)
either passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.
then add
`parser_errposition(pstate, location)))`
in various places optionally.
So I guess bundling it into a single patch should be fine?
From 181b362c66a124721c56ddd461e44158283e6548 Mon Sep 17 00:00:00 2001
From: jian he <jian.universal...@gmail.com>
Date: Sat, 30 Nov 2024 14:44:38 +0800
Subject: [PATCH v3 1/1] print out error position for some DDL command.
doing this by passing the source_string to the existing ParseState
or by making a new ParseState passing source_string to it.
With this patch, the following functions will printout the error position for certain error cases.
ATExecAddOf
DefineType
ATPrepAlterColumnType
ATExecAlterColumnType
DefineDomain
AlterType
This can be particularly helpful when working with a sequence of DML commands,
such as `create schema create schema_element`.
It also makes it easier to quickly identify the relevant error area in a single DDL command.
---
src/backend/commands/tablecmds.c | 43 +++++++++++++-------
src/backend/commands/typecmds.c | 49 +++++++++++++----------
src/backend/parser/parse_utilcmd.c | 2 +-
src/backend/tcop/utility.c | 4 +-
src/include/commands/typecmds.h | 4 +-
src/test/regress/expected/alter_table.out | 13 ++++++
src/test/regress/expected/domain.out | 17 ++++++++
src/test/regress/sql/alter_table.sql | 5 +++
src/test/regress/sql/domain.sql | 5 +++
9 files changed, 102 insertions(+), 40 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 6ccae4cb4a..efa38b1470 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -593,7 +593,8 @@ static void ATPrepAlterColumnType(List **wqueue,
AlterTableUtilityContext *context);
static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode);
+ AlterTableCmd *cmd, LOCKMODE lockmode,
+ AlterTableUtilityContext *context);
static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype,
Relation rel, AttrNumber attnum, const char *colName);
static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab);
@@ -639,7 +640,9 @@ static ObjectAddress ATExecAddInherit(Relation child_rel, RangeVar *parent, LOCK
static ObjectAddress ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode);
static void drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
DependencyType deptype);
-static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode);
+static ObjectAddress ATExecAddOf(Relation rel, const TypeName *ofTypename,
+ LOCKMODE lockmode,
+ AlterTableUtilityContext *context);
static void ATExecDropOf(Relation rel, LOCKMODE lockmode);
static void ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode);
static void ATExecGenericOptions(Relation rel, List *options);
@@ -5413,7 +5416,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
break;
case AT_AlterColumnType: /* ALTER COLUMN TYPE */
/* parse transformation was done earlier */
- address = ATExecAlterColumnType(tab, rel, cmd, lockmode);
+ address = ATExecAlterColumnType(tab, rel, cmd, lockmode, context);
break;
case AT_AlterColumnGenericOptions: /* ALTER COLUMN OPTIONS */
address =
@@ -5537,7 +5540,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
address = ATExecDropInherit(rel, (RangeVar *) cmd->def, lockmode);
break;
case AT_AddOf:
- address = ATExecAddOf(rel, (TypeName *) cmd->def, lockmode);
+ address = ATExecAddOf(rel, (TypeName *) cmd->def, lockmode, context);
break;
case AT_DropOf:
ATExecDropOf(rel, lockmode);
@@ -13218,10 +13221,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);
@@ -13237,8 +13242,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
@@ -13271,14 +13276,14 @@ ATPrepAlterColumnType(List **wqueue,
colName, RelationGetRelationName(rel))));
/* 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,
@@ -13537,7 +13542,8 @@ ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
*/
static ObjectAddress
ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
- AlterTableCmd *cmd, LOCKMODE lockmode)
+ AlterTableCmd *cmd, LOCKMODE lockmode,
+ AlterTableUtilityContext *context)
{
char *colName = cmd->name;
ColumnDef *def = (ColumnDef *) cmd->def;
@@ -13558,6 +13564,10 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
SysScanDesc scan;
HeapTuple depTup;
ObjectAddress address;
+ ParseState *pstate;
+
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = context->queryString;
/*
* Clear all the missing values if we're rewriting the table, since this
@@ -13596,11 +13606,11 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
colName)));
/* Look up the target type (should not fail, since prep found it) */
- typeTuple = typenameType(NULL, typeName, &targettypmod);
+ typeTuple = typenameType(pstate, typeName, &targettypmod);
tform = (Form_pg_type) GETSTRUCT(typeTuple);
targettype = tform->oid;
/* And the collation */
- targetcollid = GetColumnDefCollation(NULL, def, targettype);
+ targetcollid = GetColumnDefCollation(pstate, def, targettype);
/*
* If there is a default expression for the column, get it and ensure we
@@ -16976,7 +16986,8 @@ drop_parent_dependency(Oid relid, Oid refclassid, Oid refobjid,
* The address of the type is returned.
*/
static ObjectAddress
-ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
+ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode,
+ AlterTableUtilityContext *context)
{
Oid relid = RelationGetRelid(rel);
Type typetuple;
@@ -16993,9 +17004,13 @@ ATExecAddOf(Relation rel, const TypeName *ofTypename, LOCKMODE lockmode)
ObjectAddress tableobj,
typeobj;
HeapTuple classtuple;
+ ParseState *pstate;
/* Validate the type. */
- typetuple = typenameType(NULL, ofTypename, NULL);
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = context->queryString;
+
+ typetuple = typenameType(pstate, ofTypename, NULL);
check_of_type(typetuple);
typeform = (Form_pg_type) GETSTRUCT(typetuple);
typeid = typeform->oid;
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 971a8a1ebc..0d0626d1ff 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -348,7 +348,7 @@ DefineType(ParseState *pstate, List *names, List *parameters)
Type likeType;
Form_pg_type likeForm;
- likeType = typenameType(NULL, defGetTypeName(likeTypeEl), NULL);
+ likeType = typenameType(pstate, defGetTypeName(likeTypeEl), NULL);
likeForm = (Form_pg_type) GETSTRUCT(likeType);
internalLength = likeForm->typlen;
byValue = likeForm->typbyval;
@@ -694,7 +694,7 @@ RemoveTypeById(Oid typeOid)
* Registers a new domain.
*/
ObjectAddress
-DefineDomain(CreateDomainStmt *stmt)
+DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
{
char *domainName;
char *domainArrayName;
@@ -761,7 +761,7 @@ DefineDomain(CreateDomainStmt *stmt)
/*
* Look up the base type.
*/
- typeTup = typenameType(NULL, stmt->typeName, &basetypeMod);
+ typeTup = typenameType(pstate, stmt->typeName, &basetypeMod);
baseType = (Form_pg_type) GETSTRUCT(typeTup);
basetypeoid = baseType->oid;
@@ -783,7 +783,8 @@ DefineDomain(CreateDomainStmt *stmt)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("\"%s\" is not a valid base type for a domain",
- TypeNameToString(stmt->typeName))));
+ TypeNameToString(stmt->typeName)),
+ parser_errposition(pstate, stmt->typeName->location)));
aclresult = object_aclcheck(TypeRelationId, basetypeoid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
@@ -809,7 +810,8 @@ DefineDomain(CreateDomainStmt *stmt)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("collations are not supported by type %s",
- format_type_be(basetypeoid))));
+ format_type_be(basetypeoid)),
+ parser_errposition(pstate, stmt->typeName->location)));
/* passed by value */
byValue = baseType->typbyval;
@@ -880,17 +882,14 @@ DefineDomain(CreateDomainStmt *stmt)
if (saw_default)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("multiple default expressions")));
+ errmsg("multiple default expressions"),
+ parser_errposition(pstate, constr->location)));
saw_default = true;
if (constr->raw_expr)
{
- ParseState *pstate;
Node *defaultExpr;
- /* Create a dummy ParseState for transformExpr */
- pstate = make_parsestate(NULL);
-
/*
* Cook the constr->raw_expr into an expression. Note:
* name is strictly for error message
@@ -943,11 +942,13 @@ DefineDomain(CreateDomainStmt *stmt)
if (nullDefined && !typNotNull)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting NULL/NOT NULL constraints")));
+ errmsg("conflicting NULL/NOT NULL constraints"),
+ parser_errposition(pstate, constr->location)));
if (constr->is_no_inherit)
ereport(ERROR,
- errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
- errmsg("not-null constraints for domains cannot be marked NO INHERIT"));
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("not-null constraints for domains cannot be marked NO INHERIT"),
+ parser_errposition(pstate, constr->location)));
typNotNull = true;
nullDefined = true;
break;
@@ -956,7 +957,8 @@ DefineDomain(CreateDomainStmt *stmt)
if (nullDefined && typNotNull)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("conflicting NULL/NOT NULL constraints")));
+ errmsg("conflicting NULL/NOT NULL constraints"),
+ parser_errposition(pstate, constr->location)));
typNotNull = false;
nullDefined = true;
break;
@@ -981,25 +983,29 @@ DefineDomain(CreateDomainStmt *stmt)
case CONSTR_UNIQUE:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("unique constraints not possible for domains")));
+ errmsg("unique constraints not possible for domains"),
+ parser_errposition(pstate, constr->location)));
break;
case CONSTR_PRIMARY:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("primary key constraints not possible for domains")));
+ errmsg("primary key constraints not possible for domains"),
+ parser_errposition(pstate, constr->location)));
break;
case CONSTR_EXCLUSION:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("exclusion constraints not possible for domains")));
+ errmsg("exclusion constraints not possible for domains"),
+ parser_errposition(pstate, constr->location)));
break;
case CONSTR_FOREIGN:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("foreign key constraints not possible for domains")));
+ errmsg("foreign key constraints not possible for domains"),
+ parser_errposition(pstate, constr->location)));
break;
case CONSTR_ATTR_DEFERRABLE:
@@ -1008,7 +1014,8 @@ DefineDomain(CreateDomainStmt *stmt)
case CONSTR_ATTR_IMMEDIATE:
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("specifying constraint deferrability not supported for domains")));
+ errmsg("specifying constraint deferrability not supported for domains"),
+ parser_errposition(pstate, constr->location)));
break;
default:
@@ -4315,7 +4322,7 @@ AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
* adding new flexibility.
*/
ObjectAddress
-AlterType(AlterTypeStmt *stmt)
+AlterType(ParseState *pstate, AlterTypeStmt *stmt)
{
ObjectAddress address;
Relation catalog;
@@ -4331,7 +4338,7 @@ AlterType(AlterTypeStmt *stmt)
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(stmt->typeName);
- tup = typenameType(NULL, typename, NULL);
+ tup = typenameType(pstate, typename, NULL);
typeOid = typeTypeId(tup);
typForm = (Form_pg_type) GETSTRUCT(tup);
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 0f324ee4e3..95dad76683 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1615,7 +1615,7 @@ transformOfType(CreateStmtContext *cxt, TypeName *ofTypename)
Assert(ofTypename);
- tuple = typenameType(NULL, ofTypename, NULL);
+ tuple = typenameType(cxt->pstate, ofTypename, NULL);
check_of_type(tuple);
ofTypeId = ((Form_pg_type) GETSTRUCT(tuple))->oid;
ofTypename->typeOid = ofTypeId; /* cached for later */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index f28bf37105..aa24283eff 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1712,7 +1712,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_CreateDomainStmt:
- address = DefineDomain((CreateDomainStmt *) parsetree);
+ address = DefineDomain(pstate, (CreateDomainStmt *) parsetree);
break;
case T_CreateConversionStmt:
@@ -1801,7 +1801,7 @@ ProcessUtilitySlow(ParseState *pstate,
break;
case T_AlterTypeStmt:
- address = AlterType((AlterTypeStmt *) parsetree);
+ address = AlterType(pstate, (AlterTypeStmt *) parsetree);
break;
case T_CommentStmt:
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index e1b02927c4..6e493b896c 100644
--- a/src/include/commands/typecmds.h
+++ b/src/include/commands/typecmds.h
@@ -23,7 +23,7 @@
extern ObjectAddress DefineType(ParseState *pstate, List *names, List *parameters);
extern void RemoveTypeById(Oid typeOid);
-extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
+extern ObjectAddress DefineDomain(ParseState *pstate, CreateDomainStmt *stmt);
extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
extern ObjectAddress DefineRange(ParseState *pstate, CreateRangeStmt *stmt);
extern ObjectAddress AlterEnum(AlterEnumStmt *stmt);
@@ -58,6 +58,6 @@ extern Oid AlterTypeNamespaceInternal(Oid typeOid, Oid nspOid,
bool errorOnTableType,
ObjectAddresses *objsMoved);
-extern ObjectAddress AlterType(AlterTypeStmt *stmt);
+extern ObjectAddress AlterType(ParseState *pstate, AlterTypeStmt *stmt);
#endif /* TYPECMDS_H */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 2212c8dbb5..ebcac07063 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3413,6 +3413,19 @@ ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE int;
ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE text;
ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE int;
ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
+--test error report position
+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
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 42b6559f9c..fcc50d0f89 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -15,6 +15,23 @@ NOTICE: drop cascades to type dependenttypetest
-- this should fail because already gone
drop domain domaindroptest cascade;
ERROR: type "domaindroptest" does not exist
+--test syntax.
+create domain d_fail as int default 1 default 2;
+ERROR: multiple default expressions
+LINE 1: create domain d_fail as int default 1 default 2;
+ ^
+create domain d_fail int4 DEFAULT 3 + 'h';
+ERROR: invalid input syntax for type integer: "h"
+LINE 1: create domain d_fail int4 DEFAULT 3 + 'h';
+ ^
+create domain d_fail int4 collate "C";
+ERROR: collations are not supported by type integer
+LINE 1: create domain d_fail int4 collate "C";
+ ^
+create domain d_fail as anyelement;
+ERROR: "anyelement" is not a valid base type for a domain
+LINE 1: create domain d_fail as anyelement;
+ ^
-- Test domain input.
-- Note: the point of checking both INSERT and COPY FROM is that INSERT
-- exercises CoerceToDomain while COPY exercises domain_in.
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 637e3dac38..f7d45932ae 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2145,6 +2145,11 @@ ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE text;
ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE int;
ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
+--test error report position
+ALTER TABLE comment_test ALTER COLUMN xmin SET DATA TYPE x;
+ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE x;
+ALTER TABLE 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;
SELECT indexrelid::regclass::text as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass ORDER BY 1, 2;
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index ee07b03174..0966b8570b 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -16,6 +16,11 @@ drop domain domaindroptest cascade;
-- this should fail because already gone
drop domain domaindroptest cascade;
+--test syntax.
+create domain d_fail as int default 1 default 2;
+create domain d_fail int4 DEFAULT 3 + 'h';
+create domain d_fail int4 collate "C";
+create domain d_fail as anyelement;
-- Test domain input.
--
2.34.1
