Re: RFC: extensible planner state

On Mon, Aug 25, 2025 at 3:47 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> 0001 is the core "private state" patch for PlannerGlobal, PlannerInfo,
> and RelOptInfo. It is unchanged since v2, and contains only the fix
> for memory context handling since v1. However, I've now tested it, and
> I think it's OK to commit, barring further review comments.

A few nits on 0001

> From 1aa43c063edb325548fa3db30b9991bf0831f6f5 Mon Sep 17 00:00:00 2001
> From: Robert Haas <rhaas@postgresql.org>
> Date: Mon, 18 Aug 2025 16:11:10 -0400
> Subject: [PATCH v3 1/5] Allow private state in certain planner data
> + * extendplan.c
> + *      Extend core planner objects with additional private state
> + *
> + * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994-5, Regents of the University of California
> + *
> + * The interfaces defined in this file make it possible for loadable
> + * modules to their own private state inside of key planner data

You're missing a word above -- like "modules to store their own"

> + * uses set_join_pathlist_hook can arrange to compute a key intermediate
> + * result once per joinrel rather than on every call.
> + *
> + * IDENTIFICATION
> + *      src/backend/commands/extendplan.c

This path does not reflect where you put the file

> + *
> +int
> +GetPlannerExtensionId(const char *extension_name)
> +{
<--snip-->
> +
> +    /* If there's an array but it's currently full, expand it. */
> +    if (PlannerExtensionNamesAssigned >= PlannerExtensionNamesAllocated)
> +    {
> +        int            i = pg_nextpower2_32(PlannerExtensionNamesAssigned + 1);

Storing a uint32 in a signed int that could be 32-bit stuck out to me.

> +
> +        PlannerExtensionNameArray = (const char **)
> +            repalloc(PlannerExtensionNameArray, i * sizeof(char *));
> +        PlannerExtensionNamesAllocated = i;
> +    }
> +
> +    /* Assign and return new ID. */
> +    PlannerExtensionNameArray[PlannerExtensionNamesAssigned] = extension_name;

Since you don't copy the extension name, it might be worth mentioning
that the caller should provide a literal or at least something that
will be around later.

> diff --git a/src/include/optimizer/extendplan.h b/src/include/optimizer/extendplan.h
> new file mode 100644
> +extern void SetPlannerInfoExtensionState(PlannerInfo *root, int extension_id,
> +                                         void *opaque);
> +extern void SetRelOptInfoExtensionState(RelOptInfo *root, int extension_id,
> +                                        void *opaque);

You used a different variable name here than in the implementation for
the RelOptInfo parameter.

> 0002 removes join_search_private, as before. Whether it makes sense to
> go ahead with this is debatable. Needs review, and needs an opinion on
> whether this should be considered a PoC only (and discarded) or
> something that should go forward to commit.

Is there a downside to going forward with it?

As for the code itself, I thought assumeReplanning was a bit vague
since it seems like whether or not replanning is allowed could come up
outside of join order search -- but perhaps that's okay.

> For another example of how these patches could be used, see
> http://postgr.es/m/CA+TgmoZ=6jJi9TGyZCm33vads46HFkyz6Aju_saLT6GFS-iFug@mail.gmail.com
> and in particular 0001 and 0002. This patch set's planner_setup_hook
> call would go write after those patches compute default_ssa_mask and
> default_jsa_mask, allowing the hook to override those values.

So, are you saying that you would rewrite the patches in that set to
use the infrastructure in this set -- e.g. remove that set's
PlannerGlobal.default_jsa_mask and instead put it in
PlannerGlobal.extension_state? Or am I misunderstanding?

- Melanie