diff --git a/CHANGELOG.md b/CHANGELOG.md index d37c1f4d..64d5d633 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +### Release [1.7.1], TBD +#### Bug Fix +- It was possible for incremental models with the delete+insert strategy to fail if ClickHouse "light weight deletes" were +not enabled or the required setting `allow_nondetermistic_mutations` was not enabled and the user did not have permission +to apply it. This condition is now detected on startup, and an exception will be thrown if `use_lw_deletes` is configured +in the profile. Otherwise, a warning will be logged that incremental models will be slower (because such models will +be downgraded to use the `legacy` incremental strategy). This should prevent the confusing behavior in +https://github.com/ClickHouse/dbt-clickhouse/issues/197 by throwing an early exception for an unsupported configuration. + ### Release [1.7.0], 2023-12-07 #### Improvements - Minimal compatibility with dbt 1.7.x. The date_spine macro and additional automated tests have not been implemented, diff --git a/dbt/adapters/clickhouse/__version__.py b/dbt/adapters/clickhouse/__version__.py index 95381e51..1f796f9b 100644 --- a/dbt/adapters/clickhouse/__version__.py +++ b/dbt/adapters/clickhouse/__version__.py @@ -1 +1 @@ -version = '1.7.0' +version = '1.7.1' diff --git a/dbt/adapters/clickhouse/dbclient.py b/dbt/adapters/clickhouse/dbclient.py index bf6f3725..c693a82e 100644 --- a/dbt/adapters/clickhouse/dbclient.py +++ b/dbt/adapters/clickhouse/dbclient.py @@ -2,9 +2,15 @@ from abc import ABC, abstractmethod from typing import Dict -from dbt.exceptions import DbtDatabaseError, FailedToConnectError +from dbt.exceptions import DbtConfigError, DbtDatabaseError, FailedToConnectError from dbt.adapters.clickhouse.credentials import ClickHouseCredentials +from dbt.adapters.clickhouse.errors import ( + lw_deletes_not_enabled_error, + lw_deletes_not_enabled_warning, + nd_mutations_not_enabled_error, + nd_mutations_not_enabled_warning, +) from dbt.adapters.clickhouse.logger import logger from dbt.adapters.clickhouse.query import quote_identifier from dbt.adapters.clickhouse.util import compare_versions @@ -131,34 +137,42 @@ def update_model_settings(self, model_settings: Dict[str, str]): model_settings[key] = value def _check_lightweight_deletes(self, requested: bool): - lw_deletes = self.get_ch_setting(LW_DELETE_SETTING) - nd_mutations = self.get_ch_setting(ND_MUTATION_SETTING) + lw_deletes, lw_read_only = self.get_ch_setting(LW_DELETE_SETTING) + nd_mutations, nd_mutations_read_only = self.get_ch_setting(ND_MUTATION_SETTING) if lw_deletes is None or nd_mutations is None: if requested: - logger.warning( - 'use_lw_deletes requested but are not available on this ClickHouse server' - ) + logger.warning(lw_deletes_not_enabled_error) return False, False lw_deletes = int(lw_deletes) > 0 if not lw_deletes: - try: - self.command(f'SET {LW_DELETE_SETTING} = 1') - self._conn_settings[LW_DELETE_SETTING] = '1' - lw_deletes = True - except DbtDatabaseError: - pass + if lw_read_only: + lw_deletes = False + if requested: + raise DbtConfigError(lw_deletes_not_enabled_error) + logger.warning(lw_deletes_not_enabled_warning) + else: + try: + self.command(f'SET {LW_DELETE_SETTING} = 1') + self._conn_settings[LW_DELETE_SETTING] = '1' + lw_deletes = True + except DbtDatabaseError: + logger.warning(lw_deletes_not_enabled_warning) nd_mutations = int(nd_mutations) > 0 if lw_deletes and not nd_mutations: - try: - self.command(f'SET {ND_MUTATION_SETTING} = 1') - self._conn_settings[ND_MUTATION_SETTING] = '1' - nd_mutations = True - except DbtDatabaseError: - pass + if nd_mutations_read_only: + nd_mutations = False + if requested: + raise DbtConfigError(nd_mutations_not_enabled_error) + logger.warning(nd_mutations_not_enabled_warning) + else: + try: + self.command(f'SET {ND_MUTATION_SETTING} = 1') + self._conn_settings[ND_MUTATION_SETTING] = '1' + nd_mutations = True + except DbtDatabaseError: + logger.warning(nd_mutations_not_enabled_warning) if lw_deletes and nd_mutations: return True, requested - if requested: - logger.warning('use_lw_deletes requested but cannot enable on this ClickHouse server') return False, False def _ensure_database(self, database_engine, cluster_name) -> None: diff --git a/dbt/adapters/clickhouse/errors.py b/dbt/adapters/clickhouse/errors.py index 1d3b5c69..bfcd5f95 100644 --- a/dbt/adapters/clickhouse/errors.py +++ b/dbt/adapters/clickhouse/errors.py @@ -22,3 +22,24 @@ Source columns not in target: {0} """ + +lw_deletes_not_enabled_error = """ +Attempting to apply the configuration `use_lw_deletes` to enable the delete+insert incremental strategy, but +`light weight deletes` are either not available or not enabled on this ClickHouse server. +""" + +lw_deletes_not_enabled_warning = """ +`light weight deletes` are either not available or not enabled on this ClickHouse server. This prevents the use +of the delete+insert incremental strategy, which may negatively affect performance for incremental models. +""" + +nd_mutations_not_enabled_error = """ +Attempting to apply the configuration `use_lw_deletes` to enable the delete+insert incremental strategy, but +the required `allow_nondeterministic_mutations` is not enabled and is `read_only` for this user +""" + +nd_mutations_not_enabled_warning = """ +The setting `allow_nondeterministic_mutations` is not enabled and is `read_only` for this user` This prevents the use +of `light weight deletes` and therefore the delete+insert incremental strategy. This may negatively affect performance +for incremental models +""" diff --git a/dbt/adapters/clickhouse/httpclient.py b/dbt/adapters/clickhouse/httpclient.py index 6e074464..161d1256 100644 --- a/dbt/adapters/clickhouse/httpclient.py +++ b/dbt/adapters/clickhouse/httpclient.py @@ -35,7 +35,7 @@ def columns_in_query(self, sql: str, **kwargs) -> List[ClickHouseColumn]: def get_ch_setting(self, setting_name): setting = self._client.server_settings.get(setting_name) - return setting.value if setting else None + return (setting.value, setting.readonly) if setting else (None, 0) def database_dropped(self, database: str): # This is necessary for the http client to avoid exceptions when ClickHouse doesn't recognize the database diff --git a/dbt/adapters/clickhouse/nativeclient.py b/dbt/adapters/clickhouse/nativeclient.py index 6fbff418..d7532ef5 100644 --- a/dbt/adapters/clickhouse/nativeclient.py +++ b/dbt/adapters/clickhouse/nativeclient.py @@ -42,12 +42,12 @@ def columns_in_query(self, sql: str, **kwargs) -> List[ClickHouseColumn]: def get_ch_setting(self, setting_name): try: result = self._client.execute( - f"SELECT value FROM system.settings WHERE name = '{setting_name}'" + f"SELECT value, readonly FROM system.settings WHERE name = '{setting_name}'" ) except clickhouse_driver.errors.Error as ex: logger.warn('Unexpected error retrieving ClickHouse server setting', ex) return None - return result[0][0] if result else None + return (result[0][0], result[0][1]) if result else (None, 0) def close(self): self._client.disconnect()