Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 4/6/17 03:50, Craig Ringer wrote:
>> But otherwise, pending docs changes, I think it's ready for committer.
> My opinion is still that this is ultimately the wrong approach. The
> right fix for performance issues in PL/Python is to change PL/Python not
> to materialize the list of tuples. Now with this change we would be
> moving from two result materializations to one, but I think we are
> keeping the wrong one.
I just looked at these patches for the first time, and TBH I'm a bit
dubious too. I don't particularly have an opinion about the 0003 patch,
but I think that's mostly what Peter is on about. I do have opinions
about 0001 (and 0002, which ought to be merged; we do not commit features
separately from documentation around here).
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.
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.
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 /*
somethingelse */
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.
There's enough cruft that's accumulated in this area that this would
probably not be an entirely trivial patch, but I think it would be
a good way to make things cleaner.
regards, tom lane
