Merge #143673 #144247

143673: kvserver: tolerate missing store in raft transport r=tbg a=tbg

In a multi-store cluster, if a node (say n1) is restarted with fewer stores
than initially, it will take some time for replicas to be moved off of it.

Previously, while in this state, the raft transport from other nodes to n1
would frequently be reconnected. Concretely, this would happen every time a
leader on n2 would try to reach a follower on a now-missing store on n1.
n2 would respond with a StoreNotFoundError, and the original sender would
tear down the raft transport stream to n1 and reconnect, dropping messages
directed at other valid stores on n1 in the process.

We now no longer emit StoreNotFoundError from n1, and in mixed clusters such
messages are now handled as no-ops, leaving the raft transport intact and thus
preventing interruption to raft message flows to stores still present on n1.

Epic: CRDB-49133
Release note: None


144247: jsonpath: validate array indices are within int32 range r=normanchenn a=normanchenn

This commit adds validation to ensure JSONPath array indices are within
the int32 range, matching Postgres' behaviour. Now, we return an error
for indices outside the int32 range.

Epic: None
Release note: None

Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Norman Chen <[email protected]>

Author: 3 people

pkg/kv/kvserver/raft_transport.go

@@ -428,6 +428,7 @@ func (t *RaftTransport) getOutgoingMessageHandler(
 func (t *RaftTransport) handleRaftRequest(
 	ctx context.Context, req *kvserverpb.RaftMessageRequest, respStream RaftMessageResponseStream,
 ) *kvpb.Error {
+	isV1 := log.V(1)
 	for i := range req.AdmittedRaftLogEntries {
 		// Process any flow tokens that were returned over the RaftTransport. Do
 		// this first thing, before these requests enter the receive queues
@@ -444,7 +445,7 @@ func (t *RaftTransport) handleRaftRequest(
 			)
 		}
 
-		if log.V(1) {
+		if isV1 {
 			log.Infof(ctx, "informed of below-raft %s", admittedEntries)
 		}
 	}
@@ -457,9 +458,16 @@ func (t *RaftTransport) handleRaftRequest(
 
 	incomingMessageHandler, ok := t.getIncomingRaftMessageHandler(req.ToReplica.StoreID)
 	if !ok {
-		log.Warningf(ctx, "unable to accept Raft message from %+v: no handler registered for %+v",
-			req.FromReplica, req.ToReplica)
-		return kvpb.NewError(kvpb.NewStoreNotFoundError(req.ToReplica.StoreID))
+		if isV1 {
+			log.Warningf(ctx, "unable to accept Raft message from %+v: no handler registered for %+v",
+				req.FromReplica, req.ToReplica)
+		}
+		// We don't return an error to the client. If this node restarted with fewer
+		// stores than it had before (think: hardware failure), then it is expected
+		// for remote ranges to think there should be a replica on this store. We
+		// could still send an error back here, but then it would have to be dropped
+		// at the other node - not a good use of bandwidth.
+		return nil
 	}
 
 	return incomingMessageHandler.HandleRaftRequest(ctx, req, respStream)

pkg/kv/kvserver/store_raft.go

@@ -662,7 +662,13 @@ func (s *Store) HandleRaftResponse(
 			case *kvpb.StoreNotFoundError:
 				log.Warningf(ctx, "raft error: node %d claims to not contain store %d for replica %s: %s",
 					resp.FromReplica.NodeID, resp.FromReplica.StoreID, resp.FromReplica, val)
-				return val.GetDetail() // close Raft connection
+				// This error is expected if the remote node restarted with fewer stores
+				// (before rebalancing off that now dead store is complete).
+				//
+				// Fall through intentionally.
+				//
+				// NB: as of v25.2, receivers no longer return this error in this situation
+				// and eventually, this case can be removed.
 			default:
 				log.Warningf(ctx, "got error from r%d, replica %s: %s",
 					resp.RangeID, resp.FromReplica, val)

pkg/sql/logictest/testdata/logic_test/jsonb_path_query

@@ -1406,3 +1406,25 @@ query T
 SELECT jsonb_path_query('{"a": {"b": [{"c": 1}, {"c": 2}]}}', '($.a.b[1]).c');
 ----
 2
+
+statement error pgcode 22033 pq: jsonpath array subscript is out of integer range
+SELECT jsonb_path_query('[1]', 'lax $[10000000000000000]');
+
+statement error pgcode 22033 pq: jsonpath array subscript is out of integer range
+SELECT jsonb_path_query('[1]', 'lax $[-10000000000000000]');
+
+# MaxInt32
+query empty
+SELECT jsonb_path_query('[1]', '$[2147483647]');
+
+# MaxInt32 + 1
+statement error pgcode 22033 pq: jsonpath array subscript is out of integer range
+SELECT jsonb_path_query('[1]', '$[2147483648]');
+
+# MinInt32
+query empty
+SELECT jsonb_path_query('[1]', '$[-2147483648]');
+
+# MinInt32 - 1
+statement error pgcode 22033 pq: jsonpath array subscript is out of integer range
+SELECT jsonb_path_query('[1]', '$[-2147483649]');

pkg/util/jsonpath/eval/array.go

@@ -6,6 +6,8 @@
 package eval
 
 import (
+	"math"
+
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/json"
@@ -18,6 +20,7 @@ var (
 	errIndexOnNonArray        = pgerror.Newf(pgcode.SQLJSONArrayNotFound, "jsonpath array accessor can only be applied to an array")
 	errIndexOutOfBounds       = pgerror.Newf(pgcode.InvalidSQLJSONSubscript, "jsonpath array subscript is out of bounds")
 	errIndexNotSingleNumValue = pgerror.Newf(pgcode.InvalidSQLJSONSubscript, "jsonpath array subscript is not a single numeric value")
+	errInvalidSubscript       = pgerror.Newf(pgcode.InvalidSQLJSONSubscript, "jsonpath array subscript is out of integer range")
 )
 
 func (ctx *jsonpathCtx) evalArrayWildcard(jsonValue json.JSON) ([]json.JSON, error) {
@@ -110,23 +113,25 @@ func (ctx *jsonpathCtx) resolveArrayIndex(
 	if len(evalResults) != 1 || evalResults[0].Type() != json.NumberJSONType {
 		return -1, errIndexNotSingleNumValue
 	}
-	// TODO(normanchenn): Postgres returns an error if the index is outside int32
-	// range. (ex. `select jsonb_path_query('[1]', 'lax $[10000000000000000]');
 	i, err := asInt(evalResults[0])
 	if err != nil {
-		return -1, errIndexNotSingleNumValue
+		return -1, err
 	}
 	return i, nil
 }
 
 func asInt(j json.JSON) (int, error) {
 	d, ok := j.AsDecimal()
 	if !ok {
-		return 0, errInternal
+		return 0, errIndexNotSingleNumValue
 	}
 	i64, err := d.Int64()
 	if err != nil {
-		return 0, err
+		return 0, errIndexNotSingleNumValue
+	}
+	// Postgres returns an error if the index is outside int32 range.
+	if i64 < math.MinInt32 || i64 > math.MaxInt32 {
+		return 0, errInvalidSubscript
 	}
 	return int(i64), nil
 }

pkg/util/jsonpath/eval/eval.go

@@ -18,7 +18,6 @@ import (
 
 var (
 	errUnimplemented         = unimplemented.NewWithIssue(22513, "unimplemented")
-	errInternal              = errors.New("internal error")
 	errSingleBooleanRequired = pgerror.Newf(pgcode.SingletonSQLJSONItemRequired, "single boolean result is expected")
 )