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/