Re: Adjust errorcode in background worker code

On 2015/11/07 3:55, Robert Haas wrote:
> On Sun, Jun 28, 2015 at 10:43 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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