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.

Sign up for GitHub

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

Jump to bottom

ISSUES-2259 support truncate syntax #2260

Merged
merged 4 commits into from
Jun 9, 2018
Merged

ISSUES-2259 support truncate syntax #2260

merged 4 commits into from
Jun 9, 2018

Conversation

zhang2014
Copy link
Contributor

@zhang2014 zhang2014 commented Apr 22, 2018 •
edited
Loading

#2259

CheckList

  • StorageBuffer
  • StorageCatBoostPool
  • StorageDictionary
  • StorageDistributed
  • StorageFile
  • StorageKafka
  • StorageLog
  • StorageMaterializedView
  • StorageMemory
  • StorageMerge
  • StorageMergeTree
  • StorageMySQL
  • StorageNull
  • StorageODBC
  • StorageReplicatedMergeTree
  • StorageSet
  • StorageStripeLog
  • StorageTinyLog
  • StorageView
    I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

zhang2014
zhang2014 commented Apr 22, 2018
View reviewed changes
dbms/src/Storages/StorageMergeTree.cpp Outdated
@@ -126,6 +126,13 @@ void StorageMergeTree::drop()
data.dropAllData();
}

void StorageMergeTree::truncate()
{
shutdown();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexey-milovidov shutdown is synchronous? What would I do if it wasn't ? Because I need to know when the merge task is terminated.

Copy link
Member

Choose a reason for hiding this comment

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

It's synchronous.

@zhang2014 zhang2014 force-pushed the feature/support_truncate branch from 13cb9f3 to ba84566 Compare April 22, 2018 03:41
@alexey-milovidov
Copy link
Member

Please check for ReplicatedMergeTree tables. The implementation shouldn't be trivial for them.

@zhang2014 zhang2014 force-pushed the feature/support_truncate branch from ba84566 to 7c5784b Compare April 22, 2018 04:10
@zhang2014
Copy link
Contributor Author

The current idea is based on the ReplicatedMergeTreeLogEntryData::TRUNCATE_TABLE command

@zhang2014 zhang2014 force-pushed the feature/support_truncate branch from 8debac1 to f567adb Compare April 22, 2018 09:07
@alexey-milovidov alexey-milovidov requested a review from ztlpn April 23, 2018 01:03
@zhang2014 zhang2014 force-pushed the feature/support_truncate branch 6 times, most recently from a7b8106 to ad40fcf Compare April 24, 2018 12:41
@zhang2014 zhang2014 mentioned this pull request Apr 25, 2018
Merged
@zhang2014 zhang2014 force-pushed the feature/support_truncate branch 3 times, most recently from 00588d4 to f7fb7ab Compare May 2, 2018 01:32
@zhang2014 zhang2014 force-pushed the feature/support_truncate branch 5 times, most recently from fbdfa12 to 9db8f35 Compare May 5, 2018 16:06
@zhang2014 zhang2014 changed the title [WIP] ISSUES-2259 support truncate syntax ISSUES-2259 support truncate syntax May 5, 2018
@zhang2014 zhang2014 force-pushed the feature/support_truncate branch from b3e4aa4 to 2464f64 Compare May 5, 2018 17:02
@zhang2014
Copy link
Contributor Author

I'm done. Please help review the changes. @alexey-milovidov @ztlpn

@zhang2014 zhang2014 force-pushed the feature/support_truncate branch 3 times, most recently from 31c0789 to 8936d88 Compare May 5, 2018 18:13
alexey-milovidov
alexey-milovidov reviewed May 9, 2018
View reviewed changes
dbms/src/Common/ZooKeeper/Types.h Outdated
@@ -54,6 +54,9 @@ using SetResponse = ZooKeeperImpl::ZooKeeper::SetResponse;
using ListResponse = ZooKeeperImpl::ZooKeeper::ListResponse;
using CheckResponse = ZooKeeperImpl::ZooKeeper::CheckResponse;

template <typename R>
Copy link
Member

Choose a reason for hiding this comment

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

Missing comment.

alexey-milovidov
alexey-milovidov reviewed May 9, 2018
View reviewed changes
dbms/src/Common/ZooKeeper/Types.h Outdated
@@ -54,6 +54,9 @@ using SetResponse = ZooKeeperImpl::ZooKeeper::SetResponse;
using ListResponse = ZooKeeperImpl::ZooKeeper::ListResponse;
using CheckResponse = ZooKeeperImpl::ZooKeeper::CheckResponse;

template <typename R>
using MultiAsyncResponse = std::vector<std::pair<std::string, std::future<R>>>;
Copy link
Member

Choose a reason for hiding this comment

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

Bad name. It shouldn't mess up with "multi" operation in ZooKeeper.
More correct name: AsyncResponses.

@zhang2014 zhang2014 force-pushed the feature/support_truncate branch from 82bf137 to 3afb335 Compare May 21, 2018 05:27
alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Merging: Fixed half of problems #2260
d56c199
alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Merging: Fixed most of problems #2260
f3d8b4e
alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Merging: Fixed most of problems #2260
cfacdf1
alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Merging: Fixed most of problems, actualized test #2260
6475ea9
@alexey-milovidov alexey-milovidov merged commit 3afb335 into ClickHouse:master Jun 9, 2018
alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Databases/IDatabase.h
/// Delete metadata, the deletion of which differs from the recursive deletion of the directory, if any.
virtual void drop() = 0;
/// Delete database metadata, if exists.
virtual void drop(Context & context)
Copy link
Member

@alexey-milovidov alexey-milovidov Jun 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Method body should be in .cpp

This method is not quite correct, because only Ordinary databases have metadata for tables as files in metadata directory. Other databases doesn't require removal of metadata from filesystem.

Creating and removing database.sql files is not the responsibility of Database itself.
For example, database doesn't create this file, so it shouldn't remove it.

Another example: tables (Storages) are not responsible for creating and removing .sql files - it is the responsibility of Database. By the same principle, Database is not responsible for creating and removing its .sql files - this is the responsibility of caller.

If this method removes internal metadata directory, why it doesn't remove internal data directory?

Fixed.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Interpreters/ClusterProxy/TruncateStreamFactory.cpp
@@ -0,0 +1,68 @@
#include <Interpreters/ClusterProxy/TruncateStreamFactory.h>
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't proxy TRUNCATE query for Distributed table, because query ON CLUSTER already do it.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Interpreters/InterpreterDropQuery.cpp
String current_table_name = table.first->getTableName();

if (drop.detach)
else if (kind == ASTDropQuery::Kind::Truncate)
Copy link
Member

Choose a reason for hiding this comment

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

checkTableCanBeDropped should be called for truncate, because it is equally dangerous operation.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Parsers/ASTDropQuery.h
String getID() const override { return (detach ? "DetachQuery_" : "DropQuery_") + database + "_" + table; };
String getID() const override
{
if (kind == ASTDropQuery::Kind::Drop)
Copy link
Member

Choose a reason for hiding this comment

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

Non-trivial methods should be in .cpp.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Parsers/ASTDropQuery.h
<< (detach ? "DETACH TABLE " : "DROP TABLE ")
<< (if_exists ? "IF EXISTS " : "") << (settings.hilite ? hilite_none : "")
<< (!database.empty() ? backQuoteIfNeed(database) + "." : "") << backQuoteIfNeed(table);
settings.ostr << (settings.hilite ? hilite_keyword : "");
Copy link
Member

Choose a reason for hiding this comment

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

Copy-paste.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/IStorage.h
/** Delete the table data. Called before deleting the directory with the data.
* If you do not need any action other than deleting the directory with data, you can leave this method blank.
*/
virtual void truncate(const ASTPtr & /*query*/)
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a copy-paste of the above. This may lead to confusion.
I've totally rewrite this comment.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/MergeTree/MergeTreeData.cpp
@@ -797,7 +797,16 @@ void MergeTreeData::dropAllData()

LOG_TRACE(log, "dropAllData: removing data from filesystem.");

Poco::File(full_path).remove(true);
std::vector<Poco::File> data_dirs;
Copy link
Member

Choose a reason for hiding this comment

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

Motivation is unclear.

Copy link
Member

Choose a reason for hiding this comment

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

Missing comments.

alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Reverted unclear modification #2260
ebe5e3f
alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageDistributed.cpp
}
}

ClusterProxy::TruncateStreamFactory truncate_stream_factory(cluster);
Copy link
Member

Choose a reason for hiding this comment

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

I've removed this.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageMergeTree.cpp

void StorageMergeTree::truncate(const ASTPtr & /*query*/)
{
merger.merges_blocker.cancelForever();
Copy link
Member

Choose a reason for hiding this comment

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

How the merges are supposed to work after truncate?

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp

if (fake_part_name.empty())
if (dropBlocksInPartition(*zookeeper, partition_id, entry, detach))
Copy link
Member

Choose a reason for hiding this comment

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

dropParts, not Blocks.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
@@ -3808,14 +3802,18 @@ void ReplicatedMergeTreeMergeSelectingThread::clearState()
{
deduplicate = false; /// TODO: read deduplicate option from table config

uncached_merging_predicate = [this](const MergeTreeData::DataPartPtr & left, const MergeTreeData::DataPartPtr & right)
uncached_merging_predicate = [this](const MergeTreeData::DataPartPtr &left, const MergeTreeData::DataPartPtr &right)
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
{
return canMergePartsAccordingToZooKeeperInfo(left, right, storage->getZooKeeper(), storage->zookeeper_path, storage->data);
return canMergePartsAccordingToZooKeeperInfo(left,
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
};

merging_predicate_args_to_key = [](const MergeTreeData::DataPartPtr & left, const MergeTreeData::DataPartPtr & right)
merging_predicate_args_to_key = [](const MergeTreeData::DataPartPtr &left, const MergeTreeData::DataPartPtr &right)
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
{
return std::make_pair(left->name, right->name);
return std::make_pair(left->name, right->name);
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification. Wrong indentation.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
@@ -3824,11 +3822,49 @@ void ReplicatedMergeTreeMergeSelectingThread::clearState()

now = std::chrono::steady_clock::time_point();

can_merge = [&] (const MergeTreeData::DataPartPtr & left, const MergeTreeData::DataPartPtr & right, String *)
can_merge = [&](const MergeTreeData::DataPartPtr &left, const MergeTreeData::DataPartPtr &right, String *)
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification.

alexey-milovidov
alexey-milovidov reviewed Jun 9, 2018
View reviewed changes
dbms/src/Storages/StorageReplicatedMergeTree.cpp
return partsWillNotBeMergedOrDisabled(left, right, storage->queue)
&& cached_merging_predicate->get(now, uncached_merging_predicate, merging_predicate_args_to_key, left, right);
return partsWillNotBeMergedOrDisabled(left, right, storage->queue)
&& cached_merging_predicate->get(now, uncached_merging_predicate, merging_predicate_args_to_key, left, right);
Copy link
Member

Choose a reason for hiding this comment

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

Accidential modification.

alexey-milovidov added a commit that referenced this pull request Jun 9, 2018
@alexey-milovidov
Reverted unclear modification #2260
641076d
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Jun 9, 2018 •
edited
Loading

I would like to prefer different method of implementation:

There is default implementation for truncate, that simply do drop and create table under lock.
All tables will use default implementation except ReplicatedMergeTree.

Cons:

  • the lock will be in global scope, not for table scope;
  • metadata_modification_time will be updated;
  • this method is less atomic: the table may become dropped instead of truncated when server was died in the middle;

Pros:

  • simple implementation, almost no additional logic.

alexey-milovidov added a commit that referenced this pull request Jun 10, 2018
@alexey-milovidov
Removed wrong modification #2260
72af2b7
@alexey-milovidov
Copy link
Member

alexey-milovidov commented Jun 10, 2018 •
edited
Loading

@zhang2014

Thank you! This is definitely useful feature.
What to do further... For example, our users have a request to implement
CREATE OR REPLACE for tables and especially for views.

This should be done atomically (from the client point of view). It can be done under global (context) lock.

@zhang2014
Copy link
Contributor Author

zhang2014 commented Jun 11, 2018 •
edited
Loading

@alexey-milovidov Thank for you review, I have learned a lot.
If necessary, I will continue to follow up and fix these problems, But it may be a few days later, The TCP ClickHouse JDBC Driver we implemented will be released recently : )
cc @sundy-li

@alexey-milovidov alexey-milovidov mentioned this pull request Sep 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov left review comments

@ztlpn ztlpn Awaiting requested review from ztlpn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhang2014 @alexey-milovidov @ludv1x