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

fix(db_api): ensure DDL statements are being executed #290

Merged
merged 3 commits into from Mar 25, 2021

Conversation

IlyaFaer
Copy link
Member

@IlyaFaer IlyaFaer commented Mar 24, 2021

This PR ensures that DDL statements are being executed.

Currently, DDL statements aren't executed until a non-DDL statement is executed. This leads to unexpected behavior. This fix will execute DDL statements immediately when autocommit=True and when commit() is called for autocommit=False.

Note that DDL statements will still be executed when a non-DDL statements is executed in autocommit=False. e.g.

cur.execute(
"""
CREATE TABLE Singers (
    SingerId     INT64 NOT NULL,
    Name    STRING(1024),
) PRIMARY KEY (SingerId)
"""
)

# CREATE TABLE statement will be execute here; before the INSERT statement.

cur.execute(
"""
INSERT INTO Singers (SingerId, Name) VALUES (1, "Name")
"""
)

@google-cla google-cla bot added the cla: yes label Mar 24, 2021
@product-auto-label product-auto-label bot added the api: spanner label Mar 24, 2021
@@ -178,6 +178,8 @@ def execute(self, sql, args=None):
ddl = ddl.strip()
if ddl:
self.connection._ddl_statements.append(ddl)
if self.connection.autocommit:
Copy link
Contributor

@larkee larkee Mar 24, 2021

Choose a reason for hiding this comment

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

I think we should execute the statements immediately regardless of whether autocommit is set or not. Because Spanner handles DDL separately from transactions, there's no way to roll back DDL statements (which we will need to clearly document) and so I think having different behavior based on autocommit is unnecessary.

@AVaksman WDYT? Note that we now support executing multiple DDL statements at once which should alleviate the batching concern.

Copy link
Member Author

@IlyaFaer IlyaFaer Mar 24, 2021

Choose a reason for hiding this comment

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

@larkee, no problem, I'll delete the line.

I'm writing a system test to check the case.

Also I've found out that in !autocommit mode DDLs are also not executed, even if one will call commit(). Will fix it in this PR as well.

Безымянный

Copy link
Member Author

@IlyaFaer IlyaFaer Mar 24, 2021

Choose a reason for hiding this comment

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

@larkee, you know, about regarding autocommit mode... I think users don't usually go deep into frameworks code. So, we support DDLs separated with ;, still users may use frameworks, which are generating and executing DDLs one by one. In this case it's probably better to look after autocommit mode, to make DDLs pumping up in !autocommit mode - to execute them all at once little bit later.

Copy link
Contributor

@larkee larkee Mar 24, 2021

Choose a reason for hiding this comment

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

Hmm, that's a good point. In that case, we'll need to add run_prior_DDL_statements() to commit() as currently the statements aren't being executed even when commit() is called.

@IlyaFaer IlyaFaer marked this pull request as ready for review Mar 24, 2021
@IlyaFaer IlyaFaer requested a review from as a code owner Mar 24, 2021
@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Mar 24, 2021

@larkee, something failed in system tests, but it doesn't seem to be related, as DB API system tests live in another file:

__________ TestSessionAPI.test_transaction_batch_update_w_parent_span __________

self = <tests.system.test_system.TestSessionAPI testMethod=test_transaction_batch_update_w_parent_span>

    def test_transaction_batch_update_w_parent_span(self):
        try:
            from opentelemetry import trace
        except ImportError:
            return

        tracer = trace.get_tracer(__name__)

        retry = RetryInstanceState(_has_all_ddl)
        retry(self._db.reload)()

        session = self._db.session()
        session.create()
        self.to_delete.append(session)

        with session.batch() as batch:
            batch.delete(self.TABLE, self.ALL)

        insert_statement = list(self._generate_insert_statements())[0]
        update_statement = (
            "UPDATE contacts SET email = @email " "WHERE contact_id = @contact_id;",
            {"contact_id": 1, "email": "phreddy@example.com"},
            {"contact_id": param_types.INT64, "email": param_types.STRING},
        )
        delete_statement = (
            "DELETE contacts WHERE contact_id = @contact_id;",
            {"contact_id": 1},
            {"contact_id": param_types.INT64},
        )

        def unit_of_work(transaction, self):

            status, row_counts = transaction.batch_update(
                [insert_statement, update_statement, delete_statement]
            )
            self._check_batch_status(status.code)
            self.assertEqual(len(row_counts), 3)
            for row_count in row_counts:
                self.assertEqual(row_count, 1)

        with tracer.start_as_current_span("Test Span"):
            session.run_in_transaction(unit_of_work, self)

        span_list = self.memory_exporter.get_finished_spans()
>       self.assertEqual(len(span_list), 6)
E       AssertionError: 9 != 6

tests/system/test_system.py:1774: AssertionError

@larkee
Copy link
Contributor

@larkee larkee commented Mar 24, 2021

This is a known flaky test that I have an issue for: #165

I'll force the tests to rerun 👍

@larkee larkee added the kokoro:force-run label Mar 24, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Mar 24, 2021
Copy link
Contributor

@larkee larkee left a comment

LGTM, just some comments on the system tests

tests/system/test_system_dbapi.py Show resolved Hide resolved
tests/system/test_system_dbapi.py Outdated Show resolved Hide resolved
larkee
larkee approved these changes Mar 24, 2021
@IlyaFaer IlyaFaer added the kokoro:force-run label Mar 24, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Mar 24, 2021
@IlyaFaer IlyaFaer added the kokoro:force-run label Mar 24, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Mar 24, 2021
@IlyaFaer
Copy link
Member Author

@IlyaFaer IlyaFaer commented Mar 24, 2021

@larkee, checks passed. They stuck once for 5 hours, so I've reexecuted them

@larkee larkee changed the title fix(db_api): DDLs are not executed immediately in autocommit mode fix(db_api): ensure DDL statements are being executed Mar 25, 2021
@larkee larkee merged commit baa02ee into googleapis:master Mar 25, 2021
8 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 25, 2021
🤖 I have created a release \*beep\* \*boop\*
---
## [3.3.0](https://www.github.com/googleapis/python-spanner/compare/v3.2.0...v3.3.0) (2021-03-25)


### Features

* add encryption_info to Database ([#284](https://www.github.com/googleapis/python-spanner/issues/284)) ([2fd0352](https://www.github.com/googleapis/python-spanner/commit/2fd0352f695d7ab85e57d8c4388f42f91cf39435))
* add support for CMEK ([#105](https://www.github.com/googleapis/python-spanner/issues/105)) ([e990ff7](https://www.github.com/googleapis/python-spanner/commit/e990ff70342e7c2e27059e82c8d74cce39eb85d0))
* add support for custom timeout and retry parameters in execute_update method in transactions ([#251](https://www.github.com/googleapis/python-spanner/issues/251)) ([8abaebd](https://www.github.com/googleapis/python-spanner/commit/8abaebd9edac198596e7bd51d068d50147d0391d))
* added retry and timeout params to partition read in database and snapshot class ([#278](https://www.github.com/googleapis/python-spanner/issues/278)) ([1a7c9d2](https://www.github.com/googleapis/python-spanner/commit/1a7c9d296c23dfa7be7b07ea511a4a8fc2c0693f))
* **db_api:** support executing several DDLs separated by semicolon ([#277](https://www.github.com/googleapis/python-spanner/issues/277)) ([801ddc8](https://www.github.com/googleapis/python-spanner/commit/801ddc87434ff9e3c86b1281ebfeac26195c06e8))


### Bug Fixes

* avoid consuming pending null values when merging ([#286](https://www.github.com/googleapis/python-spanner/issues/286)) ([c6cba9f](https://www.github.com/googleapis/python-spanner/commit/c6cba9fbe4c717f1f8e2a97e3f76bfe6b956e55b))
* **db_api:** allow file path for credentials ([#221](https://www.github.com/googleapis/python-spanner/issues/221)) ([1de0284](https://www.github.com/googleapis/python-spanner/commit/1de028430b779a50d38242fe70567e92b560df5a))
* **db_api:** ensure DDL statements are being executed ([#290](https://www.github.com/googleapis/python-spanner/issues/290)) ([baa02ee](https://www.github.com/googleapis/python-spanner/commit/baa02ee1a352f7c509a3e169927cf220913e521f))
* **db_api:** revert Mutations API usage ([#285](https://www.github.com/googleapis/python-spanner/issues/285)) ([e5d4901](https://www.github.com/googleapis/python-spanner/commit/e5d4901e9b7111b39dfec4c56032875dc7c6e74c))


### Documentation

* fix docstring types and typos ([#259](https://www.github.com/googleapis/python-spanner/issues/259)) ([1b0ce1d](https://www.github.com/googleapis/python-spanner/commit/1b0ce1d2986085ce4033cf773eb6c5d3b904473c))
* fix snapshot usage ([#291](https://www.github.com/googleapis/python-spanner/issues/291)) ([eee2181](https://www.github.com/googleapis/python-spanner/commit/eee218164c3177586b73278aa21495280984af89))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@IlyaFaer IlyaFaer deleted the ddls_problem branch Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants