Re: [PATCH] Combine same ternary types in GIN and TSearch

[Tom Lane]

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

Re: [PATCH] Combine same ternary types in GIN and TSearch

[Pavel Borisov]

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

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: [PATCH] Combine same ternary types in GIN and TSearch

[Heikki Linnakangas]

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.



typedef char GinTernaryValue
#define GIN_FALSE 0
#define GIN_TRUE 1
#define GIN_MAYBE 2


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