Thread: postgres_fdw: Useless if-test in GetConnection()
Hi, While working on something else, I noticed that the “if (entry->conn == NULL)” test after doing disconnect_pg_server() when re-establishing a given connection in GetConnection() is pointless, because the former function ensures that entry->conn is NULL. So I removed the if-test. Attached is a patch for that. I think we could instead add an assertion, but I did not, because we already have it in make_new_connection(). This would be harmless, so I am planning to apply the patch to HEAD only. Best regards, Etsuro Fujita
Attachment
On Wed, Mar 15, 2023 at 6:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
While working on something else, I noticed that the “if (entry->conn
== NULL)” test after doing disconnect_pg_server() when re-establishing
a given connection in GetConnection() is pointless, because the former
function ensures that entry->conn is NULL. So I removed the if-test.
Attached is a patch for that. I think we could instead add an
assertion, but I did not, because we already have it in
make_new_connection().
+1. Good catch.
Thanks
Richard
Thanks
Richard
> On 15 Mar 2023, at 11:18, Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > While working on something else, I noticed that the “if (entry->conn > == NULL)” test after doing disconnect_pg_server() when re-establishing > a given connection in GetConnection() is pointless, because the former > function ensures that entry->conn is NULL. So I removed the if-test. > Attached is a patch for that. LGTM, nice catch. > I think we could instead add an assertion, but I did not, because we already > have it in make_new_connection(). Agreed, the assertion in make_new_connection is enough (and is needed there). -- Daniel Gustafsson
On Wed, Mar 15, 2023 at 7:40 PM Richard Guo <guofenglinux@gmail.com> wrote: > On Wed, Mar 15, 2023 at 6:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: >> While working on something else, I noticed that the “if (entry->conn >> == NULL)” test after doing disconnect_pg_server() when re-establishing >> a given connection in GetConnection() is pointless, because the former >> function ensures that entry->conn is NULL. So I removed the if-test. >> Attached is a patch for that. I think we could instead add an >> assertion, but I did not, because we already have it in >> make_new_connection(). > +1. Good catch. Cool! Thanks for looking! Best regards, Etsuro Fujita
On Wed, Mar 15, 2023 at 7:58 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > On 15 Mar 2023, at 11:18, Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > While working on something else, I noticed that the “if (entry->conn > > == NULL)” test after doing disconnect_pg_server() when re-establishing > > a given connection in GetConnection() is pointless, because the former > > function ensures that entry->conn is NULL. So I removed the if-test. > > Attached is a patch for that. > > LGTM, nice catch. > > > I think we could instead add an assertion, but I did not, because we already > > have it in make_new_connection(). > > Agreed, the assertion in make_new_connection is enough (and is needed there). Great! Thanks for looking! Best regards, Etsuro Fujita
On Wed, Mar 15, 2023 at 7:18 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > This would be harmless, so I am planning to apply the patch to HEAD only. I forgot to mention that this was added in v14. Done that way. Best regards, Etsuro Fujita