Re: pgsql: postgres_fdw: reestablish new connection if cached one is detect

Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> 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).

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.

Other than that, this seems reasonable by eyeball (I didn't do any
testing).

regards, tom lane