- Notifications
You must be signed in to change notification settings - Fork4.9k
Commit27cc7cd
committed
Reorder EPQ work, to fix rowmark related bugs and improve efficiency.
Inad0bda5 I changed the EvalPlanQual machinery to storesubstitution tuples in slot, instead of using plain HeapTuples. Themain motivation for that was that using HeapTuples will be inefficientfor future tableams. But it turns out that that conversion was buggyfor non-locking rowmarks - the wrong tuple descriptor was used tocreate the slot.As a secondary issue5db6df0 changed ExecLockRows() to begin EPQearlier, to allow to fetch the locked rows directly into the EPQslots, instead of having to copy tuples around. Unfortunately, as Tomcomplained, that forces some expensive initialization to happenearlier.As a third issue, the test coverage for EPQ was clearly insufficient.Fixing the first issue is unfortunately not trivial: Non-locked rowmarks were fetched at the start of EPQ, and we don't have the typeinformation for the rowmarks available at that point. While we couldchange that, it's not easy. It might be worthwhile to change that atsome point, but to fix this bug, it seems better to delay fetchingnon-locking rowmarks when they're actually needed, rather thaneagerly. They're referenced at most once, and in cases where EPQfails, might never be referenced. Fetching them when needed alsoincreases locality a bit.To be able to fetch rowmarks during execution, rather thaninitialization, we need to be able to access the active EPQState, asthat contains necessary data. To do so move EPQ related data fromEState to EPQState, and, only for EStates creates as part of EPQ,reference the associated EPQState from EState.To fix the second issue, change EPQ initialization to allow use ofEvalPlanQualSlot() to be used before EvalPlanQualBegin() (butobviously still requiring EvalPlanQualInit() to have been done).As these changes made struct EState harder to understand, e.g. byadding multiple EStates, significantly reorder the members, and add alot more comments.Also add a few more EPQ tests, including one that fails for the firstissue above. More is needed.Reported-By: yi huangAuthor: Andres FreundReviewed-By: Tom LaneDiscussion:https://postgr.es/m/CAHU7rYZo_C4ULsAx_LAj8az9zqgrD8WDd4hTegDTMM1LMqrBsg@mail.gmail.comhttps://postgr.es/m/24530.1562686693@sss.pgh.pa.usBackpatch: 12-, where the EPQ changes were introduced1 parent7e04160 commit27cc7cd
File tree
12 files changed
+705
-273
lines changed- src
- backend
- commands
- executor
- include
- executor
- nodes
- test/isolation
- expected
- specs
12 files changed
+705
-273
lines changedLines changed: 1 addition & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3363 | 3363 | | |
3364 | 3364 | | |
3365 | 3365 | | |
3366 | | - | |
3367 | | - | |
| 3366 | + | |
3368 | 3367 | | |
3369 | 3368 | | |
3370 | 3369 | | |
| |||
0 commit comments
Comments
(0)