indentation in _hash_pgaddtup()
Hi,
I was looking at :
commit d09dbeb9bde6b9faabd30e887eff4493331d6424
Author: David Rowley <drowley@postgresql.org>
Date: Thu Nov 24 17:21:44 2022 +1300
Speedup hash index builds by skipping needless binary searches
In _hash_pgaddtup(), it seems the indentation is off for the assertion.
Please take a look at the patch.
Thanks
Attachments:
hash-pgaddtup-indent.patchapplication/octet-stream; name=hash-pgaddtup-indent.patchDownload
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 9db522051e..4e99e599e8 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -292,10 +292,10 @@ _hash_pgaddtup(Relation rel, Buffer buf, Size itemsize, IndexTuple itup,
/* ensure this tuple's hashkey is >= the final existing tuple */
Assert(PageGetMaxOffsetNumber(page) == 0 ||
- _hash_get_indextuple_hashkey((IndexTuple)
- PageGetItem(page, PageGetItemId(page,
- PageGetMaxOffsetNumber(page)))) <=
- _hash_get_indextuple_hashkey(itup));
+ _hash_get_indextuple_hashkey((IndexTuple)
+ PageGetItem(page, PageGetItemId(page,
+ PageGetMaxOffsetNumber(page)))) <=
+ _hash_get_indextuple_hashkey(itup));
}
else
{
On 24 Nov 2022, at 13:42, Ted Yu <yuzhihong@gmail.com> wrote:
In _hash_pgaddtup(), it seems the indentation is off for the assertion.
Please take a look at the patch.
Indentation is handled by applying src/tools/pgindent to the code, and
re-running it on this file yields no re-indentation so this is in fact correct
according to the pgindent rules.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
On 24 Nov 2022, at 13:42, Ted Yu <yuzhihong@gmail.com> wrote:
In _hash_pgaddtup(), it seems the indentation is off for the assertion.
Indentation is handled by applying src/tools/pgindent to the code, and
re-running it on this file yields no re-indentation so this is in fact correct
according to the pgindent rules.
It is one messy bit of code though --- perhaps a little more thought
about where to put line breaks would help? Alternatively, it could
be split into multiple statements, along the lines of
#ifdef USE_ASSERT_CHECKING
if (PageGetMaxOffsetNumber(page) > 0)
{
IndexTuple lasttup = PageGetItem(page,
PageGetItemId(page,
PageGetMaxOffsetNumber(page)));
Assert(_hash_get_indextuple_hashkey(lasttup) <=
_hash_get_indextuple_hashkey(itup));
}
#endif
(details obviously tweakable)
regards, tom lane
On Thu, Nov 24, 2022 at 7:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Daniel Gustafsson <daniel@yesql.se> writes:
On 24 Nov 2022, at 13:42, Ted Yu <yuzhihong@gmail.com> wrote:
In _hash_pgaddtup(), it seems the indentation is off for the assertion.Indentation is handled by applying src/tools/pgindent to the code, and
re-running it on this file yields no re-indentation so this is in factcorrect
according to the pgindent rules.
It is one messy bit of code though --- perhaps a little more thought
about where to put line breaks would help? Alternatively, it could
be split into multiple statements, along the lines of#ifdef USE_ASSERT_CHECKING
if (PageGetMaxOffsetNumber(page) > 0)
{
IndexTuple lasttup = PageGetItem(page,
PageGetItemId(page,PageGetMaxOffsetNumber(page)));
Assert(_hash_get_indextuple_hashkey(lasttup) <=
_hash_get_indextuple_hashkey(itup));
}
#endif(details obviously tweakable)
regards, tom lane
Thanks Tom for the suggestion.
Here is patch v2.
Attachments:
hash-pgaddtup-indent-v2.patchapplication/octet-stream; name=hash-pgaddtup-indent-v2.patchDownload
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 9db522051e..7c7ab57452 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -290,12 +290,16 @@ _hash_pgaddtup(Relation rel, Buffer buf, Size itemsize, IndexTuple itup,
{
itup_off = PageGetMaxOffsetNumber(page) + 1;
+#ifdef USE_ASSERT_CHECKING
/* ensure this tuple's hashkey is >= the final existing tuple */
- Assert(PageGetMaxOffsetNumber(page) == 0 ||
- _hash_get_indextuple_hashkey((IndexTuple)
- PageGetItem(page, PageGetItemId(page,
- PageGetMaxOffsetNumber(page)))) <=
- _hash_get_indextuple_hashkey(itup));
+ if (PageGetMaxOffsetNumber(page) > 0)
+ {
+ IndexTuple lasttup = PageGetItem(page,
+ PageGetItemId(page, PageGetMaxOffsetNumber(page)));
+ Assert(_hash_get_indextuple_hashkey(lasttup) <=
+ _hash_get_indextuple_hashkey(itup));
+ }
+#endif
}
else
{
On Fri, 25 Nov 2022 at 04:29, Ted Yu <yuzhihong@gmail.com> wrote:
Here is patch v2.
After running pgindent on v2, I see it still pushes the lines out
quite far. If I add a new line after PageGetItemId(page, and put the
variable assignment away from the variable declaration then it looks a
bit better. It's still 1 char over the limit.
David
Attachments:
hash-pgaddtup-indent-v3.patchtext/plain; charset=US-ASCII; name=hash-pgaddtup-indent-v3.patchDownload
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 9db522051e..dfc7a90d68 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -290,12 +290,20 @@ _hash_pgaddtup(Relation rel, Buffer buf, Size itemsize, IndexTuple itup,
{
itup_off = PageGetMaxOffsetNumber(page) + 1;
+#ifdef USE_ASSERT_CHECKING
/* ensure this tuple's hashkey is >= the final existing tuple */
- Assert(PageGetMaxOffsetNumber(page) == 0 ||
- _hash_get_indextuple_hashkey((IndexTuple)
- PageGetItem(page, PageGetItemId(page,
- PageGetMaxOffsetNumber(page)))) <=
- _hash_get_indextuple_hashkey(itup));
+ if (PageGetMaxOffsetNumber(page) > 0)
+ {
+ IndexTuple lasttup;
+
+ lasttup = PageGetItem(page,
+ PageGetItemId(page,
+ PageGetMaxOffsetNumber(page)));
+
+ Assert(_hash_get_indextuple_hashkey(lasttup) <=
+ _hash_get_indextuple_hashkey(itup));
+ }
+#endif
}
else
{
David Rowley <dgrowleyml@gmail.com> writes:
After running pgindent on v2, I see it still pushes the lines out
quite far. If I add a new line after PageGetItemId(page, and put the
variable assignment away from the variable declaration then it looks a
bit better. It's still 1 char over the limit.
If you wanted to be hard-nosed about 80 character width, you could
pull out the PageGetItemId call into a separate local variable.
I wasn't going to be quite that picky, but I won't object if that
seems better to you.
regards, tom lane
On Thu, Nov 24, 2022 at 12:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
After running pgindent on v2, I see it still pushes the lines out
quite far. If I add a new line after PageGetItemId(page, and put the
variable assignment away from the variable declaration then it looks a
bit better. It's still 1 char over the limit.If you wanted to be hard-nosed about 80 character width, you could
pull out the PageGetItemId call into a separate local variable.
I wasn't going to be quite that picky, but I won't object if that
seems better to you.regards, tom lane
Patch v4 stores ItemId in a local variable.
Attachments:
hash-pgaddtup-indent-v4.patchapplication/octet-stream; name=hash-pgaddtup-indent-v4.patchDownload
diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash/hashinsert.c
index 9db522051e..0656c9ca92 100644
--- a/src/backend/access/hash/hashinsert.c
+++ b/src/backend/access/hash/hashinsert.c
@@ -290,12 +290,20 @@ _hash_pgaddtup(Relation rel, Buffer buf, Size itemsize, IndexTuple itup,
{
itup_off = PageGetMaxOffsetNumber(page) + 1;
+#ifdef USE_ASSERT_CHECKING
/* ensure this tuple's hashkey is >= the final existing tuple */
- Assert(PageGetMaxOffsetNumber(page) == 0 ||
- _hash_get_indextuple_hashkey((IndexTuple)
- PageGetItem(page, PageGetItemId(page,
- PageGetMaxOffsetNumber(page)))) <=
- _hash_get_indextuple_hashkey(itup));
+ if (PageGetMaxOffsetNumber(page) > 0)
+ {
+ IndexTuple lasttup;
+ ItemId itemid;
+
+ itemid = PageGetItemId(page, PageGetMaxOffsetNumber(page));
+ lasttup = PageGetItem(page, itemid);
+
+ Assert(_hash_get_indextuple_hashkey(lasttup) <=
+ _hash_get_indextuple_hashkey(itup));
+ }
+#endif
}
else
{
On Fri, 25 Nov 2022 at 09:40, Ted Yu <yuzhihong@gmail.com> wrote:
Patch v4 stores ItemId in a local variable.
ok, I pushed that one. Thank you for working on this.
David
On Fri, 25 Nov 2022 at 09:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
If you wanted to be hard-nosed about 80 character width, you could
pull out the PageGetItemId call into a separate local variable.
I wasn't going to be quite that picky, but I won't object if that
seems better to you.
I wasn't too worried about the 1 char, but Ted wrote a patch and it
looked a little nicer.
David