RE: Add Nullif case for eval_const_expressions_mutator

From: "Hou, Zhijie" <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>
To: Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: Add Nullif case for eval_const_expressions_mutator
Date: 2021-01-20 01:16:38
Message-ID: ab53b3dbdbd6436f970f33b51ccd7dd3@G08CNEXMBPEKD05.g08.fujitsu.local
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi

Thanks for the review.

> It's a bit unfortunate now that between OpExpr, DistinctExpr, NullIfExpr,
> and to a lesser extent ScalarArrayOpExpr we will now have several different
> implementations of nearly the same thing, without any explanation why one
> approach was chosen here and another there. We should at least document
> this.

I am not quiet sure where to document the difference.
Temporarily, I tried to add some comments for the Nullif to explain why this one is different.

+ /*
+ * Since NullIf is not common enough to deserve specially
+ * optimized code, use ece_generic_processing and
+ * ece_evaluate_expr to simplify the code as much as possible.
+ */

Any suggestions ?

> Some inconsistencies I found: The code for DistinctExpr calls
> expression_tree_mutator() directly, but your code for NullIfExpr calls
> ece_generic_processing(), even though the explanation in the comment for
> DistinctExpr would apply there as well.
>
> Your code for NullIfExpr doesn't appear to call set_opfuncid() anywhere.

IMO, we will call set_opfuncid in function ece_evaluate_expr.

Like the following flow:
ece_evaluate_expr-->evaluate_expr--> fix_opfuncids--> fix_opfuncids_walker--> set_opfuncid

And we do not need the opfuncid till we call ece_evaluate_expr.
So, to simplify the code as much as possible, I did not call set_opfuncid in eval_const_expressions_mutator again.

> I would move your new block for NullIfExpr after the block for DistinctExpr.
> That's the order in which these blocks appear elsewhere for generic node
> processing.
>

Changed.

> Check your whitespace usage:
>
> if(!has_nonconst_input)
>
> should have a space after the "if". (It's easy to fix of course, but I
> figure I'd point it out here since you have submitted several patches with
> this style, so it's perhaps a habit to break.)

Changed.

> Perhaps add a comment to the tests like this so it's clear what they are
> for:
>
> diff --git a/src/test/regress/sql/case.sql
> b/src/test/regress/sql/case.sql index 4742e1d0e0..98e3fb8de5 100644
> --- a/src/test/regress/sql/case.sql
> +++ b/src/test/regress/sql/case.sql
> @@ -137,6 +137,7 @@ CREATE TABLE CASE2_TBL (
> FROM CASE_TBL a, CASE2_TBL b
> WHERE COALESCE(f,b.i) = 2;
>
> +-- Tests for constant subexpression simplification
> explain (costs off)
> SELECT * FROM CASE_TBL WHERE NULLIF(1, 2) = 2;

Added.

Attatching v3 patch, please consider it for further review.

Best regards,
houzj

Attachment Content-Type Size
v3_0001-add-nullif-case-for-eval_const_expressions.patch application/octet-stream 3.1 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message tsunakawa.takay@fujitsu.com 2021-01-20 01:32:47 RE: Parallel INSERT (INTO ... SELECT ...)
Previous Message Peter Geoghegan 2021-01-20 01:14:25 Re: New IndexAM API controlling index vacuum strategies