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.fu...@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
v03_add_remote_backend_pid.patch
Description: Binary data
