[PROPOSAL] new diagnostic items for the dynamic sql

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

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

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

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

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

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/

ú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

https://commitfest.postgresql.org/34/3258/

ok, I looked it over.

Regards

Pavel

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
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.

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.

č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
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.

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

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:

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

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.

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

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

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 ...);

Regards

Pavel

Thanks Zhihong/Pavel,

On Mon, 8 Nov 2021 at 10:03, Pavel Stehule <pavel.stehule@gmail.com> wrote: