On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Sun, Dec 4, 2016 at 11:35 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>
>> Moved to next CF with "waiting on author" status.
>
> Patch v8 attempts to address the issues explicitly raised in
> Thomas Munro's review. An opaque query environment is created
> that, for now, only passes through ephemeral named relations, of
> which the only initial implementation is named tuplestores; but
> the techniques are intended to support both other types of
> ephemeral named relations and environmental properties (things
> affecting parsing, planning, and execution that are not coming from
> the system catalog) besides ENRs. There is no clue in the access
> to the QE whether something is, for example, stored in a list or a
> hash table. That's on purpose, so that the addition of other
> properties or changes to their implementation doesn't affect the
> calling code.
+1
> 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?
Should we attempt to detect if the tupledesc changes incompatibly
between planning and execution?
> Changed to "Needs review" status.
+ enr->md.enrtuples =
tuplestore_tuple_count(trigdata->tg_newtable);
I was wondering about this. As I understand it, plans for statements
in plpgsql functions are automatically cached. Is it OK for the
number of tuples on the first invocation of the trigger in a given
session to determine the estimate used for the plan? I suppose that
is the case with regular tables, so maybe it is.
+ register_enr(estate.queryEnv, enr);
+ SPI_register_relation(enr);
Here plpgsql calls both register_enr and SPI_register_relation. Yet
SPI_register_relation calls register_enr too, so is this a mistake?
Also, the return code isn't checked.
--
Thomas Munro
http://www.enterprisedb.com