ocaml-caqti icon indicating copy to clipboard operation
ocaml-caqti copied to clipboard

Add the Miou implementation for caqti

Open dinosaure opened this issue 2 months ago • 5 comments

This is a draft which requires the upstream version of Miou which is not yet released but, as far as I can, it works :+1:. The TLS implementation is not yet done even if a tls_miou implementation exists.

dinosaure avatar Apr 23 '24 22:04 dinosaure

Thanks for taking the initiative, miou support is welcome.

Thanks, it still is a draft. I try to improve Miou and, at the same time, provide multiple implementations of some libraries. I will set this PR where:

  1. Miou will be ready (and released)
  2. and when you agree with this PR

Running the test case, the binding-based drivers seems to work, but there are issues with pgx. I suspect a resource leak.

Is it possible to have a reproducible case to check if I missed something?

dinosaure avatar Apr 25 '24 14:04 dinosaure

Sounds good. I'll return with some details about how to run the testsuite to exercise the pgx driver, which I think is the case which will need debugging. I had some issue myself to run the relevant test in isolation, which I why I was vague about where the nature of the supposed resource leak.

I get the impression from the Miou docs that Unikernels is important target, in which case TLS support is highly desirable, as well [Added: ...eventually; it may be easiest to debug the rest first]. That was also one part which needed debugging for resource leaks in the eio driver. In particular the "connect" test was added to find leaks after a connect-disconnect, where I used ulimit -n 2048 to force the failure and avoid trashing. There are other stress tests which can expose issues during a session.

paurkedal avatar Apr 25 '24 20:04 paurkedal

I managed to reproduce the memory contention issue from yesterday as follows:

> ulimit -n 2048 -v 2097152
> dune exec testsuite/main_miou_unix.exe -- test -u pgx://@%2fdummy-socket-path
Testing `test_sql_miou_unix'.
This run has ID `LKAMVSGA'.

> [FAIL]        pgx          0   post_connect.
  [FAIL]        pgx          1   expand.
  [FAIL]        pgx          2   expr.
  [FAIL]        pgx          3   enum.
  [FAIL]        pgx          4   table.
  [FAIL]        pgx          5   tuples.
  [FAIL]        pgx          6   affected_count.
  [FAIL]        pgx          7   stream.
  [FAIL]        pgx          8   stream_both_ways.
  [FAIL]        pgx          9   stream_binary.
  [FAIL]        pgx         10   harness.
  [FAIL]        pgx         11   NOT NULL constraint violation.
  [FAIL]        pgx         12   UNIQUE constraint violation.
  [FAIL]        pgx         13   FOREIGN KEY constraint violation.
  [FAIL]        pgx         14   CHECK constraint violation.
  [FAIL]        pgx         15   parallel.
  ...           pgx         16   nonlinear.Aborted (core dumped)

The problem was that Caqti_platform.Pool was not designed to tolerate exceptions from the resource allocator. The issue was exposed by the invalid_arg call in Miou implementation of connect_tcp, which triggered an fast loop in Pool.acquire. dc7dc9d529f5f0faf3d19050dae28af2f3f3e23a fixes the pool implementation. I accidentally committed it to your branch, but reset it, hopefully before you pulled anything.

After fixing the URL, the testsuite goes through, but it takes an hour to complete, compared to less that a minute for other variants. Most time is spent in the stream_binary test, which make 65536 individual requests when using the pgx driver. My experience with the other variants is that we really need buffered IO to achieve good performance.

paurkedal avatar Apr 26 '24 15:04 paurkedal

After fixing the URL, the testsuite goes through, but it takes an hour to complete, compared to less that a minute for other variants. Most time is spent in the stream_binary test, which make 65536 individual requests when using the pgx driver. My experience with the other variants is that we really need buffered IO to achieve good performance.

As far as I understand, the Stream implementation corresponds to module Stream = Caqti_platform.Stream.Make (Fiber)? It will be nice to see how I can optimize the usage of Miou. Actually, the implementation is a bit dumb and did not take advantages of Miou. I updated this PR with your previous comments.

dinosaure avatar Apr 27 '24 11:04 dinosaure

The updates looks good, except I'll need the license headers as well before merging.

I re-tested with sockets, and that's quick, so the slow-down is likely due to the number of packets generated when the output stream is unbuffered. I don't think the stream implementation should be an issue, since for Miou map and bind are just applications. (The stream_binary test is expected to be slower for the pgx driver than for the postgresql driver, since it does not implement streaming, but a comparison with eio or lwt for this and other tests should be fair.)

The command link I'm using for testing assumes access to a postgresql database. I usually start it up on the system on my workstation (with a database $USER owned by $USER):

dune exec testsuite/main_miou_unix.exe -- -u "pgx:?user=$USER&host=%2fvar%2frun%2fpostgresql" # for socket
dune exec testsuite/main_miou_unix.exe -- -u "pgx://$USER:$PASSWORD@localhost:5432" # for TCP

Alternatively add the URL(s) to testsuite/uris.conf and run the whole testsuite withdune runtest.

But I also have a script to start up a postgresql server as a regular user, which I consider fixing up and committing to the repo.

paurkedal avatar Apr 28 '24 14:04 paurkedal