Thread: "unexpected EOF" messages
I had a request from a customer asking if we could make a switch to specifically disable the "unexpected EOF" message that fills lots of peoples logs. Along the same way that we have a flag to turn off the "nonstandard use of string escapes" message that is another culprit (that's actually a much *worse* problem than just the unexpected EOF). The unexpected EOF message *does* indicate the client is doing something stupid, but it's not like it's an *actual problem* in pretty much every deployment out there... Would we consider adding such a switch (it should be easy enough to do), or do we want to push this off to the mythical "let's improve the logging subsystem" project that might eventually materialize if we're lucky? Meaning - would people object to such a switch? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Thu, May 3, 2012 at 1:26 PM, Magnus Hagander <magnus@hagander.net> wrote: > I had a request from a customer asking if we could make a switch to > specifically disable the "unexpected EOF" message that fills lots of > peoples logs. Along the same way that we have a flag to turn off the > "nonstandard use of string escapes" message that is another culprit > (that's actually a much *worse* problem than just the unexpected EOF). > The unexpected EOF message *does* indicate the client is doing > something stupid, but it's not like it's an *actual problem* in pretty > much every deployment out there... > > Would we consider adding such a switch (it should be easy enough to > do), or do we want to push this off to the mythical "let's improve the > logging subsystem" project that might eventually materialize if we're > lucky? Meaning - would people object to such a switch? Yes, if the new parameter allows a generic filter on multiple user-specified message types. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, May 3, 2012 at 2:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Thu, May 3, 2012 at 1:26 PM, Magnus Hagander <magnus@hagander.net> wrote: >> I had a request from a customer asking if we could make a switch to >> specifically disable the "unexpected EOF" message that fills lots of >> peoples logs. Along the same way that we have a flag to turn off the >> "nonstandard use of string escapes" message that is another culprit >> (that's actually a much *worse* problem than just the unexpected EOF). >> The unexpected EOF message *does* indicate the client is doing >> something stupid, but it's not like it's an *actual problem* in pretty >> much every deployment out there... >> >> Would we consider adding such a switch (it should be easy enough to >> do), or do we want to push this off to the mythical "let's improve the >> logging subsystem" project that might eventually materialize if we're >> lucky? Meaning - would people object to such a switch? > > Yes, if the new parameter allows a generic filter on multiple > user-specified message types. Uh, just to be clear, you object *if* it has the generic filter? Also, AFAIK we don't *have* a "message type" at this point (one of the things said mythical project wanted to look at), so the only thing we could really filter on would be the whole text of the message, no? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Thu, May 3, 2012 at 2:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
Are you answering the "Would we consider" or the "would people object"?
> Would we consider adding such a switch (it should be easy enough toYes, if the new parameter allows a generic filter on multiple
> do), or do we want to push this off to the mythical "let's improve the
> logging subsystem" project that might eventually materialize if we're
> lucky? Meaning - would people object to such a switch?
user-specified message types.
Are you answering the "Would we consider" or the "would people object"?
On Thu, May 3, 2012 at 2:34 PM, Vik Reykja <vikreykja@gmail.com> wrote: > On Thu, May 3, 2012 at 2:31 PM, Simon Riggs <simon@2ndquadrant.com> wrote: >> >> > Would we consider adding such a switch (it should be easy enough to >> > do), or do we want to push this off to the mythical "let's improve the >> > logging subsystem" project that might eventually materialize if we're >> > lucky? Meaning - would people object to such a switch? >> >> Yes, if the new parameter allows a generic filter on multiple >> user-specified message types. > > > Are you answering the "Would we consider" or the "would people object"? Oh, nice catch - I guess my phrasing of those two questions was really stupid :) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander wrote: > Also, AFAIK we don't *have* a "message type" at this point (one of > the things said mythical project wanted to look at), so the only > thing we could really filter on would be the whole text of the > message, no? We have SQLSTATE, but this seems to be one of those situations where we've been sloppy about using the right value. We seem to be using '08P01' (protocol_violation), which is also used for finding the wrong bytes on a working connection. It seems to me a broken connection is exactly the case where you would expect to see '08006' (connection_failure). FWIW, there are also specific exceptions for rejecting a connection attempt, and for attempting to send something when no connection exists. We don't need to invent new mechanisms for categorizing messages; we just need to start consistently using the one we have correctly. -Kevin
Simon Riggs <simon@2ndQuadrant.com> writes:
> On Thu, May 3, 2012 at 1:26 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> I had a request from a customer asking if we could make a switch to
>> specifically disable the "unexpected EOF" message that fills lots of
>> peoples logs.
> Yes, if the new parameter allows a generic filter on multiple
> user-specified message types.
I agree with Simon --- a disable for that specific message seems like a
kluge, and an ugly one at that. (The right solution for this customer
is to fix their broken application.) But a generic filter capability
might be useful enough to justify its keep.
regards, tom lane
On Thu, May 3, 2012 at 2:46 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Magnus Hagander wrote: > >> Also, AFAIK we don't *have* a "message type" at this point (one of >> the things said mythical project wanted to look at), so the only >> thing we could really filter on would be the whole text of the >> message, no? > > We have SQLSTATE, but this seems to be one of those situations where > we've been sloppy about using the right value. We seem to be using > '08P01' (protocol_violation), which is also used for finding the > wrong bytes on a working connection. It seems to me a broken > connection is exactly the case where you would expect to see '08006' > (connection_failure). FWIW, there are also specific exceptions for > rejecting a connection attempt, and for attempting to send something > when no connection exists. > > We don't need to invent new mechanisms for categorizing messages; we > just need to start consistently using the one we have correctly. While it might work a bit for this one, do we really expect to be able to map a single SQLSTATE to each single message at any point? Unless we can do that, it's never going to "go all the way" - though it might still be useful of course. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Thu, May 3, 2012 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> On Thu, May 3, 2012 at 1:26 PM, Magnus Hagander <magnus@hagander.net> wrote: >>> I had a request from a customer asking if we could make a switch to >>> specifically disable the "unexpected EOF" message that fills lots of >>> peoples logs. > >> Yes, if the new parameter allows a generic filter on multiple >> user-specified message types. > > I agree with Simon --- a disable for that specific message seems like a > kluge, and an ugly one at that. (The right solution for this customer > is to fix their broken application.) But a generic filter capability > might be useful enough to justify its keep. Are you thinking basically "regexp against the main text", or something else, when you say "generic filter capacity"? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, May 3, 2012 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I agree with Simon --- a disable for that specific message seems like a
>> kluge, and an ugly one at that. (The right solution for this customer
>> is to fix their broken application.) But a generic filter capability
>> might be useful enough to justify its keep.
> Are you thinking basically "regexp against the main text", or
> something else, when you say "generic filter capacity"?
In the context of yesterday's discussions, I wonder whether a filter by
SQLSTATE would be appropriate.
regards, tom lane
On Thu, May 3, 2012 at 4:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On Thu, May 3, 2012 at 4:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I agree with Simon --- a disable for that specific message seems like a >>> kluge, and an ugly one at that. (The right solution for this customer >>> is to fix their broken application.) But a generic filter capability >>> might be useful enough to justify its keep. > >> Are you thinking basically "regexp against the main text", or >> something else, when you say "generic filter capacity"? > > In the context of yesterday's discussions, I wonder whether a filter by > SQLSTATE would be appropriate. I'm worried it's not really granular enough. regexp-on-text would also have the advantage of being able to filter stuff coming from stored procedures or such as well - without having to invent a whole bunch of SQLSTATEs to put in the stored procedures (consider the usecase when somebody else wrote the stored procedures and the DBA wants to limit the logging). We could have two parameters of course - log_filter_sqlstate and log_filter_re or something like that... -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, May 3, 2012 at 4:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> Are you thinking basically "regexp against the main text", or
>>> something else, when you say "generic filter capacity"?
>> In the context of yesterday's discussions, I wonder whether a filter by
>> SQLSTATE would be appropriate.
> I'm worried it's not really granular enough.
I dislike the idea of regex-on-text because of i18n issues. There's no
guarantee for instance that all sessions are running with the same
LC_MESSAGES locale. In any case, anybody who's dead set on doing it
that way can do it today with grep.
regards, tom lane
Excerpts from Magnus Hagander's message of jue may 03 10:58:12 -0400 2012: > On Thu, May 3, 2012 at 4:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > In the context of yesterday's discussions, I wonder whether a filter by > > SQLSTATE would be appropriate. > > I'm worried it's not really granular enough. Yeah. > regexp-on-text would also have the advantage of being able to filter > stuff coming from stored procedures or such as well - without having > to invent a whole bunch of SQLSTATEs to put in the stored procedures > (consider the usecase when somebody else wrote the stored procedures > and the DBA wants to limit the logging). > > We could have two parameters of course - log_filter_sqlstate and > log_filter_re or something like that... The problem with regexes is that they are so expensive. You just need to forget the start anchor and it's suddenly a serious problem. And if you want to filter out a second message, the config option starts to become rather unwieldy. I wonder if there's a better way to selectively filter out messages -- say some sort of config file that contains a list of filenames/numbers of messages to disable. That particular idea would be a pain to maintain, of course, not to mention that it'd change from one release to the next. Hey, maybe we could add a UUID to each ereport() call site ;-) (Maybe the sites that have a load problem caused by log traffic are not the same sites that would like to filter out messages, and thus using regexes is not really a problem. It doesn't seem to be the kind of bet that we want to do.) -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, May 3, 2012 at 11:20 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Hey, maybe we could add a UUID to each ereport() call site ;-) I can't help but feel we're designing a $10.00 solution to a $0.25 problem. I think I'd actually support adding something like a UUID to every ereport and a filtering mechanism that works on that basis. But let's face it: this particular message is exponentially more annoying than average. We're basically forcing application developers to jump through hoops to avoid filling the log with unnecessary chatter. I've spent a bunch of time trying to get rid of them in various past jobs, and I've never gotten any benefit out of having them. Maybe the solution is to just demote that particular message to DEBUG1 and declare that closing the connection is a perfectly sensible way for an application to indicate that the conversation is over. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 3, 2012 at 11:20 AM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
>> Hey, maybe we could add a UUID to each ereport() call site ;-)
> I can't help but feel we're designing a $10.00 solution to a $0.25
> problem. I think I'd actually support adding something like a UUID to
> every ereport and a filtering mechanism that works on that basis. But
> let's face it: this particular message is exponentially more annoying
> than average. We're basically forcing application developers to jump
> through hoops to avoid filling the log with unnecessary chatter. I've
> spent a bunch of time trying to get rid of them in various past jobs,
> and I've never gotten any benefit out of having them. Maybe the
> solution is to just demote that particular message to DEBUG1 and
> declare that closing the connection is a perfectly sensible way for an
> application to indicate that the conversation is over.
I could support that with one tweak: it's only DEBUG1 if you don't
have an open transaction. Dropping the connection while in a
transaction *is* an application bug; I don't care how lazy the app
programmer is feeling.
regards, tom lane
On Thu, May 3, 2012 at 11:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, May 3, 2012 at 11:20 AM, Alvaro Herrera >> <alvherre@commandprompt.com> wrote: >>> Hey, maybe we could add a UUID to each ereport() call site ;-) > >> I can't help but feel we're designing a $10.00 solution to a $0.25 >> problem. I think I'd actually support adding something like a UUID to >> every ereport and a filtering mechanism that works on that basis. But >> let's face it: this particular message is exponentially more annoying >> than average. We're basically forcing application developers to jump >> through hoops to avoid filling the log with unnecessary chatter. I've >> spent a bunch of time trying to get rid of them in various past jobs, >> and I've never gotten any benefit out of having them. Maybe the >> solution is to just demote that particular message to DEBUG1 and >> declare that closing the connection is a perfectly sensible way for an >> application to indicate that the conversation is over. > > I could support that with one tweak: it's only DEBUG1 if you don't > have an open transaction. Dropping the connection while in a > transaction *is* an application bug; I don't care how lazy the app > programmer is feeling. I agree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Magnus Hagander's message: >> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> In the context of yesterday's discussions, I wonder whether a >>> filter by SQLSTATE would be appropriate. >> >> I'm worried it's not really granular enough. > > Yeah. Just to be sure we're not inventing a problem here, can someone produce an example of a situation where it would not be granular enough (assuming we correct bad SQLSTATE choices where they exist)? I count 232 distinct SQLSTATE values (139 standard values and 93 PostgreSQL-specific values), and we can create more if we want them; although I would recommend against doing that to get finer resolution on a standard SQLSTATE value. A standard value which is too coarse would be the strongest argument for adding some other mechanism, IMO. If we do, I would be inclined toward something to identify distinct conditions within a SQLSTATE, rather than some overarching independent mechanism. -Kevin
On Thu, May 3, 2012 at 11:46 AM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Alvaro Herrera <alvherre@commandprompt.com> wrote: >> Excerpts from Magnus Hagander's message: >>> Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> In the context of yesterday's discussions, I wonder whether a >>>> filter by SQLSTATE would be appropriate. >>> >>> I'm worried it's not really granular enough. >> >> Yeah. > > Just to be sure we're not inventing a problem here, can someone > produce an example of a situation where it would not be granular > enough (assuming we correct bad SQLSTATE choices where they exist)? > > I count 232 distinct SQLSTATE values (139 standard values and 93 > PostgreSQL-specific values), and we can create more if we > want them; although I would recommend against doing that to get > finer resolution on a standard SQLSTATE value. A standard value > which is too coarse would be the strongest argument for adding some > other mechanism, IMO. If we do, I would be inclined toward > something to identify distinct conditions within a SQLSTATE, rather > than some overarching independent mechanism. Well, nearby Tom and I discussed demoting the message to DEBUG1 when no transaction is in progress. Presumably the two messages would share the same SQL state, unless we're going to create separate SQL states for connection-closed-not-in-a-txn and connection-closed-in-a-txn; and yet I think there's a very decent argument that you're much more likely to care about the latter than the former. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> Well, nearby Tom and I discussed demoting the message to DEBUG1 when
> no transaction is in progress. Presumably the two messages would
> share the same SQL state, unless we're going to create separate SQL
> states for connection-closed-not-in-a-txn and
> connection-closed-in-a-txn; and yet I think there's a very decent
> argument that you're much more likely to care about the latter than
> the former.
If we're going to treat the two cases differently then assigning
distinct SQLSTATEs seems entirely reasonable to me.
regards, tom lane
On Thu, May 3, 2012 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, May 3, 2012 at 11:20 AM, Alvaro Herrera
>> <alvherre@commandprompt.com> wrote:
>>> Hey, maybe we could add a UUID to each ereport() call site ;-)
>
>> I can't help but feel we're designing a $10.00 solution to a $0.25
>> problem. I think I'd actually support adding something like a UUID to
>> every ereport and a filtering mechanism that works on that basis. But
>> let's face it: this particular message is exponentially more annoying
>> than average. We're basically forcing application developers to jump
>> through hoops to avoid filling the log with unnecessary chatter. I've
>> spent a bunch of time trying to get rid of them in various past jobs,
>> and I've never gotten any benefit out of having them. Maybe the
>> solution is to just demote that particular message to DEBUG1 and
>> declare that closing the connection is a perfectly sensible way for an
>> application to indicate that the conversation is over.
>
> I could support that with one tweak: it's only DEBUG1 if you don't
> have an open transaction. Dropping the connection while in a
> transaction *is* an application bug; I don't care how lazy the app
> programmer is feeling.
I agree - that would certainly be a good fix for this one. One
question is do we want something like this:
- ereport(COMMERROR,
+ ereport(IsTransactionState() ? COMMERROR : DEBUG1,
(errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("unexpected EOF on client connection")));
(in a couple of places, yes)
or do we want to make the text of the error message different as well,
saying something like "unexpected EOF on client connection with an
open transaction"?
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, May 3, 2012 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I could support that with one tweak: it's only DEBUG1 if you don't
>> have an open transaction. �Dropping the connection while in a
>> transaction *is* an application bug; I don't care how lazy the app
>> programmer is feeling.
> I agree - that would certainly be a good fix for this one. One
> question is do we want something like this:
> - ereport(COMMERROR,
> + ereport(IsTransactionState() ? COMMERROR : DEBUG1,
> (errcode(ERRCODE_PROTOCOL_VIOLATION),
> errmsg("unexpected EOF on client connection")));
> or do we want to make the text of the error message different as well,
> saying something like "unexpected EOF on client connection with an
> open transaction"?
I'd vote for different texts and different SQLSTATEs too, per other
discussion. (I think we'd decided that ERRCODE_PROTOCOL_VIOLATION
was a bad choice anyway.)
Also, I'm afraid that the above patch probably doesn't work as-is;
won't elog.c try to send the DEBUG1 message to the client? I think
you'll need some additional code to shut down error message output
first. Resetting whereToSendOutput is probably sufficient.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Well, nearby Tom and I discussed demoting the message to DEBUG1 >> when no transaction is in progress. Presumably the two messages >> would share the same SQL state, unless we're going to create >> separate SQL states for connection-closed-not-in-a-txn and >> connection-closed-in-a-txn; and yet I think there's a very decent >> argument that you're much more likely to care about the latter >> than the former. > > If we're going to treat the two cases differently then assigning > distinct SQLSTATEs seems entirely reasonable to me. Would it make sense to use 08003 (connection_does_not_exist) when a broken connection for an idle process is discovered, and 08006 (connection_failure) for the "in transaction" failure? What about a failure just after COMMIT and before successfully sending that result to the client? I notice there's a SQLSTATE 08007 (transaction_resolution_unknown), but I don't know whether that makes sense on the server side, or just on the client side. -Kevin
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Would it make sense to use 08003 (connection_does_not_exist) when a
> broken connection for an idle process is discovered, and 08006
> (connection_failure) for the "in transaction" failure? What about a
> failure just after COMMIT and before successfully sending that
> result to the client? I notice there's a SQLSTATE 08007
> (transaction_resolution_unknown), but I don't know whether that
> makes sense on the server side, or just on the client side.
AFAICS, all the 08 class is meant to be issued by client-side code,
not the server. I think we probably have to use nonstandard SQLSTATEs
for these messages.
regards, tom lane
On Thu, May 3, 2012 at 7:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Thu, May 3, 2012 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I could support that with one tweak: it's only DEBUG1 if you don't
>>> have an open transaction. Dropping the connection while in a
>>> transaction *is* an application bug; I don't care how lazy the app
>>> programmer is feeling.
>
>> I agree - that would certainly be a good fix for this one. One
>> question is do we want something like this:
>
>> - ereport(COMMERROR,
>> + ereport(IsTransactionState() ? COMMERROR : DEBUG1,
>> (errcode(ERRCODE_PROTOCOL_VIOLATION),
>> errmsg("unexpected EOF on client connection")));
>
>> or do we want to make the text of the error message different as well,
>> saying something like "unexpected EOF on client connection with an
>> open transaction"?
>
> I'd vote for different texts and different SQLSTATEs too, per other
> discussion. (I think we'd decided that ERRCODE_PROTOCOL_VIOLATION
> was a bad choice anyway.)
>
> Also, I'm afraid that the above patch probably doesn't work as-is;
> won't elog.c try to send the DEBUG1 message to the client? I think
> you'll need some additional code to shut down error message output
> first. Resetting whereToSendOutput is probably sufficient.
Yeah, I didn't go as far as testing it - there's also more than one
spot where we log it... I'll cook up a patch.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Thu, May 3, 2012 at 7:21 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, May 3, 2012 at 7:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>> On Thu, May 3, 2012 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I could support that with one tweak: it's only DEBUG1 if you don't
>>>> have an open transaction. Dropping the connection while in a
>>>> transaction *is* an application bug; I don't care how lazy the app
>>>> programmer is feeling.
>>
>>> I agree - that would certainly be a good fix for this one. One
>>> question is do we want something like this:
>>
>>> - ereport(COMMERROR,
>>> + ereport(IsTransactionState() ? COMMERROR : DEBUG1,
>>> (errcode(ERRCODE_PROTOCOL_VIOLATION),
>>> errmsg("unexpected EOF on client connection")));
>>
>>> or do we want to make the text of the error message different as well,
>>> saying something like "unexpected EOF on client connection with an
>>> open transaction"?
>>
>> I'd vote for different texts and different SQLSTATEs too, per other
>> discussion. (I think we'd decided that ERRCODE_PROTOCOL_VIOLATION
>> was a bad choice anyway.)
>>
>> Also, I'm afraid that the above patch probably doesn't work as-is;
>> won't elog.c try to send the DEBUG1 message to the client? I think
>> you'll need some additional code to shut down error message output
>> first. Resetting whereToSendOutput is probably sufficient.
>
> Yeah, I didn't go as far as testing it - there's also more than one
> spot where we log it... I'll cook up a patch.
Heh - we already used ERRCODE_CONNECTION_FAILURE on the errors in
copy.c. Since COPY can only happen when there is a transaction
(right?), I just changed those error messages for consistency.
This patch works through my testing - can anyone spot a hole in it still?
The next question is - of course - whether we can sneak this in before beta...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
Attachment
Tom Lane <tgl@sss.pgh.pa.us> wrote: > AFAICS, all the 08 class is meant to be issued by client-side > code, not the server. I think we probably have to use nonstandard > SQLSTATEs for these messages. OK, if we're going that route, how about using "Class 2D * Invalid Transaction Termination"? I still think it might be useful to differentiate in our server log between the case where the transaction failed and the case where the transaction committed but we don't know that the client got the news of that. How about something like: 2DP01 connection_lost_during_transaction 2DP02 connection_lost_during_commit_notification I'm less sure what makes sense if the connection fails while idle (not in transaction). If you don't like "Class 08 * Connection Exception" for that, I'm not quite sure where it belongs. -Kevin
Magnus Hagander <magnus@hagander.net> writes:
> Heh - we already used ERRCODE_CONNECTION_FAILURE on the errors in
> copy.c. Since COPY can only happen when there is a transaction
> (right?), I just changed those error messages for consistency.
Agreed on changing the message texts to match, but I wonder whether
we ought not switch all those SQLSTATEs to something different. Per my
comment to Kevin, I think the whole 08 class is meant to be issued on
the client side. Maybe it's okay to conflate a server-detected
connection loss with client-detected loss, but I'm not convinced.
regards, tom lane
"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> I still think it might be useful to differentiate in our server log
> between the case where the transaction failed and the case where the
> transaction committed but we don't know that the client got the news
> of that. How about something like:
> 2DP01 connection_lost_during_transaction
> 2DP02 connection_lost_during_commit_notification
That would be a useful distinction, but I'm not sure how easily our
code can make it.
> I'm less sure what makes sense if the connection fails while idle
> (not in transaction). If you don't like "Class 08 * Connection
> Exception" for that, I'm not quite sure where it belongs.
I'm not convinced that these cases belong in any of the standard's
classes. IMO the standard is only standardizing application-visible
error cases, which these are not. In particular I think class 2D is
not appropriate, since AFAICS the standard means that to pertain to
incorrect issuance of a COMMIT or ROLLBACK command.
regards, tom lane
On Thu, May 3, 2012 at 7:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Heh - we already used ERRCODE_CONNECTION_FAILURE on the errors in >> copy.c. Since COPY can only happen when there is a transaction >> (right?), I just changed those error messages for consistency. > > Agreed on changing the message texts to match, but I wonder whether > we ought not switch all those SQLSTATEs to something different. Per my > comment to Kevin, I think the whole 08 class is meant to be issued on > the client side. Maybe it's okay to conflate a server-detected > connection loss with client-detected loss, but I'm not convinced. Sure,that's a simple search and replace of course... If we can come to a decision about what codes to actually use. I'm not sure I have much input other than that I agree they need to be different :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Thu, May 3, 2012 at 9:26 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Thu, May 3, 2012 at 7:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Magnus Hagander <magnus@hagander.net> writes: >>> Heh - we already used ERRCODE_CONNECTION_FAILURE on the errors in >>> copy.c. Since COPY can only happen when there is a transaction >>> (right?), I just changed those error messages for consistency. >> >> Agreed on changing the message texts to match, but I wonder whether >> we ought not switch all those SQLSTATEs to something different. Per my >> comment to Kevin, I think the whole 08 class is meant to be issued on >> the client side. Maybe it's okay to conflate a server-detected >> connection loss with client-detected loss, but I'm not convinced. > > Sure,that's a simple search and replace of course... If we can come to > a decision about what codes to actually use. I'm not sure I have much > input other than that I agree they need to be different :-) Any further suggestoins for which codes to use? If not, I think I'm going to commit the patch as I had it, because it's not any worse than what we had before (but fixes the annoying messages), and we can always revisit the actual errorcodes later. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> Any further suggestoins for which codes to use? If not, I think I'm
> going to commit the patch as I had it, because it's not any worse than
> what we had before (but fixes the annoying messages), and we can
> always revisit the actual errorcodes later.
I'm still a bit uncomfortable about using the 08 codes on the backend
side; but on reflection it's hard to see how it could cause any real
confusion, so maybe we should just go with that.
Another point is that the patch would be shorter and more reliable
if you just forced whereToSendOutput = DestNone, without trying to save
and restore it. Once the connection is known busted, there is no point
in sending any future I/O towards the client, either.
regards, tom lane
On Mon, May 7, 2012 at 5:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> Any further suggestoins for which codes to use? If not, I think I'm >> going to commit the patch as I had it, because it's not any worse than >> what we had before (but fixes the annoying messages), and we can >> always revisit the actual errorcodes later. > > I'm still a bit uncomfortable about using the 08 codes on the backend > side; but on reflection it's hard to see how it could cause any real > confusion, so maybe we should just go with that. > > Another point is that the patch would be shorter and more reliable > if you just forced whereToSendOutput = DestNone, without trying to save > and restore it. Once the connection is known busted, there is no point > in sending any future I/O towards the client, either. Makes sense, will change and commit. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Mon, May 7, 2012 at 12:39 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Makes sense, will change and commit.
Since the following hunk is repeated 3x, maybe it should be stuffed
into a function that is then called in three places:
+ if (IsTransactionState())
+ ereport(COMMERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("unexpected EOF on
client connection with an open transaction")));
+ else
+ {
+ /*
+ * Can't send DEBUG log messages to client at
this point.
+ * Since we're disconnecting right away, we
don't need to
+ * restore whereToSendOutput.
+ */
+ whereToSendOutput = DestNone;
+ ereport(DEBUG1,
+
(errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
+ errmsg("unexpected EOF on
client connection")));
+ }
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, May 7, 2012 at 7:18 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, May 7, 2012 at 12:39 PM, Magnus Hagander <magnus@hagander.net> wrote: >> Makes sense, will change and commit. > > Since the following hunk is repeated 3x, maybe it should be stuffed > into a function that is then called in three places: I considered it trivial enough not to do that for it. I can perhaps be convinced otherwise, but I doubt it's worth it.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes:
> On Mon, May 7, 2012 at 7:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Since the following hunk is repeated 3x, maybe it should be stuffed
>> into a function that is then called in three places:
> I considered it trivial enough not to do that for it. I can perhaps be
> convinced otherwise, but I doubt it's worth it..
I had considered suggesting the same, but decided not to on the grounds
that if we fold these into a subroutine, it will no longer be possible
to tell from the file-and-line-number info which call site reported the
error. I'm not sure that there would be cases where we'd want to tell
that, but I'm not sure there wouldn't be, either. So on the whole I
agree with the way Magnus coded it.
regards, tom lane