Обсуждение: Fix for Index Advisor related hooks

On 11.02.2011 22:44, Gurjeet Singh wrote:
>   Looks like the function get_actual_variable_range() was written with the
> knowledge that virtual/hypothetical indexes may exist, but the assumption
> seems wrong.
>
> One one hand get_actual_variable_range() expects that virtual indexes do not
> have an OID assigned, on the other hand explain_get_index_name_hook() is
> handed just an index's OID to get its name back; IMHO these are based on two
> conflicting assumptions about whether a virtual index will have an OID
> assigned.
>
> Attached patch fix_get_actual_variable_range.patch tries to fix this by
> introducing a new hook that can help Postgres decide if an index is
> fictitious or not.

The new hook takes an index oid as argument, so I gather that you 
resolved the contradiction by deciding that fictitious indexes have 
OIDs. How do you assign those OIDs? Do fictitious indexes have entries 
in pg_index?

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> Also attached is the patch expose_IndexSupportInitialize.patch, that makes
> the static function IndexSupportInitialize() global so that the Index
> Advisor doesn't have to reinvent the wheel to prepare an index structure
> with opfamilies and opclasses.

We are *not* doing that.  That's a private function and will remain so.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Tue, Feb 15, 2011 at 8:24 AM, Heikki Linnakangas <
> heikki.linnakangas@enterprisedb.com> wrote:
>> On 11.02.2011 22:44, Gurjeet Singh wrote:
>>> One one hand get_actual_variable_range() expects that virtual indexes do
>>> not
>>> have an OID assigned, on the other hand explain_get_index_name_hook() is
>>> handed just an index's OID to get its name back; IMHO these are based on
>>> two
>>> conflicting assumptions about whether a virtual index will have an OID
>>> assigned.

>> The new hook takes an index oid as argument, so I gather that you resolved
>> the contradiction by deciding that fictitious indexes have OIDs. How do you
>> assign those OIDs? Do fictitious indexes have entries in pg_index?

> No, a fictitious index does not touch pg_index. The  Index Advisor uses
> GetNewOid(pg_class) to generate a new OID for the fictitious index.

That seems like a very expensive, and lock-inducing, way of assigning a
fictitious OID.  They don't need to be globally unique.  I suggest you
consider the idea I suggested back in 2007:
            * In this toy example we just assign all hypothetical indexes            * OID 0, and the
explain_get_index_namehook just prints            * <hypothetical index>.  In a realistic situation we'd probably
    * assume that OIDs smaller than, say, 100 are never the OID of            * any real index, allowing us to identify
oneof up to 100            * hypothetical indexes per plan.  Then we'd need to save aside            * some state data
thatwould let the explain hooks print info            * about the selected index.
 

As far as the immediate problem goes, I agree that
get_actual_variable_range is mistaken, but I think a cleaner and cheaper
solution would be to add a bool "hypothetical" to IndexOptInfo.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> I understand that we need to hide guts of an implementation. But without
> this the Index Advisor will have to emulate what LookupOpclassInfo() does
> and that's a lot of code that I am afraid, if emulated by another function
> in Index Advisor, is more prone to obsolecence than calling
> IndexSupportInitialize().

The only reason you'd need that code is if you were trying to construct
a fake Relation structure, which seems unnecessary and undesirable.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
>     BTW, you use the term 'fictitious' in the comments, would it be better
> to standardize the term used for such an index? So either the comment would
> be changed to call it hypothetical, or the structure member would be changed
> to isfictitious.

Yeah, hypothetical is the more-established term I think.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Wed, Feb 16, 2011 at 10:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The only reason you'd need that code is if you were trying to construct
>> a fake Relation structure, which seems unnecessary and undesirable.

> The planner requires IndexOptInfo, and for the planner to choose the
> hypothetical index we need to fill in the fwdsortop, revsortop, opfamily and
> opcintype, and this is the information that IndexAdvisor populates using
> IndexSupportInitialize() (at least until c0b5fac7 changed the function
> signature.

Yeah, and the set of stuff you need in IndexOptInfo changed again last
week; see collations.  Direct access to IndexSupportInitialize is even
less useful today than it was a week ago.  This stuff has changed many
times before, and it will change again in the future, and exporting a
private function that has an unrelated purpose is not going to insulate
you from needing to deal with that.

> What would be the best way to build an IndexOptInfo for a plain BTREE index
> for different data types?

Fetch the values you need and stuff 'em in the struct.  Don't expect
relcache to do it for you.  The only reason relcache is involved in the
current workflow is that we try to cache the information across queries
in order to save on planner startup time ... but I don't think that that
concern is nearly as pressing for something like Index Advisor.  You'll
have enough to do tracking changes in IndexOptInfo, you don't need to
have to deal with refactorings inside relcache as well.
        regards, tom lane

Gurjeet Singh <singh.gurjeet@gmail.com> writes:
> On Wed, Feb 16, 2011 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Yeah, hypothetical is the more-established term I think.

> Please find the patch attached.

Applied with minor adjustments to HEAD and 9.0.
        regards, tom lane

On 16 February 2011 23:02, Gurjeet Singh <singh.gurjeet@gmail.com> wrote:
> On Wed, Feb 16, 2011 at 10:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Gurjeet Singh <singh.gurjeet@gmail.com> writes:
>> >     BTW, you use the term 'fictitious' in the comments, would it be
>> > better
>> > to standardize the term used for such an index? So either the comment
>> > would
>> > be changed to call it hypothetical, or the structure member would be
>> > changed
>> > to isfictitious.
>>
>> Yeah, hypothetical is the more-established term I think.
>
> Please find the patch attached.

For my benefit, could you explain how ishypothetical gets set to true?

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

Thom Brown <thom@linux.com> writes:
> For my benefit, could you explain how ishypothetical gets set to true?

In the core, it never does.  An index advisor plugin would set it in
IndexOptInfo structs that it makes.
        regards, tom lane

On 17 February 2011 00:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thom Brown <thom@linux.com> writes:
>> For my benefit, could you explain how ishypothetical gets set to true?
>
> In the core, it never does.  An index advisor plugin would set it in
> IndexOptInfo structs that it makes.

I get the idea.  Thanks Tom.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

On 17.02.2011 14:30, Gurjeet Singh wrote:
> On Wed, Feb 16, 2011 at 6:37 PM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>
>> Gurjeet Singh<singh.gurjeet@gmail.com>  writes:
>>> On Wed, Feb 16, 2011 at 10:25 AM, Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>>>> The only reason you'd need that code is if you were trying to construct
>>>> a fake Relation structure, which seems unnecessary and undesirable.
>>
>>> The planner requires IndexOptInfo, and for the planner to choose the
>>> hypothetical index we need to fill in the fwdsortop, revsortop, opfamily
>> and
>>> opcintype, and this is the information that IndexAdvisor populates using
>>> IndexSupportInitialize() (at least until c0b5fac7 changed the function
>>> signature.
>>
>> Yeah, and the set of stuff you need in IndexOptInfo changed again last
>> week; see collations.  Direct access to IndexSupportInitialize is even
>> less useful today than it was a week ago.  This stuff has changed many
>> times before, and it will change again in the future, and exporting a
>> private function that has an unrelated purpose is not going to insulate
>> you from needing to deal with that.
>>
>
> I guess you are right.
>
>
>>
>>> What would be the best way to build an IndexOptInfo for a plain BTREE
>> index
>>> for different data types?
>>
>> Fetch the values you need and stuff 'em in the struct.  Don't expect
>> relcache to do it for you.  The only reason relcache is involved in the
>> current workflow is that we try to cache the information across queries
>> in order to save on planner startup time ... but I don't think that that
>> concern is nearly as pressing for something like Index Advisor.  You'll
>> have enough to do tracking changes in IndexOptInfo, you don't need to
>> have to deal with refactorings inside relcache as well.
>>
>
> I also wish to make Index Advisor faster by not having to lookup this info
> every time a new query comes in and that's why I was trying to use the guts
> of IndexSupportInitialize() where it does the caching.

I doubt performance matters much here. It's not like you're going to be 
explaining hundreds of queries per second with a hypotethical index 
installed. I think of this as a manual tool that you run from a GUI when 
you wonder if an index on column X would help.

The question is, can the Index Advisor easilt access all the information 
it needs to build the IndexOptInfo? If not, what's missing?

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

On 18.02.2011 17:02, Gurjeet Singh wrote:
> On Fri, Feb 18, 2011 at 2:27 AM, Heikki Linnakangas<
> heikki.linnakangas@enterprisedb.com>  wrote:
>
>> On 17.02.2011 14:30, Gurjeet Singh wrote:
>>
>>> On Wed, Feb 16, 2011 at 6:37 PM, Tom Lane<tgl@sss.pgh.pa.us>   wrote:
>>>
>>> Fetch the values you need and stuff 'em in the struct.  Don't expect
>>>> relcache to do it for you.  The only reason relcache is involved in the
>>>> current workflow is that we try to cache the information across queries
>>>> in order to save on planner startup time ... but I don't think that that
>>>> concern is nearly as pressing for something like Index Advisor.  You'll
>>>> have enough to do tracking changes in IndexOptInfo, you don't need to
>>>> have to deal with refactorings inside relcache as well.
>>>>
>>>>
>>> I also wish to make Index Advisor faster by not having to lookup this info
>>> every time a new query comes in and that's why I was trying to use the
>>> guts
>>> of IndexSupportInitialize() where it does the caching.
>>>
>>
>> I doubt performance matters much here. It's not like you're going to be
>> explaining hundreds of queries per second with a hypotethical index
>> installed. I think of this as a manual tool that you run from a GUI when you
>> wonder if an index on column X would help.
>>
>> The question is, can the Index Advisor easilt access all the information it
>> needs to build the IndexOptInfo? If not, what's missing?
>
>
> There's a command line tool in the Index Adviser contrib that takes a file
> full of SQL and run them against the Index Adviser. People would want that
> tool to be as fast as it can be.

Nah, I don't buy that, sounds like a premature optimization. Just 
planning a bunch of SQL statements, even if there's hundreds of them in 
the file, shouldn't take that long. And even if it does, I don't believe 
without evidence that the catalog lookups for the hypothetical indexes 
is where the time is spent.

> Another use case of the Index Advisor is to be switched on for a few hours
> while the application runs, and gather the recommendations for the whole
> run. We'll need good performance that case too.

How exactly does that work? I would imagine that you log all the 
different SQL statements and how often they're run during that period. 
Similar to pgFouine, for example. And only then you run the index 
advisor on the collected SQL statements.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> On 18.02.2011 17:02, Gurjeet Singh wrote:
>>> On Wed, Feb 16, 2011 at 6:37 PM, Tom Lane<tgl@sss.pgh.pa.us>   wrote:
>>>> Fetch the values you need and stuff 'em in the struct.  Don't expect
>>>> relcache to do it for you.

>>> I also wish to make Index Advisor faster by not having to lookup this info
>>> every time a new query comes in and that's why I was trying to use the
>>> guts of IndexSupportInitialize() where it does the caching.

> Nah, I don't buy that, sounds like a premature optimization. Just 
> planning a bunch of SQL statements, even if there's hundreds of them in 
> the file, shouldn't take that long. And even if it does, I don't believe 
> without evidence that the catalog lookups for the hypothetical indexes 
> is where the time is spent.

But even more to the point, IndexSupportInitialize is simply not well
matched to the problem.  It's designed to help produce a relcache entry
from a pg_index entry, and in particular to look up opfamily and input
datatype from an opclass OID.  (Oh, and to produce info about index
support functions, which you certainly don't need.)  But as far as I can
see, an index advisor would already *have* opfamily and input datatype,
because what it's going to be starting from is some query WHERE
condition that it thinks would be worth optimizing.  What it's going to
get from looking up that operator in pg_amop is opfamily and opcintype
information.  Looking up an opclass from that is unnecessary work as far
as I can see (you don't need it to put in IndexOptInfo, for sure), and
reversing the lookup afterwards is certainly pointless.

So even granted that performance is critical, you haven't made a case
why exposing IndexSupportInitialize is going to be useful.
        regards, tom lane