Introduction

Vindexes allow Vindex-specific parameters to be passed via the params field. For example:

{
  "type": "lookup",
  "params": {
    "autocommit": "true",
    "read_lock": "none"
  }
}

Currently it is the responsibility of each Vindex to validate its own params. Some Vindexes do strict validation of params, while other Vindexes do very little param validation. All Vindexes happily accept unknown params.

Problem

For the most part, there aren’t any problems with this. If a user forgets to pass in a required param, the Vindex will complain or fail, and the user will know about it.

However, if the user passes in an optional param, but makes a typo in the param name, most Vindexes will happily accept the typo param, and do nothing with it.

In some cases that silent ignore can be a problem. For example, the lookup Vindex family supports a number of performance-impacting parameters, such as read_lock. If a user tries to set raed_lock (notice typo) to none, expecting that this will reduce lock contention, they will be in for an unpleasant surprise during the next traffic spike.

Proposal

This RFC proposes updating Vitess to warn users when they pass in unknown parameters to the Vindexes. The warnings can be surfaced to users via logs and metrics.

Importantly, warnings will be backwards-compatible, and it will be the responsibility of users to monitor logs and metrics for these warnings, and take additional corrective actions.

Optionally, a user can opt in to stricter param validation via a flag such as --vschema-strict, which will escalate these warnings to errors. A user might choose to opt in to --vschema-strict in development and staging environments, but leave the default behavior on in production.

Options

Some ways this could be implemented.

Have vindexes optionally report warnings

Introduce a ParamValidating interface like:

+ type ParamValidating interface {
+   UnknownParams() []string
+ }

Then update the logic of individual Vindexes to return non-empty UnknownParams() when any parameters passed to the Vindex during Vindex creation was unknown.

Update various parts of Vitess codebase to look for unknown params in Vindexes, and emit logs and metrics. For example:

• Update vtgate.(*VschemaStats) to export warnings in its HTTP output.
• Log warnings and increment a vtgate_vschema_vindex_unknown_param_count stat from vtgate.(*VSchemaManager).
• Update ApplyVSchema to return warnings in response, while continuing to accept the input if there aren’t any errors.

Pros

• Is not a breaking change.
• Does not require any future deprecations or removals.
• Does not add any complexity to upgrades.
• Does not require users to change their VSchemas.
• Gives users soft warnings (metrics, logs) by default with the option of opting in to hard errors.

Cons

• Requires more code changes compared to the other options.
• Potentially difficult to maintain: any time we add or remove a supported param from a Vindex, we need to update both the validation code as well as the code that actually uses that param.

Add per-Vindex VSchema params

Currently all Vindexes that accept parameters do so via the params field, which is not really strongly typed.

We can introduce stronger typing by having each Vindex which declares params register strongly typed params in the VSchema protobuf definition.

How this might look in the protobuf definition.

// Vindex is the vindex info for a Keyspace.
message Vindex {
+  message LookupVindexParams {
+    enum ReadLock {
+      READ_LOCK_EXCLUSIVE = 0;
+      READ_LOCK_SHARED = 1;
+      READ_LOCK_NONE = 2;
+    }
+  } 
+
+   bool autocommit = 1;
+   ReadLock read_lock = 2;
+ }
  // The type must match one of the predefined
  // (or plugged in) vindex names.
  string type = 1;
  // params is a map of attribute value pairs
  // that must be defined as required by the
  // vindex constructors. The values can only
  // be strings.
  map<string, string> params = 2;
  // A lookup vindex can have an owner table defined.
  // If so, rows in the lookup table are created or
  // deleted in sync with corresponding rows in the
  // owner table.
  string owner = 3;

+  // Params specific to "lookup" Vindex. 
+  LookupVindexParams lookup_params;
}

A user could opt in to stronger Vindex param validation by specifying lookup_params in the VSchema:

{
  "type": "lookup",
  "lookup_params": {
     "autocommit": true,
     "read_lock": "none" 
  }
}

Notice how in the VSchema above autocommit is a boolean, instead of a string with the word "true".

Initially we would support both params as well as lookup_params. lookup_params would take precedence when present, and fall back to params when null.

We could choose to support both params as well as {type}_params indefinitely, or else eventually deprecate and remove params. The former would give users to option of choosing between loose param validation or strict param validation by using either params or {type}_params.

Pros

• Does not require a lot of code relative to the other options. Protobuf parsing will do all the heavy lifting of parsing and validating params.
• Allows users to opt into strict validation at their own pace, one Vindex at a time.

Cons

• If we eventually deprecate and remove params in favor of {type}_params, that would be a big change to force on the community.
• If we support both params and {type}_params indefinitely, that is a burden for maintainers to support.
• Is probably a non-starter for out-of-tree Vindex plugins; we would need to support the loosely typed params field indefinitely for those out-of-tree plugins.
• Could make upgrades more difficult to execute. For example, if we ever removed the read_lock field from the protobuf definition, in order to upgrade users would need to first remove this field from their VSchema, and then upgrade their VT* components.

Recommendation

I really like that option 2 would add strong typing for Vindex params, but I think the main challenge is that out-of-tree Vindex plugins won’t be able to take advantage of that.

I recommend option 1 because:

• It does not require users to change their VSchemas in order to opt in to soft warnings.
• Users can choose to ignore warnings, monitor them, or treat them as errors via --vschema-strict.
• It does not create any breaking changes in code.
• It gives out-of-tree Vindex plugins the ability to issue their own warnings.

Decision

I like the RAPID model for decision making.

Recommend: @maxenglander Agree: @deepthi @systay Perform: @maxenglander Input: community Decide: @harshit-gangal: “I think Option 1 is good as it does not change user behaviour and gives the option to log warnings to indicate the issue with the vindex setup.”

Tasks

• Create and implement vindexes.ParamValidating interface, via #12951 .
• Expose unknown params via logs and metrics.
  • Expose VTGate metric, via #13322.
  • Report warnings from vtctl ApplyVSchema CLI command, via #13322.
  • Report warnings from ApplyVSchemaResponse gRPC response.
• Opt into strict validation via flag.