From: | Claudio Freire <klaussfreire(at)gmail(dot)com> |
---|---|
To: | Peter Geoghegan <pg(at)heroku(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
Subject: | Re: Parallel tuplesort (for parallel B-Tree index creation) |
Date: | 2016-09-06 23:55:09 |
Message-ID: | CAGTBQpYRS4uRHQbJc1yeYC9TQGOhecHPi5w8eH5o3McRCihNfA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Sep 6, 2016 at 8:28 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Tue, Sep 6, 2016 at 12:57 PM, Claudio Freire <klaussfreire(at)gmail(dot)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?
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2016-09-07 00:09:26 | Re: Speedup twophase transactions |
Previous Message | Michael Paquier | 2016-09-06 23:29:57 | Re: Let file_fdw access COPY FROM PROGRAM |