Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges - Mailing list pgsql-hackers

Christoph Berg <myon@debian.org> writes:
> I wish there was. The error reporting from failing extension scripts
> is really bad with no context at all, it has hit me a few times in the
> past already.

Nobody's spent any work on that :-(.  A really basic reporting
facility is not hard to add, as in the attached finger exercise.
The trouble with it can be explained by showing what I get after
intentionally breaking a script file command:

regression=# create extension cube;
ERROR:  syntax error at or near "CREAT"
LINE 16: CREAT FUNCTION cube_send(cube)
         ^
QUERY:  /* contrib/cube/cube--1.4--1.5.sql */

-- complain if script is sourced in psql, rather than via ALTER EXTENSION


-- Remove @ and ~
DROP OPERATOR @ (cube, cube);
DROP OPERATOR ~ (cube, cube);

-- Add binary input/output handlers
CREATE FUNCTION cube_recv(internal)
RETURNS cube
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

CREAT FUNCTION cube_send(cube)
RETURNS bytea
AS '$libdir/cube'
LANGUAGE C IMMUTABLE STRICT PARALLEL SAFE;

ALTER TYPE cube SET ( RECEIVE = cube_recv, SEND = cube_send );

CONTEXT:  extension script file "/home/postgres/install/share/extension/cube--1.4--1.5.sql"

So the first part of that is great, but if your script file is
large you probably won't be happy about having the whole thing
repeated in the "QUERY" field.  So this needs some work on
user-friendliness.

I'm inclined to think that maybe we'd be best off keeping the server
end of it straightforward, and trying to teach psql to abbreviate the
QUERY field in a useful way.  IIRC you get this same type of problem
with very large SQL-language functions and suchlike.

Also, I believe this doesn't help much for non-syntax errors
(those that aren't reported with an error location).  It might
be interesting to use the RawStmt.stmt_location/stmt_len fields
for the current parsetree to identify what to report, but
I've not dug further than this.

            regards, tom lane

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index fab59ad5f6..6af72ff422 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -107,6 +107,13 @@ typedef struct ExtensionVersionInfo
     struct ExtensionVersionInfo *previous;    /* current best predecessor */
 } ExtensionVersionInfo;

+/* Info structure for script_error_callback() */
+typedef struct
+{
+    const char *sql;
+    const char *filename;
+} script_error_callback_arg;
+
 /* Local functions */
 static List *find_update_path(List *evi_list,
                               ExtensionVersionInfo *evi_start,
@@ -670,9 +677,32 @@ read_extension_script_file(const ExtensionControlFile *control,
     return dest_str;
 }

+/*
+ * error context callback for failures in script-file execution
+ */
+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->sql);
+    }
+
+    errcontext("extension script file \"%s\"", callback_arg->filename);
+}
+
 /*
  * Execute given SQL string.
  *
+ * The filename the string came from is also provided, for error reporting.
+ *
  * 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
@@ -682,12 +712,25 @@ read_extension_script_file(const ExtensionControlFile *control,
  * could be very long.
  */
 static void
-execute_sql_string(const char *sql)
+execute_sql_string(const char *sql, const char *filename)
 {
+    script_error_callback_arg callback_arg;
+    ErrorContextCallback scripterrcontext;
     List       *raw_parsetree_list;
     DestReceiver *dest;
     ListCell   *lc1;

+    /*
+     * Setup error traceback support for ereport().
+     */
+    callback_arg.sql = sql;
+    callback_arg.filename = filename;
+
+    scripterrcontext.callback = script_error_callback;
+    scripterrcontext.arg = (void *) &callback_arg;
+    scripterrcontext.previous = error_context_stack;
+    error_context_stack = &scripterrcontext;
+
     /*
      * Parse the SQL string into a list of raw parse trees.
      */
@@ -780,6 +823,8 @@ execute_sql_string(const char *sql)

     /* Be sure to advance the command counter after the last script command */
     CommandCounterIncrement();
+
+    error_context_stack = scripterrcontext.previous;
 }

 /*
@@ -1054,7 +1099,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
         /* And now back to C string */
         c_sql = text_to_cstring(DatumGetTextPP(t_sql));

-        execute_sql_string(c_sql);
+        execute_sql_string(c_sql, filename);
     }
     PG_FINALLY();
     {