Re: delta relations in AFTER triggers

On Mon, Feb 20, 2017 at 7:44 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> Was it intentional that this test doesn't include any statements that
> reach the case where the trigger does RAISE EXCEPTION 'RI error'?
> From the output generated there doesn't seem to be any evidence that
> the triggers run at all, though I know from playing around with this
> that they do

Tests expanded to cover more. Some bugs found and fixed in the process. :-/

> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
>
> These copyright messages are missing 3 years' worth of bugfixes.

Those were only in files created with the initial patch in 2014. No
bug fixes missing. Updated the years to 2017, though.

> +SPI_get_caller_relation(const char *name)
>
> Do we need this function? It's not used by the implementation.

Good point. It seemed useful way back when, but since no uses for
it emerged, it should be removed until such time (if any) that it
would be useful.

> +typedef struct NamedTuplestoreScan
> +{
> + Scan scan;
> + char *enrname;
> +} NamedTuplestoreScan;
>
> Nearly plan node structs always have a comment for the members below
> 'scan'; I think this needs one too because 'enrname' is not
> self-explanatory.

Done.

> /*
> + * Capture the NEW and OLD transition TABLE tuplestores (if specified for
> + * this trigger).
> + */
> + if (trigdata->tg_newtable || trigdata->tg_oldtable)
> + {
> + estate.queryEnv = create_queryEnv();
> + if (trigdata->tg_newtable)
> + {
> + Enr enr = palloc(sizeof(EnrData));
> +
> + enr->md.name = trigdata->tg_trigger->tgnewtable;
> + enr->md.tupdesc = trigdata->tg_relation->rd_att;
> + enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable);
> + enr->reldata = trigdata->tg_newtable;
> + register_enr(estate.queryEnv, enr);
> + SPI_register_relation(enr);
> + }
>
> Why do we we have to call register_enr and also SPI_register_relation here?

Essentially, because plpgsql does some things through SPI and some
things not. Both cases are covered.

> On Mon, Dec 19, 2016 at 4:35 AM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>> There were a few changes Thomas included in the version he posted,
>>> without really delving into an explanation for those changes. Some
>>> or all of them are likely to be worthwhile, but I would rather
>>> incorporate them based on explicit discussion, so this version
>>> doesn't do much other than generalize the interface a little,
>>> change some names, and add more regression tests for the new
>>> feature. (The examples I worked up for the rough proof of concept
>>> of enforcement of RI through set logic rather than row-at-a-time
>>> navigation were the basis for the new tests, so the idea won't get
>>> totally lost.) Thomas, please discuss each suggested change (e.g.,
>>> the inclusion of the query environment in the parameter list of a
>>> few more functions).
>>
>> I was looking for omissions that would cause some kind of statements
>> to miss out on ENRs arbitrarily. It seemed to me that
>> parse_analyze_varparams should take a QueryEnvironment, mirroring
>> parse_analyze, so that PrepareQuery could pass it in. Otherwise,
>> PREPARE wouldn't see ENRs. Is there any reason why SPI_prepare should
>> see them but PREPARE not?
>
> Any thoughts about that?

Do you see any way to test that code, or would it be dead code there
"just in case" we later decided to do something that needed it? I'm
not a big fan of the latter. I've had to spend too much time
maintaining and/or ripping out code that fits that description.

On Mon, Feb 20, 2017 at 9:43 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Tue, Feb 21, 2017 at 7:14 AM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>> On Fri, Jan 20, 2017 at 2:49 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
>>> Attached is v9 which fixes bitrot from v8. No other changes.
>>>
>>> Still needs review.
>
> Based on a suggestion from Robert off-list I tried inserting into a
> delta relation from a trigger function and discovered that it
> segfaults

Fixed. Such an attempt now generates something like this:

ERROR: relation "d" cannot be the target of a modifying statement
CONTEXT: SQL statement "INSERT INTO d VALUES (1000000, 1000000, 'x')"
PL/pgSQL function level2_table_bad_usage_func() line 3 at SQL statement

New patch attached.

Miscellanea:

Do you suppose we should have all PLs that are part of the base
distro covered?

What is necessary to indicate an additional SQL feature covered?

--
Kevin Grittner