Re: indentation in _hash_pgaddtup()
On Fri, 25 Nov 2022 at 09:31, Tom Lane 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
Re: indentation in _hash_pgaddtup()
On Fri, 25 Nov 2022 at 09:40, Ted Yu wrote: > Patch v4 stores ItemId in a local variable. ok, I pushed that one. Thank you for working on this. David
Re: indentation in _hash_pgaddtup()
On Thu, Nov 24, 2022 at 12:31 PM Tom Lane wrote: > David Rowley 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. hash-pgaddtup-indent-v4.patch Description: Binary data
Re: indentation in _hash_pgaddtup()
David Rowley 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
Re: indentation in _hash_pgaddtup()
On Fri, 25 Nov 2022 at 04:29, Ted Yu 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
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
{
Re: indentation in _hash_pgaddtup()
On Thu, Nov 24, 2022 at 7:11 AM Tom Lane wrote:
> Daniel Gustafsson writes:
> >> On 24 Nov 2022, at 13:42, Ted Yu 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
>
Thanks Tom for the suggestion.
Here is patch v2.
hash-pgaddtup-indent-v2.patch
Description: Binary data
Re: indentation in _hash_pgaddtup()
Daniel Gustafsson writes:
>> On 24 Nov 2022, at 13:42, Ted Yu 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
Re: indentation in _hash_pgaddtup()
> On 24 Nov 2022, at 13:42, Ted Yu 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/
indentation in _hash_pgaddtup()
Hi, I was looking at : commit d09dbeb9bde6b9faabd30e887eff4493331d6424 Author: David Rowley 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 hash-pgaddtup-indent.patch Description: Binary data
