Re: [HACKERS] [PATCH] Generic type subscripting - Mailing list pgsql-hackers

From Tom Lane
Subject Re: [HACKERS] [PATCH] Generic type subscripting
Date
Msg-id 8809.1515031526@sss.pgh.pa.us
Whole thread Raw
In response to Re: [HACKERS] [PATCH] Generic type subscripting  (Dmitry Dolgov <9erthalion6@gmail.com>)
Responses Re: [HACKERS] [PATCH] Generic type subscripting
List pgsql-hackers
Dmitry Dolgov <9erthalion6@gmail.com> writes:
> Another rebased version of this patch.

Apologies for not having paid attention to this patch for so long.
Coming back to it now, I wonder what happened to the plan to separate
assignment and fetch into two different node types.  I can see that
that didn't happen so far as primnodes.h is concerned, but you've
made some changes that seem to assume it did happen, eg this bit
in clauses.c:

@@ -1345,12 +1345,10 @@ contain_nonstrict_functions_walker(Node *node, void *context)
         /* a window function could return non-null with null input */
         return true;
     }
-    if (IsA(node, ArrayRef))
+    if (IsA(node, SubscriptingRef))
     {
         /* array assignment is nonstrict, but subscripting is strict */
-        if (((ArrayRef *) node)->refassgnexpr != NULL)
-            return true;
-        /* else fall through to check args */
+        return true;
     }
     if (IsA(node, DistinctExpr))
     {

Treating the two cases alike here is just wrong.

Also, the reason I was looking at clauses.c was I realized that
my recent commit 3decd150a broke this patch, because it introduced
understanding of ArrayRef into eval_const_expressions().  I think
that you can probably just do s/ArrayRef/SubscriptingRef/ there,
but it might deserve a closer look than I've given it.

I'm not terribly happy with the cosmetics of this patch at the moment.
There are too many places where it's achingly obvious that you did
s/ArrayRef/SubscriptingRef/g and nothing else, leaving code that does not
pass the test of "does it look like it was written like that to begin
with".  There are a lot of variables still named "aref" or "arefstate"
or similar when that's no longer an apropos name; there are a lot of
sentences reading "an SubscriptingRef" which is bad English; there are a
lot of comments whose layout is not going to be too hot after pgindent
because "SubscriptingRef" is so much longer than "ArrayRef".  (I'm tempted
to suggest that we call the node type just "Subscript", to buy back some
of that.)  I'm not necessarily putting it on you to fix that stuff --- it
might be easier for a native English speaker --- but I am saying that if
I commit this, there are going to be a lot of differences in detail from
what's here now.

            regards, tom lane


pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: January CommitFest is underway!
Next
From: Robert Haas
Date:
Subject: Re: Observations in Parallel Append