From d2b2b0ac97947cced7fbadeff254c88fe23451e1 Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Wed, 7 Jun 2023 11:02:33 +0300 Subject: [PATCH 1/6] test: fix flaky decimal.TestSelect We forgot to increase the timeout [1] when we fixed tests for macos. 1. https://github.com/tarantool/go-tarantool/commit/521c0c34804e93cd2648c3b5c27bc7194d3870d0 (cherry picked from 6cddcd7) --- CHANGELOG.md | 2 ++ decimal/decimal_test.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0332f376f..6102e1486 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. ### Fixed +- Flaky decimal/TestSelect (#300) + ## [1.12.0] - 2023-06-07 The release introduces the ability to gracefully close Connection diff --git a/decimal/decimal_test.go b/decimal/decimal_test.go index b4e95ebe1..240edf913 100644 --- a/decimal/decimal_test.go +++ b/decimal/decimal_test.go @@ -19,7 +19,7 @@ var isDecimalSupported = false var server = "127.0.0.1:3013" var opts = Opts{ - Timeout: 500 * time.Millisecond, + Timeout: 5 * time.Second, User: "test", Pass: "test", } From d37e9b5614bd1e9be5502decfcc732224ff558ba Mon Sep 17 00:00:00 2001 From: Vladimir Davydov Date: Tue, 27 Jun 2023 16:40:52 +0300 Subject: [PATCH 2/6] queue: encode UUID argument of identify() as string instead of binary The identify() function expects the UUID argument to be a plain string while the go connector encodes it in MsgPack as a binary blob (MP_BIN). This works fine for now because Tarantool stores MP_BIN data in a string when decoded to Lua but this behavior is going to change soon: we're planning to introduce the new Lua type for binary data and update the MsgPack decoder to store MP_BIN data in a varbianry object instead of a plain string. Let's prepare for that by converting the UUID data to a string before encoding. Needed for tarantool/tarantool#1629 (cherry picked from c2498be) --- CHANGELOG.md | 4 ++++ queue/queue.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6102e1486..095c316bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. ### Changed +- Change encoding of the queue.Identify() UUID argument from binary blob to + plain string. Needed for upgrade to Tarantool 3.0, where a binary blob is + decoded to a varbinary object (#313). + ### Fixed - Flaky decimal/TestSelect (#300) diff --git a/queue/queue.go b/queue/queue.go index 8d161b033..f466e16af 100644 --- a/queue/queue.go +++ b/queue/queue.go @@ -232,7 +232,7 @@ func (q *queue) Identify(u *uuid.UUID) (uuid.UUID, error) { if bytes, err := u.MarshalBinary(); err != nil { return uuid.UUID{}, err } else { - args = []interface{}{bytes} + args = []interface{}{string(bytes)} } } From bf27cc93be2c1bf44e4d0b755ea1db0666bc0b47 Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Tue, 27 Jun 2023 16:22:52 +0300 Subject: [PATCH 3/6] pool: fix race condition at GetNextConnection() The `r.current` value can be changed by concurrent threads because the change happens under read-lock. We could use the atomic counter for a current connection number to avoid the race condition. Closes #309 (cherry picked from dbfaab5) --- CHANGELOG.md | 1 + connection_pool/connection_pool_test.go | 29 +++++++++++++++++++++++++ connection_pool/round_robin.go | 14 ++++++------ 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 095c316bf..924b0c812 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. ### Fixed - Flaky decimal/TestSelect (#300) +- Race condition at roundRobinStrategy.GetNextConnection() (#309) ## [1.12.0] - 2023-06-07 diff --git a/connection_pool/connection_pool_test.go b/connection_pool/connection_pool_test.go index dde6815d9..cccf9d8d2 100644 --- a/connection_pool/connection_pool_test.go +++ b/connection_pool/connection_pool_test.go @@ -2065,6 +2065,35 @@ func TestDo(t *testing.T) { require.NotNilf(t, resp, "response is nil after Ping") } +func TestDo_concurrent(t *testing.T) { + roles := []bool{true, true, false, true, false} + + err := test_helpers.SetClusterRO(servers, connOpts, roles) + require.Nilf(t, err, "fail to set roles for cluster") + + connPool, err := connection_pool.Connect(servers, connOpts) + require.Nilf(t, err, "failed to connect") + require.NotNilf(t, connPool, "conn is nil after Connect") + + defer connPool.Close() + + req := tarantool.NewPingRequest() + const concurrency = 100 + var wg sync.WaitGroup + wg.Add(concurrency) + + for i := 0; i < concurrency; i++ { + go func() { + defer wg.Done() + + _, err := connPool.Do(req, connection_pool.ANY).Get() + assert.Nil(t, err) + }() + } + + wg.Wait() +} + func TestNewPrepared(t *testing.T) { test_helpers.SkipIfSQLUnsupported(t) diff --git a/connection_pool/round_robin.go b/connection_pool/round_robin.go index a7fb73e18..eedeaca7c 100644 --- a/connection_pool/round_robin.go +++ b/connection_pool/round_robin.go @@ -2,6 +2,7 @@ package connection_pool import ( "sync" + "sync/atomic" "github.com/tarantool/go-tarantool" ) @@ -10,8 +11,8 @@ type RoundRobinStrategy struct { conns []*tarantool.Connection indexByAddr map[string]uint mutex sync.RWMutex - size uint - current uint + size uint64 + current uint64 } func NewEmptyRoundRobin(size int) *RoundRobinStrategy { @@ -98,13 +99,12 @@ func (r *RoundRobinStrategy) AddConn(addr string, conn *tarantool.Connection) { r.conns[idx] = conn } else { r.conns = append(r.conns, conn) - r.indexByAddr[addr] = r.size + r.indexByAddr[addr] = uint(r.size) r.size += 1 } } -func (r *RoundRobinStrategy) nextIndex() uint { - ret := r.current % r.size - r.current++ - return ret +func (r *RoundRobinStrategy) nextIndex() uint64 { + next := atomic.AddUint64(&r.current, 1) + return (next - 1) % r.size } From 317d100ae6ca976c300711e5d7f983677890496c Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Wed, 28 Jun 2023 11:53:27 +0300 Subject: [PATCH 4/6] decimal: incorrect MP_DECIMAL decoding with scale < 0 The `scale` value in `MP_DECIMAL` may be negative [1]. We need to handle the case. 1. https://www.tarantool.io/en/doc/latest/dev_guide/internals/msgpack_extensions/#the-decimal-type (cherry picked from 3aeb8c2) --- CHANGELOG.md | 1 + decimal/bcd.go | 44 ++++++++++++++---------------------- decimal/decimal.go | 8 +++++-- decimal/decimal_test.go | 49 ++++++++++++++++++++++++++++++++++++++++- decimal/export_test.go | 2 +- decimal/fuzzing_test.go | 11 +++++---- 6 files changed, 80 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 924b0c812..b4a7bc821 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. - Flaky decimal/TestSelect (#300) - Race condition at roundRobinStrategy.GetNextConnection() (#309) +- Incorrect decoding of an MP_DECIMAL when the `scale` value is negative (#314) ## [1.12.0] - 2023-06-07 diff --git a/decimal/bcd.go b/decimal/bcd.go index 2b5a4b258..09045daa7 100644 --- a/decimal/bcd.go +++ b/decimal/bcd.go @@ -44,8 +44,11 @@ package decimal // * An implementation in C language https://github.com/tarantool/decNumber/blob/master/decPacked.c import ( + "bytes" "fmt" "strings" + + "github.com/vmihailenco/msgpack/v5" ) const ( @@ -183,13 +186,17 @@ func encodeStringToBCD(buf string) ([]byte, error) { // ended by a 4-bit sign nibble in the least significant four bits of the final // byte. The scale is used (negated) as the exponent of the decimal number. // Note that zeroes may have a sign and/or a scale. -func decodeStringFromBCD(bcdBuf []byte) (string, error) { - // Index of a byte with scale. - const scaleIdx = 0 - scale := int(bcdBuf[scaleIdx]) +func decodeStringFromBCD(bcdBuf []byte) (string, int, error) { + // Read scale. + buf := bytes.NewBuffer(bcdBuf) + dec := msgpack.NewDecoder(buf) + scale, err := dec.DecodeInt() + if err != nil { + return "", 0, fmt.Errorf("unable to decode the decimal scale: %w", err) + } - // Get a BCD buffer without scale. - bcdBuf = bcdBuf[scaleIdx+1:] + // Get the data without the scale. + bcdBuf = buf.Bytes() bufLen := len(bcdBuf) // Every nibble contains a digit, and the last low nibble contains a @@ -204,10 +211,6 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { // Reserve bytes for dot and sign. numLen := ndigits + 2 - // Reserve bytes for zeroes. - if scale >= ndigits { - numLen += scale - ndigits - } var bld strings.Builder bld.Grow(numLen) @@ -219,26 +222,10 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { bld.WriteByte('-') } - // Add missing zeroes to the left side when scale is bigger than a - // number of digits and a single missed zero to the right side when - // equal. - if scale > ndigits { - bld.WriteByte('0') - bld.WriteByte('.') - for diff := scale - ndigits; diff > 0; diff-- { - bld.WriteByte('0') - } - } else if scale == ndigits { - bld.WriteByte('0') - } - const MaxDigit = 0x09 // Builds a buffer with symbols of decimal number (digits, dot and sign). processNibble := func(nibble byte) { if nibble <= MaxDigit { - if ndigits == scale { - bld.WriteByte('.') - } bld.WriteByte(nibble + '0') ndigits-- } @@ -254,5 +241,8 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) { processNibble(lowNibble) } - return bld.String(), nil + if bld.Len() == 0 || isNegative[sign] && bld.Len() == 1 { + bld.WriteByte('0') + } + return bld.String(), -1 * scale, nil } diff --git a/decimal/decimal.go b/decimal/decimal.go index cda08c12e..90214be6d 100644 --- a/decimal/decimal.go +++ b/decimal/decimal.go @@ -86,15 +86,19 @@ func (decNum *Decimal) UnmarshalMsgpack(b []byte) error { // +--------+-------------------+------------+===============+ // | MP_EXT | length (optional) | MP_DECIMAL | PackedDecimal | // +--------+-------------------+------------+===============+ - digits, err := decodeStringFromBCD(b) + digits, exp, err := decodeStringFromBCD(b) if err != nil { return fmt.Errorf("msgpack: can't decode string from BCD buffer (%x): %w", b, err) } + dec, err := decimal.NewFromString(digits) - *decNum = *NewDecimal(dec) if err != nil { return fmt.Errorf("msgpack: can't encode string (%s) to a decimal number: %w", digits, err) } + if exp != 0 { + dec = dec.Shift(int32(exp)) + } + *decNum = *NewDecimal(dec) return nil } diff --git a/decimal/decimal_test.go b/decimal/decimal_test.go index 240edf913..a7297e96c 100644 --- a/decimal/decimal_test.go +++ b/decimal/decimal_test.go @@ -112,6 +112,18 @@ var correctnessSamples = []struct { {"-1234567891234567890.0987654321987654321", "c7150113012345678912345678900987654321987654321d", false}, } +var correctnessDecodeSamples = []struct { + numString string + mpBuf string + fixExt bool +}{ + {"1e2", "d501fe1c", true}, + {"1e33", "c70301d0df1c", false}, + {"1.1e31", "c70301e2011c", false}, + {"13e-2", "c7030102013c", false}, + {"-1e3", "d501fd1d", true}, +} + // There is a difference between encoding result from a raw string and from // decimal.Decimal. It's expected because decimal.Decimal simplifies decimals: // 0.00010000 -> 0.0001 @@ -384,17 +396,21 @@ func TestEncodeStringToBCD(t *testing.T) { func TestDecodeStringFromBCD(t *testing.T) { samples := append(correctnessSamples, rawSamples...) + samples = append(samples, correctnessDecodeSamples...) samples = append(samples, benchmarkSamples...) for _, testcase := range samples { t.Run(testcase.numString, func(t *testing.T) { b, _ := hex.DecodeString(testcase.mpBuf) bcdBuf := trimMPHeader(b, testcase.fixExt) - s, err := DecodeStringFromBCD(bcdBuf) + s, exp, err := DecodeStringFromBCD(bcdBuf) if err != nil { t.Fatalf("Failed to decode BCD '%x' to decimal: %s", bcdBuf, err) } decActual, err := decimal.NewFromString(s) + if exp != 0 { + decActual = decActual.Shift(int32(exp)) + } if err != nil { t.Fatalf("Failed to encode string ('%s') to decimal", s) } @@ -525,6 +541,37 @@ func TestSelect(t *testing.T) { tupleValueIsDecimal(t, resp.Data, number) } +func TestUnmarshal_from_decimal_new(t *testing.T) { + skipIfDecimalUnsupported(t) + + conn := test_helpers.ConnectWithValidation(t, server, opts) + defer conn.Close() + + samples := correctnessSamples + samples = append(samples, correctnessDecodeSamples...) + samples = append(samples, benchmarkSamples...) + for _, testcase := range samples { + str := testcase.numString + t.Run(str, func(t *testing.T) { + number, err := decimal.NewFromString(str) + if err != nil { + t.Fatalf("Failed to prepare test decimal: %s", err) + } + + call := NewEvalRequest("return require('decimal').new(...)"). + Args([]interface{}{str}) + resp, err := conn.Do(call).Get() + if err != nil { + t.Fatalf("Decimal create failed: %s", err) + } + if resp == nil { + t.Fatalf("Response is nil after Call") + } + tupleValueIsDecimal(t, []interface{}{resp.Data}, number) + }) + } +} + func assertInsert(t *testing.T, conn *Connection, numString string) { number, err := decimal.NewFromString(numString) if err != nil { diff --git a/decimal/export_test.go b/decimal/export_test.go index c43a812c6..2c8fda1c7 100644 --- a/decimal/export_test.go +++ b/decimal/export_test.go @@ -4,7 +4,7 @@ func EncodeStringToBCD(buf string) ([]byte, error) { return encodeStringToBCD(buf) } -func DecodeStringFromBCD(bcdBuf []byte) (string, error) { +func DecodeStringFromBCD(bcdBuf []byte) (string, int, error) { return decodeStringFromBCD(bcdBuf) } diff --git a/decimal/fuzzing_test.go b/decimal/fuzzing_test.go index c69a68719..97e1f1815 100644 --- a/decimal/fuzzing_test.go +++ b/decimal/fuzzing_test.go @@ -10,11 +10,14 @@ import ( . "github.com/tarantool/go-tarantool/decimal" ) -func strToDecimal(t *testing.T, buf string) decimal.Decimal { +func strToDecimal(t *testing.T, buf string, exp int) decimal.Decimal { decNum, err := decimal.NewFromString(buf) if err != nil { t.Fatal(err) } + if exp != 0 { + decNum = decNum.Shift(int32(exp)) + } return decNum } @@ -33,13 +36,13 @@ func FuzzEncodeDecodeBCD(f *testing.F) { if err != nil { t.Skip("Only correct requests are interesting: %w", err) } - var dec string - dec, err = DecodeStringFromBCD(bcdBuf) + + dec, exp, err := DecodeStringFromBCD(bcdBuf) if err != nil { t.Fatalf("Failed to decode encoded value ('%s')", orig) } - if !strToDecimal(t, dec).Equal(strToDecimal(t, orig)) { + if !strToDecimal(t, dec, exp).Equal(strToDecimal(t, orig, 0)) { t.Fatal("Decimal numbers are not equal") } }) From bd36edb57fced025c74f6e78354f13de24e89946 Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Mon, 31 Jul 2023 19:59:03 +0300 Subject: [PATCH 5/6] crud: fix options for SelectRequest The patch fixes a typo that made it impossible to setup SelectOpts.After, SelectOpts.BatchSize and SelectOpts.ForceMapCall. Part of #320 (cherry picked from f56fb90) --- CHANGELOG.md | 2 ++ crud/example_test.go | 38 ++++++++++++++++++++++++++++++++++++++ crud/select.go | 6 +++--- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4a7bc821..ae52394b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. - Flaky decimal/TestSelect (#300) - Race condition at roundRobinStrategy.GetNextConnection() (#309) - Incorrect decoding of an MP_DECIMAL when the `scale` value is negative (#314) +- Incorrect options (`after`, `batch_size` and `force_map_call`) setup for + crud.SelectRequest (#320) ## [1.12.0] - 2023-06-07 diff --git a/crud/example_test.go b/crud/example_test.go index 2b80212ca..b16fe0053 100644 --- a/crud/example_test.go +++ b/crud/example_test.go @@ -148,3 +148,41 @@ func ExampleResult_errorMany() { // Output: // Failed to execute request: CallError: } + +func ExampleSelectRequest_pagination() { + conn := exampleConnect() + + const ( + fromTuple = 5 + allTuples = 10 + ) + var tuple interface{} + for i := 0; i < allTuples; i++ { + req := crud.MakeReplaceRequest(exampleSpace). + Tuple([]interface{}{uint(3000 + i), nil, "bla"}) + ret := crud.Result{} + if err := conn.Do(req).GetTyped(&ret); err != nil { + fmt.Printf("Failed to initialize the example: %s\n", err) + return + } + if i == fromTuple { + tuple = ret.Rows.([]interface{})[0] + } + } + + req := crud.MakeSelectRequest(exampleSpace). + Opts(crud.SelectOpts{ + First: crud.MakeOptInt(2), + After: crud.MakeOptTuple(tuple), + }) + ret := crud.Result{} + if err := conn.Do(req).GetTyped(&ret); err != nil { + fmt.Printf("Failed to execute request: %s", err) + return + } + fmt.Println(ret.Metadata) + fmt.Println(ret.Rows) + // Output: + // [{id unsigned false} {bucket_id unsigned true} {name string false}] + // [[3006 32 bla] [3007 33 bla]] +} diff --git a/crud/select.go b/crud/select.go index 97048a365..4e33c0b03 100644 --- a/crud/select.go +++ b/crud/select.go @@ -61,9 +61,9 @@ func (opts SelectOpts) EncodeMsgpack(enc *encoder) error { values[6], exists[6] = opts.Balance.Get() values[7], exists[7] = opts.First.Get() values[8], exists[8] = opts.After.Get() - values[8], exists[8] = opts.BatchSize.Get() - values[8], exists[8] = opts.ForceMapCall.Get() - values[8], exists[8] = opts.Fullscan.Get() + values[9], exists[9] = opts.BatchSize.Get() + values[10], exists[10] = opts.ForceMapCall.Get() + values[11], exists[11] = opts.Fullscan.Get() return encodeOptions(enc, names[:], values[:], exists[:]) } From 040255299286b78cf84a76e99b918cdeab5421b0 Mon Sep 17 00:00:00 2001 From: Oleg Jukovec Date: Thu, 3 Aug 2023 13:06:16 +0300 Subject: [PATCH 6/6] Release 1.12.1 Overview The patch release imports fixes from the master branch. Breaking changes There are no breaking changes in the release. Bugfixes Flaky decimal/TestSelect (#300). Race condition at roundRobinStrategy.GetNextConnection() (#309). Incorrect decoding of an MP_DECIMAL when the `scale` value is negative (#314). Incorrect options (`after`, `batch_size` and `force_map_call`) setup for crud.SelectRequest (#320). Other Change encoding of the queue.Identify() UUID argument from binary blob to plain string. Needed for upgrade to Tarantool 3.0, where a binary blob is decoded to a varbinary object (#313). --- CHANGELOG.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae52394b5..3129d6170 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,19 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release. ### Changed +### Fixed + +## [1.12.1] - 2023-08-03 + +The patch release with fixes from the master branch. + +### Added + +### Changed + - Change encoding of the queue.Identify() UUID argument from binary blob to plain string. Needed for upgrade to Tarantool 3.0, where a binary blob is - decoded to a varbinary object (#313). + decoded to a varbinary object (#313) ### Fixed