fix(mnesia_acl): introduce optimized schema and migration process

[savonarola]

This is an optimization to fix #5506

[qzhuyan]

maybe it is safer not to remove the old table and just leave it or?

[qzhuyan]

In general, there is one assumption that all nodes in the cluster should participate in the migration but I wonder if users is ok with that since normally they try a new version on one of the nodes then decide if they should go for it or fallback, especially when they want to try the fix in this PR on the new node only.

[qzhuyan]

In general, there is one assumption that all nodes in the cluster should participate in the migration but I wonder if users is ok with that since normally they try a new version on one of the nodes then decide if they should go for it or fallback, especially when they want to try the fix in this PR on the new node only.

I am just putting the limitation here and I am not asking for a change.

[zmstone]

@k32 do you remember where is the code for data export & import ?
since we do not plan to export any data from 4.3, so it's fine not to support it.
but for 4.2 data import, we'll need to be compatible.

[k32]

Code for the data import/export is located in this module: apps/emqx_management/src/emqx_mgmt_data_backup.erl

[zmstone]

Great job!

[qzhuyan]

LGTM.

[qzhuyan]

I am not familiar with QLC, someone should review that part.

[thalesmg]

Nit: would it make sense to use a pattern that does not depend on field order or changes?

Like:

login_match_spec_new(LoginSpec) ->
    [{#?ACL_TABLE2{_ = '_', who = LoginSpec}, [], ['$_']}].

login_match_spec_old(LoginSpec) ->
    [{#?ACL_TABLE{_ = '_', filter = {LoginSpec, '_'}}, [], ['$_']}].

[savonarola]

✅

[zmstone]

now dialyzer hates it.

[k32]

One trick to use record definition for ets match, and still have sane types is the following:

-record(foo, {a, b}). %% Record definition doesn't specify field types

-type foo() :: #foo{a :: foo(), b :: bar()}. %% Create a typedef that specifies field types and use it in the specs instead of #foo{} type

[savonarola]

But then we will loose useful warnings too 🤔
I mean, when we put #foo{b = NotABar} unintentionally.

[thalesmg]

I didn't know that was bad for dialyzer, sorry. 🙊

[zmstone]

using record is better.

we can suppress dialyzer warnings. e.g. -dialyzer({nowarn_function,foo/3})

[savonarola]

Seems that we can't just disable warnings for single funs, because dialyzer looses their typings and fails in other places, like https://github.com/emqx/emqx/runs/4013130567?check_suite_focus=true#step:6:87

[savonarola]

I pushed a variant with ets:fun2ms that also allows to construct matchspecs without concerning about unspecified fields.
Additionally, the external API of emqx_acl_mnesia_db module now does not receive match_head parts.