sql: Add system.jobs table and migration to add it to existing DBs #11722
Conversation
Built to initially be used by the migration code since our existing lease mechanisms for ranges and SQL schemas aren't very generalizable.
This is needed to allow us to make certain changes between versions that wouldn't be safe otherwise.
Now that there's a delay between when a node starts serving kv and when it starts serving SQL (due to the SQL migration code path), acceptance tests that rely on SQL can have a bad time without this.
Unfortunately, this required playing around a bit with the table creation code to get the permissions and ID right on the new table. This feels a little dangerous, so let me know what you think. I effectively ripped it straight out of cockroachdb#7073. The alternative was to add the table directly to kv, bypassing the SQL layer, but that seems dangerous to do anytime other than initial bootstrapping and won't work great for future changes like adding a new column or index to a system table. In other news, the amount of tests that have hard-coded results dependent on the system tables makes adding a new table a bit of a chore, but I suppose that adding a system table isn't a very common operation.
|
Also, the schema is a bit of a strawman sketched out with @paperstreet, who also talked with @dt. We figured we'd leave the |
|
Alex, It's probably better to leave this code out of master for now.
Do you think we can fix
#5887 using
the new mechanism. It might mean adding in some code that reads the
descriptor and rewrites it with the new function unique_rowid. I don't
think we have to go through the elaborate scheme described in the bug.
…On Wed, Nov 30, 2016 at 2:05 PM Alex Robinson ***@***.***> wrote:
Also, the schema is a bit of a strawman sketched out with @paperstreet
<https://github.com/paperstreet>, who also talked with @dt
<https://github.com/dt>. We figured we'd leave the lease field out until
we have code that uses it, but that same reasoning actually applies to this
commit in general -- it'd be fine to leave this of master out until we have
code that uses it. It's main purpose for me was to put the migration code
through its paces a bit.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11722 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALOpBBPDxnDEm47HnagGt5-w2s7fPilcks5rDcjhgaJpZM4LAjsg>
.
|
|
Yes, I believe we could fix #5887. I do think we may want to take the approach mentioned in the issue, though, given that switching from And not merging this SGTM. I'm always wary of adding things that aren't being used yet. That's why I tried this out to utilize the migration code, but the unique_rowid change can work for that as well. |
|
Closing in favor of #12228 as the first migration to use the new infrastructure. Something along these lines can be resurrected when needed. |
Ignore the first three commits - they're already under review in #11658. This is the first use of the infrastructure introduced in that PR for safely running backward-compatible migrations at node startup.
@vivekmenezes @paperstreet @bdarnell
Commit message:
Unfortunately, this required playing around a bit with the table
creation code to get the permissions and ID right on the new table.
This feels a little dangerous, so let me know what you think. I
effectively ripped it straight out of #7073.
The alternative was to add the table directly to kv, bypassing the SQL
layer, but that seems dangerous to do anytime other than initial
bootstrapping and won't work great for future changes like adding a
new column or index to a system table.
In other news, the amount of tests that have hard-coded results
dependent on the system tables makes adding a new table a bit of a
chore, but I suppose that adding a system table isn't a very common
operation.
This change is