On Tue, Sep 6, 2016 at 8:28 PM, Peter Geoghegan <p...@heroku.com> wrote: > On Tue, Sep 6, 2016 at 12:57 PM, Claudio Freire <klaussfre...@gmail.com> > wrote: >> Patch lacks any new tests, but the changed code paths seem covered >> sufficiently by existing tests. A little bit of fuzzing on the patch >> itself, like reverting some key changes, or flipping some key >> comparisons, induces test failures as it should, mostly in cluster. >> >> The logic in tuplesort_heap_root_displace seems sound, except: >> >> + */ >> + memtuples[i] = memtuples[imin]; >> + i = imin; >> + } >> + >> + Assert(state->memtupcount > 1 || imin == 0); >> + memtuples[imin] = *newtup; >> +} >> >> Why that assert? Wouldn't it make more sense to Assert(imin < n) ? > > There might only be one or two elements in the heap. Note that the > heap size is indicated by state->memtupcount at this point in the > sort, which is a little confusing (that differs from how memtupcount > is used elsewhere, where we don't partition memtuples into a heap > portion and a preread tuples portion, as we do here).
I noticed, but here n = state->memtupcount + Assert(memtuples[0].tupindex == newtup->tupindex); + + CHECK_FOR_INTERRUPTS(); + + n = state->memtupcount; /* n is heap's size, including old root */ + imin = 0; /* start with caller's "hole" in root */ + i = imin; In fact, the assert on the patch would allow writing memtuples outside the heap, as in calling tuplesort_heap_root_displace if memtupcount==0, but I don't think that should be legal (memtuples[0] == memtuples[imin] would be outside the heap). Sure, that's a weird enough case (that assert up there already reads memtuples[0] which would be equally illegal if memtupcount==0), but it goes on to show that the assert expression just seems odd for its intent. BTW, I know it's not the scope of the patch, but shouldn't root_displace be usable on the TSS_BOUNDED phase? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers