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

On 2020/10/14 3:34, Tom Lane wrote:
> 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).

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.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION