Re: Retry Cached Remote Connections for postgres_fdw in case remote backend gets killed/goes away

On 2020/07/17 21:02, Bharath Rupireddy wrote:
>>
>> On Tue, Jul 14, 2020 at 6:13 PM Ashutosh Bapat
>> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>>>
>>> Has this been added to CF, possibly next CF?
>>>
>>
>> I have not added yet. Request to get it done in this CF, as the final
>> patch for review(v3 patch) is ready and shared. We can target it to
>> the next CF if there are major issues with the patch.
>>
>
> I added this feature to the next CF - https://commitfest.postgresql.org/29/2651/

Thanks for the patch! Here are some comments from me.

+ PQreset(entry->conn);

Isn't using PQreset() to reconnect to the foreign server unsafe?
When new connection is established, some SET commands seem
to need to be issued like configure_remote_session() does.
But PQreset() doesn't do that at all.

Originally when GetConnection() establishes new connection,
it resets all transient state fields, to be sure all are clean (as the
comment says). With the patch, even when reconnecting
the remote server, shouldn't we do the same, for safe?

+ PGresult *res = NULL;
+ res = PQgetResult(entry->conn);
+ PQclear(res);

Are these really necessary? I was just thinking that's not because
pgfdw_get_result() and pgfdw_report_error() seem to do that
already in do_sql_command().

+ /* Start a new transaction or subtransaction if needed. */
+ begin_remote_xact(entry);

Even when there is no cached connection and new connection is made,
then if begin_remote_xact() reports an error, another new connection
tries to be made again. This behavior looks a bit strange.

Regards,

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