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

MDEV-31746 Problems with tx_isolation after MDEV-21921 #2707

Closed
wants to merge 1 commit into from

Conversation

grooverdan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-31746

Description

With session tracking on the tx_isolation of importance to connector frameworks, its important that tracking of tx_isolation does get informed if a user set session transaction_isolation=X as the alias for tx_isolation.

Rather than just implement this for one variable alias, it is implemented for all aliases.

To assist with this the session_tracker has is now made up of the offset types, the sys_var_bit bitmask as well.

This becomes the equivalavant of aliases that change the same memory location.

The impacts of aliases are:

  • If track one variable, its alias changes, you get a tracking change on the variable you are monitoring.
  • If you track two aliased variables of each other, changing one variable with have two tracking events.
  • If you are tracking *, you will get a change on the variable changed. If then you change its alias, you'll get two tracking events for both variable changes.

Other:

  • Add const overrides to necessary functions
  • Eliminate session tracking in bootstrap.

How can this PR be tested?

test case updated

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

Basing the PR against the correct MariaDB version

  • This is a new feature and the PR is based against the latest MariaDB development branch.
  • [ X ] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • [ X ] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [ X ] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Copy link
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

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

please, keep unrelated changes in a separate commit (ok to have them in the same PR, I know how to look at individual commits within a PR) - for example override changes.

we've discussed that the tracking id can still be one ulonglong. Because the offset is guaranteed to be within sizeof(system_variables) so less than few K (1088 in the current 10.5 and in the current 11.2). And a bit variable is always one bit, so unambiguously represented by a bit number, always less than 64. You can absolutely safely create the tracking id by, say, (bitnum << 32) | offset or (offset << 8) | bitnum

and why did you remove HASH_UNIQUE and replaced it with manual checks for duplicates? I think it'd be much simpler to let hash to enforce the uniqueness.

@grooverdan
Copy link
Member Author

please, keep unrelated changes in a separate commit (ok to have them in the same PR, I know how to look at individual commits within a PR) - for example override changes.

ack

we've discussed that the tracking id can still be one ulonglong.

Tracking id is always passes to the hash function as an address so using another constructed ulonglong seemed excessive, that's why it provides the address of the structure of existing elements.

Because the offset is guaranteed to be within sizeof(system_variables) so less than few K (1088 in the current 10.5 and in the current 11.2).
And a bit variable is always one bit

The bit variable was always constructed as a bitmask of type and not a bitnum.

, so unambiguously represented by a bit number, always less than 64. You can absolutely safely create the tracking id by, say, (bitnum << 32) | offset or (offset << 8) | bitnum

There wasn't an existing function to transform back to a bitnum that I could see. Providing the address of a structure containing the key of a few bits more that mostly needed to generate the hash was less invasive to achieve.

and why did you remove HASH_UNIQUE and replaced it with manual checks for duplicates? I think it'd be much simpler to let hash to enforce the uniqueness.

The HASH_UNIQUE was on the key. Because monitoring of aliases required the same key, and monitoring of both aliases as was possible, the uniqueness of key was removed. The duplicate checking was occurring on the part of the mapped value, in some cases only.

With session tracking on the tx_isolation of importance to
connector frameworks, its important that tracking of tx_isolation
does get informed if a user `set session transaction_isolation=X`
as the alias for tx_isolation.

Rather than just implement this for one variable alias, it
is implemented for all aliases.

To assist with this the key hash of session_tracker is now made up of
the offset, and also a bitmask in the Sys_var_bit class.

As these values and their hashs are not unique, the session tracker
defination removes its uniqueness on the key. This is to preserve
compatibility as previously it was possible to track a variable
and its alias so this behaviour is preserved.

Rather than previously where the key of the hash was unique,
we still need to enforce the uniqueness of the value (m_svar) in the lookup.
This is to preserve the behaviour monitoring a duplicate variable
silently ignores duplicates. This is implemented in the
Session_sysvars_tracker::vars_list::insert.

The impacts of aliases are:
- If track one variable, its alias changes, you get a tracking
  change on the variable you are monitoring.
- If you track two aliased variables of each other, changing
  one variable with have two tracking events.
- If you are tracking *, you will get a change on the variable
  changed. If then you change its alias, you'll get two tracking
  events for both variable changes.

Other:
* Eliminate session tracking in bootstrap.
@grooverdan
Copy link
Member Author

closed via fa2faf4

@grooverdan grooverdan closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
2 participants