|From:||Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>|
|To:||Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org|
|Subject:||[Review] Effectiveness of enable_material = off|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Before getting to the administrivia of the patch review, I think it's
worth a bit of analysis on why the planner behaves as it does.
The example query:
explain analyze select * from
( select oid, * from pg_class order by oid) as c
( select * from pg_attribute a order by attrelid) as a
on c.oid = a.attrelid;
This produces a plan that looks like it ought not to need
materialization, since the inner path is just an indexscan. However,
the reason why it does so is this: at the time that the mergejoin is
being costed, the inner path is not just an indexscan, but rather a
SubqueryScan node which will be optimized out later (in setrefs).
In this type of situation the effect of enable_material=false with
this patch will obviously be to force it to use another join type if
possible, and it strikes me that this may actually be somewhat _less_
useful than the existing behaviour where enable_material only disables
"performance-optimization" uses of Materialize. (One can after all use
enable_mergejoin to force another join type.)
So on balance I don't see a strong reason to accept the patch; I'm not
convinced that it's not worse than the current behaviour. Anyone have
strong opinions on this?
The patch applies cleanly with patch but not with git apply, since it
has a spurious 'new file mode' line; how was it prepared? (there is a
thread discussing this problem)
No tests, but I wouldn't think any are needed for this.
No doc patch, which I don't think is OK; the current wording of the
docs seems more descriptive of the current behaviour, and at least a
small change to the wording ought to be considered if the patch is to
The code passes all tests and has the expected effect on plans, but
there is a slightly unexpected effect on the explain output; the
penalty cost is applied to the mergejoin and not to the materialize
node itself. Fixing this in create_mergejoin_plan would at least make
the explain output look less surprising.
Patch state -> waiting for author, though I suggest getting more
buy-in on accepting the change before spending time on the docs or
|Next Message||Jaime Casanova||2013-09-15 21:23:32||Re: Assertions in PL/PgSQL|
|Previous Message||Andrew Dunstan||2013-09-15 17:42:04||Re: Proposal: json_populate_record and nested json objects|