On Tue, Mar 3, 2015 at 3:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I find your statement that this is a pre-existing issue in
> tuplesort_begin_datum() to be pretty misleading, unless what you mean
> by it is "pre-existing since November, when an earlier patch by Peter
> Geoghegan changed the comments without changing the code to match".
> Commit 5ea86e6e65dd2da3e9a3464484985d48328e7fe3, changed the comments
> for the sortKey field to say that it would be set in all cases except
> for the hash index case, but it did not make that statement true.
Agreed. I believe that is in fact what I meant. I'm not trying to
deflect blame - just to explain my understanding of the problem.
> Subsequently, another of your patches, which I committed as
> 5cefbf5a6c4466ac6b1cc2a4316b4eba9108c802, added a dependency on
> state->sortKeys always being set. Now you've got this patch here,
> which you claim makes sure that sortKeys is always set, but it
> actually doesn't, which is why the above still crashes. There are not
> so many tuplesort_begin_xxx routines that you couldn't audit them all
> before submitting your patches.
Sorry. I should have caught the hash index case too.
> Anyway, I propose the attached instead, which makes both Tomas's test
> case and the one above pass.
My patch actually matches Andrew Gierth's datumsort patch, in that it
also uses this convention, as I believe it should. For that reason,
I'd prefer to make the comment added in November true, rather than
changing the comment...I feel it ought to be true anyway (which is
what I meant about a pre-existing issue). You'll still need the
defenses you've added for the the hash case, of course. You might want
to also put a comment specifying why it's necessary, since the hash
tuple case is the oddball cases that doesn't always have "sortKeys"
set.
In other words, I suggest that you commit the union of our two
patches, less your changes to the comments around "sortKeys'.
Thanks
--
Peter Geoghegan