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

Christoph Berg <myon@debian.org> writes:
> Re: Tom Lane
>> (It might be worth some effort to trim away comments appearing
>> just before a command, but I didn't tackle that here.)

> The "error when psql" comments do look confusing, but I guess in other
> places the comment just before the query adds valuable context, so I'd
> say leaving the comments in is ok.

It looks like if we did want to suppress that, the right fix is to
make gram.y track statement start locations more honestly, as in
0002 attached (0001 is the same as before).  This'd add a few cycles
per grammar nonterminal reduction, which is kind of annoying but
probably is negligible in the grand scheme of things.  Still, I'd
not propose it just for this.  But if memory serves, we've had
previous complaints about pg_stat_statements failing to strip
leading comments from queries, and this'd fix that.  I think it
likely also improves error cursor positioning for cases involving
empty productions --- I'm a tad surprised that no other regression
cases changed.

            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
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b1d4642c59..4d5f197a1c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -67,39 +67,23 @@


 /*
- * Location tracking support --- simpler than bison's default, since we only
- * want to track the start position not the end position of each nonterminal.
+ * Location tracking support.  Unlike bison's default, we only want
+ * to track the start position not the end position of each nonterminal.
+ * Nonterminals that reduce to empty receive position "-1".
  */
 #define YYLLOC_DEFAULT(Current, Rhs, N) \
     do { \
-        if ((N) > 0) \
-            (Current) = (Rhs)[1]; \
-        else \
-            (Current) = (-1); \
+        (Current) = (-1); \
+        for (int _i_ = 1; _i_ <= (N); _i_++) \
+        { \
+            if ((Rhs)[_i_] >= 0) \
+            { \
+                (Current) = (Rhs)[_i_]; \
+                break; \
+            } \
+        } \
     } while (0)

-/*
- * The above macro assigns -1 (unknown) as the parse location of any
- * nonterminal that was reduced from an empty rule, or whose leftmost
- * component was reduced from an empty rule.  This is problematic
- * for nonterminals defined like
- *        OptFooList: / * EMPTY * / { ... } | OptFooList Foo { ... } ;
- * because we'll set -1 as the location during the first reduction and then
- * copy it during each subsequent reduction, leaving us with -1 for the
- * location even when the list is not empty.  To fix that, do this in the
- * action for the nonempty rule(s):
- *        if (@$ < 0) @$ = @2;
- * (Although we have many nonterminals that follow this pattern, we only
- * bother with fixing @$ like this when the nonterminal's parse location
- * is actually referenced in some rule.)
- *
- * A cleaner answer would be to make YYLLOC_DEFAULT scan all the Rhs
- * locations until it's found one that's not -1.  Then we'd get a correct
- * location for any nonterminal that isn't entirely empty.  But this way
- * would add overhead to every rule reduction, and so far there's not been
- * a compelling reason to pay that overhead.
- */
-
 /*
  * Bison doesn't allocate anything that needs to live across parser calls,
  * so we can easily have it use palloc instead of malloc.  This prevents
@@ -930,7 +914,7 @@ parse_toplevel:
             | MODE_PLPGSQL_EXPR PLpgSQL_Expr
             {
                 pg_yyget_extra(yyscanner)->parsetree =
-                    list_make1(makeRawStmt($2, 0));
+                    list_make1(makeRawStmt($2, @2));
             }
             | MODE_PLPGSQL_ASSIGN1 PLAssignStmt
             {
@@ -938,7 +922,7 @@ parse_toplevel:

                 n->nnames = 1;
                 pg_yyget_extra(yyscanner)->parsetree =
-                    list_make1(makeRawStmt((Node *) n, 0));
+                    list_make1(makeRawStmt((Node *) n, @2));
             }
             | MODE_PLPGSQL_ASSIGN2 PLAssignStmt
             {
@@ -946,7 +930,7 @@ parse_toplevel:

                 n->nnames = 2;
                 pg_yyget_extra(yyscanner)->parsetree =
-                    list_make1(makeRawStmt((Node *) n, 0));
+                    list_make1(makeRawStmt((Node *) n, @2));
             }
             | MODE_PLPGSQL_ASSIGN3 PLAssignStmt
             {
@@ -954,18 +938,13 @@ parse_toplevel:

                 n->nnames = 3;
                 pg_yyget_extra(yyscanner)->parsetree =
-                    list_make1(makeRawStmt((Node *) n, 0));
+                    list_make1(makeRawStmt((Node *) n, @2));
             }
         ;

 /*
  * At top level, we wrap each stmt with a RawStmt node carrying start location
- * and length of the stmt's text.  Notice that the start loc/len are driven
- * entirely from semicolon locations (@2).  It would seem natural to use
- * @1 or @3 to get the true start location of a stmt, but that doesn't work
- * for statements that can start with empty nonterminals (opt_with_clause is
- * the main offender here); as noted in the comments for YYLLOC_DEFAULT,
- * we'd get -1 for the location in such cases.
+ * and length of the stmt's text.
  * We also take care to discard empty statements entirely.
  */
 stmtmulti:    stmtmulti ';' toplevel_stmt
@@ -976,14 +955,14 @@ stmtmulti:    stmtmulti ';' toplevel_stmt
                         updateRawStmtEnd(llast_node(RawStmt, $1), @2);
                     }
                     if ($3 != NULL)
-                        $$ = lappend($1, makeRawStmt($3, @2 + 1));
+                        $$ = lappend($1, makeRawStmt($3, @3));
                     else
                         $$ = $1;
                 }
             | toplevel_stmt
                 {
                     if ($1 != NULL)
-                        $$ = list_make1(makeRawStmt($1, 0));
+                        $$ = list_make1(makeRawStmt($1, @1));
                     else
                         $$ = NIL;
                 }
@@ -1584,8 +1563,6 @@ CreateSchemaStmt:
 OptSchemaEltList:
             OptSchemaEltList schema_stmt
                 {
-                    if (@$ < 0)            /* see comments for YYLLOC_DEFAULT */
-                        @$ = @2;
                     $$ = lappend($1, $2);
                 }
             | /* EMPTY */
diff --git a/src/test/modules/test_extensions/expected/test_extensions.out
b/src/test/modules/test_extensions/expected/test_extensions.out
index b6370b3b4c..f77760d58e 100644
--- a/src/test/modules/test_extensions/expected/test_extensions.out
+++ b/src/test/modules/test_extensions/expected/test_extensions.out
@@ -295,14 +295,7 @@ 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
+QUERY:  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();
@@ -336,10 +329,7 @@ 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')
+QUERY:  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
@@ -400,16 +390,7 @@ 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
+QUERY:  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;