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

Extra Result node on top of CustomScan on PG15 #4928

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

sb230132
Copy link
Contributor

@sb230132 sb230132 commented Nov 7, 2022

On PG15 CustomScan by default is not projection capable, thus wraps this node in Result node. This change in PG15 causes tests result files which have EXPLAIN output to fail. This patch fixes the plan outputs.

Fixes #4833

@sb230132 sb230132 self-assigned this Nov 7, 2022
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #4928 (8921c67) into main (6c73b61) will decrease coverage by 0.02%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4928      +/-   ##
==========================================
- Coverage   89.51%   89.49%   -0.02%     
==========================================
  Files         226      226              
  Lines       50818    50826       +8     
==========================================
- Hits        45490    45487       -3     
- Misses       5328     5339      +11     
Impacted Files Coverage Δ
src/chunk.h 100.00% <ø> (ø)
src/nodes/chunk_append/planner.c 93.08% <ø> (ø)
src/chunk.c 91.07% <16.66%> (-0.09%) ⬇️
src/bgw/job.c 93.23% <50.00%> (-0.75%) ⬇️
src/process_utility.c 90.17% <100.00%> (ø)
src/bgw/scheduler.c 83.63% <0.00%> (-1.80%) ⬇️
tsl/src/nodes/data_node_dispatch.c 93.03% <0.00%> (-0.22%) ⬇️
tsl/src/bgw_policy/job.c 88.31% <0.00%> (-0.05%) ⬇️
src/loader/bgw_message_queue.c 88.63% <0.00%> (+2.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b955765...8921c67. Read the comment docs.

@sb230132 sb230132 marked this pull request as ready for review November 7, 2022 05:29
@sb230132 sb230132 force-pushed the pg15_fix_tests branch 2 times, most recently from 505a201 to a0486d2 Compare November 7, 2022 08:59
Copy link
Member

@akuzm akuzm left a comment

Choose a reason for hiding this comment

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

Sven explained the difference with earlier PG versions:

On PG <= 14 postgres will modify targetlists of any custom nodes which requires us to fix up targetlists after planning to not end up with broken plans. On PG 15 when the custom node is not marked as projection capable postgres will not mess with the targetlist of the custom node and instead inject a result node if any projection needs to happen. This means we no longer have to do the post-planning fixup. Those targetlist modifications of postgres were source of lots of subtle bugs.

So we want to actually disable these TL modifications to avoid bug-prone code, at least in the future.

@sb230132 sb230132 force-pushed the pg15_fix_tests branch 2 times, most recently from a18af79 to 943bdce Compare November 7, 2022 14:51
On PG15 CustomScan by default is not projection capable, thus wraps this
node in Result node. THis change in PG15 causes tests result files which
have EXPLAIN output to fail. This patch fixes the plan outputs.

Fixes timescale#4833
@sb230132 sb230132 enabled auto-merge (rebase) November 7, 2022 15:00
@sb230132 sb230132 merged commit 3a9688c into timescale:main Nov 7, 2022
@sb230132 sb230132 deleted the pg15_fix_tests branch November 7, 2022 16:09
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.

Extra Result node on top of CustomScan on PG15
3 participants