On 7/12/21 6:46 AM, Amit Kapila wrote:
On Mon, Jul 12, 2021 at 7:19 AM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
Hi
Andres complained about the safety of doing general expression
evaluation in pgoutput; that was first in
https://postgr.es/m/20210128022032.eq2qqc6zxkqn5...@alap3.anarazel.de
where he described a possible approach to handle it by restricting
expressions to have limited shape; and later in
http://postgr.es/m/20210331191710.kqbiwe73lur7j...@alap3.anarazel.de
I was just scanning the patch trying to see if some sort of protection
had been added for this, but I couldn't find anything. (Some functions
are under-commented, though). So, is it there already, and if so what
is it?
I think the patch is trying to prohibit arbitrary expressions in the
WHERE clause via
transformWhereClause(..EXPR_KIND_PUBLICATION_WHERE..). You can notice
that at various places the expressions are prohibited via
EXPR_KIND_PUBLICATION_WHERE. I am not sure that the checks are correct
and sufficient but I think there is some attempt to do it. For
example, the below sort of ad-hoc check for func_call doesn't seem to
be good idea.
@@ -119,6 +119,13 @@ transformExprRecurse(ParseState *pstate, Node *expr)
/* Guard against stack overflow due to overly complex expressions */
check_stack_depth();
+ /* Functions are not allowed in publication WHERE clauses */
+ if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE &&
nodeTag(expr) == T_FuncCall)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("functions are not allowed in publication WHERE expressions"),
+ parser_errposition(pstate, exprLocation(expr))));
Yes, I mentioned this bit of code in my review, although I was mostly
wondering if this is the wrong place to make this check.
Now, the other idea I had in mind was to traverse the WHERE clause
expression in publication_add_relation and identify if it contains
anything other than the ANDed list of 'foo.bar op constant'
expressions. OTOH, for index where clause expressions or policy check
expressions, we use a technique similar to what we have in the patch
to prohibit certain kinds of expressions.
Do you have any preference on how this should be addressed?
I don't think this is sufficient, because who knows where "op" comes
from? It might be from an extension, in which case the problem pointed
out by Petr Jelinek [1] would apply. OTOH I suppose we could allow
expressions like (Var op Var), i.e. "a < b" or something like that. And
then why not allow (a+b < c-10) and similar "more complex" expressions,
as long as all the operators are built-in?
In terms of implementation, I think there are two basic options - either
we can define a new "expression" type in gram.y, which would be a subset
of a_expr etc. Or we can do it as some sort of expression walker, kinda
like what the transform* functions do now.
regards
[1]
https://www.postgresql.org/message-id/92e5587d-28b8-5849-2374-5ca3863256f1%402ndquadrant.com
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
