Remove dead macro exec_subplan_get_plan

Zhang Mingli <zmlpostgres@gmail.com> writes:

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

Hm, I wonder why it's not used anymore. Maybe we no longer need
that list at all? If we do, should use of the macro be
re-introduced in the accessors?

regards, tom lane

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

Zhang Mingli <zmlpostgres@gmail.com> writes:

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

Hm, I wonder why it's not used anymore. Maybe we no longer need
that list at all? If we do, should use of the macro be
re-introduced in the accessors?

Seems nowadays no one fetches the Plan from PlannedStmt->subplans with a
certain plan_id any more. Previously back in eab6b8b2 where this macro
was introduced, it was used in explain_outNode and ExecInitSubPlan.

I find a similar macro, planner_subplan_get_plan, who fetches the Plan
from glob->subplans. We can use it in the codes where needed. For
example, in the new function SS_make_multiexprs_unique.

     /* Found one, get the associated subplan */
-    plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1);
+    plan = planner_subplan_get_plan(root, splan);

Thanks
Richard

Hi,all

Regards,
Zhang Mingli
On Sep 6, 2022, 10:22 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:

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

Zhang Mingli <zmlpostgres@gmail.com> writes:

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

Hm, I wonder why it's not used anymore.  Maybe we no longer need
that list at all?  If we do, should use of the macro be
re-introduced in the accessors?

The PlannedStmt->subplans list is still used at several places.

Seems nowadays no one fetches the Plan from PlannedStmt->subplans with a
certain plan_id any more. Previously back in eab6b8b2 where this macro
was introduced, it was used in explain_outNode and ExecInitSubPlan.

I find a similar macro, planner_subplan_get_plan, who fetches the Plan
from glob->subplans. We can use it in the codes where needed. For
example, in the new function SS_make_multiexprs_unique.

     /* Found one, get the associated subplan */
-    plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1);
+    plan = planner_subplan_get_plan(root, splan);

Thanks
Richard

Yeah, searched on history and found:
exec_subplan_get_plan was once used in ExecInitSubPlan() to create planstate.

```
Plan    *plan = exec_subplan_get_plan(estate->es_plannedstmt, subplan);
...
node->planstate = ExecInitNode(plan, sp_estate, eflags);
```

And now in ExecInitSubPlan(), planstate comes from es_subplanstates.

```
/* Link the SubPlanState to already-initialized subplan */
sstate->planstate = (PlanState *) list_nth(estate->es_subplanstates, subplan->plan_id - 1);
```

And estate->es_subplanstates is evaluated through a for-range of subplans list at some functions.

```
foreach(l, plannedstmt->subplans)
{
 ...
 estate->es_subplanstates = lappend(estate->es_subplanstates, subplanstate);
}
```

On Tue, Sep 6, 2022 at 12:39 AM Zhang Mingli <zmlpostgres@gmail.com> wrote:

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

How about add it to the CF to not lose track of it?

Thanks
Richard

On Sep 16, 2022, 14:47 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:

On Tue, Sep 6, 2022 at 12:39 AM Zhang Mingli <zmlpostgres@gmail.com> wrote:

Macro exec_subplan_get_plan is not used anymore.
Attach a patch to remove it.

How about add it to the CF to not lose track of it?

Will add it, thanks~

Regards,
Zhang Mingli

On Fri, 16 Sept 2022 at 03:33, Zhang Mingli <zmlpostgres@gmail.com> wrote:

On Sep 16, 2022, 14:47 +0800, Richard Guo <guofenglinux@gmail.com>, wrote:

How about add it to the CF to not lose track of it?

Will add it, thanks~

I guess not losing track of it is only helpful if we do eventually
commit it. Otherwise we would rather lose track of it :)

I think the conclusion here was that the actual list is still used and
cleaning up unused macros isn't worth the hassle unless it's part of
some larger patch? I mean, it doesn't seem like a lot of hassle but
nobody seems to have been interested in pursuing it since 2022 so I
guess it's not going to happen.

I don't want to keep moving patches forward release to release that
nobody's interested in committing. So I'm going to mark this one
rejected for now. We can always update that if it comes up again.

--
Gregory Stark
As Commitfest Manager