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 wrong memory context for allocation in op_rewrite.c #231

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

var77
Copy link
Collaborator

@var77 var77 commented Nov 30, 2023

The nodes were being allocated in MessageContext which doesn't live long enough and in some cases it was causing segfault when postgres was trying to access the nodes. Per source-code the plan tree is being allocated in PortalContext, so we also should allocate the overwritten nodes in PortalContext

Copy link

Benchmarks

metric old new pct change
recall (after create) 0.740 0.740 -
recall (after insert) 0.756 0.756 -
select bulk tps 481.514 493.884 +2.57%
select bulk latency (ms) 15.964 15.513 -2.83%
select bulk latency (stddev ms) 3.298 2.926 -11.28%
create latency (ms) 1217.728 1191.480 -2.16%
insert bulk tps 11.006 11.393 +3.51%
insert bulk latency (ms) 90.850 87.766 -3.39%
insert bulk latency (stddev ms) 3.735 2.286 -38.80%
disk usage (bytes) 6348800.000 6348800.000 -

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #231 (1407891) into main (b90926c) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #231   +/-   ##
=======================================
  Coverage   78.20%   78.20%           
=======================================
  Files          23       23           
  Lines        1647     1647           
  Branches      405      405           
=======================================
  Hits         1288     1288           
  Misses        178      178           
  Partials      181      181           
Files Coverage Δ
src/hooks/op_rewrite.c 83.94% <100.00%> (ø)

@Ngalstyan4
Copy link
Contributor

We should check and see whether the same change must be done here as well or no.

In there we a new Function Expression node.

@Ngalstyan4 Ngalstyan4 merged commit 0fa6925 into main Dec 1, 2023
@Ngalstyan4 Ngalstyan4 deleted the varik/fix-op-rewrite-mem-bug branch December 1, 2023 04:29
@var77
Copy link
Collaborator Author

var77 commented Dec 1, 2023

Sorry actually I made the change in this PR for Function Expression, the on for List Node is here

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.

2 participants