Re: generic plans and "initial" pruning

On Thu, Jan 6, 2022 at 3:45 PM Amul Sul <sulamul@gmail.com> wrote:
> Here are few comments for v1 patch:

Thanks Amul.  I'm thinking about Robert's latest comments addressing
which may need some rethinking of this whole design, but I decided to
post a v2 taking care of your comments.

> +   /* Caller error if we get here without contains_init_steps */
> +   Assert(pruneinfo->contains_init_steps);
>
> -       prunedata = prunestate->partprunedata[i];
> -       pprune = &prunedata->partrelprunedata[0];
>
> -       /* Perform pruning without using PARAM_EXEC Params */
> -       find_matching_subplans_recurse(prunedata, pprune, true, &result);
> +   if (parentrelids)
> +       *parentrelids = NULL;
>
> You got two blank lines after Assert.

Fixed.

> --
>
> +   /* Set up EState if not in the executor proper. */
> +   if (estate == NULL)
> +   {
> +       estate = CreateExecutorState();
> +       estate->es_param_list_info = params;
> +       free_estate = true;
>     }
>
> ... [Skip]
>
> +   if (free_estate)
> +   {
> +       FreeExecutorState(estate);
> +       estate = NULL;
>     }
>
> I think this work should be left to the caller.

Done.  Also see below...

>     /*
>      * Stuff that follows matches exactly what ExecCreatePartitionPruneState()
>      * does, except we don't need a PartitionPruneState here, so don't call
>      * that function.
>      *
>      * XXX some refactoring might be good.
>      */
>
> +1, while doing it would be nice if foreach_current_index() is used
> instead of the i & j sequence in the respective foreach() block, IMO.

Actually, I rewrote this part quite significantly so that most of the
code remains in its existing place.  I decided to let
GetLockableRelations_walker() create a PartitionPruneState and pass
that to ExecFindInitialMatchingSubPlans() that is now left more or
less as is.  Instead, ExecCreatePartitionPruneState() is changed to be
callable from outside the executor.

The temporary EState is no longer necessary.  ExprContext,
PartitionDirectory, etc. are now managed in the caller,
GetLockableRelations_walker().

> --
>
> +                   while ((i = bms_next_member(validsubplans, i)) >= 0)
> +                   {
> +                       Plan   *subplan = list_nth(subplans, i);
> +
> +                       context->relations =
> +                           bms_add_members(context->relations,
> +                                           get_plan_scanrelids(subplan));
> +                   }
>
> I think instead of get_plan_scanrelids() the
> GetLockableRelations_worker() can be used; if so, then no need to add
> get_plan_scanrelids() function.

You're right, done.

> --
>
>      /* Nodes containing prunable subnodes. */
> +       case T_MergeAppend:
> +           {
> +               PlannedStmt *plannedstmt = context->plannedstmt;
> +               List       *rtable = plannedstmt->rtable;
> +               ParamListInfo params = context->params;
> +               PartitionPruneInfo *pruneinfo;
> +               Bitmapset  *validsubplans;
> +               Bitmapset  *parentrelids;
>
> ...
>                 if (pruneinfo && pruneinfo->contains_init_steps)
>                 {
>                     int     i;
> ...
>                    return false;
>                 }
>             }
>             break;
>
> Most of the declarations need to be moved inside the if-block.

Done.

> Also, initially, I was a bit concerned regarding this code block
> inside GetLockableRelations_worker(), what if (pruneinfo &&
> pruneinfo->contains_init_steps) evaluated to false? After debugging I
> realized that plan_tree_walker() will do the needful -- a bit of
> comment would have helped.

You're right.  Added a dummy else {} block with just the comment saying so.

> +       case T_CustomScan:
> +           foreach(lc, ((CustomScan *) plan)->custom_plans)
> +           {
> +               if (walker((Plan *) lfirst(lc), context))
> +                   return true;
> +           }
> +           break;
>
> Why not plan_walk_members() call like other nodes?

Makes sense, done.

Again, most/all of this patch might need to be thrown away, but here
it is anyway.

--
Amit Langote
EDB: http://www.enterprisedb.com