public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bernd Helmle <[email protected]>
To: Andrey M. Borodin <[email protected]>
Cc: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
Date: Tue, 12 Nov 2024 16:43:50 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<CACJufxHvXHNi_9roSo2gLJVCSep1EgD-z=-ey6K_Xg96PHs9+g@mail.gmail.com>
	<CACJufxEVYNCQJEnRr6jpdrNSb9hH6YkavxrLabrRuCWoeTDHjQ@mail.gmail.com>
	<[email protected]>
	<CACJufxHSsRk9egTjZWYXK4mJF8rsoV0XuRuw4uCxymoeeYBAJw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Am Montag, dem 11.11.2024 um 23:03 +0500 schrieb Andrey M. Borodin:
> Some nitpicking:
> 0. postgres % git apply ~/Downloads/v7.3-Add-GIST-sortsupport-*
> /Users/x4mmm/Downloads/v7.3-Add-GIST-sortsupport-btree-gist.patch:19:
> space before tab in indent.

I obviously shouldn't do patches after a long work day....


[...]

> 
> 1. I'm mostly seening patches formatted with `git patch-format`
> rather than diffs as patches...
> 

Will do.

> 2. Changes in contrib/btree_gist/btree_gist.c seem unnecessary.
> 

Agreed. Will fix

> 3. Why do you move "typedef struct int32key" to btree_gist.h, but do
> not need to move all other keys?
> 

Hmm, afair i did this back in an earlier stage of the patch and forgot
about it. It needs to be moved back to btree_int4.c, will fix.

> 4. These ifdedfs are not needed, just do INJECTION_POINT()
> #ifdef USE_INJECTION_POINTS
> INJECTION_POINT("btree-gist-sorted-build");
> #endif
> 

I never worked with injection points before, i copied that pattern from
the docs somewhere, where those #ifdef's are used. Will fix.

> Also, there are 61 occurance of this code. Why not just 1 in
> gist_indexsortbuild() ?
> 
> 

Right, but after thinking about this i'd even go further: naming the
injection points for the sortsupport according to their datatype
they're called on. That would connect the regression test with the
specific datatype/sortsupport function and this would make sure the
correct sortsupport function for the specific test case is called.
Though that means that each test needs to attach/detach the injection
point itself...

What do you think?

Thanks,
	Bernd

view thread (23+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add sortsupport for range types and btree_gist
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox