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

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
Date: 2013-07-22 19:05:14
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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>
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://

Remember to vote!
Consider donating to Postgres:

Attachment Content-Type Size
ordinality_12.diff text/plain 78.9 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
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