fixing typo in comment for restriction_is_or_clause

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

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

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.

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.

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

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.

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

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

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