Rewritten Index Advisor patch

Lists: pgsql-patches
From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-patches(at)postgreSQL(dot)org
Cc: "Gurjeet Singh" <singh(dot)gurjeet(at)gmail(dot)com>
Subject: Rewritten Index Advisor patch
Date: 2007-05-25 00:45:40
Message-ID: 27516.1180053940@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-patches

Awhile back I complained that I didn't like the way that the index advisor
patch plugged into the system:
http://archives.postgresql.org/pgsql-hackers/2007-04/msg00346.php

Attached is a proposed replacement patch that keeps essentially all the
advisor logic outside the core backend, and uses the method I suggested of
modifying the result of get_relation_info() rather than installing phony
system-catalog entries. Most of the patch bulk is actually just a small
refactoring of ExplainOnePlan's API to make it more convenient to call
from an advisor plugin. I also added hooks to let an advisor work through
EXPLAIN, as I still maintain is a more useful behavior than doubling the
work involved in every planner call. However, the planner() hook is still
there for those who are insistent.

To test the code, I made up a silly little proof-of-concept "advisor" that
just checks to see if 2-column indexes would be more helpful if the column
order were switched. It's incomplete because I didn't do anything about
printing out a nice explanation of what the hypothetical index is.

Comments, objections?

regards, tom lane

Attachment Content-Type Size
advisor-hooks.patch.gz application/octet-stream 4.3 KB

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "Gurjeet Singh" <singh(dot)gurjeet(at)gmail(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: Rewritten Index Advisor patch
Date: 2007-05-25 18:16:32
Message-ID: 19929.1180116992@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-patches

I wrote:
> Attached is a proposed replacement patch that keeps essentially all the
> advisor logic outside the core backend, and uses the method I suggested of
> modifying the result of get_relation_info() rather than installing phony
> system-catalog entries.

I've applied this with one further change to make the planner_hook a bit
more general; it now looks like this:

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

PlannedStmt *
standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
{
... exactly the same as planner() was before ...
}

This avoids presuming that the hook needs to duplicate the Query, and
potentially lets a plugin replace the planner altogether.

The Index Advisor patch as-submitted is not compatible with these hooks,
but it appears to need nontrivial work anyway due to other changes
between 8.2 and 8.3 (in particular the introduction of PlannedStmt).
Rather than trying to get it into 8.3 as a contrib module, I recommend
pursuing it as a pgfoundry project, so as to decouple its release
schedule from the backend's.

regards, tom lane