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 visimap consults for unique checks during UPDATEs #423

Merged
merged 2 commits into from
May 8, 2024

Conversation

lss602726449
Copy link
Contributor

This fixes #17183.

When consulting the visimap during an UPDATE for the purposes of uniqueness checks, we used to refer to the visimap from the delete half of the update.

This is the wrong structure to look at as this structure is not meant to be consulted while deletes are in flight (we haven't reached end-of-delete where visibility info from the visimapDelete structure flows into the catalog).

Instead, we should be consulting the visimapDelete structure attached to the deleteDesc. This structure can handle visimap queries for tuples that have visimap changes that haven't yet been persisted to the catalog table.

The effect of not using this structure meant running into issues such as: "attempted to update invisible tuple" when we would attempt to persist a dirty visimap entry in AppendOnlyVisimap_IsVisible() with a call to AppendOnlyVisimap_Store(). The dirty entry is one which was introduced by the delete half of the update. Our existing test did not trip this issue because the update did not need a swap-out of the current entry. With enough data, however, the issue reproduces, as evidenced in #17183.

Reviewed-by: Haolin Wang whaolin@vmware.com

fix #ISSUE_Number


Change logs

Describe your change clearly, including what problem is being solved or what feature is being added.

If it has some breaking backward or forward compatibility, please clary.

Why are the changes needed?

Describe why the changes are necessary.

Does this PR introduce any user-facing change?

If yes, please clarify the previous behavior and the change this PR proposes.

How was this patch tested?

Please detail how the changes were tested, including manual tests and any relevant unit or integration tests.

Contributor's Checklist

Here are some reminders and checklists before/when submitting your pull request, please check them:

  • Make sure your Pull Request has a clear title and commit message. You can take git-commit template as a reference.
  • Sign the Contributor License Agreement as prompted for your first-time contribution(One-time setup).
  • Learn the coding contribution guide, including our code conventions, workflow and more.
  • List your communication in the GitHub Issues or Discussions (if has or needed).
  • Document changes.
  • Add tests for the change
  • Pass make installcheck
  • Pass make -C src/test installcheck-cbdb-parallel
  • Feel free to request cloudberrydb/dev team for review and approval when your PR is ready🥳

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

soumyadeep2007 and others added 2 commits May 8, 2024 13:47
This will be reused in a later commit.

Reviewed-by: Ashwin Agrawal <aashwin@vmware.com>
Reviewed-by: Haolin Wang <whaolin@vmware.com>
This fixes #17183.

When consulting the visimap during an UPDATE for the purposes of
uniqueness checks, we used to refer to the visimap from the delete half
of the update.

This is the wrong structure to look at as this structure is not meant to
be consulted while deletes are in flight (we haven't reached
end-of-delete where visibility info from the visimapDelete structure
flows into the catalog).

Instead, we should be consulting the visimapDelete structure attached to
the deleteDesc. This structure can handle visimap queries for tuples
that have visimap changes that haven't yet been persisted to the catalog
table.

The effect of not using this structure meant running into issues such
as: "attempted to update invisible tuple" when we would attempt to
persist a dirty visimap entry in AppendOnlyVisimap_IsVisible() with a
call to AppendOnlyVisimap_Store(). The dirty entry is one which was
introduced by the delete half of the update. Our existing test did not
trip this issue because the update did not need a swap-out of the
current entry. With enough data, however, the issue reproduces, as
evidenced in #17183.

Co-authored-by: Ashwin Agrawal <aashwin@vmware.com>
Reviewed-by: Haolin Wang <whaolin@vmware.com>
@avamingli avamingli added the cherry-pick cherry-pick upstream commts label May 8, 2024
@my-ship-it my-ship-it merged commit 80a0a07 into apache:main May 8, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick cherry-pick upstream commts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants