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

On 2020/10/15 16:21, Fujii Masao wrote:
>
>
> 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.

Pushed. Thanks!

Regards,

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