I have merged in the changes since v4 (a year and a half ago) and cured all bit-rot I found, to get the attached v5 which runs `make check world` without problem -- including the tests added for this feature.
I did remove the contrib code that David Fetter wrote to demonstrate the correctness and performance of the tuplestores as created during the transaction, and how to use them directly from C code, before any API code was written. If we want that to be committed, it should be considered separately after the main feature is in. Thanks to Thomas Munro who took a look at v4 and pointed out a bug (which is fixed in v5) and suggested a way forward for using the parameters. Initial attempts to get that working were not successful,, but I think it is fundamentally the right course, should we reach a consensus to go that way, On Thu, Jul 7, 2016 at 5:07 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Fri, May 13, 2016 at 2:37 PM, Kevin Grittner <kgri...@gmail.com> wrote: >> On Fri, May 13, 2016 at 1:02 PM, David Fetter <da...@fetter.org> wrote: >>> On Thu, Jan 22, 2015 at 02:41:42PM +0000, Kevin Grittner wrote: >> >>>> [ideas on how to pass around references to ephemeral relations] >>> >>> [almost 17 months later] >>> >>> It seems like now is getting close to the time to get this into >>> master. The patch might have suffered from some bit rot, but the >>> design so far seems sound. >>> >>> What say? >> >> I had a talk with Tom in Brussels about this. As I understood it, >> he wasn't too keen on the suggestion by Heikki (vaguely >> sorta-endorsed by Robert) of passing ephemeral named relations such >> as these tuplestores around in the structures currently used for >> parameter values. He intuitively foresaw the types of problems I >> had run into trying to use the same structure to pass a relation >> (with structure and rows and columns of data) as is used to pass, >> say, an integer. > > See, the thing that disappoints me about this is that I think we were > pretty closed to having the ParamListInfo-based approach working. Maybe, but the thing I would like to do before proceeding down that road is to confirm that we have a consensus that such a course is better than what Is on the branch which is currently working. If that's the consensus here, I'll work on that for the next CF. If not, there may not be a lot left to do before commit. (Notably, we may want to provide a way to free a tuplestore pointer when done with it, but that's not too much work.) Let me describe the API I have working. There are 11 function prototypes modified under src/include, in all cases to add a Tsrcache parameter: 1 createas.h 3 explain.h 1 prepare.h 1 analyze.h 2 tcopprot.h 3 utility.h There are three new function prototypes in SPI. NOTE: This does *not* mean that SPI is required to use named tuplestores in queries, just that there are convenience functions for any queries being run through SPI, so that any code using SPI (including any PLs that do) will find assigning a name to a tuplestore and referencing that within a query about as easy as falling off a log. A tuplestore is registered to the current SPI context and not visible outside that context. This results in a Tsrcache being passed to the functions mentioned above when that context is active, just as any non-SPI code could do. > The thing I liked about that approach is that we already know that any > place where you can provide parameters for a query, there will also be > an opportunity to provide a ParamListInfo. And I think that > parameterizing a query by an ephemeral table is conceptually similar > to parameterizing it by a scalar value. If we invent a new mechanism, > it's got to reach all of those same places in the code. Do you see someplace that the patch missed? > One other comment that I would make here is that I think that it's > important, however we pass the data, that the scope of the tuplestores > is firmly lexical and not dynamic. We need to make sure that we don't > set some sort of context via global variables that might get used for > some query other than the one to which it's intended to apply. Is this based on anything actually in the patch? For this CF, the main patch attached is a working version of the feature that people can test, review documentation, etc. Any API changes are not expected to change these visible behaviors, so any feedback on usability or documentation can be directly useful regardless of the API discussion. I have also attached a smaller patch which applies on top of the main one which rips out the Tsrcache API to get to a "no API" version that compiles cleanly and runs fine as long as you don't try to use the feature, in which case it will not recognize the tuplestore names and will give this message: "executor could not find named tuplestore \"%s\"". There may be a little bit left to rip out when adding a parameter-based API, but this is basically where we're moving from if we go that way. I include it both to help isolate the API we're discussing and in case anyone wants to play with the "no API" version to try any alternative API. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Description: GNU Zip compressed data
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers