arenadotio/pgx

A pure OCaml PostgreSQL client library

pgx's Issues

Support TLS for Pgx_lwt_unix

We added TLS support to Pgx_async using Conduit_async, but I couldn't find an STARTTLS upgrade interface for Conduit_lwt. We're open to PR's that implement this for Pgx_lwt_unix (and potentially Pgx_lwt_mirage).

We probably want to do this using Conduit_lwt or Tls_lwt.

See https://github.com/arenadotio/pgx/pull/108/files#diff-82209080f44391493efba0e6d9fab06647d561bdda34ea5099a23cd11fbe2ddc for an example of how we did this for Pgx_async. You should be able to implement this without needing to change any of the core Pgx code (although let us know if that's necessary).

Auto-upload test coverage reports

It looks like we should be able to do this with Coveralls, using the same setup bisect_ppx_uses:

https://github.com/aantron/bisect_ppx/blob/master/src/util/travis_ci_test.sh#L114

Add Pgx_value_ptime with ptime converters

Pgx_value_core has converters for Core.Date.t and Core.Time.t, but not everyone uses Core. We should add a new Pgx_value_ptime package with converters for Ptime.date and Ptime.time (and any other types that make sense to have converters).

Remove Pgx_async_test from the public API

Pgx_async_test is a global top-level module which we don't expect users to use at all. We should find a way to expose this only in tests and not export it at all.

If that doesn't work, another option would be wrapping the module and exposing it as Pgx_async.Test

Get the inline tests working in jbuilder

Right now our inline tests rely on an internal OMake build. It would be nice if they worked in the public repo too, using just jbuilder.

I haven't been able to make this work, but this seems like it should work:

https://discuss.ocaml.org/t/building-a-test-runner-with-ppx-inline-test/1245/3

Build in CI with `-p` argument

Doing dune runtest -p pgx,pgx_async,pgx_value_core,etc. will ensure that each library's deps are listed correctly so we can avoid annoyances like #90 at release time.

Support connection service file

How is the status regarding the use of a pg_service.conf file to establish a database connection?

Is there anything planned or in development, or not at all?

unix_domain_socket_dir

Looks like it's /var/run/postgresql for async and /tmp for everything else.

Use a serialization library

Right now we serialize and deserialize very, very manually (reading and writing one byte at a time). We should see if we can replace that code with something like cstruct. This should make the code more readable, and also make it more efficient.

(I also looked into bitstring and faraday, but they seem lower-level and I don't know if we want that? Faraday is probably more efficient..)

Postgres text protocol doesn't support string escaping

There's something wrong in the escaping of parameter binding.

This is reproducible in two different PostgreSQL setups with 9.1 as well as 10.5.

This issue appears when using characters outside the ASCII range, such as the following 11 letter string:

test-ä-test

Assume this string is already inserted into the database through other means, in a table test with column a:

create table test (a text);
insert into test values ('test-ä-test');

Then the following query returns 1 row in the psql tool:

select * from test where a = 'test-ä-test'

In PGX, this works only when the string is given directly in the query:

Pgx.exec ... "select * from test where a = 'test-ä-test'"
(* -> 1 row *)

However, when providing that string via parameter binding, the query returns an empty result, i.e. the comparison fails:

Pgx.exec ... ~params:[Pgx.Value.of_string "test-ä-test"] "select * from test where a = $1"
(* -> 0 rows! *)

Another way to see this is via Postgresql's length() function:

select length('test-ä-test'), length($1)

The result is:

11, 18

So the inline string was understood correctly, while the bound parameter seems to be something different. I assume that 18 is simply the length of the octal escaped UTF-8 string:

test-\303\244-test

Moreover, a similar bug exists when loading a PostgreSQL result. When the database actually does contain the string test-\303\244-test (i.e. a pure ASCII string with backslash characters), then PGX returns instead test-ä-test.

So if an application uses only PGX to read and write data, the users won't notice that problem until they access the database with a different tool (e.g. the psql command line tool).

It seems that PGX performs some escaping in the raw protocol over the wire that is only meant to be applied in the layer on top of that, i.e. when parsing escaped strings E'...' in SQL queries from the user. I can't find any escaping rules in the specification of PostgreSQL's Frontend/Backend protocol:

https://www.postgresql.org/docs/10/static/protocol.html

(Also, such rules wouldn't make a lot of sense, as it is a binary protocol anyway and strings are NUL-terminated.)

Nested Transactions

We had some work here #98, but it needs more motivation. Creating this issue to track any more work we do along these lines.

Upload test results to CircleCI

We'll have a CircleCI build once #4 lands, but it would be nice if we uploaded the test report so it could show nicer results.

OUnit doesn't seem to support this natively, but we could be able to wrap it with some sort of runner that collects the results and uploads them as JSON or something.

See info here:

https://circleci.com/docs/2.0/collect-test-data/

Support SASL and SCRAM-SHA-256

SASL has been around for a long time, but it's now become the default in common postgresql 14 distributions, and we don't support it.
#122 Turned a hang into a clear error, but it sucks that pgx won't work out of the box going forward until we fix this. It puts extra requirements on the server config to have to switch back to MD5, and it might conflict with user security requirements.

Here is a reference implementation of how to do the password handshake: https://github.com/porsager/postgres/blob/master/src/connection.js#L659

I think we could bring in nocrypto or hacl* for this, but I'm not sure how involved it is. We probably would need to use C bindings, but we wouldn't want to add a dependency to the OS for this.
pbkdf: https://github.com/abeaumont/ocaml-pbkdf
sha256:
https://ocaml.org/p/hacl-star/0.4.1/doc/Hacl_star/Hacl/index.html
https://docs.mirage.io/nocrypto/Nocrypto/Hash/SHA256/index.html

Use BINARY mode for results

Hello,

I read in the documentation of postgresql that results of query can be sent to the client in BINARY, I guess it should
improve efficiency and could even be simpler (the coding and decoding with string are often catchy).

So it would be nice if pgx whas using BINARY when possible instead of STRING ?

Connection without password fails

I'm trying to connect with a user that has no password (ie password set to null) and it raises an End_of_file exception.

I set verbose to 2 to get a little more detail on this and obtained the following logs:

FATAL: 28P01: password authentication failed for user "nathan"
R: auth_failed
L: 328
F: auth.c
V: FATAL

From a quick look at the code and the postgres documentation it looks to me like it may attempt to perform password auth when it shouldn't.

Do you have any idea what might be going wrong here ?

I'm happy to provide more logs/info to help figuring this out as well as a patch if it is indeed a bug.

Module name clash

It looks like the (wrapped false) setting in pgx/src/dune is causing me some problem when using Pgx in a local project.

Some environment information:

• esy-0.6.4
• dune-2.5.1
• ocaml-4.09.1

Error message:

$ esy dune build @all
Entering directory '/home/richard/code/project'
File "_none_", line 1:
Error: Files /home/richard/.esy/3__________________________________________________________________/i/ocaml-4.9.1000-ffe51dac/lib/ocaml/compiler-libs/ocamlcommon.cmxa
       and /home/richard/.esy/3__________________________________________________________________/i/opam__s__pgx-opam__c__1.0-1d851550/lib/pgx/pgx.cmxa
       both define a module named Types

Diagnosis:

$ esy ls-modules | grep -A4 -B9 Types
info ls-modules 0.6.4 (using esy.json) found project at ~/code/project/
├── @opam/pgx@opam:1.0 [built]
│   └── pgx
│       ├── Pgx
│       ├── Io_intf
│       ├── Pgx_intf
│       ├── Isolation
│       ├── Access
│       ├── Result_desc
│       ├── Row_desc
│       ├── Types
│       ├── Pgx_value_intf
│       ├── Pgx_value
│       ├── Error_response
│       └── Pgx_aux

It looks like some internal library in the compiler defines and exports a Types module, and so does Pgx. When I checked the Dune documentation, I saw that (wrapped false) is discouraged, but if you do want to use it, they recommend prefixing all the modules in your library manually.

Wrong clause number in LGPL special exception

The special exception part of the license file says:

By "a publicly distributed version of the Library", we mean either the unmodified Library as distributed by INRIA, or a modified version of the Library that is distributed under the conditions defined in clause 3 of the GNU Library General Public License.

It should be "clause 2", not "clause 3".

This is the same typo made in OCaml's license: ocaml/ocaml@2d26308

Server hangs up during authentication

When running the test suite of the current master (3f2c0fc) against a INET socket, each DB test hangs until the server disconnects with FATAL: canceling authentication due to timeout. This does not happen for UNIX domain socket. My test procedure is to start up a postgresql server like

docker run --rm -it -e POSTGRES_PASSWORD=Bw8D7YHufq11gcS4 -p 127.0.0.1:15432:5432 postgres

and run the test suite like

PGPORT=15432 PGHOST=localhost PGUSER=postgres PGDATABASE=postgres PGPASSWORD=Bw8D7YHufq11gcS4 dune runtest

The async tests report something like

ASSERT test db connection timed out after 2s
[exception] (monitor.ml.Error
  ( "Alcotest assertion failure\
   \n\027[31mFAIL\027[0m test db connection timed out after 2s\
   \n")
  ("Raised at file \"src/alcotest-engine/test.ml\", line 157, characters 20-48"
    "Called from file \"src/deferred1.ml\", line 17, characters 40-45"
    "Called from file \"src/job_queue.ml\", line 167, characters 6-47"))
            Raised at file "src/alcotest-engine/test.ml", line 157, characters 20-48
            Called from file "src/deferred1.ml", line 17, characters 40-45
            Called from file "src/job_queue.ml", line 167, characters 6-47

and the lwt tests report

[exception] End_of_file
            Raised by primitive operation at file "src/unix/lwt_unix.cppo.ml", line 690, characters 8-59
            Called from file "src/unix/lwt_unix.cppo.ml", line 571, characters 17-28

Add Date and Time Pgx.Values for Lwt backend

While prototyping a mirage backend for pgx (#67) I noticed that the Date/Time convertors are missing (they are present in the async backend).

For the lwt runtime maybe we can look at ptime (Calendar depends on unix so won't be usable on mirage) to provide similar support to create Pgx values?

In theory we could use the same functions as we do for the async runtime since they are using the date/time values from core_kernel which should be portable, but i'm not sure it'd be worth it to pull in core_kernel for the lwt runtime as i'm not sure if many people would use that combination.

Pgx_eof errors are hard to debug

If the server hangs up on us, we get an annoying Pgx_eof exception with no context:

https://github.com/arenadotio/pgx/blob/master/pgx_async/src/pgx_async.ml#L70

Maybe instead of throwing an exception here, we could return a Result type and then the caller can throw a more useful exception with context about what it was doing?

More guards around transactions

Something that is missing from the library is more robust handling of transactions.
A query on the same connection done outside of a transaction should wait for any transactions to finish. It might also require deadlock detection.

A first cut was attempted in #99, but it needs more work.

Mirage support

#57 and #58 are both attempts by @paurkedal to support Mirage. The point of this ticket is to:

1. Get Mirage support working
2. Ensure it stays working

For Mirage support, I'm wondering if this would be Pgx_unix, or another new Mirage-specific one?

Is there a way we can run a CircleCI build to ensure this works?

Auto-upload documentation

1. Add a make docs to run odoc
2. Upload the docs to S3 or GitHub pages
3. Auto run that, maybe CircleCI can do it when master changes land?
4. Add a link to the docs in the README

Remove our dependency on async_extra

Right now we use Async.Socket from async_extra, which means we have to build all of core. If we could replace that one module with something else, pgx_async would only need core_kernel, core_unix, async_kernel and async_unix.

Deprecated `Stream` module

Background

While working on a library that uses pgx as a client to interact with Postgresql, I have some issues building it with ocaml >= 4.14. Indeed, as described in this thread, the Stream module is deprecated in ocaml.4.14 and absent int ocaml.5.0.0.

Outcome

A good solution seems to move from Stream to Seq as it is the functional version.

Run dune-release lint in CI

Doing releases is annoying since things slowly get out of sync the longer we go without doing one. We should run dune-release lint in CI so we're always ready for a release.

Add previous contributors section to README.md

Include people who have contributed internally to this project like @jewei1997

Support TLS encryption

Support PG* environment variables for TLS configuration

psql has various environment variables that it uses for the default configuration: https://www.postgresql.org/docs/9.1/libpq-envars.html

We support add support for PGSSLMODE, PGREQUIRESSL, PGSSLCERT, PGSSLKEY, PGSSLCRL, and PGSSLROOTCERT where applicable.

Any ORM suite for OCaml , working with pgx together?

Hello, i am new to learn OCaml , i would like to know ,is there any ORM framework, like Java world Mybatis or whatever.
In OCaml world. what's the similar product can did this work?

OCaml + PGX + ??

Fix stringification of hstores containing backslash

When working on the Pgx.Value tests I found that converting an hstore containing a backslash to a string and back lost the backlash. Presumably either the stringification code needs to add an extra backslash or the parsing code needs to not remove it.

Also update test_pgx_value.ml with an hstore containing a backslash so we can go up to 100% coverage in that file.