Re: pgsql: Create hooks to let a loadable plugin monitor (or even replace)

On 5/30/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Gurjeet Singh wrote:
> >> But I did not understand the haste to commit the patch within almost
> half an
> >> hour of proposing the second version of the patch!!!
>
> > It happens some times when a patch applier has gotten as far as they can
> > go with a patch and wants to move on, with the willingness to return to
> > the patch if there is any additional feedback.
>
> Er, it was quite a bit more than half an hour; about 17 hours in fact:
> http://archives.postgresql.org/pgsql-patches/2007-05/msg00421.php
> http://archives.postgresql.org/pgsql-committers/2007-05/msg00315.php

I was referring to these two:

http://archives.postgresql.org/pgsql-patches/2007-05/msg00431.php and
http://archives.postgresql.org/pgsql-committers/2007-05/msg00315.php

In any case this patch was just a working-out of ideas I'd proposed more
> than a month previously, so I didn't expect it to be controversial.
>
> But as Bruce says, nothing is set in stone at this point. If you have
> suggestions for improvements, we can tweak the hooks pretty much any
> time up till 8.3 final.

This being a community effort, we would expect that.

I also wished to propose to allow the plugin to completely replace (or
augment) the plan produced by the planner (by passing in a double-pointer of
the plan to the plugin); but I was wary that the idea might get rejected,
for being too radical an idea.

In the last version of the planner plugin patch, the plugins were maintained
as a list, hence allowing for multiple post-planner-plugins to work one
after the other (the variable PPPList); much like the layered I/O driver
architecture of Windows' NTFS sans the guarantee of ordering between the
plugins. To this we may add the ability to pass on the result plan of one
plugin to the next, letting them improve the plan incrementally. Next, we
can add string identifiers like I/O drivers to guarantee the order in which
the plugins will be executed. But again, maybe we don't need multiple
planners working simultaneously ATM.

As for the current patch,I had only a few cosmetic changes in mind:

The comment above planner.c:planner() says '...hook variable that lets a
plugin get control before and after the standard planning ...'; but if we
look at the code, we are just replacing the call to standard_planner(); we
are not calling the plugin before and after standard_planner().

Also, another cosmetic change like reducing an 'if' as follows:

Change:
PlannedStmt *
planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
{
PlannedStmt *result;

if (planner_hook)
result = (*planner_hook) (parse, cursorOptions, boundParams);
else
result = standard_planner(parse, cursorOptions, boundParams);
return result;
}

To:
PlannedStmt *
planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
{
planner_hook_type planner_func = planner_hook ? planner_hook :
standard_planner;

return (*planner_func) (parse, cursorOptions, boundParams);
}

The extra IFs only disorient a normal flow of logic. These two statements
aren't too complicated for readability.

Best regards,

PS: We can make the code more compact (at the cost of readability) like so:

return (*(planner_hook ? planner_hook : standard_planner))(parse,
cursorOptions, boundParams);
--
gurjeet[(dot)singh](at)EnterpriseDB(dot)com
singh(dot)gurjeet(at){ gmail | hotmail | yahoo }.com

17°29'34.37"N 78°30'59.76"E - Hyderabad *
18°32'57.25"N 73°56'25.42"E - Pune

Sent from my BlackLaptop device