Re: row filtering for logical replication

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/