Re: [COMMITTERS] pgsql: Create hooks to let a loadable plugin monitor (or even replace) - Mailing list pgsql-hackers
From | Gurjeet Singh |
---|---|
Subject | Re: [COMMITTERS] pgsql: Create hooks to let a loadable plugin monitor (or even replace) |
Date | |
Msg-id | 65937bea0705291445i6d3fde7aua1e7944f8ab25fd5@mail.gmail.com Whole thread Raw |
In response to | Re: [COMMITTERS] pgsql: Create hooks to let a loadable plugin monitor (or even replace) (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: [COMMITTERS] pgsql: Create hooks to let a loadable plugin monitor (or even replace)
Re: [COMMITTERS] pgsql: Create hooks to let a loadable plugin monitor (or even replace) |
List | pgsql-hackers |
On 5/30/07, Tom Lane <tgl@sss.pgh.pa.us > wrote:
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
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);
-- 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);
}
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
pgsql-hackers by date: