On Thu, Nov 18, 2021 at 11:02 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Nov 15, 2021 at 9:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 5. Why do you need a separate variable rowfilter_valid to indicate
> > whether a valid row filter exists? Why exprstate is not sufficient?
> > Can you update comments to indicate why we need this variable
> > separately?
>
> I have improved the (existing) comment in v40 [1].
>
> >
> > 0004*
> > 6. In rowfilter_expr_checker(), the expression tree is traversed
> > twice, can't we traverse it once to detect all non-allowed stuff? It
> > can be sometimes costly to traverse the tree multiple times especially
> > when the expression is complex and it doesn't seem acceptable to do so
> > unless there is some genuine reason for the same.
>
> I kind of doubt there would be any perceptible difference for 2
> traverses instead of 1 because:
> a) filters are limited to simple expressions. Yes, a large boolean
> expression is possible but I don't think it is likely.
>
But in such cases, it will be quite costly and more importantly, I
don't see any good reason why we need to traverse it twice..
> b) the validation part is mostly a one-time execution only when the
> filter is created or changed.
>
> Anyway, I am happy to try to refactor the logic to a single traversal
> as suggested, but I'd like to combine those "validation" patches
> (v40-0005, v40-0006) first, so I can combine their walker logic. Is it
> OK?
>
That should be okay. You can combine the logic of v40-0005 and
v40-0006, and then change it so that you need to traverse the
expression once.
--
With Regards,
Amit Kapila.