some more error location support
Started by Peter Eisentrautover 7 years ago12 messages
Here are three patches to add more detailed error location support to
some parts of CREATE TABLE (defaults, check constraints, partition
specifications) as well as the COPY command. Examples can be seen in
the regression test output.
The first two are low-hanging fruit, since all the information was
already collected and just needed to be passed through one last hop.
The third one is a bit more invasive; it adds location information to
the Value node, which is used in a variety of commands, so COPY is just
a start here.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Error-position-support-for-defaults-and-check-constr.patchtext/plain; charset=UTF-8; name=0001-Error-position-support-for-defaults-and-check-constr.patch; x-mac-creator=0; x-mac-type=0Download
From 002dccbbe1df65347034d41a9582093a0cc72ba4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 08:42:49 +0200
Subject: [PATCH 1/3] Error position support for defaults and check constraints
Add support for error position reporting for the expressions contained
in defaults and check constraint definitions. This currently works only
for CREATE TABLE, not ALTER TABLE, because the latter is not set up to
pass around the original query string.
---
src/backend/catalog/heap.c | 4 +++-
src/backend/commands/tablecmds.c | 9 +++++----
src/include/catalog/heap.h | 3 ++-
src/test/regress/output/constraints.source | 2 ++
4 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ac5a677c5f..910d3db063 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2460,7 +2460,8 @@ AddRelationNewConstraints(Relation rel,
List *newConstraints,
bool allow_merge,
bool is_local,
- bool is_internal)
+ bool is_internal,
+ const char *queryString)
{
List *cookedConstraints = NIL;
TupleDesc tupleDesc;
@@ -2489,6 +2490,7 @@ AddRelationNewConstraints(Relation rel,
* rangetable entry. We need a ParseState for transformExpr.
*/
pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
rte = addRangeTableEntryForRelation(pstate,
rel,
NULL,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2a12d64aeb..4761bb911e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -985,7 +985,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
*/
if (rawDefaults || stmt->constraints)
AddRelationNewConstraints(rel, rawDefaults, stmt->constraints,
- true, true, false);
+ true, true, false, queryString);
ObjectAddressSet(address, RelationRelationId, relationId);
@@ -5587,7 +5587,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* _list_ of defaults, but we just do one.
*/
AddRelationNewConstraints(rel, list_make1(rawEnt), NIL,
- false, true, false);
+ false, true, false, NULL);
/* Make the additional catalog changes visible */
CommandCounterIncrement();
@@ -6178,7 +6178,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
* _list_ of defaults, but we just do one.
*/
AddRelationNewConstraints(rel, list_make1(rawEnt), NIL,
- false, true, false);
+ false, true, false, NULL);
}
ObjectAddressSubSet(address, RelationRelationId,
@@ -7215,7 +7215,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
list_make1(copyObject(constr)),
recursing | is_readd, /* allow_merge */
!recursing, /* is_local */
- is_readd); /* is_internal */
+ is_readd, /* is_internal */
+ NULL);
/* we don't expect more than one constraint here */
Assert(list_length(newcons) <= 1);
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index c5e40ff017..b3e8fdd9c6 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -102,7 +102,8 @@ extern List *AddRelationNewConstraints(Relation rel,
List *newConstraints,
bool allow_merge,
bool is_local,
- bool is_internal);
+ bool is_internal,
+ const char *queryString);
extern void RelationClearMissing(Relation rel);
extern void SetAttrMissing(Oid relid, char *attname, char *value);
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index a6a1df18e7..e8389064b0 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -228,6 +228,8 @@ CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
altitude int,
CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
ERROR: system column "ctid" reference in check constraint is invalid
+LINE 3: CHECK (NOT (is_capital AND ctid::text = 'sys_col_check...
+ ^
--
-- Check inheritance of defaults and constraints
--
--
2.18.0
0002-Error-position-support-for-partition-specifications.patchtext/plain; charset=UTF-8; name=0002-Error-position-support-for-partition-specifications.patch; x-mac-creator=0; x-mac-type=0Download
From f5c665c6ecb385b47cb7e5de09f75f6ab1b3785b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 08:46:58 +0200
Subject: [PATCH 2/3] Error position support for partition specifications
Add support for error position reporting for partition specifications.
---
src/backend/commands/tablecmds.c | 16 +++++++++++-----
src/test/regress/expected/create_table.out | 6 ++++++
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4761bb911e..c187508595 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -478,7 +478,7 @@ static void RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid,
static void RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy);
-static void ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
+static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
List **partexprs, Oid *partopclass, Oid *partcollation, char strategy);
static void CreateInheritance(Relation child_rel, Relation parent_rel);
static void RemoveInheritance(Relation child_rel, Relation parent_rel);
@@ -875,6 +875,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
*/
if (stmt->partspec)
{
+ ParseState *pstate;
char strategy;
int partnatts;
AttrNumber partattrs[PARTITION_MAX_KEYS];
@@ -882,6 +883,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid partcollation[PARTITION_MAX_KEYS];
List *partexprs = NIL;
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
+
partnatts = list_length(stmt->partspec->partParams);
/* Protect fixed-size arrays here and in executor */
@@ -900,7 +904,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
stmt->partspec = transformPartitionSpec(rel, stmt->partspec,
&strategy);
- ComputePartitionAttrs(rel, stmt->partspec->partParams,
+ ComputePartitionAttrs(pstate, rel, stmt->partspec->partParams,
partattrs, &partexprs, partopclass,
partcollation, strategy);
@@ -13695,7 +13699,7 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
* Expressions in the PartitionElems must be parse-analyzed already.
*/
static void
-ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
+ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
List **partexprs, Oid *partopclass, Oid *partcollation,
char strategy)
{
@@ -13722,14 +13726,16 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in partition key does not exist",
- pelem->name)));
+ pelem->name),
+ parser_errposition(pstate, pelem->location)));
attform = (Form_pg_attribute) GETSTRUCT(atttuple);
if (attform->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("cannot use system column \"%s\" in partition key",
- pelem->name)));
+ pelem->name),
+ parser_errposition(pstate, pelem->location)));
partattrs[attn] = attform->attnum;
atttype = attform->atttypid;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 8927b21ba2..744b9990a6 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -328,11 +328,15 @@ CREATE TABLE partitioned (
a int
) PARTITION BY RANGE (b);
ERROR: column "b" named in partition key does not exist
+LINE 3: ) PARTITION BY RANGE (b);
+ ^
-- cannot use system columns in partition key
CREATE TABLE partitioned (
a int
) PARTITION BY RANGE (xmin);
ERROR: cannot use system column "xmin" in partition key
+LINE 3: ) PARTITION BY RANGE (xmin);
+ ^
-- functions in key must be immutable
CREATE FUNCTION immut_func (a int) RETURNS int AS $$ SELECT a + random()::int; $$ LANGUAGE SQL;
CREATE TABLE partitioned (
@@ -719,6 +723,8 @@ SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::reg
-- specify PARTITION BY for a partition
CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
ERROR: column "c" named in partition key does not exist
+LINE 1: ...TITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
+ ^
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
--
2.18.0
0003-Add-location-information-to-Value-nodes.patchtext/plain; charset=UTF-8; name=0003-Add-location-information-to-Value-nodes.patch; x-mac-creator=0; x-mac-type=0Download
From 9231a0ca46fdc32d543d1f3e3c86a9f11ecfe2be Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 10:50:49 +0200
Subject: [PATCH 3/3] Add location information to Value nodes
This allows better location information in several commands. This
change also adds better location reporting to the COPY command, in
particular pointers into its various column lists. Support for other
commands using Value nodes can be added.
---
src/backend/commands/copy.c | 22 ++++++++++++----------
src/backend/commands/trigger.c | 18 +++++++++---------
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c | 2 ++
src/backend/nodes/value.c | 4 ++++
src/backend/parser/gram.y | 4 +++-
src/backend/parser/parse_utilcmd.c | 10 ++++++----
src/include/nodes/value.h | 1 +
src/test/regress/expected/alter_table.out | 8 ++++++++
src/test/regress/expected/copy2.out | 2 ++
10 files changed, 48 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..1e0290236f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -328,7 +328,7 @@ static Datum CopyReadBinaryAttribute(CopyState cstate,
static void CopyAttributeOutText(CopyState cstate, char *string);
static void CopyAttributeOutCSV(CopyState cstate, char *string,
bool use_quote, bool single_attr);
-static List *CopyGetAttnums(TupleDesc tupDesc, Relation rel,
+static List *CopyGetAttnums(ParseState *pstate, TupleDesc tupDesc, Relation rel,
List *attnamelist);
static char *limit_printout_length(const char *str);
@@ -850,7 +850,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
tupDesc = RelationGetDescr(rel);
- attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
+ attnums = CopyGetAttnums(pstate, tupDesc, rel, stmt->attlist);
foreach(cur, attnums)
{
int attno = lfirst_int(cur) -
@@ -1583,7 +1583,7 @@ BeginCopy(ParseState *pstate,
}
/* Generate or convert list of attributes to process */
- cstate->attnumlist = CopyGetAttnums(tupDesc, cstate->rel, attnamelist);
+ cstate->attnumlist = CopyGetAttnums(pstate, tupDesc, cstate->rel, attnamelist);
num_phys_attrs = tupDesc->natts;
@@ -1601,7 +1601,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_quote);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_quote);
foreach(cur, attnums)
{
@@ -1624,7 +1624,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_notnull);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_notnull);
foreach(cur, attnums)
{
@@ -1647,7 +1647,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_null);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_null);
foreach(cur, attnums)
{
@@ -1671,7 +1671,7 @@ BeginCopy(ParseState *pstate,
cstate->convert_select_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->convert_select);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->convert_select);
foreach(cur, attnums)
{
@@ -4907,7 +4907,7 @@ CopyAttributeOutCSV(CopyState cstate, char *string,
* rel can be NULL ... it's only used for error reports.
*/
static List *
-CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
+CopyGetAttnums(ParseState *pstate, TupleDesc tupDesc, Relation rel, List *attnamelist)
{
List *attnums = NIL;
@@ -4931,7 +4931,8 @@ CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
foreach(l, attnamelist)
{
- char *name = strVal(lfirst(l));
+ Value *val = lfirst(l);
+ char *name = strVal(val);
int attnum;
int i;
@@ -4955,7 +4956,8 @@ CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
- name, RelationGetRelationName(rel))));
+ name, RelationGetRelationName(rel)),
+ parser_errposition(pstate, val->location)));
else
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 2436692eb8..1c5da78fdf 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -161,6 +161,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
Oid funcoid, Oid parentTriggerOid, Node *whenClause,
bool isInternal, bool in_partition)
{
+ ParseState *pstate;
int16 tgtype;
int ncolumns;
int16 *columns;
@@ -188,6 +189,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
char *newtablename = NULL;
bool partition_recurse;
+ /* Set up a pstate to parse with */
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
+
if (OidIsValid(relOid))
rel = heap_open(relOid, ShareRowExclusiveLock);
else
@@ -562,15 +567,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
*/
if (!whenClause && stmt->whenClause)
{
- ParseState *pstate;
RangeTblEntry *rte;
List *varList;
ListCell *lc;
- /* Set up a pstate to parse with */
- pstate = make_parsestate(NULL);
- pstate->p_sourcetext = queryString;
-
/*
* Set up RTEs for OLD and NEW references.
*
@@ -648,8 +648,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
whenRtable = pstate->p_rtable;
qual = nodeToString(whenClause);
-
- free_parsestate(pstate);
}
else if (!whenClause)
{
@@ -892,7 +890,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
columns = (int16 *) palloc(ncolumns * sizeof(int16));
foreach(cell, stmt->columns)
{
- char *name = strVal(lfirst(cell));
+ Value *val = lfirst(cell);
+ char *name = strVal(val);
int16 attnum;
int j;
@@ -902,7 +901,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
- name, RelationGetRelationName(rel))));
+ name, RelationGetRelationName(rel)),
+ parser_errposition(pstate, val->location)));
/* Check for duplicates */
for (j = i - 1; j >= 0; j--)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7c8220cf65..b9d8855b8b 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4738,6 +4738,7 @@ _copyValue(const Value *from)
(int) from->type);
break;
}
+ COPY_LOCATION_FIELD(location);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 378f2facb8..abf7701a28 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2975,6 +2975,8 @@ _equalValue(const Value *a, const Value *b)
break;
}
+ COMPARE_LOCATION_FIELD(location);
+
return true;
}
diff --git a/src/backend/nodes/value.c b/src/backend/nodes/value.c
index 2a30307baf..f41a5c0be7 100644
--- a/src/backend/nodes/value.c
+++ b/src/backend/nodes/value.c
@@ -26,6 +26,7 @@ makeInteger(int i)
v->type = T_Integer;
v->val.ival = i;
+ v->location = -1;
return v;
}
@@ -41,6 +42,7 @@ makeFloat(char *numericStr)
v->type = T_Float;
v->val.str = numericStr;
+ v->location = -1;
return v;
}
@@ -56,6 +58,7 @@ makeString(char *str)
v->type = T_String;
v->val.str = str;
+ v->location = -1;
return v;
}
@@ -71,5 +74,6 @@ makeBitString(char *str)
v->type = T_BitString;
v->val.str = str;
+ v->location = -1;
return v;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 4bd2223f26..e9a3a8efab 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3798,7 +3798,9 @@ columnList:
columnElem: ColId
{
- $$ = (Node *) makeString($1);
+ Value *v = makeString($1);
+ v->location = @1;
+ $$ = (Node *) v;
}
;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 656b1b5f1b..91531dd88d 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2148,7 +2148,8 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
{
foreach(lc, constraint->keys)
{
- char *key = strVal(lfirst(lc));
+ Value *val = lfirst(lc);
+ char *key = strVal(val);
bool found = false;
ColumnDef *column = NULL;
ListCell *columns;
@@ -2236,7 +2237,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in key does not exist", key),
- parser_errposition(cxt->pstate, constraint->location)));
+ parser_errposition(cxt->pstate, val->location)));
/* Check for PRIMARY KEY(foo, foo) */
foreach(columns, index->indexParams)
@@ -2275,7 +2276,8 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
/* Add included columns to index definition */
foreach(lc, constraint->including)
{
- char *key = strVal(lfirst(lc));
+ Value *val = lfirst(lc);
+ char *key = strVal(val);
bool found = false;
ColumnDef *column = NULL;
ListCell *columns;
@@ -2360,7 +2362,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in key does not exist", key),
- parser_errposition(cxt->pstate, constraint->location)));
+ parser_errposition(cxt->pstate, val->location)));
/* OK, add it to the index definition */
iparam = makeNode(IndexElem);
diff --git a/src/include/nodes/value.h b/src/include/nodes/value.h
index 1665714515..8583a9f874 100644
--- a/src/include/nodes/value.h
+++ b/src/include/nodes/value.h
@@ -47,6 +47,7 @@ typedef struct Value
int ival; /* machine integer */
char *str; /* string */
} val;
+ int location;
} Value;
#define intVal(v) (((Value *)(v))->val.ival)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 0218c2c362..a9d6b3aa03 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1485,8 +1485,12 @@ copy attest to stdout;
2 3
copy attest(a) to stdout;
ERROR: column "a" of relation "attest" does not exist
+LINE 1: copy attest(a) to stdout;
+ ^
copy attest("........pg.dropped.1........") to stdout;
ERROR: column "........pg.dropped.1........" of relation "attest" does not exist
+LINE 1: copy attest("........pg.dropped.1........") to stdout;
+ ^
copy attest from stdin;
ERROR: extra data after last expected column
CONTEXT: COPY attest, line 1: "10 11 12"
@@ -1506,8 +1510,12 @@ select * from attest;
copy attest(a) from stdin;
ERROR: column "a" of relation "attest" does not exist
+LINE 1: copy attest(a) from stdin;
+ ^
copy attest("........pg.dropped.1........") from stdin;
ERROR: column "........pg.dropped.1........" of relation "attest" does not exist
+LINE 1: copy attest("........pg.dropped.1........") from stdin;
+ ^
copy attest(b,c) from stdin;
select * from attest;
b | c
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index eb9e4b9774..171afed28c 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -28,6 +28,8 @@ COPY x (a, b, c, d, e) from stdin;
-- non-existent column in column list: should fail
COPY x (xyz) from stdin;
ERROR: column "xyz" of relation "x" does not exist
+LINE 1: COPY x (xyz) from stdin;
+ ^
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
--
2.18.0
Re: some more error location support
Hello Peter,
Here are three patches to add more detailed error location support to
some parts of CREATE TABLE (defaults, check constraints, partition
specifications) as well as the COPY command. Examples can be seen in
the regression test output.The first two are low-hanging fruit, since all the information was
already collected and just needed to be passed through one last hop.
The third one is a bit more invasive; it adds location information to
the Value node, which is used in a variety of commands, so COPY is just
a start here.
Patch 1 applies cleanly, compiles, "make check" is okay.
I noticed that you provide NULL from "ALTER TABLE" which is calling the
create table machinery:
postgres=# CREATE TABLE foo(id SERIAL CHECK (x = 0));
ERROR: column "x" does not exist
LINE 1: CREATE TABLE foo(id SERIAL CHECK (x = 0));
^
postgres=# CREATE TABLE foo();
CREATE TABLE
postgres=# ALTER TABLE foo ADD COLUMN id SERIAL CHECK (x = 0);
ERROR: column "x" does not exist
<no location>
Would it be easily possible to provide the query in that case as well?
--
Fabien.
Re: some more error location support
On 27/08/2018 10:41, Fabien COELHO wrote:
I noticed that you provide NULL from "ALTER TABLE" which is calling the
create table machinery:
The structure of the ALTER TABLE code is such that it would be quite
complicated to pass through the required state. It's doable, but would
be a bigger and independent project.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: some more error location support
I noticed that you provide NULL from "ALTER TABLE" which is calling the
create table machinery:The structure of the ALTER TABLE code is such that it would be quite
complicated to pass through the required state. It's doable, but would
be a bigger and independent project.
Ok, so no "easy" way about that.
I'd consider providing a comment about that.
--
Fabien.
Re: some more error location support
Hello Peter,
Here are three patches to add more detailed error location support to
some parts of CREATE TABLE (defaults, check constraints, partition
specifications) as well as the COPY command. Examples can be seen in
the regression test output.The first two are low-hanging fruit, since all the information was
already collected and just needed to be passed through one last hop.
The third one is a bit more invasive; it adds location information to
the Value node, which is used in a variety of commands, so COPY is just
a start here.
About patch 2: applies cleanly independently of the first one, compiles,
"make check" is ok.
There is a "make_parsestate", but no corresponding free. The usual
pattern, although there seems to be a few exception, is to "make" and
"free".
Even if there is some under-the-hood garbage collection, I'd suggest to
add a free after the call to ComputePartitionAttrs.
--
Fabien.
Re: some more error location support
Here are three patches to add more detailed error location support to
some parts of CREATE TABLE (defaults, check constraints, partition
specifications) as well as the COPY command. Examples can be seen in
the regression test output.The first two are low-hanging fruit, since all the information was
already collected and just needed to be passed through one last hop.
The third one is a bit more invasive; it adds location information to
the Value node, which is used in a variety of commands, so COPY is just
a start here.
About patch 3: applies cleanly independently of the 2 others, compiles,
"make check" is okay.
A few comments:
There seems to be several somehow unrelated changes: one about copy,
one about trigger and one about constraints? The two later changes do not
seem to impact the tests, though.
In "CreateTrigger", you moved "make_parsestate" but removed
"free_parsestate". I'd rather move it than remove it.
In "value.h", the added location field deserves a "/* token location, or
-1 if unknown */" comment like others in "parsenode.h", "plannode.h" and
"primnodes.h".
Copying and comparing values are updaed, but value in/out functions are
not updated to read/write the location, although other objects have their
location serialized. ISTM that it should be done as well.
--
Fabien.
Re: some more error location support
On 27/08/2018 10:53, Fabien COELHO wrote:
There is a "make_parsestate", but no corresponding free. The usual
pattern, although there seems to be a few exception, is to "make" and
"free".Even if there is some under-the-hood garbage collection, I'd suggest to
add a free after the call to ComputePartitionAttrs.
Hmm, I didn't know about free_parsestate(). It doesn't seem to be used
consistently. I suppose you'll want to use it when you have a target
relation that will be closed by it, but otherwise, for DDL commands,
it's not all that useful.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: some more error location support
Even if there is some under-the-hood garbage collection, I'd suggest to
add a free after the call to ComputePartitionAttrs.Hmm, I didn't know about free_parsestate(). It doesn't seem to be used
consistently. I suppose you'll want to use it when you have a target
relation that will be closed by it, but otherwise, for DDL commands,
it's not all that useful.
Probably.
The majority rule (34 make & 22 free) suggest that it is more often use
than not. I'd suggest to stick to that for consistency & homogeneity.
--
Fabien.
Re: some more error location support
On 28/08/2018 08:58, Fabien COELHO wrote:
Even if there is some under-the-hood garbage collection, I'd suggest to
add a free after the call to ComputePartitionAttrs.Hmm, I didn't know about free_parsestate(). It doesn't seem to be used
consistently. I suppose you'll want to use it when you have a target
relation that will be closed by it, but otherwise, for DDL commands,
it's not all that useful.Probably.
The majority rule (34 make & 22 free) suggest that it is more often use
than not. I'd suggest to stick to that for consistency & homogeneity.
But it's consistently not used in DDL command implementations, only in
normal query parsing.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: some more error location support
On 27/08/2018 11:17, Fabien COELHO wrote:
About patch 3: applies cleanly independently of the 2 others, compiles,
"make check" is okay.A few comments:
There seems to be several somehow unrelated changes: one about copy,
one about trigger and one about constraints? The two later changes do not
seem to impact the tests, though.
added more tests
In "CreateTrigger", you moved "make_parsestate" but removed
"free_parsestate". I'd rather move it than remove it.
See also previous discussion, but I've moved it around for now.
In "value.h", the added location field deserves a "/* token location, or
-1 if unknown */" comment like others in "parsenode.h", "plannode.h" and
"primnodes.h".
done
Copying and comparing values are updaed, but value in/out functions are
not updated to read/write the location, although other objects have their
location serialized. ISTM that it should be done as well.
Hmm, maybe that's a problem, because the serialization of a Value node
is just a scalar. It doesn't have any structure where to put additional
fields. Maybe we should think about not using Value as a parse
representation for column name lists. Let me think about that.
Attached is another patch set. I think the first two patches are OK
now, but the third one might need to be rethought.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Error-position-support-for-defaults-and-check-con.patchtext/plain; charset=UTF-8; name=v2-0001-Error-position-support-for-defaults-and-check-con.patch; x-mac-creator=0; x-mac-type=0Download
From 1816353610c6af345f72f4753cdb629c5304123d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 08:42:49 +0200
Subject: [PATCH v2 1/3] Error position support for defaults and check
constraints
Add support for error position reporting for the expressions contained
in defaults and check constraint definitions. This currently works only
for CREATE TABLE, not ALTER TABLE, because the latter is not set up to
pass around the original query string.
---
src/backend/catalog/heap.c | 4 +++-
src/backend/commands/tablecmds.c | 9 +++++----
src/include/catalog/heap.h | 3 ++-
src/test/regress/output/constraints.source | 2 ++
4 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ac5a677c5f..910d3db063 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2460,7 +2460,8 @@ AddRelationNewConstraints(Relation rel,
List *newConstraints,
bool allow_merge,
bool is_local,
- bool is_internal)
+ bool is_internal,
+ const char *queryString)
{
List *cookedConstraints = NIL;
TupleDesc tupleDesc;
@@ -2489,6 +2490,7 @@ AddRelationNewConstraints(Relation rel,
* rangetable entry. We need a ParseState for transformExpr.
*/
pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
rte = addRangeTableEntryForRelation(pstate,
rel,
NULL,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 2a12d64aeb..552ad8c592 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -985,7 +985,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
*/
if (rawDefaults || stmt->constraints)
AddRelationNewConstraints(rel, rawDefaults, stmt->constraints,
- true, true, false);
+ true, true, false, queryString);
ObjectAddressSet(address, RelationRelationId, relationId);
@@ -5587,7 +5587,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* _list_ of defaults, but we just do one.
*/
AddRelationNewConstraints(rel, list_make1(rawEnt), NIL,
- false, true, false);
+ false, true, false, NULL);
/* Make the additional catalog changes visible */
CommandCounterIncrement();
@@ -6178,7 +6178,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
* _list_ of defaults, but we just do one.
*/
AddRelationNewConstraints(rel, list_make1(rawEnt), NIL,
- false, true, false);
+ false, true, false, NULL);
}
ObjectAddressSubSet(address, RelationRelationId,
@@ -7215,7 +7215,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
list_make1(copyObject(constr)),
recursing | is_readd, /* allow_merge */
!recursing, /* is_local */
- is_readd); /* is_internal */
+ is_readd, /* is_internal */
+ NULL); /* queryString not available here */
/* we don't expect more than one constraint here */
Assert(list_length(newcons) <= 1);
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index c5e40ff017..b3e8fdd9c6 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -102,7 +102,8 @@ extern List *AddRelationNewConstraints(Relation rel,
List *newConstraints,
bool allow_merge,
bool is_local,
- bool is_internal);
+ bool is_internal,
+ const char *queryString);
extern void RelationClearMissing(Relation rel);
extern void SetAttrMissing(Oid relid, char *attname, char *value);
diff --git a/src/test/regress/output/constraints.source b/src/test/regress/output/constraints.source
index a6a1df18e7..e8389064b0 100644
--- a/src/test/regress/output/constraints.source
+++ b/src/test/regress/output/constraints.source
@@ -228,6 +228,8 @@ CREATE TABLE SYS_COL_CHECK_TBL (city text, state text, is_capital bool,
altitude int,
CHECK (NOT (is_capital AND ctid::text = 'sys_col_check_tbl')));
ERROR: system column "ctid" reference in check constraint is invalid
+LINE 3: CHECK (NOT (is_capital AND ctid::text = 'sys_col_check...
+ ^
--
-- Check inheritance of defaults and constraints
--
--
2.18.0
v2-0002-Error-position-support-for-partition-specificatio.patchtext/plain; charset=UTF-8; name=v2-0002-Error-position-support-for-partition-specificatio.patch; x-mac-creator=0; x-mac-type=0Download
From 7d69182adadef754676e29cd844f3cdebaef38c5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 08:46:58 +0200
Subject: [PATCH v2 2/3] Error position support for partition specifications
Add support for error position reporting for partition specifications.
---
src/backend/commands/tablecmds.c | 16 +++++++++++-----
src/test/regress/expected/create_table.out | 6 ++++++
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 552ad8c592..48743dbfa8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -478,7 +478,7 @@ static void RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid,
static void RangeVarCallbackForAlterRelation(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
static PartitionSpec *transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy);
-static void ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
+static void ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
List **partexprs, Oid *partopclass, Oid *partcollation, char strategy);
static void CreateInheritance(Relation child_rel, Relation parent_rel);
static void RemoveInheritance(Relation child_rel, Relation parent_rel);
@@ -875,6 +875,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
*/
if (stmt->partspec)
{
+ ParseState *pstate;
char strategy;
int partnatts;
AttrNumber partattrs[PARTITION_MAX_KEYS];
@@ -882,6 +883,9 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
Oid partcollation[PARTITION_MAX_KEYS];
List *partexprs = NIL;
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
+
partnatts = list_length(stmt->partspec->partParams);
/* Protect fixed-size arrays here and in executor */
@@ -900,7 +904,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
stmt->partspec = transformPartitionSpec(rel, stmt->partspec,
&strategy);
- ComputePartitionAttrs(rel, stmt->partspec->partParams,
+ ComputePartitionAttrs(pstate, rel, stmt->partspec->partParams,
partattrs, &partexprs, partopclass,
partcollation, strategy);
@@ -13695,7 +13699,7 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
* Expressions in the PartitionElems must be parse-analyzed already.
*/
static void
-ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
+ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNumber *partattrs,
List **partexprs, Oid *partopclass, Oid *partcollation,
char strategy)
{
@@ -13722,14 +13726,16 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in partition key does not exist",
- pelem->name)));
+ pelem->name),
+ parser_errposition(pstate, pelem->location)));
attform = (Form_pg_attribute) GETSTRUCT(atttuple);
if (attform->attnum <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("cannot use system column \"%s\" in partition key",
- pelem->name)));
+ pelem->name),
+ parser_errposition(pstate, pelem->location)));
partattrs[attn] = attform->attnum;
atttype = attform->atttypid;
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 8927b21ba2..744b9990a6 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -328,11 +328,15 @@ CREATE TABLE partitioned (
a int
) PARTITION BY RANGE (b);
ERROR: column "b" named in partition key does not exist
+LINE 3: ) PARTITION BY RANGE (b);
+ ^
-- cannot use system columns in partition key
CREATE TABLE partitioned (
a int
) PARTITION BY RANGE (xmin);
ERROR: cannot use system column "xmin" in partition key
+LINE 3: ) PARTITION BY RANGE (xmin);
+ ^
-- functions in key must be immutable
CREATE FUNCTION immut_func (a int) RETURNS int AS $$ SELECT a + random()::int; $$ LANGUAGE SQL;
CREATE TABLE partitioned (
@@ -719,6 +723,8 @@ SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 'part_b'::reg
-- specify PARTITION BY for a partition
CREATE TABLE fail_part_col_not_found PARTITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
ERROR: column "c" named in partition key does not exist
+LINE 1: ...TITION OF parted FOR VALUES IN ('c') PARTITION BY RANGE (c);
+ ^
CREATE TABLE part_c PARTITION OF parted (b WITH OPTIONS NOT NULL DEFAULT 0) FOR VALUES IN ('c') PARTITION BY RANGE ((b));
-- create a level-2 partition
CREATE TABLE part_c_1_10 PARTITION OF part_c FOR VALUES FROM (1) TO (10);
--
2.18.0
v2-0003-Add-location-information-to-Value-nodes.patchtext/plain; charset=UTF-8; name=v2-0003-Add-location-information-to-Value-nodes.patch; x-mac-creator=0; x-mac-type=0Download
From 79eeea4cf1c0a07e6370ea54f20714208d67ba0b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 22 Aug 2018 10:50:49 +0200
Subject: [PATCH v2 3/3] Add location information to Value nodes
This allows better location information in several commands. This
change also adds better location reporting to the COPY command, in
particular pointers into its various column lists. Support for other
commands using Value nodes can be added.
---
src/backend/commands/copy.c | 22 ++++++++++++----------
src/backend/commands/trigger.c | 20 +++++++++++---------
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c | 2 ++
src/backend/nodes/value.c | 4 ++++
src/backend/parser/gram.y | 4 +++-
src/backend/parser/parse_utilcmd.c | 10 ++++++----
src/include/nodes/value.h | 1 +
src/test/regress/expected/alter_table.out | 8 ++++++++
src/test/regress/expected/copy2.out | 2 ++
src/test/regress/expected/create_table.out | 4 ++++
src/test/regress/expected/triggers.out | 8 ++++++++
src/test/regress/sql/create_table.sql | 1 +
src/test/regress/sql/triggers.sql | 4 ++++
14 files changed, 67 insertions(+), 24 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..1e0290236f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -328,7 +328,7 @@ static Datum CopyReadBinaryAttribute(CopyState cstate,
static void CopyAttributeOutText(CopyState cstate, char *string);
static void CopyAttributeOutCSV(CopyState cstate, char *string,
bool use_quote, bool single_attr);
-static List *CopyGetAttnums(TupleDesc tupDesc, Relation rel,
+static List *CopyGetAttnums(ParseState *pstate, TupleDesc tupDesc, Relation rel,
List *attnamelist);
static char *limit_printout_length(const char *str);
@@ -850,7 +850,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
tupDesc = RelationGetDescr(rel);
- attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
+ attnums = CopyGetAttnums(pstate, tupDesc, rel, stmt->attlist);
foreach(cur, attnums)
{
int attno = lfirst_int(cur) -
@@ -1583,7 +1583,7 @@ BeginCopy(ParseState *pstate,
}
/* Generate or convert list of attributes to process */
- cstate->attnumlist = CopyGetAttnums(tupDesc, cstate->rel, attnamelist);
+ cstate->attnumlist = CopyGetAttnums(pstate, tupDesc, cstate->rel, attnamelist);
num_phys_attrs = tupDesc->natts;
@@ -1601,7 +1601,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_quote);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_quote);
foreach(cur, attnums)
{
@@ -1624,7 +1624,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_notnull);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_notnull);
foreach(cur, attnums)
{
@@ -1647,7 +1647,7 @@ BeginCopy(ParseState *pstate,
List *attnums;
ListCell *cur;
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->force_null);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->force_null);
foreach(cur, attnums)
{
@@ -1671,7 +1671,7 @@ BeginCopy(ParseState *pstate,
cstate->convert_select_flags = (bool *) palloc0(num_phys_attrs * sizeof(bool));
- attnums = CopyGetAttnums(tupDesc, cstate->rel, cstate->convert_select);
+ attnums = CopyGetAttnums(pstate, tupDesc, cstate->rel, cstate->convert_select);
foreach(cur, attnums)
{
@@ -4907,7 +4907,7 @@ CopyAttributeOutCSV(CopyState cstate, char *string,
* rel can be NULL ... it's only used for error reports.
*/
static List *
-CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
+CopyGetAttnums(ParseState *pstate, TupleDesc tupDesc, Relation rel, List *attnamelist)
{
List *attnums = NIL;
@@ -4931,7 +4931,8 @@ CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
foreach(l, attnamelist)
{
- char *name = strVal(lfirst(l));
+ Value *val = lfirst(l);
+ char *name = strVal(val);
int attnum;
int i;
@@ -4955,7 +4956,8 @@ CopyGetAttnums(TupleDesc tupDesc, Relation rel, List *attnamelist)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
- name, RelationGetRelationName(rel))));
+ name, RelationGetRelationName(rel)),
+ parser_errposition(pstate, val->location)));
else
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 2436692eb8..b097498bd5 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -161,6 +161,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
Oid funcoid, Oid parentTriggerOid, Node *whenClause,
bool isInternal, bool in_partition)
{
+ ParseState *pstate;
int16 tgtype;
int ncolumns;
int16 *columns;
@@ -188,6 +189,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
char *newtablename = NULL;
bool partition_recurse;
+ /* Set up a pstate to parse with */
+ pstate = make_parsestate(NULL);
+ pstate->p_sourcetext = queryString;
+
if (OidIsValid(relOid))
rel = heap_open(relOid, ShareRowExclusiveLock);
else
@@ -562,15 +567,10 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
*/
if (!whenClause && stmt->whenClause)
{
- ParseState *pstate;
RangeTblEntry *rte;
List *varList;
ListCell *lc;
- /* Set up a pstate to parse with */
- pstate = make_parsestate(NULL);
- pstate->p_sourcetext = queryString;
-
/*
* Set up RTEs for OLD and NEW references.
*
@@ -648,8 +648,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
whenRtable = pstate->p_rtable;
qual = nodeToString(whenClause);
-
- free_parsestate(pstate);
}
else if (!whenClause)
{
@@ -892,7 +890,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
columns = (int16 *) palloc(ncolumns * sizeof(int16));
foreach(cell, stmt->columns)
{
- char *name = strVal(lfirst(cell));
+ Value *val = lfirst(cell);
+ char *name = strVal(val);
int16 attnum;
int j;
@@ -902,7 +901,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" of relation \"%s\" does not exist",
- name, RelationGetRelationName(rel))));
+ name, RelationGetRelationName(rel)),
+ parser_errposition(pstate, val->location)));
/* Check for duplicates */
for (j = i - 1; j >= 0; j--)
@@ -1191,6 +1191,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
/* Keep lock on target rel until end of xact */
heap_close(rel, NoLock);
+ free_parsestate(pstate);
+
return myself;
}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7c8220cf65..b9d8855b8b 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -4738,6 +4738,7 @@ _copyValue(const Value *from)
(int) from->type);
break;
}
+ COPY_LOCATION_FIELD(location);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 378f2facb8..abf7701a28 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2975,6 +2975,8 @@ _equalValue(const Value *a, const Value *b)
break;
}
+ COMPARE_LOCATION_FIELD(location);
+
return true;
}
diff --git a/src/backend/nodes/value.c b/src/backend/nodes/value.c
index 2a30307baf..f41a5c0be7 100644
--- a/src/backend/nodes/value.c
+++ b/src/backend/nodes/value.c
@@ -26,6 +26,7 @@ makeInteger(int i)
v->type = T_Integer;
v->val.ival = i;
+ v->location = -1;
return v;
}
@@ -41,6 +42,7 @@ makeFloat(char *numericStr)
v->type = T_Float;
v->val.str = numericStr;
+ v->location = -1;
return v;
}
@@ -56,6 +58,7 @@ makeString(char *str)
v->type = T_String;
v->val.str = str;
+ v->location = -1;
return v;
}
@@ -71,5 +74,6 @@ makeBitString(char *str)
v->type = T_BitString;
v->val.str = str;
+ v->location = -1;
return v;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 4bd2223f26..e9a3a8efab 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -3798,7 +3798,9 @@ columnList:
columnElem: ColId
{
- $$ = (Node *) makeString($1);
+ Value *v = makeString($1);
+ v->location = @1;
+ $$ = (Node *) v;
}
;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 656b1b5f1b..91531dd88d 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2148,7 +2148,8 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
{
foreach(lc, constraint->keys)
{
- char *key = strVal(lfirst(lc));
+ Value *val = lfirst(lc);
+ char *key = strVal(val);
bool found = false;
ColumnDef *column = NULL;
ListCell *columns;
@@ -2236,7 +2237,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in key does not exist", key),
- parser_errposition(cxt->pstate, constraint->location)));
+ parser_errposition(cxt->pstate, val->location)));
/* Check for PRIMARY KEY(foo, foo) */
foreach(columns, index->indexParams)
@@ -2275,7 +2276,8 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
/* Add included columns to index definition */
foreach(lc, constraint->including)
{
- char *key = strVal(lfirst(lc));
+ Value *val = lfirst(lc);
+ char *key = strVal(val);
bool found = false;
ColumnDef *column = NULL;
ListCell *columns;
@@ -2360,7 +2362,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("column \"%s\" named in key does not exist", key),
- parser_errposition(cxt->pstate, constraint->location)));
+ parser_errposition(cxt->pstate, val->location)));
/* OK, add it to the index definition */
iparam = makeNode(IndexElem);
diff --git a/src/include/nodes/value.h b/src/include/nodes/value.h
index 1665714515..069a715cd7 100644
--- a/src/include/nodes/value.h
+++ b/src/include/nodes/value.h
@@ -47,6 +47,7 @@ typedef struct Value
int ival; /* machine integer */
char *str; /* string */
} val;
+ int location; /* token location, or -1 if unknown */
} Value;
#define intVal(v) (((Value *)(v))->val.ival)
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 0218c2c362..a9d6b3aa03 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1485,8 +1485,12 @@ copy attest to stdout;
2 3
copy attest(a) to stdout;
ERROR: column "a" of relation "attest" does not exist
+LINE 1: copy attest(a) to stdout;
+ ^
copy attest("........pg.dropped.1........") to stdout;
ERROR: column "........pg.dropped.1........" of relation "attest" does not exist
+LINE 1: copy attest("........pg.dropped.1........") to stdout;
+ ^
copy attest from stdin;
ERROR: extra data after last expected column
CONTEXT: COPY attest, line 1: "10 11 12"
@@ -1506,8 +1510,12 @@ select * from attest;
copy attest(a) from stdin;
ERROR: column "a" of relation "attest" does not exist
+LINE 1: copy attest(a) from stdin;
+ ^
copy attest("........pg.dropped.1........") from stdin;
ERROR: column "........pg.dropped.1........" of relation "attest" does not exist
+LINE 1: copy attest("........pg.dropped.1........") from stdin;
+ ^
copy attest(b,c) from stdin;
select * from attest;
b | c
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index eb9e4b9774..171afed28c 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -28,6 +28,8 @@ COPY x (a, b, c, d, e) from stdin;
-- non-existent column in column list: should fail
COPY x (xyz) from stdin;
ERROR: column "xyz" of relation "x" does not exist
+LINE 1: COPY x (xyz) from stdin;
+ ^
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index 744b9990a6..fbf55e2045 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -220,6 +220,10 @@ CREATE TABLE tas_case WITH ("Fillfactor" = 10) AS SELECT 1 a;
ERROR: unrecognized parameter "Fillfactor"
CREATE TABLE tas_case (a text) WITH ("Oids" = true);
ERROR: unrecognized parameter "Oids"
+CREATE TABLE error (a int, primary key (x)); -- error
+ERROR: column "x" named in key does not exist
+LINE 1: CREATE TABLE error (a int, primary key (x));
+ ^
CREATE UNLOGGED TABLE unlogged1 (a int primary key); -- OK
CREATE TEMPORARY TABLE unlogged2 (a int primary key); -- OK
SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^unlogged\d' ORDER BY relname;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 7d59de98eb..3e91d41531 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -528,6 +528,14 @@ NOTICE: dummy_update_func(before) called: action = UPDATE, old = (f), new = (t)
NOTICE: dummy_update_func(aftera) called: action = UPDATE, old = (f), new = (t)
DROP TABLE some_t;
-- bogus cases
+CREATE TRIGGER error_nonexistent_table BEFORE UPDATE ON nonexistent_table
+FOR EACH ROW EXECUTE PROCEDURE trigger_func('nonexistent_table');
+ERROR: relation "nonexistent_table" does not exist
+CREATE TRIGGER error_nonexistent_columns BEFORE UPDATE OF nonexistent_columns ON main_table
+FOR EACH ROW EXECUTE PROCEDURE trigger_func('nonexistent_columns');
+ERROR: column "nonexistent_columns" of relation "main_table" does not exist
+LINE 1: ...RIGGER error_nonexistent_columns BEFORE UPDATE OF nonexisten...
+ ^
CREATE TRIGGER error_upd_and_col BEFORE UPDATE OR UPDATE OF a ON main_table
FOR EACH ROW EXECUTE PROCEDURE trigger_func('error_upd_and_col');
ERROR: duplicate trigger events specified at or near "ON"
diff --git a/src/test/regress/sql/create_table.sql b/src/test/regress/sql/create_table.sql
index 81fa7658b0..bef7ba19ca 100644
--- a/src/test/regress/sql/create_table.sql
+++ b/src/test/regress/sql/create_table.sql
@@ -257,6 +257,7 @@ CREATE TABLE IF NOT EXISTS test_tsvector(
CREATE TABLE tas_case WITH ("Fillfactor" = 10) AS SELECT 1 a;
CREATE TABLE tas_case (a text) WITH ("Oids" = true);
+CREATE TABLE error (a int, primary key (x)); -- error
CREATE UNLOGGED TABLE unlogged1 (a int primary key); -- OK
CREATE TEMPORARY TABLE unlogged2 (a int primary key); -- OK
SELECT relname, relkind, relpersistence FROM pg_class WHERE relname ~ '^unlogged\d' ORDER BY relname;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index d7dfd753be..6da79a55f9 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -348,6 +348,10 @@ CREATE TRIGGER some_trig_afterb AFTER UPDATE ON some_t FOR EACH ROW
DROP TABLE some_t;
-- bogus cases
+CREATE TRIGGER error_nonexistent_table BEFORE UPDATE ON nonexistent_table
+FOR EACH ROW EXECUTE PROCEDURE trigger_func('nonexistent_table');
+CREATE TRIGGER error_nonexistent_columns BEFORE UPDATE OF nonexistent_columns ON main_table
+FOR EACH ROW EXECUTE PROCEDURE trigger_func('nonexistent_columns');
CREATE TRIGGER error_upd_and_col BEFORE UPDATE OR UPDATE OF a ON main_table
FOR EACH ROW EXECUTE PROCEDURE trigger_func('error_upd_and_col');
CREATE TRIGGER error_upd_a_a BEFORE UPDATE OF a, a ON main_table
--
2.18.0
Re: some more error location support
The majority rule (34 make & 22 free) suggest that it is more often use
than not. I'd suggest to stick to that for consistency & homogeneity.But it's consistently not used in DDL command implementations, only in
normal query parsing.
I try to avoid complicated (context-sensitive) rules when I can, esp as
some functions may be called from DDL and DML.
But fine with me.
--
Fabien.
Re: some more error location support
On 29/08/2018 16:39, Fabien COELHO wrote:
The majority rule (34 make & 22 free) suggest that it is more often use
than not. I'd suggest to stick to that for consistency & homogeneity.But it's consistently not used in DDL command implementations, only in
normal query parsing.I try to avoid complicated (context-sensitive) rules when I can, esp as
some functions may be called from DDL and DML.But fine with me.
Committed 0001 and 0002, keeping 0003 for future research, as discussed.
Thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services