On 2025/02/26 13:09, Sagar Shedge wrote:
>> However, if PGconn is NULL, it seems like postgres_fdw_get_connections()
>> wouldn't include that connection in the result. If the connection status
>> is not CONNECTION_OK, it looks like the connection would be closed by
>> pgfdw_reset_xact_state() before the local backend processes the query
>> calling postgres_fdw_get_connections(). So, can remote_backend_pid really
>> be NULL?
> I agree on this point with the current implementation of postgres_fdw.
> There can be different state of connection (PGconn) status like
> CONNECTION_OK, CONNECTION_BAD, CONNECTION_STARTED etc. Currently
> connect_pg_server makes sure to create connections with status
> CONNECTION_OK or abort current transaction if it fails. All other
> intermediate states are handled within the libpq library API
> libpqsrv_connect_params. So there is no way in the connection workflow
> to return connection with status other than CONNECTION_OK and we are
> safe here.
> There is a case where connection status can be set to CONNECTION_BAD
> if we failed to read the result. But in that case we invoke an error
> and abort the transaction. As you mentioned, pgfdw_reset_xact_state
> gets called in a transaction callback which will make sure to close
> the connection at the end of transaction. Again here also we look safe
> in the query execution workflow.
>
> Only thing which bothers me is the asynchronous workflow. postgres_fdw
> uses libpq library and library provides mechanism to perform
> asynchronous API [1]. These asynchronous API's can set connection
> status to CONNECTION_BAD (during pqReadData). Currently postgres_fdw
> makes sure to close connections at the end of query if something
> fails. But let's say in the future we support SQL commands to initiate
> pipeline mode and retrieve data asynchronously.In this case we end up
> with CONNECTION_BAD status across query?
Yes, this may happen in the future. But wouldn't it be enough to just
update postgres_fdw_get_connections() to handle it when the time comes?
So, for now basically postgres_fdw_get_connections() doesn't need to
handle that future case, does it?
For now, I think it makes sense to keep postgres_fdw_get_connections()
aligned with the current implementation. Otherwise, explaining what
remote_backend_pid = NULL means could be confusing, especially since
pipeline mode isn't supported yet in postgres_fdw.
Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
