| From: | James Coleman <jtc331(at)gmail(dot)com> |
|---|---|
| To: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Misleading comment in tuplesort_set_bound |
| Date: | 2019-06-23 19:22:04 |
| Message-ID: | CAAaqYe9GD__4Crm=ddz+-XXcNhfY_V5gFYdLdmkFNq=2VHO56Q@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
While digging into the incremental sort patch, I noticed in
tuplesort.c at the beginning of the function in $SUBJECT we have this
comment and assertion:
tuplesort_set_bound(Tuplesortstate *state, int64 bound)
{
/* Assert we're called before loading any tuples */
Assert(state->status == TSS_INITIAL);
But AFAICT from reading the code in puttuple_common the state remains
TSS_INITIAL while tuples are inserted (unless we reach a point where
we decide to transition it to TSS_BOUNDED or TSS_BUILDRUNS).
Therefore it's not true that the assertion guards against having
loaded any tuples; rather it guarantees that we remain in standard
memory quicksort mode.
Assuming my understanding is correct, I've attached a small patch to
update the comment to "Assert we're still in memory quicksort mode and
haven't transitioned to heap or tape mode".
Note: this also means the function header comment "Must be called
before inserting any tuples" is a condition that isn't actually
validated, but I think that's fine given it's not a new problem and
even more so since the same comment goes on to say that that's
probably not a strict requirement.
James Coleman
| Attachment | Content-Type | Size |
|---|---|---|
| tuplesort_set_bound_comment-v1.patch | text/x-patch | 1.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Eisentraut | 2019-06-23 19:44:15 | Re: how to run encoding-dependent tests by default |
| Previous Message | Peter Eisentraut | 2019-06-23 18:13:28 | Re: Add CREATE DATABASE LOCALE option |