v6 fixes recently-introduced bit-rot. Kevin Grittner
On Wed, Aug 31, 2016 at 3:24 PM, Kevin Grittner <kgri...@gmail.com> wrote: > 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 -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
trigger-transition-tables-v6.patch.gz
Description: GNU Zip compressed data
trigger-transition-tables-v6-noapi.patch.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
