Re: Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers

"Joe Wildish" <joe(at)lateraljoin(dot)com> writes:
> The main change is a switch to using SPI for expression evaluation. The plans are also cached along the same lines as the RI trigger plans.

I really dislike this implementation technique. Aside from the likely
performance hit for existing triggers, I think it opens serious security
holes, because we can't fully guarantee that deparse-and-reparse doesn't
change the semantics. For comparison, the RI trigger code goes to
ridiculous lengths to force exact parsing of the queries it makes,
and succeeds only because it needs just a very stylized subset of SQL.
With a generic user-written expression, we'd be at risk for several
inherently-ambiguous SQL constructs such as IS DISTINCT FROM (see
relevant reading at [1]).

You could argue that the same hazards apply if the user writes the same
query within the body of the trigger, and you'd have a point. But
we've made a policy decision that users are on the hook to write their
functions securely. No such decision has ever been taken with respect
to pre-parsed expression trees. In general, users may expect that
once those are parsed by the accepting DDL command, they'll hold still,
not get re-interpreted at runtime.

> a. I originally disallowed functions and table-valued functions from appearing in the expression as they could potentially do anything and everything. However, I noticed that we allow functions in FOR EACH ROW triggers so we are already in that position. Do we want to continue allowing that in FOR EACH STATEMENT triggers? If so, then the choice to restrict the expression to just OLD, NEW and the table being triggered against might be wrong.

Meh ... users have always been able to write CHECK constraints, WHEN
clauses, etc, that have side-effects --- they just have to bury that
inside a function. It's only their own good taste and the lack of
predictability of when the side-effects will happen that stop them.
I don't see much point in enforcing restrictions that are easily
evaded by making a function.

(Relevant to that, I wonder why this patch is only concerned with
WHEN clauses and not all the other places where we forbid subqueries
for implementation simplicity.)

> b. If a WHEN expression is defined as "n = (SELECT ...)", there is the possibility that a user gets the error "more than one row returned by a subquery used as an expression" when performing DML, which would be rather cryptic if they didn't know there was a trigger involved. To avoid this, we could disallow scalar expressions, with a hint to use the ANY/ALL quantifiers.

How is that any more cryptic than any other error that can occur
in a WHEN expression?

regards, tom lane

[1] https://www.postgresql.org/message-id/10492.1531515255%40sss.pgh.pa.us