Skip to content

Commit

Permalink
Always build a custom plan node's targetlist from the path's pathtarget.
Browse files Browse the repository at this point in the history
We were applying the use_physical_tlist optimization to all relation
scan plans, even those implemented by custom scan providers.  However,
that's a bad idea for a couple of reasons.  The custom provider might
be unable to provide columns that it hadn't expected to be asked for
(for example, the custom scan might depend on an index-only scan).
Even more to the point, there's no good reason to suppose that this
"optimization" is a win for a custom scan; whatever the custom provider
is doing is likely not based on simply returning physical heap tuples.
(As a counterexample, if the custom scan is an interface to a column store,
demanding all columns would be a huge loss.)  If it is a win, the custom
provider could make that decision for itself and insert a suitable
pathtarget into the path, anyway.

Per discussion with Dmitry Ivanov.  Back-patch to 9.5 where custom scan
support was introduced.  The argument that the custom provider can adjust
the behavior by changing the pathtarget only applies to 9.6+, but on
balance it seems more likely that use_physical_tlist will hurt custom
scans than help them.

Discussion: https://postgr.es/m/e29ddd30-8ef9-4da5-a50b-2bb7b8c7198d@postgrespro.ru
  • Loading branch information
tglsfdc committed Apr 17, 2017
1 parent 9e0e555 commit 76799fc
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/backend/optimizer/plan/createplan.c
Expand Up @@ -799,6 +799,15 @@ use_physical_tlist(PlannerInfo *root, Path *path, int flags)
if (rel->reloptkind != RELOPT_BASEREL)
return false;

/*
* Also, don't do it to a CustomPath; the premise that we're extracting
* columns from a simple physical tuple is unlikely to hold for those.
* (When it does make sense, the custom path creator can set up the path's
* pathtarget that way.)
*/
if (IsA(path, CustomPath))
return false;

/*
* Can't do it if any system columns or whole-row Vars are requested.
* (This could possibly be fixed but would take some fragile assumptions
Expand Down

0 comments on commit 76799fc

Please sign in to comment.