sql: views do not track string-based dependencies properly #24556
Assignees
Labels
A-schema-changes
A-schema-descriptors
Relating to SQL table/db descriptor handling.
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Comments
knz
added
the
C-bug
|
It's likely that the logic that restricts BACKUP/RESTORE to work on "closed" sets of objects is similarly flawed. |
knz
changed the title
dt
added a commit
to dt/cockroach
that referenced
this issue
Apr 6, 2018
The `RESTORE ... into_db = 'foo'` option allows overriding the destination database for all restored tables, views and sequences. Previously this option could not be used when restoring views, as the view query string could contain references to tables qualified with their original database name(s), and we did not support rewriting that query to reflect the overridden destination for those tables. This change adds that rewriting (except for tables/sequences named in strings, see cockroachdb#24556). Since a destination override is applied to all tables being restored, and since restoring a view requires restoring all tables it references, any table referenced by the view query -- regardless of its prior qualification -- can be re-qualified with the override DB. Release note (enterprise change): support restoring views when using the 'into_db' option.
dt
added a commit
to dt/cockroach
that referenced
this issue
Apr 6, 2018
The `RESTORE ... into_db = 'foo'` option allows overriding the destination database for all restored tables, views and sequences. Previously this option could not be used when restoring views, as the view query string could contain references to tables qualified with their original database name(s), and we did not support rewriting that query to reflect the overridden destination for those tables. This change adds that rewriting (except for tables/sequences named in strings, see cockroachdb#24556). Since a destination override is applied to all tables being restored, and since restoring a view requires restoring all tables it references, any table referenced by the view query -- regardless of its prior qualification -- can be re-qualified with the override DB. Release note (enterprise change): support restoring views when using the 'into_db' option.
dt
added a commit
to dt/cockroach
that referenced
this issue
Apr 8, 2018
The `RESTORE ... into_db = 'foo'` option allows overriding the destination database for all restored tables, views and sequences. Previously this option could not be used when restoring views, as the view query string could contain references to tables qualified with their original database name(s), and we did not support rewriting that query to reflect the overridden destination for those tables. This change adds that rewriting (except for tables/sequences named in strings, see cockroachdb#24556). Since a destination override is applied to all tables being restored, and since restoring a view requires restoring all tables it references, any table referenced by the view query -- regardless of its prior qualification -- can be re-qualified with the override DB. Release note (enterprise change): support restoring views when using the 'into_db' option.
|
Hey @rohany I'm putting this on your mental roadmap. I think we should fix this kind of thing at some point in the medium term. |
RichardJCai
added a commit
to RichardJCai/cockroach
that referenced
this issue
Jun 11, 2020
…creating a view. Also fixed minor bug where column dependencies were not being added when a sequence is referred to in a currval call. Fixes cockroachdb#24556, 50033 Release note (sql change): Sequences passed as a string argument into views are now tracked as a dependency. Example: CREATE VIEW v AS SELECT nextval('s') will now add a dependency on sequence s.
RichardJCai
added a commit
to RichardJCai/cockroach
that referenced
this issue
Jun 11, 2020
…creating a view. Also fixed minor bug where column dependencies were not being added when a sequence is referred to in a currval call. Fixes cockroachdb#24556, 50033 Release note (sql change): Sequences passed as a string argument into views are now tracked as a dependency. Example: CREATE VIEW v AS SELECT nextval('s') will now add a dependency on sequence s.
craig bot
pushed a commit
that referenced
this issue
Jun 16, 2020
49934: colexec: optimize aggregate functions r=yuzefovich a=yuzefovich **colexec: simplify aggregateFunc interface a bit** This commit simplifies `SetOutputIndex` to no longer be required to unset nulls after the desired output index. The unsetting is only necessary when we have "overflow" tuples in the ordered aggregator, so it makes that it should be the one resetting the nulls. Additionally, the implementation of `SetOutputIndex` no longer checks whether `curIdx` is not -1 (meaning the function has already performed some aggregation). I think this was an artifact from the past and is no longer necessary. This commit also makes a minor performance optimization to unwrap the output batch in the ordered aggregator. Release note: None **colexec: optimize any_not_null func for hash aggregation** When we're performing hash aggregation, we know that there will be exactly one group. This insight allows us optimize the aggregate functions, and this commit performs the optimization of any_not_null. Now we will have two separate files for both aggregator kinds (hash and ordered), and that part is handled by a preprocessing step that replaces `_AGGKIND` template "variable" with the corresponding aggregator kind. I looked into introducing some base struct for aggregates to reuse, but it is rather complicated at the moment - some aggregate functions iterate over `[]*oneArgOverload`, and we could flatten that out. However, other functions operate with a custom "tmpl info" struct. Furthermore, extending the struct to include `AggKind` variable would require refactoring the templates themselves because we assume that `.` (dot) of the template usually is at `lastArgWidthOverload` struct. Probably in order to provide some "aggregate base tmpl info" struct that would help with hash vs ordered aggregation generation we would need to introduce an interface which custom tmpl infos will implement. Release note: None **colexec: optimize all aggregate funcs for hash** This commit adds a simple optimization of omitting the check for a start of a new group in all aggregate functions if we're performing hash aggregation. This change also allows us to slightly simplify the hash aggregator. Release note: None **colexec: optimize aggregateFunc contract a bit** This commit adjusts the contract of `aggregateFunc.Init` method to require that the very first tuple in the whole input should *not* have a `true` value set in `groups` vector. This requirement allows us to optimize all aggregate functions for the case of ordered aggregator. The ordered aggregator has been adjusted accordingly to satisfy the contract. This commit also marks `oneShotOp` as `NonExplainable`. Release note: None 50103: sql: add a view dep on sequence string arguments r=RichardJCai a=RichardJCai Add a dependency on sequences that are used via string argument when creating a view. Also fixed minor bug where column dependencies were not being added when a sequence is referred to in a currval call. Fixes (part of) #24556 and #50033 Release note (sql change): Sequences passed as a string argument into views are now tracked as a dependency. Example: CREATE VIEW v AS SELECT nextval('s') will now add a dependency on sequence s. 50247: colexec: fix resetting of the hashtable with distinct build mode r=yuzefovich a=yuzefovich Some time ago we introduced "distinct" build mode to the hash table which is used by the unordered distinct. In that addition we forgot to add one thing to be reset whenever the hash table is reset which causes partially ordered distinct to behave incorrectly. The damage, however, is negligible because the bug doesn't affect "default" build mode nor unordered distinct as is, only when we have partially ordered distinct which uses the unordered distinct internally and resets it between different "chunks". And we don't currently plan partially ordered distinct, so only our unit tests are able to hit it, and there is no need for a backport. Fixes: #50006. Release note: None 50253: roachtest: run EXPLAIN ANALYZE (DEBUG) in tpchvec/perf on failure r=yuzefovich a=yuzefovich In order to get more information that could explain occasional slowness on query 7 this commit updates the usage of `EXPLAIN ANALYZE` to `EXPLAIN ANALYZE (DEBUG)` which will provide us with traces and other useful information. The bundles are `curl`ed into `logs` folder so that the test runner fetched it alongside normal log files. Addresses: #50029. Release note: None 50263: roachtest: skip rotten cancel/tpcc test r=yuzefovich a=asubiotto This test is pretty flaky due to a number of reasons. It fails pretty noisiliy so skip it until we fix the underlying causes. Fixing this test is tracked in #42103 Release note: None 50267: logictest: add -print-blocklist-issues flag r=yuzefovich a=asubiotto Previously all issues associated with a blocklist were printed out, even when running a single test. This commit turns that printing off by default, which can be enabled by specifying the -print-blocklist-issues flag. Release note: None (testing change) Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: richardjcai <[email protected]> Co-authored-by: Alfonso Subiotto Marques <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Fails with:
This is because CockroachDB fails to peek into strings that designate object names.
Other possible failing cases:
'tablename'::regclassand related constructs.The reason why pg does not suffer from this is that object name strings are converted to OIDs internally, and those OIDs don't change across renames.
cc @jordanlewis
The text was updated successfully, but these errors were encountered: