From: Tom Lane <tgl@sss.pgh.pa.us>
To: Christoph Berg <myon@debian.org>
Cc: Michael Banck <mbanck@gmx.net>
Cc: Tommy Pavlicek <tommypav122@gmail.com>
Cc: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: jian.universality@gmail.com
Subject: Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
Date: Thu, 26 Sep 2024 11:51:47 -0400
Message-ID: <325394.1727365907@sss.pgh.pa.us> (raw)
In-Reply-To: <ZvV1ClhnbJLCz7Sm@msg.df7cb.de>
References: <CAEhP-W-bY5CTXgrsH_J=NyKJpqMOgUPqMQ9-oTtx2N+zND1ZuA@mail.gmail.com>
<1092363.1695932314@sss.pgh.pa.us>
<CAEhP-W8MjoA0yDCipp=FgZvoQA-OVjQws3-XO6bpCWXunO8t8A@mail.gmail.com>
<447166.1696969927@sss.pgh.pa.us>
<CAEhP-W_1bMTLan2LtzS+xr=b-Wep56gw+OoirABQ8b2aOw+5yg@mail.gmail.com>
<ZTfL1G1fBPQHVPWV@msg.df7cb.de>
<560086.1698160614@sss.pgh.pa.us>
<ZTfjXzOKFI9AawJF@msg.df7cb.de>
<570833.1698162135@sss.pgh.pa.us>
<66f33840.050a0220.1f9e60.7fb9@mx.google.com>
<ZvV1ClhnbJLCz7Sm@msg.df7cb.de>
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
Attachments:extension-error-reporting-poc.patch (text/x-diff, 2.8K) downloaddiff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.cindex 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();
{
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, myon@debian.org, mbanck@gmx.net, tommypav122@gmail.com, ilmari@ilmari.org, tomas.vondra@enterprisedb.com, jian.universality@gmail.com
Subject: Re: [PATCH] Extend ALTER OPERATOR to support adding commutator, negator, hashes, and merges
In-Reply-To: <325394.1727365907@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox