On 2020/10/15 16:21, Fujii Masao wrote:
On 2020/10/14 3:34, Tom Lane wrote:
Fujii Masao <[email protected]> writes:
On 2020/10/11 9:16, Tom Lane wrote:
Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:
* The control flow seems rather forced. I think it was designed
on the assumption that reindenting the existing code is forbidden.
Isn't it simpler and easier-to-read to just reestablish new connection again
in the retry case instead of using a loop because we retry only once?
Patch attached.
Yeah, that seems better.
* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile. It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting. It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.
"PQstatus()==CONNECTION_BAD" was checked for that purpose. But that's not
enough?
Anyway, in the patch, I changed the code so that
sqlstate==ERRCODE_CONNECTION_FAILURE
is checked.
The reason I'm concerned about this is that we could potentially throw an
elog(ERROR) somewhere between return from libpq and the expected ereport
call in pgfdw_report_error. It wouldn't be wise to ignore such a
condition (it might be out-of-memory or some such).
Understood.
Personally I'd code the check with explicit tests for *both*
PQstatus()==CONNECTION_BAD and sqlstate==ERRCODE_CONNECTION_FAILURE,
rather than just asserting that the latter must imply the former.
By my reading, pgfdw_report_error will report ERRCODE_CONNECTION_FAILURE
for any libpq-originated error condition, but not all of those will
set CONNECTION_BAD.
Yes, this makes sense. I updated the patch so that both sqlstate and
PQstatus() are checked. Patch attached.
Other than that, this seems reasonable by eyeball (I didn't do any
testing).
Thanks! Barring any objection, I will commit it.
Pushed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
