sql: remove public key column check when fetching#76788
sql: remove public key column check when fetching#76788craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This commit removes a check that fails creation of an IndexFetchSpec if a key column is not public. The only case where this comes up AFAICT is when we hit a duplicate key error and try to fetch the row to get the values for the error message. In this case, even though the column is not public, we know that the row we care about exists in the index, or we wouldn't have hit the error. Release note: None
|
This change is |
76795: rowenc: various improvements to IndexFetchSpec initialization r=RaduBerinde a=RaduBerinde First commit is #76788. #### catalog: store IndexFetchSpec key columns in table descriptor We now cache the IndexFetchSpec_KeyAndSuffixColumns in the table descriptor, making it cheaper to populate an IndexFetchSpec. Release note: None #### catalog: cleanup KeysPerRow This change renames KeysPerRow to IndexKeysPerRow and changes the argument to an IndexDescriptor, removing the error case. Release note: None #### catalog: cache family default columns in table descriptor We now cache the `IndexFetchSpec_FamilyDefaultColumn`s in the table descriptor, to avoid an allocation when creating an `IndexFetchSpec`. Release note: None #### rowenc: calculate key prefix length without allocating We now calculate the key prefix length directly instead of creating a throw-away key. Release note: None Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
|
Oops, I accidentally merged this with #76795. I'd still like a review, I can make any changes in another PR. |
| if !col.Public() { | ||
| // Key columns must be public. | ||
| return fmt.Errorf("column %q (%d) is not public", col.GetName(), col.GetID()) | ||
| } |
There was a problem hiding this comment.
Sorry it took me a while to look at this @RaduBerinde .
I'm really not sure how legit removing this check is. Any column which is not public is, by definition, not readable, and it seems to me that this function might be used to perform reads.
If that's not exclusively the case, and if UPSERTs are not considered to be reads, and this function is called to perform a write, then this column state check can be relaxed from "readable" to "writable".
There was a problem hiding this comment.
The nuance here is that if an UPSERT fails because of a conflict on a non-public column, that particular conflicting row value is in a way readable (because it is influencing a user-visible outcome, in this case an error). It's really only a matter of whether we include it in the error message or not.
This commit removes a check that fails creation of an IndexFetchSpec
if a key column is not public.
The only case where this comes up AFAICT is when we hit a duplicate
key error and try to fetch the row to get the values for the error
message. In this case, even though the column is not public, we know
that the row we care about exists in the index, or we wouldn't have
hit the error.
Release note: None