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

Bruce Momjian <bruce@momjian.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
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.

            regards, tom lane

"Gurjeet Singh" <singh.gurjeet@gmail.com> writes:
> As for the current patch,I had only a few cosmetic changes in mind:

I don't actually find those suggestions to make it more readable,
rather the reverse ...

            regards, tom lane

Gurjeet, do you have a patch to be applied for this?

---------------------------------------------------------------------------

Gurjeet Singh wrote:
> On 5/30/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Bruce Momjian <bruce@momjian.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[.singh]@EnterpriseDB.com
> singh.gurjeet@{ 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

--  Bruce Momjian  <bruce@momjian.us>          http://momjian.us EnterpriseDB
http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +