Re: [HACKERS] GSoC 2017: Foreign Key Arrays

>On Thu, Feb 11, 2021, at 16:50, Mark Rofail wrote:
>> Here comes a first review of the anyarray_anyelement_operators-v1.patch.
>Great, thanks! I’ll start applying your comments today and release a new patch.

Here comes some more feedback:

I was surprised to see <<@ and @>> don't complain when trying to compare incompatible types:

regression=# select '1'::text <<@ ARRAY[1::integer,2::integer];
?column?
----------
f
(1 row)

I would expect the same result as if using the <@ and @> operators,
and wrapping the value in an array:

regression=# select ARRAY['1'::text] <@ ARRAY[1::integer,2::integer];
ERROR: operator does not exist: text[] <@ integer[]
LINE 1: select ARRAY['1'::text] <@ ARRAY[1::integer,2::integer];
^
HINT: No operator matches the given name and argument types. You might need to add explicit type casts.

The error above for the existing <@ operator is expected,
and I think the <<@ should give a similar error.

Even worse, when using integer on the left side and text in the array,
the <<@ operator causes a seg fault:

regression=# select 1::integer <<@ ARRAY['1'::text,'2'::text];
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

2021-02-11 22:18:53.083 CET [91680] LOG: server process (PID 1666) was terminated by signal 11: Segmentation fault: 11
2021-02-11 22:18:53.083 CET [91680] DETAIL: Failed process was running: select 1::integer <<@ ARRAY['1'::text,'2'::text];
2021-02-11 22:18:53.083 CET [91680] LOG: terminating any other active server processes
2021-02-11 22:18:53.084 CET [1735] FATAL: the database system is in recovery mode
2021-02-11 22:18:53.084 CET [91680] LOG: all server processes terminated; reinitializing
2021-02-11 22:18:53.092 CET [1736] LOG: database system was interrupted; last known up at 2021-02-11 22:14:41 CET
2021-02-11 22:18:53.194 CET [1736] LOG: database system was not properly shut down; automatic recovery in progress
2021-02-11 22:18:53.197 CET [1736] LOG: redo starts at 0/2BCA5520
2021-02-11 22:18:53.197 CET [1736] LOG: invalid record length at 0/2BCA5558: wanted 24, got 0
2021-02-11 22:18:53.197 CET [1736] LOG: redo done at 0/2BCA5520 system usage: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
2021-02-11 22:18:53.207 CET [91680] LOG: database system is ready to accept connections

Maybe it's the combination of "anyarray" and "anycompatiblenonarray" that is the problem here?

Some more comments on the code:

array_contains_elem(AnyArrayType *array, Datum elem,
+ /*
+ * Apply the comparison operator to each pair of array elements.
+ */
This comment has been copy/pasted from array_contain_compare().
Maybe the wording should clarify there is only one array in this function,
the word "pair" seems to imply working with two arrays.

+ for (i = 0; i < nelems; i++)
+ {
+ Datum elt1;

The name `elt1` originates from the array_contain_compare() function.
But since this function, array_contains_elem(), doesn't have a `elt2`,
it would be better to use `elt` as a name here. The same goes for `it1`.

/Joel