Re: Review: UNNEST (and other functions) WITH ORDINALITY

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 :)

Cheers,
David.
--
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
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate