Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix starting the server with tables having default expressions containing dictGet() #19805

Conversation

vitlibar
Copy link
Member

@vitlibar vitlibar commented Jan 29, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category:

  • Bug Fix

Changelog entry:
Fix starting the server with tables having default expressions containing dictGet().
Allow getting return type of dictGet() without loading dictionary.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Jan 29, 2021
@kitaisreal kitaisreal self-assigned this Jan 29, 2021
@@ -140,6 +149,7 @@ class FunctionDictHelper
const ExternalDictionariesLoader & external_loader;
/// Access cannot be not granted, since in this case checkAccess() will throw and access_checked will not be updated.
std::atomic<bool> access_checked = false;
std::unique_ptr<DictionaryStructure> structure_holder;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there can be a race condition, because this helper is used in IFunction getReturnType, and this getReturnType can be called from multiple threads.

Copy link
Member Author

@vitlibar vitlibar Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. Actually we can't use that kind of caching here at all due to #16205. I've changed the code.

auto dictionary = helper.getDictionary(dictionary_name);

return helper.getDictionaryAttribute(dictionary, attribute_name).type;
return helper.getDictionaryAttribute(dictionary_name, attribute_name).type;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove TODO at line 280.

/// TODO: We can load only dictionary structure

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@vitlibar vitlibar force-pushed the allow-getting-return-type-of-dictget-without-loading-dictionary branch from 29b317c to 2a90b1b Compare January 29, 2021 18:49
@vitlibar vitlibar changed the title Allow getting return type of dictGet() without loading dictionary. Fix starting the server with tables having columns' default expressions containing dictGet() Jan 29, 2021
@vitlibar vitlibar changed the title Fix starting the server with tables having columns' default expressions containing dictGet() Fix starting the server with tables having default expressions containing dictGet() Jan 29, 2021
@robot-clickhouse robot-clickhouse added pr-bugfix Pull request with bugfix, not backported by default and removed pr-improvement Pull request with some product improvements labels Jan 29, 2021
@vitlibar vitlibar force-pushed the allow-getting-return-type-of-dictget-without-loading-dictionary branch from 2a90b1b to 2741f81 Compare January 29, 2021 19:04
@filimonov
Copy link
Contributor

Closes #13613

@filimonov
Copy link
Contributor

Also #13059 and maybe more.

@kitaisreal kitaisreal added the force tests Force test ignoring fast test output. label Jan 29, 2021
Vitaly Baranov added 2 commits January 30, 2021 19:06
…ns containing dictGet().

Allow getting return type of dictGet() without loading dictionary.
@vitlibar vitlibar force-pushed the allow-getting-return-type-of-dictget-without-loading-dictionary branch from abfaf3a to 95177dc Compare January 30, 2021 16:07
@vitlibar vitlibar force-pushed the allow-getting-return-type-of-dictget-without-loading-dictionary branch from 195d4ad to cbd4bac Compare January 31, 2021 02:25
@vitlibar vitlibar merged commit 9427d5d into ClickHouse:master Feb 2, 2021
@vitlibar
Copy link
Member Author

vitlibar commented Feb 2, 2021

  1. test_materialize_mysql_database/test.py::test_select_without_columns_8_0 will be fixed in Fix flaky integration tests #19971

  2. AST fuzzer (UBSan)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior:
../src/AggregateFunctions/ReservoirSamplerDeterministic.h:59:31: runtime error: shift exponent 32 is too large for 32-bit type 'UInt32' (aka 'unsigned int')

../src/AggregateFunctions/ReservoirSamplerDeterministic.h:59:

class ReservoirSamplerDeterministic
{
    bool good(const UInt32 hash)
    {
        return hash == ((hash >> skip_degree) << skip_degree); /// <-- line 59
    }

#20879

  1. Stress test (thread)
WARNING: ThreadSanitizer: data race (pid=299)
  Write of size 4 at 0x7b580093a4c0 by thread T139 (mutexes: write M16829229):
    #0 DB::IMergeTreeDataPart::setState
Previous read of size 4 at 0x7b580093a4c0 by thread T739:
    #0 DB::IMergeTreeDataPart::stateString()

robot-clickhouse pushed a commit that referenced this pull request Feb 4, 2021
…default expressions containing dictGet()
robot-clickhouse pushed a commit that referenced this pull request Feb 4, 2021
…default expressions containing dictGet()
vitlibar pushed a commit that referenced this pull request Feb 5, 2021
…efault expressions containing dictGet()
vitlibar pushed a commit that referenced this pull request Feb 5, 2021
…default expressions containing dictGet()
vitlibar pushed a commit that referenced this pull request Feb 5, 2021
…default expressions containing dictGet()
vitlibar pushed a commit that referenced this pull request Feb 8, 2021
Backport #19805 to 20.12: Fix starting the server with tables having default expressions containing dictGet()
vitlibar pushed a commit that referenced this pull request Feb 8, 2021
Backport #19805 to 21.1: Fix starting the server with tables having default expressions containing dictGet()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force tests Force test ignoring fast test output. pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants