Subject: Re: row filtering for logical replication
To: Amit Kapila <amit.kapila16@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Euler Taveira <euler@eulerto.com>, Greg Nancarrow <gregn4422@gmail.com>,
 Peter Smith <smithpb2250@gmail.com>, Rahila Syed <rahilasyed90@gmail.com>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 =?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <onderkalaci@gmail.com>,
 japin <japinli@hotmail.com>, Michael Paquier <michael@paquier.xyz>,
 David Steele <david@pgmasters.net>, Craig Ringer <craig@2ndquadrant.com>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Amit Langote <amitlangote09@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <849ee491-bba3-c0ae-cc25-4fce1c03f105@enterprisedb.com>
 <202107120148.l4nueeibgynb@alvherre.pgsql>
 <CAA4eK1Jumuio6jZK8AVQd6z7gpDsZydQhK6d=MUARxk3nS7+Pw@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <33c033f7-be44-e241-5fdf-da1b328c288d@enterprisedb.com>
Date: Mon, 12 Jul 2021 11:31:23 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: 
 <CAA4eK1Jumuio6jZK8AVQd6z7gpDsZydQhK6d=MUARxk3nS7+Pw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/33c033f7-be44-e241-5fdf-da1b328c288d%40enterprisedb.com>
Precedence: bulk


On 7/12/21 6:46 AM, Amit Kapila wrote:
> On Mon, Jul 12, 2021 at 7:19 AM Alvaro Herrera <alvherre@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.eq2qqc6zxkqn5syt@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.kqbiwe73lur7jo2e@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