Re: Better error reporting from extension scripts (Was: Extend ALTER OPERATOR)

[jian he]

On Tue, Oct 15, 2024 at 12:45 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> jian he <jian.universal...@gmail.com> writes:
> > On Mon, Oct 14, 2024 at 1:13 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> Right, but we might not have entered either of those previous
> >> if-blocks.
>
> > in src/backend/parser/gram.y
> > your makeRawStmt changes (v4) seem to guarantee that
> > RawStmt->stmt_location >= 0.
>
> Yes, I would expect that any RawStmt we see here will have valid
> stmt_location.  What you seem to be missing is that an error could
> be thrown from
>
> >     raw_parsetree_list = pg_parse_query(sql);
>
> before execute_sql_string reaches its loop over RawStmts.  In that
> case we'll reach script_error_callback with callback_arg.stmt_location
> still being -1.
>
Thanks for your reply.
I think I got it.



        if (location < 0 || syntaxerrposition < location ||
            (len > 0 && syntaxerrposition > location + len))
        {
            int            slen = strlen(query);
            location = len = 0;
            .....
        }
        else
            elog(INFO, "should not reached here");

just found out the"elog(INFO, "should not reached here");" part never reached.
in script_error_callback, for syntax error (error while parsing the raw string.)
we can bookkeeping the line number.

so we don't need to loop it again for syntax error in:
        for (query = callback_arg->sql; *query; query++)
        {
            if (--location < 0)
                break;
            if (*query == '\n')
                linenumber++;
        }
since we already did it in loop
"for (int loc = 0; loc < slen; loc++)"


i guess, we don't need performance in script_error_callback,
but in script_error_callback arrange code seperate  syntax error(raw
parser) and other error seems good.
please check the attached minor refactor.

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 86ea9cd9da..ff7d77f8e9 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -692,6 +692,8 @@ script_error_callback(void *arg)
 	const char *query = callback_arg->sql;
 	int			location = callback_arg->stmt_location;
 	int			len = callback_arg->stmt_len;
+	bool		raw_syntaxerr = false;
+	int			syntaxerrline_no = 0;
 	int			syntaxerrposition;
 	const char *lastslash;
 
@@ -730,6 +732,8 @@ script_error_callback(void *arg)
 			location = len = 0;
 			for (int loc = 0; loc < slen; loc++)
 			{
+				if (query[loc] == '\n')
+					syntaxerrline_no++;
 				if (query[loc] != ';')
 					continue;
 				if (query[loc + 1] == '\r')
@@ -766,6 +770,7 @@ script_error_callback(void *arg)
 		errposition(0);
 		internalerrposition(syntaxerrposition);
 		internalerrquery(pnstrdup(query, len));
+		raw_syntaxerr = true;
 	}
 	else if (location >= 0)
 	{
@@ -794,17 +799,22 @@ script_error_callback(void *arg)
 	 */
 	if (location >= 0)
 	{
-		int			linenumber = 1;
-
-		for (query = callback_arg->sql; *query; query++)
+		if (!raw_syntaxerr)
 		{
-			if (--location < 0)
-				break;
-			if (*query == '\n')
-				linenumber++;
+			int			linenumber = 1;
+			for (query = callback_arg->sql; *query; query++)
+			{
+				if (--location < 0)
+					break;
+				if (*query == '\n')
+					linenumber++;
+			}
+			errcontext("extension script file \"%s\", near line %d",
+					lastslash, linenumber);
 		}
-		errcontext("extension script file \"%s\", near line %d",
-				   lastslash, linenumber);
+		else
+			errcontext("extension script file \"%s\", near line %d",
+					lastslash, syntaxerrline_no);
 	}
 	else
 		errcontext("extension script file \"%s\"", lastslash);