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.
Attach the V76 patch set which addressed above comments and comments from[1][2].
[1] https://www.postgresql.org/message-id/CAA4eK1L6hLRxFVphDO8mwuguc9kVdMu-DT2Dw2GXHwvprLoxrw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1L6hLRxFVphDO8mwuguc9kVdMu-DT2Dw2GXHwvprLoxrw%40mail.gmail.com
Best regards,
Hou zj
