Fix plans counter - #820
Conversation
8253335 to
3bb3cd0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #820 +/- ##
==========================================
+ Coverage 88.46% 88.49% +0.02%
==========================================
Files 3 3
Lines 1318 1321 +3
Branches 187 188 +1
==========================================
+ Hits 1166 1169 +3
Misses 77 77
Partials 75 75 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3bb3cd0 to
531c38c
Compare
|
|
||
| RESET plan_cache_mode; | ||
| -- the generic plan is built once and then reused, so a replan forced by a | ||
| -- cache invalidation still leaves fewer plans than calls |
There was a problem hiding this comment.
Is this even true? Also how do you imagine a cache invalidation would force a replan? This looks like LLMed nonsense.
There was a problem hiding this comment.
This is indeed LLM slope. I haven't done proper review of that part. Reworked.
| ?column? | ||
| ---------- | ||
| 42 | ||
| (1 row) |
There was a problem hiding this comment.
No reason. Changed to 2.
| planned | fewer_plans_than_calls | min_plan_time_positive | calls | ||
| ---------+------------------------+------------------------+------- | ||
| t | t | t | 4 | ||
| (1 row) |
There was a problem hiding this comment.
Why do you check for that minimum plan time is positive?
There was a problem hiding this comment.
Before fix time counter may be decremented down to zero. I reworked this test to check only plan counter, I think that's enough.
| t | ||
| --- | ||
| t | ||
| (1 row) |
There was a problem hiding this comment.
Not sure why we need to reset stats.
There was a problem hiding this comment.
To not have collision with previous test case. Simple select and prepared are pretty much the same query and fills the same entry in PGSM.
| * src may carry more than one planning event (a statement can be planned | ||
| * several times before it is executed, e.g. by EXPLAIN); all of them are | ||
| * counted, but their accumulated time is still folded as one sample. | ||
| */ |
| if (src->plancalls.calls > 0) | ||
| { | ||
| double old_mean = dst->plantime.mean_time; | ||
| bool first_sample = (dst->plancalls.calls == 0); |
There was a problem hiding this comment.
Why not just move incrementing .calls until later and check if calls is zero directly in the if?
| - Make sure that for prepared statements utility statement exec info read at the executor start hook, where data is not yet modified by query itself | ||
| - Do not acquire LWLock under spinlock | ||
| - Race condition where we could leak memory for the parent query | ||
| - The `plans` counter counts how many times a statement was actually planned. |
There was a problem hiding this comment.
I think this should be clarified a bit. Also it should not end in full stop.
plans counter was incremented uncondidionally, even when there was no plan at all (utility statements). Fix it so it will reflect how many times planning events happened. Also this fixes associated timers.
531c38c to
2cab212
Compare
2 participants
plans counter was incremented unconditionally, even when there was no plan at all (utility statements). Fix it so it will reflect how many times planning events happened. Also this fixes associated timers.