Horton

Thread

Commits

  1. Move resolution of AlternativeSubPlan choices to the planner.

  2. Arrange to convert EXISTS subqueries that are equivalent to hashable IN

  1. Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-22T00:20:25Z

    Back in bd3daddaf232d95b0c9ba6f99b0170a0147dd8af, which introduced
AlternativeSubPlans, I wrote:
    
  There is a lot more that could be done based on this infrastructure: in
  particular it's interesting to consider switching to the hash plan if we start
  out using the non-hashed plan but find a lot more upper rows going by than we
  expected.  I have therefore left some minor inefficiencies in place, such as
  initializing both subplans even though we will currently only use one.

That commit will be twelve years old come August, and nobody has either
built anything else atop it or shown any interest in making the plan choice
switchable mid-run.  So it seems like kind of a failed experiment.

Therefore, I'm considering the idea of ripping out all executor support
for AlternativeSubPlan and instead having the planner replace an
AlternativeSubPlan with the desired specific SubPlan somewhere late in
planning, possibly setrefs.c.

Admittedly, the relevant executor support only amounts to a couple hundred
lines, but that's not nothing.  A perhaps-more-useful effect is to get rid
of the confusing and poorly documented EXPLAIN output that you get for an
AlternativeSubPlan.

I also noted that the existing subplan-selection logic in
ExecInitAlternativeSubPlan is really pretty darn bogus, in that it uses a
one-size-fits-all execution count estimate of parent->plan->plan_rows, no
matter which subexpression the subplan is in.  This is only appropriate
for subplans in the plan node's targetlist, and can be either too high
or too low elsewhere.  It'd be relatively easy for setrefs.c to do
better, I think, since it knows which subexpression it's working on
at any point.

Thoughts?

			regards, tom lane

  2. Re: Get rid of runtime handling of AlternativeSubPlan?

    David Rowley <dgrowleyml@gmail.com> — 2020-06-22T01:29:31Z

    On Mon, 22 Jun 2020 at 12:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Back in bd3daddaf232d95b0c9ba6f99b0170a0147dd8af, which introduced
> AlternativeSubPlans, I wrote:
>
>   There is a lot more that could be done based on this infrastructure: in
>   particular it's interesting to consider switching to the hash plan if we start
>   out using the non-hashed plan but find a lot more upper rows going by than we
>   expected.  I have therefore left some minor inefficiencies in place, such as
>   initializing both subplans even though we will currently only use one.
>
> That commit will be twelve years old come August, and nobody has either
> built anything else atop it or shown any interest in making the plan choice
> switchable mid-run.  So it seems like kind of a failed experiment.
>
> Therefore, I'm considering the idea of ripping out all executor support
> for AlternativeSubPlan and instead having the planner replace an
> AlternativeSubPlan with the desired specific SubPlan somewhere late in
> planning, possibly setrefs.c.
>
> Admittedly, the relevant executor support only amounts to a couple hundred
> lines, but that's not nothing.  A perhaps-more-useful effect is to get rid
> of the confusing and poorly documented EXPLAIN output that you get for an
> AlternativeSubPlan.
>
> I also noted that the existing subplan-selection logic in
> ExecInitAlternativeSubPlan is really pretty darn bogus, in that it uses a
> one-size-fits-all execution count estimate of parent->plan->plan_rows, no
> matter which subexpression the subplan is in.  This is only appropriate
> for subplans in the plan node's targetlist, and can be either too high
> or too low elsewhere.  It'd be relatively easy for setrefs.c to do
> better, I think, since it knows which subexpression it's working on
> at any point.

When I was working on [1] a few weeks ago, I did wonder if I'd have to
use an AlternativeSubPlan when doing result caching for subqueries.
The problem is likely the same as why they were invented in the first
place; we basically don't know how many rows the parent will produce
when planning the subplan.

For my case, I have an interest in both the number of rows in the
outer plan, and the ndistinct estimate on the subplan parameters.  If
the parameters for the subquery are all distinct, then there's not
much sense in trying to cache results to use later. We're never going
to need them.

Right now, if I wanted to use AlternativeSubPlan to delay the choice
of this until run-time, then I'd be missing information about the
ndistinct estimation since we don't have that information available in
the final plan. Perhaps that's an argument for doing this in setrefs.c
instead. I could look up the ndistinct estimate there.

For switching plans on the fly during execution. I can see the sense
in that as an idea.  For the hashed subplan case, we'd likely want to
switch to hashing mode if we discovered that there were many more rows
in the outer query than we had thought there would be.   However, I'm
uncertain if Result Cache would never need anything similar as
technically we could just switch off the caching if we discovered our
cache hit ration was either terrible or 0.  We would have an
additional node to pull tuples through, however.   Switching would
also require that the tupleslot type was the same between the
alternatives.

David

[1] https://www.postgresql.org/message-id/CAApHDvrPcQyQdWERGYWx8J+2DLUNgXu+fOSbQ1UscxrunyXyrQ@mail.gmail.com

  3. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-29T23:26:36Z

    I wrote:
> Back in bd3daddaf232d95b0c9ba6f99b0170a0147dd8af, which introduced
> AlternativeSubPlans, I wrote:
>   There is a lot more that could be done based on this infrastructure: in
>   particular it's interesting to consider switching to the hash plan if we start
>   out using the non-hashed plan but find a lot more upper rows going by than we
>   expected.  I have therefore left some minor inefficiencies in place, such as
>   initializing both subplans even though we will currently only use one.
>
> That commit will be twelve years old come August, and nobody has either
> built anything else atop it or shown any interest in making the plan choice
> switchable mid-run.  So it seems like kind of a failed experiment.
>
> Therefore, I'm considering the idea of ripping out all executor support
> for AlternativeSubPlan and instead having the planner replace an
> AlternativeSubPlan with the desired specific SubPlan somewhere late in
> planning, possibly setrefs.c.

Here's a proposed patchset for that.  This runs with the idea I'd had
that setrefs.c could be smarter than the executor about which plan node
subexpressions will be executed how many times.  I did not take it very
far, for fear of adding an undue number of planning cycles, but it's still
better than what we have now.

For ease of review, 0001 adds the new planner logic, while 0002 removes
the now-dead executor support.

There's one bit of dead code that I left in place for the moment, which is
ruleutils.c's support for printing AlternativeSubPlans.  I'm not sure if
that's worth keeping or not --- it's dead code for normal use, but if
someone tried to use ruleutils.c to print partially-planned expression
trees, maybe there'd be a use for it?

(It's also arguable that readfuncs.c's support is now dead code, but
I have little interest in stripping that out.)

			regards, tom lane

  4. Re: Get rid of runtime handling of AlternativeSubPlan?

    Andy Fan <zhihui.fan1213@gmail.com> — 2020-08-31T00:23:53Z

    On Sun, Aug 30, 2020 at 7:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > Back in bd3daddaf232d95b0c9ba6f99b0170a0147dd8af, which introduced
> > AlternativeSubPlans, I wrote:
> >   There is a lot more that could be done based on this infrastructure: in
> >   particular it's interesting to consider switching to the hash plan if
> we start
> >   out using the non-hashed plan but find a lot more upper rows going by
> than we
> >   expected.  I have therefore left some minor inefficiencies in place,
> such as
> >   initializing both subplans even though we will currently only use one.
> >
> > That commit will be twelve years old come August, and nobody has either
> > built anything else atop it or shown any interest in making the plan
> choice
> > switchable mid-run.  So it seems like kind of a failed experiment.
> >
> > Therefore, I'm considering the idea of ripping out all executor support
> > for AlternativeSubPlan and instead having the planner replace an
> > AlternativeSubPlan with the desired specific SubPlan somewhere late in
> > planning, possibly setrefs.c.
>
> Here's a proposed patchset for that.  This runs with the idea I'd had
> that setrefs.c could be smarter than the executor about which plan node
> subexpressions will be executed how many times.  I did not take it very
> far, for fear of adding an undue number of planning cycles, but it's still
> better than what we have now.
>
> For ease of review, 0001 adds the new planner logic, while 0002 removes
> the now-dead executor support.
>
> There's one bit of dead code that I left in place for the moment, which is
> ruleutils.c's support for printing AlternativeSubPlans.  I'm not sure if
> that's worth keeping or not --- it's dead code for normal use, but if
> someone tried to use ruleutils.c to print partially-planned expression
> trees, maybe there'd be a use for it?
>
> (It's also arguable that readfuncs.c's support is now dead code, but
> I have little interest in stripping that out.)
>
>                         regards, tom lane
>
>
Thank you for this code!  I still have some confusion about when a SubPlan
should be executed when a join is involved.  I care about this because this
has an impact on when we can get the num_exec for a subplan.

The subplan in a target list,  it is executed after the join in my case.
The subplan
can be execute after the scan of T1(see below example) and it can also be
executed
after the join. Which one is better depends on which methods make the
num_exec
smaller.  Is it something we already considered? I drill-down to
populate_joinrel_with_paths and not find this logic.

# explain (costs off) select (select a from t2 where t2.b = t1.b) from t1,
t3;
          QUERY PLAN
------------------------------
 Nested Loop
   ->  Seq Scan on t1
   ->  Materialize
         ->  Seq Scan on t3
   SubPlan 1
     ->  Seq Scan on t2
           Filter: (b = t1.b)
(7 rows)


When the subplan is in a Qual, it is supposed to be executed as soon as
possible,
The current implementation matches the below cases.  So can we say we
knows the num_execs of SubPlan just after we plan the dependent rels?
(In Q1 below the dependent rel is t1 vs t3,  in Q2 it is t1 only) If we can
choose
a subplan and recost the related path during(not after) creating the best
path,  will
we get better results for some cases (due to the current cost method for
AlternativeSubPlan[1])?

-- the subplan depends on the result of t1 join t3
# explain (costs off) select t1.* from t1, t3 where
   t1.a > (select max(a) from t2 where t2.b = t1.b and t2.c  = t3.c);
                     QUERY PLAN
-----------------------------------------------------
 Nested Loop
   Join Filter: (t1.a > (SubPlan 1))
   ->  Seq Scan on t1
   ->  Materialize
         ->  Seq Scan on t3
   SubPlan 1
     ->  Aggregate
           ->  Seq Scan on t2
                 Filter: ((b = t1.b) AND (c = t3.c))
(9 rows)

-- the subplan only depends on t1.
# explain (costs off) select t1.* from t1, t3 where
t1.a > (select max(a) from t2 where t2.b = t1.b);
                   QUERY PLAN
------------------------------------------------
 Nested Loop
   ->  Seq Scan on t3
   ->  Materialize
         ->  Seq Scan on t1
               Filter: (a > (SubPlan 1))
               SubPlan 1
                 ->  Aggregate
                       ->  Seq Scan on t2
                             Filter: (b = t1.b)
(9 rows)


At last,  I want to use the commonly used table
in src/test/regress/sql/create_table.sql
when providing an example, but I always have issues running the
create_table.sql which
makes me uncomfortable to use that. Am I missing something?

CREATE TABLE fail_part PARTITION OF range_parted3 FOR VALUES FROM (1,
minvalue) TO (1, maxvalue);
psql:src/test/regress/sql/create_table.sql:611: ERROR:  partition
"fail_part" would overlap partition "part10"

CREATE TABLE fail_part PARTITION OF hash_parted2 FOR VALUES WITH (MODULUS
2, REMAINDER 1);
psql:src/test/regress/sql/create_table.sql:622: ERROR:  partition
"fail_part" would overlap partition "h2part_4"

[1]
https://www.postgresql.org/message-id/07b3fa88-aa4e-2e13-423d-8389eb1712cf%40imap.cc


-- 
Best Regards
Andy Fan

  5. Re: Get rid of runtime handling of AlternativeSubPlan?

    David Rowley <dgrowleyml@gmail.com> — 2020-08-31T02:35:32Z

    On Sun, 30 Aug 2020 at 11:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Therefore, I'm considering the idea of ripping out all executor support
> > for AlternativeSubPlan and instead having the planner replace an
> > AlternativeSubPlan with the desired specific SubPlan somewhere late in
> > planning, possibly setrefs.c.
>
> Here's a proposed patchset for that.

Do you feel that the choice to create_plan() on the subplan before
planning the outer query is still a good one?  ISTM that that was
required when the AlternativeSubplan decision was made during
execution, since we, of course, need a plan to execute. If the
decision is now being made in the planner then is it not better to
delay the create_plan() until later in planning?

From looking at the code it seems that Paths won't really do here as
we're dealing with two separate PlannerInfos rather than two paths
belonging to the same PlannerInfo, but maybe it's better to invent
something else that's similar to a list of paths and just do
create_plan() for the subquery once.

David

  6. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-31T15:41:46Z

    David Rowley <dgrowleyml@gmail.com> writes:
> Do you feel that the choice to create_plan() on the subplan before
> planning the outer query is still a good one?  ISTM that that was
> required when the AlternativeSubplan decision was made during
> execution, since we, of course, need a plan to execute. If the
> decision is now being made in the planner then is it not better to
> delay the create_plan() until later in planning?

Hm.  That's well outside the scope I had in mind for this patch.
In principle, you're right that we could postpone final planning
of the subquery till later; but I fear it'd require quite a lot
of refactoring to make it work that way.  There's a lot of rather
subtle timing dependencies in the processing done by createplan.c
and setrefs.c, so I think this might be a lot more painful than
it seems at first glance.  And we'd only gain anything in cases that
use AlternativeSubPlan, which is a minority of subplans, so on the
whole I rather doubt it's worth the trouble.

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? */
            if (subplan_is_hashable(plan))
            {

The only inputs subplan_is_hashable needs are the predicted rowcount
and output width, which surely we could get from the Path.  So we
could save doing create_plan() when we decide the subquery output is
too big to hash.  OTOH, that's probably a pretty small minority of
use-cases, so it might not be worth troubling over.

			regards, tom lane

  7. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-31T17:22:11Z

    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.

			regards, tom lane

  8. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-31T17:42:15Z

    Andy Fan <zhihui.fan1213@gmail.com> writes:
> Thank you for this code!  I still have some confusion about when a SubPlan
> should be executed when a join is involved.  I care about this because this
> has an impact on when we can get the num_exec for a subplan.

> The subplan in a target list,  it is executed after the join in my case.
> The subplan
> can be execute after the scan of T1(see below example) and it can also be
> executed
> after the join. Which one is better depends on which methods make the
> num_exec
> smaller.  Is it something we already considered?

Uh, I'm not following your concern.  SubPlans appearing in the join
targetlist *must* be executed "after the join", ie only for valid
join rows.  Otherwise we could have cases where, say, they throw
errors that should not occur.  On the other hand, SubPlans appearing
in the join's qual conditions have to be executed "before the join",
although exactly what that means is fuzzy because we don't make any
promises about the relative ordering of different qual conditions.

> When the subplan is in a Qual, it is supposed to be executed as soon as
> possible,
> The current implementation matches the below cases.  So can we say we
> knows the num_execs of SubPlan just after we plan the dependent rels?

I wouldn't say so.  If the SubPlan's qual actually only depends on one
of the input rels, distribute_qual_to_rels would have pushed it down
further than the join.  Among the quals that do have to be evaluated
at the join, a qual involving a SubPlan is best executed last on cost
grounds, or so I'd guess anyway.  So the number of executions is probably
less than the product of the input rel sizes.  That's what motivates
the choice of NUM_EXEC_QUAL in my patch.

			regards, tom lane

  9. Re: Get rid of runtime handling of AlternativeSubPlan?

    Andy Fan <zhihui.fan1213@gmail.com> — 2020-09-01T00:10:35Z

    On Tue, Sep 1, 2020 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Andy Fan <zhihui.fan1213@gmail.com> writes:
> > Thank you for this code!  I still have some confusion about when a
> SubPlan
> > should be executed when a join is involved.  I care about this because
> this
> > has an impact on when we can get the num_exec for a subplan.
>
> > The subplan in a target list,  it is executed after the join in my case.
> > The subplan
> > can be execute after the scan of T1(see below example) and it can also be
> > executed
> > after the join. Which one is better depends on which methods make the
> > num_exec
> > smaller.  Is it something we already considered?
>
> Uh, I'm not following your concern.  SubPlans appearing in the join
> targetlist *must* be executed "after the join", ie only for valid
> join rows.  Otherwise we could have cases where, say, they throw
> errors that should not occur.


I am feeling I'm wrong somewhere however I can't figure it out until now.

Q1:  select (select t3.a from t3 where t3.c = t1.c) from t1, t2 where t1.b
= t2.b;

should equals Q2:

1. select (select t3.a from t3 where t3.c = t1.c) as a, b from t1  ==>
t13.
2. select t13.a from t13, t2 where t13.b = t2.b;

With the following data,  Q1 will execute the subplan twice (since we get 2
rows
after join t1, t2).  while Q2 executes the subplan once (since t1 has only
1 row).
Finally the result is the same.

postgres=# select * from t1;
 a | b | c
---+---+---
 1 | 1 | 1
(1 row)

postgres=# select * from t2;
 a | b | c
---+---+---
 1 | 1 | 1
 1 | 1 | 2
(2 rows)

postgres=# select * from t3;
 a | b | c
---+---+---
 1 | 1 | 1
(1 row)

On the other hand, SubPlans appearing
> in the join's qual conditions have to be executed "before the join",
> although exactly what that means is fuzzy because we don't make any
> promises about the relative ordering of different qual conditions.
>
> > When the subplan is in a Qual, it is supposed to be executed as soon as
> > possible,
> > The current implementation matches the below cases.  So can we say we
> > knows the num_execs of SubPlan just after we plan the dependent rels?
>
> I wouldn't say so.  If the SubPlan's qual actually only depends on one
> of the input rels, distribute_qual_to_rels would have pushed it down
> further than the join. Among the quals that do have to be evaluated at the
> join,

a qual involving a SubPlan is best executed last on cost
> grounds, or so I'd guess anyway.  So the number of executions is probably

less than the product of the input rel sizes.  That's what motivates
> the choice of NUM_EXEC_QUAL in my patch.


Understand now.  Thank you!

-- 
Best Regards
Andy Fan

  10. Re: Get rid of runtime handling of AlternativeSubPlan?

    David Rowley <dgrowleyml@gmail.com> — 2020-09-26T15:56:35Z

    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

  11. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-26T21:03:27Z

    Thanks for reviewing!

David Rowley <dgrowleyml@gmail.com> writes:
> 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.

Uh, really?  And if it's true, why would we change all the call sites
rather than improving the pg_list.h macros?

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

It seemed to me that dealing with the general case would make
fix_alternative_subplan() noticeably more complex and less obviously
correct.  I might be wrong though; what specific coding did you have in
mind?

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

Fair enough, that comment could be improved.

> On a side note, I was playing around with the following case:
> ...
> 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:

Hm, actually isn't it the other way around?  *If* there are any matching
rows, then what's being done here is an accurate estimate.  But if there
are not, we're going to have to scan the entire subquery output to verify
that.  I wonder if we should just be taking the subquery cost at face
value, ie be pessimistic not optimistic.  If the user is bothering to
test EXISTS, we should expect that the no-match case does happen.

However, I think that's a distinct concern from this patch; this patch
is only meant to improve the processing of alternative subplans, not
to change the costing rules around them.  If we fool with it I'd rather
do so as a separate patch.

			regards, tom lane

  12. Re: Get rid of runtime handling of AlternativeSubPlan?

    David Rowley <dgrowleyml@gmail.com> — 2020-09-27T08:48:20Z

    On Sun, 27 Sep 2020 at 10:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Thanks for reviewing!
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > 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.
>
> Uh, really?

Yeah. Using linitial() and lsecond() will check if the list is
not-NIL. lsecond() does an additional check to ensure the list has at
least two elements. None of which are required since we already know
the list has two elements.

>  And if it's true, why would we change all the call sites
> rather than improving the pg_list.h macros?

Maybe we should. Despite the non-NIL check and length check in
list_head(), list_second_cell(), list_third_cell() functions, the
corresponding macro will crash anyway if those functions were to
return NULL.  We might as well just use list_nth_cell() to get the
ListCell without any checks to see if the cell exists.  I  can go off
and fix those separately. I attached a 0004 patch to help explain what
I'm talking about.

> > 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.
>
> It seemed to me that dealing with the general case would make
> fix_alternative_subplan() noticeably more complex and less obviously
> correct.  I might be wrong though; what specific coding did you have in
> mind?

I had thought something like 0003 (attached). It's a net reduction of
3 entire lines, including the removal of the comment that explained
that there's always two in the list.

> > On a side note, I was playing around with the following case:
> > ...
> > 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:
>
> Hm, actually isn't it the other way around?  *If* there are any matching
> rows, then what's being done here is an accurate estimate.  But if there
> are not, we're going to have to scan the entire subquery output to verify
> that.  I wonder if we should just be taking the subquery cost at face
> value, ie be pessimistic not optimistic.  If the user is bothering to
> test EXISTS, we should expect that the no-match case does happen.
>
> However, I think that's a distinct concern from this patch; this patch
> is only meant to improve the processing of alternative subplans, not
> to change the costing rules around them.  If we fool with it I'd rather
> do so as a separate patch.

Yeah, agreed. I'll open another thread.

David

  13. Re: Get rid of runtime handling of AlternativeSubPlan?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-27T15:59:26Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Sun, 27 Sep 2020 at 10:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> And if it's true, why would we change all the call sites
>> rather than improving the pg_list.h macros?

> Maybe we should. Despite the non-NIL check and length check in
> list_head(), list_second_cell(), list_third_cell() functions, the
> corresponding macro will crash anyway if those functions were to
> return NULL.

Hm, good point.

> We might as well just use list_nth_cell() to get the
> ListCell without any checks to see if the cell exists.  I  can go off
> and fix those separately. I attached a 0004 patch to help explain what
> I'm talking about.

Yeah, that should be dealt with separately.

>> It seemed to me that dealing with the general case would make
>> fix_alternative_subplan() noticeably more complex and less obviously
>> correct.  I might be wrong though; what specific coding did you have in
>> mind?

> I had thought something like 0003 (attached). It's a net reduction of
> 3 entire lines, including the removal of the comment that explained
> that there's always two in the list.

Meh.  This seems to prove my point, as it's in fact wrong; you are only
nulling out the discarded subplans-list entry in one of the two cases.
Once you fix that it's not really shorter anymore, nor clearer.  Still,
I suppose there's some value in removing the assumption about exactly
two subplans.

I'll fix up fix_alternative_subplan and push this.  I think the other
topics should be raised in separate threads.

Thanks for reviewing!

			regards, tom lane