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

SQL schema migration fixes and testing #3982

Merged
merged 5 commits into from
Feb 1, 2023

Conversation

nosnilmot
Copy link
Contributor

  • Un-deprecate ejabberd_config:set_option/2
    This might be controversial, but it was necessary for the next item. Is there another way to change config settings at runtime?
  • New schema migration 'update_sql' improvements
    • Check that server_host column does not already exist before addding it and making other changes to table (update_sql becomes idempotent, yay!)
    • Check that indexes exist before dropping them (some are historical and are not created in more recent deployments), elminating spurious errors from logs
    • Update new_sql_schema config after migration, to allow minimizing downtime during migrations (and help with automated testing)
  • Add ability to run tests on upgraded DB
    And enable them in github actions - only run if changes are detected in schema or update_sql code (it would be good to also run this at least once pre-release, but it might be overkill to run for every change)
  • Add support for running tests on MS SQL
    And enable them in github actions
  • Fix a long standing bug in new schema on PostgreSQL
    Same bug as vcard_iq_set crashed when running mod_vcard:vcard_iq_set/1  #3695 which was fixed for new installations but not for upgrades. The fix for any existing impacted installations is the same:
ALTER TABLE vcard_search DROP CONSTRAINT vcard_search_pkey;
ALTER TABLE vcard_search ADD PRIMARY KEY (server_host, lusername);

finding that bug almost made all the effort to enable automated schema upgrade testing worthwhile

There does not appear to be an alternative way to set individual config
options, and this is already used by test/ejabberd_SUITE.erl
- check that server_host column does not already exist before addding it
  and making other changes to table (update_sql becomes idempotent,
  yay!)
- check that indexes exist before dropping them (some are historical and
  are not created in more recent deployments), elminating spurious
  errors from logs
- update new_sql_schema config after migration, to allow near
  zero-downtime migrations (and help with automated testing)
To test update_sql operation and functionality of resulting DB:

1. Load original schema to DB
2. Set {update_sql, true} in suite.erl
3. Run tests
... and make the test that uncovered it explicitly fail (there was already a
TODO) instead of passing but with errors logged
@coveralls
Copy link

coveralls commented Jan 21, 2023

Coverage Status

Coverage: 33.171% (+0.06%) from 33.11% when pulling 0c1cf43 on nosnilmot:sql-update-tests into 3b34538 on processone:master.

@prefiks prefiks self-assigned this Jan 24, 2023
@Neustradamus
Copy link
Contributor

To follow :)

@prefiks
Copy link
Member

prefiks commented Feb 1, 2023

Took me some time to review it, but it all looks sensible. Let's merge it.

@prefiks prefiks merged commit 74c9aa8 into processone:master Feb 1, 2023
@badlop badlop added this to the ejabberd 23.xx milestone Feb 1, 2023
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.

5 participants