Horton

Thread

Commits

  1. postgres_fdw: Remove useless if-test in GetConnection().

  1. postgres_fdw: Useless if-test in GetConnection()

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2023-03-15T10:18:41Z

    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

  2. Re: postgres_fdw: Useless if-test in GetConnection()

    Richard Guo <guofenglinux@gmail.com> — 2023-03-15T10:40:00Z

    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

  3. Re: postgres_fdw: Useless if-test in GetConnection()

    Daniel Gustafsson <daniel@yesql.se> — 2023-03-15T10:58:33Z

    > 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

  4. Re: postgres_fdw: Useless if-test in GetConnection()

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2023-03-16T09:25:46Z

    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

  5. Re: postgres_fdw: Useless if-test in GetConnection()

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2023-03-16T09:28:41Z

    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

  6. Re: postgres_fdw: Useless if-test in GetConnection()

    Etsuro Fujita <etsuro.fujita@gmail.com> — 2023-03-17T09:28:20Z

    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