pgsql: pg_plan_advice: Invent DO_NOT_SCAN(relation_identifier). - Mailing list pgsql-committers

pg_plan_advice: Invent DO_NOT_SCAN(relation_identifier).

The premise of src/test/modules/test_plan_advice is that if we plan
a query once, generate plan advice, and then replan it using that
same advice, all of that advice should apply cleanly, since the
settings and everything else are the same. Unfortunately, that's
not the case: the test suite is the main regression tests, and
concurrent activity can change the statistics on tables involved
in the query, especially system catalogs. That's OK as long as it
only affects costing, but in a few cases, it affects which relations
appear in the final plan at all.

In the buildfarm failures observed to date, this happens because
we consider alternative subplans for the same portion of the query;
in theory, MinMaxAggPath is vulnerable to a similar hazard. In both
cases, the planner clones an entire subquery, and the clone has a
different plan name, and therefore different range table identifiers,
than the original. If a cost change results in flipping between one
of these plans and the other, the test_plan_advice tests will fail,
because the range table identifiers to which advice was applied won't
even be present in the output of the second planning cycle.

To fix, invent a new DO_NOT_SCAN advice tag. When generating advice,
emit it for relations that should not appear in the final plan at
all, because some alternative version of that relation was used
instead. When DO_NOT_SCAN is supplied, disable all scan methods for
that relation.

To make this work, we reuse a bunch of the machinery that previously
existed for the purpose of ensuring that we build the same set of
relation identifiers during planning as we do from the final
PlannedStmt. In the process, this commit slightly weakens the
cross-check mechanism: before this commit, it would fire whenever
the pg_plan_advice module was loaded, even if pg_plan_advice wasn't
actually doing anything; now, it will only engage when we have some
other reason to create a pgpa_planner_state. The old way was complex
and didn't add much useful test coverage, so this seems like an
acceptable sacrifice.

Discussion: http://postgr.es/m/CA+TgmoYuWmN-00Ec5pY7zAcpSFQUQLbgAdVWGR9kOR-HM-fHrA@mail.gmail.com
Reviewed-by: Lukas Fittl <lukas@fittl.com>

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6455e55b0da47255f332a96f005ba0dd1c7176c2

Modified Files
--------------
contrib/pg_plan_advice/README                    |   7 +
contrib/pg_plan_advice/expected/alternatives.out | 158 +++++++++++++++++
contrib/pg_plan_advice/expected/scan.out         |  17 +-
contrib/pg_plan_advice/meson.build               |   1 +
contrib/pg_plan_advice/pgpa_ast.c                |   6 +
contrib/pg_plan_advice/pgpa_ast.h                |   1 +
contrib/pg_plan_advice/pgpa_output.c             |  35 ++++
contrib/pg_plan_advice/pgpa_planner.c            | 213 +++++++++++++----------
contrib/pg_plan_advice/pgpa_planner.h            |  26 ++-
contrib/pg_plan_advice/pgpa_trove.c              |   1 +
contrib/pg_plan_advice/pgpa_walker.c             | 156 ++++++++++++++---
contrib/pg_plan_advice/pgpa_walker.h             |   1 +
contrib/pg_plan_advice/sql/alternatives.sql      |  58 ++++++
contrib/pg_plan_advice/sql/scan.sql              |   5 +-
doc/src/sgml/pgplanadvice.sgml                   |  14 +-
15 files changed, 581 insertions(+), 118 deletions(-)