Thread: fixing typo in comment for restriction_is_or_clause
Hi,
When I was looking at src/backend/optimizer/util/restrictinfo.c, I found a typo in one of the comments.
I also took the chance to simplify the code a little bit.
Please take a look at the patch.
Thanks
Attachment
On Tue, Oct 25, 2022 at 12:19 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
> Hi,
> When I was looking at src/backend/optimizer/util/restrictinfo.c, I found a typo in one of the comments.
Using "t" as an abbreviation for "true" was probably intentional, so not a typo. There is no doubt what the behavior is.
> I also took the chance to simplify the code a little bit.
It's perfectly clear and simple now, even if it doesn't win at "code golf".
--
John Naylor
EDB: http://www.enterprisedb.com
It's perfectly clear and simple now, even if it doesn't win at "code golf".
--
John Naylor
EDB: http://www.enterprisedb.com
On Tue, Oct 25, 2022 at 10:05 AM John Naylor <john.naylor@enterprisedb.com> wrote:
On Tue, Oct 25, 2022 at 12:19 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>
> Hi,
> When I was looking at src/backend/optimizer/util/restrictinfo.c, I found a typo in one of the comments.Using "t" as an abbreviation for "true" was probably intentional, so not a typo. There is no doubt what the behavior is.> I also took the chance to simplify the code a little bit.
It's perfectly clear and simple now, even if it doesn't win at "code golf".
Agree with your point. Do you think we can further make the one-line
function a macro or an inline function in the .h file? I think this
function is called quite frequently during planning, so maybe doing that
would bring a little bit of efficiency.
Thanks
Richard
function a macro or an inline function in the .h file? I think this
function is called quite frequently during planning, so maybe doing that
would bring a little bit of efficiency.
Thanks
Richard
On Tue, 25 Oct 2022 at 10:48, Richard Guo <guofenglinux@gmail.com> wrote: > On Tue, Oct 25, 2022 at 10:05 AM John Naylor <john.naylor@enterprisedb.com> > wrote: > >> >> On Tue, Oct 25, 2022 at 12:19 AM Zhihong Yu <zyu@yugabyte.com> wrote: >> > >> > Hi, >> > When I was looking at src/backend/optimizer/util/restrictinfo.c, I found >> a typo in one of the comments. >> >> Using "t" as an abbreviation for "true" was probably intentional, so not a >> typo. There is no doubt what the behavior is. >> >> > I also took the chance to simplify the code a little bit. >> >> It's perfectly clear and simple now, even if it doesn't win at "code golf". >> > > Agree with your point. Do you think we can further make the one-line > function a macro or an inline function in the .h file? I think this > function is called quite frequently during planning, so maybe doing that > would bring a little bit of efficiency. > +1, same goes for restriction_is_securely_promotable. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
On Mon, Oct 24, 2022 at 7:58 PM Japin Li <japinli@hotmail.com> wrote:
On Tue, 25 Oct 2022 at 10:48, Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Oct 25, 2022 at 10:05 AM John Naylor <john.naylor@enterprisedb.com>
> wrote:
>
>>
>> On Tue, Oct 25, 2022 at 12:19 AM Zhihong Yu <zyu@yugabyte.com> wrote:
>> >
>> > Hi,
>> > When I was looking at src/backend/optimizer/util/restrictinfo.c, I found
>> a typo in one of the comments.
>>
>> Using "t" as an abbreviation for "true" was probably intentional, so not a
>> typo. There is no doubt what the behavior is.
>>
>> > I also took the chance to simplify the code a little bit.
>>
>> It's perfectly clear and simple now, even if it doesn't win at "code golf".
>>
>
> Agree with your point. Do you think we can further make the one-line
> function a macro or an inline function in the .h file? I think this
> function is called quite frequently during planning, so maybe doing that
> would bring a little bit of efficiency.
>
+1, same goes for restriction_is_securely_promotable.
Hi,
Thanks for the comments.
Please take a look at patch v2.
Attachment
On Tue, 25 Oct 2022 at 11:07, Zhihong Yu <zyu@yugabyte.com> wrote:
> Please take a look at patch v2.
Maybe we should define those functions in headers. See patch v3.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index ef8df3d098..8a6812b4b1 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -375,41 +375,6 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op)
return result;
}
-/*
- * restriction_is_or_clause
- *
- * Returns t iff the restrictinfo node contains an 'or' clause.
- */
-bool
-restriction_is_or_clause(RestrictInfo *restrictinfo)
-{
- if (restrictinfo->orclause != NULL)
- return true;
- else
- return false;
-}
-
-/*
- * restriction_is_securely_promotable
- *
- * Returns true if it's okay to evaluate this clause "early", that is before
- * other restriction clauses attached to the specified relation.
- */
-bool
-restriction_is_securely_promotable(RestrictInfo *restrictinfo,
- RelOptInfo *rel)
-{
- /*
- * It's okay if there are no baserestrictinfo clauses for the rel that
- * would need to go before this one, *or* if this one is leakproof.
- */
- if (restrictinfo->security_level <= rel->baserestrict_min_security ||
- restrictinfo->leakproof)
- return true;
- else
- return false;
-}
-
/*
* get_actual_clauses
*
diff --git a/src/include/optimizer/restrictinfo.h b/src/include/optimizer/restrictinfo.h
index 6d30bd5e9d..dbfbebff48 100644
--- a/src/include/optimizer/restrictinfo.h
+++ b/src/include/optimizer/restrictinfo.h
@@ -31,9 +31,6 @@ extern RestrictInfo *make_restrictinfo(PlannerInfo *root,
Relids outer_relids,
Relids nullable_relids);
extern RestrictInfo *commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op);
-extern bool restriction_is_or_clause(RestrictInfo *restrictinfo);
-extern bool restriction_is_securely_promotable(RestrictInfo *restrictinfo,
- RelOptInfo *rel);
extern List *get_actual_clauses(List *restrictinfo_list);
extern List *extract_actual_clauses(List *restrictinfo_list,
bool pseudoconstant);
@@ -46,4 +43,36 @@ extern bool join_clause_is_movable_into(RestrictInfo *rinfo,
Relids currentrelids,
Relids current_and_outer);
+/*
+ * restriction_is_or_clause
+ *
+ * Returns true iff the restrictinfo node contains an 'or' clause.
+ */
+static inline bool
+restriction_is_or_clause(RestrictInfo *restrictinfo)
+{
+ return (restrictinfo->orclause != NULL);
+}
+
+/*
+ * restriction_is_securely_promotable
+ *
+ * Returns true if it's okay to evaluate this clause "early", that is before
+ * other restriction clauses attached to the specified relation.
+ */
+static inline bool
+restriction_is_securely_promotable(RestrictInfo *restrictinfo,
+ RelOptInfo *rel)
+{
+ /*
+ * It's okay if there are no baserestrictinfo clauses for the rel that
+ * would need to go before this one, *or* if this one is leakproof.
+ */
+ if (restrictinfo->security_level <= rel->baserestrict_min_security ||
+ restrictinfo->leakproof)
+ return true;
+ else
+ return false;
+}
+
#endif /* RESTRICTINFO_H */
On Tue, Oct 25, 2022 at 11:46 AM Japin Li <japinli@hotmail.com> wrote:
On Tue, 25 Oct 2022 at 11:07, Zhihong Yu <zyu@yugabyte.com> wrote:
> Please take a look at patch v2.
Maybe we should define those functions in headers. See patch v3.
Yes, putting them in .h file is better to me. For the v3 patch, we can
do the same one-line trick for restriction_is_securely_promotable.
Thanks
Richard
do the same one-line trick for restriction_is_securely_promotable.
Thanks
Richard
On Tue, 25 Oct 2022 at 12:01, Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Oct 25, 2022 at 11:46 AM Japin Li <japinli@hotmail.com> wrote:
>
>>
>> On Tue, 25 Oct 2022 at 11:07, Zhihong Yu <zyu@yugabyte.com> wrote:
>> > Please take a look at patch v2.
>>
>> Maybe we should define those functions in headers. See patch v3.
>
>
> Yes, putting them in .h file is better to me. For the v3 patch, we can
> do the same one-line trick for restriction_is_securely_promotable.
>
Fixed. Please consider the v4 for further review.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index ef8df3d098..8a6812b4b1 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -375,41 +375,6 @@ commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op)
return result;
}
-/*
- * restriction_is_or_clause
- *
- * Returns t iff the restrictinfo node contains an 'or' clause.
- */
-bool
-restriction_is_or_clause(RestrictInfo *restrictinfo)
-{
- if (restrictinfo->orclause != NULL)
- return true;
- else
- return false;
-}
-
-/*
- * restriction_is_securely_promotable
- *
- * Returns true if it's okay to evaluate this clause "early", that is before
- * other restriction clauses attached to the specified relation.
- */
-bool
-restriction_is_securely_promotable(RestrictInfo *restrictinfo,
- RelOptInfo *rel)
-{
- /*
- * It's okay if there are no baserestrictinfo clauses for the rel that
- * would need to go before this one, *or* if this one is leakproof.
- */
- if (restrictinfo->security_level <= rel->baserestrict_min_security ||
- restrictinfo->leakproof)
- return true;
- else
- return false;
-}
-
/*
* get_actual_clauses
*
diff --git a/src/include/optimizer/restrictinfo.h b/src/include/optimizer/restrictinfo.h
index 6d30bd5e9d..b61707fd26 100644
--- a/src/include/optimizer/restrictinfo.h
+++ b/src/include/optimizer/restrictinfo.h
@@ -31,9 +31,6 @@ extern RestrictInfo *make_restrictinfo(PlannerInfo *root,
Relids outer_relids,
Relids nullable_relids);
extern RestrictInfo *commute_restrictinfo(RestrictInfo *rinfo, Oid comm_op);
-extern bool restriction_is_or_clause(RestrictInfo *restrictinfo);
-extern bool restriction_is_securely_promotable(RestrictInfo *restrictinfo,
- RelOptInfo *rel);
extern List *get_actual_clauses(List *restrictinfo_list);
extern List *extract_actual_clauses(List *restrictinfo_list,
bool pseudoconstant);
@@ -46,4 +43,33 @@ extern bool join_clause_is_movable_into(RestrictInfo *rinfo,
Relids currentrelids,
Relids current_and_outer);
+/*
+ * restriction_is_or_clause
+ *
+ * Returns true iff the restrictinfo node contains an 'or' clause.
+ */
+static inline bool
+restriction_is_or_clause(RestrictInfo *restrictinfo)
+{
+ return (restrictinfo->orclause != NULL);
+}
+
+/*
+ * restriction_is_securely_promotable
+ *
+ * Returns true if it's okay to evaluate this clause "early", that is before
+ * other restriction clauses attached to the specified relation.
+ */
+static inline bool
+restriction_is_securely_promotable(RestrictInfo *restrictinfo,
+ RelOptInfo *rel)
+{
+ /*
+ * It's okay if there are no baserestrictinfo clauses for the rel that
+ * would need to go before this one, *or* if this one is leakproof.
+ */
+ return (restrictinfo->security_level <= rel->baserestrict_min_security ||
+ restrictinfo->leakproof);
+}
+
#endif /* RESTRICTINFO_H */
On Tue, Oct 25, 2022 at 9:48 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
>
> On Tue, Oct 25, 2022 at 10:05 AM John Naylor <john.naylor@enterprisedb.com> wrote:
>>
>> It's perfectly clear and simple now, even if it doesn't win at "code golf".
>
>
> Agree with your point. Do you think we can further make the one-line
> function a macro or an inline function in the .h file? I think this
> function is called quite frequently during planning, so maybe doing that
> would bring a little bit of efficiency.
My point was misunderstood, which is: I don't think we need to do anything at all here if the goal was purely about aesthetics.
If the goal has now changed to efficiency, I have no opinion about that yet since no evidence has been presented.
--
John Naylor
EDB: http://www.enterprisedb.com
--
John Naylor
EDB: http://www.enterprisedb.com
On 2022-Oct-25, Richard Guo wrote: > Agree with your point. Do you think we can further make the one-line > function a macro or an inline function in the .h file? We can, but should we? > I think this function is called quite frequently during planning, so > maybe doing that would bring a little bit of efficiency. You'd need to measure it and show some gains. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
On Tue, Oct 25, 2022 at 2:25 PM John Naylor <john.naylor@enterprisedb.com> wrote:
On Tue, Oct 25, 2022 at 9:48 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
>
> On Tue, Oct 25, 2022 at 10:05 AM John Naylor <john.naylor@enterprisedb.com> wrote:
>>
>> It's perfectly clear and simple now, even if it doesn't win at "code golf".
>
>
> Agree with your point. Do you think we can further make the one-line
> function a macro or an inline function in the .h file? I think this
> function is called quite frequently during planning, so maybe doing that
> would bring a little bit of efficiency.
My point was misunderstood, which is: I don't think we need to do anything at all here if the goal was purely about aesthetics.If the goal has now changed to efficiency, I have no opinion about that yet since no evidence has been presented.
Now I think I've got your point. Sorry for the misread.
Your concern makes sense. When talking about efficiency we'd better
attach some concrete proof, such as benchmark tests.
Thanks
Richard
Your concern makes sense. When talking about efficiency we'd better
attach some concrete proof, such as benchmark tests.
Thanks
Richard
On Tue, Oct 25, 2022 at 3:37 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2022-Oct-25, Richard Guo wrote:
> Agree with your point. Do you think we can further make the one-line
> function a macro or an inline function in the .h file?
We can, but should we?
> I think this function is called quite frequently during planning, so
> maybe doing that would bring a little bit of efficiency.
You'd need to measure it and show some gains.
Yeah, that is what has to be done to make it happen.
Thanks
Richard
Thanks
Richard