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

Execute script #3837

Merged
merged 35 commits into from
Jun 17, 2022
Merged

Execute script #3837

merged 35 commits into from
Jun 17, 2022

Conversation

fantix
Copy link
Member

@fantix fantix commented May 6, 2022

Depends on #3814, edgedb/edgedb-python#310

  • Add tests
  • Return result of the last command in a script
  • Always use stmt_mode = "all" in EdgeConnection._compile() and check in server.compiler
  • Fix tests
  • Support constant extraction on multiple-statement queries
  • Drop remaining SimpleQuery

Sample scenarios of EdgeDB Execute generating Postgres messages using pseudo-code:

Single-command EdgeQL

EdgeDB Protocol:

Execute(
    insert Hero { name := 'Daisy Johnson' }
)

Generated Postgres messages:

Parse("INSERT INTO heros (name) VALUES ($1)")
Bind('Daisy Johnson')
Execute()
Sync()

On protocol-level, transaction control commands like START TRANSACTION, COMMIT, ROLLBACK [...] are allowed.

Multi-command EdgeQL

EdgeDB Protocol:

Execute(
    insert Hero { name := 'Daisy Johnson' };
    insert Hero { name := 'Phillip J. Coulson' };
)

Generated Postgres messages:

Parse("INSERT INTO heros (name) VALUES ($1)")
Bind('Daisy Johnson')
Execute()
Parse("INSERT INTO heros (name) VALUES ($1)")  # Maybe omitted if prepared statement rules apply
Bind('Phillip J. Coulson')
Execute()
Sync()

There is only one Sync per EdgeDB Execute, so it is either an atomic implicit transaction in Postgres, or a part of the outer existing transaction.

On protocol-level, transaction control commands are NOT allowed.

Flush messages may be added if SET GLOBAL is in the script, but that won't affect the atomicity. Only the values generated from the last command will be returned.

Migration Commands

START MIGRATION ..., COMMIT MIGRATION and ABORT MIGRATION are allowed in scripts, but only if all START MIGRATION ... have a matching COMMIT MIGRATION or ABORT MIGRATION. Implementation-wise, migration commands in scripts will never open or close a database transaction or savepoint, they are always a part of the outer implicit or explicit transaction.

In addition, there will be a new capability MIGRATION_TX, gating the ability of the migration commands to actually open or close a database transaction. The client bindings should always turn this capability off, so that the following practice is forbidden:

client.execute("START MIGRATION TO ...")
...
client.execute("COMMIT MIGRATION")

Instead, the client could either run migration in a script:

client.execute("""
    START MIGRATION TO ...;
    ...
    COMMIT MIGRATION;
""")

Or use an explicit transaction to wrap the separate migration commands:

for tx in client.transaction():
    with tx:
        tx.execute("START MIGRATION TO ...")
        ...
        tx.execute("COMMIT MIGRATION")
        # Or just use a script again:
        tx.execute("""
            START MIGRATION TO ...;
            ...
            COMMIT MIGRATION;
        """)

Client Bindings

Both query() and execute() will use the same Parse/Execute EdgeDB protocol messages, both accepting single-command and multi-command EdgeQL. The difference is, execute() will generate Parse/Execute messages with output_format=NULL (previously known as io_format), while query() will use output_format=BINARY (or JSON for the query_*_json() family).

Rejected Ideas

Transactional Script

EdgeDB Protocol:

Execute(
    start transaction;
    insert Hero { name := 'Daisy Johnson' };
    commit;
    start transaction;
    insert Hero { name := 'Phillip J. Coulson' };
    rollback;
)

Generated Postgres messages:

Parse("START TRANSACTION")
Bind()
Execute()
Parse("INSERT INTO heros (name) VALUES ($1)")
Bind('Daisy Johnson')
Execute()
Parse("COMMIT")
Bind()
Execute()
Sync()

# If we're in error state: (or always ROLLBACK after COMMIT to save round-trip time)
Parse("ROLLBACK")
Bind()
Execute()
Sync()

Parse("START TRANSACTION")
Bind()
Execute()
Parse("INSERT INTO heros (name) VALUES ($1)")  # Maybe omitted if prepared statement rules apply
Bind('Phillip J. Coulson')
Execute()
Sync()  # make sure the following ROLLBACK works, clearing `ignore_till_sync` pg state
Parse("ROLLBACK")
Bind()
Execute()
Sync()

Returning values is NOT allowed.

Illegal Transactional Script

Statements outside transactions are not allowed:

Execute(
    start transaction;
    insert Hero { name := 'Daisy Johnson' };
    commit;
    insert Hero { name := 'Phillip J. Coulson' };
)

Mismatching transaction boundaries are not allowed:

Execute(
    insert Hero { name := 'Daisy Johnson' };
    commit;
    start transaction;
    insert Hero { name := 'Phillip J. Coulson' };
    rollback;
)

@fantix fantix force-pushed the execute-script branch 4 times, most recently from 321ef01 to 310ef16 Compare May 11, 2022 15:32
@fantix fantix changed the base branch from proto-v1 to master May 12, 2022 16:09
@fantix fantix force-pushed the execute-script branch 2 times, most recently from c0f7fa7 to 59f5a08 Compare May 12, 2022 17:43
@fantix fantix force-pushed the execute-script branch 4 times, most recently from 92d414b to 7523d4c Compare May 18, 2022 19:42
@fantix fantix force-pushed the execute-script branch 16 times, most recently from f62aa94 to 95bbe94 Compare May 25, 2022 17:20
@1st1 1st1 self-requested a review May 25, 2022 17:22
docs/reference/protocol/messages.rst Outdated Show resolved Hide resolved
edb/server/compiler/compiler.py Outdated Show resolved Hide resolved
edb/server/pgcon/pgcon.pyx Outdated Show resolved Hide resolved
edb/server/protocol/binary.pyx Outdated Show resolved Hide resolved
tests/test_edgeql_data_migration.py Outdated Show resolved Hide resolved
This partially reverts commit bb8dd54
@fantix fantix force-pushed the execute-script branch 2 times, most recently from 4f99867 to b0000b4 Compare June 16, 2022 22:37
This partially reverts commit 32c0d63.
Copy link
Member

@elprans elprans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No obvious issues jumped out at me, so let's merge this and do post-merge review and testing.

docs/reference/protocol/index.rst Outdated Show resolved Hide resolved
docs/reference/protocol/index.rst Outdated Show resolved Hide resolved
protocol_version=ctx.protocol_version,
)
else:
# Legacy protocol support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this? We shouldn't be supporting 0.11 and below anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is for restoring pre-0.12 dumps - I'll complete the comment here.

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this.

Co-authored-by: Elvis Pranskevichus <elvis@edgedb.com>
@fantix fantix merged commit f9aada9 into master Jun 17, 2022
@fantix fantix deleted the execute-script branch June 17, 2022 14:31
fmoor added a commit to edgedb/edgedb-go that referenced this pull request Jul 2, 2022
fantix added a commit that referenced this pull request Dec 6, 2023
The reason it was kept is explained in #3120, mitigated in #3814 #3837.
But the core issue that compiler assumes mutation of transaction state
for any compilation even by Parse message still exists. We can drop the
`_last_anon_compiled` because the client bindings are no longer sending
Parse + Execute for `commit` or `rollback` commands (which triggered two
server compilation of the same command) like edgedb/edgedb-python#337.
However, we still need to double check all bindings to make sure the
transaction control commands are using only one `Execute` message.
@fantix fantix mentioned this pull request Dec 6, 2023
9 tasks
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.

None yet

5 participants