internal/protocol: Expose 32-bit parameter count support #243

cole-miller · 2023-04-11T23:24:03Z

Closes #210

Signed-off-by: Cole Miller cole.miller@canonical.com

cole-miller · 2023-04-12T00:11:48Z

Note that there's no attempt in this PR to figure out whether the remote server has the patch canonical/dqlite#407 or retry the request if it's refused due to an unrecognized schema version. If we care about the combination of new go-dqlite + old dqlite, I can add such code.

internal/protocol/schema.sh

freeekanayaka · 2023-04-12T07:19:43Z

Looks good to me. I'm agnostic about support for go-dqlite + old dqlite, although this change might hit some users.

One approach would be to use schema 0 if there are at most 256 parameters, and schema 1 if there are more. In that way the only difference with an old dqlite server would be that the old go-dqlite would throw a panic, and this new go-dqlite would get an error from the server. It would be a bit of work to implement though since the code doesn't quite support switching between schema versions.

MathieuBordere · 2023-04-12T11:47:05Z

One approach would be to use schema 0 if there are at most 256 parameters, and schema 1 if there are more. In that way the only difference with an old dqlite server would be that the old go-dqlite would throw a panic, and this new go-dqlite would get an error from the server. It would be a bit of work to implement though since the code doesn't quite support switching between schema versions.

I would be in favor of that, using schema version 0 when parameter count is <= 255.

cole-miller · 2023-04-12T15:15:30Z

Implemented the schema-switching suggestion; this should be ready to go.

TheSignPainter98 · 2023-04-12T16:38:34Z

internal/protocol/response.go

@@ -15,13 +15,13 @@ func DecodeFailure(response *Message) (code uint64, message string, err error) {
 		e := ErrRequest{}
 		e.Code = response.getUint64()
 		e.Description = response.getString()
-                err = e


I'm guessing these whitespace changes come from the changes to schema.sh. Is it worth checking gofmt in the CI to avoid this problem coming back in future? I imagine this would probably be a separate PR

Good catch.

driver/driver.go

TheSignPainter98 · 2023-04-12T17:04:54Z

internal/protocol/request.go

 }

-// EncodeExec encodes a Exec request.
-func EncodeExec(request *Message, db uint32, stmt uint32, values NamedValues) {
+// EncodeExecV0 encodes a Exec request.


This comment doesn't highlight the difference between EncodeExecV0 and EncodeExecV1---the choice between each is unclear. As this comes up in quite a few places, is it worth putting a doc at the top of the file to establish the convention? Perhaps something like:

// Many functions in this file have two versions, distinguished // by their suffix. Those named FooV0 handle queries which accept // at most 255 arguments. Those named FooV1 handle queries with // arbitrarily-many arguments but ...

It would be possible to do this, but it would make schema.sh more complicated and these functions are all internal, so I don't think that documenting them in detail is urgent.

TheSignPainter98 · 2023-04-12T17:14:36Z

internal/protocol/message.go

+	}
+
+	n := uint8(l) // N of params
+	if n == 0 {


Somehow this early return on len(values) == 0 would feel more natural sooner. Would it be better to have it just before the l > 255 check?

The same would hold for Message.putNamedValues32

I don't think it can happen earlier, because the point of the l > 255 check is to be sure that the n := uint8(l) cast is safe.

I'm not sure I follow, the > 255 check makes the != 0 check look independent to me; is this not possible?

l := len(values) if l == 0 { return } if l > 255 { panic("...") } m.putUint8(uint8(l))

Ah yeah, sorry I misunderstood what you meant. Yes, your code looks correct, and I agree it feels slightly more natural.

TheSignPainter98 · 2023-04-12T17:17:51Z

internal/protocol/message.go

+// parameter count (new format).
+func (m *Message) putNamedValues32(values NamedValues) {
+	l := len(values)
+	if l > (1<<32) - 1 {


Would it be clearer to use math.MaxUint32 here? Admittedly this would feel strange

Sure, seems reasonable

TheSignPainter98 · 2023-04-12T17:25:44Z

internal/protocol/message.go

+	l := len(values)
+	if l > 255 {
+		// safeguard, should have been checked beforehand.
+		panic("too many parameters")


Admittedly this isn't a convention elsewhere (so likely not an issue for this PR) but would it be better to have "internal error: ..." to highlight that the user isn't at-fault for these panics?

internal/protocol/schema.sh

freeekanayaka · 2023-04-12T18:34:41Z

driver/driver_test.go

@@ -252,7 +252,7 @@ func TestStmt_ExecTooManyParams(t *testing.T) {
 		values[i] = int64(1)
 	}
 	_, err = stmt.Exec(values)
-	require.Errorf(t, err, "too many parameters (256) max = 255")
+	require.NoError(t, err)


It would be nice to not only check that no error occurred, but that the parameters where actually correctly handled, i.e. 256 rows were inserted, with values ranging from 1 to 255.

freeekanayaka · 2023-04-12T18:38:40Z

driver/driver_test.go

@@ -330,7 +330,7 @@ func TestStmt_QueryTooManyParams(t *testing.T) {
 		values[i] = int64(1)
 	}
 	_, err = stmt.Query(values)
-	require.Errorf(t, err, "too many parameters (256) max = 255")
+	require.NoError(t, err)


Same here. It would be good to do something like:

insert 300 rows, with values ranging from 1 to 300

run the SELECT statement in the test

check that exactly 256 rows are returned, with the expected values

Perhaps it'd be worth using a slightly larger value than 256, say 260 or 270, just to test not only the boundary value but also beyond.

freeekanayaka · 2023-04-12T18:43:31Z

internal/protocol/message.go

+	}
+
+	n := uint8(l) // N of params
+	if n == 0 {


I don't think it can happen earlier, because the point of the l > 255 check is to be sure that the n := uint8(l) cast is safe.

freeekanayaka · 2023-04-12T18:47:08Z

internal/protocol/response.go

@@ -15,13 +15,13 @@ func DecodeFailure(response *Message) (code uint64, message string, err error) {
 		e := ErrRequest{}
 		e.Code = response.getUint64()
 		e.Description = response.getString()
-                err = e


Good catch.

internal/protocol/schema.sh

Tabs only. Signed-off-by: Cole Miller <cole.miller@canonical.com>

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller mentioned this pull request Apr 11, 2023

Support for schema v1 requests with more parameters #210

Closed

cole-miller force-pushed the more-params branch 6 times, most recently from 7579b55 to b130bdf Compare April 11, 2023 23:56

cole-miller marked this pull request as ready for review April 12, 2023 00:09

cole-miller requested a review from MathieuBordere April 12, 2023 00:09

cole-miller commented Apr 12, 2023

View reviewed changes

internal/protocol/schema.sh Show resolved Hide resolved

cole-miller force-pushed the more-params branch 4 times, most recently from fa989af to 7f0d6bc Compare April 12, 2023 14:59

TheSignPainter98 reviewed Apr 12, 2023

View reviewed changes

freeekanayaka reviewed Apr 12, 2023

View reviewed changes

cole-miller force-pushed the more-params branch 7 times, most recently from c8c5727 to 22b58b5 Compare April 14, 2023 20:43

cole-miller added 2 commits May 2, 2023 13:40

schema.sh: Fix whitespace

e1a4024

Tabs only. Signed-off-by: Cole Miller <cole.miller@canonical.com>

internal/protocol: Update to support more parameters

61fc072

Signed-off-by: Cole Miller <cole.miller@canonical.com>

cole-miller force-pushed the more-params branch from 22b58b5 to 61fc072 Compare May 2, 2023 11:41

cole-miller merged commit 49cc09c into canonical:master May 2, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/protocol: Expose 32-bit parameter count support #243

internal/protocol: Expose 32-bit parameter count support #243

cole-miller commented Apr 11, 2023

cole-miller commented Apr 12, 2023

freeekanayaka commented Apr 12, 2023

MathieuBordere commented Apr 12, 2023

cole-miller commented Apr 12, 2023

TheSignPainter98 Apr 12, 2023 •

edited

Loading

freeekanayaka Apr 12, 2023

TheSignPainter98 Apr 12, 2023

cole-miller Apr 12, 2023

TheSignPainter98 Apr 12, 2023

freeekanayaka Apr 12, 2023

TheSignPainter98 Apr 13, 2023

freeekanayaka Apr 13, 2023

TheSignPainter98 Apr 12, 2023

cole-miller Apr 12, 2023

TheSignPainter98 Apr 12, 2023

freeekanayaka Apr 12, 2023

freeekanayaka Apr 12, 2023

freeekanayaka Apr 12, 2023

freeekanayaka Apr 12, 2023

internal/protocol: Expose 32-bit parameter count support #243

internal/protocol: Expose 32-bit parameter count support #243

Conversation

cole-miller commented Apr 11, 2023

cole-miller commented Apr 12, 2023

freeekanayaka commented Apr 12, 2023

MathieuBordere commented Apr 12, 2023

cole-miller commented Apr 12, 2023

TheSignPainter98 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSignPainter98 Apr 12, 2023 •

edited

Loading