Horton

Re: row filtering for logical replication

Euler Taveira <euler@eulerto.com>

View thread
From: "Euler Taveira" <euler@eulerto.com>
To: "Greg Nancarrow" <gregn4422@gmail.com>
Cc: "Amit Kapila" <amit.kapila16@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-11T19:48:26Z
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 Mon, Jul 5, 2021, at 12:14 AM, Greg Nancarrow wrote:
> I have some review comments on the "Row filter for logical replication" patch:
> 
> (1) Suggested update to patch comment:
> (There are some missing words and things which could be better expressed)
I incorporated all your wording suggestions.

> (2) Some inconsistent error message wording:
> 
> Currently:
> err = _("cannot use subquery in publication WHERE expression");
> 
> Suggest changing it to:
> err = _("subqueries are not allowed in publication WHERE expressions");
The same expression "cannot use subquery in ..." is used in the other switch
cases. If you think this message can be improved, I suggest that you submit a
separate patch to change all sentences.

> 
> Other examples from the patch:
> err = _("aggregate functions are not allowed in publication WHERE expressions");
> err = _("grouping operations are not allowed in publication WHERE expressions");
> err = _("window functions are not allowed in publication WHERE expressions");
> errmsg("functions are not allowed in publication WHERE expressions"),
> err = _("set-returning functions are not allowed in publication WHERE
> expressions");
This is a different function. I just followed the same wording from similar
sentences around it.

> 
> (3) The current code still allows arbitrary code execution, e.g. via a
> user-defined operator:
I fixed it in v18.

> Perhaps add the following after the existing shell error-check in make_op():
> 
> /* User-defined operators are not allowed in publication WHERE clauses */
> if (pstate->p_expr_kind == EXPR_KIND_PUBLICATION_WHERE && opform->oid
> >= FirstNormalObjectId)
>     ereport(ERROR,
>     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>     errmsg("user-defined operators are not allowed in publication
> WHERE expressions"),
>     parser_errposition(pstate, location)));
I'm still working on a way to accept built-in functions but while we don't have
it, let's forbid custom operators too.


> 
> Also, I believe it's also allowing user-defined CASTs (so could add a
> similar check to above in transformTypeCast()).
> Ideally, it would be preferable to validate/check publication WHERE
> expressions in one central place, rather than scattered all over the
> place, but that might be easier said than done.
> You need to update the patch comment accordingly.
I forgot to mention it in the patch I sent a few minutes ago. I'm not sure we
need to mention every error condition (specially one that will be rarely used).

> (4) src/backend/replication/pgoutput/pgoutput.c
> pgoutput_change()
> 
> The 3 added calls to pgoutput_row_filter() are returning from
> pgoutput_change(), if false is returned, but instead they should break
> from the switch, otherwise cleanup code is missed. This is surely a
> bug.
Fixed.

In summary, v18 contains

* Peter Smith's review
* Greg Nancarrow's review
* cache ExprState
* cache TupleTableSlot
* forbid custom operators
* various fixes


--
Euler Taveira
EDB   https://www.enterprisedb.com/