Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Fix: Add ao_unique_index_build test in greenplum_schedule #562

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Fix: Add ao_unique_index_build test in greenplum_schedule #562

merged 1 commit into from
Aug 13, 2024

Conversation

lss602726449
Copy link
Contributor

Test ao_unique_index_build test is lost in PR: Feature: Porting unique index for AO table to CBDB.

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🥳

@soumyadeep2007 @lss602726449
ao/co: Address index build scan FIXME
d71c4b9 
By addressing the FIXME, this commit addresses 2 main issues:

* We have to let the index AM know if a tuple is deleted.
Failing to do the above can have consequences. There are some index AMs
that don't really do anything with this information (like bitmap
indexes). However, AMs like B-Tree rely on this information (specially
for uniqueness conflicts during unique index builds). Below is an
example of how things can go wrong if we bubble up incorrect visibility
information:

CREATE TABLE bar(i int) USING ao_row;
INSERT INTO bar VALUES(1),(1);
DELETE FROM bar WHERE i = 1;
CREATE UNIQUE INDEX ON bar(i);
ERROR:  could not create unique index "bar_i_idx"  (seg1 ...)
DETAIL:  Key (i)=(1) is duplicated.

* If a tuple is deleted, we shouldn't add it to the table's reltuples on
the segment. We were adding it regardless, leading to an incorrect value
of reltuples.

This commit introduces a visimap check to the table AM API for index
builds, to weed out deleted tuples, such that the visibility info and
reltuples calculation is correct.

Further, it adds coverage for the reltuples calculation for general
index builds and coverage for unique index builds.
@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.

@gfphoenix78
Copy link
Contributor

Is the patch backported from greenplum?

@reshke
Copy link
Contributor

reshke commented Aug 9, 2024

Is the patch backported from greenplum?

looks like greenplum-db/gpdb-archive@0c6e53f

reshke
reshke approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@reshke reshke left a comment

Choose a reason for hiding this comment

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

Coding changes looks good, however i didnt get why we omit
src/test/regress/input/uao_dml/ao_unique_index_build.source
src/test/regress/output/uao_dml/ao_unique_index_build.source files from original commit

@lss602726449
Copy link
Contributor Author

Is the patch backported from greenplum?

Yes, Its my fault to lost this PR when porting feature unique index.

@lss602726449
Copy link
Contributor Author

Coding changes looks good, however i didnt get why we omit src/test/regress/input/uao_dml/ao_unique_index_build.source src/test/regress/output/uao_dml/ao_unique_index_build.source files from original commit

Sorry for my mistake.

@reshke
Copy link
Contributor

reshke commented Aug 10, 2024

Test ao_unique_index_build test is lost in PR: Feature: Porting unique index for AO table to CBDB.

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](https://github.com/cloudberrydb/cloudberrydb/blob/main/.gitmessage) template as a reference.

* [ ]  Sign the Contributor License Agreement as prompted for your first-time contribution(_One-time setup_).

* [ ]  Learn the [coding contribution guide](https://cloudberrydb.org/contribute/code), including our code conventions, workflow and more.

* [ ]  List your communication in the [GitHub Issues](https://github.com/cloudberrydb/cloudberrydb/issues) or [Discussions](https://github.com/orgs/cloudberrydb/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🥳

You need to sing CLA to commit to this repo
Simply follow the link from comment
#562 (comment)

@tuhaihe
Copy link
Member

tuhaihe commented Aug 12, 2024

You need to sing CLA to commit to this repo Simply follow the link from comment #562 (comment)

Hi @reshke, thanks for the reminder. If the commits are cherry-picked from the upstream, there's no need to ask for the original author to sign the CLA. The CLA is one option that is not required for the pull request merging.

@avamingli avamingli added the cherry-pick cherry-pick upstream commts label Aug 12, 2024
@avamingli avamingli merged commit b9aec75 into apache:main Aug 13, 2024
10 of 11 checks passed
@avamingli
Copy link
Contributor

Thanks, pushed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@avamingli avamingli avamingli approved these changes

@reshke reshke reshke approved these changes

Assignees
No one assigned
Labels
cherry-pick cherry-pick upstream commts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lss602726449 @CLAassistant @gfphoenix78 @reshke @tuhaihe @avamingli @soumyadeep2007