Обсуждение: Weird behavior during CREATE EXTENSION
This behavior had be quite baffled... > ~@decina.local/29760# create extension "trunklet-format" CASCADE; > NOTICE: installing required extension "trunklet" > NOTICE: installing required extension "variant" > CREATE EXTENSION > ~@decina.local/29760# create extension "pgxntool-test"; > ERROR: syntax error at or near "-" > LINE 1: create extension "pgxntool-test"; > ^ > ~@decina.local/29760# select * from pg_available_extensions where name ~'-'; > name | default_version | installed_version | comment > -----------------+-----------------+-------------------+------------------------------------------------- > pgxntool-test | 0.1.0 | | > trunklet-format | 0.1.1 | 0.1.1 | A format()-based template language for trunklet > (2 rows) Eventually, I realized the problem was the first line of the extension file itself: CREATE FUNCTION pgxntool-test( wrapping that in "s fixed the issue. (The reason that still doesn't line up with the ^ above is because the ^ is accounting for "LINE 1: ".) This makes debugging extensions quite tedious. Part of the explanation is in the comment for execute_sql_string(): > /* > * Execute given SQL string. > * > * filename is used only to report errors. > * > * Note: it's tempting to just use SPI to execute the string, but that does > * not work very well. The really serious problem is that SPI will parse, > * analyze, and plan the whole string before executing any of it; of course > * this fails if there are any plannable statements referring to objects > * created earlier in the script. A lesser annoyance is that SPI insists > * on printing the whole string as errcontext in case of any error, and that > * could be very long. > */ I can think of 4 ways to fix this: 1) Have psql parse the script into separate commands for us. 2) Pull enough of psql's parsing into the backend code to be able to do #1 3) Add *file* line numbers to the output of pg_parse_query() 4) Have ereport spit out the context you'd normally get from SPI if it sees that it was called from somewhere underneath execute_sql_string(). My preference would actually be #1, because that would make it easy for any tool that wanted to to get access to that. Jon Erdman actually looked into a similar alternative in the past and it was only a few lines of code. Basically, when the "parse file" option is chosen don't even attempt to connect to a database, just parse things, execute \ commands and print the results instead of sending them via libpq. That wouldn't work directly here because we want to split commands apart, but it wouldn't be hard to have psql spit out a special command separator line and then look for that. psql would have to ignore \quit in this mode though, but I think that's fine. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> I can think of 4 ways to fix this:
> 1) Have psql parse the script into separate commands for us.
> 2) Pull enough of psql's parsing into the backend code to be able to do #1
> 3) Add *file* line numbers to the output of pg_parse_query()
> 4) Have ereport spit out the context you'd normally get from SPI if it
> sees that it was called from somewhere underneath execute_sql_string().
> My preference would actually be #1, because that would make it easy for
> any tool that wanted to to get access to that. Jon Erdman actually
> looked into a similar alternative in the past and it was only a few
> lines of code.
So, CREATE EXTENSION would fail outright if the backend couldn't locate a
psql executable? Seems like a nonstarter to me (psql is not necessarily
installed in a server).
There's certainly room to improve error reporting for extension scripts,
but I think you are heading into a dead end in the name of saving a little
time coding. I'd suggest looking into an errcontext callback, instead.
Also, there's some technology in CREATE FUNCTION that deals with the fact
that we may be calling the parser on a string different from the original
user command, which might be worth borrowing here --- at least part of
the confusion is that it's evidently reporting a cursor position relative
to the extension script as though it applied to the CREATE EXTENSION.
regards, tom lane
On 1/12/16 5:00 PM, Tom Lane wrote: > There's certainly room to improve error reporting for extension scripts, > but I think you are heading into a dead end in the name of saving a little > time coding. I'd suggest looking into an errcontext callback, instead. > > Also, there's some technology in CREATE FUNCTION that deals with the fact > that we may be calling the parser on a string different from the original > user command, which might be worth borrowing here --- at least part of > the confusion is that it's evidently reporting a cursor position relative > to the extension script as though it applied to the CREATE EXTENSION. Are you talking about plpgsql_compile_error_callback()? It looks like it does it's magic by relying on the plpgsql parser to keep track of where it's at. ISTM part of the goal here should be to show what the actual command was that failed (ie: the command in the extension file). I'm guessing the way to do that would be to have pg_parse_query() keep the original statement in the parse nodes? I guess if this was easy it would already have been fixed... -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 1/12/16 5:00 PM, Tom Lane wrote:
>> Also, there's some technology in CREATE FUNCTION that deals with the fact
>> that we may be calling the parser on a string different from the original
>> user command, which might be worth borrowing here --- at least part of
>> the confusion is that it's evidently reporting a cursor position relative
>> to the extension script as though it applied to the CREATE EXTENSION.
> Are you talking about plpgsql_compile_error_callback()? It looks like it
> does it's magic by relying on the plpgsql parser to keep track of where
> it's at.
Not really. It's about transposing the error to an "internal query"
instead of reporting it against the user's text.
I was imagining something roughly like the attached. Experimenting with
this, the good news is that you get a decent error position:
regression=# create extension pg_trgm ;
ERROR: syntax error at or near "USINGgist"
LINE 129: ALTER OPERATOR FAMILY gist_trgm_ops USINGgist ADD
^
QUERY: /* contrib/pg_trgm/pg_trgm--1.2.sql */
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
...
ALTER OPERATOR FAMILY gin_trgm_ops USING gin ADD
FUNCTION 6 (text,text) gin_trgm_triconsistent (internal, int2, text, int4, internal, internal,
internal);
CONTEXT: extension script file "/home/postgres/testversion/share/extension/pg_trgm--1.2.sql"
regression=#
The bad news is that psql regurgitates the whole script in the QUERY
field, because that's what we said was the internal query.
There are various ways you could imagine printing just part of the script;
for instance, starting from the cursor position, scan forward and back for
semicolons at line ends, and trim the query string to report just that
much. Or maybe it's worth teaching the grammar to report the first token
location of each parsetree, and then the loop in execute_sql_string could
stash that away for use by the error reporter.
I don't really have time to fool with this, but if someone else wants
to run with it ...
regards, tom lane
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 9d84b79..b4b9286 100644
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
*************** typedef struct ExtensionVersionInfo
*** 96,102 ****
--- 96,110 ----
struct ExtensionVersionInfo *previous; /* current best predecessor */
} ExtensionVersionInfo;
+ /* State needed by script_error_callback */
+ typedef struct
+ {
+ const char *script;
+ const char *filename;
+ } script_error_callback_arg;
+
/* Local functions */
+ static void script_error_callback(void *arg);
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
ExtensionVersionInfo *evi_target,
*************** read_extension_script_file(const Extensi
*** 682,692 ****
--- 690,713 ----
static void
execute_sql_string(const char *sql, const char *filename)
{
+ script_error_callback_arg callback_arg;
+ ErrorContextCallback sqlerrcontext;
List *raw_parsetree_list;
DestReceiver *dest;
ListCell *lc1;
/*
+ * Setup error traceback support for ereport().
+ */
+ callback_arg.script = sql;
+ callback_arg.filename = filename;
+
+ sqlerrcontext.callback = script_error_callback;
+ sqlerrcontext.arg = (void *) &callback_arg;
+ sqlerrcontext.previous = error_context_stack;
+ error_context_stack = &sqlerrcontext;
+
+ /*
* Parse the SQL string into a list of raw parse trees.
*/
raw_parsetree_list = pg_parse_query(sql);
*************** execute_sql_string(const char *sql, cons
*** 755,765 ****
--- 776,809 ----
}
}
+ error_context_stack = sqlerrcontext.previous;
+
/* Be sure to advance the command counter after the last script command */
CommandCounterIncrement();
}
/*
+ * error context callback for errors within an extension script
+ */
+ static void
+ script_error_callback(void *arg)
+ {
+ script_error_callback_arg *callback_arg = (script_error_callback_arg *) arg;
+ int syntaxerrposition;
+
+ /* If it's a syntax error, convert to internal syntax error report */
+ syntaxerrposition = geterrposition();
+ if (syntaxerrposition > 0)
+ {
+ errposition(0);
+ internalerrposition(syntaxerrposition);
+ internalerrquery(callback_arg->script);
+ }
+
+ errcontext("extension script file \"%s\"", callback_arg->filename);
+ }
+
+ /*
* Execute the appropriate script file for installing or updating the extension
*
* If from_version isn't NULL, it's an update