pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Arthur Zakirov <a.zakirov@postgrespro.ru>
Cc: Oleg Bartunov <obartunov@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: David Steele <david@pgmasters.net>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Date: Wed, 03 Jan 2018 21:05:26 -0500
Message-ID: <8809.1515031526@sss.pgh.pa.us> (raw)
In-Reply-To: <CA+q6zcX7W56i-k-_KK51ZQA3GZZKsFx0KzGAGm2rtC8M7948kQ@mail.gmail.com>
References: <CA+q6zcVJdNNGG63aFR6Kf6rt71VsPAsO4ugqjq856zkhdA74Xg@mail.gmail.com>
	<20171031150515.GA20467@zakirov.localdomain>
	<CA+q6zcX3nL_u3hf7Ofwb=ERANgZMX+PBJf0u96X3+bAR9Db=kQ@mail.gmail.com>
	<20171108162529.GA7953@zakirov.localdomain>
	<CA+q6zcX5_meFnpnQj=wY5iyNXs-i_x+KpJ8eyZGnvn3CaUS2YQ@mail.gmail.com>
	<20171113131105.GA23603@zakirov.localdomain>
	<CA+q6zcUaOE=fVTEZrZ_sB2bC+fJEnAe5MkeZpv8VFJJWmiHXaw@mail.gmail.com>
	<CA+q6zcUws7270pFKgT7G0jFuLxmCaBLqPL0o4sfgfLnRBGg9gg@mail.gmail.com>
	<20171116114016.GA21162@zakirov.localdomain>
	<CA+q6zcUn+9OnuMh3Mvu=XaNpSmLGo2HrYcowgJqCDrhKifaz+A@mail.gmail.com>
	<20171119151350.GA23853@arthur.localdomain>
	<CA+q6zcXff34m4EskCiWdvc2BTx2J++w5gwSb43X0ejgg+sj9Wg@mail.gmail.com>
	<CAB7nPqSQFonLWsVr1OuDDd7NBHOBMNiHdvRwhxpkNV2W9-QUgg@mail.gmail.com>
	<CA+q6zcW52cJ-T3xrGm5BJ8ET6bcc0n3OYhM_oA9AQxAMZR2Otg@mail.gmail.com>
	<CA+q6zcX7W56i-k-_KK51ZQA3GZZKsFx0KzGAGm2rtC8M7948kQ@mail.gmail.com>

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




view thread (276+ messages)  latest in thread

Message-ID: <8809.1515031526@sss.pgh.pa.us>
Permalink:  ../8809.1515031526@sss.pgh.pa.us/
Also on:    postgresql.org/message-id/8809.1515031526@sss.pgh.pa.us

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: tgl@sss.pgh.pa.us, 9erthalion6@gmail.com, michael.paquier@gmail.com, a.zakirov@postgrespro.ru, obartunov@gmail.com, peter.eisentraut@2ndquadrant.com, david@pgmasters.net
  Subject: Re: [HACKERS] [PATCH] Generic type subscripting
  In-Reply-To: <8809.1515031526@sss.pgh.pa.us>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox