Allow unowned lookup_unique vindex columns to be null

[arthurschreiber]

Description

We're running into an issue when inserting data into a table that uses a secondary, unowned lookup_unique VIndex.

When inserting data in such a table with a NULL value for the VIndex column, Vitess returns an error:

value must be supplied for column <column name>

We believe that stems from a bug in the code that processes unowned VIndexes for INSERT operations, as we don't see a reason why a value for these columns would be required.

Example

Let's say we have two tables, issues and pull_requests. An issues row can optionally be associated with a pull_requests row via the issues.pull_request_id column. Both issues and pull_requests are sharded by repository_id. An issue can only be associated to a pull request with the same repository_id.

SQL Schema for sharded keyspace

CREATE TABLE `pull_requests_id_keyspace_idx` (
  `id` bigint(20) unsigned NOT NULL,
  `keyspace_id` varbinary(10) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

CREATE TABLE `issues_id_keyspace_idx` (
  `id` bigint(20) unsigned NOT NULL,
  `keyspace_id` varbinary(10) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

CREATE TABLE `pull_requests` (
  `id` bigint(20) unsigned NOT NULL,
  `repository_id` bigint(20) unsigned NOT NULL,
  PRIMARY KEY (`id`),
  KEY `index_pull_requests_on_repository_id` (`repository_id`),
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

CREATE TABLE `issues` (
  `id` bigint(20) unsigned NOT NULL,
  `repository_id` bigint(20) unsigned NOT NULL,
  `pull_request_id` bigint(20) unsigned DEFAULT NULL,
  PRIMARY KEY (`id`),
  KEY `index_issues_on_pull_request_id` (`pull_request_id`),
  KEY `index_issues_on_repository_id` (`repository_id`),
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;

VSchema for sharded keyspace

{
  "sharded": true,
  "vindexes": {
    "hash": {
      "type": "hash"
    },
    "issues_id_keyspace_idx": {
      "type": "lookup_unique",
      "params": {
        "table": "issues_id_keyspace_idx",
        "from": "id",
        "to": "keyspace_id"
      },
      "owner": "issues"
    },
    "pull_requests_id_keyspace_idx": {
      "type": "lookup_unique",
      "params": {
        "table": "pull_requests_id_keyspace_idx",
        "from": "id",
        "to": "keyspace_id"
      },
      "owner": "pull_requests"
    },
  },
  "tables": {
    "issues_id_keyspace_idx": {
      "column_vindexes": [
        {
          "column": "id",
          "name": "hash"
        }
      ]
    },
    "issues": {
      "column_vindexes": [
        {
          "column": "repository_id",
          "name": "hash"
        },
        {
          "column": "id",
          "name": "issues_id_keyspace_idx"
        },
        {
          "column": "pull_request_id",
          "name": "pull_requests_id_keyspace_idx"
        }
      ]
    },
    "pull_requests_id_keyspace_idx": {
      "column_vindexes": [
        {
          "column": "id",
          "name": "hash"
        }
      ]
    },
    "pull_requests": {
      "column_vindexes": [
        {
          "column": "repository_id",
          "name": "hash"
        },
        {
          "column": "id",
          "name": "pull_requests_id_keyspace_idx"
        }
      ]
    }
  }
}

When we want to create a pull request, we first add a new pull_requests row, which will write a new entry to the pull_requests_id_keyspace_idx lookup table. Then we create an issues row with a pull_request_id value - this will cause a lookup to the pull_requests_id_keyspace_idx lookup table to ensure that the keyspace id matches with the keyspace id generated from the repository_id column.

But when we want to create a standalone issues row, where pull_requests_id is NULL, this will fail with the above mentioned error.

Other Considerations

This change is unrelated to the ignore_nulls option. ignore_nulls allows NULL values on the owner table of the lookup vindex, but this is not the case here. We don't want to allow NULL values on e.g. pull_requests.id on the owner table.

Adding another option to the lookup vindex for this functionality is not required - nullability of the column is fully handled by the underlying database schema (in the example above, the issues.pull_request_id column itself defines whether it can contain NULL values or not).

Writing a NULL value to an unowned, non-unique lookup Vindex (lookup/consistent_lookup) was possible even without the fix - due to the nature of the bug this only affected lookup_unique, consistent_lookup_unique and other non-reversible single column vindexes.

Related Issue(s)

N/A

Checklist

• Should this PR be backported?
• Tests were added or are not required
• Documentation was added or is not required

[arthurschreiber]

My understanding of Go is not great, but I believe the verifyKsids != nil check was always returning true, because verifyKsids is declared as an empty slice, not a nil slice. Using len(verifyIndexes) > 0 is more in line of the actual intention of this code.

[arthurschreiber]

Apparently using nil slices is preferred in Go, according to https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices. I'll make that change and push it up shortly.

[harshit-gangal]

The changes overall looks good.

Could you please add an end to end test for it here go/test/endtoend/vtgate/lookup_test.go ?

[harshit-gangal]

nit:
require.NoError(t, err) instead of if condition

[arthurschreiber]

@harshit-gangal Thank you for the review! ❤️ I'll try to find some time to make the requested change and add a few end to end test cases tomorrow! 👍

[arthurschreiber]

@harshit-gangal I updated the pull request to also add an end to end test case. 🙇‍♂️

[mattlord]

@arthurschreiber if you rebase on origin/main then the dependency issues will be fixed. Thank you for the contribution!

[arthurschreiber]

@mattlord CI has been quite flaky for me. Could you kick off that one failing build again? 🙇‍♂️