Re: [HACKERS] improved DefElem list processing

[Peter Eisentraut]

On 8/22/16 10:28 AM, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
> 
>> I'm not happy that utils/acl.h has prototypes for aclchk.c, because
>> acl.h is included all over the place.  Perhaps I should make a
>> src/include/catalog/aclchk.c to clean that up.
> 
> I've been bothered by that too in the past.  +1 for the cleanup.

Here is a patch for that.

It didn't quite achieve the elegance I was hoping for.  Most uses of
acl.h actually use aclchk.c functions, and the new aclchk.h must include
acl.h, so basically you end up just changing most includes of acl.h to
aclchk.h while still effectively including acl.h everywhere.  But I
think having one header file serve two separate .c files is still a
confusing pattern that is worth cleaning up.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d4f9090..06041f0 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -38,6 +38,7 @@
 
 #include "access/htup_details.h"
 #include "access/reloptions.h"
+#include "catalog/aclchk.h"
 #include "catalog/indexing.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_foreign_server.h"
@@ -50,7 +51,6 @@
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "parser/scansup.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/guc.h"
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index c3a518c..d312d74 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -16,12 +16,12 @@
 #include 
 
 #include "access/heapam.h"
+#include "catalog/aclchk.h"
 #include "catalog/catalog.h"
 #include "fmgr.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "storage/smgr.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index e20e7f8..1c8fff0 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -27,12 +27,12 @@
 #include "access/multixact.h"
 #include "access/relscan.h"
 #include "access/xact.h"
+#include "catalog/aclchk.h"
 #include "catalog/namespace.h"
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "storage/procarray.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/rel.h"
 #include "utils/snapmgr.h"
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 1b45a4c..4a48e92 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -22,6 +22,7 @@
 #include "access/reloptions.h"
 #include "access/relscan.h"
 #include "access/xloginsert.h"
+#include "catalog/aclchk.h"
 #include "catalog/index.h"
 #include "catalog/pg_am.h"
 #include "miscadmin.h"
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index b56d0e3..a1b213e 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -20,10 +20,10 @@
 #include "postgres.h"
 
 #include "access/htup_details.h"
+#include "catalog/aclchk.h"
 #include "catalog/pg_type.h"
 #include "miscadmin.h"
 #include "parser/parse_type.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/resowner_private.h"
 #include "utils/syscache.h"
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 6b709db..2305782 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -22,11 +22,11 @@
 #include "access/xloginsert.h"
 #include "access/xlog.h"
 #include "commands/vacuum.h"
+#include "catalog/aclchk.h"
 #include "catalog/pg_am.h"
 #include "miscadmin.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
-#include "utils/acl.h"
 #include "postmaster/autovacuum.h"
 #include "storage/indexfsm.h"
 #include "storage/lmgr.h"
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 65c941d..c3a1997 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -21,11 +21,11 @@
 
 #include "access/relscan.h"
 #include "access/transam.h"
+#include "catalog/aclchk.h"
 #include "catalog/index.h"
 #include "lib/stringinfo.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
-#include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/rel.h"
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index a585c3a..9face59 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -22,6 +22,7 @@
 #include "access/htup_details.h"
 #include "access/sysattr.h"
 #include "access/xact.h"
+#include "catalog/aclchk.h"
 #include "catalog/binary_upgrade.h"
 #include "catalog/catalog.h"
 #include "catalog/dependency.h"
@@ -59,7 +60,6 @@
 #include "nodes/makefuncs.h"
 #include 

Re: [HACKERS] improved DefElem list processing

[Peter Eisentraut]

On 9/6/16 7:23 PM, Tom Lane wrote:
> I'm curious where you are on that?  I find myself needing to whack around
> this processing in CREATE EXTENSION, but I don't want to create a merge
> problem for you if you're close to committing.

I have committed what I have for now.  Thanks.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Tom Lane]

Peter Eisentraut  writes:
> Here are two WIP patches to improve the DefElem list processing that is
> used by many utility commands.

> One factors out the duplicate checks, which are currently taking up a
> lot of space with duplicate code.  I haven't applied this everywhere
> yet, but the patch shows how much boring code can be saved.

I'm curious where you are on that?  I find myself needing to whack around
this processing in CREATE EXTENSION, but I don't want to create a merge
problem for you if you're close to committing.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Alvaro Herrera]

Peter Eisentraut wrote:

> I'm not happy that utils/acl.h has prototypes for aclchk.c, because
> acl.h is included all over the place.  Perhaps I should make a
> src/include/catalog/aclchk.c to clean that up.

I've been bothered by that too in the past.  +1 for the cleanup.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Michael Paquier]

On Mon, Aug 22, 2016 at 10:41 PM, Pavel Stehule  wrote:
> 1. This patch introduce location in DefElement node, and inject ParserState
> to SQL commands, where ParserState was not used. It allows to show the
> position of an error. This patch is not small, but almost changes are
> trivial.
>
> 2. There are no problems with patching, compiling, tests - all tests passed.
>
> 3. There is not any new functionality, so new tests and new documentation is
> not necessary.
>
> I'll mark this patch as ready for commiter.

Now that I look at those patches, +1 for both. Particularly the
redundant-option checks will remove a lot of boring code.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Pavel Stehule]

Hi


2016-08-11 17:32 GMT+02:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 8/5/16 11:25 AM, Peter Eisentraut wrote:
> > On 8/4/16 2:21 PM, Tom Lane wrote:
> >> Forgot to mention: seems like you should have added a location
> >> argument to makeDefElem.
> >
> > I was hesitating to do that lest it break extensions or something, but I
> > guess we break bigger things than that all the time.  I'll change it.
>
> In order not to work on two patches that directly conflict with each
> other, I have proceeded with the location patch and postponed the
> duplicate checking patch.
>
> Attached is a biggish patch to review.  It adds location information to
> all places DefElems are created in the parser and then adds errposition
> information in a lot of places, but surely not all of them.  That can be
> improved over time.
>
> I'm not happy that utils/acl.h has prototypes for aclchk.c, because
> acl.h is included all over the place.  Perhaps I should make a
> src/include/catalog/aclchk.c to clean that up.
>
> Here are some example commands to try for getting suitable error messages:
>
> create collation foo (foo = bar, bar = foo);
> copy test from stdin (null 'x', null 'x');
> create function foo (a int, b int) returns int as $$ select a+b $$
> language sql language sql;
> create function foo (a int, b int) returns int as $$ select a+b $$
> language sql volatile stable;
> create function foo (a int, b int) returns int as $$ select a+b $$
> language sql with (foo = bar);
> create sequence foo minvalue 1 minvalue 2;
> create type foo (foo = bar);
> create user foo createdb nocreatedb;
> explain (foo, bar) select 1;
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
I am sending a review of this patch:

1. This patch introduce location in DefElement node, and inject ParserState
to SQL commands, where ParserState was not used. It allows to show the
position of an error. This patch is not small, but almost changes are
trivial.

2. There are no problems with patching, compiling, tests - all tests passed.

3. There is not any new functionality, so new tests and new documentation
is not necessary.

I'll mark this patch as ready for commiter.

Regards

Pavel





>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] improved DefElem list processing

[Peter Eisentraut]

On 8/5/16 11:25 AM, Peter Eisentraut wrote:
> On 8/4/16 2:21 PM, Tom Lane wrote:
>> Forgot to mention: seems like you should have added a location
>> argument to makeDefElem.
> 
> I was hesitating to do that lest it break extensions or something, but I
> guess we break bigger things than that all the time.  I'll change it.

In order not to work on two patches that directly conflict with each
other, I have proceeded with the location patch and postponed the
duplicate checking patch.

Attached is a biggish patch to review.  It adds location information to
all places DefElems are created in the parser and then adds errposition
information in a lot of places, but surely not all of them.  That can be
improved over time.

I'm not happy that utils/acl.h has prototypes for aclchk.c, because
acl.h is included all over the place.  Perhaps I should make a
src/include/catalog/aclchk.c to clean that up.

Here are some example commands to try for getting suitable error messages:

create collation foo (foo = bar, bar = foo);
copy test from stdin (null 'x', null 'x');
create function foo (a int, b int) returns int as $$ select a+b $$
language sql language sql;
create function foo (a int, b int) returns int as $$ select a+b $$
language sql volatile stable;
create function foo (a int, b int) returns int as $$ select a+b $$
language sql with (foo = bar);
create sequence foo minvalue 1 minvalue 2;
create type foo (foo = bar);
create user foo createdb nocreatedb;
explain (foo, bar) select 1;

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8751bc7fe1ac057e0b66bf879d5e1988d132db38 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 11 Aug 2016 12:00:00 -0400
Subject: [PATCH v2] Add location field to DefElem

Add a location field to the DefElem struct, used to parse many utility
commands.  Update various error messages to supply error position
information.

To propogate the error position information in a more systematic way,
create a ParseState in standard_ProcessUtility() and pass that to
interested functions implementing the utility commands.  This seems
better than passing the query string and then reassembling a parse state
ad hoc, which violates the encapsulation of the ParseState type.
---
 contrib/file_fdw/file_fdw.c|  16 +-
 src/backend/access/common/reloptions.c |   2 +-
 src/backend/catalog/aclchk.c   |   8 +-
 src/backend/commands/aggregatecmds.c   |   7 +-
 src/backend/commands/collationcmds.c   |   5 +-
 src/backend/commands/copy.c|  93 ++
 src/backend/commands/dbcommands.c  |  61 +++---
 src/backend/commands/define.c  |   9 -
 src/backend/commands/explain.c |   8 +-
 src/backend/commands/extension.c   |  25 ++-
 src/backend/commands/functioncmds.c|  57 +++---
 src/backend/commands/sequence.c|  36 ++--
 src/backend/commands/tsearchcmds.c |   8 +-
 src/backend/commands/typecmds.c|   8 +-
 src/backend/commands/user.c|  41 ++--
 src/backend/commands/view.c|   4 +-
 src/backend/nodes/copyfuncs.c  |   1 +
 src/backend/nodes/equalfuncs.c |   2 +
 src/backend/nodes/makefuncs.c  |   6 +-
 src/backend/nodes/outfuncs.c   |   1 +
 src/backend/nodes/readfuncs.c  |   1 +
 src/backend/parser/gram.y  | 248 -
 src/backend/parser/parse_utilcmd.c |   5 +-
 src/backend/replication/logical/logicalfuncs.c |   2 +-
 src/backend/replication/repl_gram.y|  16 +-
 src/backend/tcop/utility.c |  64 ---
 src/include/commands/collationcmds.h   |   2 +-
 src/include/commands/copy.h|   7 +-
 src/include/commands/dbcommands.h  |   4 +-
 src/include/commands/defrem.h  |  13 +-
 src/include/commands/explain.h |   3 +-
 src/include/commands/extension.h   |   4 +-
 src/include/commands/sequence.h|   5 +-
 src/include/commands/typecmds.h|   2 +-
 src/include/commands/user.h|   3 +-
 src/include/nodes/makefuncs.h  |   4 +-
 src/include/nodes/parsenodes.h |   1 +
 src/include/utils/acl.h|   4 +-
 38 files changed, 438 insertions(+), 348 deletions(-)

diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index c049131..1d0049f 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -293,7 +293,7 @@ file_fdw_validator(PG_FUNCTION_ARGS)
 	/*
 	 * Now apply the core COPY code's validation logic for more checks.
 	 */
-	ProcessCopyOptions(NULL, true, other_options);
+	ProcessCopyOptions(NULL, NULL, true, 

Re: [HACKERS] improved DefElem list processing

[Peter Eisentraut]

On 8/4/16 2:21 PM, Tom Lane wrote:
> Forgot to mention: seems like you should have added a location
> argument to makeDefElem.

I was hesitating to do that lest it break extensions or something, but I
guess we break bigger things than that all the time.  I'll change it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Peter Eisentraut]

On 8/4/16 2:08 PM, Tom Lane wrote:
> +1 on the general idea, but I wonder if it's a problem that you replaced
> an O(N) check with an O(N^2) check.  I don't think there are any commands
> that would be likely to have so many options that this would become a
> serious issue, but ...

I'll run some tests.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Tom Lane]

I wrote:
> Peter Eisentraut  writes:
>> The other adds a location field to the DefElem node.

> +1 for sure, lots of places where that would be a good thing
> (the duplicate check itself, for starters).

Forgot to mention: seems like you should have added a location
argument to makeDefElem.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] improved DefElem list processing

[Tom Lane]

Peter Eisentraut  writes:
> Here are two WIP patches to improve the DefElem list processing that is
> used by many utility commands.

> One factors out the duplicate checks, which are currently taking up a
> lot of space with duplicate code.  I haven't applied this everywhere
> yet, but the patch shows how much boring code can be saved.

+1 on the general idea, but I wonder if it's a problem that you replaced
an O(N) check with an O(N^2) check.  I don't think there are any commands
that would be likely to have so many options that this would become a
serious issue, but ...

> The other adds a location field to the DefElem node.

+1 for sure, lots of places where that would be a good thing
(the duplicate check itself, for starters).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers