pgwire: pre-allocate a temporary buffer to use when serializing arrays/tuples #66941
Conversation
|
|
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
blathers-crl
bot
added
O-community
|
This change is |
biradarganesh25
force-pushed
the
reduce_allocation
branch
from
7656792
to
6c99ca3
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
biradarganesh25
changed the title
|
I ran BenchmarkEncodings for only arrays/tuples and it came out to be 20 secs faster with this change. Have attached the outputs. |
biradarganesh25
changed the title
There was a problem hiding this comment.
Thanks for the patch!
I ran BenchmarkEncodings for only arrays/tuples and it came out to be 20 secs faster with this change. Have attached the outputs.
If you want to present benchmark results, please use the benchstat tool to create a diff between the two outputs. The "20s faster" doesn't mean much by itself because the running time of the benchmark is not directly tied to the running time of each iteration (i.e. different benchmark runs might run for a different number of iterations). Having said this, your numbers do look very good. Consider adding a b.ReportAllocs() call to the encoding benchmark, and then, besides the running time speedup, we should also see a decrease in allocations per iteration.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @biradarganesh25, @jordanlewis, @knz, and @rafiss)
pkg/sql/pgwire/types.go, line 651 at r1 (raw file):
case *tree.DTuple: subWriter := b.tempWriteBuffer
I think the way you patch is doing it does not feel very encapsulated: we're reaching into b and operating on the tempWriteBuffer directly, and we're also burdening the user of b with having to call the reset below. Also having the writeBuffer contain another writeBuffer seems to suggest that we're missing the right layering - for example it'd be illegal to call finishMsg on the tempWriteBuffer because we're not initializing its byteCount metric.
I would like to suggest an alternative: add writeTuple/writeArray methods to the writeBuffer. Instead of tempWriteBuffer, add another bytes.Buffer (perhaps named scratch). Make the implementation of writeTuple something like:
func (b *writeBuffer) writeTuple(t) {
// Temporarily hijack b.wrapped.
mainBuf := b.wrapped
b.wrapped = b.scratch
b.putInt32(int32(len(v.D)))
tupleTypes := t.TupleContents()
for i, elem := range v.D {
oid := tupleTypes[i].Oid()
subWriter.putInt32(int32(oid))
subWriter.writeBinaryDatum(ctx, elem, sessionLoc, tupleTypes[i])
}
b.wrapped = mainBuf
b.writeLengthPrefixedBuffer(b.scratch)
b.scratch.Reset()
}What do you think?
pkg/sql/pgwire/write_buffer.go, line 44 at r1 (raw file):
(Fixes #21711)
Delete this part :)
pkg/sql/pgwire/write_buffer.go, line 44 at r1 (raw file):
bytecount *metric.Counter // (Fixes #21711) This tempWriteBuffer will be used to store temporary data when serializing
nit: we generally make comments start with the field name, so scratch the "This".
pkg/sql/pgwire/write_buffer.go, line 46 at r1 (raw file):
// (Fixes #21711) This tempWriteBuffer will be used to store temporary data when serializing // array/tuple tempWriteBuffer *writeBuffer
does this need to be a pointer? If not, let's make it a value to save an allocation and to make the ownership clear.
pkg/sql/pgwire/write_buffer.go, line 53 at r1 (raw file):
b.init(bytecount) // Initializing the temporary buffer also, see the writeBuffer defination for more info
s/defination/definition
pkg/sql/pgwire/write_buffer.go, line 54 at r1 (raw file):
// Initializing the temporary buffer also, see the writeBuffer defination for more info b.tempWriteBuffer = new(writeBuffer)
I think you want this code in the init method below. Note that the user of writeBuffer that matters does not call newWriteBuffer.
|
@andreimatei Thanks a lot for the review :) Please give me some time, I will address all the comments and fix the code accordingly. |
biradarganesh25
force-pushed
the
reduce_allocation
branch
from
6c99ca3
to
00a6a81
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 651 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I think the way you patch is doing it does not feel very encapsulated: we're reaching into
band operating on thetempWriteBufferdirectly, and we're also burdening the user ofbwith having to call theresetbelow. Also having thewriteBuffercontain anotherwriteBufferseems to suggest that we're missing the right layering - for example it'd be illegal to callfinishMsgon thetempWriteBufferbecause we're not initializing itsbyteCountmetric.
I would like to suggest an alternative: addwriteTuple/writeArraymethods to thewriteBuffer. Instead oftempWriteBuffer, add anotherbytes.Buffer(perhaps namedscratch). Make the implementation ofwriteTuplesomething like:func (b *writeBuffer) writeTuple(t) { // Temporarily hijack b.wrapped. mainBuf := b.wrapped b.wrapped = b.scratch b.putInt32(int32(len(v.D))) tupleTypes := t.TupleContents() for i, elem := range v.D { oid := tupleTypes[i].Oid() subWriter.putInt32(int32(oid)) subWriter.writeBinaryDatum(ctx, elem, sessionLoc, tupleTypes[i]) } b.wrapped = mainBuf b.writeLengthPrefixedBuffer(b.scratch) b.scratch.Reset() }What do you think?
This makes much more sense, I have made the change. Please review. Thanks :)
pkg/sql/pgwire/write_buffer.go, line 44 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
(Fixes #21711)Delete this part :)
Done.
pkg/sql/pgwire/write_buffer.go, line 44 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: we generally make comments start with the field name, so scratch the "This".
Done.
pkg/sql/pgwire/write_buffer.go, line 46 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
does this need to be a pointer? If not, let's make it a value to save an allocation and to make the ownership clear.
Changed the implementation, no longer required.
pkg/sql/pgwire/write_buffer.go, line 53 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
s/defination/definition
Changed the implementation, no longer required.
pkg/sql/pgwire/write_buffer.go, line 54 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I think you want this code in the
initmethod below. Note that the user ofwriteBufferthat matters does not callnewWriteBuffer.
Changed the implementation, no longer required.
|
I also used |
There was a problem hiding this comment.
Seems like only linting checks are failing, I will fix those once the implementation is finalized.
Reviewable status:
pkg/sql/pgwire/write_buffer.go, line 202 at r2 (raw file):
} func (b *writeBuffer) writeArray(ctx context.Context, v *tree.DArray, sessionLoc *time.Location, t *types.T){
There is some repeated code here when compared to writeTuple, but I was not sure if I should combine both the functions. If I did, I'd have to pass the datum d and unwrap it here again and use type switching (not sure if there is another way). Not sure what to do here.
There was a problem hiding this comment.
@andreimatei can you review the result thanks
I have only minor comments on the remaining.
Reviewable status:
pkg/sql/pgwire/write_buffer.go, line 171 at r2 (raw file):
} func (b *writeBuffer) writeTuple(ctx context.Context, v *tree.DTuple, sessionLoc *time.Location, t *types.T){
This function and the one below belong to types.go. Please don't move them across files.
You can keep them in the original files and if you're bothered about the ideas that the methods of writeBuffer "belong" to write_buffer.go,
then change the functions to take the writeBuffer as argument instead.
pkg/sql/pgwire/write_buffer.go, line 172 at r2 (raw file):
func (b *writeBuffer) writeTuple(ctx context.Context, v *tree.DTuple, sessionLoc *time.Location, t *types.T){ // initialState will be used to decide whether to swap the wrapped and the scratch buffer
all comments are sentences and thus should finish with a period.
here and below.
pkg/sql/pgwire/write_buffer.go, line 191 at r2 (raw file):
if initialState == false{
I think you forgot to run gofmt on the files.
biradarganesh25
force-pushed
the
reduce_allocation
branch
from
00a6a81
to
a36ff90
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/write_buffer.go, line 171 at r2 (raw file):
Previously, knz (kena) wrote…
This function and the one below belong to types.go. Please don't move them across files.
You can keep them in the original files and if you're bothered about the ideas that the methods ofwriteBuffer"belong" to write_buffer.go,
then change the functions to take the writeBuffer as argument instead.
Done.
pkg/sql/pgwire/write_buffer.go, line 172 at r2 (raw file):
Previously, knz (kena) wrote…
all comments are sentences and thus should finish with a period.
here and below.
Done.
pkg/sql/pgwire/write_buffer.go, line 191 at r2 (raw file):
Previously, knz (kena) wrote…
I think you forgot to run
gofmton the files.
Done.
biradarganesh25
force-pushed
the
reduce_allocation
branch
2 times, most recently
from
ea1ab93
to
d777e8e
Compare
There was a problem hiding this comment.
Please reword the release note. The audience for it is regular CRDB users, and the way it's written now won't make much sense for them. How about simply:
Release note (performance improvement): The performance of queries returning many arrays has been improved.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
if initialState == false { // We are done with recursive calls, copy scratch buffer to wrapped buffer and reset it.
Is this correct for the tuple-inside-tuple case? I think we need to perform the b.writeLengthPrefixedBuffer(&b.scratch)at every level of the recursion, don't we?
If that's right, please add a test that encodes and then decodes a tuple of tuples and an array of arrays. You can get inspiration from TestByteArrayRoundTrip I think.
The way I would do it, I think, is detect whether we're inside a recursive call (like you're doing with initialState and, if we are, revert to allocating a new writeBuffer.
But actually:
Now that I look at the details, it seems to me that we don't need the extra buffer. I thought that the difficulty with writing the tuples/arrays was that we need to prepend their encoding with a variable-length size field. But now that I read writeBuffer.putInt32(), I see that the prefix is not variable-length at all; it's always 4 bytes. So I think all we need to do is leave 4 empty bytes in the buffer, and come back to writing them later. The bytes.Buffer lets you do that through a trick: buf.Bytes() returns a slice that aliases the buffer, so if you modify arbitrary bytes in that slice, it'll be reflected in the buffer. For example: https://play.golang.org/p/wxCsLrxcA9J
So I don't think we need these new functions, and we don't need to add fields to writeBuffer. I thknk all we need to do is change the tuple and array cases in writeBinaryDatumNotNull to not use that subWriter any more; instead, just write 4 dummy bytes, then do the writeBinaryDatum calls on b, and then go back and fill those 4 bytes.
Makes sense?
pkg/sql/pgwire/types.go, line 177 at r3 (raw file):
// Look at writeTuple for more info on initialState. initialState := b.usingScratch if b.usingScratch == false {
nit: if !b.usingScratch
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Is this correct for the tuple-inside-tuple case? I think we need to perform the
b.writeLengthPrefixedBuffer(&b.scratch)at every level of the recursion, don't we?
If that's right, please add a test that encodes and then decodes a tuple of tuples and an array of arrays. You can get inspiration fromTestByteArrayRoundTripI think.The way I would do it, I think, is detect whether we're inside a recursive call (like you're doing with
initialStateand, if we are, revert to allocating a newwriteBuffer.But actually:
Now that I look at the details, it seems to me that we don't need the extra buffer. I thought that the difficulty with writing the tuples/arrays was that we need to prepend their encoding with a variable-length size field. But now that I readwriteBuffer.putInt32(), I see that the prefix is not variable-length at all; it's always 4 bytes. So I think all we need to do is leave 4 empty bytes in the buffer, and come back to writing them later. Thebytes.Bufferlets you do that through a trick:buf.Bytes()returns a slice that aliases the buffer, so if you modify arbitrary bytes in that slice, it'll be reflected in the buffer. For example: https://play.golang.org/p/wxCsLrxcA9JSo I don't think we need these new functions, and we don't need to add fields to
writeBuffer. I thknk all we need to do is change the tuple and array cases inwriteBinaryDatumNotNullto not use thatsubWriterany more; instead, just write 4 dummy bytes, then do thewriteBinaryDatumcalls onb, and then go back and fill those 4 bytes.
Makes sense?
1.) Yes, you are correct - my code will only work for one level of recursion, we do need b.writeLengthPrefixedBuffer(&b.scratch) for every level.
2.) I was wondering why the tests passed and you are correct again - there were no nested tuple/array testcases. Just to confirm are they supported (i.e do we need a testcase for it)? I am asking because I thought that there might be some reason as to why that particular test case was not already added. If the test case is needed, I will try to add it (might need a bit more time as I am not completely familiar with the code base).
3.) I get your point about reserving the 4 byte length - I will change the implementation accordingly.
pkg/sql/pgwire/types.go, line 177 at r3 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit:
if !b.usingScratch
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, biradarganesh25 (Ganesh Biradar) wrote…
1.) Yes, you are correct - my code will only work for one level of recursion, we do need
b.writeLengthPrefixedBuffer(&b.scratch)for every level.2.) I was wondering why the tests passed and you are correct again - there were no nested tuple/array testcases. Just to confirm are they supported (i.e do we need a testcase for it)? I am asking because I thought that there might be some reason as to why that particular test case was not already added. If the test case is needed, I will try to add it (might need a bit more time as I am not completely familiar with the code base).
3.) I get your point about reserving the 4 byte length - I will change the implementation accordingly.
CockroachDB does not support nested arrays, but it does support nested tuples.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
CockroachDB does not support nested arrays, but it does support nested tuples.
I am having a bit of trouble implementing the test function to test nested tuples. After looking at TestByteArrayRoundTrip, I thought the way to test to it would be to make nested tuples using NewDTuple , encode it using the writeBinaryDatum function and then decode it and compare with the original *tree.DTuple. But I am unable to find decode function for tuples and DecodeDatum function handles all types except tuples. I feel like I am missing something here, would appreciate if someone gave me a few more pointers on how to implement the test for nested tuples.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, biradarganesh25 (Ganesh Biradar) wrote…
I am having a bit of trouble implementing the test function to test nested tuples. After looking at
TestByteArrayRoundTrip, I thought the way to test to it would be to make nested tuples usingNewDTuple, encode it using thewriteBinaryDatumfunction and then decode it and compare with the original*tree.DTuple. But I am unable to find decode function for tuples andDecodeDatumfunction handles all types except tuples. I feel like I am missing something here, would appreciate if someone gave me a few more pointers on how to implement the test for nested tuples.
I've spent some time looking at this. I don't have great answers for you. I hope @rafiss or @knz or perhaps @otan will have more to say.
Some hints, though:
This comment says that tuples "cannot (yet) be used in tables":
cockroach/pkg/sql/types/types.go
Lines 128 to 132 in a40102a
Presumably this explains why the code for decoding them is hard to find - clients never send them? But I believe we do send them to clients (for example in response to a
SELECT (1,2,3), so we have to encode them.
This test, run through TestEncodings, seems to have a golden encoded tuple.
The Postgres docs say to check the source for the binary encoding of the different types :)
I think it'd be a good idea for DecodeDatum to learn how to decode them. I'd also be interested in how our command line client knows to decode them. If you run cockroach sql and pass select (1,2,3), the tuple seems to get properly decoded somehow, so presumably the code exists somewhere.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
If you run cockroach sql and pass select (1,2,3), the tuple seems to get properly decoded somehow, so presumably the code exists somewhere.
By this I meant that postgres drivers need to know how to decode these. I think cockroach sql uses pgx, and I've looked around here briefly
for a tuple type, but I didn't find it.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
If you run cockroach sql and pass select (1,2,3), the tuple seems to get properly decoded somehow, so presumably the code exists somewhere.
By this I meant that postgres drivers need to know how to decode these. I think
cockroach sqlusespgx, and I've looked around here briefly
for a tuple type, but I didn't find it.
Yeah, I should have said we support nested tuples in a limited way that I have trouble fully understanding.
Some points:
- the npgsql driver docs have a helpful thing about the binary formats of all the types including tuples: https://www.npgsql.org/dev/types.html Note that these docs (and Postgres) call it a "record". We use both names in various parts of our code.
- I believe this issue is why we cannot use tuples in tables right now sql: fix tuple key encoding and decoding #49975
- You're correct that we don't have decoding logic for tuples. TestEncodings skips the decode test for them
cockroach/pkg/sql/pgwire/encoding_test.go
Lines 240 to 242 in bcc6002
- Here is where pgx implements record (aka tuple) decoding https://github.com/jackc/pgtype/blob/master/record.go
- Note that it doesn't allow you to encode tuples, since Postgres doesn't allow them as input either
- I'm actually a bit surprised by the comment there: "The text format output format from PostgreSQL does not include type information and is therefore impossible to decode." We definitely return tuples in a text encoding, and our CLI handles it. (NB: our CLI uses lib/pq and database/sql right now though, and I think that can just handle text encodings automatically?)
Anyway to your question, how do you make a nested tuple? Try modifying pkg/cmd/generate-binary/main.go to add this case (see the comment in TestEncodings in encoding_test.go for more instructions). I'll be able to try that out myself in a little bit, in case you are unable to.
|
Our CLI does not decode tuples. It simply prints their text encoding as text. |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
Yeah, I should have said we support nested tuples in a limited way that I have trouble fully understanding.
Some points:
- the npgsql driver docs have a helpful thing about the binary formats of all the types including tuples: https://www.npgsql.org/dev/types.html Note that these docs (and Postgres) call it a "record". We use both names in various parts of our code.
- I believe this issue is why we cannot use tuples in tables right now sql: fix tuple key encoding and decoding #49975
- You're correct that we don't have decoding logic for tuples. TestEncodings skips the decode test for them
cockroach/pkg/sql/pgwire/encoding_test.go
Lines 240 to 242 in bcc6002
- Here is where pgx implements record (aka tuple) decoding https://github.com/jackc/pgtype/blob/master/record.go
- Note that it doesn't allow you to encode tuples, since Postgres doesn't allow them as input either
- I'm actually a bit surprised by the comment there: "The text format output format from PostgreSQL does not include type information and is therefore impossible to decode." We definitely return tuples in a text encoding, and our CLI handles it. (NB: our CLI uses lib/pq and database/sql right now though, and I think that can just handle text encodings automatically?)
Anyway to your question, how do you make a nested tuple? Try modifying
pkg/cmd/generate-binary/main.goto add this case (see the comment inTestEncodingsin encoding_test.go for more instructions). I'll be able to try that out myself in a little bit, in case you are unable to.
here's a diff you can use to get a nested tuple in TestEncodings
diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go
index 994aa228e0..ce4f620b22 100644
--- a/pkg/cmd/generate-binary/main.go
+++ b/pkg/cmd/generate-binary/main.go
@@ -525,6 +525,7 @@ var inputs = map[string][]string{
`array['test',NULL]::text[]`,
`array['test',NULL]::name[]`,
`array[]::int4[]`,
+ `(1, 2, ('hi', 3))`,
},
"(%s,null)": {
diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json
index cd8cabac6c..9cda6e2938 100644
--- a/pkg/sql/pgwire/testdata/encodings.json
+++ b/pkg/sql/pgwire/testdata/encodings.json
@@ -41,6 +41,13 @@
"TextAsBinary": [123, 125],
"Binary": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 23]
},
+ {
+ "SQL": "(1, 2, ('hi', 3))",
+ "Oid": 2249,
+ "Text": "(1,2,\"(hi,3)\")",
+ "TextAsBinary": [40, 49, 44, 50, 44, 34, 40, 104, 105, 44, 51, 41, 34, 41],
+ "Binary": [0, 0, 0, 3, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 1, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 2, 0, 0, 8, 201, 0, 0, 0, 26, 0, 0, 0, 2, 0, 0, 2, 193, 0, 0, 0, 2, 104, 105, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 3]
+ },
{
"SQL": "'hello'::char(8)",
"Oid": 1042,
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
here's a diff you can use to get a nested tuple in TestEncodings
diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go index 994aa228e0..ce4f620b22 100644 --- a/pkg/cmd/generate-binary/main.go +++ b/pkg/cmd/generate-binary/main.go @@ -525,6 +525,7 @@ var inputs = map[string][]string{ `array['test',NULL]::text[]`, `array['test',NULL]::name[]`, `array[]::int4[]`, + `(1, 2, ('hi', 3))`, }, "(%s,null)": { diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json index cd8cabac6c..9cda6e2938 100644 --- a/pkg/sql/pgwire/testdata/encodings.json +++ b/pkg/sql/pgwire/testdata/encodings.json @@ -41,6 +41,13 @@ "TextAsBinary": [123, 125], "Binary": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 23] }, + { + "SQL": "(1, 2, ('hi', 3))", + "Oid": 2249, + "Text": "(1,2,\"(hi,3)\")", + "TextAsBinary": [40, 49, 44, 50, 44, 34, 40, 104, 105, 44, 51, 41, 34, 41], + "Binary": [0, 0, 0, 3, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 1, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 2, 0, 0, 8, 201, 0, 0, 0, 26, 0, 0, 0, 2, 0, 0, 2, 193, 0, 0, 0, 2, 104, 105, 0, 0, 0, 23, 0, 0, 0, 4, 0, 0, 0, 3] + }, { "SQL": "'hello'::char(8)", "Oid": 1042,
Thanks for info Andrei, Rafi and Kena! So I am thinking of doing this: add this test case and change the implementation to reserve 4 bytes in the beginning and make sure it passes. Does that sound good?
Also I have a doubt: while I understand what these few functions are doing, I'm having a bit of trouble understanding the big picture. So this is what I've understood: the client serializes the statement (using pgx), we receive it and parse in it serveImpl and push the AST into statement buffer from where it is picked up by the connection executor, which does a lot of magic and produces results in tree.Datums. We serialize this and send it back to the client, where it is decoded by the pgx driver. Please correct me if I've misunderstood something regarding the pgwire's role.
Am I correct in assuming that we parse the serialized statement from client? (and not the query string?)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
So I am thinking of doing this: add this test case and change the implementation to reserve 4 bytes in the beginning and make sure it passes. Does that sound good?
Yes, sounds good.
In addition, I would also teach DecodeDatum to decode arrays and tuples, and I'd make TestEncodings not skip them for the decode subtest (the code Rafi linked above). I don't think there's any reason for DecodeDatum to not support them.
Also I have a doubt: while I understand what these few functions are doing, I'm having a bit of trouble understanding the big picture. So this is what I've understood: the client serializes the statement (using pgx), we receive it and parse in it serveImpl and push the AST into statement buffer from where it is picked up by the connection executor, which does a lot of magic and produces results in tree.Datums. We serialize this and send it back to the client, where it is decoded by the pgx driver. Please correct me if I've misunderstood something regarding the pgwire's role.
That's right.
I think (but I'm not sure) that the serialization/deserialization goes both ways: the server (CRDB) serializes rows that it sends to the client, and I think the client can also serialize values that it sends to the server. When the client wants to execute a prepared statement , it can send the arguments in either form or binary form (through a Bind protocol message). I assume that the binary form uses the same encodings that we're talking about here.
Thanks for staying on this!
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Yes, sounds good.
In addition, I would also teach DecodeDatum to decode arrays and tuples, and I'd make TestEncodings not skip them for the decode subtest (the code Rafi linked above). I don't think there's any reason for DecodeDatum to not support them.
Sure, I will try to add that part also. Will need more time to update the PR.
And thanks all of you for the having the patience to put up with a newbie :)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, biradarganesh25 (Ganesh Biradar) wrote…
Yes, sounds good.
In addition, I would also teach DecodeDatum to decode arrays and tuples, and I'd make TestEncodings not skip them for the decode subtest (the code Rafi linked above). I don't think there's any reason for DecodeDatum to not support them.Sure, I will try to add that part also. Will need more time to update the PR.
And thanks all of you for the having the patience to put up with a newbie :)
Hi @andreimatei , I've run into an issue. I came up with the following code to decode tuples (after consulting this: https://www.npgsql.org/dev/types.html (need to handle error conditions and stuff)
diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go
index 29c87f390c..80280a3ca6 100644
--- a/pkg/sql/pgwire/pgwirebase/encoding.go
+++ b/pkg/sql/pgwire/pgwirebase/encoding.go
@@ -493,6 +493,32 @@ func DecodeDatum(
}
case FormatBinary:
switch id {
+ case oid.T_record:
+ numberOfCols := int32(binary.BigEndian.Uint32(b[0:4]))
+ typs := make([]*types.T, 0, numberOfCols)
+ datums := make(tree.Datums, 0,numberOfCols)
+ curByte := int32(4)
+ curCol := int32(0)
+
+ for curCol < numberOfCols {
+ colOid := int32(binary.BigEndian.Uint32(b[curByte:curByte+4]))
+ colType := types.OidToType[oid.Oid(colOid)]
+ typs = append(typs, colType)
+ curByte = curByte + 4
+ colLength := int32(binary.BigEndian.Uint32(b[curByte:curByte+4]))
+ curByte = curByte + 4
+ if colLength == -1 {
+ datums = append(datums, tree.DNull)
+ } else {
+ colDatum, _:= DecodeDatum(evalCtx, colType, code, b[curByte:curByte+colLength])
+ curByte = curByte + colLength
+ datums = append(datums, colDatum)
+ }
+ curCol += 1
+ }
+
+ tupleTyps := types.MakeTuple(typs)
+ return tree.NewDTuple(tupleTyps, datums...), nil
case oid.T_bool:
if len(b) > 0 {
switch b[0] {
(END)
This is working properly, except for collated strings, where decoding is expected to fail
cockroach/pkg/sql/pgwire/encoding_test.go
Lines 243 to 246 in 7121f6d
The particular test case where it failed was:
SQL statement was: (1::char(2) COLLATE "en_US",null)
--- FAIL: TestEncodings/decode (0.06s)
panic: unsupported comparison: string to collatedstring{en_US} [recovered]
panic: unsupported comparison: string to collatedstring{en_US}
How should I solve this? Should I write another function to test only Tuples where there are no collated strings?
Couple of cases where its passed:
SQL statement was: (1::int8,2::int8,3::int8,4::int8,null)
SQL statement was: ('test with spaces'::BYTEA,null)
SQL statement was: ('test with spaces'::TEXT,null)
SQL statement was: ('name'::NAME,null)
SQL statement was: ('false'::JSONB,null)
SQL statement was: ('{"a": []}'::JSONB,null)
SQL statement was: (1::int4,null)
SQL statement was: (1::int2,null)
SQL statement was: (1::char(2),null)
SQL statement was: (1::char(1),null)
SQL statement was: (1::varchar(4),null)
SQL statement was: (1::text,null)
Also tested for a nested tuple and it passed. (@rafiss example:)
{
"SQL": "(1, 2, ('hi', 3))",
"Oid": 2249,
"Text": "(1,2,\"(hi,3)\")",
"TextAsBinary": [40, 49, 44, 50, 44, 34, 40, 104, 105, 44, 51, 41, 34, 41],
"Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 8, 201, 0, 0, 0, 30, 0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 2, 104, 105, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 3]
},
Although, I had to change the Binary format as the encoding as the one Rafi got was incorrect - the one generated by generate-binary/main.go (it identified 'hi' as T_unkown(705), but it should be T_text(25). The above encoding I generated from using our code - writeBinaryDatum. Could it be that the generate-binary/main.go is not generating properly for nested tuples for some reason? (I have not tried it myself, wanted to get the decoding code working properly first).
Also, just wanted to mention that decoding tuples from text as binary is not possible as there is no type information for individual elements of the tuple.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, biradarganesh25 (Ganesh Biradar) wrote…
Hi @andreimatei , I've run into an issue. I came up with the following code to decode tuples (after consulting this: https://www.npgsql.org/dev/types.html (need to handle error conditions and stuff)
diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go index 29c87f390c..80280a3ca6 100644 --- a/pkg/sql/pgwire/pgwirebase/encoding.go +++ b/pkg/sql/pgwire/pgwirebase/encoding.go @@ -493,6 +493,32 @@ func DecodeDatum( } case FormatBinary: switch id { + case oid.T_record: + numberOfCols := int32(binary.BigEndian.Uint32(b[0:4])) + typs := make([]*types.T, 0, numberOfCols) + datums := make(tree.Datums, 0,numberOfCols) + curByte := int32(4) + curCol := int32(0) + + for curCol < numberOfCols { + colOid := int32(binary.BigEndian.Uint32(b[curByte:curByte+4])) + colType := types.OidToType[oid.Oid(colOid)] + typs = append(typs, colType) + curByte = curByte + 4 + colLength := int32(binary.BigEndian.Uint32(b[curByte:curByte+4])) + curByte = curByte + 4 + if colLength == -1 { + datums = append(datums, tree.DNull) + } else { + colDatum, _:= DecodeDatum(evalCtx, colType, code, b[curByte:curByte+colLength]) + curByte = curByte + colLength + datums = append(datums, colDatum) + } + curCol += 1 + } + + tupleTyps := types.MakeTuple(typs) + return tree.NewDTuple(tupleTyps, datums...), nil case oid.T_bool: if len(b) > 0 { switch b[0] { (END)This is working properly, except for collated strings, where decoding is expected to fail
cockroach/pkg/sql/pgwire/encoding_test.go
Lines 243 to 246 in 7121f6d
The particular test case where it failed was:
SQL statement was: (1::char(2) COLLATE "en_US",null) --- FAIL: TestEncodings/decode (0.06s) panic: unsupported comparison: string to collatedstring{en_US} [recovered] panic: unsupported comparison: string to collatedstring{en_US}How should I solve this? Should I write another function to test only Tuples where there are no collated strings?
Couple of cases where its passed:
SQL statement was: (1::int8,2::int8,3::int8,4::int8,null) SQL statement was: ('test with spaces'::BYTEA,null) SQL statement was: ('test with spaces'::TEXT,null) SQL statement was: ('name'::NAME,null) SQL statement was: ('false'::JSONB,null) SQL statement was: ('{"a": []}'::JSONB,null) SQL statement was: (1::int4,null) SQL statement was: (1::int2,null) SQL statement was: (1::char(2),null) SQL statement was: (1::char(1),null) SQL statement was: (1::varchar(4),null) SQL statement was: (1::text,null)Also tested for a nested tuple and it passed. (@rafiss example:)
{ "SQL": "(1, 2, ('hi', 3))", "Oid": 2249, "Text": "(1,2,\"(hi,3)\")", "TextAsBinary": [40, 49, 44, 50, 44, 34, 40, 104, 105, 44, 51, 41, 34, 41], "Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 8, 201, 0, 0, 0, 30, 0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 2, 104, 105, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 3] },Although, I had to change the Binary format as the encoding as the one Rafi got was incorrect - the one generated by generate-binary/main.go (it identified 'hi' as
T_unkown(705), but it should beT_text(25). The above encoding I generated from using our code -writeBinaryDatum. Could it be that the generate-binary/main.go is not generating properly for nested tuples for some reason? (I have not tried it myself, wanted to get the decoding code working properly first).Also, just wanted to mention that decoding tuples from text as binary is not possible as there is no type information for individual elements of the tuple.
Oh I just realized Rafi must've put that as just an example. Most probably the generator is working fine, I will look into it later, sorry for bringing it up.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
let's not fix the mess with collated strings in this PR -- so I think you should update the test to skip the decode test if it is a tuple that contains collated strings. maybe you could do something like this in the decode test
t.Run("decode", func(t *testing.T) {
for _, tc := range tests {
switch tc.Datum.(type) {
case *tree.DFloat:
// Skip floats because postgres rounds them different than Go.
continue
case *tree.DTuple:
hasCollatedString := false
for _, elem := range tc.Datum.ResolvedType().TupleContents() {
if elem.Family() == types.CollatedStringFamily {
hasCollatedString = true
}
}
if hasCollatedString {
// Unsupported.
continue
}
Although, I had to change the Binary format as the encoding as the one Rafi got was incorrect - the one generated by generate-binary/main.go (it identified 'hi' as T_unkown(705), but it should be T_text(25). The above encoding I generated from using our code - writeBinaryDatum. Could it be that the generate-binary/main.go is not generating properly for nested tuples for some reason? (I have not tried it myself, wanted to get the decoding code working properly first).
No worries at all about asking! The way generate-binary/main.go works is by running against a Postgres server and capturing how Postgres encodes the data. That's interesting that Postgres doesn't have the correct type, but I'd also consider that a separate concern outside of what this PR should do. Let's go with this diff instead, so we just explicitly add the type
diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go
index 994aa228e0..ed28fb6549 100644
--- a/pkg/cmd/generate-binary/main.go
+++ b/pkg/cmd/generate-binary/main.go
@@ -544,5 +544,7 @@ var inputs = map[string][]string{
`1::char(1) COLLATE "en_US"`,
`1::varchar(4) COLLATE "en_US"`,
`1::text COLLATE "en_US"`,
+ `1::int8,(2::int8,3::int8)`,
+ `1::int8,('hi'::TEXT,3::int2)`,
},
}
diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json
index cd8cabac6c..9c3f549c69 100644
--- a/pkg/sql/pgwire/testdata/encodings.json
+++ b/pkg/sql/pgwire/testdata/encodings.json
@@ -2008,6 +2008,20 @@
"TextAsBinary": [40, 49, 44, 41],
"Binary": [0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 1, 49, 0, 0, 2, 193, 255, 255, 255, 255]
},
+ {
+ "SQL": "(1::int8,(2::int8,3::int8),null)",
+ "Oid": 2249,
+ "Text": "(1,\"(2,3)\",)",
+ "TextAsBinary": [40, 49, 44, 34, 40, 50, 44, 51, 41, 34, 44, 41],
+ "Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 8, 201, 0, 0, 0, 36, 0, 0, 0, 2, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 2, 193, 255, 255, 255, 255]
+ },
+ {
+ "SQL": "(1::int8,('hi'::TEXT,3::int2),null)",
+ "Oid": 2249,
+ "Text": "(1,\"(hi,3)\",)",
+ "TextAsBinary": [40, 49, 44, 34, 40, 104, 105, 44, 51, 41, 34, 44, 41],
+ "Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 8, 201, 0, 0, 0, 24, 0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 2, 104, 105, 0, 0, 0, 21, 0, 0, 0, 2, 0, 3, 0, 0, 2, 193, 255, 255, 255, 255]
+ },
{
"SQL": "B''",
"Oid": 1560,
Hope all this helps! I think with these changes your tests should be good. Thanks for your work.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
let's not fix the mess with collated strings in this PR -- so I think you should update the test to skip the decode test if it is a tuple that contains collated strings. maybe you could do something like this in the decode test
t.Run("decode", func(t *testing.T) { for _, tc := range tests { switch tc.Datum.(type) { case *tree.DFloat: // Skip floats because postgres rounds them different than Go. continue case *tree.DTuple: hasCollatedString := false for _, elem := range tc.Datum.ResolvedType().TupleContents() { if elem.Family() == types.CollatedStringFamily { hasCollatedString = true } } if hasCollatedString { // Unsupported. continue }Although, I had to change the Binary format as the encoding as the one Rafi got was incorrect - the one generated by generate-binary/main.go (it identified 'hi' as T_unkown(705), but it should be T_text(25). The above encoding I generated from using our code - writeBinaryDatum. Could it be that the generate-binary/main.go is not generating properly for nested tuples for some reason? (I have not tried it myself, wanted to get the decoding code working properly first).
No worries at all about asking! The way generate-binary/main.go works is by running against a Postgres server and capturing how Postgres encodes the data. That's interesting that Postgres doesn't have the correct type, but I'd also consider that a separate concern outside of what this PR should do. Let's go with this diff instead, so we just explicitly add the type
diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go index 994aa228e0..ed28fb6549 100644 --- a/pkg/cmd/generate-binary/main.go +++ b/pkg/cmd/generate-binary/main.go @@ -544,5 +544,7 @@ var inputs = map[string][]string{ `1::char(1) COLLATE "en_US"`, `1::varchar(4) COLLATE "en_US"`, `1::text COLLATE "en_US"`, + `1::int8,(2::int8,3::int8)`, + `1::int8,('hi'::TEXT,3::int2)`, }, } diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json index cd8cabac6c..9c3f549c69 100644 --- a/pkg/sql/pgwire/testdata/encodings.json +++ b/pkg/sql/pgwire/testdata/encodings.json @@ -2008,6 +2008,20 @@ "TextAsBinary": [40, 49, 44, 41], "Binary": [0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 1, 49, 0, 0, 2, 193, 255, 255, 255, 255] }, + { + "SQL": "(1::int8,(2::int8,3::int8),null)", + "Oid": 2249, + "Text": "(1,\"(2,3)\",)", + "TextAsBinary": [40, 49, 44, 34, 40, 50, 44, 51, 41, 34, 44, 41], + "Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 8, 201, 0, 0, 0, 36, 0, 0, 0, 2, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 2, 193, 255, 255, 255, 255] + }, + { + "SQL": "(1::int8,('hi'::TEXT,3::int2),null)", + "Oid": 2249, + "Text": "(1,\"(hi,3)\",)", + "TextAsBinary": [40, 49, 44, 34, 40, 104, 105, 44, 51, 41, 34, 44, 41], + "Binary": [0, 0, 0, 3, 0, 0, 0, 20, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 8, 201, 0, 0, 0, 24, 0, 0, 0, 2, 0, 0, 0, 25, 0, 0, 0, 2, 104, 105, 0, 0, 0, 21, 0, 0, 0, 2, 0, 3, 0, 0, 2, 193, 255, 255, 255, 255] + }, { "SQL": "B''", "Oid": 1560,Hope all this helps! I think with these changes your tests should be good. Thanks for your work.
Yes that helps a lot Rafi, thanks! Will clean it up a little and update the PR :)
biradarganesh25
force-pushed
the
reduce_allocation
branch
from
d777e8e
to
3b40481
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/pgwire/types.go, line 156 at r3 (raw file):
Previously, biradarganesh25 (Ganesh Biradar) wrote…
Yes that helps a lot Rafi, thanks! Will clean it up a little and update the PR :)
Hey @knz , @rafiss , @andreimatei I've updated the PR, please review when you get some time :) It has changes for a new function to decode tuple, two new nested tuple test cases and a change when encoding tuples and arrays by reserving 4 bytes for the length in the beginning. Thanks!
There was a problem hiding this comment.
thank! nice work, i just have style comments
Reviewable status:
pkg/sql/pgwire/types.go, line 718 at r5 (raw file):
lengthToWrite := b.Len() - (initialLen + 4) b.putInt32AtIndex(initialLen /* index to write at */, int32(lengthToWrite))
nit: please remove the writeLengthPrefixedByteBuffer method. it is unused now
pkg/sql/pgwire/pgwirebase/encoding.go, line 955 at r5 (raw file):
} func decodeTuple(
name this function decodeBinaryTuple
pkg/sql/pgwire/pgwirebase/encoding.go, line 956 at r5 (raw file):
func decodeTuple( evalCtx *tree.EvalContext, t *types.T, code FormatCode, b []byte,
remove the code FormatCode parameter
pkg/sql/pgwire/pgwirebase/encoding.go, line 959 at r5 (raw file):
) (tree.Datum, error) { if len(b) < 4 { return nil, pgerror.Newf(pgcode.Syntax, "Tuple needs to have atleast 4 bytes to indicate the number of columns.")
nit: make the error pgerror.Newf(pgcode.Syntax, "tuple requires a 4 byte header for binary format")
(i adjusted the punctuation and capitalization to match the other error messages from this code)
pkg/sql/pgwire/pgwirebase/encoding.go, line 965 at r5 (raw file):
numberOfCols := int32(binary.BigEndian.Uint32(b[0:4])) typs := make([]*types.T, 0, numberOfCols) datums := make(tree.Datums, 0, numberOfCols)
nit: to me it seems more readable to do
typs := make([]*types.T, numberOfCols)
datums := make(tree.Datums, numberOfCols)
then set the elements at each index in the loop below
pkg/sql/pgwire/pgwirebase/encoding.go, line 969 at r5 (raw file):
curCol := int32(0) for curCol < numberOfCols {
nit: i think "column" is misleading. can you rename
curCol -> curIdx
numberOfCols -> numberOfElements
colOid -> elementOID
colType -> elementType
colLength -> elementLength
pkg/sql/pgwire/pgwirebase/encoding.go, line 972 at r5 (raw file):
if totalLength < curByte+4 { return nil, pgerror.Newf(pgcode.Syntax, "Tuple does not have enough bytes for column oid.")
nit: make it pgerror.Newf(pgcode.Syntax, "tuple requires 4 bytes for each element OID for binary format")
pkg/sql/pgwire/pgwirebase/encoding.go, line 981 at r5 (raw file):
if totalLength < curByte+4 { return nil, pgerror.Newf(pgcode.Syntax, "Tuple does not have enough bytes for column length.")
nit: pgerror.Newf(pgcode.Syntax, "tuple requires 4 bytes for the size of each element for binary format")
pkg/sql/pgwire/pgwirebase/encoding.go, line 989 at r5 (raw file):
if colLength == -1 { datums = append(datums, tree.DNull) } else {
nit: add a check for if totalLength < curByte+colLength and return
pgerror.Newf(pgcode.Syntax, "tuple requires %d bytes for element %d for binary format", colLength, curCol)
pkg/sql/pgwire/pgwirebase/encoding.go, line 990 at r5 (raw file):
datums = append(datums, tree.DNull) } else { colDatum, err := DecodeDatum(evalCtx, colType, code, b[curByte:curByte+colLength])
nit: this must always be FormatBinaryhere, so you can pass in that constant instead of thecode` variable
|
Thank you for updating your pull request. Before a member of our team reviews your PR, I have some potential action items for you:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
There was a problem hiding this comment.
@rafiss I've made the changes, please review. I've added a temp commit on top of the actual one as it lets me keep track of style changes and stuff and I did not want to force push for every change. Once you guys tell me that the code looks good, I will squash all the commits into the original commit with the proper commit message and force push that in the end. I hope this is fine.
Reviewable status:
pkg/sql/pgwire/types.go, line 718 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: please remove the
writeLengthPrefixedByteBuffermethod. it is unused now
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 955 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
name this function
decodeBinaryTuple
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 956 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
remove the
code FormatCodeparameter
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 959 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: make the error
pgerror.Newf(pgcode.Syntax, "tuple requires a 4 byte header for binary format")(i adjusted the punctuation and capitalization to match the other error messages from this code)
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 965 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: to me it seems more readable to do
typs := make([]*types.T, numberOfCols) datums := make(tree.Datums, numberOfCols)then set the elements at each index in the loop below
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 969 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: i think "column" is misleading. can you rename
curCol->curIdx
numberOfCols->numberOfElements
colOid->elementOID
colType->elementType
colLength->elementLength
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 972 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: make it
pgerror.Newf(pgcode.Syntax, "tuple requires 4 bytes for each element OID for binary format")
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 981 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit:
pgerror.Newf(pgcode.Syntax, "tuple requires 4 bytes for the size of each element for binary format")
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 989 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: add a check for
if totalLength < curByte+colLengthand return
pgerror.Newf(pgcode.Syntax, "tuple requires %d bytes for element %d for binary format", colLength, curCol)
Done.
pkg/sql/pgwire/pgwirebase/encoding.go, line 990 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit:
this must always beFormatBinaryhere, so you can pass in that constant instead of thecode` variable
Done.
There was a problem hiding this comment.
this looks good to me! thanks for working on this. we can merge after the commits are squashed
btw, in case you didn't know, you can use the "Reviewable" tool and change the the view to "combine commits for review" and you can view your changes in the way you want
…s/tuples A new "writeBuffer" was being allocated every time when serializing an array/tuple. This is very heavy and unecessary. To address this, this patch reserves 4 bytes before writing data to the buffer. After writing the data, the total length of the written data is calculated which is then written to the 4 bytes reserved. This is more efficient than allocating a new temporary "writebuffer" every time. The patch also adds functionality for decoding nested tuples. A new test has been added to test nested tuples. Tuple decoding tests are not skipped anymore. Release note (performance improvement): The performance of queries returning many arrays has been improved.
biradarganesh25
force-pushed
the
reduce_allocation
branch
from
9f2f9de
to
db1657f
Compare
There was a problem hiding this comment.
Done! And thanks for the tip about reviewable, that helps :) Also - if you are aware of any open issues that are related to the same area and that a relative beginner like me can work on, please put them here :) I would go search for the issues, but I was hoping working on the right issues would help me become familiar with the codebase faster. I'd go pretty slow as I will be starting my masters, but I'd like to work on it in my free time.
Reviewable status:
rafiss
changed the title
|
thanks again! i don't know any other issues directly related to this one, but as you say, please do look out for issues labeled with bors r+ |
|
Build succeeded: |
A new "writeBuffer" was being allocated every time when serializing
an array/tuple. This is very heavy and unecessary.
To address this, this patch creates a temporary buffer in the
existing "writeBuffer" struct which is used specially for serializing
array/tuple. This is more efficient than allocating a new
temporary "writebuffer" every time.
Release note (performance improvement): A special temporary buffer was
created to be used only for serializing arrays/tuples. This provided
better performance. The increase in performance was seen by running the
BenchmarkEncodings benchmark for arrays and tuples.
Fixes: #21711