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

Reply via email to