Re: plperl error format vs plpgsql error format vs pgTAP

I use pgTAP to make sure my functions produce the correct errors using
throws_ok(). So when I get an error from a plpgsql function, it looks
like this:

ERROR: upper bound of FOR loop cannot be null
CONTEXT: PL/pgSQL function "foo" line 35 at FOR with integer loop
variable

...which I can then test using throws_ok by giving it the string
'upper bound of FOR loop cannot be null'. However, in a plperl
function, errors come out in this format:

error from Perl function "check_no_loop": Loops not allowed! Node 1
cannot be part of node 3 at line 13.

Unfortunately, I can't test for this without including the line
number, which means that changing any plperl function that I have such
a test for pretty much guarantees that I'll need to change the test to
reflect the new line numbers the errors would be thrown from in the
function.

Is it possible to unify the error reporting format, so pgTAP can test
them without needing line numbers from plperl functions?

Kevin Field <kevinjamesfield(at)gmail(dot)com> writes:
> I use pgTAP to make sure my functions produce the correct errors using
> throws_ok(). So when I get an error from a plpgsql function, it looks
> like this:

> ERROR: upper bound of FOR loop cannot be null
> CONTEXT: PL/pgSQL function "foo" line 35 at FOR with integer loop
> variable

> ...which I can then test using throws_ok by giving it the string
> 'upper bound of FOR loop cannot be null'.

Surely, this is a completely brain-dead approach to testing errors
in the first place ... what will happen in a localized installation?

What you need is a test that looks at the SQLSTATE code, and little
if anything else.

regards, tom lane

Kevin Field wrote:
> I use pgTAP to make sure my functions produce the correct errors using
> throws_ok(). So when I get an error from a plpgsql function, it looks
> like this:
>
> ERROR: upper bound of FOR loop cannot be null
> CONTEXT: PL/pgSQL function "foo" line 35 at FOR with integer loop
> variable
>
> ...which I can then test using throws_ok by giving it the string
> 'upper bound of FOR loop cannot be null'. However, in a plperl
> function, errors come out in this format:
>
> error from Perl function "check_no_loop": Loops not allowed! Node 1
> cannot be part of node 3 at line 13.
>
> Unfortunately, I can't test for this without including the line
> number, which means that changing any plperl function that I have such
> a test for pretty much guarantees that I'll need to change the test to
> reflect the new line numbers the errors would be thrown from in the
> function.
>
> Is it possible to unify the error reporting format, so pgTAP can test
> them without needing line numbers from plperl functions?
>
>

This is under perl's control, not ours. The perl docco says:

If the last element of LIST does not end in a newline, the current
script line number and input line number (if any) are also printed
and a newline is supplied.

Can pgTap check for a regex instead if just a string?

cheers

andrew

On May 28, 3:22 pm, t(dot)(dot)(dot)(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) wrote:
> Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> writes:
> > I use pgTAP to make sure my functions produce the correct errors using
> > throws_ok(). So when I get an error from a plpgsql function, it looks
> > like this:
> > ERROR: upper bound of FOR loop cannot be null
> > CONTEXT: PL/pgSQL function "foo" line 35 at FOR with integer loop
> > variable
> > ...which I can then test using throws_ok by giving it the string
> > 'upper bound of FOR loop cannot be null'.
>
> Surely, this is a completely brain-dead approach to testing errors
> in the first place ... what will happen in a localized installation?
>
> What you need is a test that looks at the SQLSTATE code, and little
> if anything else.

There won't be any localized installations.

I wanted to use the SQLSTATE code, but it's always XX000. If there
were some way to set it when calling elog() so I knew the right error
was being reached, that would be a great option. Is that something
under the control of PostgreSQL?

On May 28, 3:28 pm, and(dot)(dot)(dot)(at)dunslane(dot)net (Andrew Dunstan) wrote:
> Kevin Field wrote:
> > I use pgTAP to make sure my functions produce the correct errors using
> > throws_ok(). So when I get an error from a plpgsql function, it looks
> > like this:
>
> > ERROR: upper bound of FOR loop cannot be null
> > CONTEXT: PL/pgSQL function "foo" line 35 at FOR with integer loop
> > variable
>
> > ...which I can then test using throws_ok by giving it the string
> > 'upper bound of FOR loop cannot be null'. However, in a plperl
> > function, errors come out in this format:
>
> > error from Perl function "check_no_loop": Loops not allowed! Node 1
> > cannot be part of node 3 at line 13.
>
> > Unfortunately, I can't test for this without including the line
> > number, which means that changing any plperl function that I have such
> > a test for pretty much guarantees that I'll need to change the test to
> > reflect the new line numbers the errors would be thrown from in the
> > function.
>
> > Is it possible to unify the error reporting format, so pgTAP can test
> > them without needing line numbers from plperl functions?
>
> This is under perl's control, not ours. The perl docco says:
>
> If the last element of LIST does not end in a newline, the current
> script line number and input line number (if any) are also printed
> and a newline is supplied.
>
> Can pgTap check for a regex instead if just a string?

That's the other option, if the pgTAP author is willing...if the
SQLSTATE thing doesn't work out I guess we'll have to go down that
road.

Kevin Field <kevinjamesfield(at)gmail(dot)com> writes:
> I wanted to use the SQLSTATE code, but it's always XX000. If there
> were some way to set it when calling elog()

ereport?

http://developer.postgresql.org/pgdocs/postgres/error-message-reporting.html

At the plpgsql level, since 8.4 you can specify a SQLSTATE in RAISE.
AFAIR nobody's gotten round to doing anything about it in plperl.

regards, tom lane

On May 28, 2009, at 12:22 PM, Tom Lane wrote:

> What you need is a test that looks at the SQLSTATE code, and little
> if anything else.

Yes, and throws_ok() supports that:

SELECT throws_ok( 'SELECT 1 / 0', '22012' );

Best,

David

On May 28, 2009, at 12:53 PM, Kevin Field wrote:

>> Can pgTap check for a regex instead if just a string?
>
> That's the other option, if the pgTAP author is willing...if the
> SQLSTATE thing doesn't work out I guess we'll have to go down that
> road.

Patches welcome. ;-)

http://github.com/theory/pgtap/tree/master/

I'm getting a new version ready to release as I type.

Best,

David

On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>
> >> Can pgTap check for a regex instead if just a string?
>
> > That's the other option, if the pgTAP author is willing...if the
> > SQLSTATE thing doesn't work out I guess we'll have to go down that
> > road.
>
> Patches welcome. ;-)
>
> http://github.com/theory/pgtap/tree/master/
>
> I'm getting a new version ready to release as I type.

Thanks, great to know. :) Although, I do think changing plperl is
the more proper option, so I'm going to try there first...

On Fri, May 29, 2009 at 7:59 AM, Kevin Field <kevinjamesfield(at)gmail(dot)com> wrote:
> On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
>> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>>
>> >> Can pgTap check for a regex instead if just a string?
>>
>> > That's the other option, if the pgTAP author is willing...if the
>> > SQLSTATE thing doesn't work out I guess we'll have to go down that
>> > road.
>>
>> Patches welcome. ;-)
>>
>>    http://github.com/theory/pgtap/tree/master/
>>
>> I'm getting a new version ready to release as I type.
>
> Thanks, great to know.  :)  Although, I do think changing plperl is
> the more proper option, so I'm going to try there first...

It seems to me that removing line numbers from PL/perl error messages
is not a good solution to anything. Line numbers are extremely useful
for debugging purposes, and getting rid of them because one particular
testing framework doesn't know how to use regular expressions is
solving the wrong problem.

I'm also a bit confused because your original post had a line number
in the PL/pgsql output, too, just formatted slightly differently. Why
doesn't that one cause a problem?

...Robert

Kevin Field wrote:
> On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
>
>> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>>
>>
>>>> Can pgTap check for a regex instead if just a string?
>>>>
>>> That's the other option, if the pgTAP author is willing...if the
>>> SQLSTATE thing doesn't work out I guess we'll have to go down that
>>> road.
>>>
>> Patches welcome. ;-)
>>
>> http://github.com/theory/pgtap/tree/master/
>>
>> I'm getting a new version ready to release as I type.
>>
>
> Thanks, great to know. :) Although, I do think changing plperl is
> the more proper option, so I'm going to try there first...
>
>

As I pointed out before, these line numbers are put there by the perl
engine, not by the plperl glue code.

If you want to make plperl strip out the line number from every error
message the perl engine produces, I am going to object. It might make
things easier for pgTap but it will make life much harder in other ways.

cheers

andrew

On May 29, 11:35 am, robertmh(dot)(dot)(dot)(at)gmail(dot)com (Robert Haas) wrote:
> On Fri, May 29, 2009 at 7:59 AM, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> > On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
> >> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>
> >> >> Can pgTap check for a regex instead if just a string?
>
> >> > That's the other option, if the pgTAP author is willing...if the
> >> > SQLSTATE thing doesn't work out I guess we'll have to go down that
> >> > road.
>
> >> Patches welcome. ;-)
>
> >> http://github.com/theory/pgtap/tree/master/
>
> >> I'm getting a new version ready to release as I type.
>
> > Thanks, great to know. :) Although, I do think changing plperl is
> > the more proper option, so I'm going to try there first...
>
> It seems to me that removing line numbers from PL/perl error messages
> is not a good solution to anything. Line numbers are extremely useful
> for debugging purposes, and getting rid of them because one particular
> testing framework doesn't know how to use regular expressions is
> solving the wrong problem.

You're right, but that's not what I'm proposing...

> I'm also a bit confused because your original post had a line number
> in the PL/pgsql output, too, just formatted slightly differently. Why
> doesn't that one cause a problem?

The difference is, in PL/pgsql they're in the CONTEXT: line, whereas
in plperl they're in the error line. This is inconsistent; if we fix
it, we don't need to add kludge to pgTAP.

But later in the thread the desired fix became not changing perl but
instead making a way to report error codes from plperl, which is what
I'm attempting to do with my rusty C skills soon. plperl should have
ereport() *anyway*, as I believe Tom had insinuated.

On May 29, 11:48 am, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> On May 29, 11:35 am, robertmh(dot)(dot)(dot)(at)gmail(dot)com (Robert Haas) wrote:
>
>
>
> > On Fri, May 29, 2009 at 7:59 AM, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> > > On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
> > >> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>
> > >> >> Can pgTap check for a regex instead if just a string?
>
> > >> > That's the other option, if the pgTAP author is willing...if the
> > >> > SQLSTATE thing doesn't work out I guess we'll have to go down that
> > >> > road.
>
> > >> Patches welcome. ;-)
>
> > >> http://github.com/theory/pgtap/tree/master/
>
> > >> I'm getting a new version ready to release as I type.
>
> > > Thanks, great to know. :) Although, I do think changing plperl is
> > > the more proper option, so I'm going to try there first...
>
> > It seems to me that removing line numbers from PL/perl error messages
> > is not a good solution to anything. Line numbers are extremely useful
> > for debugging purposes, and getting rid of them because one particular
> > testing framework doesn't know how to use regular expressions is
> > solving the wrong problem.
>
> You're right, but that's not what I'm proposing...
>
> > I'm also a bit confused because your original post had a line number
> > in the PL/pgsql output, too, just formatted slightly differently. Why
> > doesn't that one cause a problem?
>
> The difference is, in PL/pgsql they're in the CONTEXT: line, whereas
> in plperl they're in the error line. This is inconsistent; if we fix
> it, we don't need to add kludge to pgTAP.
>
> But later in the thread the desired fix became not changing perl but
> instead making a way to report error codes from plperl, which is what
> I'm attempting to do with my rusty C skills soon. plperl should have
> ereport() *anyway*, as I believe Tom had insinuated.

BTW, I noticed in exec_stmt_raise() in src/pl/plpgsql/src/pl_exec.c
that the comment still says "throw it with elog()" rather than "ereport
()" even though ereport() is used in all places but one in the
function:

default:
elog(ERROR, "unrecognized raise option: %d", opt->opt_type);

Should this be changed to:

default:
ereport(ERROR, (errmsg_internal("unrecognized raise option: %d",
opt->opt_type)));

...along with the comment?

On May 29, 2009, at 4:59 AM, Kevin Field wrote:

>> http://github.com/theory/pgtap/tree/master/
>>
>> I'm getting a new version ready to release as I type.
>
> Thanks, great to know. :) Although, I do think changing plperl is
> the more proper option, so I'm going to try there first...

I added `throws_like()` to the To Do list, so if anyone wants to do
that…fork and clone!

Best,

David

On May 29, 11:48 am, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> On May 29, 11:35 am, robertmh(dot)(dot)(dot)(at)gmail(dot)com (Robert Haas) wrote:
>
>
>
> > On Fri, May 29, 2009 at 7:59 AM, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> > > On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
> > >> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>
> > >> >> Can pgTap check for a regex instead if just a string?
>
> > >> > That's the other option, if the pgTAP author is willing...if the
> > >> > SQLSTATE thing doesn't work out I guess we'll have to go down that
> > >> > road.
>
> > >> Patches welcome. ;-)
>
> > >> http://github.com/theory/pgtap/tree/master/
>
> > >> I'm getting a new version ready to release as I type.
>
> > > Thanks, great to know. :) Although, I do think changing plperl is
> > > the more proper option, so I'm going to try there first...
>
> > It seems to me that removing line numbers from PL/perl error messages
> > is not a good solution to anything. Line numbers are extremely useful
> > for debugging purposes, and getting rid of them because one particular
> > testing framework doesn't know how to use regular expressions is
> > solving the wrong problem.
>
> You're right, but that's not what I'm proposing...
>
> > I'm also a bit confused because your original post had a line number
> > in the PL/pgsql output, too, just formatted slightly differently. Why
> > doesn't that one cause a problem?
>
> The difference is, in PL/pgsql they're in the CONTEXT: line, whereas
> in plperl they're in the error line. This is inconsistent; if we fix
> it, we don't need to add kludge to pgTAP.
>
> But later in the thread the desired fix became not changing perl but
> instead making a way to report error codes from plperl, which is what
> I'm attempting to do with my rusty C skills soon. plperl should have
> ereport() *anyway*, as I believe Tom had insinuated.

Hmm, I'm rustier than I thought. I might need some help with this
later.

Kevin Field <kevinjamesfield(at)gmail(dot)com> writes:
> default:
> elog(ERROR, "unrecognized raise option: %d", opt->opt_type);

> Should this be changed to:

> default:
> ereport(ERROR, (errmsg_internal("unrecognized raise option: %d",
> opt->opt_type)));

No, we generally don't bother with that. The above two are exactly
equivalent and the first is easier to write, so why complicate the code?
ereport is needed if you want to specify a SQLSTATE, provide a
translatable error message, etc, but for internal shouldn't-happen cases
we customarily just use elog.

regards, tom lane

On May 29, 1:04 pm, t(dot)(dot)(dot)(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) wrote:
> Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> writes:
> > default:
> > elog(ERROR, "unrecognized raise option: %d", opt->opt_type);
> > Should this be changed to:
> > default:
> > ereport(ERROR, (errmsg_internal("unrecognized raise option: %d",
> > opt->opt_type)));
>
> No, we generally don't bother with that. The above two are exactly
> equivalent and the first is easier to write, so why complicate the code?
> ereport is needed if you want to specify a SQLSTATE, provide a
> translatable error message, etc, but for internal shouldn't-happen cases
> we customarily just use elog.

Ah, I had missed that. I understand. The function's comment's still
out of date though, I think, since it uses ereport at the end.

On May 29, 12:43 pm, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> On May 29, 11:48 am, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
>
>
>
> > On May 29, 11:35 am, robertmh(dot)(dot)(dot)(at)gmail(dot)com (Robert Haas) wrote:
>
> > > On Fri, May 29, 2009 at 7:59 AM, Kevin Field <kevinjamesfi(dot)(dot)(dot)(at)gmail(dot)com> wrote:
> > > > On May 28, 5:19 pm, da(dot)(dot)(dot)(at)kineticode(dot)com ("David E. Wheeler") wrote:
> > > >> On May 28, 2009, at 12:53 PM, Kevin Field wrote:
>
> > > >> >> Can pgTap check for a regex instead if just a string?
>
> > > >> > That's the other option, if the pgTAP author is willing...if the
> > > >> > SQLSTATE thing doesn't work out I guess we'll have to go down that
> > > >> > road.
>
> > > >> Patches welcome. ;-)
>
> > > >> http://github.com/theory/pgtap/tree/master/
>
> > > >> I'm getting a new version ready to release as I type.
>
> > > > Thanks, great to know. :) Although, I do think changing plperl is
> > > > the more proper option, so I'm going to try there first...
>
> > > It seems to me that removing line numbers from PL/perl error messages
> > > is not a good solution to anything. Line numbers are extremely useful
> > > for debugging purposes, and getting rid of them because one particular
> > > testing framework doesn't know how to use regular expressions is
> > > solving the wrong problem.
>
> > You're right, but that's not what I'm proposing...
>
> > > I'm also a bit confused because your original post had a line number
> > > in the PL/pgsql output, too, just formatted slightly differently. Why
> > > doesn't that one cause a problem?
>
> > The difference is, in PL/pgsql they're in the CONTEXT: line, whereas
> > in plperl they're in the error line. This is inconsistent; if we fix
> > it, we don't need to add kludge to pgTAP.
>
> > But later in the thread the desired fix became not changing perl but
> > instead making a way to report error codes from plperl, which is what
> > I'm attempting to do with my rusty C skills soon. plperl should have
> > ereport() *anyway*, as I believe Tom had insinuated.
>
> Hmm, I'm rustier than I thought. I might need some help with this
> later.

Actually, I'm not sure I'll be able to be of any use on this after
all. Would someone be able to add plperl ereport to the todo list for
me at least?