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

Refactor verify_constraints_exist #4967

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

morgando
Copy link
Contributor

No description provided.

for (ii = 0; ii < thedb->num_dbs; ii++) {
from_db = get_newer_db(thedb->dbs[ii], new_db);
n_errors += verify_constraints_exist(
from_db, from_db == to_db ? NULL : to_db, new_db, s);
Copy link
Contributor Author

@morgando morgando Jan 16, 2025

Choose a reason for hiding this comment

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

Because of this line, calling

verify_constraints_exist(/*from_db*/ NULL, /*to_db*/ to_db, /*new_db*/ new_db)

will generate this call under the hood:

verify_constraints_exist(/*from_db*/ to_db, /*to_db*/ NULL, /*new_db*/ new_db).

To make this behavior explicit, I replaced all calls to verify_constraints_exist that have no value for from_db but a value for to_db with calls to verify_constraints_to_and_from_dbtable.

@morgando morgando marked this pull request as draft January 16, 2025 20:38
Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Refactor

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>

Tweak impl

Signed-off-by: Morgan Douglas <[email protected]>
Copy link

@roborivers roborivers left a comment

Choose a reason for hiding this comment

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

Coding style check: Error. ⚠.
Smoke testing: Success ✓.
Cbuild submission: Success ✓.
Regression testing: 1/595 tests failed ⚠.

The first 10 failing tests are:
sc_inserts_deletes [setup failure]

Signed-off-by: Morgan Douglas <[email protected]>
@morgando morgando marked this pull request as ready for review January 16, 2025 22:06
*/
static int verify_constraint(constraint_t * const ct,
struct dbtable *source_db,
struct dbtable *target_db,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Give source_db and target_db variables clearer names--maybe ct_source_db/ct_target_db?

return n_errors;
}

int verify_constraints_to_and_from_dbtable(struct dbtable *new_db, int is_updated, struct schema_change_type *s)
Copy link
Contributor Author

@morgando morgando Jan 17, 2025

Choose a reason for hiding this comment

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

TODO: Considering renaming all is_updated variables to undergoing_rebuild/is_rebuild/is_rebuild_needed

return n_errors;
}

int verify_constraints_to_and_from_dbtable(struct dbtable *new_db, int is_updated, struct schema_change_type *s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Rename new_db parameter just db



/*
* Verifies constraints that refer to `target_db`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: This is slightly inaccurate.

Every constraint in the database will be validated, but only constraint rules referring to target_db will be validated.

Update comment to clarify

@morgando morgando requested a review from LiSodium January 17, 2025 19:09
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.

2 participants