Re: RFC: extensible planner state

Robert Haas <robertmhaas@gmail.com> writes:
> I think that was rebased over a patch I inadvertently committed to my
> local master branch. Trying again.

I looked through the v5 patchset.

0001: The allocation logic in Set*ExtensionState fails to guarantee
that it's made the array(s) big enough to hold the passed ID, which
would be problematic in the face of lots of extensions.
I think this'd be enough to fix it:
-        i = pg_nextpower2_32(glob->extension_state_allocated + 1);
+        i = pg_nextpower2_32(extension_id + 1);
But maybe you should also reconsider whether blindly starting
the array sizes at 4, rather than say Max(4, extension_id + 1),
is good.

Now that I look, SetExplainExtensionState also has these issues.

Also a couple nitpicks:
* alphabetization fail in util/Makefile
* utils/palloc.h is already included by postgres.h

0002: LGTM

0003: In the wake of 70407d39b, you should avoid "struct
ExplainState" in favor of using duplicate typedefs.
Also, although you're not the first sinner, I really think
this new argument to planner() should be documented.  Maybe
the rest too while we're at it.

0004: maybe the planner_shutdown_hook should be called before
DestroyPartitionDirectory?  It's not entirely clear whether the hook
might like to look at that.  Also "struct ExplainState" again.

0005: okay

            regards, tom lane