Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers - Mailing list pgsql-hackers
| From | Joe Wildish |
|---|---|
| Subject | Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers |
| Date | |
| Msg-id | f9685084-f3fb-45aa-9e04-c4bb1c90f05b@www.fastmail.com Whole thread Raw |
| In response to | Re: Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers
Re: [PATCH] Allow queries in WHEN expression of FOR EACH STATEMENT triggers |
| List | pgsql-hackers |
Hi Tom,
On Wed, 22 Sep 2021, at 17:09, Tom Lane wrote:
> 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 likelyperformance hit for existing triggers, I think it opens serious securityholes, because we can't fully guarantee that deparse-and-reparse doesn'tchange the semantics. For comparison, the RI trigger code goes toridiculous 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 severalinherently-ambiguous SQL constructs such as IS DISTINCT FROM (seerelevant reading at [1]).
Where do you consider the performance hit to be? I just read the code again. The only time the new code paths are hit are when a FOR EACH STATEMENT trigger fires that has a WHEN condition. Given the existing restrictions for such a scenario, that can only mean a WHEN condition that is a simple function call; so, "SELECT foo()" vs "foo()"? Or have I misunderstood?
Regarding the deparse-and-reparse --- if I understand correctly, the core problem is that we have no way of going from a node tree to a string, such that the string is guaranteed to have the same meaning as the node tree? (I did try just now to produce such a scenario with the patch but I couldn't get ruleutils to emit the wrong thing). Moreover, we couldn't store the string for use with SPI, as the string would be subject to trigger-time search path lookups. That pretty much rules out SPI for this then. Do you have a suggestion for an alternative? I guess it would be go to the planner/executor directly with the node tree?
In general, users may expect thatonce those are parsed by the accepting DDL command, they'll hold still,not get re-interpreted at runtime.
I agree entirely. I wasn't aware of the deparsing/reparsing problem.
...(Relevant to that, I wonder why this patch is only concerned withWHEN clauses and not all the other places where we forbid subqueriesfor implementation simplicity.)
I don't know how many other places WHEN clauses are used. Rules, perhaps? The short answer is this patch was written to solve a specific problem I had rather than it being a more general attempt to remove all "subquery forbidden" restrictions.
> 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 occurin a WHEN expression?
It isn't. What *is* different about it, is that -- AFAIK -- it is the only cryptic message that can come about due to the syntactic structure of an expression. Yes, someone could have a function that does "RAISE ERROR 'foo'". There's not a lot that can be done about that. But scalar subqueries are detectable and they have an obvious rewrite using the quantifiers, hence the suggestion. However, it was just that; a suggestion.
-Joe
pgsql-hackers by date: