Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot scan when query is reused and iterator is closed without reading all items #1445

Closed
mmatczuk opened this issue May 22, 2020 · 6 comments

Comments

@mmatczuk
Copy link
Contributor

mmatczuk commented May 22, 2020

What version of Cassandra are you using?

Scylla 4.0

What version of Gocql are you using?

v0.0.0-20200519160334-799061058e31

What did you do?

package gocqlx

import (
	"fmt"
	"testing"

	"github.com/gocql/gocql"
)

func TestName(t *testing.T) {
	c := gocql.NewCluster("127.0.0.1")

	session, err := WrapSession(c.CreateSession())
	if err != nil {
		t.Fatal(err)
	}

	err = session.ExecStmt("CREATE KEYSPACE IF NOT EXISTS xx  WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1}")
	if err != nil {
		t.Fatal(err)
	}
	err = session.ExecStmt("CREATE TABLE IF NOT EXISTS xx.xx (id int PRIMARY KEY)")
	if err != nil {
		t.Fatal(err)
	}

	insert := session.Query("INSERT INTO xx.xx (id) VALUES (?)", nil)
	for i := 0; i < 1000; i++ {
		if err := insert.Bind(i).Exec(); err != nil {
			t.Fatal(err)
		}
	}

	cnt := 0
	q := session.Query("SELECT * from xx.xx", nil)
	q.PageSize(10)

	for {

		iter := q.Iter()
		var id int
		for i := 0; i < 12; i++ {
			if !iter.Scan(&id) {
				t.Fatal("WTF", cnt, iter.Close())
			}
			fmt.Println(id)
			cnt++
		}
		fmt.Println("Close", iter.Close())
	}
}

What did you expect to see?

No error

What did you see instead?

In this example we fill table with 1k items, set page size to 10 and try to always read first 12 items.
If query is reused after reading 600 items it breaks.
Changing page size changes when it breaks but it always breaks.

Typical tail of test

165
576
562
301
656
192
Close <nil>
--- FAIL: TestName (0.42s)
    bla_test.go:44: WTF 600 <nil>
FAIL
exit status 1
FAIL    github.com/scylladb/gocqlx/v2   0.419s

Creating new query for each iterator works fine.

@tengu-alt
Copy link
Contributor

tengu-alt commented Jan 21, 2025

I have reproduced this on the provided version.
That happens because PageSize and amount of iterations are not equal. After we get the first 10 items - Scan() returns a new Iter, which we use for getting the rest 2 items. After that, the new Iter is created (with a new paging state) and we skip the remaining 8 items. That explains why we got only 600 elements in the result instead of 1000.
I have set the for loop for i < 14 and got 700 elements in the result.

After I tried to run the provided code on the current version I encountered endless recursion if we don't reset the query PageState manually after each iteration. (Additionally: if we use the PageState() method - we got only 22 elements, because it disables autopaging)

@joao-r-reis Is it desired state when we fall into recursion if we don't reset pageState? Or is it a bug that needs to be fixed?

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Jan 22, 2025

This looks somewhat related to #1447 #1508 #1511 #1229

There's currently an open PR to change the Query/Batch API so that Query and Batch objects can be reused but I think this has a bit of a different scope to this current issue (#1445).

As far as I understand there shouldn't be a bug with the current paging implementation, I think the issue here is that a new Iter object is being created without setting the paging state (as @tengu-alt mentioned). If you see the example on the documentation, the paging state is stored on a variable and set when creating a new Iter object on the next iteration of the for loop: https://pkg.go.dev/github.com/gocql/gocql#example-package-Paging

@tengu-alt
Copy link
Contributor

tengu-alt commented Jan 23, 2025

#1508

The recursion appeared in that one.
If I understand correctly the issue should be closed?

@joao-r-reis
Copy link
Contributor

Yes, I believe this should be closed as it looks like API misuse

@joao-r-reis joao-r-reis closed this as not planned Won't fix, can't repro, duplicate, stale Jan 23, 2025
@joao-r-reis
Copy link
Contributor

joao-r-reis commented Jan 23, 2025

My earlier reply mentions manual paging but even without setting the page state manually it should work fine without recursion. If you use a single Iter object and call iter.Scan on a for loop on that same iter object it should work fine right? Or alternatively if you create a new Query object everytime you call Iter() then it should also work fine. Calling Iter() multiple times on the same query object can lead to weird behavior which is why we want to make Query immutable in CASSGO-22

@tengu-alt
Copy link
Contributor

tengu-alt commented Jan 24, 2025

If you use a single Iter object and call iter.Scan on a for loop on that same iter object it should work fine right?

Yes. I've created iter outside of the loops and it worked fine.

Or alternatively if you create a new Query object everytime you call Iter() then it should also work fine.

If I understood correctly, and it means creating new query inside of an endless loop after the nested (creating new query inside of that also woked) loop is done - then it also works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants