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
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 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
From 1816353610c6af345f72f4753cdb629c5304123d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
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,
fals
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
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 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
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
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
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
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
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 Would it be easily possible to provide the query in that case as well? -- Fabien.
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.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 002dccbbe1df65347034d41a9582093a0cc72ba4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
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 */
+
