FIX: Repeated iteration should return all records

[sminnee]

See #9097

This is a bugfix whose root-cause is a long-standing misunderstanding of the Iterator::rewind() API. Therefore this change is correcting that – rewind() no longer returns a value; nor does its supporting method, seek().

Is this an API change or a bugfix? Given that this is generally speaking an internal API accessed via Iterator, and our implementation is wrong, I recommend that we include it on the 4.4 branch. To put it another way, the stated API was the bug.

[sminnee]

To put it another way - what's more likely: that people are going to be iterating on queries more than once, or calling and that they are calling seek() and rewind() directly and expecting a result? I think the former. We can only fix one.

Arguably, I could keep PDOQuery::rewind() and seek() returning a value because of the memory-inefficient way it has been built. Happy to get feedback on that.

[sminnee]

The other thing I could do is preserve the behaviour of seek() by

• Defining seekWithoutReturn() on MySQLQuery and PostgreSQLQuery, marked experimental.
• Calling that if it exists in favour of seek() (but not including it as abstract in Query)

That, and excluding our mis-implementation of Iterator::rewind() from the public API, would mean that we're not breaking a public API with this change. Bit messier code, but that seems okay. I'd put a simpler fix on master too.

[ScopeyNZ]

Can't we just fix this by:

    public function seek($row)
    {
        if (is_object($this->handle)) {
            $this->handle->data_seek($row);
            $record = $this->nextRecord();
            $this->handle->data_seek($row);
            return $record;
        }
        return null;
    }

Yes it's weird but it's non-breaking. And hopefully the cost of seeking is negligible in this case?

[sminnee]

And hopefully the cost of seeking is negligible in this case?

Doubtful.

[sminnee]

But we should probably do some profiling.

[ScopeyNZ]

I don't really think this API is well used. I'd much rather accept the patch as-is rather than introducing a seekWithoutReturn API. If the seek cost is measurable then I think it's also worth accepting this patch as is - assuming that rewind is sometimes called without it actually being necessarily required (more than it's called and then used).

[ScopeyNZ]

Also, I'm okay with dropping the return on rewind (that's Iterator API definition that we're breaking). seek is our own API definition though.

[sminnee]

OK well your change doubles the execution time of calling map() on a 4-records resultset 100 times.

However, for a 100 executions it goes from 0.55ms to 1.1ms. So it's probably negligible, especially given that this is a somewhat rare API. The proportionate difference was also less with 10,000 executions (74ms vs 99ms).

So I'll put Guy's suggestion in the PR.

[ScopeyNZ]

Perfect. I think this is good.

• Seeking is probably already rare
• seek is our own API definition - I feel worse about breaking it so seeking twice is a nice compromise
• Happy to remove the return in rewind because the Iterator interface declares a void return type anyway.
• SS5 will not have this problem

[sminnee]

This PR will be needed to get non-PDO PGSQL passing
silverstripe/silverstripe-postgresql#99

[sminnee]

It looks like filterByCallback at least is returning the first record twice, so we're erroring the other way! :-|

TLDR: this still needs a bit of work

[sminnee]

OK looks like the fix to MySQLStatement::rewind() has sorted this out; we'll just need a fix for PGSQL (and a linting fix)

[sminnee]

I'm also gonna rebase this against 4 so that the php 7.4 test suite is reinstated.

[sminnee]

OK relevant postgresql fix has been merged now and I've pushed a linting fix. I think this is ready to merge if the tests pass, but there shouldn't be any "benign" test failure now.

[sminnee]

💚