Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m2fRi-0005hB-5m for pgsql-hackers@arkaria.postgresql.org; Sun, 11 Jul 2021 19:48:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1m2fRf-0004VH-CM for pgsql-hackers@arkaria.postgresql.org; Sun, 11 Jul 2021 19:48:55 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m2fRf-0004V9-3Q for pgsql-hackers@lists.postgresql.org; Sun, 11 Jul 2021 19:48:55 +0000 Received: from new3-smtp.messagingengine.com ([66.111.4.229]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m2fRb-0002bH-Pe for pgsql-hackers@lists.postgresql.org; Sun, 11 Jul 2021 19:48:54 +0000 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id D9708580781; Sun, 11 Jul 2021 15:48:48 -0400 (EDT) Received: from imap21 ([10.202.2.71]) by compute1.internal (MEProxy); Sun, 11 Jul 2021 15:48:48 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eulerto.com; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm2; bh=hwuhoMdS4K8OgDet1cgpvJM1qjGKVGN nOOspQPSzDY0=; b=UUW2wnxEfsOiPfzUI7PkoLs5MZJ6q7LR3TrmlalScWD5Ixp Y3+Qt7XI1rGYuClRdnqhsLjj7yOXQqT3c6yPBfJMu2IGk5iQBIW9ER61USRM31xn j3wnduv4dxc+Jyiuo3UqUohnXzGplp2MfZaLpsvcGLVZOsfeJAYkikr7FRnHP8lY /LXPsiy7LNCIM8Nl4WZNNo4yqspoG1brYgrbNcBt08GNAb4wma6wEbKlLtIzw2cN KDoCzV1C6pbmyy2YrSRpDk8maSeIazlLAqbAzpeQHYXWjlcDsn+0idxfzFShcshN mKJ8a94eHXTe/+9sf/qtwFynNCuQQTiKqIbEPIw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=hwuhoM dS4K8OgDet1cgpvJM1qjGKVGNnOOspQPSzDY0=; b=HBmzv2mlk8UM9VRGPhZhf4 bUtT9z0EVh4QAIxas4zoA/eEML/Xdin1JXXZq7n8A7d78edeLq6ZMTNTppbOwOMb rNtGR4eXjb1yX8Zu/rSnqkBvRb+LkUpGabSWt+j1wzHV3KTSpmoam0UMNg7KBQt3 EFiTdtazKIwBYskrl8eZcGJaPjxxpiC+drTZ2nJu6Y0rM1Hr1fD+VabFVTSBS6Ju KgzCqoh/01XY4eLSYKr5UufcWRX9ib4V3u00Ph0aRKbhEBdKkVUPke5Jm0fb7Vbc ha0v5md/cWo3dTmteXdvMQYAJONHzRk1s9RZV/BF5tAIGS4dV/Xq0tDZk+/y+zkQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddruddtgddugedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesrgdtreerreertdenucfhrhhomhepfdfguhhl vghrucfvrghvvghirhgrfdcuoegvuhhlvghrsegvuhhlvghrthhordgtohhmqeenucggtf frrghtthgvrhhnpeejtefhuedvgffftdetvedufedtjeeuffekleevkefhveekgeelfefh jeehtdejgeenucffohhmrghinhepvghnthgvrhhprhhishgvuggsrdgtohhmnecuvehluh hsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepvghulhgvrhesvghu lhgvrhhtohdrtghomh X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 03AEC51C0060; Sun, 11 Jul 2021 15:48:47 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-531-g1160beca77-fm-20210705.001-g1160beca Mime-Version: 1.0 Message-Id: <34b61db2-3e26-42ed-bb84-b14a9fe0a60d@www.fastmail.com> In-Reply-To: References: <8cf1ea71-b609-00e8-b8f0-c85ba61b7312@pgmasters.net> <4dbad191-9b40-4bb6-8940-d18f2e93c3ce@www.fastmail.com> <5b9fceed-8f91-467f-a64e-2134c5c3f496@www.fastmail.com> <52edc9d2-8e52-4cde-9a15-991961b78387@www.fastmail.com> <9d5ef845-391a-4c66-b298-0f594d9e6828@www.fastmail.com> <7e4b5c48-a3d3-4b7e-b455-2d50c60a0ab7@www.fastmail.com> <57373e8b-1264-cd37-404e-8edbcf7884cc@enterprisedb.com> <86d76e89-9a96-411a-8077-f8918cbae2e1@www.fastmail.com> <3dbf7241-a417-45c9-9a0e-04a079de9b9f@www.fastmail.com> <0ca5b27f-edf8-4ef9-a9e2-533216890f62@www.fastmail.com> Date: Sun, 11 Jul 2021 16:48:26 -0300 From: "Euler Taveira" To: "Greg Nancarrow" Cc: "Amit Kapila" , "Peter Smith" , "Rahila Syed" , "Peter Eisentraut" , =?UTF-8?Q?=C3=96nder_Kalac=C4=B1?= , japin , "Michael Paquier" , "David Steele" , "Craig Ringer" , "Tomas Vondra" , "Amit Langote" , "PostgreSQL Hackers" Subject: Re: row filtering for logical replication Content-Type: multipart/alternative; boundary=9bcb21910be6413fa7a67fbf62daf1cb List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --9bcb21910be6413fa7a67fbf62daf1cb Content-Type: text/plain 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/ --9bcb21910be6413fa7a67fbf62daf1cb Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Mon, Ju= l 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 missin= g words and things which could be better expressed)
I incorporated all your wording suggestions.

(2) Some inconsi= stent error message wording:

Currently:
=
err =3D _("cannot use subquery in publication WHERE expressio= n");

Suggest changing it to:
= err =3D _("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
sepa= rate patch to change all sentences.


Other examples fr= om the patch:
err =3D _("aggregate functions are not allow= ed in publication WHERE expressions");
err =3D _("grouping= operations are not allowed in publication WHERE expressions");
err =3D _("window functions are not allowed in publication WHERE e= xpressions");
errmsg("functions are not allowed in publica= tion WHERE expressions"),
err =3D _("set-returning functio= ns are not allowed in publication WHERE
expressions");
=
This is a different function. I just followed th= e same wording from similar
sentences around it.
=


<= /div>
(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 she= ll error-check in make_op():

/* User-define= d operators are not allowed in publication WHERE clauses */
if (pstate->p_expr_kind =3D=3D EXPR_KIND_PUBLICATION_WHERE &&am= p; opform->oid
>=3D FirstNormalObjectId)
    ereport(ERROR,
    (e= rrcode(ERRCODE_FEATURE_NOT_SUPPORTED),
    = errmsg("user-defined operators are not allowed in publication
<= div>WHERE expressions"),
    parser_errposi= tion(pstate, location)));
I'm still working o= n a way to accept built-in functions but while we don't have
it, let's forbid custom operators too.

<= br>

<= div>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 WHER= E
expressions in one central place, rather than scattered = all over the
place, but that might be easier said than don= e.
You need to update the patch comment accordingly.
I forgot to mention it in the patch I sent a few m= inutes ago. I'm not sure we
need to mention every error co= ndition (specially one that will be rarely used).

(4) src/backend/re= plication/pgoutput/pgoutput.c
pgoutput_change()
<= div>
The 3 added calls to pgoutput_row_filter() are return= ing from
pgoutput_change(), if false is returned, but inst= ead they should break
from the switch, otherwise cleanup c= ode is missed. This is surely a
bug.
Fixed.

In summary, v18 contains

* Peter Smith's review
* Greg Nanc= arrow's review
* cache ExprState
* cache Tup= leTableSlot
* forbid custom operators
* vari= ous fixes


--
Euler Ta= veira
EDB   https://www.enterprisedb.com/

--9bcb21910be6413fa7a67fbf62daf1cb--