Hi Pavel,
On Sun, 7 Nov 2021 at 12:53, Pavel Stehule <pavel.steh...@gmail.com> wrote:
> Hi
>
> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> Hi Daniel,
>>
>> Thank you for your follow up, and attaching a new patch which addresses
>> Pavel's comments.
>> Let me know If I miss anything here.
>>
>>
>> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson <dan...@yesql.se> wrote:
>>
>>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.ku...@migops.com>
>>> wrote:
>>>
>>> > Let me try to fix the suggested case(I tried to fix this case in the
>>> past, but this time I will try to spend more time on this), and will submit
>>> a new patch.
>>>
>>> This CF entry is marked Waiting on Author, have you had the chance to
>>> prepare a
>>> new version of the patch addressing the comments from Pavel?
>>>
>>
> I think the functionality is correct. I am not sure about implementation
>
>
Thank you for your time in validating this patch.
> 1. Is it necessary to introduce a new field "current_query"? Cannot be
> used "internalquery" instead? Introducing a new field opens some questions
> - what is difference between internalquery and current_query, and where and
> when have to be used first and when second? ErrorData is a fundamental
> generic structure for Postgres, and can be confusing to enhance it by one
> field just for one purpose, that is not used across Postgres.
>
> Internalquery is the one, which was generated by another command.
For example, "DROP <OBJECT> CASCADE"(current_query) will generate many
internal query statements for each of the dependent objects.
At this moment, we do save the current_query in PG_CONTEXT.
As we know, PG_CONTEXT returns the whole statements as stacktrace and it's
hard to fetch the required SQL from it.
I failed to see another way to access the current_query apart from the
PG_CONTEXT.
Hence, we have introduced the new member "current_query" to the "ErrorData"
object.
We tested the internalquery for this purpose, but there are few(10+ unit
test) test cases that failed.
This is also another reason we added this new member to the "ErrorData",
and with the current patch all test cases are successful,
and we are not disturbing the existing functionality.
> 2. The name set_current_err_query is not consistent with names in elog.c -
> probably something like errquery or set_errquery or set_errcurrent_query
> can be more consistent with other names.
>
> Updated as per your suggestion
Please find the new patch version.
> Regards
>
> Pavel
>
>
>
>>> --
>>> Daniel Gustafsson https://vmware.com/
>>>
>>>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index e5c1356d8c..f402c9f012 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2995,6 +2995,16 @@ GET STACKED DIAGNOSTICS <replaceable>variable</replaceable> { = | := } <replacea
<entry><type>text</type></entry>
<entry>the name of the schema related to exception</entry>
</row>
+ <row>
+ <entry><literal>PG_SQL_TEXT</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>invalid sql statement, if any</entry>
+ </row>
+ <row>
+ <entry><literal>PG_ERROR_LOCATION</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>invalid dynamic sql statement's text cursor position, if any</entry>
+ </row>
<row>
<entry><literal>PG_EXCEPTION_DETAIL</literal></entry>
<entry><type>text</type></entry>
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 0568ae123f..121507b90b 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2868,6 +2868,7 @@ _SPI_error_callback(void *arg)
errcontext("PL/pgSQL assignment \"%s\"", query);
break;
default:
+ set_errcurrent_query(query);
errcontext("SQL statement \"%s\"", query);
break;
}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..a070de54b0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -621,6 +621,8 @@ errfinish(const char *filename, int lineno, const char *funcname)
pfree(edata->constraint_name);
if (edata->internalquery)
pfree(edata->internalquery);
+ if (edata->current_query)
+ pfree(edata->current_query);
errordata_stack_depth--;
@@ -1599,6 +1601,8 @@ CopyErrorData(void)
newedata->constraint_name = pstrdup(newedata->constraint_name);
if (newedata->internalquery)
newedata->internalquery = pstrdup(newedata->internalquery);
+ if (newedata->current_query)
+ newedata->current_query = pstrdup(newedata->current_query);
/* Use the calling context for string allocation */
newedata->assoc_context = CurrentMemoryContext;
@@ -1639,6 +1643,8 @@ FreeErrorData(ErrorData *edata)
pfree(edata->constraint_name);
if (edata->internalquery)
pfree(edata->internalquery);
+ if (edata->current_query)
+ pfree(edata->current_query);
pfree(edata);
}
@@ -1718,6 +1724,8 @@ ThrowErrorData(ErrorData *edata)
newedata->internalpos = edata->internalpos;
if (edata->internalquery)
newedata->internalquery = pstrdup(edata->internalquery);
+ if (edata->current_query)
+ newedata->current_query = pstrdup(edata->current_query);
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
@@ -1784,6 +1792,8 @@ ReThrowError(ErrorData *edata)
newedata->constraint_name = pstrdup(newedata->constraint_name);
if (newedata->internalquery)
newedata->internalquery = pstrdup(newedata->internalquery);
+ if (newedata->current_query)
+ newedata->current_query = pstrdup(newedata->current_query);
/* Reset the assoc_context to be ErrorContext */
newedata->assoc_context = ErrorContext;
@@ -3602,3 +3612,34 @@ trace_recovery(int trace_level)
return trace_level;
}
+
+/*
+ * set_errcurrent_query --- set the current query in the PL/PGSQL block, which is causing the exception
+ *
+ * We have PG_CONTEXT diagnostic item, which returns a text string with line(s) of text describing the call stack.
+ * To get the exact SQL query which is causing the error requires some text processing.
+ *
+ * To skip the text processing to get the sql query, we store the SQL statement in the 'ErrorData' object, and return the plain SQL when the user requests for it.
+ * In PL/PGSQL context, at this moment the `ErrorData` holds two types of SQL error members.
+ * 1. internalquery, which always returns the sql query which is not a valid sql statement
+ * 2. current_query, which always returns the sql query which is causing the current exception
+ */
+int
+set_errcurrent_query (const char *query)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ if (edata->current_query)
+ {
+ pfree(edata->current_query);
+ edata->current_query = NULL;
+ }
+
+ if (query)
+ edata->current_query = MemoryContextStrdup(edata->assoc_context, query);
+
+ return 0;
+}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f53607e12e..574e5ffa8b 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -216,6 +216,7 @@ extern int errposition(int cursorpos);
extern int internalerrposition(int cursorpos);
extern int internalerrquery(const char *query);
+extern int set_errcurrent_query(const char* query);
extern int err_generic_string(int field, const char *str);
@@ -395,7 +396,7 @@ typedef struct ErrorData
int internalpos; /* cursor index into internalquery */
char *internalquery; /* text of internally-generated query */
int saved_errno; /* errno at entry */
-
+ char *current_query; /* text of current query */
/* context containing associated non-constant strings */
struct MemoryContextData *assoc_context;
} ErrorData;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6dbfdb7be0..d0aeaee736 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2416,6 +2416,17 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, PLpgSQL_stmt_getdiag *stmt)
estate->cur_error->hint);
break;
+ case PLPGSQL_GETDIAG_SQL_TEXT:
+ if (estate->cur_error->internalquery)
+ exec_assign_c_string(estate, var, estate->cur_error->internalquery);
+ else
+ exec_assign_c_string(estate, var, estate->cur_error->current_query);
+ break;
+
+ case PLPGSQL_GETDIAG_ERROR_LOCATION:
+ exec_assign_value(estate, var, UInt64GetDatum(estate->cur_error->internalpos), false, INT8OID, -1);
+ break;
+
case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
exec_assign_c_string(estate, var,
unpack_sql_state(estate->cur_error->sqlerrcode));
diff --git a/src/pl/plpgsql/src/pl_funcs.c b/src/pl/plpgsql/src/pl_funcs.c
index e0863acb3d..f00a745e9a 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -311,6 +311,10 @@ plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind)
return "PG_EXCEPTION_DETAIL";
case PLPGSQL_GETDIAG_ERROR_HINT:
return "PG_EXCEPTION_HINT";
+ case PLPGSQL_GETDIAG_SQL_TEXT:
+ return "PG_SQL_TEXT";
+ case PLPGSQL_GETDIAG_ERROR_LOCATION:
+ return "PG_ERROR_LOCATION";
case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
return "RETURNED_SQLSTATE";
case PLPGSQL_GETDIAG_COLUMN_NAME:
diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 0f6a5b30b1..0fd0d9c179 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -321,6 +321,8 @@ static void check_raise_parameters(PLpgSQL_stmt_raise *stmt);
%token <keyword> K_PG_CONTEXT
%token <keyword> K_PG_DATATYPE_NAME
%token <keyword> K_PG_EXCEPTION_CONTEXT
+%token <keyword> K_PG_SQL_TEXT
+%token <keyword> K_PG_ERROR_LOCATION
%token <keyword> K_PG_EXCEPTION_DETAIL
%token <keyword> K_PG_EXCEPTION_HINT
%token <keyword> K_PRINT_STRICT_PARAMS
@@ -1047,6 +1049,8 @@ stmt_getdiag : K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
case PLPGSQL_GETDIAG_ERROR_CONTEXT:
case PLPGSQL_GETDIAG_ERROR_DETAIL:
case PLPGSQL_GETDIAG_ERROR_HINT:
+ case PLPGSQL_GETDIAG_SQL_TEXT:
+ case PLPGSQL_GETDIAG_ERROR_LOCATION:
case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
case PLPGSQL_GETDIAG_COLUMN_NAME:
case PLPGSQL_GETDIAG_CONSTRAINT_NAME:
@@ -1130,6 +1134,10 @@ getdiag_item :
else if (tok_is_keyword(tok, &yylval,
K_PG_EXCEPTION_CONTEXT, "pg_exception_context"))
$$ = PLPGSQL_GETDIAG_ERROR_CONTEXT;
+ else if (tok_is_keyword(tok, &yylval, K_PG_SQL_TEXT, "pg_sql_text"))
+ $$ = PLPGSQL_GETDIAG_SQL_TEXT;
+ else if (tok_is_keyword(tok, &yylval, K_PG_ERROR_LOCATION, "pg_error_location"))
+ $$ = PLPGSQL_GETDIAG_ERROR_LOCATION;
else if (tok_is_keyword(tok, &yylval,
K_COLUMN_NAME, "column_name"))
$$ = PLPGSQL_GETDIAG_COLUMN_NAME;
diff --git a/src/pl/plpgsql/src/pl_unreserved_kwlist.h b/src/pl/plpgsql/src/pl_unreserved_kwlist.h
index fcb34f7c7f..233cadf59c 100644
--- a/src/pl/plpgsql/src/pl_unreserved_kwlist.h
+++ b/src/pl/plpgsql/src/pl_unreserved_kwlist.h
@@ -81,9 +81,11 @@ PG_KEYWORD("option", K_OPTION)
PG_KEYWORD("perform", K_PERFORM)
PG_KEYWORD("pg_context", K_PG_CONTEXT)
PG_KEYWORD("pg_datatype_name", K_PG_DATATYPE_NAME)
+PG_KEYWORD("pg_error_location", K_PG_ERROR_LOCATION)
PG_KEYWORD("pg_exception_context", K_PG_EXCEPTION_CONTEXT)
PG_KEYWORD("pg_exception_detail", K_PG_EXCEPTION_DETAIL)
PG_KEYWORD("pg_exception_hint", K_PG_EXCEPTION_HINT)
+PG_KEYWORD("pg_sql_text", K_PG_SQL_TEXT)
PG_KEYWORD("print_strict_params", K_PRINT_STRICT_PARAMS)
PG_KEYWORD("prior", K_PRIOR)
PG_KEYWORD("query", K_QUERY)
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 3936866bb7..6bb4f2c9e4 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -151,6 +151,8 @@ typedef enum PLpgSQL_getdiag_kind
PLPGSQL_GETDIAG_ERROR_CONTEXT,
PLPGSQL_GETDIAG_ERROR_DETAIL,
PLPGSQL_GETDIAG_ERROR_HINT,
+ PLPGSQL_GETDIAG_SQL_TEXT,
+ PLPGSQL_GETDIAG_ERROR_LOCATION,
PLPGSQL_GETDIAG_RETURNED_SQLSTATE,
PLPGSQL_GETDIAG_COLUMN_NAME,
PLPGSQL_GETDIAG_CONSTRAINT_NAME,
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 33eb5e54b9..c9845248c9 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5752,3 +5752,67 @@ END; $$ LANGUAGE plpgsql;
ERROR: "x" is not a scalar variable
LINE 3: GET DIAGNOSTICS x = ROW_COUNT;
^
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and text cursor position, if the dynamic sql is syntactically incorrect
+--
+DO
+$$
+DECLARE
+ v_sql TEXT:='SELECT 1 JOIN SELECT 2';
+ v_err_sql_stmt TEXT;
+ v_err_sql_pos INT;
+BEGIN
+ EXECUTE v_sql;
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+NOTICE: exception sql SELECT 1 JOIN SELECT 2
+NOTICE: exception sql position 15
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and an empty cursor position, if the dynamic sql is valid and throw the exception
+--
+DO
+$$
+DECLARE
+ v_err_sql_pos INT;
+ v_err_sql_stmt TEXT;
+BEGIN
+ EXECUTE 'SELECT 1/0';
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+NOTICE: exception sql SELECT 1/0
+NOTICE: exception sql position 0
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and an empty cursor position, if the sql is a valid and throw the exception
+--
+DO
+$$
+DECLARE
+ v_err_sql_pos INT;
+ v_err_sql_stmt TEXT;
+BEGIN
+ PERFORM 1/0;
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+NOTICE: exception sql SELECT 1/0
+NOTICE: exception sql position 0
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index bffb7b703b..1d9a8b043a 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4687,3 +4687,64 @@ BEGIN
GET DIAGNOSTICS x = ROW_COUNT;
RETURN;
END; $$ LANGUAGE plpgsql;
+
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and text cursor position, if the dynamic sql is syntactically incorrect
+--
+DO
+$$
+DECLARE
+ v_sql TEXT:='SELECT 1 JOIN SELECT 2';
+ v_err_sql_stmt TEXT;
+ v_err_sql_pos INT;
+BEGIN
+ EXECUTE v_sql;
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and an empty cursor position, if the dynamic sql is valid and throw the exception
+--
+DO
+$$
+DECLARE
+ v_err_sql_pos INT;
+ v_err_sql_stmt TEXT;
+BEGIN
+ EXECUTE 'SELECT 1/0';
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+
+--
+-- PG_SQL_TEXT, PG_ERROR_LOCATION
+-- should return sql and an empty cursor position, if the sql is a valid and throw the exception
+--
+DO
+$$
+DECLARE
+ v_err_sql_pos INT;
+ v_err_sql_stmt TEXT;
+BEGIN
+ PERFORM 1/0;
+EXCEPTION
+WHEN OTHERS THEN
+ GET STACKED DIAGNOSTICS v_err_sql_stmt = PG_SQL_TEXT,
+ v_err_sql_pos = PG_ERROR_LOCATION;
+ RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+ RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
