Skip to content

Michal/mnesia/add_tests_for_compare_storage_type #9444

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

Merged

Conversation

Mikaka27
Copy link
Contributor

Those tests are related to #9311.
When investigating problem during schema merge it was found that when some nodes in the cluster are down, it's still possible to call mnesia:change_table_copy_type/3 for 2 special cases:

  • ram_copies -> disc_copies
  • disc_copies -> disc_only_copies

The cause of this is that mnesia_schema:compare_storage_types/3 does not consider the two (mentioned above) conversions as incompatible, this can be seen here:

compare_storage_type(_Retry, ram_copies, disc_copies) ->
{diff, disc_copies};
compare_storage_type(_Retry, disc_copies, disc_only_copies) ->
{diff, disc_only_copies};

The cause is in mnesia_schema:make_change_table_copy_type/3, one can see that only when storage type is incompatible, it calls ensure_active(Cs)

case compare_storage_type(false, FromS, ToSExp) of
{same, _} ->
mnesia:abort({already_exists, Tab, Node, ToSExp});
{diff, _} ->
ignore;
incompatible ->
ensure_active(Cs)
end,

In addition to that:

compare_storage_type(_Retry, ram_copies, disc_copies) ->
    {diff, disc_copies};

is used for merging a schema after it's removal (when node rejoins the cluster), this can be seen in mnesia_config_test:dynamic_basic/1 when doing following operations:

?match([], mnesia_test_lib:kill_mnesia([N2])),
?match(ok, mnesia:delete_schema([N2])),
?match(ok, mnesia:dirty_write({tab1, 1, 1})),
?match(ok, mnesia:dirty_write({tab2, 1, 1})),
?match(ok, rpc:call(N2, mnesia, start, [[{extra_db_nodes, [N1]}, {schema, ?BACKEND}]])),

The cause of that is when we remove node's schema it restarts with ram_copies schema, and later has to join a cluster and convert it's schema to disc_copies.
This is mentioned in the documentation: https://www.erlang.org/doc/apps/mnesia/mnesia.html#module-configuration-parameters

opt_disc - Optional disc. The schema can reside on disc or in RAM. If the schema is found on disc, Mnesia starts as a disc-based node and the storage type of the schema table is disc_copies. If no schema is found on disc, Mnesia starts as a disc-less node and the storage type of the schema table is ram_copies. Default value for the application parameter is opt_disc.

but

compare_storage_type(_Retry, disc_copies, disc_only_copies) ->
    {diff, disc_only_copies};

is not covered by any tests, so add tests for both of those cases.

@Mikaka27 Mikaka27 added the team:PS Assigned to OTP team PS label Feb 14, 2025
@Mikaka27 Mikaka27 requested a review from dgud February 14, 2025 17:25
Copy link
Contributor

github-actions bot commented Feb 14, 2025

CT Test Results

  2 files   59 suites   20m 12s ⏱️
683 tests 534 ✅ 148 💤 1 ❌
737 runs  573 ✅ 163 💤 1 ❌

For more details on these failures, see this check.

Results for commit f09ed97.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@Mikaka27 Mikaka27 changed the title Add tests for compare_storage_type Michal/mnesia/add_tests_for_compare_storage_type Feb 14, 2025
dgud
dgud previously approved these changes Feb 26, 2025
?match({atomic, ok}, mnesia:change_table_copy_type(tab, N1, disc_copies)),

?match(ok, rpc:call(N2, mnesia, start, [[{extra_db_nodes, [N1]}, {schema, ?BACKEND}]])),
?verify_mnesia(All, []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
?verify_mnesia(All, []),
?verify_mnesia(All, [tab]),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added check if table type was changed instead.

?match({atomic, ok}, mnesia:change_table_copy_type(tab, N1, disc_only_copies)),

?match(ok, rpc:call(N2, mnesia, start, [[{extra_db_nodes, [N1]}, {schema, ?BACKEND}]])),
?verify_mnesia(All, []).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
?verify_mnesia(All, []).
?verify_mnesia(All, [tab]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added check if table type was changed instead.

@Mikaka27 Mikaka27 force-pushed the michal/mnesia/tests_for_compare_storage_type branch from 54cb7c6 to 74f663b Compare March 4, 2025 10:47
@Mikaka27 Mikaka27 requested a review from dgud March 4, 2025 10:50
@Mikaka27 Mikaka27 force-pushed the michal/mnesia/tests_for_compare_storage_type branch from 74f663b to f09ed97 Compare March 4, 2025 12:40
@Mikaka27 Mikaka27 added the testing currently being tested, tag is used by OTP internal CI label Mar 4, 2025
@Mikaka27 Mikaka27 force-pushed the michal/mnesia/tests_for_compare_storage_type branch from f09ed97 to 2606259 Compare March 4, 2025 13:40
@Mikaka27 Mikaka27 force-pushed the michal/mnesia/tests_for_compare_storage_type branch from 2606259 to f1a5aa3 Compare March 4, 2025 14:19
@Mikaka27 Mikaka27 merged commit 9d1f5ad into erlang:master Mar 11, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants