Utilize Postgres 19 plan generation strategies & rework parallel hints#234
Utilize Postgres 19 plan generation strategies & rework parallel hints#234lfittl wants to merge 11 commits intoossc-db:masterfrom
Conversation
The setting has been removed in upstream Postgres commit 45762084545e, now always defaulting to on.
This was causing incorrect behaviours when applying both a join type hint (e.g. NestLoop) and a Memoize hint.
The "available indexes" information was sometimes incomplete, for example when the restriction wasn't used because all indexes were ignored. This inconsistency becomes a problem for evaluating a future change's impact on regression tests, so make sure we use the actual index list the planner uses for the debug output.
Previously this hint relied on setting parallel_*_cost to zero to get Postgres to consistently chose a parallel path. This had unexpected side effects when it came to placing the Gather node on top of or below a join node - the existing code caused Gather to be placed below the join node often due to the zero cost settings. Additionally zero cost values make it hard to use pg_hint_plan for understanding the regular cost of a particular query plan in debugging. Instead, use the disabled node mechanism and turn off non-partial paths (i.e. those *not* used by parallel query) if "hard" is specified for the Parallel hint. This is in anticipation of an future commit that relies on a new API on Postgres to have the same effect. For historic reasons, this commit keeps non-partial paths in place if they are a sequential scan on an empty relation. This will be removed in a subsequent commit.
Distinguishing this from index scans on empty relations with the new path generation logic seems challenging, and its not clear why this existed in the first place.
Previously the "max_parallel_workers_per_gather" GUC was utilized for setting the number of parallel workers should be utilized for paths on a given relation. But modifying the GUC like that is brittle, and the planner already has an existing mechanism to influence the number of parallel workers, through RelOptInfo's rel_parallel_workers field. Because that field is still clamped by the current max_p_w_p_gather setting, we now increase the setting in effect for the whole query if any of the parallel worker hints specify a higher worker count than is currently allowed. As part of this change, also remove the modification of the per-path parallel_workers value after base rel paths are generated (its no longer necessary), but keep the logic that adjusts join rels and append rels to have a parallel_workers that is the highest of the children, since Postgres doesn't do that on its own, and it may be useful to keep that historic behaviour.
This replaces the previous mechanism of modifying GUC settings during the planning process, with the new plan generation strategy (PGS) mechanism in upstream commit 4020b370f214. Instead of GUCs this uses the new "pgs_mask" field to control which paths get generated during planning for a given base or join rel. This mask is set using the existing get_relation_info_hook, as well as the newly added joinrel_setup_hook / join_path_setup_hook to influence joinrels. Minor regression test output changes due to the difference in the order of how joins are influenced.
This completes the transition to the plan generation strategy mechanism, and completely drops copying of core planner code, as well as the repeat invocation of set_plain_rel_pathlist. Because this avoids double processing of certain internal lists in set_plain_rel_pathlist, some regression tests results have cosmetic changes in their output.
Sample scans can never be partial.
Thanks for doing this stuff, Lukas. Yes, I was planning to wait for the feature freeze of v19 to take effect before diving too much into that. Which patches in this set would you qualify as preparatory work? |
I think the following commits could be done independently to help reduce the diff (and I think they're pretty non-controversial), you'd just have to test against a Postgres commit before the plan generation change went in, e.g. e6d6e32f4240.
The same applies for the "parallel hint" changes, but those are a bit more "is this the right thing to do?", so I could open a separate PR for that to help with discussing it, if you'd like (when looked at separately, you'd have to also test against e6d6e32f4240 or earlier to have the tests pass). |
Well, yes. I need to clean up that anyway. I am just waiting for the feature freeze to evaluate the total amount of the damages, to avoid doing twice the same amount of work. It is true that some things may have to change during the v19 beta cycle, but that cannot be avoided. Point updates while a release cycle is still processing are basically pointless for an extension module of this size.
Hmm. Why do you think that the change with arbiter indexes in INDEX ON CONFLICT matters? Perhaps you mean a different commit? |
Sounds good, let's park this PR until then! In case anything needs more discussion, happy to also talk post-feature freeze at PGConf.Dev.
To clarify, I just referenced e6d6e32f4240 because its the last commit before the plan generation strategies commit upstream (4020b370f21) - in case you wanted to test the earlier changes here independently. Just to note that down (for future reference in mid-April), in terms of how to test this PR: Utilize PG19 plan generation strategies to affect plans needs to go along with having at least 4020b370f21 on the Postgres side. Earlier changes are intended to be tested against the commit before that. |
You got me confused for a couple of minutes. 4020b370f21 makes sense, of course. |
2 participants
With the upstream commit 4020b370f21 for plan generation strategies for Postgres 19 being in-tree for a week now, I wanted to share the latest patches of how I think it would make sense for pg_hint_plan to adapt to Postgres 19. Making a change is necessary because the previous GUC based mechanism no longer works - most planner GUCs are now only read at the beginning of planning.
There are a couple of preparatory patches that could be merged anytime, but I'd assume we'd want to hold off on the main commits until feature freeze / beta1 in the off-chance it gets reverted. For now, parking this here to avoid duplicated work.
Let me know if you'd like me to split this up differently for easier merging. Regression test changes are with their respective commits, so its easy to follow along which change changed which tests.