Re: [BUGS] BUG #4203: perform dblink() in begin/exception returns wrong SQLSTATE code

The following bug has been logged online:

Bug reference: 4203
Logged by: Henry Combrinck
Email address: henry(at)zen(dot)co(dot)za
PostgreSQL version: 8.2.6 and 8.3.1
Operating system: Linux
Description: perform dblink() in begin/exception returns wrong
SQLSTATE code
Details:

Apologies if this is the wrong forum to report this (perhaps it needs to go
to the dblink() maintainer?)

In a function on a machine using 8.2.6, the following returns a strange
SQLSTATE code:

begin
perform dblink ('host=other_machine ...',
'insert into table...');
exception when others then
raise notice 'SQLSTATE: %', SQLSTATE;
end;

The code returned is always 42601 (syntax_error) irrespective of the actual
error (eg, unique_violation).

"Henry Combrinck" <henry(at)zen(dot)co(dot)za> writes:
> Description: perform dblink() in begin/exception returns wrong
> SQLSTATE code

> The code returned is always 42601 (syntax_error) irrespective of the actual
> error (eg, unique_violation).

Yeah, the dblink code should probably try a bit harder to propagate the
original error fields. I'm inclined to think that it should propagate
sqlstate/message/detail/hint verbatim, and indicate the fact that this
happened on a dblink connection as CONTEXT, rather than structuring the
ereport the way it does now. Joe, what do you think?

regards, tom lane

Tom Lane wrote:
> "Henry Combrinck" <henry(at)zen(dot)co(dot)za> writes:
>> Description: perform dblink() in begin/exception returns wrong
>> SQLSTATE code
>
>> The code returned is always 42601 (syntax_error) irrespective of the actual
>> error (eg, unique_violation).
>
> Yeah, the dblink code should probably try a bit harder to propagate the
> original error fields. I'm inclined to think that it should propagate
> sqlstate/message/detail/hint verbatim, and indicate the fact that this
> happened on a dblink connection as CONTEXT, rather than structuring the
> ereport the way it does now. Joe, what do you think?

Sounds reasonable. Do you think this is a bug fix or an 8.4 enhancement?
I will try to take a closer look at the specific fix this weekend.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> Yeah, the dblink code should probably try a bit harder to propagate the
>> original error fields.

> Sounds reasonable. Do you think this is a bug fix or an 8.4 enhancement?

8.4 enhancement I think, since a behavioral change here could pose a
compatibility issue for applications.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> Tom Lane wrote:
>>> Yeah, the dblink code should probably try a bit harder to propagate the
>>> original error fields.
>
>> Sounds reasonable. Do you think this is a bug fix or an 8.4 enhancement?
>
> 8.4 enhancement I think, since a behavioral change here could pose a
> compatibility issue for applications.
>

Here is my proposed patch -- as suggested for cvs tip only.

I haven't been around enough lately to be sure I understand the process
these days. Should I be posting this to the wiki and waiting for the
next commit fest, or just apply myself in a day or two assuming no
objections?

Thanks,

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Here is my proposed patch -- as suggested for cvs tip only.

A few comments:

Don't use errstart/errfinish directly. A reasonable way to deal with
the type of situation you have here is

ereport(ERROR,
(errcode(...),
errmsg(...),
det_msg ? errdetail("%s", det_msg) : 0,
hint_msg ? errhint("%s", hint_msg) : 0,
...));

You can't expect the result of PQresultErrorField to still be valid
after you've PQclear'd the PGresult. I think you'll need to pstrdup
the non-null results first. Or maybe use a PG_TRY block to free the
PGresult on the way out after the error escape ... but pstrdup is
probably cleaner.

This code doesn't cope with the possibility that no SQLSTATE
is available (a distinct possibility for libpq-detected errors).
You'll need to use some generic error code in that case. I'm tempted
to suggest ERRCODE_CONNECTION_FAILURE, on the assumption that if it's
libpq-detected then it's a connection problem.

It would probably be useful to show the name of the dblink connection
in the context.

I'm thinking that you are getting well past what is reasonable to
put in a macro. Time to use an out-of-line function.

Don't use "unable to..." --- this is against the message style guide.
"could not" is approved style. Also note the expectation that context
entries be complete sentences.

> I haven't been around enough lately to be sure I understand the process
> these days. Should I be posting this to the wiki and waiting for the
> next commit fest, or just apply myself in a day or two assuming no
> objections?

No, you can apply it yourself when you feel it's ready. The wiki queue
is just to keep track of stuff that is submitted by non-committers or
that a committer wants extra review of.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> Here is my proposed patch -- as suggested for cvs tip only.
>
> A few comments:

[...great comments as usual...]

Thanks for the excellent feedback! I think the attached addresses it all.

>> I haven't been around enough lately to be sure I understand the process
>> these days. Should I be posting this to the wiki and waiting for the
>> next commit fest, or just apply myself in a day or two assuming no
>> objections?
>
> No, you can apply it yourself when you feel it's ready. The wiki queue
> is just to keep track of stuff that is submitted by non-committers or
> that a committer wants extra review of.

Great. Assuming no other objections I'll commit in a day or three.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> [ improved patch ]

Still a couple quibbles:

> + ereport(level,
> + (errcode(sqlstate),
> + errmsg(message_primary),

This *must* be errmsg("%s", message_primary), else you have big problems
with % in the text. Also, I think it's at least theoretically possible
for message_primary to be null, in which case you'd better substitute
"unknown error" or some such.

You could avoid the ugly cast-away-const by making
dblink_context_conname be const char *, no?

Since dblink_res_error isn't going to return if fail = true, seems
like you could skip the "if (!fail)" tests occurring after calls to it.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> [ improved patch ]
>
> Still a couple quibbles:

[ more good feedback ]

All valid complaints, and noticeably improved/simplified code as a
result. Third patch attached.

Joe

Joe Conway wrote:
> Tom Lane wrote:
>> Joe Conway <mail(at)joeconway(dot)com> writes:
>>> [ improved patch ]
>>
>> Still a couple quibbles:
>
> [ more good feedback ]
>
> All valid complaints, and noticeably improved/simplified code as a
> result. Third patch attached.

Patch applied to HEAD.

Joe