Re: delta relations in AFTER triggers

v6 fixes recently-introduced bit-rot.

Kevin Grittner


On Wed, Aug 31, 2016 at 3:24 PM, Kevin Grittner <kgrittn@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 <robertmhaas@gmail.com> wrote:
>> On Fri, May 13, 2016 at 2:37 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
>>> On Fri, May 13, 2016 at 1:02 PM, David Fetter <david@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