Re: [PROPOSAL] new diagnostic items for the dynamic sql

On Sun, Nov 7, 2021 at 5:23 AM Dinesh Chemuduru <dinesh(dot)kumar(at)migops(dot)com>
wrote:

> Hi Pavel,
>
> On Sun, 7 Nov 2021 at 12:53, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>> Hi
>>
>> pá 5. 11. 2021 v 19:27 odesílatel Dinesh Chemuduru <
>> dinesh(dot)kumar(at)migops(dot)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(at)yesql(dot)se> wrote:
>>>
>>>> > On 9 Sep 2021, at 08:23, Dinesh Chemuduru <dinesh(dot)kumar(at)migops(dot)com>
>>>> wrote:
>>>>
>>>> > Let me try to fix the suggested case(I tried to fix this case in the
>>>> past, but this time I will try to spend more time on this), and will submit
>>>> a new patch.
>>>>
>>>> This CF entry is marked Waiting on Author, have you had the chance to
>>>> prepare a
>>>> new version of the patch addressing the comments from Pavel?
>>>>
>>>
>> I think the functionality is correct. I am not sure about implementation
>>
>>
> Thank you for your time in validating this patch.
>
>
>> 1. Is it necessary to introduce a new field "current_query"? Cannot be
>> used "internalquery" instead? Introducing a new field opens some questions
>> - what is difference between internalquery and current_query, and where and
>> when have to be used first and when second? ErrorData is a fundamental
>> generic structure for Postgres, and can be confusing to enhance it by one
>> field just for one purpose, that is not used across Postgres.
>>
>> Internalquery is the one, which was generated by another command.
> For example, "DROP <OBJECT> CASCADE"(current_query) will generate many
> internal query statements for each of the dependent objects.
>
> At this moment, we do save the current_query in PG_CONTEXT.
> As we know, PG_CONTEXT returns the whole statements as stacktrace and it's
> hard to fetch the required SQL from it.
>
> I failed to see another way to access the current_query apart from the
> PG_CONTEXT.
> Hence, we have introduced the new member "current_query" to the
> "ErrorData" object.
>
> We tested the internalquery for this purpose, but there are few(10+ unit
> test) test cases that failed.
> This is also another reason we added this new member to the "ErrorData",
> and with the current patch all test cases are successful,
> and we are not disturbing the existing functionality.
>
>
>> 2. The name set_current_err_query is not consistent with names in elog.c
>> - probably something like errquery or set_errquery or set_errcurrent_query
>> can be more consistent with other names.
>>
>> Updated as per your suggestion
>
> Please find the new patch version.
>
>
>> Regards
>>
>> Pavel
>>
>>
>>
>>>> --
>>>> Daniel Gustafsson https://vmware.com/
>>>>
>>>> Hi,

+set_errcurrent_query (const char *query)

You can remove the space prior to (.
I wonder if the new field can be named current_err_query because that's
what the setter implies.
current_query may give the impression that the field can store normal query
(which doesn't cause exception).
The following code implies that only one of internalquery and current_query
would be set.

+ if (estate->cur_error->internalquery)
+ exec_assign_c_string(estate, var,
estate->cur_error->internalquery);
+ else
+ exec_assign_c_string(estate, var,
estate->cur_error->current_query);

Cheers