Horton

Thread

Commits

  1. Minor code-cleanliness improvements for btree.

  2. Change some test macros to return true booleans

  1. Small code improvement for btree

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-08-04T08:27:35Z

    Hi,

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)
                Page            page = (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;

                PageSetLSN(page, lsn);

We can use P_INCOMPLETE_SPLIT() instead here.

---
@@ -598,7 +598,7 @@
btree_xlog_delete_get_latestRemovedXid(XLogReaderState *record)
                        UnlockReleaseBuffer(ibuffer);
                        return InvalidTransactionId;
                }
-               LockBuffer(hbuffer, BUFFER_LOCK_SHARE);
+               LockBuffer(hbuffer, BT_READ);
                hpage = (Page) BufferGetPage(hbuffer);

                /*

We should use BT_READ here rather than BUFFER_LOCK_SHARE.

I think that since such codes are sometimes hard to be found easily by
grep, so could be a cause of bugs when changing the code.
Attached small patch fixes these issues.

Regards,

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

  2. Re: Small code improvement for btree

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-08-04T18:12:33Z

    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)
>                 Page            page = (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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: Small code improvement for btree

    Peter Geoghegan <pg@bowt.ie> — 2017-08-04T18:29:22Z

    On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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

  4. Re: Small code improvement for btree

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-08-07T04:44:27Z

    On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan <pg@bowt.ie> wrote:
> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> 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

  5. Re: Small code improvement for btree

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-08-09T02:23:56Z

    On Mon, Aug 7, 2017 at 1:44 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan <pg@bowt.ie> wrote:
>> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> 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

  6. Re: Small code improvement for btree

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-08-10T03:43:07Z

    On Wed, Aug 9, 2017 at 11:23 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Mon, Aug 7, 2017 at 1:44 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> On Sat, Aug 5, 2017 at 3:29 AM, Peter Geoghegan <pg@bowt.ie> wrote:
>>> On Fri, Aug 4, 2017 at 11:12 AM, Alvaro Herrera
>>> <alvherre@2ndquadrant.com> 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

  7. Re: Small code improvement for btree

    Douglas Doole <dougdoole@gmail.com> — 2017-09-15T00:37:00Z

    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

  8. Re: Small code improvement for btree

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-18T20:39:59Z

    Doug Doole <DougDoole@gmail.com> 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

  9. Re: Small code improvement for btree

    Masahiko Sawada <sawada.mshk@gmail.com> — 2017-09-19T01:01:25Z

    On Tue, Sep 19, 2017 at 5:39 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Doug Doole <DougDoole@gmail.com> 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