Thread: 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
Attachment
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
Attachment
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
Attachment
On Thu, Feb 23, 2023 at 4:47 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
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
Looks good to me, passes make check-world. Thanks for slogging through this.
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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