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