Thread: Re: Extend postgres_fdw_get_connections to return remote backend pid
On 2025/02/18 23:44, Sagar Shedge wrote: > Dear Hackers, > > I want to propose to extend existing postgres_fdw_get_connections > functionality to return remote server backend pid. > Using postgres_fdw extension, backend can establish connections to > remote postgres servers. Recently we added functionality to get > connection status which can help users to detect closed connections > immediately. But currently there is no way to get a remote backend PID > for these connections. I assume you're planning to extend postgres_fdw_get_connections() to also return the result of PQbackendPID(entry->conn). However, the patch you attached doesn't seem to include that change. Did you attach the wrong patch? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2025/02/19 2:06, Sagar Shedge wrote:
> Hi Fujii,
>
> On Tue, Feb 18, 2025 at 10:25 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>
>>
>> I assume you're planning to extend postgres_fdw_get_connections() to
>> also return the result of PQbackendPID(entry->conn).
>> However, the patch you attached doesn't seem to include that change.
>> Did you attach the wrong patch?
>
> My bad!!
> You are right. I was going through an old discussion and attached the
> same old patch file.
>
> Please refer to the patch file to return the remote backend pid.
Thanks for the patch!
Here are my review comments:
The documentation needs to be updated.
+ OUT closed boolean, OUT remote_backend_pid INTEGER)
Naming is always tricky, but remote_backend_pid feels a bit too long.
Would remote_pid be sufficient?
* For API version 1.2 and later, this function takes an input parameter
* to check a connection status and returns the following
* additional values along with the three values from version 1.1:
"three values" should be changed to "four values".
+ * - remote_backend_pid - return remote server backend pid
For consistency with other field comments, "return" seems unnecessary.
How about: "PID of the remote backend handling the connection" instead?
+ /*
+ * If a connection status is not closed and remote backend
+ * ID is valid, return remote backend ID. Otherwise, return NULL.
+ */
+ remote_backend_pid = PQbackendPID(entry->conn);
+ if ((is_conn_closed != 1) && (remote_backend_pid != 0))
+ values[i++] = remote_backend_pid;
Wouldn't it be better to return the result of PQbackendPID() instead of NULL
even when the connection is closed, for debugging purposes? This way,
users can see which remote backend previously handled the "closed" connection,
which might be helpful for troubleshooting.
The postgres_fdw regression test failed on my MacBook with the following diff:
--- /Users/postgres/pgsql/git/contrib/postgres_fdw/expected/postgres_fdw.out 2025-02-19 12:53:27
+++ /Users/postgres/pgsql/git/contrib/postgres_fdw/results/postgres_fdw.out 2025-02-19 17:40:04
@@ -12443,7 +12443,7 @@
FROM postgres_fdw_get_connections(true);
server_name | remote_conn_closed | remote_backend_pid
-------------+--------------------+--------------------
- loopback | false | available
+ loopback | true | available
(1 row)
-- After terminating the remote backend, since the connection is closed,
@@ -12458,7 +12458,7 @@
FROM postgres_fdw_get_connections(true);
server_name | remote_conn_closed | remote_backend_pid
-------------+--------------------+--------------------
- loopback | true | not available
+ loopback | true | available
(1 row)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi Fujii, > Naming is always tricky, but remote_backend_pid feels a bit too long. Would remote_pid be sufficient? Point looks valid. I had another perspective is to align the naming convention to pg_backend_pid(). remote_pid is not helping to identify whether pid belongs to postgres backend or not. Does this make sense? Or I'm fine to go with concise name like `remote_pid` > "three values" should be changed to "four values". Done. Good catch!! > How about: "PID of the remote backend handling the connection" instead? Updated in v2. > Wouldn't it be better to return the result of PQbackendPID() instead of NULL even when the connection is closed, for debugging purposes? This way, users can see which remote backend previously handled the "closed" connection, which might be helpful for troubleshooting. Agree. Updated logic to return backend pid always except when pid is 0 which indicates no backend attached to session. Returning 0 found misleading. What's your thought on this? > The postgres_fdw regression test failed on my MacBook with the following diff: I updated tests to make it more stable. Let me know if it's still failing on your setup. On Wed, Feb 19, 2025 at 2:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/19 2:06, Sagar Shedge wrote: > > Hi Fujii, > > > > On Tue, Feb 18, 2025 at 10:25 PM Fujii Masao > > <masao.fujii@oss.nttdata.com> wrote: > > > >> > >> I assume you're planning to extend postgres_fdw_get_connections() to > >> also return the result of PQbackendPID(entry->conn). > >> However, the patch you attached doesn't seem to include that change. > >> Did you attach the wrong patch? > > > > My bad!! > > You are right. I was going through an old discussion and attached the > > same old patch file. > > > > Please refer to the patch file to return the remote backend pid. > > Thanks for the patch! > > Here are my review comments: > > The documentation needs to be updated. > > > + OUT closed boolean, OUT remote_backend_pid INTEGER) > > Naming is always tricky, but remote_backend_pid feels a bit too long. > Would remote_pid be sufficient? > > > * For API version 1.2 and later, this function takes an input parameter > * to check a connection status and returns the following > * additional values along with the three values from version 1.1: > > "three values" should be changed to "four values". > > > + * - remote_backend_pid - return remote server backend pid > > For consistency with other field comments, "return" seems unnecessary. > How about: "PID of the remote backend handling the connection" instead? > > > + /* > + * If a connection status is not closed and remote backend > + * ID is valid, return remote backend ID. Otherwise, return NULL. > + */ > + remote_backend_pid = PQbackendPID(entry->conn); > + if ((is_conn_closed != 1) && (remote_backend_pid != 0)) > + values[i++] = remote_backend_pid; > > Wouldn't it be better to return the result of PQbackendPID() instead of NULL > even when the connection is closed, for debugging purposes? This way, > users can see which remote backend previously handled the "closed" connection, > which might be helpful for troubleshooting. > > > The postgres_fdw regression test failed on my MacBook with the following diff: > > --- /Users/postgres/pgsql/git/contrib/postgres_fdw/expected/postgres_fdw.out 2025-02-19 12:53:27 > +++ /Users/postgres/pgsql/git/contrib/postgres_fdw/results/postgres_fdw.out 2025-02-19 17:40:04 > @@ -12443,7 +12443,7 @@ > FROM postgres_fdw_get_connections(true); > server_name | remote_conn_closed | remote_backend_pid > -------------+--------------------+-------------------- > - loopback | false | available > + loopback | true | available > (1 row) > > -- After terminating the remote backend, since the connection is closed, > @@ -12458,7 +12458,7 @@ > FROM postgres_fdw_get_connections(true); > server_name | remote_conn_closed | remote_backend_pid > -------------+--------------------+-------------------- > - loopback | true | not available > + loopback | true | available > (1 row) > > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/21 0:54, Sagar Shedge wrote:
> Hi Fujii,
>
>> Naming is always tricky, but remote_backend_pid feels a bit too long.
> Would remote_pid be sufficient?
> Point looks valid. I had another perspective is to align the naming
> convention to pg_backend_pid(). remote_pid is not helping to identify
> whether pid belongs to postgres backend or not. Does this make sense?
> Or I'm fine to go with concise name like `remote_pid`
I initially thought "remote_pid" was sufficient since the postgres_fdw
connection clearly corresponds to a backend process. However, I'm fine
with keeping "remote_backend_pid" as the column name for now. If we find
a better name later, we can rename it.
>> How about: "PID of the remote backend handling the connection" instead?
> Updated in v2.
Thanks for updating the patch!
You still need to update the documentation. Could you add descriptions
for postgres_fdw_get_connections()?
>> Wouldn't it be better to return the result of PQbackendPID() instead of NULL
> even when the connection is closed, for debugging purposes? This way,
> users can see which remote backend previously handled the "closed" connection,
> which might be helpful for troubleshooting.
> Agree. Updated logic to return backend pid always except when pid is 0
> which indicates no backend attached to session. Returning 0 found
> misleading. What's your thought on this?
Your approach makes sense to me.
>> The postgres_fdw regression test failed on my MacBook with the following diff:
> I updated tests to make it more stable. Let me know if it's still
> failing on your setup.
Yes, the regression test passed successfully on my machine.
--- dropped.
-SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed
+-- dropped. remote_backend_pid will continue to return available as it fetch remote
+-- server backend pid from cached connections.
+SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed,
+CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid
Instead of checking whether remote_backend_pid is NOT NULL, how about
verifying that it actually matches a remote backend's PID? For example:
remote_backend_pid = ANY(SELECT pid FROM pg_stat_activity WHERE backend_type = 'client backend' AND pid <>
pg_backend_pid())AS "remote_backend_pid = remote pg_stat_activity.pid"
-SELECT CASE WHEN closed IS NOT true THEN 1 ELSE 0 END
+SELECT server_name, CASE WHEN closed IS NOT true THEN 'false' ELSE 'true' END AS remote_conn_closed,
+CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid
Similarly, instead of checking if remote_backend_pid is NOT NULL,
how about verifying it against pg_stat_activity?
remote_backend_pid = (SELECT pid FROM pg_stat_activity WHERE application_name = 'fdw_conn_check')
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi Fujii, Please review the latest patch. > Could you add descriptions for postgres_fdw_get_connections()? Done. > how about verifying it against pg_stat_activity? Yes. This approach will make tests more reliable. Updated. On Fri, Feb 21, 2025 at 8:15 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/21 0:54, Sagar Shedge wrote: > > Hi Fujii, > > > >> Naming is always tricky, but remote_backend_pid feels a bit too long. > > Would remote_pid be sufficient? > > Point looks valid. I had another perspective is to align the naming > > convention to pg_backend_pid(). remote_pid is not helping to identify > > whether pid belongs to postgres backend or not. Does this make sense? > > Or I'm fine to go with concise name like `remote_pid` > > I initially thought "remote_pid" was sufficient since the postgres_fdw > connection clearly corresponds to a backend process. However, I'm fine > with keeping "remote_backend_pid" as the column name for now. If we find > a better name later, we can rename it. > > > >> How about: "PID of the remote backend handling the connection" instead? > > Updated in v2. > > Thanks for updating the patch! > > You still need to update the documentation. Could you add descriptions > for postgres_fdw_get_connections()? > > > >> Wouldn't it be better to return the result of PQbackendPID() instead of NULL > > even when the connection is closed, for debugging purposes? This way, > > users can see which remote backend previously handled the "closed" connection, > > which might be helpful for troubleshooting. > > Agree. Updated logic to return backend pid always except when pid is 0 > > which indicates no backend attached to session. Returning 0 found > > misleading. What's your thought on this? > > Your approach makes sense to me. > > > >> The postgres_fdw regression test failed on my MacBook with the following diff: > > I updated tests to make it more stable. Let me know if it's still > > failing on your setup. > > Yes, the regression test passed successfully on my machine. > > > --- dropped. > -SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed > +-- dropped. remote_backend_pid will continue to return available as it fetch remote > +-- server backend pid from cached connections. > +SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed, > +CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid > > Instead of checking whether remote_backend_pid is NOT NULL, how about > verifying that it actually matches a remote backend's PID? For example: > > remote_backend_pid = ANY(SELECT pid FROM pg_stat_activity WHERE backend_type = 'client backend' AND pid <> pg_backend_pid())AS "remote_backend_pid = remote pg_stat_activity.pid" > > > -SELECT CASE WHEN closed IS NOT true THEN 1 ELSE 0 END > +SELECT server_name, CASE WHEN closed IS NOT true THEN 'false' ELSE 'true' END AS remote_conn_closed, > +CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid > > Similarly, instead of checking if remote_backend_pid is NOT NULL, > how about verifying it against pg_stat_activity? > > remote_backend_pid = (SELECT pid FROM pg_stat_activity WHERE application_name = 'fdw_conn_check') > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/21 22:43, Sagar Shedge wrote: > Hi Fujii, > > Please review the latest patch. Thanks for updating the patch! >> Could you add descriptions for postgres_fdw_get_connections()? > Done. Thanks! But, I realize my previous comment may have been unclear. I was referring to updating doc/src/sgml/postgres-fdw.sgml, which contains the explanation of postgres_fdw_get_connections(). Could you update that? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Hi Fujii, > Could you update that? I apologize. Updated documentation. Please review and let me know if you have suggestions to improve wording. On Fri, Feb 21, 2025 at 7:56 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/21 22:43, Sagar Shedge wrote: > > Hi Fujii, > > > > Please review the latest patch. > > Thanks for updating the patch! > > > >> Could you add descriptions for postgres_fdw_get_connections()? > > Done. > > Thanks! But, I realize my previous comment may have been unclear. > I was referring to updating doc/src/sgml/postgres-fdw.sgml, > which contains the explanation of postgres_fdw_get_connections(). > Could you update that? > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, Pune. With Regards.
Attachment
Hi Team, Created patch as per recommendation in postgres wiki. -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/25 1:55, Sagar Shedge wrote:
> Hi Team,
>
> Created patch as per recommendation in postgres wiki.
Thanks for updating the patch!
+ Process ID of remote backend handling connection. If <literal>valid</literal>
+ is set to <literal>false</literal> (i.e., marked as invalid), this will be
+ process ID of remote backend which handled this connection. <literal>NULL</literal>
+ is returned if no backend assigned to this connection or connection status is bad.
I have one question; is there actually a case where remote_backend_pid
returns NULL in postgres_fdw_get_connections()? Based on the current patch,
if the connection object (PGconn) is NULL or its state isn’t CONNECTION_OK,
PQbackendPID() returns 0, so remote_backend_pid becomes NULL.
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?
If remote_backend_pid will never be NULL, wouldn't a simpler description
like the following be clearer and less confusing for users?
Otherwise, users might wonder why remote_backend_pid isn't NULL
even if the remote server-side connection is closed.
Process ID of the remote backend, on the foreign server, handling the connection.
Also, rather than setting NULL when PQbackendPID() returns 0, would it
make sense for postgres_fdw_get_connections() to just return the result
of PQbackendPID() as-is?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
> 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? Other states might also occur during (and only during) an asynchronous connection procedure. These indicate the current stage of the connection procedure and might be useful to provide feedback to the user for example. [1] - https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PQCONNECTSTARTPARAMS -- Sagar Dilip Shedge, SDE AWS
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
> 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. Make sense. I have created a patch according to the above suggestions. Please review. -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/28 15:10, Sagar Shedge wrote: >> 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. > > Make sense. I have created a patch according to the above suggestions. > Please review. Thanks for updating the patch! I made a few cosmetic adjustments and attached the revised version. Unless there are any objections, I plan to commit this. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Attachment
Hi Fujii, Thanks for updates. Looks good to me. We can proceed with latest potch. -- Sagar Dilip Shedge, SDE AWS
On 2025/02/28 21:23, Sagar Shedge wrote: > Hi Fujii, > > Thanks for updates. > Looks good to me. We can proceed with latest potch. Thanks for the review! I've pushed the patch. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION