Re: Better error reporting from extension scripts (Was: Extend ALTER OPERATOR) - Mailing list pgsql-hackers

Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane
>> 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.

> Does this really have to be addressed? It would be way better than it
> is now, and errors during extension creation are rare and mostly for
> developers only, so it doesn't have to be pretty.

Perhaps.  I spent a little more effort on this and added code to
report errors that don't come with an error location.  On those,
we don't have any constraints about what to report in the QUERY
field, so I made it trim the string to just the current query
within the script, which makes things quite a bit better.  You
can see the results in the test_extensions regression test changes.

(It might be worth some effort to trim away comments appearing
just before a command, but I didn't tackle that here.)

            regards, tom lane

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index fab59ad5f6..0fae1332d2 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -54,6 +54,7 @@
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "nodes/queryjumble.h"
 #include "storage/fd.h"
 #include "tcop/utility.h"
 #include "utils/acl.h"
@@ -107,6 +108,17 @@ typedef struct ExtensionVersionInfo
     struct ExtensionVersionInfo *previous;    /* current best predecessor */
 } ExtensionVersionInfo;

+/*
+ * Information for script_error_callback()
+ */
+typedef struct
+{
+    const char *sql;            /* entire script file contents */
+    const char *filename;        /* script file pathname */
+    ParseLoc    stmt_location;    /* current stmt start loc, or -1 if unknown */
+    ParseLoc    stmt_len;        /* length in bytes; 0 means "rest of string" */
+} script_error_callback_arg;
+
 /* Local functions */
 static List *find_update_path(List *evi_list,
                               ExtensionVersionInfo *evi_start,
@@ -670,9 +682,60 @@ 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;
+    const char *lastslash;
+
+    /* If it's a syntax error, convert to internal syntax error report */
+    syntaxerrposition = geterrposition();
+    if (syntaxerrposition > 0)
+    {
+        /*
+         * We must report the whole string because otherwise details such as
+         * psql's line number report would be wrong.
+         */
+        errposition(0);
+        internalerrposition(syntaxerrposition);
+        internalerrquery(callback_arg->sql);
+    }
+    else if (callback_arg->stmt_location >= 0)
+    {
+        /*
+         * Since no syntax cursor will be shown, it's okay and helpful to trim
+         * the reported query string to just the current statement.
+         */
+        const char *query = callback_arg->sql;
+        int            location = callback_arg->stmt_location;
+        int            len = callback_arg->stmt_len;
+
+        query = CleanQuerytext(query, &location, &len);
+        internalerrquery(pnstrdup(query, len));
+    }
+
+    /*
+     * Trim the reported file name to remove the path.  We know that
+     * get_extension_script_filename() inserted a '/', regardless of whether
+     * we're on Windows.
+     */
+    lastslash = strrchr(callback_arg->filename, '/');
+    if (lastslash)
+        lastslash++;
+    else
+        lastslash = callback_arg->filename; /* shouldn't happen, but cope */
+    errcontext("extension script file \"%s\"", lastslash);
+}
+
 /*
  * 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 +745,27 @@ 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;
+    callback_arg.stmt_location = -1;
+    callback_arg.stmt_len = -1;
+
+    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.
      */
@@ -709,6 +787,10 @@ execute_sql_string(const char *sql)
         List       *stmt_list;
         ListCell   *lc2;

+        /* Report location of this query for error context callback */
+        callback_arg.stmt_location = parsetree->stmt_location;
+        callback_arg.stmt_len = parsetree->stmt_len;
+
         /*
          * We do the work for each parsetree in a short-lived context, to
          * limit the memory used when there are many commands in the string.
@@ -778,6 +860,8 @@ execute_sql_string(const char *sql)
         MemoryContextDelete(per_parsetree_context);
     }

+    error_context_stack = scripterrcontext.previous;
+
     /* Be sure to advance the command counter after the last script command */
     CommandCounterIncrement();
 }
@@ -1054,7 +1138,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();
     {
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out
b/src/test/modules/test_extensions/expected/test_extensions.out
index f357cc21aa..b6370b3b4c 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -295,6 +295,16 @@ CREATE FUNCTION ext_cor_func() RETURNS text
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  function ext_cor_func() is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
+QUERY:  /* src/test/modules/test_extensions/test_ext_cor--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+
+
+-- It's generally bad style to use CREATE OR REPLACE unnecessarily.
+-- Test what happens if an extension does it anyway.
+
+CREATE OR REPLACE FUNCTION ext_cor_func() RETURNS text
+  AS $$ SELECT 'ext_cor_func: from extension'::text $$ LANGUAGE sql
+CONTEXT:  extension script file "test_ext_cor--1.0.sql"
 SELECT ext_cor_func();
       ext_cor_func
 ------------------------
@@ -307,6 +317,9 @@ CREATE VIEW ext_cor_view AS
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  view ext_cor_view is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
+QUERY:  CREATE OR REPLACE VIEW ext_cor_view AS
+  SELECT 'ext_cor_view: from extension'::text AS col
+CONTEXT:  extension script file "test_ext_cor--1.0.sql"
 SELECT ext_cor_func();
 ERROR:  function ext_cor_func() does not exist
 LINE 1: SELECT ext_cor_func();
@@ -323,6 +336,11 @@ CREATE TYPE test_ext_type;
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  type test_ext_type is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
+QUERY:  -- These are for testing replacement of a shell type/operator, which works
+-- enough like an implicit OR REPLACE to be important to check.
+
+CREATE TYPE test_ext_type AS ENUM('x', 'y')
+CONTEXT:  extension script file "test_ext_cor--1.0.sql"
 DROP TYPE test_ext_type;
 -- this makes a shell "point <<@@ polygon" operator too
 CREATE OPERATOR @@>> ( PROCEDURE = poly_contain_pt,
@@ -331,6 +349,9 @@ CREATE OPERATOR @@>> ( PROCEDURE = poly_contain_pt,
 CREATE EXTENSION test_ext_cor;  -- fail
 ERROR:  operator <<@@(point,polygon) is not a member of extension "test_ext_cor"
 DETAIL:  An extension is not allowed to replace an object that it does not own.
+QUERY:  CREATE OPERATOR <<@@ ( PROCEDURE = pt_contained_poly,
+  LEFTARG = point, RIGHTARG = polygon )
+CONTEXT:  extension script file "test_ext_cor--1.0.sql"
 DROP OPERATOR <<@@ (point, polygon);
 CREATE EXTENSION test_ext_cor;  -- now it should work
 SELECT ext_cor_func();
@@ -379,37 +400,61 @@ CREATE COLLATION ext_cine_coll
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  collation ext_cine_coll is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  /* src/test/modules/test_extensions/test_ext_cine--1.0.sql */
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+
+
+--
+-- CREATE IF NOT EXISTS is an entirely unsound thing for an extension
+-- to be doing, but let's at least plug the major security hole in it.
+--
+
+CREATE COLLATION IF NOT EXISTS ext_cine_coll
+  ( LC_COLLATE = "POSIX", LC_CTYPE = "POSIX" )
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP COLLATION ext_cine_coll;
 CREATE MATERIALIZED VIEW ext_cine_mv AS SELECT 11 AS f1;
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  materialized view ext_cine_mv is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE MATERIALIZED VIEW IF NOT EXISTS ext_cine_mv AS SELECT 42 AS f1
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP MATERIALIZED VIEW ext_cine_mv;
 CREATE FOREIGN DATA WRAPPER dummy;
 CREATE SERVER ext_cine_srv FOREIGN DATA WRAPPER dummy;
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  server ext_cine_srv is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE SERVER IF NOT EXISTS ext_cine_srv FOREIGN DATA WRAPPER ext_cine_fdw
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP SERVER ext_cine_srv;
 CREATE SCHEMA ext_cine_schema;
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  schema ext_cine_schema is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE SCHEMA IF NOT EXISTS ext_cine_schema
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP SCHEMA ext_cine_schema;
 CREATE SEQUENCE ext_cine_seq;
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  sequence ext_cine_seq is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE SEQUENCE IF NOT EXISTS ext_cine_seq
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP SEQUENCE ext_cine_seq;
 CREATE TABLE ext_cine_tab1 (x int);
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  table ext_cine_tab1 is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE TABLE IF NOT EXISTS ext_cine_tab1 (x int)
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP TABLE ext_cine_tab1;
 CREATE TABLE ext_cine_tab2 AS SELECT 42 AS y;
 CREATE EXTENSION test_ext_cine;  -- fail
 ERROR:  table ext_cine_tab2 is not a member of extension "test_ext_cine"
 DETAIL:  An extension may only use CREATE ... IF NOT EXISTS to skip object creation if the conflicting object is one
thatit already owns. 
+QUERY:  CREATE TABLE IF NOT EXISTS ext_cine_tab2 AS SELECT 42 AS y
+CONTEXT:  extension script file "test_ext_cine--1.0.sql"
 DROP TABLE ext_cine_tab2;
 CREATE EXTENSION test_ext_cine;
 \dx+ test_ext_cine
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b6135f0347..9620d3e9d2 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3882,6 +3882,7 @@ saophash_hash
 save_buffer
 scram_state
 scram_state_enum
+script_error_callback_arg
 security_class_t
 sem_t
 sepgsql_context_info_t