RE: row filtering for logical replication

On Thursday, February 3, 2022 11:11 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
> On Tuesday, February 1, 2022 7:22 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Feb 1, 2022 at 9:15 AM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com>
> > wrote:
> > >
> > > On Monday, January 31, 2022 9:02 PM Amit Kapila
> > > <amit.kapila16@gmail.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.
> 
> I adjusted the code in check_simple_rowfilter_expr_walker to handle the
> collation/type/function.
> 
> > 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.
> 
> I will look into this and change it soon.

Since the v76-0000-clean-up-pgoutput-cache-invalidation.patch has been
committed, attach a new version patch set to make the cfbot happy. Also
addressed the above comments related to tab-complete in 0002 patch.

Best regards,
Hou zj