Re: [HACKERS] Small code improvement for btree

[Masahiko Sawada]

On Tue, Sep 19, 2017 at 5:39 AM, Tom Lane  wrote:
> Doug Doole  writes:
>> Looks good to me.
>> The new status of this patch is: Ready for Committer
>
> Pushed.  Grepping found a few more places that should be changed to
> use these macros rather than referencing btpo_flags directly,
> so I did that.

Thank you!

> I tend to agree with Alvaro that it'd be better to get rid of
> BT_READ/BT_WRITE in favor of using the same buffer flags used
> elsewhere; but I also agree that as long as they're there we
> should use them, so I included that change as well.
>

Agreed. Thanks, again.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Tom Lane]

Doug Doole  writes:
> Looks good to me.
> The new status of this patch is: Ready for Committer

Pushed.  Grepping found a few more places that should be changed to
use these macros rather than referencing btpo_flags directly,
so I did that.

I tend to agree with Alvaro that it'd be better to get rid of
BT_READ/BT_WRITE in favor of using the same buffer flags used
elsewhere; but I also agree that as long as they're there we
should use them, so I included that change as well.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Doug Doole]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:not tested

Looks good to me.

The new status of this patch is: Ready for Committer

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Masahiko Sawada]

On Wed, Aug 9, 2017 at 11:23 AM, Masahiko Sawada  wrote:
> On Mon, Aug 7, 2017 at 1:44 PM, Masahiko Sawada  wrote:
>> On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan  wrote:
>>> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
>>>  wrote:
 Interesting.  We learned elsewhere that it's better to integrate the
 "!= 0" test as part of the macro definition; so a
 better formulation of this patch would be to change the
 P_INCOMPLETE_SPLIT macro and omit the comparison in the Assert.  (See
 commit 594e61a1de03 for an example).
>>
>> Thank you for the information. The macros other than
>> P_INCOMPLETE_SPLIT in btree.h such as P_ISLEAF, P_ISROOT also doesn't
>> return booleans. Should we deal with them as well?
>>


> -   LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
> +   LockBuffer(hbuffer, BT_READ);
>>>
>>> +1.
>>>
>>> One Linus Torvalds rant that I actually agreed with was a rant against
>>> the use of bool as a type in C code. It's fine, as long as you never
>>> forget that it's actually just another integer.
>>>
 I think BT_READ and BT_WRITE are useless, and I'd rather get rid of
 them ...
>>>
>>> Fair enough, but we should either use them consistently or not at all.
>>> I'm not especially concerned about which, as long as it's one of those
>>> two.
>>>
>>
>> I definitely agreed.
>>
>
> Attached updated patch. I'll add it to next CF.
>

Added to the next CF. Feedback and comment are very welcome.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Masahiko Sawada]

On Mon, Aug 7, 2017 at 1:44 PM, Masahiko Sawada  wrote:
> On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan  wrote:
>> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
>>  wrote:
>>> Interesting.  We learned elsewhere that it's better to integrate the
>>> "!= 0" test as part of the macro definition; so a
>>> better formulation of this patch would be to change the
>>> P_INCOMPLETE_SPLIT macro and omit the comparison in the Assert.  (See
>>> commit 594e61a1de03 for an example).
>
> Thank you for the information. The macros other than
> P_INCOMPLETE_SPLIT in btree.h such as P_ISLEAF, P_ISROOT also doesn't
> return booleans. Should we deal with them as well?
>
>>>
>>>
 -   LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
 +   LockBuffer(hbuffer, BT_READ);
>>
>> +1.
>>
>> One Linus Torvalds rant that I actually agreed with was a rant against
>> the use of bool as a type in C code. It's fine, as long as you never
>> forget that it's actually just another integer.
>>
>>> I think BT_READ and BT_WRITE are useless, and I'd rather get rid of
>>> them ...
>>
>> Fair enough, but we should either use them consistently or not at all.
>> I'm not especially concerned about which, as long as it's one of those
>> two.
>>
>
> I definitely agreed.
>

Attached updated patch. I'll add it to next CF.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


code_improvement_for_btree_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Masahiko Sawada]

On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan  wrote:
> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
>  wrote:
>> Interesting.  We learned elsewhere that it's better to integrate the
>> "!= 0" test as part of the macro definition; so a
>> better formulation of this patch would be to change the
>> P_INCOMPLETE_SPLIT macro and omit the comparison in the Assert.  (See
>> commit 594e61a1de03 for an example).

Thank you for the information. The macros other than
P_INCOMPLETE_SPLIT in btree.h such as P_ISLEAF, P_ISROOT also doesn't
return booleans. Should we deal with them as well?

>>
>>
>>> -   LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
>>> +   LockBuffer(hbuffer, BT_READ);
>
> +1.
>
> One Linus Torvalds rant that I actually agreed with was a rant against
> the use of bool as a type in C code. It's fine, as long as you never
> forget that it's actually just another integer.
>
>> I think BT_READ and BT_WRITE are useless, and I'd rather get rid of
>> them ...
>
> Fair enough, but we should either use them consistently or not at all.
> I'm not especially concerned about which, as long as it's one of those
> two.
>

I definitely agreed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Peter Geoghegan]

On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
 wrote:
> Interesting.  We learned elsewhere that it's better to integrate the
> "!= 0" test as part of the macro definition; so a
> better formulation of this patch would be to change the
> P_INCOMPLETE_SPLIT macro and omit the comparison in the Assert.  (See
> commit 594e61a1de03 for an example).
>
>
>> -   LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
>> +   LockBuffer(hbuffer, BT_READ);

+1.

One Linus Torvalds rant that I actually agreed with was a rant against
the use of bool as a type in C code. It's fine, as long as you never
forget that it's actually just another integer.

> I think BT_READ and BT_WRITE are useless, and I'd rather get rid of
> them ...

Fair enough, but we should either use them consistently or not at all.
I'm not especially concerned about which, as long as it's one of those
two.

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Small code improvement for btree

[Alvaro Herrera]

Masahiko Sawada wrote:

> While hacking the btree code I found two points we can improve in nbtxlog.c.
> 
> @@ -135,7 +135,7 @@ _bt_clear_incomplete_split(XLogReaderState
> *record, uint8 block_id)
> Pagepage = (Page) BufferGetPage(buf);
> BTPageOpaque pageop = (BTPageOpaque)
> PageGetSpecialPointer(page);
> 
> -   Assert((pageop->btpo_flags & BTP_INCOMPLETE_SPLIT) != 0);
> +Assert(P_INCOMPLETE_SPLIT(pageop) != 0);
> pageop->btpo_flags &= ~BTP_INCOMPLETE_SPLIT;

Interesting.  We learned elsewhere that it's better to integrate the
"!= 0" test as part of the macro definition; so a
better formulation of this patch would be to change the
P_INCOMPLETE_SPLIT macro and omit the comparison in the Assert.  (See
commit 594e61a1de03 for an example).


> -   LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
> +   LockBuffer(hbuffer, BT_READ);

I think BT_READ and BT_WRITE are useless, and I'd rather get rid of
them ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers