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 various bugs in database reflection, connection caching #3245

Closed
wants to merge 20 commits into from

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Oct 13, 2023

Related to #3230

Noteworthy changes:

  • Introduce db.credentials.DbCredentials; a way to uniquely identify a connection to a database; improves on how we previously did that by passing around multiple arguments (like hostname, db_name, username etc);
  • Use of DbCredentials to key the engine cache; before we were incorrectly using the db_name to identify which database a given engine is for: that would break down when you had two databases with the same name (but different host or port);
  • Not a change, but discovered that our reset_reflection and make_sure_initial_reflection_happened constructs still key database by their name (that's a significant bug);
  • Have reflection be triggered whenever a database model is created saved or updated: our reflection policy has been that whenever something changes that might affect Postgres state, we reflect: however, this has not been observed when it came to Database model creations/updates: we were able to create a Database, or even change what actual Postgres database it's refering to without triggering a reflection: now that's imposible;
  • Cleaned up engine management in our testing suite and reflection code: unfortunately, this was necessary, because above change to database reflection (effectively reflecting earlier), 1) caused bugs in testing fixtures due to now incorrect assumptions about when the first reflection would be triggered, and 2) increased simultaneous connection use beyond the 100 Postgres limit, which made the testing suite inoperable; 2) was due to incorrect connection management, which was made worse by the new reflection changes, because they caused more reflections to happen in fixtures (because they make a lot of changes to dbs);
  • Worth noting that above changes to testing suite brought simultaneous connection use down to single digits per testing thread, thus we should be able to run tests via ~9 concurrent threads (again), though haven't thoroughly tested this yet.

Checklist

  • My pull request has a descriptive title (not a vague title like Update ixdex.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@dmos62 dmos62 requested a review from mathemancer October 13, 2023 21:49
@dmos62 dmos62 added affects: technical debt Improves the state of the codebase affects: ux Related to user experience pr-status: review A PR awaiting review type: bug Something isn't working labels Oct 13, 2023
@dmos62 dmos62 added this to the v0.1.4 milestone Oct 13, 2023
@dmos62 dmos62 added the work: backend Related to Python, Django, and simple SQL label Oct 13, 2023
@dmos62
Copy link
Contributor Author

dmos62 commented Oct 17, 2023

Not sure what's happening with the CI. Tests pass fine on my 4 GB dev server, but exit with 137 (mem usage over limit, I presume) over here. Should not block review @mathemancer.

@dmos62
Copy link
Contributor Author

dmos62 commented Oct 20, 2023

Found that the CI was failing during tests because there was a typo in code that was being called by db/install.py::install_mathesar, but no test called it, so tests passed locally, but during setup the CI would break. Added a basic test for this.

Something is breaking in the CI, might be this, not sure.
This reverts commit db8140d.

Revert "Revert change to how port is defined in testing"

This reverts commit f62977c.

Revert "Redo how default db credentials are obtained"

This reverts commit 667d9dc.

Revert "Ignore unused parameter"

This reverts commit efcd9a7.

Revert "Linter fix"

This reverts commit cf0653a.

Revert "Add test for db.install.install_mathesar"

This reverts commit 1518211.
@dmos62
Copy link
Contributor Author

dmos62 commented Oct 23, 2023

When I made above comment I was under the impression that I knew how to get CI tests to pass, because one of the commits there had its CI tests pass, but that was a false-positive. Now, the CI tests seem to be really passing. Once reviewed, ready for merging.

Demo code logic was not changed, only some variables renamed and some
routines broken out into methods.
@mathemancer
Copy link
Contributor

We've completely reorganized the way we're going connections and credentials for databases, and this PR isn't really relevant in that context. I'm going to close it, though we may grab some code from it if needed.

@kgodey kgodey deleted the db-reflection-fixes branch December 24, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: technical debt Improves the state of the codebase affects: ux Related to user experience pr-status: review A PR awaiting review type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants