Skip to content

Fetches all rows by default on query, like libpq. #117

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RangelReale
Copy link

This allows multiple queries to be active simultaneously
Provides a singlerowmode=true parameter, like libpq (to keep working like today)

fixes #81

…ple queries to be active simultaneously

* Provides a singlerowmode=true parameter, like libpq (to keep working like today)
@@ -607,6 +620,66 @@ func (rs *rows) Next(dest []driver.Value) (err error) {
panic("not reached")
}

type rowscomplete struct {
rows *list.List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just [][]driver.Value here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't slices costly to append? As I can't know beforehand how many records there will be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

append behaviour is fairly reasonable, it doesn't just grow the slice by one each time you append. list.List also uses reflection, so there's a performance hit from that.

http://play.golang.org/p/SybwNAQOQ9

PASS
BenchmarkSlice1000     10000        125335 ns/op
BenchmarkSlice10000     1000       2363422 ns/op
BenchmarkSlice100000          50      28006284 ns/op
BenchmarkSlice1000000          5     273778224 ns/op
BenchmarkSlice10000000         1    3812814581 ns/op
BenchmarkList1000       5000        468516 ns/op
BenchmarkList10000       500       5936557 ns/op
BenchmarkList100000       20      70525959 ns/op
BenchmarkList1000000           5     737343756 ns/op
BenchmarkList10000000          1    8153616298 ns/op
ok      bench   31.456s

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, I updated my fork.

@johto
Copy link
Contributor

johto commented Oct 7, 2013

-  return &rows{st: st}, nil
+  r := &rows{st: st}
+  if st.cn.singlerowmode {
+    return r, nil
+  } 

Isn't this the wrong way round? Inverting the condition seems to fix the problem with regression tests failing.

Also, is reverting 31326ad really necessary? As far as I can tell, you never have two queries actually running on the protocol level, so I don't see how it would cause any problems.

@RangelReale
Copy link
Author

Hmm I made a pull request using my master branch, now GitHub got my latest changes which weren't meant to be here.

The revert needed to be done because the scrath buffer was getting overriten when 2 queries were done at the same time, parts of the resultset for one query were appearing on the other one.

@johto
Copy link
Contributor

johto commented Oct 12, 2013

Oh, I think I now see how that could happen.

I think instead of making everyone pay the performance penalty from reverting that commit, you should only impose it on people using the singlerowmode by copying the data before releasing the connection back into the pool.

Also, is there a specific reason you chose not to implement the sql.Rows interface for rowscomplete?

@johto
Copy link
Contributor

johto commented Oct 12, 2013

Also, is there a specific reason you chose not to implement the sql.Rows interface for rowscomplete?

Oh, looks like you're not even supposed to; database/sql should take care of that. Scratch that part.

@timbunce
Copy link

timbunce commented Oct 9, 2017

@RangelReale any chance you could update this PR?

The current code changes the behavior to fetch-all from the current implicit 'single-row mode'. If you also inverted the option so the current behaviour was maintained by default I think you'd address the concerns raised in #81 and stand a better chance of getting this merged.

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

Successfully merging this pull request may close these issues.

Can't run two queries in a transaction?
4 participants