Better comments

src/DataStreams/IBlockInputStream.h

@@ -6,7 +6,7 @@
 #include <DataStreams/SizeLimits.h>
 #include <DataStreams/ExecutionSpeedLimits.h>
 #include <IO/Progress.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 #include <Common/TypePromotion.h>
 
 #include <atomic>

src/DataStreams/IBlockOutputStream.h

@@ -2,7 +2,7 @@
 
 #include <Core/Block.h>
 #include <DataStreams/IBlockStream_fwd.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 
 #include <boost/noncopyable.hpp>
 

src/Databases/IDatabase.h

@@ -236,7 +236,7 @@ public:
     using ASTModifier = std::function<void(IAST &)>;
 
     /// Change the table structure in metadata.
-    /// You must call under the TableStructureLock of the corresponding table . If engine_modifier is empty, then engine does not change.
+    /// You must call under the alter_lock of the corresponding table . If engine_modifier is empty, then engine does not change.
     virtual void alterTable(
         const Context & /*context*/,
         const StorageID & /*table_id*/,

src/Functions/FunctionJoinGet.h

@@ -1,6 +1,6 @@
 #include <Functions/IFunctionImpl.h>
 #include <Storages/IStorage_fwd.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 
 namespace DB
 {

src/Interpreters/InterpreterSelectQuery.h

@@ -10,7 +10,7 @@
 #include <Interpreters/IInterpreter.h>
 #include <Interpreters/SelectQueryOptions.h>
 #include <Storages/SelectQueryInfo.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 #include <Storages/ReadInOrderOptimizer.h>
 #include <Interpreters/StorageID.h>
 

src/Processors/QueryPipeline.h

@@ -7,7 +7,7 @@
 #include <DataStreams/IBlockOutputStream.h>
 
 #include <Storages/IStorage_fwd.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 
 namespace DB
 {

src/Processors/QueryPlan/ReadFromStorageStep.h

@@ -1,6 +1,6 @@
 #include <Processors/QueryPlan/IQueryPlanStep.h>
 #include <Core/QueryProcessingStage.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 #include <Interpreters/SelectQueryOptions.h>
 
 namespace DB

src/Storages/IStorage.h

@@ -8,7 +8,7 @@
 #include <Storages/IStorage_fwd.h>
 #include <Interpreters/StorageID.h>
 #include <Storages/SelectQueryInfo.h>
-#include <Storages/TableStructureLockHolder.h>
+#include <Storages/TableLockHolder.h>
 #include <Storages/CheckResults.h>
 #include <Storages/StorageInMemoryMetadata.h>
 #include <Storages/ColumnDependency.h>
@@ -138,10 +138,19 @@ public:
 
 public:
 
+    /// Get mutable version (snapshot) of storage metadata. Metadata object is
+    /// multiversion, so it can be concurrently chaged, but returned copy can be
+    /// used without any locks.
     StorageInMemoryMetadata getInMemoryMetadata() const { return *metadata.get(); }
 
+    /// Get immutable version (snapshot) of storage metadata. Metadata object is
+    /// multiversion, so it can be concurrently chaged, but returned copy can be
+    /// used without any locks.
     StorageMetadataPtr getInMemoryMetadataPtr() const { return metadata.get(); }
 
+    /// Update storage metadata. Used in ALTER or initialization of Storage.
+    /// Metadata object is multiversion, so this method can be called without
+    /// any locks.
     void setInMemoryMetadata(const StorageInMemoryMetadata & metadata_)
     {
         metadata.set(std::make_unique<StorageInMemoryMetadata>(metadata_));
@@ -171,16 +180,31 @@ private:
     StorageID storage_id;
     mutable std::mutex id_mutex;
 
+    /// Multiversion storage metadata. Allows to read/write storage metadata
+    /// without locks.
     MultiVersionStorageMetadataPtr metadata;
 private:
     RWLockImpl::LockHolder tryLockTimed(
         const RWLock & rwlock, RWLockImpl::Type type, const String & query_id, const SettingSeconds & acquire_timeout) const;
 
 public:
+    /// Lock table for share. This lock must be acuqired if you want to be sure,
+    /// that table will be not dropped while you holding this lock. It's used in
+    /// variety of cases starting from SELECT queries to background merges in
+    /// MergeTree.
     TableLockHolder lockForShare(const String & query_id, const SettingSeconds & acquire_timeout);
 
+    /// Lock table for alter. This lock must be acuqired in ALTER queries to be
+    /// sure, that we execute only one simultaneous alter. Doesn't affect share lock.
     TableLockHolder lockForAlter(const String & query_id, const SettingSeconds & acquire_timeout);
 
+    /// Lock table exclusively. This lock must be acuired if you want to be
+    /// sure, that no other thread (SELECT, merge, ALTER, etc.) doing something
+    /// with table. For example it allows to wait all threads before DROP or
+    /// truncate query.
+    ///
+    /// NOTE: You have to be 100% sure that you need this lock. It's extremely
+    /// heavyweight and makes table irresponsive.
     TableExclusiveLockHolder lockExclusively(const String & query_id, const SettingSeconds & acquire_timeout);
 
     /** Returns stage to which query is going to be processed in read() function.
@@ -247,7 +271,10 @@ public:
       * num_streams - a recommendation, how many streams to return,
       *  if the storage can return a different number of streams.
       *
-      * It is guaranteed that the structure of the table will not change over the lifetime of the returned streams (that is, there will not be ALTER, RENAME and DROP).
+      * metadata_snapshot is consistent snapshot of table metadata, it should be
+      * passed in all parts of the returned pipeline. Storage metadata can be
+      * changed during lifetime of the returned pipeline, but the snapshot is
+      * guaranteed to be immutable.
       */
     virtual Pipes read(
         const Names & /*column_names*/,
@@ -265,7 +292,10 @@ public:
       * Receives a description of the query, which can contain information about the data write method.
       * Returns an object by which you can write data sequentially.
       *
-      * It is guaranteed that the table structure will not change over the lifetime of the returned streams (that is, there will not be ALTER, RENAME and DROP).
+      * metadata_snapshot is consistent snapshot of table metadata, it should be
+      * passed in all parts of the returned streams. Storage metadata can be
+      * changed during lifetime of the returned streams, but the snapshot is
+      * guaranteed to be immutable.
       */
     virtual BlockOutputStreamPtr write(
         const ASTPtr & /*query*/,
@@ -284,7 +314,7 @@ public:
     virtual void drop() {}
 
     /** Clear the table data and leave it empty.
-      * Must be called under lockForAlter.
+      * Must be called under exclusive lock (lockExclusively).
       */
     virtual void truncate(
         const ASTPtr & /*query*/,
@@ -312,9 +342,8 @@ public:
      */
     virtual void renameInMemory(const StorageID & new_table_id);
 
-    /** ALTER tables in the form of column changes that do not affect the change to Storage or its parameters.
-      * This method must fully execute the ALTER query, taking care of the locks itself.
-      * To update the table metadata on disk, this method should call InterpreterAlterQuery::updateMetadata->
+    /** ALTER tables in the form of column changes that do not affect the change
+      * to Storage or its parameters. Executes under alter lock (lockForAlter).
       */
     virtual void alter(const AlterCommands & params, const Context & context, TableLockHolder & alter_lock_holder);
 
@@ -434,8 +463,17 @@ public:
     }
 
 private:
+    /// Lock required for alter queries (lockForAlter). Always taken for write
+    /// (actually can be replaced with std::mutex, but for consistency we use
+    /// RWLock). Allows to execute only one simultaneous alter query. Also it
+    /// should be taken by DROP-like queries, to be sure, that all alters are
+    /// finished.
     mutable RWLock alter_lock = RWLockImpl::create();
 
+    /// Lock required for drop queries. Every thread that want to ensure, that
+    /// table is not dropped have to tabke this lock for read (lockForShare).
+    /// DROP-like queries take this lock for write (lockExclusively), to be sure
+    /// that all table threads finished.
     mutable RWLock drop_lock = RWLockImpl::create();
 };
 

src/Storages/StorageInMemoryMetadata.h

@@ -14,6 +14,8 @@
 namespace DB
 {
 
+/// Common metadata for all storages. Contains all possible parts of CREATE
+/// query from all storages, but only some subset used.
 struct StorageInMemoryMetadata
 {
     /// Columns of table with their names, types,
@@ -46,12 +48,17 @@ struct StorageInMemoryMetadata
     StorageInMemoryMetadata(const StorageInMemoryMetadata & other);
     StorageInMemoryMetadata & operator=(const StorageInMemoryMetadata & other);
 
+    /// NOTE: Thread unsafe part. You should modify same StorageInMemoryMetadata
+    /// structure from different threads. It should be used as MultiVersion
+    /// object. See example in IStorage.
 
-    ////////////////////////////////////////////////////////////////////////
-    void setColumns(ColumnsDescription columns_); /// sets only real columns, possibly overwrites virtual ones.
+    /// Sets only real columns, possibly overwrites virtual ones.
+    void setColumns(ColumnsDescription columns_);
 
+    /// Sets secondary indices
     void setSecondaryIndices(IndicesDescription secondary_indices_);
 
+    /// Sets constraints
     void setConstraints(ConstraintsDescription constraints_);
 
     /// Set partition key for storage (methods bellow, are just wrappers for this
@@ -67,19 +74,28 @@ struct StorageInMemoryMetadata
     /// struct).
     void setSamplingKey(const KeyDescription & sampling_key_);
 
+    /// Set common table TTLs
     void setTableTTLs(const TTLTableDescription & table_ttl_);
 
+    /// TTLs for seperate columns
     void setColumnTTLs(const TTLColumnsDescription & column_ttls_by_name_);
 
+    /// Set settings changes in metadata (some settings exlicetely specified in
+    /// CREATE query)
     void setSettingsChanges(const ASTPtr & settings_changes_);
 
+    /// Set SELECT query for (Materialized)View
     void setSelectQuery(const SelectQueryDescription & select_);
 
-    const ColumnsDescription & getColumns() const; /// returns combined set of columns
+    /// Returns combined set of columns
+    const ColumnsDescription & getColumns() const;
+    /// Returns secondary indices
+
     const IndicesDescription & getSecondaryIndices() const;
     /// Has at least one non primary index
     bool hasSecondaryIndices() const;
 
+    /// Return table constraints
     const ConstraintsDescription & getConstraints() const;
 
     /// Returns true if there is set table TTL, any column TTL or any move TTL.
@@ -106,11 +122,24 @@ struct StorageInMemoryMetadata
     /// indices, TTL expressions) if we update @updated_columns set of columns.
     ColumnDependencies getColumnDependencies(const NameSet & updated_columns) const;
 
-    Block getSampleBlock() const; /// ordinary + materialized.
-    Block getSampleBlockNonMaterialized() const; /// ordinary.
-    Block getSampleBlockWithVirtuals(const NamesAndTypesList & virtuals) const; /// ordinary + materialized + virtuals.
+    /// Block with ordinary + materialized columns.
+    Block getSampleBlock() const;
+
+    /// Block with ordinary columns.
+    Block getSampleBlockNonMaterialized() const;
+
+    /// Block with ordinary + materialized + virtuals. Virtuals have to be
+    /// explicitely specified, because they are part of Storage type, not
+    /// Storage metadata.
+    Block getSampleBlockWithVirtuals(const NamesAndTypesList & virtuals) const;
+
+
+    /// Block with ordinary + materialized + aliases + virtuals. Virtuals have
+    /// to be explicitely specified, because they are part of Storage type, not
+    /// Storage metadata. StorageID required only for more clear exception
+    /// message.
     Block getSampleBlockForColumns(
-        const Names & column_names, const NamesAndTypesList & virtuals, const StorageID & storage_id) const; /// ordinary + materialized + aliases + virtuals.
+        const Names & column_names, const NamesAndTypesList & virtuals, const StorageID & storage_id) const;
 
     /// Returns structure with partition key.
     const KeyDescription & getPartitionKey() const;

src/Storages/StorageReplicatedMergeTree.h

@@ -329,7 +329,7 @@ private:
       * If no one has such a part, nothing checks.
       * Not very reliable: if two replicas add a part almost at the same time, no checks will occur.
       * Adds actions to `ops` that add data about the part into ZooKeeper.
-      * Call under TableStructureLock.
+      * Call under lockForShare.
       */
     void checkPartChecksumsAndAddCommitOps(const zkutil::ZooKeeperPtr & zookeeper, const DataPartPtr & part,
                                            Coordination::Requests & ops, String part_name = "", NameSet * absent_replicas_paths = nullptr);

src/Storages/TableLockHolder.h

@@ -5,6 +5,10 @@
 namespace DB
 {
 
+using TableLockHolder = RWLockImpl::LockHolder;
+
+/// Table exclusive lock, holds both alter and drop locks. Useful for DROP-like
+/// queries.
 struct TableExclusiveLockHolder
 {
     void release() { *this = TableExclusiveLockHolder(); }
@@ -13,9 +17,8 @@ private:
     friend class IStorage;
 
     /// Order is important.
-    RWLockImpl::LockHolder alter_lock;
-    RWLockImpl::LockHolder drop_lock;
+    TableLockHolder alter_lock;
+    TableLockHolder drop_lock;
 };
 
-using TableLockHolder = RWLockImpl::LockHolder;
 }