Thread: [HACKERS] Small code improvement for btree
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers