feat: use gen_random_uuid() for CockroachDB INSERTs

[alnr]

Using the gen_random_uuid() builtin can significantly speed up inserts because the primary key uniqueness constraint check is elided.

I've also fixed up code where there was a prepared statement being created for just one query, which is super inefficient.

I'm kinda having trouble running the tests locally, so I'd appreciate a workflow approval too. @aeneasr @sio4 👍

[aeneasr]

Approval given :)

You can run the tests locally using ./test.sh which executes tests against all DBs, or using:

go build -v -tags sqlite -o tsoda ./soda

SODA_DIALECT=cockroach

./tsoda drop -e $SODA_DIALECT -c ./database.yml -p ./testdata/migrations
./tsoda create -e $SODA_DIALECT -c ./database.yml -p ./testdata/migrations
./tsoda migrate -e $SODA_DIALECT -c ./database.yml -p ./testdata/migrations

go test -cover -race -tags sqlite -count=1 $args ./...

[aeneasr]

This looks very nice already! Do we have a test showing that the ID is generated correctly for Cockroach? I do believe that such tests exist, but it would be good to have proof that we don't introduce regressions :)

[aeneasr]

Good point, it makes no sense to use prepared statements if they're not cached.

[alnr]

• A couple of tests were not idempotent. I think I've fixed them all.
• Fixed missing Close on sql.Rows
• Added more explicit checks about UUID generation to test

ready for review :)

[sio4]

Hi @alnr,
Thank you for your PR.

Before reviewing the main topic of this PR, please remove changes regarding testing configurations (database.yml and docker-compose.yml). I don't think they are parts of the topic of this PR. Changing the supported database version should have more consideration around other conditions and may not be a part of this feature PR.

Please keep PR as simple as possible with a single point of interest.

[sio4]

LGTM!

[sio4]

Hi @alnr,

I am just wondering. What could be the cons and pros of using gen_random_uuid() for generating a random UUID ID on the server side?

Personally, I prefer to use application-side actions (spending the application server's CPU) rather than offloading CPU tasks to the database side since the application is more flexible to spread out (scaling) than controlling database load unless the action should be taken on the database side for integrity or similar things. Also for the main topic of this PR, it is easier to maintain the code if we use the same generic method in the generic dialect.

Could you please explain your idea and/or considerations?

significantly speed up inserts because the primary key uniqueness constraint check is elided

Do you have any data regarding this? Having the real or test data here would be great!

[alnr]

I am just wondering. What could be the cons and pros of using gen_random_uuid() for generating a random UUID ID on the server side?

Pros:

• primary key uniqueness constraint check will be elided on insert. This makes a huge difference in CockroachDB multiregion databases. If you don't use gen_random_uuid(), the uniqueness constraint will be verified prior to insert on all regional shards, which requires contacting them. Across continents, that's multiple hundred milliseconds. I've confirmed this with a US-EU-APAC cluster on a hosted enterprise Cockroach cluster.

Cons:

• can't think of any. Generating a UUID is free. Doesn't matter if the app or the DB does it.

Personally, I prefer to use application-side actions (spending the application server's CPU) rather than offloading CPU tasks to the database side since the application is more flexible to spread out (scaling) than controlling database load unless the action should be taken on the database side for integrity or similar things. Also for the main topic of this PR, it is easier to maintain the code if we use the same generic method in the generic dialect.

After this change, we burn a lot fewer CPU cycles anyway, because the DB does not have to verify the uniqueness constraint on the primary key. Checking that constraint takes approximately infinity times more time than generating a UUID.

[alnr]

Not sure what's going on with that failing test 🤷

[sio4]

Not sure what's going on with that failing test shrug

Don't worry. The short test on the Windows machine is failing many times, and I don't think it is related to your PR. We need to make it better but actually, the Windows environment is too hard. :-)

For the main topic, I quickly researched the Cockroach's resources and I found the followings:

• https://www.cockroachlabs.com/blog/regional-by-row/#enforcing-uniqueness-with-implicit-partitioning
• https://www.cockroachlabs.com/docs/stable/schema-design-table.html#primary-key-examples
• opt: remove uniqueness checks for gen_random_uuid() cockroachdb/cockroach#61132
• opt: remove uniqueness checks for gen_random_uuid() cockroachdb/docs#10118

It seems like we can take another approach for this, not for right now but for later:

• improve the generic routine can allow empty UUID id value (with a new option)
• improve fizz to create a better CREATE statement with the default gen_random_uuid() (or manually?)

I think this approach will make Pop simple without losing the database side performance. Any ideas or suggestions will help us greatly!

Checking that constraint takes approximately infinity times more time than generating a UUID.

Yeah, could be in a global cluster across the universe 😆

[alnr]

Thanks for merging!

Honestly I would keep the code as-is for now. Not everyone uses fizz to create schemas. Regarding the empty UUID value: I don't think allowing null UUIDs makes sense for the ID (primary key) column. It makes a lot of sense for other columns, of course, but that's not where this optimization is most useful anyway.

[sio4]

Honestly I would keep the code as-is for now. Not everyone uses fizz to create schemas. Regarding the empty UUID value: I don't think allowing null UUIDs makes sense for the ID (primary key) column.

Yes, for now. and...
Maybe my explanation is not enough. I don't mean ID can be null UUID. Currently, we remove an integer ID field from the model when executing Create() because we assume auto-increment is enabled on the database side. With the same concept, if we know (with option or anything) the database table was created with a UUID key with DEFAULT like below (example taken from the cockroach's document), we can just remove the ID field so the database will handle it.

CREATE TABLE movr.max_schema.vehicles (
      id UUID DEFAULT gen_random_uuid() PRIMARY KEY,
      type movr.max_schema.vtype,
      creation_time TIMESTAMPTZ,
      available BOOL,
      last_location STRING
  );

The function or method to create an auto key can be different by database engines, but we can make the Create() routine the same.

There are also related issues (opposite actually) for the integer ID that cannot be preserved when if the user sets ID explicitly and I think those issues can be considered together with it. (#382, #586)