Обсуждение: [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

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

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

 

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

Regards,

Dinesh Kumar

 

Regards,

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

 

Regards

Pavel

Regards,

Dinesh Kumar

 

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

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

 

Regards

Pavel

Regards,

Dinesh Kumar

 

Regards,

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/

 

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

 

Regards

Pavel

Regards,

Dinesh Kumar

 

Regards,

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

 

Second, but minor, objection to this patch is zero formatting in a regress test.

Regards

Pavel

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?

--
Daniel Gustafsson               https://vmware.com/

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

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

 

Regards

Pavel

--
Daniel Gustafsson               https://vmware.com/

+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

+               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

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:

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.

 

Regards

Pavel

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

Hi,

+int

+set_errquery(const char *query)

Since the return value is ignored, the return type can be void.

Cheers