Thread: [PATCH] Combine same ternary types in GIN and TSearch
Hi, hackers!
--
For historical reasons, now we have two differently named but similar ternary data types in TSearch and Gin text-related types. Before v13 there was also Gin's private TS_execute() version, from which we eventually shifted to Tsearch's TS_execute().
To make things more even and beautiful I've made a minor refactor to combine two left ternary types into one.
<gin.h>
typedef char GinTernaryValue
#define GIN_FALSE 0
#define GIN_TRUE 1
#define GIN_MAYBE 2
<ts_utils.h>
typedef enum { TS_NO, TS_YES, TS_MAYBE } TSTernaryValue;
The change is simple and most of it is just the text replacement. The only thing worth noting is that some code does pointer cast between *bool and *TernaryValue so the size of them should coincide. (Declaration done in char type because simple enum on most architectures will be of int size). There is no actual change in the code despite the order of header files inclusion in some modules.
What do you think about this?
Attachment
On 13/11/2020 11:04, Pavel Borisov wrote:
> Hi, hackers!
>
> For historical reasons, now we have two differently named but similar
> ternary data types in TSearch and Gin text-related types. Before v13
> there was also Gin's private TS_execute() version, from which we
> eventually shifted to Tsearch's TS_execute().
>
> To make things more even and beautiful I've made a minor refactor to
> combine two left ternary types into one.
>
> <gin.h>
> typedef char GinTernaryValue
> #define GIN_FALSE 0
> #define GIN_TRUE 1
> #define GIN_MAYBE 2
>
> <ts_utils.h>
> typedef enum { TS_NO, TS_YES, TS_MAYBE } TSTernaryValue;
>
> The change is simple and most of it is just the text replacement. The
> only thing worth noting is that some code does pointer cast between
> *bool and *TernaryValue so the size of them should coincide.
> (Declaration done in /char/ type because simple enum on most
> architectures will be of /int/ size). There is no actual change in the
> code despite the order of header files inclusion in some modules.
>
> What do you think about this?
GIN is not just for full-text search, so using TSTernaryValue in
GinScanKeyData is wrong. And it would break existing extensions.
I didn't look much further than that, but I've got a feeling that
combining those is a bad idea. TSTernaryValue is used in text-search
code, even when there is no GIN involved. It's a separate concept, even
though it happens to have the same values.
- Heikki
GIN is not just for full-text search, so using TSTernaryValue in
GinScanKeyData is wrong. And it would break existing extensions.
I didn't look much further than that, but I've got a feeling that
combining those is a bad idea. TSTernaryValue is used in text-search
code, even when there is no GIN involved. It's a separate concept, even
though it happens to have the same values.
Probably you are right. But now the code already rely on equivalent value assignments for GinTernaryValue and TSTernaryValue (in checkcondition_gin()). So my idea was to combine them and use them like we use other global data types. We may declare it somewhere outside both gin and search. Or just leave as it is.
Thank you, Heikki for your feedback!
--
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 13/11/2020 11:04, Pavel Borisov wrote:
>> For historical reasons, now we have two differently named but similar
>> ternary data types in TSearch and Gin text-related types. Before v13
>> there was also Gin's private TS_execute() version, from which we
>> eventually shifted to Tsearch's TS_execute().
>> To make things more even and beautiful I've made a minor refactor to
>> combine two left ternary types into one.
> GIN is not just for full-text search, so using TSTernaryValue in
> GinScanKeyData is wrong. And it would break existing extensions.
> I didn't look much further than that, but I've got a feeling that
> combining those is a bad idea. TSTernaryValue is used in text-search
> code, even when there is no GIN involved. It's a separate concept, even
> though it happens to have the same values.
I'm definitely not on board with importing a TS-specific type into GIN,
and even less with requiring major GIN headers to import random
TS-related headers.
There might be a case for having just one neutrally-named "ternary" enum
type, declared in a neutral (probably new) header, that both areas of
the code could use. But it's not clear that it'd be worth the code
thrashing to do that. As Heikki says, this will surely break some
extensions; and I'd prefer that there be some non-cosmetic benefit
if we ask extension authors to cope with that.
regards, tom lane