Thread: SQLSTATE of notice PGresult
Hey all,<br /><br />Accordingly to the documentation of libpq, SQLSTATE code field "is not localizable, and<br />is
alwayspresent.". But it seems, in some cases it isn't. E.g.<br /><br /> /* the main code */<br /> PGresult* res =
Pg::PQexec(conn,"select 1");<br /> Oid id = PQparamtype(res, 1);<br /><br /> /* the notice receiver */<br /> void
myNoticeReceiver(void*arg, const PGresult *res)<br />{<br /> /* Presents - "NOTICE" */<br /> const char* severity =
Pg::PQresultErrorField(res,PG_DIAG_SEVERITY);<br /><br /> /* NOT presents - NULL. Why not "00000" ? */<br /> const
char*sqlstate = Pg::PQresultErrorField(res, PG_DIAG_SQLSTATE);<br /><br /> /* Presents - "parameter number 1 is out of
range0..-1" */<br /> const char* primary = Pg::PQresultErrorField(res, PG_DIAG_MESSAGE_PRIMARY);<br /> }<br /><br
/>So,SQLSTATE field is not always presents.<br /><br />Regards,<br />Dmitriy<br />
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@timbira.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@timbira.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@timbira.com> wrote: > Robert Haas escreveu: >> On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira >> <euler@timbira.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@gmail.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@timbira.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,<br /><br />Okay, as Robert points, "00000" code in successful messages seems as waste<br />of bytes. But accordingto the documentation, "All messages emitted by the<br /><span class="PRODUCTNAME">PostgreSQL</span> server are assignedfive-character error codes that follow the SQL<br /> standard's conventions for <span class="QUOTE">"SQLSTATE"</span>codes." - the first sentence of<br /><a href="http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html">http://www.postgresql.org/docs/9.0/static/errcodes-appendix.html</a><br clear="all"/><br />-- <br />Regards,<br />Dmitriy<br /><br />
On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin <dmitigr@gmail.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@gmail.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@gmail.com> writes: > On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin <dmitigr@gmail.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