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

Custom Scan (ColumnarScan): exclude outer_join_rels from CandidateRelids #7703

Open
wants to merge 1 commit into
base: release-13.0
Choose a base branch
from

Conversation

OlgaSergeyevaB
Copy link

@OlgaSergeyevaB OlgaSergeyevaB commented Oct 14, 2024

DESCRIPTION: Fixes a crash in columnar custom scan that happens when a columnar table is used in a join.

Fixes #7647.

@ivan-v-kush
Copy link
Contributor

ivan-v-kush commented Jan 20, 2025

@OlgaSergeyevaB Olga, thanks for PR. You can take the tests from here.
https://github.com/hydradatabase/hydra/pull/279/files

outer_join_rels were introduced in postgres/postgres@2489d76

@ivan-v-kush
Copy link
Contributor

here is also one fix for distributed case
#7789

@OlgaSergeyevaB
Copy link
Author

here is also one fix for distributed case #7789
@ivan-v-kush, these are different cases

@OlgaSergeyevaB
Copy link
Author

@OlgaSergeyevaB Olga, thanks for PR. You can take the tests from here. https://github.com/hydradatabase/hydra/pull/279/files

outer_join_rels were introduced in postgres/postgres@2489d76

@ivan-v-kush, thanks for your tests. But its fall without sorting.

@OlgaSergeyevaB
Copy link
Author

@microsoft-github-policy-service agree

@colm-mchugh colm-mchugh linked an issue Feb 11, 2025 that may be closed by this pull request
@colm-mchugh
Copy link
Contributor

@OlgaSergeyevaB thank you for submitting this PR. Can you rebase it to the release-13.0 branch ? We would like to include this in Citus' next patch release.

@OlgaSergeyevaB
Copy link
Author

@colm-mchugh , I rebase it to the release-13.0 branch

@colm-mchugh
Copy link
Contributor

colm-mchugh commented Feb 15, 2025

@colm-mchugh , I rebase it to the release-13.0 branch

Can you squash your commits into 1 commit ? Then we can review. Thanks @OlgaSergeyevaB .

@colm-mchugh colm-mchugh changed the base branch from main to release-13.0 February 15, 2025 12:45
@OlgaSergeyevaB
Copy link
Author

@colm-mchugh, I squash my commits into 1 commit. Thanks @colm-mchugh

Copy link

codecov bot commented Feb 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.94%. Comparing base (c55bc8c) to head (45bdce7).
Report is 18 commits behind head on release-13.0.

❌ Your project check has failed because the head coverage (72.94%) is below the target coverage (87.50%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (c55bc8c) and HEAD (45bdce7). Click for more details.

HEAD has 81 uploads less than BASE
Flag BASE (c55bc8c) HEAD (45bdce7)
14_regress_check-pytest 1 0
16_regress_check-pytest 1 0
_upgrade 25 0
14_15_upgrade 1 0
15_regress_check-follower-cluster 1 0
16_17_upgrade 1 0
15_17_upgrade 1 0
15_16_upgrade 1 0
14_regress_check-columnar-isolation 1 0
17_regress_check-pytest 1 0
15_regress_check-pytest 1 0
16_regress_check-columnar-isolation 1 0
17_regress_check-follower-cluster 1 0
14_17_upgrade 1 0
14_regress_check-enterprise-isolation-logicalrep-3 1 0
15_regress_check-columnar-isolation 1 0
16_regress_check-follower-cluster 1 0
14_regress_check-follower-cluster 1 0
17_regress_check-query-generator 1 0
14_regress_check-enterprise-failure 1 0
16_regress_check-query-generator 1 0
14_regress_check-split 1 0
15_regress_check-enterprise 1 0
17_regress_check-columnar 1 0
14_regress_check-vanilla 1 0
14_regress_check-enterprise-isolation-logicalrep-1 1 0
15_regress_check-multi-mx 1 0
14_regress_check-enterprise-isolation 1 0
14_regress_check-query-generator 1 0
14_regress_check-enterprise-isolation-logicalrep-2 1 0
16_regress_check-operations 1 0
17_regress_check-operations 1 0
15_regress_check-operations 1 0
14_regress_check-operations 1 0
16_regress_check-isolation 1 0
15_regress_check-isolation 1 0
17_regress_check-isolation 1 0
14_regress_check-multi 1 0
14_regress_check-isolation 1 0
14_regress_check-multi-mx 1 0
15_cdc_installcheck 1 0
16_regress_check-multi-mx 1 0
16_cdc_installcheck 1 0
16_regress_check-multi-1 1 0
15_regress_check-multi-1 1 0
17_cdc_installcheck 1 0
14_regress_check-multi-1 1 0
14_regress_check-enterprise 1 0
14_regress_check-failure 1 0
17_regress_check-enterprise-isolation-logicalrep-1 1 0
15_regress_check-failure 1 0
17_regress_check-failure 1 0
14_regress_check-columnar 1 0
15_regress_check-multi 1 0
16_regress_check-multi 1 0
17_regress_check-multi 1 0
14_16_upgrade 1 0
Additional details and impacted files
@@                Coverage Diff                @@
##           release-13.0    #7703       +/-   ##
=================================================
- Coverage         89.48%   72.94%   -16.55%     
=================================================
  Files               276      276               
  Lines             60063    59873      -190     
  Branches           7524     7505       -19     
=================================================
- Hits              53747    43673    -10074     
- Misses             4166    13482     +9316     
- Partials           2150     2718      +568     

@onurctirtir onurctirtir changed the title fix #7647 Custom Scan (ColumnarScan): exclude outer_join_rels from Ca… Custom Scan (ColumnarScan): exclude outer_join_rels from CandidateRelids Feb 17, 2025
@colm-mchugh
Copy link
Contributor

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.94%. Comparing base (c55bc8c) to head (45bdce7).
Report is 18 commits behind head on release-13.0.

❌ Your project check has failed because the head coverage (72.94%) is below the target coverage (87.50%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (c55bc8c) and HEAD (45bdce7). Click for more details.
HEAD has 81 uploads less than BASE

Flag
BASE (c55bc8c)
HEAD (45bdce7)

@m3hm3t @naisila the fix in this PR looks good to me but I don't understand why codecov is failing - do you know what can we or the PR author do to have it go green ?

@OlgaSergeyevaB
Copy link
Author

@colm-mchugh , maybe I should add another test test

@naisila
Copy link
Member

naisila commented Feb 18, 2025

the fix in this PR looks good to me but I don't understand why codecov is failing - do you know what can we or the PR author do to have it go green ?

@colm-mchugh We can ignore that since I believe it doesn't take into account commits that are not part of the repository, but rather belong to a fork. I was looking at other community contributions to Citus, and in many cases codecov failed.

maybe I should add another test

@OlgaSergeyevaB even though codecov would still fail, please do include an EXPLAIN query, seems useful. Thanks! I am approving the PR as well.

@@ -1051,6 +1051,11 @@ FindCandidateRelids(PlannerInfo *root, RelOptInfo *rel, List *joinClauses)

candidateRelids = bms_del_members(candidateRelids, rel->relids);
candidateRelids = bms_del_members(candidateRelids, rel->lateral_relids);

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here for the relevant PG16 commit requiring this addition:
postgres/postgres@2489d76

Copy link
Author

Choose a reason for hiding this comment

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

@naisila , I add comment

@colm-mchugh
Copy link
Contributor

@colm-mchugh We can ignore that since I believe it doesn't take into account commits that are not part of the repository, but rather belong to a fork. I was looking at other community contributions to Citus, and in many cases codecov failed.

Thanks, sounds like a reasonable explanation, I've approved also.

maybe I should add another test

Yes @OlgaSergeyevaB +1 for the test. The EXPLAIN output might differ with different PG versions, but let's see.

@OlgaSergeyevaB
Copy link
Author

@naisila, @colm-mchugh , I add test

…s from CandidateRelids

For the relevant PG16 commit requiring this addition:
* postgres/postgres@2489d76

Add test from https://github.com/hydradatabase/hydra/pull/279/files
add order by for test and add explain
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.

postgresql is crashing when selecting from citus columnar table
4 participants