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

Fix swap of relfrozenxid, relfrozenxid and relallvisible #377

Merged
merged 3 commits into from
May 21, 2024

Conversation

za-arthur
Copy link
Collaborator

@za-arthur za-arthur commented Jan 5, 2024

This PR is follow up of #157.

I think we still need to check the current state of swap_relation_files (https://github.com/postgres/postgres/blob/ae69c4fcf1337af399a788ab8d96af540bd1cd8e/src/backend/commands/cluster.c?plain=1#L1055). It has a few differences with the code of repack.c. We don't need to copy all the code, since not all features are supported by pg_repack. For example we don't repack system tables, mapped tables. And maybe we don't need to support swap_toast_by_content?

I'll try to come up with a PR to try to sync the code.

Copy link
Contributor

@MasahikoSawada MasahikoSawada left a comment

Choose a reason for hiding this comment

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

I agree with overall of these changes, but I have some minor comments.

lib/repack.c Outdated Show resolved Hide resolved
lib/repack.c Outdated
relform1->relfrozenxid = relform2->relfrozenxid;
relform2->relfrozenxid = swaptempxid;

swaptempxid = relform1->relminmxid;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use a MultiXactId variable instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used TransactionId mainly because relminmxid has that type, regardless of the comment in pg_class.h, stating that it is a MultiXactId.
I wonder why pg_class.h doesn't use MultiXactId for relminmxid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why pg_class.h doesn't use MultiXactId for relminmxid.

I think MultiXactId doesn't have the corresponding SQL data type unlike TransactionId. Data types that can be used in system catalog are somewhat limited. It makes sense to use MultiXactId internally since we use it in C functions.

@za-arthur
Copy link
Collaborator Author

Thank you @MasahikoSawada for the review! I pushed changes.

@MasahikoSawada
Copy link
Contributor

MasahikoSawada commented May 21, 2024

Thank you for updating the changes!

The changes look good to me. I'll merge it, barring any objections. Or if you can merge it by yourself, please go ahead.

@za-arthur
Copy link
Collaborator Author

The changes look good to me. I'll merge it, barring any objections. Or if you can merge it by yourself, please go ahead.

Thank you. I'm merging it.

@za-arthur za-arthur merged commit cfa429a into master May 21, 2024
19 checks passed
@za-arthur za-arthur deleted the fix_relfrozenxid branch May 21, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants