Обсуждение: Misleading comment in tuplesort_set_bound

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

James Coleman <jtc331@gmail.com> writes:
> 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).

You missed the relevance of the next line:

    Assert(state->memtupcount == 0);

I think the comment is fine as-is.  Perhaps the code would be clearer
though, if we merged those two asserts into one?

    /* Assert we're called before loading any tuples */
    Assert(state->status == TSS_INITIAL &&
           state->memtupcount == 0);

I'm not totally sure about the usefulness/relevance of the two
assertions following these, but they could likely do with their
own comment(s), because this one surely isn't covering them.

            regards, tom lane

On 2019-Aug-26, Tom Lane wrote:

> James Coleman <jtc331@gmail.com> writes:

> I think the comment is fine as-is.  Perhaps the code would be clearer
> though, if we merged those two asserts into one?
> 
>     /* Assert we're called before loading any tuples */
>     Assert(state->status == TSS_INITIAL &&
>            state->memtupcount == 0);

Makes sense to me.  James, do you want to submit a new patch?

> I'm not totally sure about the usefulness/relevance of the two
> assertions following these, but they could likely do with their
> own comment(s), because this one surely isn't covering them.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Yes, planning on it, just a bit behind right now so will likely be a
few more days at least.

On Thu, Sep 5, 2019 at 4:57 PM Alvaro Herrera from 2ndQuadrant
<alvherre@alvh.no-ip.org> wrote:
>
> On 2019-Aug-26, Tom Lane wrote:
>
> > James Coleman <jtc331@gmail.com> writes:
>
> > I think the comment is fine as-is.  Perhaps the code would be clearer
> > though, if we merged those two asserts into one?
> >
> >       /* Assert we're called before loading any tuples */
> >       Assert(state->status == TSS_INITIAL &&
> >              state->memtupcount == 0);
>
> Makes sense to me.  James, do you want to submit a new patch?
>
> > I'm not totally sure about the usefulness/relevance of the two
> > assertions following these, but they could likely do with their
> > own comment(s), because this one surely isn't covering them.
>
>
> --
> Álvaro Herrera                https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Sep-05, James Coleman wrote:

> Yes, planning on it, just a bit behind right now so will likely be a
> few more days at least.

[ shrug ]  It seemed to require no further work, so I just pushed Tom's
proposed change.

I added an empty line after the new combined assertion, which makes
clearer (to me anyway) that the other assertions are unrelated.

Thanks

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> [ shrug ]  It seemed to require no further work, so I just pushed Tom's
> proposed change.
> I added an empty line after the new combined assertion, which makes
> clearer (to me anyway) that the other assertions are unrelated.

Actually, the thing I wanted to add was some actual comments for
those other assertions.  But that requires a bit of research that
I hadn't made time for...

            regards, tom lane