MaterializedPostgreSQL: allow dynamically adding/deleting tables, altering settings by kssenii · Pull Request #28301 · ClickHouse/ClickHouse

kssenii · 2021-08-28T14:47:00Z

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

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support adding / deleting tables to replication from PostgreSQL dynamically in database engine MaterializedPostgreSQL. Support alter for database settings. Closes #27573.

…o materialized-postgresql

…alized-postgresql

alesapin

Need to fix potential race conditions.

alesapin · 2021-09-03T14:40:42Z

src/Databases/DatabaseAtomic.h

    String path_to_table_symlinks;
    String path_to_metadata_symlink;
    const UUID db_uuid;
+    ASTPtr storage_def;


Why database atomic now have storage? What is it?

Forgot to delete..

Hm, no, it is already deleted..

alesapin · 2021-09-03T14:43:04Z

src/Databases/IDatabase.h

+
+    virtual void modifySettingsMetadata(const SettingsChanges &, ContextPtr)
+    {
+        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Database engine {} does not support settings", getEngineName());


Doesn't support settings alter?

alesapin · 2021-09-03T14:46:25Z

src/Databases/IDatabase.h

+        throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Database engine {} does not support settings", getEngineName());
+    }
+
+    virtual void tryApplySettings(const SettingsChanges &, ContextPtr)


What is the difference between this method and previous? how modifySettingsMetadata differs from tryApplySettings? BTW, tryXXX usually returns bool.

alesapin · 2021-09-03T14:47:09Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

        const String & metadata_path_,
        UUID uuid_,
-        const ASTStorage * database_engine_define_,
+        ASTPtr storage_def_,


database_engine_define_ is much more clear, than storage_def.

alesapin · 2021-09-03T14:47:51Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+}
+
+
+void DatabaseMaterializedPostgreSQL::tryApplySettings(const SettingsChanges & settings_changes, ContextPtr local_context)


local_context -> query_context?

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

alesapin · 2021-09-06T09:02:37Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

 }


+String DatabaseMaterializedPostgreSQL::getTablesList(const String & except) const


Unclear method interface. Method return string, not list. What is except -- it's also comma-separated string?

alesapin · 2021-09-06T09:08:30Z

src/Interpreters/InterpreterAlterQuery.cpp

+    if (!alter_commands.empty())
+    {
+        database->checkAlterIsPossible(alter_commands, getContext());
+        alter_commands.apply(database, getContext());


It's better to leave AlterCommands as lightweight class which operates on AST's or some simple structures like StorageInMemoryMetadata. Taking IDatabase as argument is not the best solution.

alesapin · 2021-09-06T09:08:59Z

src/Storages/AlterCommands.cpp

+        {
+            if (command.type == AlterCommand::MODIFY_DATABASE_SETTING)
+            {
+                database->tryApplySettings(command.settings_changes, context);


What if database will be dropped here?

Changed the order of writing settings changes to disk and writing to postgresql replication flow.
Then if we crash in between, it will be Ok for this database engine because it always checks metadata on startup and makes according modifications. Though cannot say the same for other engines if they will also support settings changes..

alesapin · 2021-09-06T09:09:50Z

src/Storages/PostgreSQL/MaterializedPostgreSQLConsumer.cpp

-    : log(&Poco::Logger::get("PostgreSQLReaplicaConsumer"))
+    Storages storages_,
+    const String & name_for_logger)
+    : log(&Poco::Logger::get("PostgreSQLReplicaConsumer("+ name_for_logger +")"))


Suggested change

: log(&Poco::Logger::get("PostgreSQLReplicaConsumer("+ name_for_logger +")"))

: log(&Poco::Logger::get("PostgreSQLReplicaConsumer(" + name_for_logger + ")"))

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

…alized-postgresql

alesapin

Alter part is almost ok (expect overwrite), locking also ok. Barely understand postgres replication part, but looks like we create too many connections. Maybe we can have one shared connection for handler?

alesapin · 2021-09-13T10:35:39Z

src/Databases/DatabaseAtomic.cpp

 #include <Databases/DatabaseOnDisk.h>
 #include <IO/ReadHelpers.h>
 #include <IO/WriteHelpers.h>
+#include <IO/WriteBufferFromFile.h>


Is it required?

alesapin · 2021-09-13T10:38:38Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+            if (!query_context->isInternalQuery())
+                throw Exception(ErrorCodes::QUERY_NOT_ALLOWED, "Changing setting `{}` is not allowed", change.name);
+
+            DatabaseOnDisk::modifySettingsMetadata(settings_changes, query_context);


What if we changed 10 settings at one query? We will override whole metadata on disk 10 times?

alesapin · 2021-09-13T10:41:03Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+{
+    /// If there is query context then we need to detach materialized storage.
+    /// If there is no query context then we need to detach internal storage from atomic database.
+    if (CurrentThread::isInitialized() && CurrentThread::get().getQueryContext())


Maybe move this to a separate method?)

No, I think it is better here. It is rather compact.

alesapin · 2021-09-13T10:48:54Z

src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp

            try
            {
-                nested_storages[table_name] = loadFromSnapshot(snapshot_name, table_name, storage->as <StorageMaterializedPostgreSQL>());
+                nested_storages[table_name] = loadFromSnapshot(tmp_connection, snapshot_name, table_name, storage->as <StorageMaterializedPostgreSQL>());


Suggested change

nested_storages[table_name] = loadFromSnapshot(tmp_connection, snapshot_name, table_name, storage->as <StorageMaterializedPostgreSQL>());

nested_storages[table_name] = loadFromSnapshot(tmp_connection, snapshot_name, table_name, storage->as<StorageMaterializedPostgreSQL>());

alesapin · 2021-09-13T11:07:51Z

src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp

-StoragePtr PostgreSQLReplicationHandler::loadFromSnapshot(String & snapshot_name, const String & table_name,
+ASTPtr PostgreSQLReplicationHandler::getCreateNestedTableQuery(StorageMaterializedPostgreSQL * storage, const String & table_name)
+{
+    postgres::Connection connection(connection_info);


I see pqlib (and pqxx) for the first time. All connections will be correctly closed in destructors?

BTW, is it ok to create new connection each time?

I see pqlib (and pqxx) for the first time. All connections will be correctly closed in destructors?

yes, they will be closed.

BTW, is it ok to create new connection each time?

Yes, quite bad.
I cannot make one global connection for all methods in this replication handler becuase it is dangerous as they can be called concurrently.
A solution could be to add a connection pool here. But I am not sure it is for the better because all these connections are opened only while initial tables dump or when some non ordinary rare actions are made.
Overall in normal lifestyle of this engine (after initial synchronization is completed) there will be only 1 active connection at a time.

alesapin · 2021-09-13T11:14:16Z

src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp

            auto * materialized_storage = storage->as <StorageMaterializedPostgreSQL>();
            try
            {
+                /// FIXME: Looks like it is possible we might get here if there is no nested storage or at least nested storage id field might be empty.


Very scary.

Not very scary because "nested storage id might be empty" means not that there is no StorageID in some storage, but that a field nested_table_id in MaterializedPostgreSQL storage is std::nullopt.
Is is set in 2 cases:

At constructor -- if MaterializedPostgreSQL storage is created with a ready fully made nested storage passed into its constrcutor.

in storage->prepare() call -- right after materialized storage has created and saved nested storage inside itself.

I'll add this to a comment.

No I changed my mind..

Very scary.

Yes, it is scary.
(if it will happen, but I (want to) believe its not possible)

alesapin · 2021-09-13T11:18:30Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+
+        auto nested = table_to_delete->as<StorageMaterializedPostgreSQL>()->getNested();
+        if (!nested)
+            throw Exception(ErrorCodes::UNKNOWN_TABLE, "Inner table `{}` does not exist", table_name);


Looks like logical error? Or it can be dropped manually?

BTW, is it ok to create new connection each time?

yes, fixed.

Or it can be dropped manually?

no.

alesapin · 2021-09-13T11:33:30Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+        {
+            auto current_context = Context::createCopy(getContext()->getGlobalContext());
+            current_context->makeQueryContext();
+            DatabaseAtomic::dropTable(current_context, table_name, true);


But why we drop it in detach method?

Because postgres table can be removed from replication via detach only -- for consistency with attach (because we cannot add table to replication via create table because we do not know table structure).
So we call detach for materialized table and drop for inner table because this is the behaviour I need.

alesapin · 2021-09-13T11:36:28Z

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp

+
+        try
+        {
+            auto tables_to_replicate = settings->materialized_postgresql_tables_list.value;


I don't understand where the source of truth? Which tables we have in materialized_postgresql_tables_list which tables we have in materialized_tables?

I don't understand where the source of truth?

Sounds so philosophically...

p.s. discussed in dm that I need to add more comments.

alesapin · 2021-09-13T11:44:16Z

src/Databases/IDatabase.h

    /// Delete data and metadata stored inside the database, if exists.
    virtual void drop(ContextPtr /*context*/) {}

+    virtual void applyNewSettings(const SettingsChanges &, ContextPtr)


applySettingsChanges?

…alized-postgresql

alesapin

Ok

kssenii added 6 commits August 28, 2021 17:33

alter

2dfd5b1

Almost done

4cd6222

Dynamically add tables complete

1743400

Dynamically remove tables

a2b0996

Allow modify some other settings

fd62138

Slightly better

cc90d78

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Aug 28, 2021

kssenii added 2 commits August 28, 2021 19:51

Fix tests

378e1f7

Fix tests

9d04447

alesapin self-assigned this Sep 2, 2021

kssenii added 4 commits September 3, 2021 09:03

Merge branch 'master' of https://github.com/ClickHouse/ClickHouse int…

f17d1bc

…o materialized-postgresql

Fix

fbd2f40

Merge branch 'master' of github.com:ClickHouse/ClickHouse into materi…

d011374

…alized-postgresql

Minor fixes

76e4933

alesapin requested changes Sep 6, 2021

View reviewed changes

alesapin reviewed Sep 6, 2021

View reviewed changes

src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp Show resolved Hide resolved

kssenii added 4 commits September 8, 2021 22:52

Merge branch 'master' of github.com:ClickHouse/ClickHouse into materi…

797f858

…alized-postgresql

Review fixes

bed2688

Fix checks

f26da04

Merge branch 'master' of github.com:ClickHouse/ClickHouse into materi…

904cf74

…alized-postgresql

alesapin reviewed Sep 13, 2021

View reviewed changes

kssenii added 2 commits September 15, 2021 12:28

Merge branch 'master' of github.com:ClickHouse/ClickHouse into materi…

710c8d1

…alized-postgresql

Review fixes

489a92c

alesapin approved these changes Sep 20, 2021

View reviewed changes

alesapin merged commit a249dcc into ClickHouse:master Sep 20, 2021

		}


		void DatabaseMaterializedPostgreSQL::tryApplySettings(const SettingsChanges & settings_changes, ContextPtr local_context)

		}


		String DatabaseMaterializedPostgreSQL::getTablesList(const String & except) const

	: log(&Poco::Logger::get("PostgreSQLReplicaConsumer("+ name_for_logger +")"))
	: log(&Poco::Logger::get("PostgreSQLReplicaConsumer(" + name_for_logger + ")"))

	nested_storages[table_name] = loadFromSnapshot(tmp_connection, snapshot_name, table_name, storage->as <StorageMaterializedPostgreSQL>());
	nested_storages[table_name] = loadFromSnapshot(tmp_connection, snapshot_name, table_name, storage->as<StorageMaterializedPostgreSQL>());

Conversation

kssenii commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kssenii Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

kssenii commented Aug 28, 2021 •

edited

Loading

kssenii Sep 13, 2021 •

edited

Loading