[PROPOSAL] new diagnostic items for the dynamic sql
Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.
1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
position)
Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the position of
JOIN clause).
This will be helpful, while dealing with long queries.
Now, if we run the same statement as a dynamic SQL(by using EXECUTE <sql
statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.
Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET STACKED
DIAGNOSTICS
exec_me
---------
(1 row)
From the above results, by using all the existing diag items, we are unable
to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.
Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------
(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL statement.
We are attaching the patch for this proposal, and will be looking for your
inputs.
Regards,
Dinesh Kumar
Attachments:
01_diag_parse_sql_statement.patchtext/x-patch; charset=US-ASCII; name=01_diag_parse_sql_statement.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 5c546f630f..aee4bc1461 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2869,6 +2869,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_PARSE_SQL_STATEMENT</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>the parse failed sql statement, if any</entry>
+ </row>
+ <row>
+ <entry><literal>PG_PARSE_SQL_STATEMENT_POSITION</literal></entry>
+ <entry><type>text</type></entry>
+ <entry>the parse failed 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/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 2f473c14c4..b8bbfd314a 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2443,6 +2443,14 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, PLpgSQL_stmt_getdiag *stmt)
estate->cur_error->hint);
break;
+ case PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT:
+ exec_assign_c_string(estate, var, estate->cur_error->internalquery);
+ break;
+
+ case PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT_POSITION:
+ 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 ee60ced583..9086ebd20d 100644
--- a/src/pl/plpgsql/src/pl_funcs.c
+++ b/src/pl/plpgsql/src/pl_funcs.c
@@ -313,6 +313,10 @@ plpgsql_getdiag_kindname(PLpgSQL_getdiag_kind kind)
return "PG_EXCEPTION_DETAIL";
case PLPGSQL_GETDIAG_ERROR_HINT:
return "PG_EXCEPTION_HINT";
+ case PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT:
+ return "PG_PARSE_SQL_STATEMENT";
+ case PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT_POSITION:
+ return "PG_PARSE_SQL_STATEMENT_POSITION";
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 3e84162487..05c6e1c8a0 100644
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -323,6 +323,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_PARSE_SQL_STATEMENT
+%token <keyword> K_PG_PARSE_SQL_STATEMENT_POSITION
%token <keyword> K_PG_EXCEPTION_DETAIL
%token <keyword> K_PG_EXCEPTION_HINT
%token <keyword> K_PRINT_STRICT_PARAMS
@@ -998,6 +1000,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_PARSE_SQL_STATEMENT:
+ case PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT_POSITION:
case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
case PLPGSQL_GETDIAG_COLUMN_NAME:
case PLPGSQL_GETDIAG_CONSTRAINT_NAME:
@@ -1081,6 +1085,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_PARSE_SQL_STATEMENT, "pg_parse_sql_statement"))
+ $$ = PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT;
+ else if (tok_is_keyword(tok, &yylval, K_PG_PARSE_SQL_STATEMENT_POSITION, "pg_parse_sql_statement_position"))
+ $$ = PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT_POSITION;
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 99b3cf7d8a..ae1ef657d4 100644
--- a/src/pl/plpgsql/src/pl_unreserved_kwlist.h
+++ b/src/pl/plpgsql/src/pl_unreserved_kwlist.h
@@ -84,6 +84,8 @@ PG_KEYWORD("pg_datatype_name", K_PG_DATATYPE_NAME)
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_parse_sql_statement", K_PG_PARSE_SQL_STATEMENT)
+PG_KEYWORD("pg_parse_sql_statement_position", K_PG_PARSE_SQL_STATEMENT_POSITION)
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 0c3d30fb13..259c53c310 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -153,6 +153,8 @@ typedef enum PLpgSQL_getdiag_kind
PLPGSQL_GETDIAG_ERROR_CONTEXT,
PLPGSQL_GETDIAG_ERROR_DETAIL,
PLPGSQL_GETDIAG_ERROR_HINT,
+ PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT,
+ PLPGSQL_GETDIAG_PARSE_SQL_STATEMENT_POSITION,
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 d0a6b630b8..e76789857d 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5703,3 +5703,46 @@ END; $$ LANGUAGE plpgsql;
ERROR: "x" is not a scalar variable
LINE 3: GET DIAGNOSTICS x = ROW_COUNT;
^
+--
+-- PG_PARSE_SQL_STATEMENT, PG_PARSE_SQL_STATEMENT_POSITION
+-- should return parse exception details, if sql's parse operation fail
+--
+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_PARSE_SQL_STATEMENT,
+v_err_sql_pos = PG_PARSE_SQL_STATEMENT_POSITION;
+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 10
+--
+-- PG_PARSE_SQL_STATEMENT, PG_PARSE_SQL_STATEMENT_POSITION
+-- should return empty results, if sql's parse operation success
+--
+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_PARSE_SQL_STATEMENT,
+v_err_sql_pos = PG_PARSE_SQL_STATEMENT_POSITION;
+RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+NOTICE: exception sql
+NOTICE: exception sql position 0
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 07c60c80e4..092befa79f 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4647,3 +4647,44 @@ BEGIN
GET DIAGNOSTICS x = ROW_COUNT;
RETURN;
END; $$ LANGUAGE plpgsql;
+
+--
+-- PG_PARSE_SQL_STATEMENT, PG_PARSE_SQL_STATEMENT_POSITION
+-- should return parse exception details, if sql's parse operation fail
+--
+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_PARSE_SQL_STATEMENT,
+v_err_sql_pos = PG_PARSE_SQL_STATEMENT_POSITION;
+RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
+
+--
+-- PG_PARSE_SQL_STATEMENT, PG_PARSE_SQL_STATEMENT_POSITION
+-- should return empty results, if sql's parse operation success
+--
+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_PARSE_SQL_STATEMENT,
+v_err_sql_pos = PG_PARSE_SQL_STATEMENT_POSITION;
+RAISE NOTICE 'exception sql %', v_err_sql_stmt;
+RAISE NOTICE 'exception sql position %', v_err_sql_pos;
+END;
+$$;
Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the position
of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE <sql
statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET STACKED
DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL statement.We are attaching the patch for this proposal, and will be looking for your
inputs.
+1 It is good idea. I am not sure if the used names are good. I propose
PG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Show quoted text
Regards,
Dinesh Kumar
On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the position
of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE <sql
statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL statement.We are attaching the patch for this proposal, and will be looking for
your inputs.+1 It is good idea. I am not sure if the used names are good. I propose
PG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.
But, as we are only dealing with the parsing failure, I thought of adding
that to the diag name.
Regards,
Dinesh Kumar
Show quoted text
Regards,
Dinesh Kumar
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text cursor
position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the position
of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE <sql
statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18 at
EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL statement.We are attaching the patch for this proposal, and will be looking for
your inputs.+1 It is good idea. I am not sure if the used names are good. I propose
PG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.But, as we are only dealing with the parsing failure, I thought of adding
that to the diag name.
I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...
The idea is good, and you found the case, where it has benefits for users.
Naming is hard.
Regards
Pavel
Show quoted text
Regards,
Dinesh KumarRegards,
Dinesh Kumar
On Sun, 25 Jul 2021 at 16:34, Pavel Stehule <pavel.stehule@gmail.com> wrote:
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text
cursor position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the
position of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE
<sql statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18
at EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL
statement.We are attaching the patch for this proposal, and will be looking for
your inputs.+1 It is good idea. I am not sure if the used names are good. I propose
PG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.But, as we are only dealing with the parsing failure, I thought of adding
that to the diag name.I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...The idea is good, and you found the case, where it has benefits for users.
Naming is hard.
Thanks for your time and suggestions Pavel.
I updated the patch as per the suggestions, and attached it here for
further inputs.
Regards,
Dinesh Kumar
Show quoted text
Regards
Pavel
Regards,
Dinesh KumarRegards,
Dinesh Kumar
Attachments:
02_diag_parse_sql_statement.patchtext/x-patch; charset=US-ASCII; name=02_diag_parse_sql_statement.patchDownload
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 4cd4bcba80..3f732453e2 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2985,6 +2985,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 dynamic 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/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b5c208d83d..b6a977aa5c 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2402,6 +2402,14 @@ exec_stmt_getdiag(PLpgSQL_execstate *estate, PLpgSQL_stmt_getdiag *stmt)
estate->cur_error->hint);
break;
+ case PLPGSQL_GETDIAG_SQL_TEXT:
+ exec_assign_c_string(estate, var, estate->cur_error->internalquery);
+ 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 3fcca43b90..804faabb97 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 fcbfcd678b..6120837740 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 6ea169d9ad..4344d2f048 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -5701,3 +5701,46 @@ 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 empty results, if the dynamic sql is a valid
+--
+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
+NOTICE: exception sql position 0
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 781666a83a..3a53dc9e67 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4645,3 +4645,44 @@ 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 empty results, if the dynamic sql is a valid
+--
+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;
+$$;
Hi
pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
On Sun, 25 Jul 2021 at 16:34, Pavel Stehule <pavel.stehule@gmail.com>
wrote:
please, can you register this patch to commitfest app
https://commitfest.postgresql.org/34/
Regards
Pavel
Show quoted text
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text
cursor position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the
position of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE
<sql statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18
at EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic SQL
statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL
statement.We are attaching the patch for this proposal, and will be looking for
your inputs.+1 It is good idea. I am not sure if the used names are good. I propose
PG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.But, as we are only dealing with the parsing failure, I thought of
adding that to the diag name.I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...The idea is good, and you found the case, where it has benefits for
users. Naming is hard.Thanks for your time and suggestions Pavel.
I updated the patch as per the suggestions, and attached it here for
further inputs.Regards,
Dinesh KumarRegards
Pavel
Regards,
Dinesh KumarRegards,
Dinesh Kumar
Hi Pavel,
On Tue, 24 Aug 2021 at 00:19, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hi
pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sun, 25 Jul 2021 at 16:34, Pavel Stehule <pavel.stehule@gmail.com>
wrote:please, can you register this patch to commitfest app
https://commitfest.postgresql.org/34/
This patch is registered
https://commitfest.postgresql.org/34/3258/
Show quoted text
Regards
Pavel
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text
cursor position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the
position of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE
<sql statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line 18
at EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic
SQL statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL
statement.We are attaching the patch for this proposal, and will be looking for
your inputs.+1 It is good idea. I am not sure if the used names are good. I
proposePG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are much
better and generic.But, as we are only dealing with the parsing failure, I thought of
adding that to the diag name.I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...The idea is good, and you found the case, where it has benefits for
users. Naming is hard.Thanks for your time and suggestions Pavel.
I updated the patch as per the suggestions, and attached it here for
further inputs.Regards,
Dinesh KumarRegards
Pavel
Regards,
Dinesh KumarRegards,
Dinesh Kumar
út 24. 8. 2021 v 8:16 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
Hi Pavel,
On Tue, 24 Aug 2021 at 00:19, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sun, 25 Jul 2021 at 16:34, Pavel Stehule <pavel.stehule@gmail.com>
wrote:please, can you register this patch to commitfest app
https://commitfest.postgresql.org/34/
This patch is registered
ok, I looked it over.
Regards
Pavel
Show quoted text
Regards
Pavel
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:On Sat, 17 Jul 2021 at 01:29, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.kumar@migops.com> napsal:Hi Everyone,
We would like to propose the below 2 new plpgsql diagnostic items,
related to parsing. Because, the current diag items are not providing
the useful diagnostics about the dynamic SQL statements.1. PG_PARSE_SQL_STATEMENT (returns parse failed sql statement)
2. PG_PARSE_SQL_STATEMENT_POSITION (returns parse failed sql text
cursor position)Consider the below example, which is an invalid SQL statement.
postgres=# SELECT 1 JOIN SELECT 2;
ERROR: syntax error at or near "JOIN"
LINE 1: SELECT 1 JOIN SELECT 2;
^
Here, there is a syntax error at JOIN clause,
and also we are getting the syntax error position(^ symbol, the
position of JOIN clause).
This will be helpful, while dealing with long queries.Now, if we run the same statement as a dynamic SQL(by using EXECUTE
<sql statement>),
then it seems we are not getting the text cursor position,
and the SQL statement which is failing at parse level.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: RETURNED_SQLSTATE 42601
NOTICE: COLUMN_NAME
NOTICE: CONSTRAINT_NAME
NOTICE: PG_DATATYPE_NAME
NOTICE: MESSAGE_TEXT syntax error at or near "JOIN"
NOTICE: TABLE_NAME
NOTICE: SCHEMA_NAME
NOTICE: PG_EXCEPTION_DETAIL
NOTICE: PG_EXCEPTION_HINT
NOTICE: PG_EXCEPTION_CONTEXT PL/pgSQL function exec_me(text) line
18 at EXECUTE
NOTICE: PG_CONTEXT PL/pgSQL function exec_me(text) line 21 at GET
STACKED DIAGNOSTICS
exec_me
---------(1 row)
From the above results, by using all the existing diag items, we are
unable to get the position of "JOIN" in the submitted SQL statement.
By using these proposed diag items, we will be getting the required
information,
which will be helpful while running long SQL statements as dynamic
SQL statements.Please find the below example.
postgres=# SELECT exec_me('SELECT 1 JOIN SELECT 2');
NOTICE: PG_PARSE_SQL_STATEMENT SELECT 1 JOIN SELECT 2
NOTICE: PG_PARSE_SQL_STATEMENT_POSITION 10
exec_me
---------(1 row)
From the above results, by using these diag items,
we are able to get what is failing and it's position as well.
This information will be much helpful to debug the issue,
while a long running SQL statement is running as a dynamic SQL
statement.We are attaching the patch for this proposal, and will be looking
for your inputs.+1 It is good idea. I am not sure if the used names are good. I
proposePG_SQL_TEXT and PG_ERROR_LOCATION
Regards
Pavel
Thanks Pavel,
Sorry for the late reply.
The proposed diag items are `PG_SQL_TEXT`, `PG_ERROR_LOCATION` are
much better and generic.But, as we are only dealing with the parsing failure, I thought of
adding that to the diag name.I understand. But parsing is only one case - and these variables can be
used for any case. Sure, ***we don't want*** to have PG_PARSE_SQL_TEXT,
PG_ANALYZE_SQL_TEXT, PG_EXECUTION_SQL_TEXT ...The idea is good, and you found the case, where it has benefits for
users. Naming is hard.Thanks for your time and suggestions Pavel.
I updated the patch as per the suggestions, and attached it here for
further inputs.Regards,
Dinesh KumarRegards
Pavel
Regards,
Dinesh KumarRegards,
Dinesh Kumar
Hi
I tested the last patch, and I think I found unwanted behavior.
The value of PG_SQL_TEXT is not empty only when the error is related to the
parser stage. When the error is raised in the query evaluation stage, then
the value is empty.
I think this is too confusing. PL/pgSQL is a high level language, and the
behaviour should be consistent independent of internal implementation. I am
afraid this feature requires much more work.
postgres=# DO $$
DECLARE
err_sql_stmt TEXT;
err_sql_pos INT;
BEGIN
EXECUTE 'SELECT 1/0';
EXCEPTION
WHEN OTHERS THEN
GET STACKED DIAGNOSTICS
err_sql_stmt = PG_SQL_TEXT,
err_sql_pos = PG_ERROR_LOCATION;
RAISE NOTICE 'exception sql "%"', err_sql_stmt;
RAISE NOTICE 'exception sql position %', err_sql_pos;
END;
$$;
NOTICE: exception sql ""
NOTICE: exception sql position 0
DO
For this case, the empty result is not acceptable in this language. It is
too confusing. The implemented behaviour is well described in regress
tests, but I don't think it is user (developer) friendly. The location
field is not important, and can be 0 some times. But query text should be
not empty in all possible cases related to any query evaluation. I think
this can be a nice and useful feature, but the behavior should be
consistent.
Second, but minor, objection to this patch is zero formatting in a regress
test.
Regards
Pavel
On Thu, 9 Sept 2021 at 11:07, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hi
I tested the last patch, and I think I found unwanted behavior.
The value of PG_SQL_TEXT is not empty only when the error is related to
the parser stage. When the error is raised in the query evaluation stage,
then the value is empty.
I think this is too confusing. PL/pgSQL is a high level language, and the
behaviour should be consistent independent of internal implementation. I am
afraid this feature requires much more work.postgres=# DO $$
DECLARE
err_sql_stmt TEXT;
err_sql_pos INT;
BEGIN
EXECUTE 'SELECT 1/0';
EXCEPTION
WHEN OTHERS THEN
GET STACKED DIAGNOSTICS
err_sql_stmt = PG_SQL_TEXT,
err_sql_pos = PG_ERROR_LOCATION;
RAISE NOTICE 'exception sql "%"', err_sql_stmt;
RAISE NOTICE 'exception sql position %', err_sql_pos;
END;
$$;
NOTICE: exception sql ""
NOTICE: exception sql position 0
DOFor this case, the empty result is not acceptable in this language. It is
too confusing. The implemented behaviour is well described in regress
tests, but I don't think it is user (developer) friendly. The location
field is not important, and can be 0 some times. But query text should be
not empty in all possible cases related to any query evaluation. I think
this can be a nice and useful feature, but the behavior should be
consistent.Thanks for your time in evaluating this patch.
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.
Show quoted text
Second, but minor, objection to this patch is zero formatting in a regress
test.Regards
Pavel
čt 9. 9. 2021 v 8:23 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
On Thu, 9 Sept 2021 at 11:07, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
I tested the last patch, and I think I found unwanted behavior.
The value of PG_SQL_TEXT is not empty only when the error is related to
the parser stage. When the error is raised in the query evaluation stage,
then the value is empty.
I think this is too confusing. PL/pgSQL is a high level language, and the
behaviour should be consistent independent of internal implementation. I am
afraid this feature requires much more work.postgres=# DO $$
DECLARE
err_sql_stmt TEXT;
err_sql_pos INT;
BEGIN
EXECUTE 'SELECT 1/0';
EXCEPTION
WHEN OTHERS THEN
GET STACKED DIAGNOSTICS
err_sql_stmt = PG_SQL_TEXT,
err_sql_pos = PG_ERROR_LOCATION;
RAISE NOTICE 'exception sql "%"', err_sql_stmt;
RAISE NOTICE 'exception sql position %', err_sql_pos;
END;
$$;
NOTICE: exception sql ""
NOTICE: exception sql position 0
DOFor this case, the empty result is not acceptable in this language. It is
too confusing. The implemented behaviour is well described in regress
tests, but I don't think it is user (developer) friendly. The location
field is not important, and can be 0 some times. But query text should be
not empty in all possible cases related to any query evaluation. I think
this can be a nice and useful feature, but the behavior should be
consistent.Thanks for your time in evaluating this patch.
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.
sure
Pavel
Show quoted text
Second, but minor, objection to this patch is zero formatting in a
regress test.Regards
Pavel
On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.kumar@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?
--
Daniel Gustafsson https://vmware.com/
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 <daniel@yesql.se> wrote:
Show quoted text
On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.kumar@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?--
Daniel Gustafsson https://vmware.com/
Attachments:
03_diag_parse_sql_statement.patchapplication/x-patch; name=03_diag_parse_sql_statement.patchDownload
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..d705bf9e1c 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_current_err_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..42e2422e10 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_current_err_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_current_err_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..9343a1441a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -395,7 +395,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;
+$$;
Hi
pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <dinesh.kumar@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 <daniel@yesql.se> wrote:
On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.kumar@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
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.
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.
Regards
Pavel
Show quoted text
--
Daniel Gustafsson https://vmware.com/
Hi Pavel,
On Sun, 7 Nov 2021 at 12:53, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hi
pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
dinesh.kumar@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 <daniel@yesql.se> wrote:
On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.kumar@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.
Show quoted text
Regards
Pavel
--
Daniel Gustafsson https://vmware.com/
Attachments:
04_diag_parse_sql_statement.patchtext/x-patch; charset=US-ASCII; name=04_diag_parse_sql_statement.patchDownload
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;
+$$;
On Sun, Nov 7, 2021 at 5:23 AM Dinesh Chemuduru <dinesh.kumar@migops.com>
wrote:
Hi Pavel,
On Sun, 7 Nov 2021 at 12:53, Pavel Stehule <pavel.stehule@gmail.com>
wrote:Hi
pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
dinesh.kumar@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 <daniel@yesql.se> wrote:
On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh.kumar@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/Hi,
+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal query
(which doesn't cause exception).
The following code implies that only one of internalquery and current_query
would be set.
+ 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);Cheers
+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal
query (which doesn't cause exception).
The following code implies that only one of internalquery and
current_query would be set.
yes, I think so current_query is not a good name too. Maybe query can be
good enough - all in ErrorData is related to error
Show quoted 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);Cheers
po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:
+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal
query (which doesn't cause exception).
The following code implies that only one of internalquery and
current_query would be set.yes, I think so current_query is not a good name too. Maybe query can be
good enough - all in ErrorData is related to error
so the name of field can be query, and routine for setting errquery or
set_errquery
Show quoted 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);Cheers
po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:
po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal
query (which doesn't cause exception).
The following code implies that only one of internalquery and
current_query would be set.yes, I think so current_query is not a good name too. Maybe query can be
good enough - all in ErrorData is related to errorso the name of field can be query, and routine for setting errquery or
set_errquery
and this part is not correct
<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->set_errcurrent_query(query);
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}
<-->}
set_errcurrent_query should be outside the switch
We want PG_SQL_TEXT for assign statements too
_t := (select ...);
Regards
Pavel
Thanks Zhihong/Pavel,
On Mon, 8 Nov 2021 at 10:03, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Show quoted text
po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal
query (which doesn't cause exception).
The following code implies that only one of internalquery and
current_query would be set.yes, I think so current_query is not a good name too. Maybe query can be
good enough - all in ErrorData is related to errorso the name of field can be query, and routine for setting errquery or
set_errqueryand this part is not correct
<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->set_errcurrent_query(query);
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}
<-->}set_errcurrent_query should be outside the switch
We want PG_SQL_TEXT for assign statements too
_t := (select ...);
Please find the new patch, which has the suggested changes.
Regards
Pavel
Attachments:
05_diag_parse_sql_statement.patchtext/x-patch; charset=US-ASCII; name=05_diag_parse_sql_statement.patchDownload
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..a504bf9ed4 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2856,6 +2856,7 @@ _SPI_error_callback(void *arg)
}
else
{
+ set_errquery(query);
/* Use the parse mode to decide how to describe the query */
switch (carg->mode)
{
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..03bf3e7b45 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->query)
+ pfree(edata->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->query)
+ newedata->query = pstrdup(newedata->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->query)
+ pfree(edata->query);
pfree(edata);
}
@@ -1718,6 +1724,8 @@ ThrowErrorData(ErrorData *edata)
newedata->internalpos = edata->internalpos;
if (edata->internalquery)
newedata->internalquery = pstrdup(edata->internalquery);
+ if (edata->query)
+ newedata->query = pstrdup(edata->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->query)
+ newedata->query = pstrdup(newedata->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_errquery --- set the 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. query, which always returns the sql query which is causing the current exception
+ */
+int
+set_errquery(const char *query)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ if (edata->query)
+ {
+ pfree(edata->query);
+ edata->query = NULL;
+ }
+
+ if (query)
+ edata->query = MemoryContextStrdup(edata->assoc_context, query);
+
+ return 0;
+}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f53607e12e..e2cf49671e 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_errquery(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 *query; /* text 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..203e3a0596 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->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;
+$$;
Hi
po 8. 11. 2021 v 9:57 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com>
napsal:
Thanks Zhihong/Pavel,
On Mon, 8 Nov 2021 at 10:03, Pavel Stehule <pavel.stehule@gmail.com>
wrote:po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because
that's what the setter implies.
current_query may give the impression that the field can store normal
query (which doesn't cause exception).
The following code implies that only one of internalquery and
current_query would be set.yes, I think so current_query is not a good name too. Maybe query can
be good enough - all in ErrorData is related to errorso the name of field can be query, and routine for setting errquery or
set_errqueryand this part is not correct
<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->set_errcurrent_query(query);
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}
<-->}set_errcurrent_query should be outside the switch
We want PG_SQL_TEXT for assign statements too
_t := (select ...);
Please find the new patch, which has the suggested changes.
Now, I have only minor objection
+ <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>I think so an these text should be little bit enhanced
"the text of query or command of invalid sql statement (dynamic or
embedded)"
and
"the location of error of invalid dynamic text, if any"
I am not a native speaker, so I am sure my proposal can be enhanced too.
I have not any other objections
all tests passed without any problem
Regards
Pavel
Show quoted text
Regards
Pavel
Thanks for your time Pavel
On 09-Nov-2021, at 13:32, Pavel Stehule <pavel.stehule@gmail.com> wrote:
Hipo 8. 11. 2021 v 9:57 odesílatel Dinesh Chemuduru <dinesh.kumar@migops.com> napsal:
Thanks Zhihong/Pavel,
On Mon, 8 Nov 2021 at 10:03, Pavel Stehule <pavel.stehule@gmail.com> wrote:
po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule <pavel.stehule@gmail.com> napsal:
+set_errcurrent_query (const char *query)
You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's what the setter implies.
current_query may give the impression that the field can store normal query (which doesn't cause exception).
The following code implies that only one of internalquery and current_query would be set.yes, I think so current_query is not a good name too. Maybe query can be good enough - all in ErrorData is related to error
so the name of field can be query, and routine for setting errquery or set_errquery
and this part is not correct
<--><-->switch (carg->mode)
<--><-->{
<--><--><-->case RAW_PARSE_PLPGSQL_EXPR:
<--><--><--><-->errcontext("SQL expression \"%s\"", query);
<--><--><--><-->break;
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN1:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN2:
<--><--><-->case RAW_PARSE_PLPGSQL_ASSIGN3:
<--><--><--><-->errcontext("PL/pgSQL assignment \"%s\"", query);
<--><--><--><-->break;
<--><--><-->default:
<--><--><--><-->set_errcurrent_query(query);
<--><--><--><-->errcontext("SQL statement \"%s\"", query);
<--><--><--><-->break;
<--><-->}
<-->}set_errcurrent_query should be outside the switch
We want PG_SQL_TEXT for assign statements too
_t := (select ...);
Please find the new patch, which has the suggested changes.
Now, I have only minor objection + <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>I think so an these text should be little bit enhanced
"the text of query or command of invalid sql statement (dynamic or embedded)"
and
"the location of error of invalid dynamic text, if any"
I am not a native speaker, so I am sure my proposal can be enhanced too.
The proposed statements are much clear, but will wait for other’s suggestion, and will fix it accordingly.
Show quoted text
I have not any other objections
all tests passed without any problem
Regards
Pavel
Regards
Pavel
On Wed, Nov 10, 2021 at 01:58:58PM +0530, Dinesh Chemuduru wrote:
The proposed statements are much clear, but will wait for other’s
suggestion, and will fix it accordingly.
This update was three weeks ago, and no new version has been
provided, so I am marking this as returned with feedback in the CF
app. If you can work more on this proposal and send an updated patch,
please feel free to resubmit.
--
Michael
Hi Michael,
Attaching the latest patch here(It's the recent patch), and looking for
more suggestions/inputs from the team.
On Fri, 3 Dec 2021 at 13:09, Michael Paquier <michael@paquier.xyz> wrote:
Show quoted text
On Wed, Nov 10, 2021 at 01:58:58PM +0530, Dinesh Chemuduru wrote:
The proposed statements are much clear, but will wait for other’s
suggestion, and will fix it accordingly.This update was three weeks ago, and no new version has been
provided, so I am marking this as returned with feedback in the CF
app. If you can work more on this proposal and send an updated patch,
please feel free to resubmit.
--
Michael
Attachments:
05_diag_parse_sql_statement.patchapplication/octet-stream; name=05_diag_parse_sql_statement.patchDownload
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..a504bf9ed4 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2856,6 +2856,7 @@ _SPI_error_callback(void *arg)
}
else
{
+ set_errquery(query);
/* Use the parse mode to decide how to describe the query */
switch (carg->mode)
{
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index f33729513a..03bf3e7b45 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->query)
+ pfree(edata->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->query)
+ newedata->query = pstrdup(newedata->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->query)
+ pfree(edata->query);
pfree(edata);
}
@@ -1718,6 +1724,8 @@ ThrowErrorData(ErrorData *edata)
newedata->internalpos = edata->internalpos;
if (edata->internalquery)
newedata->internalquery = pstrdup(edata->internalquery);
+ if (edata->query)
+ newedata->query = pstrdup(edata->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->query)
+ newedata->query = pstrdup(newedata->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_errquery --- set the 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. query, which always returns the sql query which is causing the current exception
+ */
+int
+set_errquery(const char *query)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ if (edata->query)
+ {
+ pfree(edata->query);
+ edata->query = NULL;
+ }
+
+ if (query)
+ edata->query = MemoryContextStrdup(edata->assoc_context, query);
+
+ return 0;
+}
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index f53607e12e..e2cf49671e 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_errquery(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 *query; /* text 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..203e3a0596 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->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;
+$$;
On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru <dinesh.kumar@migops.com>
wrote:
Hi Michael,
Attaching the latest patch here(It's the recent patch), and looking for
more suggestions/inputs from the team.On Fri, 3 Dec 2021 at 13:09, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 10, 2021 at 01:58:58PM +0530, Dinesh Chemuduru wrote:
The proposed statements are much clear, but will wait for other’s
suggestion, and will fix it accordingly.This update was three weeks ago, and no new version has been
provided, so I am marking this as returned with feedback in the CF
app. If you can work more on this proposal and send an updated patch,
please feel free to resubmit.
--
MichaelHi,
+int
+set_errquery(const char *query)
Since the return value is ignored, the return type can be void.
Cheers
On Fri, 3 Dec 2021 at 22:04, Zhihong Yu <zyu@yugabyte.com> wrote:
On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru <dinesh.kumar@migops.com>
wrote:Hi Michael,
Attaching the latest patch here(It's the recent patch), and looking for
more suggestions/inputs from the team.On Fri, 3 Dec 2021 at 13:09, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Nov 10, 2021 at 01:58:58PM +0530, Dinesh Chemuduru wrote:
The proposed statements are much clear, but will wait for other’s
suggestion, and will fix it accordingly.This update was three weeks ago, and no new version has been
provided, so I am marking this as returned with feedback in the CF
app. If you can work more on this proposal and send an updated patch,
please feel free to resubmit.
--
MichaelHi,
+int
+set_errquery(const char *query)Agreed,
The other error log relateds functions are also not following the void as
return type and they are using the int.
So, I tried to submit the same behaviour.
See other error log related functions in src/backend/utils/error/elog.c
Show quoted text
Since the return value is ignored, the return type can be void.
Cheers
Hi Everyone,
Let me know if anything else is needed on my end
On Fri, 17 Dec 2021 at 10:54, Dinesh Chemuduru <dinesh.kumar@migops.com>
wrote:
Show quoted text
On Fri, 3 Dec 2021 at 22:04, Zhihong Yu <zyu@yugabyte.com> wrote:
On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru <dinesh.kumar@migops.com>
wrote:Hi Michael,
Attaching the latest patch here(It's the recent patch), and looking for
more suggestions/inputs from the team.On Fri, 3 Dec 2021 at 13:09, Michael Paquier <michael@paquier.xyz>
wrote:On Wed, Nov 10, 2021 at 01:58:58PM +0530, Dinesh Chemuduru wrote:
The proposed statements are much clear, but will wait for other’s
suggestion, and will fix it accordingly.This update was three weeks ago, and no new version has been
provided, so I am marking this as returned with feedback in the CF
app. If you can work more on this proposal and send an updated patch,
please feel free to resubmit.
--
MichaelHi,
+int
+set_errquery(const char *query)Agreed,
The other error log relateds functions are also not following the void as
return type and they are using the int.
So, I tried to submit the same behaviour.See other error log related functions in src/backend/utils/error/elog.c
Since the return value is ignored, the return type can be void.
Cheers
From looking at this patch and its history [1, 2], I think momentum was
probably lost during the January CF, where this patch was unregistered
(presumably by accident).
I've carried it forward, but it needs some help to keep from stalling
out. Definitely make sure it's rebased and up to date by the time the
next CF starts, to give it the best chance at getting additional review
(if you haven't received any by then).
--Jacob
[1]: https://commitfest.postgresql.org/34/3258/
[2]: https://commitfest.postgresql.org/38/3537/
On 8/2/22 15:09, Jacob Champion wrote:
I've carried it forward, but it needs some help to keep from stalling
out. Definitely make sure it's rebased and up to date by the time the
next CF starts, to give it the best chance at getting additional review
(if you haven't received any by then).
...and Dinesh's email has just bounced back undelivered. :(
Anybody interested in running with this? If no one speaks up, I think we
should return this as "needs more interest" before the next CF starts.
--Jacob
Jacob Champion <jchampion@timescale.com> writes:
...and Dinesh's email has just bounced back undelivered. :(
Anybody interested in running with this? If no one speaks up, I think we
should return this as "needs more interest" before the next CF starts.
Meh ... the last versions of the patch were far too invasive for a
use-case that seemed pretty hypothetical to begin with. It was never
explained why somebody would be trying to debug dynamic SQL without
use of the reporting that already exists:
regression=# do $$ begin
regression$# execute 'SELECT 1 JOIN SELECT 2';
regression$# end $$;
ERROR: syntax error at or near "SELECT"
LINE 1: SELECT 1 JOIN SELECT 2
^
QUERY: SELECT 1 JOIN SELECT 2
CONTEXT: PL/pgSQL function inline_code_block line 2 at EXECUTE
psql didn't provide that query text and cursor position out of thin air.
Now admittedly, what it did base that on is the PG_DIAG_INTERNAL_QUERY and
PG_DIAG_INTERNAL_POSITION fields of the error report, and the fact that
those aren't available to plpgsql error trapping logic is arguably a
deficiency. It's not a big deficiency, because what an EXCEPTION clause
probably ought to do in a case like this is just re-RAISE, which will
preserve those fields in the eventual client error report. But maybe
it's worth fixing.
I think the real reason this patch stalled is that Pavel wanted the
goal posts moved into the next stadium. Rather than just duplicate
the functionality available in the wire protocol, he wanted some other
definition entirely, hiding the fact that not every error report has
those fields. There isn't infrastructure for that, and I doubt that
this patch is enough to create it, even if there were consensus that
the definition is right. If we were to go forward, I'd recommend
reverting to a wire-protocol-equivalent definition, and just returning
NULL in the cases where the data isn't supplied.
regards, tom lane
2022年8月3日(水) 7:56 Tom Lane <tgl@sss.pgh.pa.us>:
Jacob Champion <jchampion@timescale.com> writes:
...and Dinesh's email has just bounced back undelivered. :(
Anybody interested in running with this? If no one speaks up, I think we
should return this as "needs more interest" before the next CF starts.Meh ... the last versions of the patch were far too invasive for a
use-case that seemed pretty hypothetical to begin with. It was never
explained why somebody would be trying to debug dynamic SQL without
use of the reporting that already exists:regression=# do $$ begin
regression$# execute 'SELECT 1 JOIN SELECT 2';
regression$# end $$;
ERROR: syntax error at or near "SELECT"
LINE 1: SELECT 1 JOIN SELECT 2
^
QUERY: SELECT 1 JOIN SELECT 2
CONTEXT: PL/pgSQL function inline_code_block line 2 at EXECUTEpsql didn't provide that query text and cursor position out of thin air.
Now admittedly, what it did base that on is the PG_DIAG_INTERNAL_QUERY and
PG_DIAG_INTERNAL_POSITION fields of the error report, and the fact that
those aren't available to plpgsql error trapping logic is arguably a
deficiency. It's not a big deficiency, because what an EXCEPTION clause
probably ought to do in a case like this is just re-RAISE, which will
preserve those fields in the eventual client error report. But maybe
it's worth fixing.I think the real reason this patch stalled is that Pavel wanted the
goal posts moved into the next stadium. Rather than just duplicate
the functionality available in the wire protocol, he wanted some other
definition entirely, hiding the fact that not every error report has
those fields. There isn't infrastructure for that, and I doubt that
this patch is enough to create it, even if there were consensus that
the definition is right. If we were to go forward, I'd recommend
reverting to a wire-protocol-equivalent definition, and just returning
NULL in the cases where the data isn't supplied.
I think given this patch has gone nowhere for the past year, we can mark
it as returned with feedback. If there's potential for the items mentioned
by Tom and someone wants to run with them, that'd be better done
with a fresh entry, maybe referencing this one.
Regards
Ian Barwick