Small code improvement for btree

Started by Masahiko Sawadaover 8 years ago9 messages

sawada.mshk@gmail.com

over 8 years ago

1 attachment(s)

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

Attachments:

code_improvement_for_btree.patchapplication/octet-stream; name=code_improvement_for_btree.patchDownload

diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index ac60db0..b894f8a 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/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);
@@ -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);
 
 		/*

Alvaro Herrera

alvherre@2ndquadrant.com

over 8 years ago

In reply to: Masahiko Sawada (#1)

Re: Small code improvement for btree

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

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

Peter Geoghegan

pg@bowt.ie

over 8 years ago

In reply to: Alvaro Herrera (#2)

Re: Small code improvement for btree

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

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Peter Geoghegan (#3)

Re: Small code improvement for btree

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

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#4)

1 attachment(s)

Re: Small code improvement for btree

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

Attachments:

code_improvement_for_btree_v2.patchapplication/octet-stream; name=code_improvement_for_btree_v2.patchDownload

diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c
index ac60db0..4e3a825 100644
--- a/src/backend/access/nbtree/nbtxlog.c
+++ b/src/backend/access/nbtree/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));
 		pageop->btpo_flags &= ~BTP_INCOMPLETE_SPLIT;
 
 		PageSetLSN(page, lsn);
@@ -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);
 
 		/*
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index e6abbec..2d4c36d 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -173,14 +173,14 @@ typedef struct BTMetaPageData
  */
 #define P_LEFTMOST(opaque)		((opaque)->btpo_prev == P_NONE)
 #define P_RIGHTMOST(opaque)		((opaque)->btpo_next == P_NONE)
-#define P_ISLEAF(opaque)		((opaque)->btpo_flags & BTP_LEAF)
-#define P_ISROOT(opaque)		((opaque)->btpo_flags & BTP_ROOT)
-#define P_ISDELETED(opaque)		((opaque)->btpo_flags & BTP_DELETED)
-#define P_ISMETA(opaque)		((opaque)->btpo_flags & BTP_META)
-#define P_ISHALFDEAD(opaque)	((opaque)->btpo_flags & BTP_HALF_DEAD)
-#define P_IGNORE(opaque)		((opaque)->btpo_flags & (BTP_DELETED|BTP_HALF_DEAD))
-#define P_HAS_GARBAGE(opaque)	((opaque)->btpo_flags & BTP_HAS_GARBAGE)
-#define P_INCOMPLETE_SPLIT(opaque)	((opaque)->btpo_flags & BTP_INCOMPLETE_SPLIT)
+#define P_ISLEAF(opaque)		(((opaque)->btpo_flags & BTP_LEAF) != 0)
+#define P_ISROOT(opaque)		(((opaque)->btpo_flags & BTP_ROOT) != 0)
+#define P_ISDELETED(opaque)		(((opaque)->btpo_flags & BTP_DELETED) != 0)
+#define P_ISMETA(opaque)		(((opaque)->btpo_flags & BTP_META) != 0)
+#define P_ISHALFDEAD(opaque)	(((opaque)->btpo_flags & BTP_HALF_DEAD) != 0)
+#define P_IGNORE(opaque)		(((opaque)->btpo_flags & (BTP_DELETED|BTP_HALF_DEAD)) != 0)
+#define P_HAS_GARBAGE(opaque)	(((opaque)->btpo_flags & BTP_HAS_GARBAGE) != 0)
+#define P_INCOMPLETE_SPLIT(opaque)	(((opaque)->btpo_flags & BTP_INCOMPLETE_SPLIT) != 0)
 
 /*
  *	Lehman and Yao's algorithm requires a ``high key'' on every non-rightmost

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#5)

Re: Small code improvement for btree

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

--
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

over 8 years ago

In reply to: Masahiko Sawada (#6)

Re: Small code improvement for btree

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

Tom Lane

tgl@sss.pgh.pa.us

over 8 years ago

In reply to: Doug Doole (#7)

Re: Small code improvement for btree

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Tom Lane (#8)

Re: Small code improvement for btree

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