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.