Option to use `Typer.Strategy.BuiltinsOnly` to workaround bug in Skunk's oid codec?

benhutchison commented 4 months ago

When I tried out Dumbo for the first time (on a cockroachDB database that has been working with Skunk+Flyway for a while), I hit an error. Excess stack traces are boring so I trimmed for relevance.

skunk.exception.DecodeException:
🔥
🔥  DecodeException
🔥
🔥    Problem: Decoding error.
🔥     Detail: This query's decoder was unable to decode a row of data.
🔥
🔥  The statement under consideration was defined
🔥    at «skunk internal»:0
🔥
🔥    SELECT oid typid, typname, typarray, typrelid
🔥    FROM   pg_type
🔥    WHERE typnamespace IN (
🔥      SELECT oid
🔥      FROM   pg_namespace
🔥      WHERE nspname = ANY(current_schemas(true))
🔥    )
🔥
🔥  The row in question returned the following values (truncated to 15 chars).
🔥
🔥    typid     oid   ->  4294967108    ├── java.lang.NumberFormatException (see below)
🔥    typname   name  ->  pg_aggregate
🔥    typarray  oid   ->  0
🔥    typrelid  oid   ->  4294967108
🔥
🔥  The decoder threw the following exception:
🔥
🔥    java.lang.NumberFormatException: For input string: "4294967108"
🔥      java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:67)
🔥      java.base/java.lang.Integer.parseInt(Integer.java:668)
🔥      java.base/java.lang.Integer.parseInt(Integer.java:786)
🔥      scala.collection.StringOps$.toInt$extension(StringOps.scala:910)
🔥      skunk.util.Typer$ProtocolOps.$init$$$anonfun$4(Typer.scala:180)
🔥      skunk.Codec$.simple$$anonfun$2(Codec.scala:113)
...

Root cause is a bug in Skunk's oid Codec, which tries to fit an unsigned Int into an Int 😱

This potentially has a lot of surface area in Skunk's Typer and may not be a straightforward fix.

As a possibly easier workaround, would you consider some config option to instead use the simpler Typer.Strategy.BuiltinsOnly in Dumbo, which IIUC won't need to hit the problematic code?

rolang commented 4 months ago

hi @benhutchison, thanks for reporting. I haven't tested it with cockroachDB before. After I just tried there seem to be other issues / incompatibilities, like with history table types and locking seem to work differently in cockroachDB. May I know what cockroachDB version you were running it with? I guess different versions may behave differently.

Making the Typer.Strategy configurable or changing the default to BuiltinsOnly should be simple, but I think there are more things that would need changing. I'd need to run some more experiments.

benhutchison commented 4 months ago

Im using CockroachDB cloud, v23.2.4

On Sun, May 5, 2024 at 12:39 AM Roman Langolf @.***> wrote:

hi @benhutchison https://github.com/benhutchison, thanks for reporting. I haven't tested it with cockroachDB before. After I just tried there seem to be other issues / incompatibilities, like with history table types and locking seem to work differently in cockroachDB. May I know what cockroachDB version you were running it with? I guess different versions may behave differently.

Making the Typer.Strategy configurable or changing the default to BuiltinsOnly should be simple, but I think there are more things that would need changing. I'd need to run some more experiments.

— Reply to this email directly, view it on GitHub https://github.com/rolang/dumbo/issues/42#issuecomment-2094224464, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAXJZBUYIYEIUT64FCDG2LZATXJ7AVCNFSM6AAAAABHFODMOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGIZDINBWGQ . You are receiving this because you were mentioned.Message ID: @.***>

rolang commented 3 months ago

@benhutchison I updated the tests to run with CockroachDB (v23.2.4), together with some compatibility updates. It's released with 0.3.2. You can give it another try. I hardcoded the Typer.Strategy to BuiltinsOnly and don't think that it'll need changing, dumbo actually doesn't need to deal with user defined types.

There are some (maybe minor) differences in behavior:

I couldn't enforce Flyway to run migrations in a transaction for some test cases (like here or here). So Flyway partially failed and was not able to rollback applied changes, can't tell whether it's on purpose or not 🤷. Latest Flyway (as of now 10.12.0) also gives a warning about version support: warn [Database] Flyway upgrade recommended: CockroachDB 23.2 is newer than this version of Flyway and support has not been tested. The latest supported version of CockroachDB is 22.1.
Dumbo will always try to send off a migration as simple query with multiple statements that'll run within a transaction by default. Just something to keep in mind. CockroachDB may deal with it differently I guess. I added it as a note to the readme.
this example below that is mentioned in the docs is not going to work. CockroachDB doesn't support table-level locks with TABLE LOCK like Postgres. I considered this one as not really important for now and didn't put effort into checking on how to read locks in cockroachDB. I am also not sure whether this feature is needed or going to stay in dumbo.
```
// migration progress logs can be added optionally in case you'd like dumbo to provide some feedback on longer running queries
// it will perform requests to Postgres in given interval to check for queries that are causing the lock on migration history table
dumboWithResouces.withMigrationStateLogAfter[IO](5.seconds)(
/* use config as above */
)
```

rolang commented 3 months ago

Updated skunk to 1.0.0-M6 which includes the oid fix in release 0.3.3. Going to close the issue.

rolang / dumbo

Option to use `Typer.Strategy.BuiltinsOnly` to workaround bug in Skunk's oid codec? #42