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

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