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

PG-1222 Fix bug where we confuse relations with the same RelFileNumber in different databases #60

Open
wants to merge 5 commits into
base: TDE_REL_17_STABLE
Choose a base branch
from

Conversation

jeltz
Copy link
Collaborator

@jeltz jeltz commented Feb 12, 2025

This fixes bug PG-1222 which was caused by the relation key cache only having the RelFileNumber as key when looking up relations. The RelFileNumber is only unique per database. On the other hand we did not have the same bug with keys stored on disk since there we used one file per database so there I instead refactored the code to make it more obvious that is what we are doing.

While at it I also removed a layer of indirection in to tde_re_file_cache to simplify the code.

The same comment is also at GetRelationKey() where it should be.
Removes an unnecessary level of indirection which simplifies the code
and in theory should give a miniscule perfomrance improvement. While at
it we also remove a usless NULL check which could never happen plus move
at variable declaration to another scope.
This fixes a bug where if the same RelFileNumber was used in two
databases we could confuse the two relations and use the wrong key,
which could result in a corrupted database.
Instead of recalculating the path when we get an error we just pass
it along to the function. Less code and more correct.
Since the maps on file are per database and only store the RelFileNumber
it is confusing if we pass the whole locator to these functions. This
is unlike the in-memory cache which is shared between all databases so
the keys need to be both database oid and RelFileNumber.
@jeltz jeltz requested review from dutow and dAdAbird as code owners February 12, 2025 13:45
@it-percona-cla
Copy link

it-percona-cla commented Feb 12, 2025

CLA assistant check
All committers have signed the CLA.

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.

3 participants