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
MaterializedPostgreSQL: Support "generated columns" and default values #57568
MaterializedPostgreSQL: Support "generated columns" and default values #57568
Conversation
This is an automated comment for commit 1e4d61d with description of existing statuses. It's updated for the latest CI running ⏳ Click here to open a full report in a separate page Successful checks
|
No related failures:
00180_no_seek_avoiding_when_reading_from_cache
00180_no_seek_avoiding_when_reading_from_cache
00180_no_seek_avoiding_when_reading_from_cache |
…-issue-with-generated-columns
@@ -195,7 +197,8 @@ void StorageMaterializedPostgreSQL::createNestedIfNeeded(PostgreSQLTableStructur | |||
const auto ast_create = getCreateNestedTableQuery(std::move(table_structure), table_override); | |||
auto table_id = getStorageID(); | |||
auto tmp_nested_table_id = StorageID(table_id.database_name, getNestedTableName()); | |||
LOG_DEBUG(log, "Creating clickhouse table for postgresql table {}", table_id.getNameForLogs()); | |||
LOG_DEBUG(log, "Creating clickhouse table for postgresql table {} (ast: {})", | |||
table_id.getNameForLogs(), serializeAST(*ast_create)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, we have IAST::formatForLogging
that formats ast with settings suitable for logging (oneline, hide secrets, etc)
Line 283 in efb82a9
String formatForLogging(size_t max_length = 0) const |
Doesn't make to much difference here, but just in case.
@@ -444,12 +447,16 @@ StorageInfo PostgreSQLReplicationHandler::loadFromSnapshot(postgres::Connection | |||
assertBlocksHaveEqualStructure(input->getPort().getHeader(), block_io.pipeline.getHeader(), "postgresql replica load from snapshot"); | |||
block_io.pipeline.complete(Pipe(std::move(input))); | |||
|
|||
/// TODO: make a test when we fail in the middle of inserting data from source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps introducing a fail point can make it easier to reproduce
ClickHouse/src/Common/FailPoint.cpp
Line 41 in efb82a9
REGULAR(dummy_failpoint) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, cool, did not know about that, thanks.
Though this comment can be removed, because a test to which this comment is referring was added in #57567.
columns_.begin(), columns_.end(), | ||
[](const auto & col) { return col.name == "_sign" || col.name == "_version"; })) | ||
{ | ||
throw Exception(ErrorCodes::LOGICAL_ERROR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't user create a columns with such names somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, he can of course, this code checks if they were added during ALTER query after MaterializedPostgreSQL table was created successfully as there. But it is written in doc that these column names are reserved for internal usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be another error code then LOGICAL_ERROR
that indicated bug in out code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point.
@@ -507,47 +549,94 @@ void MaterializedPostgreSQLConsumer::processReplicationMessage(const char * repl | |||
return; | |||
} | |||
|
|||
auto log_table_structure_changed = [&](std::string_view reason) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure, but it seems like there might be a potential issue with the lifetime of the temporary std::string object returned by fmt::format.
auto log_table_structure_changed = [&](std::string_view reason) | |
auto log_table_structure_changed = [&](const std::string & reason) |
Looks like it breaks |
columns.push_back(NameAndTypePair(std::get<0>(row), data_type)); | ||
columns.push_back(NameAndTypePair(column_name, data_type)); | ||
auto attgenerated = std::get<6>(row); | ||
LOG_TEST(&Poco::Logger::get("kssenii"), "KSSENII: attgenerated: {}", attgenerated); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug logging, I will remove it here - #58198
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New PR - #58237
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support PostgreSQL generated columns and default column values in
MaterializedPostgreSQL
. Closes #40449.