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

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