-
Notifications
You must be signed in to change notification settings - Fork 110
Introduce option to setup transaction before executing queries #3471
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
base: main
Are you sure you want to change the base?
Conversation
Introduces a setup query config to have code executed transactionally with the query itself. Also, a way to define the setup in one place, and reuse it. Also, fix some issues with transacitonal code. This gets it working for embedded and jdbc-in-process
I also added an ExternalSingleServerConfig to help myself debug, and figured it was worth keeping.
...rc/main/java/com/apple/foundationdb/relational/server/jdbc/v1/TransactionRequestHandler.java
Outdated
Show resolved
Hide resolved
...onal-jdbc/src/main/java/com/apple/foundationdb/relational/jdbc/JDBCRelationalConnection.java
Outdated
Show resolved
Hide resolved
...ain/java/com/apple/foundationdb/relational/yamltests/configs/ExternalSingleServerConfig.java
Outdated
Show resolved
Hide resolved
create table t1(id bigint, col1 bigint, primary key(id)) | ||
create table table_t1(id bigint, col1 bigint, primary key(id)) | ||
--- | ||
transaction_setups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into using yaml's repeated nodes feature, but since each test_block is a separate "document" you can't really do that, so instead I'm adding our own reference section.
create table table_t1(id bigint, col1 bigint, primary key(id)) | ||
--- | ||
transaction_setups: | ||
t1_less_than_50: create temporary function t1() on commit drop function AS SELECT * FROM table_t1 where id < 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, it wouldn't be hard to change this to allow an array of strings if there's a bunch of setup steps
Conflicted with the new Debugger query config
maxRows:;initialVersionAtLeast:;initialVersionLessThan:;;mode:;repetition:;seed:;setup:;transaction_setups:; | ||
setupReference:;check_cache:;connection_lifecycle:;steps:;preset:;statement_type:;!r;!in;!a;" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem pretty incosistent in general about whether things should be snake_case
or camelCase
The bug was fixed in 4.4.7.0 so this no longer needs to be !current_version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember that you introduced some unit tests to support the supported_version
functionality. Will it be good to have some like those that could check for disallowed yaml layouts here, like having setups before query and key collison.
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryCommand.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryExecutor.java
Show resolved
Hide resolved
# The map is basically from a name (e.g. t1_less_than_50) to a statement to run at the start of every query. | ||
# They're referenced in the query using `setupReference`. Right now it only supports a single statement as a string, but | ||
# it could be expanded to support an array in the future. | ||
transaction_setups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like multiple transaction_setups
are allowed, so they can be spread all over the file. However, since all of them are processed in parsing
, the lifecycle of all the kv pairs is entire runtime. I think it will be good to disallow that or have an example citing this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to work as expected; I added some tests that cover this, and show that only test_block
s after the transaction_setup
can refer to it. It doesn't make a lot of sense if you have just tests, but if you have some tests, some setup, and then some transaction_setups it may make more sense.
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/SQLFunction.java
Outdated
Show resolved
Hide resolved
Some of these test checks that were suggested in the PR, but not yet implemented
setup could be used for inserts, but it may also be used for installing schema via metadata. Everything except test blocks are executed when they are hit, so this seems logical
It does do what you would expect in terms of registering
Other operations are hard to reason about, especially with parallelism, so don't allow it, at least for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There is just a minor fix required in one of the tests.
- setupReference: t1_less_than_50 | ||
- result: [{id: 30, col1: 40}] | ||
- query: select * from t1 where id > 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of commands here is not in line with the filename!
This adds the ability to specific a transaction setup to yamsql.
More specifically for an individual query you can have:
setup:
which will be executed at the start of every transaction for that queryTo avoid writing the same setup repeatedly, it also adds a new block:
transaction_setups:
which allows you to create references for the setups. Inside the query, it can now be referenced via the key, usingsetupReference:
.This can be useful if you want to test with temporary functions, but still have all the benefits of randomized, and forced-continuations.
This currently only supports a single query in the setup, but it wouldn't be hard to extend it to take an array.