Re: [HACKERS] [PATCH] Generic type subscripting

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, Oleg Bartunov <obartunov(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Date: 2018-01-04 02:05:26
Message-ID: 8809.1515031526@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dmitry Dolgov <9erthalion6(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2018-01-04 02:09:24 Re: pgsql: Add parallel-aware hash joins.
Previous Message Thomas Munro 2018-01-04 01:26:28 Re: pgsql: Add parallel-aware hash joins.