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

InternalServerError: failed to lookup transaction or savepoint #3120

Closed
jaclarke opened this issue Nov 2, 2021 · 2 comments
Closed

InternalServerError: failed to lookup transaction or savepoint #3120

jaclarke opened this issue Nov 2, 2021 · 2 comments
Labels
Projects

Comments

@jaclarke
Copy link
Member

jaclarke commented Nov 2, 2021

  • EdgeDB Version: 1.0-rc.2+dev6135.d2021110220.ga97561df3.cv202111010000

Steps to Reproduce:

import edgedb

def main():
    conn = edgedb.connect('_localdev')

    conn.query('start transaction')

    try:
        conn.query_single('rollback')
    except edgedb.InterfaceError:
        # ignore 'query cannot be executed with query_single() as it does not return any data' error
        pass

    conn.query('rollback')

    conn.close()

if __name__ == '__main__':
    main()
$ python test.py
Traceback (most recent call last):
  File "/home/james/dev/edgedb-python/test.py", line 19, in <module>
    main()
  File "/home/james/dev/edgedb-python/test.py", line 14, in main
    conn.query('rollback')
  File "/home/james/dev/edgedb-python/edgedb/blocking_con.py", line 346, in query
    return self._execute(
  File "/home/james/dev/edgedb-python/edgedb/blocking_con.py", line 325, in _execute
    raise e
  File "/home/james/dev/edgedb-python/edgedb/blocking_con.py", line 314, in _execute
    return self._get_protocol().sync_execute_anonymous(
  File "edgedb/protocol/blocking_proto.pyx", line 105, in edgedb.protocol.blocking_proto.BlockingIOProtocol.sync_execute_anonymous
    result, _headers = self._iter_coroutine(
  File "edgedb/protocol/blocking_proto.pyx", line 91, in edgedb.protocol.blocking_proto.BlockingIOProtocol._iter_coroutine
    coro.send(None)
  File "edgedb/protocol/protocol.pyx", line 617, in execute_anonymous
    codecs = await self._parse(
  File "edgedb/protocol/protocol.pyx", line 283, in _parse
    raise exc
edgedb.errors.InternalServerError: failed to lookup transaction or savepoint with id=35938955293271
@jaclarke jaclarke added the bug label Nov 2, 2021
@1st1 1st1 added this to To do in 1.0 via automation Nov 4, 2021
@1st1 1st1 removed this from To do in 1.0 Nov 4, 2021
@1st1 1st1 added this to To do in 2.0 via automation Nov 4, 2021
@1st1
Copy link
Member

1st1 commented Nov 4, 2021

I've looked at this and I doubt we will fix it in 1.0.

The problem is that the compiler eagerly evaluates the compiled query against its transactional state and assumes that once the query is compiled it will be executed.

From the standpoint of the compiler, the first conn.query_single('rollback') would succeed so it assumes the transaction will be closed and kills its internal transactional state.

The way to fix this is to introduce some sort of phased synchronization between the IO process and the compiler: the compiler should build the state but not assume anything about it until the IO process actually confirms the execution. But I'm not sure how hard this would be to implement so I doubt we can (or should) fix this before 1.0. The risk of destabilizing the code base is too high imo.

fantix added a commit that referenced this issue May 3, 2022
The OptimisticExecute was not ready for the Parse/Execute convention
because it was always looking up in the cache and ignoring the Parse
result. This was an issue because some statement is not cacheable and
compiling again confused the state machine (#3120). Fixed in a way that
the previously parsed result is used if the query hash matches, and
dropped immediately on _execute() to mimic the legacy OptimisticExecute
behavior. test_server_proto_tx_03() and _04() covers both cases.
fantix added a commit that referenced this issue May 16, 2022
The OptimisticExecute was not ready for the Parse/Execute convention
because it was always looking up in the cache and ignoring the Parse
result. This was an issue because some statement is not cacheable and
compiling again confused the state machine (#3120). Fixed in a way that
the previously parsed result is used if the query hash matches, and
dropped immediately on _execute() to mimic the legacy OptimisticExecute
behavior. test_server_proto_tx_03() and _04() covers both cases.
fantix added a commit that referenced this issue May 18, 2022
The OptimisticExecute was not ready for the Parse/Execute convention
because it was always looking up in the cache and ignoring the Parse
result. This was an issue because some statement is not cacheable and
compiling again confused the state machine (#3120). Fixed in a way that
the previously parsed result is used if the query hash matches, and
dropped immediately on _execute() to mimic the legacy OptimisticExecute
behavior. test_server_proto_tx_03() and _04() covers both cases.
fantix added a commit that referenced this issue May 18, 2022
* Drop support for protocol < 0.13
* Add v0-compatible protocol
* Bump current protocol to 1.0 and preserve compatible support
* Drop statement name of Parse/Prepare message
* Drop statement name of Execute
* Drop the DescribeStatement message
* Rename make_describe_msg to make_command_data_description_msg
* Drop the Execute message

The OptimisticExecute was not ready for the Parse/Execute convention
because it was always looking up in the cache and ignoring the Parse
result. This was an issue because some statement is not cacheable and
compiling again confused the state machine (#3120). Fixed in a way that
the previously parsed result is used if the query hash matches, and
dropped immediately on _execute() to mimic the legacy OptimisticExecute
behavior. test_server_proto_tx_03() and _04() covers both cases.

* Docs: rename Prepare -> Parse, OptimisticExecute -> Execute
* Drop the "optimistic" wording in the code
* Use edgedb 0.24.0a1
@msullivan
Copy link
Member

This is fixed

2.0 automation moved this from To do to Done Jan 26, 2023
fantix added a commit that referenced this issue 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
2.0
  
Done
Development

No branches or pull requests

3 participants