Обсуждение: Effectiveness of enable_material = off

I recently could not shift a plan off of using a materialize, to see
what other options were out there, by setting enable_material to off.

>From src/backend/optimizer/path/costsize.c:


     * We don't test the value of enable_material here, because
     * materialization is required for correctness in this case, and turning
     * it off does not entitle us to deliver an invalid plan.

True, but I don't think that the materialization being necessary for
correctness entitles us to ignore enable_material=off.  If we need to
return a plan with materialization in order for this plan to be
correct, it should be punished like all other violations of enable_*
are punished.

The attached patch does that.

Having done this, I wonder if the line here should also be changed to
remove the enable_material test:

    if (enable_material && mat_inner_cost < bare_inner_cost)

If the disable_cost is not enough to make the "mat_inner_cost <
bare_inner_cost" to be false, then I don't see that we need a special
case to prevent use of the materialize anyway, as most other enable_*
implementations do not get one--they rely solely on disable_cost.
However, out of conservatism, I have not made the change of removing
this.

The example I used for demonstrating this is from
http://www.depesz.com/2013/05/09/explaining-the-unexplainable-part-3/


explain analyze select * from
    ( select oid, * from pg_class order by oid) as c
    join
    ( select * from pg_attribute a order by attrelid) as a
    on c.oid = a.attrelid;

Why this can serve as an example is a bit mysterious to me anyway.
The ORDER BY is implemented via:
Index Scan using pg_attribute_relid_attnum_index on pg_attribute a

The index scan should support mark-restore, so why does
ExecSupportsMarkRestore return false in the first place?  (This is a
side question, not really important to the disposition of the proposed
patch).

Cheers,

Jeff

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   join   ( select * from pg_attribute
aorder 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?

Administrivia:

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
be accepted.

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
costing issue.

-- 
Andrew (irc:RhodiumToad)