Thread: Adjust errorcode in background worker code
Hi, How about the attached that adjusts errorcode for the error related to checking the flag bgw_flags in BackgroundWorkerInitializeConnection*() functions so that it matches the treatment in SanityCheckBackgroundWorker()? s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g There is already a "/* XXX is this the right errcode? */" there. Thanks, Amit
Attachment
On 2015-06-29 AM 11:36, Amit Langote wrote: > Hi, > > How about the attached that adjusts errorcode for the error related to > checking the flag bgw_flags in BackgroundWorkerInitializeConnection*() > functions so that it matches the treatment in SanityCheckBackgroundWorker()? > > s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g > > There is already a "/* XXX is this the right errcode? */" there. > Oops, a wrong thing got attached. Please find correct one attached this time. Thanks, Amit
Attachment
On Sun, Jun 28, 2015 at 10:43 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > On 2015-06-29 AM 11:36, Amit Langote wrote: >> Hi, >> >> How about the attached that adjusts errorcode for the error related to >> checking the flag bgw_flags in BackgroundWorkerInitializeConnection*() >> functions so that it matches the treatment in SanityCheckBackgroundWorker()? >> >> s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g >> >> There is already a "/* XXX is this the right errcode? */" there. > > Oops, a wrong thing got attached. > > Please find correct one attached this time. Well, I'm just catching up on some old email and saw this thread. I like the idea of trying to use the best possible error code, but I'm not so sure this is an improvement. One problem is that ERRCODE_INVALID_PARAMETER_VALUE is that we use it, uh, a lot: [rhaas pgsql]$ git grep ERRCODE_ | sed 's/.*ERRCODE_/ERRCODE_/; s/[^A-Z0-9_].*//;' | sort | uniq -c | sort -n -r | head540 ERRCODE_FEATURE_NOT_SUPPORTED442 ERRCODE_INVALID_PARAMETER_VALUE380ERRCODE_SYNTAX_ERROR194 ERRCODE_WRONG_OBJECT_TYPE194 ERRCODE_UNDEFINED_OBJECT181 ERRCODE_DATATYPE_MISMATCH180ERRCODE_INSUFFICIENT_PRIVILEGE150 ERRCODE_INVALID_TEXT_REPRESENTATION137 ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE123ERRCODE_PROGRAM_LIMIT_EXCEEDED I wonder if we need to think about inventing some new error codes. I can sort of understand that "feature not supported" is something that can come in a large number of different contexts and mean pretty much the same all the time, but I'm betting that things like "invalid parameter value" and "invalid text representation" and "object not in prerequisite state" cover an amazing breadth of errors that may not actually be that similar to each other. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 7 November 2015 at 02:55, Robert Haas <robertmhaas@gmail.com> wrote: > I wonder if we need to think about inventing some new error codes. I > can sort of understand that "feature not supported" is something that > can come in a large number of different contexts and mean pretty much > the same all the time, but I'm betting that things like "invalid > parameter value" and "invalid text representation" and "object not in > prerequisite state" cover an amazing breadth of errors that may not > actually be that similar to each other. That depends on what client applications are going to do about the error. If the app's only choice is likely to be "huh, that's weird, I'll barf and tell the user about it" then there's little point changing anything. The app just spits out a different error and nobody gains anything. If there are error sites where an application could take specific, useful action in response to an error then they could well benefit from being changed. ERRCODE_T_R_DEADLOCK_DETECTED for example is extremely useful to applications. So the first step should be auditing error sites of interest to see whether the client app could possibly do anything constructive about the error. Then see if there's an existing category it'd fit better, or if we have a group of errors that would go well together in a new category where the application action for them is similar. Having too many highly specific errors is at least as bad as too few broad categories. You land up with huge case statements and error number lists - which you inevitably miss something from. They're annoying to write, so they start getting re-used and cargo culted in increasingly outdated forms. Next thing you know you've invented Java exception handling ;) Personally the only PostgreSQL errors I've ever cared about handling specifically have been: * deadlock detected, serialization failure for tx abort and retry loop * connection-level errors, for reconnect and retry handling, graceful reporting of DB shutdown, etc * constraint violations, where useful info can be extracted to tell the user what exactly was wrong with their input * access permission errors, to drive more informative client-side UI The great majority of sever-side errors go into the "huh, something's broken that shouldn't have, tell the user to tell the admin" box. I'm nuts about handling errors in my code compared to most of what I've seen. The majority of apps I see aren't even prepared to cope with a deadlock without data loss, tending to barf an error into the logs and/or to the user, forget what they were doing, and hope someone comes to rescue them or re-enter the info. Do you think anyone has *ever* written code to trap ERRCODE_AMBIGUOUS_FUNCTION or ERRCODE_INVALID_ARGUMENT_FOR_LOG? Not counting simple translation layers that map every exception ... I doubt it. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2015/11/07 3:55, Robert Haas wrote: > On Sun, Jun 28, 2015 at 10:43 PM, Amit Langote > <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> On 2015-06-29 AM 11:36, Amit Langote wrote: >>> Hi, >>> >>> How about the attached that adjusts errorcode for the error related to >>> checking the flag bgw_flags in BackgroundWorkerInitializeConnection*() >>> functions so that it matches the treatment in SanityCheckBackgroundWorker()? >>> >>> s/ERRCODE_PROGRAM_LIMIT_EXCEEDED/ERRCODE_INVALID_PARAMETER_VALUE/g >>> >>> There is already a "/* XXX is this the right errcode? */" there. >> >> Oops, a wrong thing got attached. >> >> Please find correct one attached this time. > > Well, I'm just catching up on some old email and saw this thread. I > like the idea of trying to use the best possible error code, but I'm > not so sure this is an improvement. One problem is that > ERRCODE_INVALID_PARAMETER_VALUE is that we use it, uh, a lot: > > [rhaas pgsql]$ git grep ERRCODE_ | sed 's/.*ERRCODE_/ERRCODE_/; > s/[^A-Z0-9_].*//;' | sort | uniq -c | sort -n -r | head > 540 ERRCODE_FEATURE_NOT_SUPPORTED > 442 ERRCODE_INVALID_PARAMETER_VALUE > 380 ERRCODE_SYNTAX_ERROR > 194 ERRCODE_WRONG_OBJECT_TYPE > 194 ERRCODE_UNDEFINED_OBJECT > 181 ERRCODE_DATATYPE_MISMATCH > 180 ERRCODE_INSUFFICIENT_PRIVILEGE > 150 ERRCODE_INVALID_TEXT_REPRESENTATION > 137 ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE > 123 ERRCODE_PROGRAM_LIMIT_EXCEEDED > > I wonder if we need to think about inventing some new error codes. I > can sort of understand that "feature not supported" is something that > can come in a large number of different contexts and mean pretty much > the same all the time, but I'm betting that things like "invalid > parameter value" and "invalid text representation" and "object not in > prerequisite state" cover an amazing breadth of errors that may not > actually be that similar to each other. > Now that I see it, ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE may be a better choice there. In general, I tend to agree with the observations expressed by Craig down-thread. I'd think this particular error code (or more to the point, the error site) does not concern a client app itself but rather suggests a bug in a loadable module implementation which the client app has no way to fix itself. In general, it seems we should only ever report error codes that a client app can do something about. But I guess that's a whole different area and vast project to investigate about. For a rather contrived example related to OP, it would have made sense to send an error code if there was a parameter like worker_spi.can_connect_db which the app set to false and then later did something with it that required a database connection. Thanks, Amit