On Thu, Nov 18, 2021 at 11:02 AM Peter Smith <smithpb2...@gmail.com> wrote: > > On Mon, Nov 15, 2021 at 9:31 PM Amit Kapila <amit.kapil...@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.
