Обсуждение: verbose mode for pg_input_error_message?

I've been wondering if it might be a good idea to have a third parameter
for pg_input_error_message() which would default to false, but which if
true would cause it to emit the detail and hint fields, if any,  as well
as the message field from the error_data.

Thoughts?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> I've been wondering if it might be a good idea to have a third parameter
> for pg_input_error_message() which would default to false, but which if
> true would cause it to emit the detail and hint fields, if any, as well
> as the message field from the error_data.

I don't think that just concatenating those strings would make for a
pleasant API.  More sensible, perhaps, to have a separate function
that returns a record.  Or we could redefine the existing function
that way, but I suspect that "just the primary error" will be a
principal use-case.

Being able to get the SQLSTATE is likely to be interesting too.

            regards, tom lane

On 2023-01-02 Mo 10:44, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> I've been wondering if it might be a good idea to have a third parameter
>> for pg_input_error_message() which would default to false, but which if
>> true would cause it to emit the detail and hint fields, if any, as well
>> as the message field from the error_data.
> I don't think that just concatenating those strings would make for a
> pleasant API.  More sensible, perhaps, to have a separate function
> that returns a record.  Or we could redefine the existing function
> that way, but I suspect that "just the primary error" will be a
> principal use-case.
>
> Being able to get the SQLSTATE is likely to be interesting too.
>
>             


OK, here's a patch along those lines.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Jan 04, 2023 at 04:18:59PM -0500, Andrew Dunstan wrote:
> On 2023-01-02 Mo 10:44, Tom Lane wrote:
>> I don't think that just concatenating those strings would make for a
>> pleasant API.  More sensible, perhaps, to have a separate function
>> that returns a record.  Or we could redefine the existing function
>> that way, but I suspect that "just the primary error" will be a
>> principal use-case.
>>
>> Being able to get the SQLSTATE is likely to be interesting too.
> 
> OK, here's a patch along those lines.

My vote would be to redefine the existing pg_input_error_message() function
to return a record, but I recognize that this would inflate the patch quite
a bit due to all the existing uses in the tests.  If this is the only
argument against this approach, I'm happy to help with the patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 10, 2023 at 03:41:12PM -0800, Nathan Bossart wrote:
> My vote would be to redefine the existing pg_input_error_message() function
> to return a record, but I recognize that this would inflate the patch quite
> a bit due to all the existing uses in the tests.  If this is the only
> argument against this approach, I'm happy to help with the patch.

Here's an attempt at this.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Feb 23, 2023 at 10:40:48AM -0800, Nathan Bossart wrote:
> On Tue, Jan 10, 2023 at 03:41:12PM -0800, Nathan Bossart wrote:
>> My vote would be to redefine the existing pg_input_error_message() function
>> to return a record, but I recognize that this would inflate the patch quite
>> a bit due to all the existing uses in the tests.  If this is the only
>> argument against this approach, I'm happy to help with the patch.
> 
> Here's an attempt at this.

This seems to have made cfbot angry.  Will post a new version shortly.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Feb 23, 2023 at 11:30:38AM -0800, Nathan Bossart wrote:
> Will post a new version shortly.

As promised...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Feb 24, 2023 at 05:36:42PM -0500, Corey Huinker wrote:
> Looks good to me, passes make check-world. Thanks for slogging through this.

FWIW, I agree that switching pg_input_error_message() to return a row
would be nicer in the long-run than just getting an error message
because it has the merit to be extensible at will with all the data
we'd like to attach to it (I suspect that getting more fields is not
much likely, but who knows..).

pg_input_error_message() does not strike me as a good function name,
though, because it now returns much more than an error message.
Hence, couldn't something like pg_input_error() be better, because
more generic?
--
Michael

On Sat, Feb 25, 2023 at 01:39:21PM +0900, Michael Paquier wrote:
> pg_input_error_message() does not strike me as a good function name,
> though, because it now returns much more than an error message.
> Hence, couldn't something like pg_input_error() be better, because
> more generic?

I personally think the existing name is fine.  It returns the error
message, which includes the primary, detail, and hint messages.  Also, I'm
not sure that pg_input_error() is descriptive enough.  That being said, I'm
happy to run the sed command to change the name to whatever folks think is
best.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sat, Feb 25, 2023 at 01:39:21PM +0900, Michael Paquier wrote:
>> pg_input_error_message() does not strike me as a good function name,
>> though, because it now returns much more than an error message.
>> Hence, couldn't something like pg_input_error() be better, because
>> more generic?

> I personally think the existing name is fine.  It returns the error
> message, which includes the primary, detail, and hint messages.  Also, I'm
> not sure that pg_input_error() is descriptive enough.  That being said, I'm
> happy to run the sed command to change the name to whatever folks think is
> best.

Maybe pg_input_error_info()?  I tend to agree with Michael that as
soon as you throw things like the SQLSTATE code into it, "message"
seems not very apropos.  I'm not dead set on that position, though.

            regards, tom lane

On Sat, Feb 25, 2023 at 08:07:33PM -0500, Tom Lane wrote:
> Maybe pg_input_error_info()?  I tend to agree with Michael that as
> soon as you throw things like the SQLSTATE code into it, "message"
> seems not very apropos.  I'm not dead set on that position, though.

pg_input_error_info() seems more descriptive to me.  I changed the name to
that in v4.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sat, Feb 25, 2023 at 08:58:17PM -0800, Nathan Bossart wrote:
> pg_input_error_info() seems more descriptive to me.  I changed the name to
> that in v4.

error_info() is fine by me.  My recent history is poor lately when it
comes to name new things.

+       values[0] = CStringGetTextDatum(escontext.error_data->message);
+
+       if (escontext.error_data->detail != NULL)
+           values[1] = CStringGetTextDatum(escontext.error_data->detail);
+       else
+           isnull[1] = true;
+
+       if (escontext.error_data->hint != NULL)
+           values[2] = CStringGetTextDatum(escontext.error_data->hint);
+       else
+           isnull[2] = true;
+
+       values[3] = CStringGetTextDatum(
+           unpack_sql_state(escontext.error_data->sqlerrcode));

I am OK with this data set as well.  If somebody makes a case about
more fields in ErrorData, we could always consider these separately.

FWIW, I would like to change some of the regression tests as we are
bikeshedding the whole.

+SELECT pg_input_error_info(repeat('too_long', 32), 'rainbow');
For example, we could use the expanded display for this case in
enum.sql.

 -- test non-error-throwing API
 SELECT str as jsonpath,
        pg_input_is_valid(str,'jsonpath') as ok,
-       pg_input_error_message(str,'jsonpath') as errmsg
+       pg_input_error_info(str,'jsonpath') as errmsg
This case in jsonpath.sql is actually wrong, because we have more than
just the error message.

For the others, I would make the choice of expanding the calls of
pg_input_error_info() rather than just showing row outputs, though I
agree that this part is minor.
--
Michael

On Sun, Feb 26, 2023 at 02:35:22PM +0900, Michael Paquier wrote:
> For the others, I would make the choice of expanding the calls of
> pg_input_error_info() rather than just showing row outputs, though I
> agree that this part is minor.

While bike-shedding all the regression tests, I have noticed that
float4-misrounded-input.out was missing a refresh (the query was
right, not the output).  The rest was pretty much OK for me, still I
found all the errmsg aliases a bit out of context as the function is
now extended with more attributes, so I have painted a couple of
LATERALs over that.

Are you OK with the attached?
--
Michael

On Mon, Feb 27, 2023 at 03:37:37PM +0900, Michael Paquier wrote:
> Are you OK with the attached?

I found a couple of more small changes required to make cfbot happy.
Otherwise, it looks good to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Feb 27, 2023 at 11:25:01AM -0800, Nathan Bossart wrote:
> I found a couple of more small changes required to make cfbot happy.
> Otherwise, it looks good to me.

Thanks, I have confirmed the spots the CI was complaining about, so
applied.  There was an extra place that was not right in xml_2.out as
reported by prion, parula and snakefly because of a bad copy-paste, so
fixed as well.
--
Michael

On Tue, Feb 28, 2023 at 09:01:48AM +0900, Michael Paquier wrote:
> On Mon, Feb 27, 2023 at 11:25:01AM -0800, Nathan Bossart wrote:
>> I found a couple of more small changes required to make cfbot happy.
>> Otherwise, it looks good to me.
> 
> Thanks, I have confirmed the spots the CI was complaining about, so
> applied.  There was an extra place that was not right in xml_2.out as
> reported by prion, parula and snakefly because of a bad copy-paste, so
> fixed as well.

Thanks!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com