Horton

Re: Inlining comparators as a performance optimisation

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter Geoghegan <peter@2ndquadrant.com>, PG Hackers <pgsql-hackers@postgresql.org>
Date: 2011-12-02T04:00:36Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Speed up conversion of signed integers to C strings.

  2. Remove some unnecessary tests of pgstat_track_counts.

  3. Remove cvs keywords from all files.

  4. Code cleanup for function prototypes: change two K&R-style prototypes

  5. Use Min() instead of min() in qsort, for consistency and to avoid

  6. pgindent run for 8.2.

  7. Switch over to using our own qsort() all the time, as has been proposed

On Thu, Dec 1, 2011 at 10:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

PG_init?

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

Ah!  That seems quite nice.

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

I'm slightly worried about whether that'll be adding too much overhead
to the case where there is no non-FCI comparator.  But it may be no
worse than what we're doing now.

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