Re: [HACKERS] Faster methods for getting SPI results (460% improvement) - Mailing list pgsql-hackers

On 7 April 2017 at 00:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I can certainly get on board with the idea of letting a SPI caller provide
> a DestReceiver instead of accumulating the query results into a
> SPITupleTable, but the way it was done here seems quite bizarre.  I think
> for instance you've mishandled non-canSetTag queries; those should have
> any results discarded, full stop.  External callers will only be
> interested in the result of the canSetTag subquery.

That's something I didn't even know was a thing to look for. Thanks
for spotting it.

For other readers who may also be confused, this refers to internally
generated queries that should not be visible to the caller. We use
these in things like CREATE SCHEMA, ALTER TABLE, in FKs, etc.

I wasn't aware that such queries could ever return a result set, though.

> I don't much like DestSPICallback either.  We may well need some better
> way for extension code to create a custom DestReceiver that does something
> out of the ordinary with query result tuples.  But if so, it should not be
> tied to SPI, not even just by name.
>
> I think that 0001/0002 need to be refactored as (A) a change to make
> DestReceiver creation more flexible, and then (B) a change to SPI to allow
> a caller to pass in the receiver to use.

That's exactly what I tried to avoid suggesting upthread, since it'd
be quite much more invasive than the current patch, though definitely
a desirable cleanup.

Personally I think this patch should be allowed to bypass
CreateDestReceiver and create its own struct. I don't really see that
it should be required to clean up the whole API first.

It's on the pointy end for Pg10, and I thought we'd be fine to include
this in pg10 then aim to clean up DestReceiver in early pg11, or even
as a post-feature-freeze refactoring fixup in pg10. Should the
callback approach be blocked because the API it has to use is a bit
ugly?

> After poking around a bit, it seems like we've allowed the original
> notion of CommandDest to get way out of hand.  The only places where
> CreateDestReceiver gets called with a non-constant argument (that is,
> where the caller doesn't know perfectly well which kind of DestReceiver
> it wants) are two calls in postgres.c, and both of those are ultimately
> passing whereToSendOutput, which has got only a really limited set of
> possible values.  I am thinking that we should cut down enum CommandDest
> to be just
>
>     DestNone,                    /* results are discarded */
>     DestDebug,                   /* results go to debugging output */
>     DestRemote,                  /* results sent to frontend process */
>     DestOther                    /* something else */
>
> and change CreateDestReceiver so it throws an error for DestOther; the
> way to create any type of receiver other than these few is to call the
> underlying creation function directly, rather than going through
> CreateDestReceiver.
>
> Having done that, the means for creating a custom receiver is just to
> set up an appropriate struct that embeds struct _DestReceiver and
> always has mydest = DestOther (or maybe we should just lose the mydest
> field altogether).  We could document that a bit better, but it's really
> not complicated.

I strongly agree that this is the way DestReceiver creation should be
refactored.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services