Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the Miou implementation for caqti #117

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dinosaure
Copy link

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

@paurkedal paurkedal self-assigned this Apr 25, 2024
Copy link
Owner

@paurkedal paurkedal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the initiative, miou support is welcome.

To get the trivia out of the way, I'll need license headers on all OCaml source files, so that they are recorded by Git along with your contribution.

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

caqti-miou/lib/caqti_miou.mli Outdated Show resolved Hide resolved
caqti-miou/lib/system.ml Outdated Show resolved Hide resolved
caqti-miou/lib/system.ml Outdated Show resolved Hide resolved
caqti-miou/lib/system.ml Outdated Show resolved Hide resolved
caqti-miou/lib/system.ml Outdated Show resolved Hide resolved
@dinosaure
Copy link
Author

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?

@paurkedal
Copy link
Owner

paurkedal commented Apr 25, 2024

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
Copy link
Owner

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. dc7dc9d 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.

@dinosaure
Copy link
Author

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.

@paurkedal
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants