Horton

Re: row filtering for logical replication

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
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>, Önder Kalacı <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>
Date: 2021-07-12T09:31:23Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Release cache tuple when no longer needed

  2. Add some additional tests for row filters in logical replication.

  3. Fix one of the tests introduced in commit 52e4f0cd47.

  4. Allow specifying row filters for logical replication of tables.

  5. Move scanint8() to numutils.c

  6. Replace Test::More plans with done_testing

  7. Reduce relcache access in WAL sender streaming logical changes

  8. Small cleanups related to PUBLICATION framework code

  9. Add a view to show the stats of subscription workers.

  10. Allow publishing the tables of schema.

  11. Doc: improve documentation of CREATE/ALTER SUBSCRIPTION.

  12. Add PublicationTable and PublicationRelInfo structs

  13. Remove unused argument "txn" in maybe_send_schema().

  14. Add prepare API support for streaming transactions in logical replication.

  15. Unify PostgresNode's new() and get_new_node() methods

  16. Use l*_node() family of functions where appropriate

  17. Add support for prepared transactions to built-in logical replication.

  18. Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.

  19. Rename a parse node to be more general

  20. Remove unused column atttypmod from initial tablesync query

  21. SEARCH and CYCLE clauses


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