Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: "SELECT ... FROM DUAL" is not quite as silly as it appears
From: Mark Dilger <hornschnorter@gmail.com>
In-Reply-To: <20496.1548464946@sss.pgh.pa.us>
Date: Sun, 27 Jan 2019 12:04:52 -0800
Cc: David Rowley <david.rowley@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: 7bit
Message-Id: <810DE61F-7332-492E-A730-E78006388BB8@gmail.com>
References: <15944.1521127664@sss.pgh.pa.us> <9565.1539573909@sss.pgh.pa.us>
 <154342395130.2093.8013327151440360209.pgcf@coridan.postgresql.org>
 <12357.1543518669@sss.pgh.pa.us>
 <CAKJS1f-ZRNnb7-NxY1odPR=ZjEiJx=_094aQZsEvJ6JEnzDjKg@mail.gmail.com>
 <20472.1546631337@sss.pgh.pa.us>
 <CAKJS1f8OKaw4LV6rTf4gedO97fzgo-V6bFOoC-r4UK6Fdaex=Q@mail.gmail.com>
 <21565.1547498912@sss.pgh.pa.us>
 <CAKJS1f9RAWOscHa0Tboo4XhgaMqq_hHhXdapsxGV-Yru4mzr-g@mail.gmail.com>
 <20496.1548464946@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Precedence: bulk


> On Jan 25, 2019, at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> As far as I can see the patch is ready to go, but I'll defer to Mark,
>> who's also listed on the reviewer list for this patch.
> 
> Mark, are you planning to do further review on this patch?
> I'd like to move it along, since (IMO anyway) we need it in
> before progress can be made on
> https://commitfest.postgresql.org/21/1664/
> 
> 			regards, tom lane


These two observations are not based on any deep understanding of
your patch, but just some cursory review:

The swtich in src/backend/parser/analyze.c circa line 2819 should
probably have an explicit case for RTE_RESULT rather than just a
comment and allowing the default to log "unrecognized RTE type",
since it's not really unrecognized, just unexpected.  But I'm not
too exercised about that, and won't argue if you want to leave it
as is.

Similarly, in src/backend/commands/explain.c, should there be a
case for T_Result in ExplainTargetRel's switch circa line 2974?
I'm not sure given your design whether this could ever be relevant,
but I noticed that T_Result / RTE_RELATION isn't handled here.


Applying your patch and running the regression tests is failing
left and right, so I'm working to pull a fresh copy from git and
build again -- probably just something wrong in my working directory.

mark