[PATCH] Combine same ternary types in GIN and TSearch

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!

--
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com <http://www.postgrespro.com&gt;

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