From 79eeea4cf1c0a07e6370ea54f20714208d67ba0b Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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