Re: row filtering for logical replication

On Tue, Feb 1, 2022 at 9:15 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, January 31, 2022 9:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> >

Review Comments:
===============
1.
+ else if (IsA(node, OpExpr))
+ {
+ /* OK, except user-defined operators are not allowed. */
+ if (((OpExpr *) node)->opno >= FirstNormalObjectId)
+ errdetail_msg = _("User-defined operators are not allowed.");
+ }

Is it sufficient to check only the allowed operators for OpExpr? Don't
we need to check opfuncid to ensure that the corresponding function is
immutable? Also, what about opresulttype, opcollid, and inputcollid? I
think we don't want to allow user-defined types or collations but as
we are restricting the opexp to use a built-in operator, those should
not be present in such an expression. If that is the case, then I
think we can add a comment for the same.

2. Can we handle RelabelType node in
check_simple_rowfilter_expr_walker()? I think you need to check
resulttype and collation id to ensure that they are not user-defined.
There doesn't seem to be a need to check resulttypmod as that refers
to pg_attribute.atttypmod and that can't have anything unsafe. This
helps us to handle cases like the following which currently gives an
error:
create table t1(c1 int, c2 varchar(100));
create publication pub1 for table t1 where (c2 < 'john');

3. Similar to above, don't we need to consider disallowing
non-built-in collation of Var type nodes? Now, as we are only
supporting built-in types this might not be required. So, probably a
comment would suffice.

4.
A minor nitpick in tab-complete:
postgres=# Alter PUBLICATION pub1 ADD TABLE t2 WHERE ( c2 > 10)
, WHERE (

After the Where clause, it should not allow adding WHERE. This doesn't
happen for CREATE PUBLICATION case.

--
With Regards,
Amit Kapila.