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

PG16: Fix multinode deparsing issues #6183

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Oct 10, 2023

With the changes introduced in PG16 by moving the permission checking information out of the range table entries to a new data struct named RTEPermissionInfo we can't use the rte->updatedcols anymore for the target_attrs when deparsing, so we need to build new target_attrs based on the get_rel_all_updated_cols and for our multinode implementation we need to get rid the generated always attributes to don't risk to build the parameters in stmt_params_create using this column. Postgres FDW also have it own logic to skip generated columns as well.

Disable-check: force-changelog-file

@fabriziomello fabriziomello self-assigned this Oct 10, 2023
@fabriziomello fabriziomello added pg16 Issue/PR related to support for PG16 multinode labels Oct 10, 2023
@fabriziomello fabriziomello force-pushed the pg16-fix-multinode-deparse-issues branch from c501c66 to aced296 Compare October 10, 2023 19:16
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #6183 (df085d7) into main (6af0cb0) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #6183      +/-   ##
==========================================
- Coverage   81.45%   81.41%   -0.04%     
==========================================
  Files         246      246              
  Lines       56978    56938      -40     
  Branches    12626    12614      -12     
==========================================
- Hits        46409    46357      -52     
+ Misses       8191     8169      -22     
- Partials     2378     2412      +34     
Files Coverage Δ
tsl/src/fdw/deparse.c 43.30% <66.66%> (+0.01%) ⬆️
tsl/src/fdw/modify_plan.c 81.92% <83.33%> (-3.26%) ⬇️

... and 44 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fabriziomello fabriziomello force-pushed the pg16-fix-multinode-deparse-issues branch 3 times, most recently from cf70e49 to 1b2c6c0 Compare October 10, 2023 19:40
@fabriziomello fabriziomello marked this pull request as ready for review October 10, 2023 19:47
@github-actions github-actions bot requested review from erimatnor and konskov October 10, 2023 19:48
@github-actions
Copy link

@erimatnor, @konskov: please review this pull request.

Powered by pull-review

@fabriziomello fabriziomello force-pushed the pg16-fix-multinode-deparse-issues branch from 1b2c6c0 to 34a483d Compare October 10, 2023 20:34
Copy link
Contributor

@erimatnor erimatnor left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits and suggestions.

tsl/src/fdw/modify_plan.c Outdated Show resolved Hide resolved
tsl/src/fdw/modify_plan.c Outdated Show resolved Hide resolved
tsl/src/fdw/modify_plan.c Outdated Show resolved Hide resolved
tsl/src/fdw/modify_plan.c Show resolved Hide resolved
With the changes introduced in PG16 by moving the permission checking
information out of the range table entries to a new data struct named
`RTEPermissionInfo` we can't use the rte->updatedcols anymore for the
target_attrs when deparsing, so we need to build new target_attrs based
on the `get_rel_all_updated_cols` and for our multinode implementation
we need to get rid the generated always attributes to don't risk to
build the parameters in `stmt_params_create` using this column. Postgres
FDW also have it own logic to skip generated columns as well.

postgres/postgres@a61b1f74
@fabriziomello fabriziomello force-pushed the pg16-fix-multinode-deparse-issues branch from 34a483d to df085d7 Compare October 11, 2023 13:38
Copy link
Member

@svenklemm svenklemm left a comment

Choose a reason for hiding this comment

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

Based on the discussion we had today i dont think this should be merged.

@fabriziomello
Copy link
Contributor Author

Based on the discussion we had today i dont think this should be merged.

Ok, will send another PR blocking usage of distributed hypertables

@fabriziomello
Copy link
Contributor Author

Closing because we decided to don't support multinode on PG16. This PR #6190 remove the support of multinode on PG16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multinode pg16 Issue/PR related to support for PG16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants