SQLSTATE of notice PGresult

Hey all,

Accordingly to the documentation of libpq, SQLSTATE code field "is not
localizable, and
is always present.". But it seems, in some cases it isn't. E.g.

/* the main code */
PGresult* res = Pg::PQexec(conn, "select 1");
Oid id = PQparamtype(res, 1);

/* the notice receiver */
void myNoticeReceiver(void *arg, const PGresult *res)
{
/* Presents - "NOTICE" */
const char* severity = Pg::PQresultErrorField(res, PG_DIAG_SEVERITY);

/* NOT presents - NULL. Why not "00000" ? */
const char* sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);

/* Presents - "parameter number 1 is out of range 0..-1" */
const char* primary = Pg::PQresultErrorField(res,
PG_DIAG_MESSAGE_PRIMARY);
}

So, SQLSTATE field is not always presents.

Regards,
Dmitriy

Dmitriy Igrishin escreveu:
> /* NOT presents - NULL. Why not "00000" ? */
> const char* sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);
>
That's because the protocol doesn't set error field when the command
succeeded. IMHO it's an oversight (the documentation is correct but the code
is not) and should be correct because the spec enforces it.

--
Euler Taveira de Oliveira
http://www.timbira.com/

On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> Dmitriy Igrishin escreveu:
>>   /* NOT presents - NULL. Why not "00000" ? */
>>   const char* sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);
>>
> That's because the protocol doesn't set error field when the command
> succeeded. IMHO it's an oversight (the documentation is correct but the code
> is not) and should be correct because the spec enforces it.

Seems like a waste of bytes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas escreveu:
> On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira
> <euler(at)timbira(dot)com> wrote:
>> Dmitriy Igrishin escreveu:
>>> /* NOT presents - NULL. Why not "00000" ? */
>>> const char* sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);
>>>
>> That's because the protocol doesn't set error field when the command
>> succeeded. IMHO it's an oversight (the documentation is correct but the code
>> is not) and should be correct because the spec enforces it.
>
> Seems like a waste of bytes.
>
Ugh? It is a matter of correctness. I'm not arguing in favor of it but if we
don't implement it, it is better document it. I don't actually rely on sql
state to check errors but can have applications out there that expect the spec
behavior but we don't provide it and, also fail to document it. Talking about
the patch, it is just pqSaveMessageField() calls in *Complete messages. I can
provide a patch for it.

--
Euler Taveira de Oliveira
http://www.timbira.com/

On Tue, Aug 24, 2010 at 9:44 PM, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> Robert Haas escreveu:
>> On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira
>> <euler(at)timbira(dot)com> wrote:
>>> Dmitriy Igrishin escreveu:
>>>>   /* NOT presents - NULL. Why not "00000" ? */
>>>>   const char* sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);
>>>>
>>> That's because the protocol doesn't set error field when the command
>>> succeeded. IMHO it's an oversight (the documentation is correct but the code
>>> is not) and should be correct because the spec enforces it.
>>
>> Seems like a waste of bytes.
>>
> Ugh? It is a matter of correctness. I'm not arguing in favor of it but if we
> don't implement it, it is better document it.

<does a little more looking>

It appears to me that it already is documented. The very first
sentence of the documentation reads:

Returns an individual field of an error report.

And a few sentences later it says:

NULL is returned if the PGresult is not an error or warning result

> I don't actually rely on sql
> state to check errors but can have applications out there that expect the spec
> behavior but we don't provide it and, also fail to document it. Talking about
> the patch, it is just pqSaveMessageField() calls in *Complete messages. I can
> provide a patch for it.

I suppose we could change the function to return 00000 always when the
operation is not an error or warning report, rather than NULL, but
certainly we wouldn't want to include those bytes in *every* success
message, so they'd have to be something that the libpq inferred. And
I'm not clear why that behavior would be any more useful than what we
have now; indeed, it seems like it would needlessly break backward
compatibility. If you're arguing that this behavior is required by
the spec, let's have a cite. I find it a bit surprising that the spec
would cover the behavior of individual libpq functions in this level
of detail.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I suppose we could change the function to return 00000 always when the
> operation is not an error or warning report, rather than NULL, but
> certainly we wouldn't want to include those bytes in *every* success
> message, so they'd have to be something that the libpq inferred. And
> I'm not clear why that behavior would be any more useful than what we
> have now; indeed, it seems like it would needlessly break backward
> compatibility.

Um. You're missing the point here. This isn't a message from the
backend, it's a complaint generated internally by libpq. The real issue
here is that there are no SQLSTATEs assigned for any error/warning
conditions generated internally in libpq. Fixing this is just a Small
Matter Of Programming, but no one's yet taken an interest in doing it.
Seeing that that's been a TODO item since 7.4, I wouldn't advise holding
your breath.

As far as this particular example goes, I think it's highly debatable
whether "out of range parameter number" should be only a NOTICE, and
almost certainly wrong to say that it ought to be associated with an
00000 SQLSTATE. But figuring out what it ought to be is part of the
dogwork that nobody's done yet.

> If you're arguing that this behavior is required by
> the spec, let's have a cite. I find it a bit surprising that the spec
> would cover the behavior of individual libpq functions in this level
> of detail.

I believe the text about "always present" is cribbed from our FE/BE
protocol specification. It is true (or at least should be true) for
error and notice messages sent from the backend.

regards, tom lane

Robert Haas escreveu:
> It appears to me that it already is documented. The very first
> sentence of the documentation reads:
>
> Returns an individual field of an error report.
>
> And a few sentences later it says:
>
> NULL is returned if the PGresult is not an error or warning result
>
I'm referring to [1].

> I suppose we could change the function to return 00000 always when the
> operation is not an error or warning report, rather than NULL, but
> certainly we wouldn't want to include those bytes in *every* success
> message, so they'd have to be something that the libpq inferred. And
> I'm not clear why that behavior would be any more useful than what we
> have now; indeed, it seems like it would needlessly break backward
> compatibility. If you're arguing that this behavior is required by
> the spec, let's have a cite. I find it a bit surprising that the spec
> would cover the behavior of individual libpq functions in this level
> of detail.
>
It seems we can't infer the success message from libpq; it is necessary to
build the sql state message field. As I said both behaviors have the same goal
(in this case, NULL means success, i.e. sqlstate is not assigned) but it
doesn't match the spec.

[1] http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html

--
Euler Taveira de Oliveira
http://www.timbira.com/

Tom Lane escreveu:
> The real issue
> here is that there are no SQLSTATEs assigned for any error/warning
> conditions generated internally in libpq.
>
Did you mean successful conditions? Only warning/error conditions produce a
SQLSTATE.

> As far as this particular example goes, I think it's highly debatable
> whether "out of range parameter number" should be only a NOTICE, and
> almost certainly wrong to say that it ought to be associated with an
> 00000 SQLSTATE. But figuring out what it ought to be is part of the
> dogwork that nobody's done yet.
>
It should match the actual PostgreSQL behavior. There are two classes (01xxx
and 02xxx) for warnings.

What I'm thinking is something like

*** src/interfaces/libpq/fe-protocol3.c 28 Apr 2010 13:46:23 -0000 1.43
--- src/interfaces/libpq/fe-protocol3.c 21 Aug 2010 02:41:01 -0000
***************
*** 206,211 ****
--- 206,219 ----
if (!conn->result)
return;
}
+ /*
+ * If the command was successful completed, set the
+ * appropriate SQLSTATE. Pre-9.1 don't set it.
+ * ERRCODE_SUCCESSFUL_COMPLETION code (aka 00000) is
+ * hardcoded here because we avoid including elog routines
+ * here.
+ */
+ pqSaveMessageField(conn->result, PG_DIAG_SQLSTATE, "00000");
strncpy(conn->result->cmdStatus, conn->workBuffer.data,
CMDSTATUS_LEN);
conn->asyncStatus = PGASYNC_READY;

(I only patch the 'Command Complete' message here but it is necessary to patch
other success messages too.)

--
Euler Taveira de Oliveira
http://www.timbira.com/

Euler Taveira de Oliveira <euler(at)timbira(dot)com> writes:
> What I'm thinking is something like

You didn't actually read what I said, did you? That patch will have
precisely zero effect on the OP's example.

regards, tom lane

Tom Lane escreveu:
> You didn't actually read what I said, did you? That patch will have
> precisely zero effect on the OP's example.
>
Oh, I see your point. Didn't pay attention at the OP's example. I was only
worried about the successful queries that doesn't return SQLSTATE but as you
point out, that part of the code deserves a refactoring to cover OP's case too.

--
Euler Taveira de Oliveira
http://www.timbira.com/

Hey all,

Okay, as Robert points, "00000" code in successful messages seems as waste
of bytes. But according to the documentation, "All messages emitted by the
PostgreSQL server are assigned five-character error codes that follow the
SQL
standard's conventions for "SQLSTATE" codes." - the first sentence of
http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html

--
Regards,
Dmitriy

On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin <dmitigr(at)gmail(dot)com> wrote:
> Okay, as Robert points, "00000" code in successful messages seems as waste
> of bytes. But according to the documentation, "All messages emitted by the
> PostgreSQL server are assigned five-character error codes that follow the
> SQL
> standard's conventions for "SQLSTATE" codes." - the first sentence of
> http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html

Sounds like that wording needs some adjustment. I'm not even sure
that it would be correct to say "All error messages...", unless
elog(ERROR, "can't happen") throws something into that field.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I'm not even sure that it would be correct to say "All error
> messages...", unless elog(ERROR, "can't happen") throws something
> into that field.

If it doesn't, it should. Probably 'XX000' (internal_error).

-Kevin

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin <dmitigr(at)gmail(dot)com> wrote:
>> Okay, as Robert points, "00000" code in successful messages seems as waste
>> of bytes. But according to the documentation, "All messages emitted by the
>> PostgreSQL server are assigned five-character error codes that follow the
>> SQL
>> standard's conventions for "SQLSTATE" codes." - the first sentence of
>> http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html

> Sounds like that wording needs some adjustment.

That wording is correct as it stands.

If I recall the previous discussion here, the problem is that the OP
is reading that and thinking that it applies also to errors generated
internally by libpq. We should, but don't, have any support for
assigning SQLSTATEs to those. But the server always emits a SQLSTATE
when sending a notice or error message --- read
send_message_to_frontend() if you doubt it.

regards, tom lane