Parametrisation by the Mutex module + remove use for condition

[craff]

I think (might be wrong) that the condition and connection state `Used was useless and that protecting all call by a mutex seems enough to me. This PR removes the condition and replace the connection state by a boolean. It also offers a functor parametrized by the mutex module.

Remark : if you can avoid the fact that I had to copy the class type connection both in .ml (without comments) and in .mli,
I would be happy. Moving all type defs in sig.ml is not so nice for the generated documentation.

Remark : In asynch mode, it could have been useful to have a Used state, but in the code before this PR send_query or send_query_prepare got the mode back to `Free. Moreover, there is no way to know when the connection is free, that is when the user does not need the connection result anymore.

[craff]

I tested with a current ongoing project ... seems to work, but more test would be nice.

As a comment, because I protext connection use my own implementation or connectionPool, but
because bug are always possible, I use this kind of code:

module Check = struct
  type t = bool Atomic.t
  let create () = Atomic.make false
  let lock m =
    if not (Atomic.compare_and_set m false true) then
      failwith "Concurrent use of a Postgres connection (at lock)"
  let unlock m =
    if not (Atomic.compare_and_set m true false) then
      failwith "Concurrent use of a Postgres connection (at unlock)"
end

module Postgresql = struct
  include Postgresql

  include Connection(Check)
end

I added this in the documentation.

[mmottl]

Unfortunately, people may still need cancellation of synchronous queries. Keeping the mutex locked during the operation would prevent cancellation. That's why I don't think we should be merging this, it would probably break some people's code.

I'm not sure what a good solution would look like, but it might be a good idea to use the more secure, modern API: https://www.postgresql.org/docs/current/libpq-cancel.html

I believe we may need to protect the cancel object with a separate mutex so only one cancellation can happen at a time. I believe it may be safe to otherwise access the connection and the cancel object simultaneously. These changes would be a bit more involved, and I unfortunately don't have time to help with this right now.

[craff]

Markus Mottl ***@***.***> writes:

* mmottl left a comment (mmottl/postgresql-ocaml#55) Unfortunately, people may still need cancellation of synchronous queries. Keeping the mutex locked during the operation would prevent cancellation. That's why I don't think we should be merging this, it would probably break some people's code. I'm not sure what a good solution would look like, but it might be a good idea to use the more secure, modern API: https://www.postgresql.org/docs/current/libpq-cancel.html I believe we may need to protect the cancel object with a separate mutex so only one cancellation can happen at a time. I believe it may be safe to otherwise access the connection and the cancel object simultaneously. These changes would be a bit more involved, and I unfortunately don't have time to help with this right now.

Thanks for the pointer, I will look at the modern API and try to improve my PR, keep it open for now please.

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Christophe Raffalli web: https://raffalli.eu

[craff]

Unfortunately, people may still need cancellation of synchronous queries. Keeping the mutex locked during the operation would prevent cancellation. That's why I don't think we should be merging this, it would probably break some people's code.

I fixed this but by using a separate mutex for cancel.

The condition in the current code is overkill and does not bring enough benefit. Indeed,
we may call cancel when the PQ command finished but just before acquiring the mutex that protect the condition.
So wwe have to check anyway cancel for errors.

From the documentation, PQcancel should be used anytime and checked for error, because it can fail in many case.

A separate mutex seems enough for me and simplifies the parametrized version.

I also removed the finished boolean I added. In the rare case the connection is finished, checking the boolean + is_null is overkill. I also consider that this check_null is too costly and we should let an exception propagate from the PQ call if the connection was finished. This happens rarely and the usage of libpq is not to check if the connection is alive before every single command ? I might do this in a separate PR.

I'm not sure what a good solution would look like, but it might be a good idea to use the more secure, modern API: https://www.postgresql.org/docs/current/libpq-cancel.html

Unfortunatly I use postgresql 15 (the default under debian 12) So I will do a PR for this when I switch to trixie.

[craff]

I also removed the finished boolean I added. In the rare case the connection is finished, checking the boolean + is_null is overkill. I also consider that this check_null is too costly and we should let an exception propagate from the PQ call if the connection was finished. This happens rarely and the usage of libpq is not to check if the connection is alive before every single command ? I might do this in a separate PR.

I misunderstood that code at this point. check_null is mandatory. Could be removed if we do not free the structure at finish, but only when the GC collect the connection ? Anyway this is a completely different subject and may be should be discussed in a separate issue.

[craff]

A last comment (sorry, they are too many) that summarises my work:

• PQ command (except cancel and finish) are protected by a mutex
• cancel is protected by a separate mutex
• finish acquires both mutex and set a boolean finished to true, avoiding a C-call for check is the connection is still allocated.

There seems to be a visible speed gain on a full application (like 1-3%) by removing the C call to check null. This needs to be confirmed.