|From:||David Fetter <david(at)fetter(dot)org>|
|To:||Greg Stark <stark(at)mit(dot)edu>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Josh Berkus <josh(at)agliodbs(dot)com>, pgsql-hackers(at)postgresql(dot)org|
|Subject:||Re: Review: UNNEST (and other functions) WITH ORDINALITY|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Mon, Jul 22, 2013 at 05:19:35PM +0100, Greg Stark wrote:
> On Mon, Jul 22, 2013 at 4:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I haven't read this patch, but that comment scares the heck out of me.
> > Even if the patch isn't broken today, it will be tomorrow, if it's
> > making random changes like that in data structure semantics.
> It's not making random changes. It's just that it has two code paths,
> in one it has the function scan write directly to the scan slot and in
> the other it has the function write to a different slot and copies the
> fields over to the scan slot. It's actually doing the right thing it's
> just hard to tell that at first (imho) because it's using the scan
> slots to determine which case applies instead of having a flag
> something to drive the decision.
> > Also, if you're confused, so will be everybody else who has to deal with
> > the code later --- so I don't think fixing the comments and variable
> > names is optional.
> Well that's the main benefit of having someone review the code in my
> opinion. I'm no smarter than David or Andrew who wrote the code and
> there's no guarantee I'll spot any bugs. But at least I can observe
> myself and see where I have difficulty following the logic which the
> original author is inherently not in a position to do.
> Honestly this is pretty straightforward and well written so I'm just
> being especially careful not having committed anything recently.
It turns out Andrew Gierth found two issues: backward scanning and
docs mentioning incorrect data type for the ordinality column. The
attached patch fixes both. His chunk is the backward scans; mine the
one-word change :)
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
|Next Message||Greg Smith||2013-07-22 19:06:26||Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])|
|Previous Message||Robert Haas||2013-07-22 19:03:00||Re: proposal - psql - show longest tables|