Re: [HACKERS] SQLSTATE of notice PGresult
Hey all, Okay, as Robert points, 0 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
Re: [HACKERS] SQLSTATE of notice PGresult
On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin dmit...@gmail.com wrote: Okay, as Robert points, 0 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Robert Haas robertmh...@gmail.com writes: On Wed, Sep 22, 2010 at 4:18 AM, Dmitriy Igrishin dmit...@gmail.com wrote: Okay, as Robert points, 0 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira eu...@timbira.com wrote: Dmitriy Igrishin escreveu: /* NOT presents - NULL. Why not 0 ? */ 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Robert Haas escreveu: On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira eu...@timbira.com wrote: Dmitriy Igrishin escreveu: /* NOT presents - NULL. Why not 0 ? */ 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
On Tue, Aug 24, 2010 at 9:44 PM, Euler Taveira de Oliveira eu...@timbira.com wrote: Robert Haas escreveu: On Fri, Aug 20, 2010 at 11:05 AM, Euler Taveira de Oliveira eu...@timbira.com wrote: Dmitriy Igrishin escreveu: /* NOT presents - NULL. Why not 0 ? */ 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 0 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Robert Haas robertmh...@gmail.com writes: I suppose we could change the function to return 0 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 0 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
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 0 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
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 0 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 - 1.43 --- src/interfaces/libpq/fe-protocol3.c 21 Aug 2010 02:41:01 - *** *** 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 0) is +* hardcoded here because we avoid including elog routines +* here. +*/ + pqSaveMessageField(conn-result, PG_DIAG_SQLSTATE, 0); 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Euler Taveira de Oliveira eu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SQLSTATE of notice PGresult
Dmitriy Igrishin escreveu: /* NOT presents - NULL. Why not 0 ? */ 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
