Re: [HACKERS] GSoC 2017: Foreign Key Arrays

Hi Mark,

On Mon, Feb 8, 2021, at 09:40, Mark Rofail wrote:
> anyarray_anyelement_operators-v1.patch

Here comes a first review of the anyarray_anyelement_operators-v1.patch.

doc/src/sgml/func.sgml
+ Does the array contain specified element ?

* Maybe remove the extra blank space before question mark?

doc/src/sgml/indices.sgml
-<@   @>   =   &&
+<@ @>   <<@ @>>   =   &&

* To me it looks like the pattern is to insert   between each operator, in which case this should be written:

<@   @>   <<@ @>>   =   &&

I.e.,   is missing between <@ and @>.

src/backend/access/gin/ginarrayproc.c
/* Make copy of array input to ensure it doesn't disappear while in use */
- ArrayType *array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+ ArrayType *array;

I think the comment above should be changed/moved since the copy has been moved and isn't always performed due to the if. Since array variable is only used in the else block, couldn't both the comment and the declaration of array be moved to the else block as well?

src/backend/utils/adt/arrayfuncs.c
+ /*
+ * We assume that the comparison operator is strict, so a NULL can't
+ * match anything. XXX this diverges from the "NULL=NULL" behavior of
+ * array_eq, should we act like that?
+ */

The comment above is copy/pasted from array_contain_compare(). It seems unnecessary to have this open question, word-by-word, on two different places. I think a reference from here to the existing similar code would be better. And also to add a comment in array_contain_compare() about the existence of this new code where the same question is discussed.

If this would be new code, then this question should probably be answered before committing, but since this is old code, maybe the behaviour now can't be changed anyway, since the old code in array_contain_compare() has been around since 2006, when it was introduced in commit f5b4d9a9e08199e6bcdb050ef42ea7ec0f7525ca.

/Joel