Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

Attached is a new patchset with various changes. I created a dedicated
0002 patch to add tests for the already existing cancellation
functions, because that seemed useful for another thread where changes
to the cancellation protocol are being proposed[1].

[1]: https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi

On Wed, 6 Mar 2024 at 19:22, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Docs: one bogus "that that".

This was already fixed by my previous doc changes in v32, I guess that
email got crossed with this one

> Did we consider having PQcancelConn() instead be called
> PQcancelCreate()?

Done

> "Asynchronously cancel a query on the given connection. This requires
> polling the returned PGcancelConn to actually complete the cancellation
> of the query." but this is no longer a good description of what this
> function does.

Fixed

> Anyway, maybe there are reasons for this; but in any case we
> should set ->cancelRequest in all cases, not only after the first tests
> for errors.

Done

> I think the extra PGconn inside pg_cancel_conn is useless; it would be
> simpler to typedef PGcancelConn to PGconn in fe-cancel.c, and remove the
> indirection through the extra struct. You're actually dereferencing the
> object in two ways in the new code, both by casting the outer object
> straight to PGconn (taking advantage that the struct member is first in
> the struct), and by using PGcancelConn->conn. This seems pointless. I
> mean, if we're going to cast to "PGconn *" in some places anyway, then
> we may as well access all members directly. Perhaps, if you want, you
> could add asserts that ->cancelRequest is set true in all the
> fe-cancel.c functions. Anyway, we'd still have compiler support to tell
> you that you're passing the wrong struct to the function. (I didn't
> actually try to change the code this way, so I might be wrong.)

Turns out you were wrong about the compiler support to tell us we're
passing the wrong struct: When both the PGconn and PGcancelConn
typedefs refer to the same struct, the compiler allows passing PGconn
to PGcancelConn functions and vice versa without complaining. This
seems enough reason for me to keep indirection through the extra
struct.

So instead of adding the proposed typed this typedef I chose to add a
comment to pg_cancel_conn explaining its purpose, as well as not
casting PGcancelConn to PGconn but always accessing the conn field for
consistency.

> We could move the definition of struct pg_cancel to fe-cancel.c. Nobody
> outside that needs to know that definition anyway.

Done in 0003