Re: knngist - 0.8

On Fri, Oct 15, 2010 at 7:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I still feel vaguely uneasy about the fact that the proposed patch
>> can't handle ASC/DESC or NULLS FIRST/LAST, and that unease grew a bit
>> more last night when I read Peter's patch to add collation support.
>
> Good point.
>
>> We could possibly cram ASC/DESC and NULLS FIRST/LAST in by defining
>> four new categories of operator strategies rather than one, but
>> there's no way that's going to work for collations.  Is there some
>> other way to approach this problem?  Can we leave pg_amop as it is,
>> and pass the context (sort vs. qual, ASC/DESC, NULLS FIRST/LAST,
>> collation, whatever...) to the index via some sort of side channel?
>
> Well, we cannot avoid changing pg_amop, or at least changing its
> interpretation, because the current scheme simply can't represent
> indexable operators that are used for anything except simple boolean
> WHERE tests.  I agree though that we do *not* want pg_amop involved
> in the details of exactly what sort ordering is referenced by a sortable
> operator.  Somehow that needs to be passed in a side channel.

I spent some time tonight looking at how this is handled in
builtin_knngist_core-0.9.  It looks like the requested sort, if there
is one, just gets tossed into the indexquals list and eventually
transformed into a scankey with a new flag SK_ORDER set.  I think what
we should do instead is add an additional IndexPath field which can
point to a node indicating the desired ordering properties.  This node
might look similar to a scan key but having it be a separate node type
will allow us to add in whatever, ahem, miscellaneous crap we want to
pass: currently ASC/DESC and NULLS FIRST/LAST, and eventually
collation and so forth.  That can then get propagated into the
IndexScan and passed to index_beginscan, where it can be passed to the
AM's ambeginscan function (i.e. we'll add an additional argument to
the signatures of those functions).

There's a second problem, too.  If we assume that we want to treat
this problem with some degree of generality - that is, we really do
care about things like ASC/DESC and NULLS FIRST/LAST and eventually
collation - then the proposed amcanorderbyop flag isn't really
sufficient.  The AM will need to be able to indicate whether a given
combination of parameters is one that it can handle.  I'm thinking
perhaps in lieu of a boolean, we can add another indexam method which,
if not InvalidOid, gets called when we're wondering about whether a
given clause is something that the index can order by.  Although
knngist focuses on a the ORDER BY col OP constant case, which
certainly seems like the most useful one, there's no theoretical
reason why an AM couldn't allow ordering by some more complex clause.
I don't know whether anyone will ever feel like writing code to do
something like that, but if we set the API up this way then it should
be at least theoretically possible.

Comments?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company