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

Implement the Postgres integration #175

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

domMayhew
Copy link
Contributor

@domMayhew domMayhew commented Jun 19, 2024

In this PR

  1. Implement Kupo.App.Database.Postgres, except for the CopyDatabase function.
  2. Convert newDBPool to withDBPool to ensure that resources are cleaned up appropriately.
  3. Hide the implementation of creating a DB for testing within the database modules through withTestDatabaseLocation. This function is responsible for instantiating a test DB and destroying it after the tests have run, and provides the DatabaseLocation of the freshly instantiated DB.
    1. In the case of SQLite, there is no instantiation/destruction, simply an InMemory Nothing location is passed.
    2. In the case of Postgres, a DB with the specified name is created and dropped after the action has been run. More thoughts on this below.
  4. Switch Kupo.Data.Database.patternToSql to take a function that escapes a binary literal, as the formats differ for SQLite and PostgreSQL.

Problems

  1. The current implementation of withTestDatabaseLocation for Postgres is very flakey. Sometimes when the DB is dropped, an error occurs because another user (the Kupo process spun up by the test) is still using the DB. This leaves the test DB in tact which causes an error to be thrown on the next test run. I'm not sure why the test connection is not closed by the time the DB is dropped. My best guess is that it's because libpq sometimes throws an error that "another command is already in progress", which seems to be ongoing issue with async usage of postgresql-simple.
    1. Additionally, I think I've ended up with zombie threads if the test gets interrupted partway through, but I'm not 100% sure.
  2. The SQLite tests are failing due to the query plan not being as expected. I'm not sure what changes I have made have caused that.
  3. The Makefile commands still do not work. As far as I can tell, there is no -iog-full flavor in github:CardanoSolutions/devx. The README does not mention this flavor and I verified with builtins.attrNames (builtins.getFlake github:CardanoSolutions/devx).outputs.devShells.x86_64-linux.

Work Remaining

  1. I have left a number of TODOs in areas that I think especially need review.
  2. Fix the failing SQLite tests and flakey PG test.
  3. Update the tests in DatabaseSpec to work for both modules. Currently, those tests are "pending" in the PG version.
  4. Update relevant documentation.
  5. Update CI/CD.
  6. Review the performance of the PG implementation.
    1. Are there alternatives to the SQLite PRAGMAs that are necessary?
    2. Are different concurrency parameters required (connection pool timeouts and max connections)?

Notes for future contributors

  1. Be aware that in postgresql-simple, parameters with data types like Text, String, and I think even ByteString are wrapped in single quotes when inserted into the statement. Identifiers like column names or index names need to either be wrapped in the Identifier data type, which results in the parameter being wrapped in single quotes, or they need to be wrapped in Plain which does not escape or wrap the parameter at all. I have chosen to use Identifier, but this means that the names are case-sensitive.

Write an initial implementation of PG DB operations that compile.
The goal here is to port the existing SQLite operations into the
correct types/DSL of `postgresql-simple` without worrying about
whether or not there are SQL incompatibilities with Postgres.
Additionally, provide a function to `Data/Database` to convert a ByteString into a bytea/BLOB literal
depending on the calling context.
- Switch to `withDBPool` to ensure proper cleanup of resoureces.
- Use `Identifier` wrapper for table/column/index names to cause them to be wrapped in double quotes, but use `String`/`Text` for querying index names because the identifier is retrieved as a string (and therefore needs to be compared using single quotes).
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.

1 participant