Re: [PROPOSAL] new diagnostic items for the dynamic sql
2022年8月3日(水) 7:56 Tom Lane : > > Jacob Champion 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. 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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Jacob Champion 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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
>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/
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi Everyone, Let me know if anything else is needed on my end On Fri, 17 Dec 2021 at 10:54, Dinesh Chemuduru wrote: > > > On Fri, 3 Dec 2021 at 22:04, Zhihong Yu wrote: > >> >> >> On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru >> 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 >>> 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) >> >> 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 >> >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Fri, 3 Dec 2021 at 22:04, Zhihong Yu wrote: > > > On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru > 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 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) > > 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 >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Fri, Dec 3, 2021 at 3:15 AM Dinesh Chemuduru 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 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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
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 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 > 05_diag_parse_sql_statement.patch Description: Binary data
Re: [PROPOSAL] new diagnostic items for the dynamic sql
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 signature.asc Description: PGP signature
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Thanks for your time Pavel
> On 09-Nov-2021, at 13:32, Pavel Stehule wrote:
>
>
> Hi
>
> po 8. 11. 2021 v 9:57 odesílatel Dinesh Chemuduru
> napsal:
>> Thanks Zhihong/Pavel,
>>
>>> On Mon, 8 Nov 2021 at 10:03, Pavel Stehule wrote:
>>>
>>>
>>> po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule
>>> napsal:
po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule
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
> +
> + PG_SQL_TEXT
> + text
> + invalid sql statement, if any
> +
> +
> + PG_ERROR_LOCATION
> + text
> + invalid dynamic sql statement's text cursor position, if
> any
> +
>
> 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.
> I have not any other objections
>
> all tests passed without any problem
>
> Regards
>
> Pavel
>
>
>>
>>
>>> Regards
>>>
>>> Pavel
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi
po 8. 11. 2021 v 9:57 odesílatel Dinesh Chemuduru
napsal:
> Thanks Zhihong/Pavel,
>
> On Mon, 8 Nov 2021 at 10:03, Pavel Stehule
> wrote:
>
>>
>>
>> po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule
>> napsal:
>>
>>>
>>>
>>> po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule
>>> 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
+
+ PG_SQL_TEXT
+ text
+ invalid sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if
any
+
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
>
>
>> Regards
>>
>> Pavel
>>
>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Thanks Zhihong/Pavel,
On Mon, 8 Nov 2021 at 10:03, Pavel Stehule wrote:
>
>
> po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule
> napsal:
>
>>
>>
>> po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule
>> 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
>
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 variable { = | := } text
the name of the schema related to exception
+
+ PG_SQL_TEXT
+ text
+ invalid sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if any
+
PG_EXCEPTION_DETAIL
text
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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
po 8. 11. 2021 v 5:24 odesílatel Pavel Stehule
napsal:
>
>
> po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule
> 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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
po 8. 11. 2021 v 5:07 odesílatel Pavel Stehule 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 > > >> + 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 >> >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
> > > > +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 >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Sun, Nov 7, 2021 at 5:23 AM Dinesh Chemuduru wrote: > Hi Pavel, > > On Sun, 7 Nov 2021 at 12:53, Pavel Stehule > wrote: > >> Hi >> >> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru < >> dinesh.ku...@migops.com> napsal: >> >>> Hi Daniel, >>> >>> Thank you for your follow up, and attaching a new patch which addresses >>> Pavel's comments. >>> Let me know If I miss anything here. >>> >>> >>> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson wrote: >>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru 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 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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi Pavel,
On Sun, 7 Nov 2021 at 12:53, Pavel Stehule wrote:
> Hi
>
> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> Hi Daniel,
>>
>> Thank you for your follow up, and attaching a new patch which addresses
>> Pavel's comments.
>> Let me know If I miss anything here.
>>
>>
>> On Thu, 4 Nov 2021 at 17:40, Daniel Gustafsson wrote:
>>
>>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru
>>> 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 CASCADE"(current_query) will generate many
internal query statements for each of the dependent objects.
At this moment, we do save the current_query in PG_CONTEXT.
As we know, PG_CONTEXT returns the whole statements as stacktrace and it's
hard to fetch the required SQL from it.
I failed to see another way to access the current_query apart from the
PG_CONTEXT.
Hence, we have introduced the new member "current_query" to the "ErrorData"
object.
We tested the internalquery for this purpose, but there are few(10+ unit
test) test cases that failed.
This is also another reason we added this new member to the "ErrorData",
and with the current patch all test cases are successful,
and we are not disturbing the existing functionality.
> 2. The name set_current_err_query is not consistent with names in elog.c -
> probably something like errquery or set_errquery or set_errcurrent_query
> can be more consistent with other names.
>
> Updated as per your suggestion
Please find the new patch version.
> Regards
>
> Pavel
>
>
>
>>> --
>>> Daniel Gustafsson https://vmware.com/
>>>
>>>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index e5c1356d8c..f402c9f012 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2995,6 +2995,16 @@ GET STACKED DIAGNOSTICS variable { = | := } text
the name of the schema related to exception
+
+ PG_SQL_TEXT
+ text
+ invalid sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if any
+
PG_EXCEPTION_DETAIL
text
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 @@
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru 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 wrote: > >> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru >> 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/ >> >>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
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 wrote:
> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru
> 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/
>
>
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 variable { = | := } text
the name of the schema related to exception
+
+ PG_SQL_TEXT
+ text
+ invalid sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if any
+
PG_EXCEPTION_DETAIL
text
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_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; /*
Re: [PROPOSAL] new diagnostic items for the dynamic sql
> On 9 Sep 2021, at 08:23, Dinesh Chemuduru 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/
Re: [PROPOSAL] new diagnostic items for the dynamic sql
čt 9. 9. 2021 v 8:23 odesílatel Dinesh Chemuduru napsal: > > > On Thu, 9 Sept 2021 at 11:07, Pavel Stehule > 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 > > >> Second, but minor, objection to this patch is zero formatting in a >> regress test. >> >> Regards >> >> Pavel >> >>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Thu, 9 Sept 2021 at 11:07, Pavel Stehule 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 > >
Re: [PROPOSAL] new diagnostic items for the dynamic sql
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
Re: [PROPOSAL] new diagnostic items for the dynamic sql
út 24. 8. 2021 v 8:16 odesílatel Dinesh Chemuduru
napsal:
> Hi Pavel,
>
> On Tue, 24 Aug 2021 at 00:19, Pavel Stehule
> wrote:
>
>> Hi
>>
>> pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru <
>> dinesh.ku...@migops.com> napsal:
>>
>>> On Sun, 25 Jul 2021 at 16:34, Pavel Stehule
>>> 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
>
>> Regards
>>
>> Pavel
>>
>>>
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
dinesh.ku...@migops.com> napsal:
> On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
> wrote:
>
>> Hi
>>
>> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
>> dinesh.ku...@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
>>> ),
>>> 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
>>>
>>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi Pavel,
On Tue, 24 Aug 2021 at 00:19, Pavel Stehule wrote:
> Hi
>
> pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> On Sun, 25 Jul 2021 at 16:34, Pavel Stehule
>> 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.ku...@migops.com> napsal:
>>>
On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
wrote:
> Hi
>
> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
> dinesh.ku...@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
>> ),
>> 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
>>
>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi
pá 20. 8. 2021 v 10:24 odesílatel Dinesh Chemuduru
napsal:
> On Sun, 25 Jul 2021 at 16:34, Pavel Stehule
> 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.ku...@migops.com> napsal:
>>
>>> On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
>>> wrote:
>>>
Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
dinesh.ku...@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
> ),
> 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
>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Sun, 25 Jul 2021 at 16:34, Pavel Stehule wrote:
>
>
> ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru <
> dinesh.ku...@migops.com> napsal:
>
>> On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
>> wrote:
>>
>>> Hi
>>>
>>> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
>>> dinesh.ku...@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
),
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
>>>
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 variable { = | := } text
the name of the schema related to exception
+
+ PG_SQL_TEXT
+ text
+ invalid dynamic sql statement, if any
+
+
+ PG_ERROR_LOCATION
+ text
+ invalid dynamic sql statement's text cursor position, if any
+
PG_EXCEPTION_DETAIL
text
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);
+
Re: [PROPOSAL] new diagnostic items for the dynamic sql
ne 25. 7. 2021 v 12:52 odesílatel Dinesh Chemuduru
napsal:
> On Sat, 17 Jul 2021 at 01:29, Pavel Stehule
> wrote:
>
>> Hi
>>
>> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
>> dinesh.ku...@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 >> 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
>>>
>>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
On Sat, 17 Jul 2021 at 01:29, Pavel Stehule wrote:
> Hi
>
> pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru <
> dinesh.ku...@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 > 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
>>
>
Re: [PROPOSAL] new diagnostic items for the dynamic sql
Hi
pá 16. 7. 2021 v 21:47 odesílatel Dinesh Chemuduru
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 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
>
[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 ),
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
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 variable { = | := } text
the name of the schema related to exception
+
+ PG_PARSE_SQL_STATEMENT
+ text
+ the parse failed sql statement, if any
+
+
+ PG_PARSE_SQL_STATEMENT_POSITION
+ text
+ the parse failed sql statement's text cursor position, if any
+
PG_EXCEPTION_DETAIL
text
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 K_PG_CONTEXT
%token K_PG_DATATYPE_NAME
%token K_PG_EXCEPTION_CONTEXT
+%token K_PG_PARSE_SQL_STATEMENT
+%token K_PG_PARSE_SQL_STATEMENT_POSITION
%token K_PG_EXCEPTION_DETAIL
%token K_PG_EXCEPTION_HINT
%token K_PRINT_STRICT_PARAMS
@@ -998,6 +1000,8 @@ stmt_getdiag : K_GET getdiag_area_opt K_DIAGNOSTICS getdiag_list ';'
