On Mon, Jul 22, 2013 at 4:42 PM, Tom Lane <tgl@sss.pgh.pa.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.
--
greg
