sql: enable adding columns which are not written to the current primary index #59149
Comments
ajwerner
added
the
C-enhancement
ajwerner
added
the
A-schema-descriptors
|
@RaduBerinde I'm assigning you only because I'd love to get some intel from you on implications in the optimizer for the two proposals to the table descriptor structure. |
|
The optimizer's catalog treats the primary index just like any index - we already present all columns as stored columns (with the optimizer catalog objects like optTable making the translation). The difference in the catalog would be rather small (e.g. the primary index could simply not present these mutation columns). However, the optimizer does assume that a scan of the primary index can produce all necessary columns - this is a core assumption that isn't easy to work around. So from the optimizer side, we will run into problems if we ever have to fetch a value on such a column. Seems like we shouldn't, but I remember there were surprising cases where the execution engine needed the old value of a mutation column. I don't think I ever exactly understood the details, perhaps @andy-kimball remembers. There are some mentions in #46285 Would we ever have a secondary index on such a mutation column? In that case, it's pretty clear that the optimizer must produce the old value, e.g. for a DELETE. I'm hoping that we sequence these schema changes so we only add such indexes after we finished adding the new column. |
|
We should never be reading values in mutation columns. However, we've had bugs in the past where we were reading them. I fixed some cases where we were mistakenly reading those values, but there may be others. |
I'm not sure reordering is possible. If we were to add a column, and then add a secondary index on the column later in the same transaction (which today we do support), I think we do have to keep the column non-public until after we finished backfilling the index on it. The reason is that "finishing adding the new column" means swapping the primary index, but at that point it's difficult to roll back. Just to clarify, don't we still need to "produce the old value" on deletes even just for the secondary index which is the replacement primary index? It's still not clear to me why this, by itself, isn't already a problem. |
I think it's sort of possible. Today we support having columns which are not public in the primary index. This happens both when adding and when dropping. I think it may make sense to do the primary index swap before making new columns (or secondary indexes) public. In today's world that leaves us in a somewhat weird place regarding constraints but I've come around to that being okay (actually, sort of good). The summary is that if we keep constraints around for quite a long time, then we can roll things back safely. The downside is that it might break some small amount of schema changes, but things are super broken today. Also, no transaction will need to write anything other than default values to columns which exist in indexes but don't exist in the primary key. There will never be a requirement to read column values for columns not in the primary index. Below find a riff on #47989 (comment) Let's consider the following scenario: Let's dig into what this might look like in some different scenarios. TodayThe above today goes extremely poorly #46541.
Let's also look at the observed behaviors:
Index swapping (non-transactional, keeping constraints around)
This isn't a super happy vision however. Imagine we didn't drop the check on |
@andy-kimball I am referring to this discussion in #46285:
Here is a current test; notice how it's scanning the |
@lucy-zhang we really only need values for columns that are part of a key; if there are no secondary indexes on the new column we shouldn't need the value. |
To clarify, we may need the values of new column not in the primary index for the purpose of writing, it's just that we will be able to produce their values without reading anywhere. For new columns which are not computed, that will mean using their default value. For computed columns, it will mean computing them. Also, those computed columns may rely on non-computed columns, so the default expr evaluation may need to be performed. |
|
In fact, it's somewhat complex. I think the way that the update will work is that before the new column is made public, the new primary index will need to be swapped in so that new updates using the version preceding the column becoming public will retain values of writes which saw the column. |
|
For a simpler scenario:
|
|
In this scenario, during "index backfill to unique index" we will need to read values of |
Indeed |
|
Updated and added numbers for future discussion clarity |
|
Maybe the secondary index should not enter
|
|
This leaves:
|
|
Questions about this proposal that I have yet to think through:
Ultimately, I still think the "correct" way to solve this would be to allow secondary indexes, at least in this restricted case, to also provide values for non-public columns. I would be disappointed if we ended up codifying this set of schema change state dependencies purely to work around the limitation on indexes on non-public columns (AFAIK that would be the only reason why we'd adopt it). It's true that we can change the state graphs much more easily in the new schema changer, but reasoning about correctness is still not straightforward. |
|
@RaduBerinde Will check and foreign key constraints work as-is for mutation columns not in the primary index, or do we also need to make changes there? (FK constraints aren't supported for computed columns, but they are supported for columns with default values.) |
|
Great point Lucy! I lost sight of the whole drop column debacle motivating this. I think the saving fact for all of this is the fact that new columns, until they are public, are only getting written with default values (or computed values which can be derived from them). I think we need to keep those new columns in that state until nothing can fail. I believe that this motivates the original ordering of the proposal (edited away) where the swap to the new primary index happens only after all new indexes have been built and constraints verified. |
|
I don't think we support FKs on mutation columns currently (or at the very least, they're not tested in the optimizer).. I was under the impression that this isn't currently possible? I don't see how that would work, you need to read values to check FKs (and you can't just recompute the default value every time, because it could change, e.g. |
This comes back to the need to have another state that indicates that the values have been backfilled and are readable. |
We definitely do support FKs on mutation columns (as Andrew implied, we first get the column into delete-and-write-only, then backfill, then validate, then make the column public). This is how we support statements like I'll come back to the other points later. |
|
The FK relation only appears (to the optimizer) after the column backfill, correct? If yes, then it works but only because the column is really in this "non-public, but readable" state. It's definitely fragile as far as opt is concerned. |
OK, that makes sense, and was my vague impression as well (@andy-kimball and I discussed it in the context of check constraints a long time ago). We do want to introduce a more formal notion of this state (non-public, but readable) in the new schema changer, since it's essential to constraint validation. |
But Radu's point was that we can't proceed with this approach without extensive changes to the optimizer, right? We have to be able to read the default values that we wrote (since default expressions can use impure functions) in order to delete from the secondary index on the non-public column, but those values aren't in the old primary index. It seems like we have two unpalatable options:
Are there other options I'm missing? |
|
1 would be a lot of effort.. And might need some kind of execution support (for the "combine those results" part). Regarding the "non-public but readable" column state - from a planning/execution standpoint, is there any difference between this state and a public hidden column (other than not being able to select the column even by name)? If not, the best way to expose that to the optimizer to avoid a lot of unnecessary complication would be to present it as a regular column and add a new kind of "hidden" property ("non-selectable" or whatever). |
|
This issue turns out the be the biggest blocker for 21.2 and the new schema changer. To recap:
There are two basic directions we could take:
Additional nuance:
|
|
Note that currently when you do an UPDATE on a row, the optimizer projects the default expression for any mutation column (because the updated row might not get backfilled). So there is precedent for re-evaluating these expressions. Any weirdness with sequences and the like is not a correctness violation (we don't make any guarantees about the order in which we backfill rows, and values generated by sequences are allowed to have gaps). |
|
@RaduBerinde here's the related question. Say we do go down this path where we're going to swap to using a new primary index. The main things we'd need to be able to support is:
The important thing to note here is that we can fully synthesize these values. It seems possible to constraint this concept of having columns which we do not want to encode into the current primary index entirely down in the row writing layer. Does that seem correct? My reading of the code (similar to what you just said) is that we synthesize new values for all of these mutation columns any time we insert or update. |
|
We also read the "old" values on updates, even if we synthesize new ones; this is problematic because the canonical primary index scans have to be able to produce these columns. These values are needed by execution, my guess is for when they are part of an index key. This probably stems from a disconnect on what "mutation" columns/index combinations are possible, compounded with the lack of a "non-public but backfilled" state. If we make it clear that a mutation column can never be part of an index (is that true?), then we could fix this. |
That's roughly my plan. It's to leave the indexes which involve new columns in |
|
Even in delete-only index state, if the column is part of the index key, we need to read the old value in order to delete the corresponding KV. So we would only be able to support indexes that store these columns. |
|
I think the best way to evaluate the proposal from the optimizer side is to make a tentative diff to the opt/cat documentation which explains precisely which combinations of mutation states would be possible. It will take some effort and back-and-forth, but it will be invaluable. |
We can deal with that, it'll take some descriptor finagling but we can add yet another descriptor step. We'll just commit the new indexes as totally not existing and then move them through the relevant steps after the primary index swap occurs. |
|
Alright, I think we've reached a plan here. We're going to accept the extra index backfill in some cases. Those cases are: and either or For various reasons, this seems fine. The new concepts we need to introduce is a column which we mark as not being stored in the primary index. The critical limitation of these is that they must not appear as a key in any index. We'll expose the first concept to the optimizer just to make sure we don't do anything insane but the optimizer doesn't need to do anything about it. The main user of this is going to be the row writing logic which is going to need to avoid writing omitted columns into the primary index. This will be quite well contained. We don't need to expose any new absent index concepts because the new schema changer state already represents that. There's an invariant that we'll enforce that any time a column is a key in a secondary index it much be present in the current primary index of the table. We'll want to make that rule concrete in the schemachange planner. I'm going to prototype this tomorrow. |
In a recent commit, the StoreColumnIDs and StoreColumnNames slices in primary indexes were populated when previously they had simply been empty. We simply assumed that all non-virtual columns in a table would be stored in the primary index: primary key columns in the key, the rest in the value. This commit breaks that assumption by using the StoreColumnIDs slice to determine what goes into the primary index. This makes it possible for the new schema changer to add columns safely, preventing unwanted writes to the old primary index while the schema change is underway. Fixes #59149. Release note: None
In a recent commit, the StoreColumnIDs and StoreColumnNames slices in primary indexes were populated when previously they had simply been empty. We simply assumed that all non-virtual columns in a table would be stored in the primary index: primary key columns in the key, the rest in the value. This commit breaks that assumption by using the StoreColumnIDs slice to determine what goes into the primary index. This makes it possible for the new schema changer to add columns safely, preventing unwanted writes to the old primary index while the schema change is underway. Fixes #59149. Release note: None
In a recent commit, the StoreColumnIDs and StoreColumnNames slices in primary indexes were populated when previously they had simply been empty. We simply assumed that all non-virtual columns in a table would be stored in the primary index: primary key columns in the key, the rest in the value. This commit breaks that assumption by using the StoreColumnIDs slice to determine what goes into the primary index. This makes it possible for the new schema changer to add columns safely, preventing unwanted writes to the old primary index while the schema change is underway. Fixes cockroachdb#59149. Release note: None
In a recent commit, the StoreColumnIDs and StoreColumnNames slices in primary indexes were populated when previously they had simply been empty. We simply assumed that all non-virtual columns in a table would be stored in the primary index: primary key columns in the key, the rest in the value. This commit breaks that assumption by using the StoreColumnIDs slice to determine what goes into the primary index. This makes it possible for the new schema changer to add columns safely, preventing unwanted writes to the old primary index while the schema change is underway. Fixes cockroachdb#59149. Release note: None
66599: sql: enable adding columns which are not written to the current primary index r=postamar a=postamar
row,schemachanger: use StoreColumnIDs/Names in primary index
In a recent commit, the StoreColumnIDs and StoreColumnNames slices in
primary indexes were populated when previously they had simply been
empty. We simply assumed that all non-virtual columns in a table would
be stored in the primary index: primary key columns in the key, the rest
in the value.
This commit breaks that assumption by using the StoreColumnIDs slice to
determine what goes into the primary index. This makes it possible for
the new schema changer to add columns safely, preventing unwanted writes
to the old primary index while the schema change is underway.
Fixes #59149.
Release note: None
sql,tabledesc: add new IndexDescriptorVersion for primary indexes
Previously, the IndexDescriptorVersion type was only used to describe
the encoding of secondary indexes. This commit adds a new value for
use in primary indexes, PrimaryIndexWithStoredColumnsVersion, to signify
that the StoredColumnIDs and StoredColumnNames slices are populated
correctly.
Previously, these slices did not need to be populated at all. This is
because the set of columns comprising the primary index of a table is
assumed to be all non-virtual columns of that table. Our upcoming work on
the new schema changer will require us to violate that assumption
however. This commit is in preparation of that change.
In our effort to make meaningful the concept of stored columns in
primary indexes, this commit also changes the contents of the
information_schema.statistics table. As a result, SHOW INDEXES and SHOW
COLUMNS behave the same way regardless of whether an index is primary or
secondary.
Release note (sql change): The contents of the statistics table in the
information schema have changed, therefore so have the results of SHOW
INDEX and SHOW COLUMNS. A column which is not in the primary key will
now be listed as belonging to the primary index as a stored column.
Previously, it was simply not listed as belonging to the primary index.
66664: sql: First round of cleanup of schemachange/random-load r=ajwerner,otan a=ajstorm
This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:
- Makes several errors pgcodes, so that they can be properly added to
the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
times in an index definition.
- Allows for column type changes, which must have the experimental flag
enable_experimental_alter_column_type_general flag set.
It also disables the testing of setColumnType (tracked with #66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with #66663).
Release note: None.
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Adam Storm <[email protected]>
exalate-issue-sync
bot
added
T-sql-foundations
Is your feature request related to a problem? Please describe.
This issue is a breakout of point 2 in #47989 (comment). Column backfills for changing the set of columns in a table have a number of issues outlined in #47989. The alternative is to build a new primary index and swap to it. We support this in theory with primary key changes, however, there's some slight nuance in that primary key changes require the set of columns in the new and old primary index are the same. This issue is concerned with the requirement that concurrent writer not write any new columns being backfilled to this new primary index to the old primary index.
The root of the problem is that primary index descriptors are handled specially. For secondary indexes, the columns stored in the value are primary index columns specified in
ExtraColumnIDsfor unique indexes and the columns inStoreColumnIDs. For primary indexes, however, all of the table columns are stored.The hazard is that, if we don't change anything, the column in DELETE_AND_WRITE_ONLY will end up being written to the existing primary index.
Describe the solution you'd like
The thrust of the solution is twofold:
Updated descriptor structure
I have two proposals here, one that I prefer but is riskier and one that is less invasive but worse. The latter does not in any
A clear and concise description of what you want to happen.
A) Make primary indexes look like secondary indexes and populate their
StoreColumnIDs.B) Add another field to the table descriptor to encode the set of columns which should not be written to the primary index
Adopting change to descriptor structure.
Let's assume we're going to go with B) as it seems more tractable. One thing which will need to change is the code which writes rows. My current reading is that we might be able to isolate this change to legitimately just this function. @yuzefovich could I ask you to verify that claim and generally have a look at this issue?
The bigger unknown for me is what changes would need to be made in the optimizer. @RaduBerinde could I ask you to give review this and provide some guidance.
Additional context
Something must be done here to realize #47989. More pressingly, we are working to not include support for column backfills in the new schema change rewrite so realistically something needs to be done here in the next few weeks.
Epic: CRDB-2356
The text was updated successfully, but these errors were encountered: