sql/schemachanger: add TTL dependency checking for ALTER TYPE in DSC#127436
Conversation
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
nice, this is a solid improvement! just had some nits about changing the string type, then feel free to merge after addressing those
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go line 42 at r1 (raw file):
// Check for elements depending on the column we are altering. walkColumnDependencies(b, col, "alter type of", "column", func(e scpb.Element, op, objType string) {
nit: let's make these parameters be of type redact.SafeString, so that way they don't get redacted in logs and errors.
pkg/sql/sqlerrors/errors.go line 297 at r1 (raw file):
// the internal column created for the duration expression. These are tables // that define the ttl_expire_after setting. func NewAlterDependsOnDurationExprError(op, objType, colName, tabName string) error {
nit: let's make these parameters be of type redact.SafeString, so that way they don't get redacted in logs and errors.
pkg/sql/sqlerrors/errors.go line 312 at r1 (raw file):
// the column that is referenced in the expiration expression. func NewAlterDependsOnExpirationExprError( op, objType, colName, tabName, expirationExpr string,
nit: let's make these parameters be of type redact.SafeString, so that way they don't get redacted in logs and errors.
24ed396 to
d96e0a8
Compare
spilchen
left a comment
There was a problem hiding this comment.
tftr @rafiss. You approved the changed, but I wasn't able to apply the change exactly as you had requested. I would appreciate another look when you have a chance.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go line 42 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: let's make these parameters be of type
redact.SafeString, so that way they don't get redacted in logs and errors.
Making this change had a snowball effect. I modified about 14 files and around 100 lines, but more changes were still needed. Is it sufficient to cast the string to redact.SafeString right when the error is logged? That's what I ended up doing instead, but I'm not completely sure if this will ensure redaction in the logs.
pkg/sql/sqlerrors/errors.go line 297 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: let's make these parameters be of type
redact.SafeString, so that way they don't get redacted in logs and errors.
See my comment about the snowball affect of such a change.
pkg/sql/sqlerrors/errors.go line 312 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: let's make these parameters be of type
redact.SafeString, so that way they don't get redacted in logs and errors.
See my comment about the snowball affect of such a change.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go line 42 at r1 (raw file):
Previously, spilchen wrote…
Making this change had a snowball effect. I modified about 14 files and around 100 lines, but more changes were still needed. Is it sufficient to cast the string to
redact.SafeStringright when the error is logged? That's what I ended up doing instead, but I'm not completely sure if this will ensure redaction in the logs.
yes, casting to redact.SafeString when constructing the error will also achieve the desired result.
to clarify, the goal is to not perform redaction on those strings, since we know they don't have sensitive data. all strings are redacted by default unless we opt-in to non-redaction in some way. more details are here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1824817806/Log+and+error+redactability
pkg/sql/sqlerrors/errors.go line 302 at r2 (raw file):
pgcode.InvalidTableDefinition, `cannot %s %s %s while ttl_expire_after is set`, redact.SafeString(op), redact.SafeString(objType), redact.SafeString(colName),
ah this was my bad - the "operation" and "object type" are both safe strings, since these are strings that we construct within our own code. but the column name (and table name) are not safe strings, since these come from customer input
pkg/sql/sqlerrors/errors.go line 318 at r2 (raw file):
pgcode.InvalidTableDefinition, `cannot %s %s %q referenced by row-level TTL expiration expression %q`, redact.SafeString(op), redact.SafeString(objType), redact.SafeString(colName),
same here, colName should not be considered safe
This extracts the TTL expression, whether user-defined or system-defined, into a declarative schema changer (DSC) Expression element. This allows us to correctly establish column dependencies for the column used in the TTL expression. By doing so, we can traverse these column dependencies to return appropriate errors when attempting to alter a column that the TTL expression depends on. Previously, checking the TTL expression was an outstanding TODO in the alter type code. The new dependency checking I added allows us to address that TODO. Using column dependencies in DSC elements is more manageable than the previous method, which was in `checkRowLevelTTLColumn`. Therefore, this update also modifies the drop column functionality to use column dependencies, allowing us to get rid of that function. Additionally, this update standardizes the error messages for attempts to alter a column dependent on the TTL expression. I opted for more verbose error messages with hints, leading to updates in some of the errors checked in logic tests. Epic: CRDB-25314 Closes: cockroachdb#126143 Release note: None
d96e0a8 to
3225b9a
Compare
spilchen
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go line 42 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
yes, casting to
redact.SafeStringwhen constructing the error will also achieve the desired result.to clarify, the goal is to not perform redaction on those strings, since we know they don't have sensitive data. all strings are redacted by default unless we opt-in to non-redaction in some way. more details are here: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1824817806/Log+and+error+redactability
Thanks for the context.
pkg/sql/sqlerrors/errors.go line 302 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
ah this was my bad - the "operation" and "object type" are both safe strings, since these are strings that we construct within our own code. but the column name (and table name) are not safe strings, since these come from customer input
Done.
pkg/sql/sqlerrors/errors.go line 318 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
same here, colName should not be considered safe
Done.
|
tftr bors r+ |
|
Build failed: |
|
bors retry |
This extracts the TTL expression, whether user-defined or system-defined, into a declarative schema changer (DSC) Expression element. This allows us to correctly establish column dependencies for the column used in the TTL expression. By doing so, we can traverse these column dependencies to return appropriate errors when attempting to alter a column that the TTL expression depends on.
Previously, checking the TTL expression was an outstanding TODO in the new alter type code. The new dependency checking I added allows us to address that TODO.
Using column dependencies in DSC elements is more manageable than the previous method, which was in
checkRowLevelTTLColumn. Therefore, this update also modifies the drop column functionality to use column dependencies, allowing us to get rid of that function.Additionally, this update standardizes the error messages for attempts to alter a column dependent on the TTL expression. I opted for more verbose error messages with hints, leading to updates in some of the errors checked in logic tests.
Epic: CRDB-25314
Closes: #126143
Release note: None