Peter Eisentraut <peter.eisentr...@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 /* 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. 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers