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
Attachments:
remove-useless-if-test.patchapplication/octet-stream; name=remove-useless-if-test.patchDownload+1-2
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
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