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

Upgrade from psycopg2 to psycopg3 #10742

Merged
merged 21 commits into from
Aug 1, 2023
Merged

Conversation

di
Copy link
Member

@di di commented Feb 13, 2022

Closes #13710.

@di
Copy link
Member Author

di commented Feb 13, 2022

Blocked on mahmoudimus/sqlalchemy-citext#26

@di di added the blocked Issues we can't or shouldn't get to yet label Feb 13, 2022
@di di added the dependencies Pull requests that update a dependency file label Mar 7, 2022
@di
Copy link
Member Author

di commented Apr 24, 2023

Potentially resolved by upgrading to sqlalchemy 2.0.7: mahmoudimus/sqlalchemy-citext#27 (comment)

@di di force-pushed the upgrade-from-psycopg2-to-psycopg3 branch from da2d16a to 6f4bc75 Compare July 26, 2023 23:46
@di di requested a review from a team as a code owner July 26, 2023 23:46
@di
Copy link
Member Author

di commented Jul 26, 2023

I tried to get the migrations to work by using with op.get_context().autocommit_block(): for operations that can't run inside a transaction block, but couldn't get it to work.

@di di removed the blocked Issues we can't or shouldn't get to yet label Jul 27, 2023
@di di force-pushed the upgrade-from-psycopg2-to-psycopg3 branch from 6f5f59b to b81e8e2 Compare July 27, 2023 16:08
requirements/main.in Outdated Show resolved Hide resolved
docs/dev/development/getting-started.rst Outdated Show resolved Hide resolved
@di
Copy link
Member Author

di commented Jul 27, 2023

make initdb is currently failing with:

{"logger": "alembic.runtime.migration", "level": "INFO", "event": "Running upgrade d18d443f89f0 -> 1b97443dea8a, create missing FK indexes", "thread": 140380821546816}
Traceback (most recent call last):
  File "/opt/warehouse/src/warehouse/cli/db/__init__.py", line 33, in alembic_lock
    yield alembic_config
  File "/opt/warehouse/src/warehouse/cli/db/upgrade.py", line 29, in upgrade
    alembic.command.upgrade(alembic_config, revision, **kwargs)
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/command.py", line 385, in upgrade
    script.run_env()
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/script/base.py", line 582, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/util/pyfiles.py", line 94, in load_python_file
    module = load_module_py(module_id, path)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/util/pyfiles.py", line 110, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/opt/warehouse/src/warehouse/migrations/env.py", line 81, in <module>
    run_migrations_online()
  File "/opt/warehouse/src/warehouse/migrations/env.py", line 75, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/runtime/environment.py", line 928, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/runtime/migration.py", line 628, in run_migrations
    step.migration_fn(**kw)
  File "/opt/warehouse/src/warehouse/migrations/versions/1b97443dea8a_create_missing_fk_indexes.py", line 30, in upgrade
    with op.get_context().autocommit_block():
  File "/usr/local/lib/python3.11/contextlib.py", line 137, in __enter__
    return next(self.gen)
           ^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/alembic/runtime/migration.py", line 353, in autocommit_block
    fake_trans: Optional[Transaction] = self.connection.begin()
                                        ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 849, in begin
    self._transaction = RootTransaction(self)
                        ^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 2665, in __init__
    TransactionalContext._trans_ctx_check(connection)
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/util.py", line 110, in _trans_ctx_check
    raise exc.InvalidRequestError(
sqlalchemy.exc.InvalidRequestError: Can't operate on closed transaction inside context manager.  Please complete the context manager before emitting further commands.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/opt/warehouse/src/warehouse/__main__.py", line 18, in <module>
    sys.exit(warehouse())
             ^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/click/decorators.py", line 45, in new_func
    return f(get_current_context().obj, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/src/warehouse/cli/db/upgrade.py", line 26, in upgrade
    with alembic_lock(
  File "/usr/local/lib/python3.11/contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/opt/warehouse/src/warehouse/cli/db/__init__.py", line 36, in alembic_lock
    connection.execute(text("SELECT pg_advisory_unlock(hashtext('alembic'))"))
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1412, in execute
    return meth(
           ^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/sql/elements.py", line 483, in _execute_on_connection
    return connection._execute_clauseelement(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1635, in _execute_clauseelement
    ret = self._execute_context(
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/base.py", line 1831, in _execute_context
    TransactionalContext._trans_ctx_check(self)
  File "/opt/warehouse/lib/python3.11/site-packages/sqlalchemy/engine/util.py", line 110, in _trans_ctx_check
    raise exc.InvalidRequestError(
sqlalchemy.exc.InvalidRequestError: Can't operate on closed transaction inside context manager.  Please complete the context manager before emitting further commands.
make[1]: *** [Makefile:112: runmigrations] Error 1
make[1]: Leaving directory '/workspaces/warehouse'
make: *** [Makefile:106: initdb] Error 2

@di
Copy link
Member Author

di commented Jul 28, 2023

This gets the migrations to run, but the warehouse/migrations/versions/5dcbd2bc748f_create_organizationapplication_model.py migration seems to fail to fully apply, because the table it creates does not exist:

self = <psycopg.Cursor [closed] [INERROR] (host=db user=postgres database=tests) at 0x7f16be57efb0>
query = 'SELECT normalize_pep426_name(organization_applications.name) AS normalize_pep426_name_1, organization_applications.su...R) AND %(param_1)s::UUID = organization_applications.submitted_by_id AND organization_applications.is_approved IS NULL'
params = {'normalize_pep426_name_2': 'psf', 'param_1': UUID('b86feefa-6130-4f96-88d1-96399020275f')}

    def execute(
        self: _Self,
        query: Query,
        params: Optional[Params] = None,
        *,
        prepare: Optional[bool] = None,
        binary: Optional[bool] = None,
    ) -> _Self:
        """
        Execute a query or command to the database.
        """
        try:
            with self._conn.lock:
                self._conn.wait(
                    self._execute_gen(query, params, prepare=prepare, binary=binary)
                )
        except e._NO_TRACEBACK as ex:
>           raise ex.with_traceback(None)
E           sqlalchemy.exc.ProgrammingError: (psycopg.errors.UndefinedTable) relation "organization_applications" does not exist
E           LINE 2: FROM organization_applications 
E                        ^
E           [SQL: SELECT normalize_pep426_name(organization_applications.name) AS normalize_pep426_name_1, organization_applications.submitted_by_id AS organization_applications_submitted_by_id, organization_applications.submitted AS organization_applications_submitted, organization_applications.organization_id AS organization_applications_organization_id, organization_applications.name AS organization_applications_name, organization_applications.display_name AS organization_applications_display_name, organization_applications.orgtype AS organization_applications_orgtype, organization_applications.link_url AS organization_applications_link_url, organization_applications.description AS organization_applications_description, organization_applications.is_approved AS organization_applications_is_approved, organization_applications.id AS organization_applications_id 
E           FROM organization_applications 
E           WHERE normalize_pep426_name(organization_applications.name) = normalize_pep426_name(%(normalize_pep426_name_2)s::VARCHAR) AND %(param_1)s::UUID = organization_applications.submitted_by_id AND organization_applications.is_approved IS NULL]
E           [parameters: {'normalize_pep426_name_2': 'psf', 'param_1': UUID('b86feefa-6130-4f96-88d1-96399020275f')}]
E           (Background on this error at: https://sqlalche.me/e/20/f405)

../lib/python3.11/site-packages/psycopg/cursor.py:723: ProgrammingError
=================================================================== warnings summary ====================================================================
../lib/python3.11/site-packages/google/rpc/__init__.py:20
  /opt/warehouse/lib/python3.11/site-packages/google/rpc/__init__.py:20: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('google.rpc')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    pkg_resources.declare_namespace(__name__)

../lib/python3.11/site-packages/pkg_resources/__init__.py:2350
  /opt/warehouse/lib/python3.11/site-packages/pkg_resources/__init__.py:2350: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('google')`.
  Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
    declare_namespace(parent)

../lib/python3.11/site-packages/ddtrace/internal/module.py:220
  /opt/warehouse/lib/python3.11/site-packages/ddtrace/internal/module.py:220: DeprecationWarning: 'cgi' is deprecated and slated for removal in Python 3.13
    self.loader.exec_module(module)

../lib/python3.11/site-packages/ddtrace/internal/module.py:220
  /opt/warehouse/lib/python3.11/site-packages/ddtrace/internal/module.py:220: DeprecationWarning: 'crypt' is deprecated and slated for removal in Python 3.13
    self.loader.exec_module(module)

../lib/python3.11/site-packages/ddtrace/internal/module.py:220
  /opt/warehouse/lib/python3.11/site-packages/ddtrace/internal/module.py:220: DeprecationWarning: module 'sre_constants' is deprecated
    self.loader.exec_module(module)

../lib/python3.11/site-packages/b2sdk/file_version.py:220
  /opt/warehouse/lib/python3.11/site-packages/b2sdk/file_version.py:220: DeprecationWarning: invalid escape sequence '\_'
    """

../lib/python3.11/site-packages/b2sdk/raw_simulator.py:1152
  /opt/warehouse/lib/python3.11/site-packages/b2sdk/raw_simulator.py:1152: DeprecationWarning: invalid escape sequence '\?'
    '/b2api/v[0-9]+/b2_download_file_by_id\?fileId=(?P<file_id>[^/]+)',

../lib/python3.11/site-packages/forcediphttpsadapter/adapters.py:92
../lib/python3.11/site-packages/forcediphttpsadapter/adapters.py:92
  /opt/warehouse/lib/python3.11/site-packages/forcediphttpsadapter/adapters.py:92: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    if StrictVersion(requests.__version__) < StrictVersion('2.4.0'):

tests/unit/oidc/test_services.py:961
  /opt/warehouse/src/tests/unit/oidc/test_services.py:961: PytestCollectionWarning: cannot collect test class 'TestPyJWTBackstop' because it has a __init__ constructor (from: tests/unit/oidc/test_services.py)
    class TestPyJWTBackstop:

tests/functional/test_basic.py: 10 warnings
  /opt/warehouse/src/warehouse/legacy/action_routing.py:25: DeprecationWarning: The "custom_predicates" argument to Configurator.add_route is deprecated as of Pyramid 1.5. Use "config.add_route_predicate" and use the registered route predicate as a predicate argument to add_route instead. See "Adding A Custom View, Route, or Subscriber Predicate" in the "Hooks" chapter of the documentation for more information.
    config.add_route(name, "/pypi", custom_predicates=custom_predicates, **kwargs)

tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
  /opt/warehouse/src/warehouse/redirects.py:33: DeprecationWarning: The "custom_predicates" argument to Configurator.add_route is deprecated as of Pyramid 1.5. Use "config.add_route_predicate" and use the registered route predicate as a predicate argument to add_route instead. See "Adding A Custom View, Route, or Subscriber Predicate" in the "Hooks" chapter of the documentation for more information.
    config.add_route(route_name, source, **kw)

tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
  /opt/warehouse/src/warehouse/forklift/action_routing.py:20: DeprecationWarning: The "custom_predicates" argument to Configurator.add_route is deprecated as of Pyramid 1.5. Use "config.add_route_predicate" and use the registered route predicate as a predicate argument to add_route instead. See "Adding A Custom View, Route, or Subscriber Predicate" in the "Hooks" chapter of the documentation for more information.
    config.add_route(name, "/legacy/", custom_predicates=custom_predicates, **kwargs)

tests/functional/test_basic.py: 220 warnings
  <frozen importlib._bootstrap_external>:629: DeprecationWarning: find_module() is deprecated and slated for removal in Python 3.12; use find_spec() instead

tests/functional/test_basic.py: 220 warnings
  <frozen importlib._bootstrap_external>:1591: DeprecationWarning: FileFinder.find_loader() is deprecated and slated for removal in Python 3.12; use find_spec() instead

tests/functional/test_basic.py: 3 warnings
tests/functional/test_caching.py: 1 warning
tests/functional/legacy_api/test_basic.py: 1 warning
tests/functional/legacy_api/test_removed.py: 4 warnings
tests/functional/legacy_api/test_xmlrpc.py: 5 warnings
  /opt/warehouse/lib/python3.11/site-packages/webob/acceptparse.py:3412: DeprecationWarning: The behavior of .best_match for the Accept-Encoding classes is currently being maintained for backward compatibility, but the method will be deprecated in the future, as its behavior is not specified in (and currently does not conform to) RFC 7231.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================= slowest 20 durations ==================================================================
12.18s setup    tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
0.79s call     tests/functional/test_caching.py::test_basic_views_dont_vary[/]
0.31s call     tests/functional/test_templates.py::test_render_templates[template14]
0.30s call     tests/functional/test_basic.py::test_non_existent_route_404
0.30s setup    tests/functional/manage/test_views.py::TestManageAccount::test_save_account
0.28s call     tests/functional/test_templates.py::test_render_templates[template159]
0.25s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template159]
0.25s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template14]
0.22s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template15]
0.19s call     tests/functional/test_basic.py::test_robots_txt[pypi.org-True]
0.16s call     tests/functional/manage/test_views.py::TestManageAccount::test_save_account
0.15s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template42]
0.15s call     tests/functional/test_templates.py::test_render_templates[template42]
0.14s call     tests/functional/test_templates.py::test_render_templates[template15]
0.13s call     tests/functional/test_templates.py::test_render_templates[template40]
0.13s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template23]
0.13s call     tests/functional/test_templates.py::test_render_templates[template20]
0.12s call     tests/functional/test_templates.py::test_render_templates[template23]
0.12s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template40]
0.12s call     tests/functional/test_templates.py::test_templates_for_empty_titles[template16]
================================================================ short test summary info ================================================================
FAILED tests/functional/manage/test_views.py::TestManageOrganizations::test_create_organization - sqlalchemy.exc.ProgrammingError: (psycopg.errors.UndefinedTable) relation "organization_applications" does not exist

The context manager issues a ROLLBACK unless we commit inside it.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Now that the concept is removed, we need to remove assertions that fail
in its absence.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
psycopg2 passed strings, but psycopg 3 uses real types.

Use `type_coerce()` instead of `cast()` to have the coercion done in
Python, and not emit a `CAST()` in the generated SQL.

Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
@miketheman miketheman force-pushed the upgrade-from-psycopg2-to-psycopg3 branch from 2627fab to 52f8dcf Compare July 29, 2023 20:31
@miketheman
Copy link
Member

I added a few more commits - should pass cleanly now, but worth reviewing nonetheless.

psycopg 3 now uses "real" types with PG (INET, datetime et al) - something we should continue to work on migrating to SQLAlchemy 2.0 typed model syntax so mismatches and implicit coercion can be handled more cleanly.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
warehouse/migrations/env.py Outdated Show resolved Hide resolved
@miketheman
Copy link
Member

Also just noticed you likely wanna remove types-psycopg2 from requirements/lint as well

warehouse/migrations/env.py Show resolved Hide resolved
@@ -55,7 +55,7 @@ def upgrade():
"""
)
)
conn.execute(sa.text("COMMIT"))
op.get_bind().commit()
Copy link
Member

Choose a reason for hiding this comment

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

I don't thing we need to go back and change historical migrations at this point other than the minimum to get them working. I just wanted to note somewhere that I think in the future for this pattern, an auto commit block wrapping the while loop might end up being cleaner.

@ewdurbin ewdurbin merged commit 92e21da into main Aug 1, 2023
@ewdurbin ewdurbin deleted the upgrade-from-psycopg2-to-psycopg3 branch August 1, 2023 19:13
ewdurbin added a commit that referenced this pull request Aug 1, 2023
ewdurbin added a commit that referenced this pull request Aug 1, 2023
di added a commit that referenced this pull request Aug 1, 2023
miketheman added a commit that referenced this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants