Re: improved DefElem list processing

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.

The other adds a location field to the DefElem node. This allows
showing an error pointer if there is a problem in one of the options,
which can be useful for complex commands such as COPY or CREATE USER.

At the moment, these patches are independent and don't work together,
but the second one would become much easier if the first one is accepted.

If these ideas are acceptable, I'll produce a complete patch series.

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

I wrote:
> Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

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

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

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

Hi

2016-08-11 17:32 GMT+02:00 Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On Mon, Aug 22, 2016 at 10:41 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> 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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

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

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