Skip to content

Added aggregate function aggThrow#8446

Merged
alexey-milovidov merged 7 commits intomasterfrom
agg-throw
Dec 28, 2019
Merged

Added aggregate function aggThrow#8446
alexey-milovidov merged 7 commits intomasterfrom
agg-throw

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Dec 28, 2019

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

Changelog category (leave one):

  • Bugfix;

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):
Fixed error in function arrayReduce that may lead to "double free" and error in aggregate function combinator Resample that may lead to memory leak. Added aggregate function aggThrow. This function can be used for testing purposes.

@alexey-milovidov alexey-milovidov added the pr-build Pull request with build/testing/packaging improvement label Dec 28, 2019
@alexey-milovidov alexey-milovidov added the pr-bugfix Pull request with bugfix, not backported by default label Dec 28, 2019
catch (...)
{
agg_func.destroy(places[i]);
for (size_t j = 0; j < i; ++j)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice catch! I've also examined all sites of agg state construction and found that https://github.com/yandex/ClickHouse/blob/master/dbms/src/AggregateFunctions/AggregateFunctionResample.h#L102 needs a fix too

        for (size_t i = 0; i < total; ++i)
        {
            try
            {
                nested_function->create(place + i * size_of_data);
            }
            catch (...)
            {
                for (size_t j = 0; j < i; ++j)
                    nested_function->destroy(place + j * size_of_data);
                throw;
            }
        }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, I'll do it also.

@alexey-milovidov alexey-milovidov merged commit 95b43aa into master Dec 28, 2019
@alesapin
Copy link
Copy Markdown
Member

conflicts in 19.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-build Pull request with build/testing/packaging improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants