Re: Inlining comparators as a performance optimisation

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Dec 1, 2011 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Nor does the "register a pointer" scheme you suggest make
>> any sense to me: you still need to connect these things to catalog
>> entries, pg_opclass entries in particular, and the intermediate handle
>> adds nothing to the situation except for creating a risk of collisions.

> I think you might be misinterpreting what I had in mind.  Right now,
> pg_amproc entries have an "amproc" column that points to a pg_proc
> entry that in turn points to a function that takes
> FunctionCallInfoData as an argument.  What I'm proposing to do is add
> an additional column to that catalog that points more or less directly
> to a non-SQL-callable function, but it can't actually just be the
> address of the function because that's not stable.  So what I'm
> proposing is that we interpose the thinnest possible shim layer
> between the catalog and a function pointer, and an int64 -> function
> pointer mapping seemed to me like something that would fit the bill.

But you still need a lot of mechanism to do that mapping, including an
initialization function that has noplace to be executed (unless every
.so that defines a btree opclass has to be listed in preload_libraries),
so it doesn't seem like the "thinnest possible shim" to me.

>> One thing I'm not too certain about is whether to define the APIs just
>> as above, or to support a passthrough argument of some sort (and if so,
>> what does it reference)? �Possibly any datatype that we'd actually care
>> about this for is going to be simple enough to not need any state data.
>> Or possibly not. �And what about collations?

> Maybe there should be a comparator_setup function that gets the
> collation OID and returns void *, and then that void * value gets
> passed as a third argument to each call to the comparator function.

Maybe.  Or perhaps we could merge that work into the
function-pointer-setup function --- that is, give it the collation and
some extra workspace to fool with.  We would always know the
collation at the time we're doing that setup.

At this point the struct filled in by the setup function is starting
to feel a lot like FmgrInfo, and history teaches us a lot about what
needs to be in there.  So maybe

typedef struct SortSupportInfoData *SortSupportInfo;

typedef struct SortSupportInfoData
{MemoryContext    info_cxt;    /* where to allocate subsidiary data */void        *extra;        /* any data needed by
functions*/Oid        collation;    /* provided by caller */
 
void    (*inline_qsort) (Datum *elements, int nelements,             SortSupportInfo info);int    (*comparator) (Datum
a,Datum b,               SortSupportInfo info);/* possibly other function pointers in future */    
 
} SortSupportInfoData;

I am thinking that the btree code, at least, would want to just
unconditionally do
colsortinfo->comparator(datum1, datum2, colsortinfo)

so for an opclass that fails to supply the low-overhead comparator,
it would insert into the "comparator" pointer a shim function that
calls the opclass' old-style FCI-using comparator.  (Anybody who
complains about the added overhead would be told to get busy and
supply a low-overhead comparator for their datatype...)  But to do
that, we have to have enough infrastructure here to cover all cases,
so omitting collation or not having a place to stash an FmgrInfo
won't do.
        regards, tom lane