Re: Get rid of runtime handling of AlternativeSubPlan? - Mailing list pgsql-hackers

On Tue, 1 Sep 2020 at 05:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > One inefficiency I see that we could probably get rid of is
> > where make_subplan() is doing
> >             /* Now we can check if it'll fit in hash_mem */
> >             /* XXX can we check this at the Path stage? */
>
> I went ahead and fixed that, and I also realized there's another small
> improvement to be made: we can remove the unused SubPlan from the
> subplans list of the finished PlannedStmt, by setting that list cell
> to NULL.  (This is already specifically allowed by the comments for
> PlannedStmt.subplans.)  Initially I supposed that this'd only save the
> costs of copying that subtree when we copy the whole plan.  On looking
> closer, though, InitPlan actually runs ExecInitNode on every list
> entry, even unused ones, so this will make some difference in executor
> startup time.
>
> Hence, an updated 0001 patch.  0002 hasn't changed.

I had a look over these two. A handful of very small things:

0001:

1. I think we should be moving away from using linitial() and second()
when we know there are two items in the list. Using list_nth() has
less overhead.

subplan1 = (SubPlan *) linitial(asplan->subplans);
subplan2 = (SubPlan *) lsecond(asplan->subplans);

2. I did have sight concerns that fix_alternative_subplan() always
assumes the list of subplans will always be 2, though on looking at
the definition of AlternativeSubPlan, I see always having two in the
list is mentioned. It feels like fix_alternative_subplan() wouldn't
become much more complex to allow any non-zero number of subplans, but
maybe making that happen should wait until there is some need for more
than two. It just feels a bit icky to have to document the special
case when not having the special case is not that hard to implement.

3. Wouldn't it be better to say NULLify rather than delete?

+ * node or higher-level nodes.  However, we do delete the rejected subplan
+ * from root->glob->subplans, to minimize cycles expended on it later.

0002:

I don't have much to say about this.  Leaving the code in
get_rule_expr() for the reasons you mentioned in the new comment does
make sense.


On a side note, I was playing around with the following case:

create table t (a int, b int, c int);
insert into t select x,1,2 from generate_Series(1,10000)x;
create index on t (b);
vacuum freeze analyze t;

and ran:

select * from t where exists (select 1 from t t2 where t.a=t2.b) or a < 0;

EXPLAIN ANALYZE shows:

                                                   QUERY PLAN
----------------------------------------------------------------------------------------------------------------
 Seq Scan on t  (cost=0.00..360.00 rows=5000 width=12) (actual
time=0.020..7468.452 rows=1 loops=1)
   Filter: ((SubPlan 1) OR (a < 0))
   Rows Removed by Filter: 9999
   SubPlan 1
     ->  Seq Scan on t t2  (cost=0.00..180.00 rows=10000 width=0)
(actual time=0.746..0.746 rows=0 loops=10000)
           Filter: (t.a = b)
           Rows Removed by Filter: 9999
 Planning Time: 0.552 ms
 Execution Time: 7468.481 ms
(9 rows)


Notice that the SubPlan's estimated rows are 10000. This is due to the
ndistinct for "b" being 1 and since t.a is a parameter, the
selectivity is estimated to be 1.0 by var_eq_non_const().
Unfortunately, for this reason, the index on t(b) is not used either.
The planner thinks all rows are being selected, in which case, an
index is not much help.

both master and patched seem to not choose to use the hashed subplan
which results in a pretty slow execution time. This seems to be down
to cost_subplan() doing:

/* we only need to fetch 1 tuple; clamp to avoid zero divide */
sp_cost.per_tuple += plan_run_cost / clamp_row_est(plan->plan_rows);

I imagine / 2 might be more realistic to account for the early abort,
which is pretty much what the ALL_SUBLINK and ANY_SUBLINK do just
below:

Changing that makes the run-time of that query go from 7.4 seconds for
me down to 3.7 ms, about 2000 times faster.

I understand there will be other cases where that's not so ideal, but
this slowness is not ideal either. Of course, not the fault of this
patch.

David