sql: remove experimental_unique_bytes (requires system table migration)

[petermattis]

We should remove the experimental_unique_bytes builtin and replace usage of it with unique_rowid. Unfortunately, this is a bit involved as system.eventlog uses experimental_unique_bytes. We currently do not have a facility for performing schema changes on system tables. And because the uniqueID field is stored in the primary key (which we can't modify via a schema change), we'd have to do something like:

  create table system.eventlog2 ...;
  insert into system.eventlog2 select from system.eventlog;
  drop table system.eventlog;
  alter table rename system.eventlog2 system.eventlog;

[a-robinson]

As noted on #11722, this will involve changing the type of uniqueID from BYTES to INT. I'm not sure we can change existing data like that in a way that doesn't break consumers of the table, given that AFAIK ints aren't parseable as bytes. Right now, our only consumer is the admin server. The UI doesn't appear to actually read the field from the admin server, so at some level we don't have any part of the code that would truly care about changing it.

However:

• If we do this change in a non-stop-the-world fashion, admin server requests for events would break on servers running the old version due to not being able to parse the field as a bytes type. That doesn't really seem like a big deal at this stage of our product, but it'll happen.
• There may be other clients out there programmatically consuming the uniqueID field. This seems fairly unlikely, but it is possible.

It was proposed in #11722 that this would be a good first use for the migration code (in #11658) since we don't have code ready to use a system.jobs table yet. I don't think the two drawbacks are all that risky, but would appreciate other opinions.

@vivekmenezes @petermattis @bdarnell

[petermattis]

I thought there was a proposal to replace the default with unique_rowid()::bytes. This maintains the same type but replaces the function. That doesn't actually work, so you might need unique_rowid()::string::bytes.

[a-robinson]

Ah ok, I should have checked whether a conversion like that was possible.

However, it will still leave the field pretty awkwardly inconsistent with all our other system tables that use ints for IDs. It may be worth changing now to get that inconsistency out of the way before we're definitely stuck with it after we start getting real users.

[mrtracy]

When do the migrations run? It seems wrong that you can end up in a situation where there are servers running the old version, but the migration has already been run.

Another alternative would be to simply create a new table and migrate the old data to the new table. That wouldn't be appropriate if this was the only change, but while we're taking a look at it i'd like to give these tables a bit of an overhaul if possible: specifically, i'd like to separate DDL events from node events, storing them in two different tables.

That's a larger scope than this change, and if this is a priority for Q4 then I'm okay with the drawbacks listed by Alex (although I think our migration process should strive avoid situations like that in the future).

[a-robinson]

When do the migrations run? It seems wrong that you can end up in a situation where there are servers running the old version, but the migration has already been run.

Right now, they just run when a node starts up with a new version and sees that one of the migrations it knows about hasn't been run.

Of course, that only works for migrations that are backwards compatible, so when we need to run a migration that isn't backward compatible, we'll have to get smarter and only run them once all nodes are at some minimum version. In that case, relevant code that relies on the thing being changed will have to support both the before and after states.

Most meaningful migrations will probably end up in the second bucket, but we didn't have any Q4 goals that needed it.

[petermattis]

It may be worth changing now to get that inconsistency out of the way before we're definitely stuck with it after we start getting real users.

I don't mind a stop-the-world upgrade during the beta period to remove this wart.

[a-robinson]

I may have been a bit overzealous in wanting to get rid of said wart. How much do other people care about it? The commit to do so (b310788) is admittedly quite a bit messier than if we were just swapping out experimental_unique_bytes() with unique_rowid()::string::bytes or uuid_v4(), which could be done with a single ALTER TABLE command and wouldn't end up with a system table that isn't using one of the reserved IDs.

@petermattis @bdarnell @vivekmenezes @paperstreet

[vivekmenezes]

swapping it out sounds fine to me. Can you really do it via ALTER TABLE. I thought you would need some hack to implement the swap.

[a-robinson]

Yeah, an ALTER TABLE can trivially change the default value of a primary key column, just not its data type:

root@:26257> CREATE TABLE eventlog (timestamp TIMESTAMP NOT NULL, eventType STRING NOT NULL, targetID INT NOT NULL, reportingID INT NOT NULL, info STRING, uniqueID BYTES DEFAULT experimental_unique_bytes(), PRIMARY KEY (timestamp, uniqueID));
CREATE TABLE
root@:26257> ALTER TABLE eventlog ALTER COLUMN uniqueID SET DEFAULT uuid_v4();
ALTER TABLE
root@:26257> show create table eventlog;
+----------+-----------------------------------------------------------------+
|  Table   |                           CreateTable                           |
+----------+-----------------------------------------------------------------+
| eventlog | CREATE TABLE eventlog (␤                                        |
|          |     timestamp TIMESTAMP NOT NULL,␤                              |
|          |     eventType STRING NOT NULL,␤                                 |
|          |     targetID INT NOT NULL,␤                                     |
|          |     reportingID INT NOT NULL,␤                                  |
|          |     info STRING NULL,␤                                          |
|          |     uniqueID BYTES NOT NULL DEFAULT uuid_v4(),␤                 |
|          |     CONSTRAINT "primary" PRIMARY KEY (timestamp, uniqueID),␤    |
|          |     FAMILY "primary" (timestamp, eventType, uniqueID),␤         |
|          |     FAMILY fam_1_targetID_reportingID (targetID, reportingID),␤ |
|          |     FAMILY fam_2_info (info)␤                                   |
|          | )                                                               |
+----------+-----------------------------------------------------------------+
(1 row)

[bdarnell]

I'm not bothered by the existence of experimental_unique_bytes at all and would be fine keeping it around forever. I think this issue is mainly interesting as a proof of concept for the migration system and forcing us to think through how we'd do this when it really matters. Given the difficulties you're encountering I think it makes sense to either make the simpler DEFAULT change or defer this until later.

[a-robinson]

Fixed by #12228