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