Re: row filtering for logical replication

Поиск
Список
Период
Сортировка
От Önder Kalacı
Тема Re: row filtering for logical replication
Дата
Msg-id CACawEhW_iMnY9XK2tEb1ig+A+gKeB4cxdJcxMsoCU0SaKPExxg@mail.gmail.com
обсуждение исходный текст
Ответ на Re: row filtering for logical replication  (David Steele <david@pgmasters.net>)
Ответы Re: row filtering for logical replication  (Masahiko Sawada <sawada.mshk@gmail.com>)
Re: row filtering for logical replication  (Andres Freund <andres@anarazel.de>)
Список pgsql-hackers
Дерево обсуждения
Hi all,

I'm also interested in this patch. I rebased the changes to the current master branch and attached. The rebase had two issues. First, patch-8 was conflicting, and that seems only helpful for debugging purposes during development. So, I dropped it for simplicity. Second, the changes have a conflict with `publish_via_partition_root` changes. I tried to fix the issues, but ended-up having a limitation for now. The limitation is that "cannot create publication with WHERE clause on the partitioned table without publish_via_partition_root is set to true". This restriction can be lifted, though I left out for the sake of focusing on the some issues that I observed on this patch.

Please see my review:

+       if (list_length(relentry->qual) > 0)
+       {
+               HeapTuple       old_tuple;
+               HeapTuple       new_tuple;
+               TupleDesc       tupdesc;
+               EState     *estate;
+               ExprContext *ecxt;
+               MemoryContext oldcxt;
+               ListCell   *lc;
+               bool            matched = true;
+
+               old_tuple = change->data.tp.oldtuple ? &change->data.tp.oldtuple->tuple : NULL;
+               new_tuple = change->data.tp.newtuple ? &change->data.tp.newtuple->tuple : NULL;
+               tupdesc = RelationGetDescr(relation);
+               estate = create_estate_for_relation(relation);
+
+               /* prepare context per tuple */
+               ecxt = GetPerTupleExprContext(estate);
+               oldcxt = MemoryContextSwitchTo(estate->es_query_cxt);
+               ecxt->ecxt_scantuple = ExecInitExtraTupleSlot(estate, tupdesc, &TTSOpsHeapTuple);
+
+               ExecStoreHeapTuple(new_tuple ? new_tuple : old_tuple, ecxt->ecxt_scantuple, false);
+
+               foreach(lc, relentry->qual)
+               {
+                       Node       *qual;
+                       ExprState  *expr_state;
+                       Expr       *expr;
+                       Oid                     expr_type;
+                       Datum           res;
+                       bool            isnull;
+
+                       qual = (Node *) lfirst(lc);
+
+                       /* evaluates row filter */
+                       expr_type = exprType(qual);
+                       expr = (Expr *) coerce_to_target_type(NULL, qual, expr_type, BOOLOID, -1, COERCION_ASSIGNMENT, COERCE_IMPLICIT_CAST, -1);
+                       expr = expression_planner(expr);
+                       expr_state = ExecInitExpr(expr, NULL);
+                       res = ExecEvalExpr(expr_state, ecxt, &isnull);
+
+                       /* if tuple does not match row filter, bail out */
+                       if (!DatumGetBool(res) || isnull)
+                       {
+                               matched = false;
+                               break;
+                       }
+               }
+
+               MemoryContextSwitchTo(oldcxt);
+


The above part can be considered the core of the logic, executed per tuple. As far as I can see, it has two downsides.

First, calling `expression_planner()` for every tuple can be quite expensive. I created a sample table, loaded data and ran a quick benchmark to see its effect. I attached the very simple script that I used to reproduce the issue on my laptop. I'm pretty sure you can find nicer ways of doing similar perf tests, just sharing as a reference.

The idea of the test is to add a WHERE clause to a table, but none of the tuples are filtered out. They just go through this code-path and send it to the remote node.  

#rows       Patched    | Master
1M  00:00:25.067536    | 00:00:16.633988
10M  00:04:50.770791    | 00:02:40.945358


So, it seems a significant overhead to me. What do you think?

Secondly, probably more importantly, allowing any operator is as dangerous as allowing any function as users can create/overload operator(s). For example, assume that users create an operator which modifies the table that is being filtered out:

```
CREATE OR REPLACE FUNCTION function_that_modifies_table(left_art INTEGER, right_arg INTEGER)
RETURNS BOOL AS
$$
BEGIN
 
  INSERT INTO test SELECT * FROM test;
  
  return left_art > right_arg;
 END;
$$ LANGUAGE PLPGSQL VOLATILE;

CREATE OPERATOR >>= (
  PROCEDURE = function_that_modifies_table,
  LEFTARG = INTEGER,
  RIGHTARG = INTEGER
);

CREATE PUBLICATION pub FOR TABLE test WHERE (key >>= 0);
``

With the above, we seem to be in trouble. Although the above is an extreme example, it felt useful to share to the extent of the problem. We probably cannot allow any free-form SQL to be on the filters.

To overcome these issues, one approach could be to rely on known safe operators and functions. I believe the btree and hash operators should provide a pretty strong coverage across many use cases. As far as I can see, the procs that the following query returns can be our baseline:

```
select   DISTINCT amproc.amproc::regproc AS opfamily_procedure
from     pg_am am,
         pg_opfamily opf,
         pg_amproc amproc
where    opf.opfmethod = am.oid
and      amproc.amprocfamily = opf.oid
order by 
         opfamily_procedure;
```

With that, we aim to prevent users easily shooting themselves by the foot.

The other problematic area was the performance, as calling `expression_planner()` for every tuple can be very expensive. To avoid that, it might be considered to ask users to provide a function instead of a free form WHERE clause, such that if the function returns true, the tuple is sent. The allowed functions need to be immutable SQL functions with bool return type. As we can parse the  SQL functions, we should be able to allow only functions that rely on the above mentioned procs. We can apply as many restrictions (such as no modification query) as possible. For example, see below:
```

CREATE OR REPLACE function filter_tuples_for_test(int) returns bool as
$body$
    select $1 > 100;
$body$
language sql immutable;

CREATE PUBLICATION pub FOR TABLE test FILTER = filter_tuples_for_tes(key);
```

In  terms of performance, calling the function should avoid calling the `expression_planner()` and yield better performance. Though, this needs to be verified.

If such an approach makes sense, I'd be happy to work on the patch. Please provide me feedback.

Thanks,
Onder KALACI
Software Engineer at Microsoft &
Developing the Citus database extension for PostgreSQL

David Steele <david@pgmasters.net>, 16 Ara 2020 Çar, 21:43 tarihinde şunu yazdı:
On 3/3/20 12:39 PM, David Steele wrote:
> Hi Euler,
>
> On 1/21/20 2:32 AM, Craig Ringer wrote:
>> On Fri, 17 Jan 2020 at 07:58, Euler Taveira <euler@timbira.com.br> wrote:
>>>
>>> Em qui., 16 de jan. de 2020 às 18:57, Tomas Vondra
>>> <tomas.vondra@2ndquadrant.com> escreveu:
>>>>
>>>> Euler, this patch is still in "waiting on author" since 11/25. Do you
>>>> plan to review changes made by Amit in the patches he submitted, or
>>>> what
>>>> are your plans with this patch?
>>>>
>>> Yes, I'm working on Amit suggestions. I'll post a new patch as soon
>>> as possible.
>>
>> Great. I think this'd be nice to see.
>
> The last CF for PG13 has started. Do you have a new patch ready?

I have marked this patch Returned with Feedback since no new patch has
been posted.

Please submit to a future CF when a new patch is available.

Regards,
--
-David
david@pgmasters.net




Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Masahiko Sawada
Дата:
Сообщение: Re: [HACKERS] logical decoding of two-phase transactions
Следующее
От: Thomas Munro
Дата:
Сообщение: Re: Cache relation sizes?