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

I like the new interface... it supports all the current Index Advisor
requirements, and probably the future requirements too.

I'll submit a new patch taking the current support routines into
consideration.

But I did not understand the haste to commit the patch within almost half an
hour of proposing the second version of the patch!!!
--
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

Gurjeet Singh wrote:

I like the new interface... it supports all the current Index Advisor
requirements, and probably the future requirements too.

I'll submit a new patch taking the current support routines into
consideration.

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.

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

--
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

On 5/25/07, Tom Lane <tgl@postgresql.org> wrote:

Log Message:
-----------
Create hooks to let a loadable plugin monitor (or even replace) the
planner
and/or create plans for hypothetical situations; in particular,
investigate
plans that would be generated using hypothetical indexes. This is a
heavily-rewritten version of the hooks proposed by Gurjeet Singh for his
Index Advisor project. In this formulation, the index advisor can be
entirely a loadable module instead of requiring a significant part to be
in the core backend, and plans can be generated for hypothetical indexes
without requiring the creation and rolling-back of system catalog entries.

The index advisor patch as-submitted is not compatible with these hooks,
but it needs significant work anyway due to other 8.2-to-8.3 planner
changes. With these hooks in the core backend, development of the advisor
can proceed as a pgfoundry project.

Modified Files:
--------------
pgsql/src/backend/commands:
explain.c (r1.163 -> r1.164)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/explain.c.diff?r1=1.163&amp;r2=1.164
)
prepare.c (r1.75 -> r1.76)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/prepare.c.diff?r1=1.75&amp;r2=1.76
)
pgsql/src/backend/executor:
nodeBitmapIndexscan.c (r1.22 -> r1.23)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/nodeBitmapIndexscan.c.diff?r1=1.22&amp;r2=1.23
)
nodeIndexscan.c (r1.121 -> r1.122)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/nodeIndexscan.c.diff?r1=1.121&amp;r2=1.122
)
pgsql/src/backend/optimizer/plan:
planner.c (r1.219 -> r1.220)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/optimizer/plan/planner.c.diff?r1=1.219&amp;r2=1.220
)
pgsql/src/backend/optimizer/util:
plancat.c (r1.134 -> r1.135)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/optimizer/util/plancat.c.diff?r1=1.134&amp;r2=1.135
)
pgsql/src/include/commands:
explain.h (r1.30 -> r1.31)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/commands/explain.h.diff?r1=1.30&amp;r2=1.31
)
pgsql/src/include/optimizer:
plancat.h (r1.43 -> r1.44)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/optimizer/plancat.h.diff?r1=1.43&amp;r2=1.44
)
planner.h (r1.39 -> r1.40)
(
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/optimizer/planner.h.diff?r1=1.39&amp;r2=1.40
)

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

--
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. +

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

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

"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. +