From 9bd3f04c87d2513a6d386c619a5581a80760068a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:31:05 +0100 Subject: [PATCH 1/6] feat: create the Recall database automatically and detect views an older version left --- .../sqlbuild-cli/src/argv/command_table.json | 2 +- .../_helpers/recall/clickhouse_store.rs | 127 ++++++-- .../src/bindings/_helpers/recall/constants.rs | 2 + .../src/bindings/_helpers/recall/session.rs | 28 +- .../sqlbuild-recall/src/schema/constants.rs | 2 + .../schema/main/create_database_statement.rs | 16 + crates/sqlbuild-recall/src/schema/main/mod.rs | 2 + .../schema/main/store_version_with_views.rs | 36 +++ .../src/schema/tests/test_types.rs | 13 + .../src/schema/tests/test_version.rs | 128 +++++++- .../commands/main/clickhouse/_test_types.py | 33 ++ .../test_clickhouse_recall_setup.py | 290 ++++++++++++++++++ website/src/content/docs/docs/cli/recall.mdx | 9 +- .../docs/concepts/observability/recall.mdx | 44 +-- 14 files changed, 669 insertions(+), 63 deletions(-) create mode 100644 crates/sqlbuild-recall/src/schema/main/create_database_statement.rs create mode 100644 crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs create mode 100644 tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py diff --git a/crates/sqlbuild-cli/src/argv/command_table.json b/crates/sqlbuild-cli/src/argv/command_table.json index 0377f47be8..95a778e52a 100644 --- a/crates/sqlbuild-cli/src/argv/command_table.json +++ b/crates/sqlbuild-cli/src/argv/command_table.json @@ -7027,7 +7027,7 @@ "nargs": 0, "const": true, "default": false, - "help": "print the SQL that creates the Recall store or brings it up to this version", + "help": "print the SQL an administrator runs when the writer may not create or upgrade the Recall store", "mutex_group": 0, "group": 1 }, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs index 51f6942e87..6c1a2614dc 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs @@ -25,9 +25,12 @@ use sqlbuild_recall::schema::constants::{ }; use sqlbuild_recall::schema::main::add_column_statement::add_column_statement; use sqlbuild_recall::schema::main::clickhouse_relation::clickhouse_relation; +use sqlbuild_recall::schema::main::create_database_statement::create_database_statement; +use sqlbuild_recall::schema::main::ddl_statements::ddl_statements; use sqlbuild_recall::schema::main::render_script::render_script; use sqlbuild_recall::schema::main::retention_fix_statements::retention_fix_statements; use sqlbuild_recall::schema::main::schema_age::schema_age; +use sqlbuild_recall::schema::main::store_version_with_views::store_version_with_views; use sqlbuild_recall::schema::main::table_columns::table_columns; use sqlbuild_recall::schema::main::table_spec::table_spec; use sqlbuild_recall::schema::main::table_statements::table_statements; @@ -38,6 +41,7 @@ use sqlbuild_recall::schema::main::view_statements::view_statements; use sqlbuild_recall::schema::models::{ ColumnSpec, DdlOptions, Dialect, ObservedSchema, SchemaAge, StoreVersion, Table, VersionPlan, }; +use sqlbuild_recall::settings::constants::CLICKHOUSE_DATABASE_KEY; use sqlbuild_recall::store::constants::LAST_INVOCATION_SELECTOR; use sqlbuild_recall::store::errors::{StoreError, StoreErrorKind}; use sqlbuild_recall::store::models::{ @@ -49,7 +53,8 @@ use crate::bindings::_helpers::recall::clickhouse_calls::{ self as calls, ConnectionRole, QueryStatistics, StoreHandle, }; use crate::bindings::_helpers::recall::constants::{ - READ_ONLY_QUERIES, READONLY_SETTING, SECONDS_PER_DAY, UNKNOWN_TABLE_ERROR, + READ_ONLY_QUERIES, READONLY_SETTING, SECONDS_PER_DAY, SERVER_DEFAULT_DATABASE, + UNKNOWN_TABLE_ERROR, }; /// Where a ClickHouse store lives and how long it keeps history. @@ -96,6 +101,8 @@ pub(crate) struct ClickHouseStoreOpener { struct StoreState { tables: Vec<(String, String)>, views: HashSet, + /// The views' `CREATE` queries, which show what an older version wrote when columns cannot. + view_definitions: Vec, version: i64, /// Seconds since the newest version row was recorded. version_age_seconds: i64, @@ -158,10 +165,12 @@ impl ClickHouseStore { let relations: QueryResult = self.query(py, &sql.relations, "inspect the Recall store")?; let mut tables: Vec<(String, String)> = Vec::new(); let mut views: HashSet = HashSet::new(); + let mut view_definitions: Vec = Vec::new(); for row in &relations.rows { let name = text(row.first()); if integer(row.get(1)) == 1 { views.insert(name); + view_definitions.push(text(row.get(2))); } else { tables.push((name, text(row.get(2)))); } @@ -182,16 +191,29 @@ impl ClickHouseStore { Ok(StoreState { tables, views, + view_definitions, version, version_age_seconds, }) } - /// The recorded version, or without one how the tables, read by `read_columns`, and the views compare with this build's. + /// The recorded version, or without one how the tables, read by `read_columns`, and the views compare with this build's; either is lowered when a view definition shows an older version wrote it. fn store_version( &self, py: Python<'_>, state: &StoreState, + ) -> Result { + let definitions: Vec<&str> = state.view_definitions.iter().map(String::as_str).collect(); + Ok(store_version_with_views( + self.columns_version(py, state)?, + &definitions, + )) + } + + fn columns_version( + &self, + py: Python<'_>, + state: &StoreState, ) -> Result { if state.version > 0 { return Ok(StoreVersion::Recorded { @@ -235,7 +257,7 @@ impl ClickHouseStore { ))) } - fn require_database(&self, py: Python<'_>) -> Result<(), StoreError> { + fn database_exists(&self, py: Python<'_>) -> Result { let found = self.query( py, &format!( @@ -244,17 +266,35 @@ impl ClickHouseStore { ), "inspect the Recall store", )?; - if integer(found.rows.first().and_then(|row| row.first())) == 0 { - return Err(StoreError { - kind: StoreErrorKind::NotFound, - message: format!( - "ClickHouse database `{}` does not exist; Recall creates its tables only in an \ - existing database", + Ok(integer(found.rows.first().and_then(|row| row.first())) > 0) + } + + /// Create the configured database when it is missing, saying so; an existing one, whatever its engine, is left alone. A user who may not create it gets the statement to hand to an administrator. + fn ensure_database(&mut self, py: Python<'_>) -> Result<(), StoreError> { + if self.database_exists(py)? { + return Ok(()); + } + let Some(statement) = create_database_statement(&self.location.options) else { + return Ok(()); + }; + match self.command(py, &statement, "create the Recall database") { + Ok(()) => { + self.notices.push(format!( + "created ClickHouse database `{}` for the store", self.location.database() + )); + Ok(()) + } + Err(error) if error.kind == StoreErrorKind::Permission => Err(StoreError { + kind: StoreErrorKind::Permission, + message: format!( + "{} ({})", + missing_database_reason(self.location.database()), + error.message ), - }); + }), + Err(error) => Err(error), } - Ok(()) } fn read_columns(&mut self, py: Python<'_>) -> Result<(), StoreError> { @@ -372,7 +412,7 @@ impl ClickHouseStore { envelope: &RowEnvelope, views: &HashSet, ) -> Result<(), StoreError> { - self.require_database(py)?; + self.ensure_database(py)?; for statement in table_statements(Dialect::ClickHouse, &self.location.options) { self.command(py, &statement, "create the Recall tables")?; } @@ -591,11 +631,7 @@ impl RecallStoreOpener for ClickHouseStoreOpener { Python::attach(|py| { let mut store = ClickHouseStore { location: self.location.clone(), - connection: Some(calls::connect( - py, - (&self.location.config, ConnectionRole::Writer), - &self.location.connect_action(), - )?), + connection: Some(connect_store(py, &self.location, ConnectionRole::Writer)?), columns: HashMap::new(), notices: Vec::new(), }; @@ -683,15 +719,25 @@ impl ClickHouseReader { ) -> Result, StoreError> { let mut store = ClickHouseStore { location: location.clone(), - connection: Some(calls::connect( - py, - (&location.config, ConnectionRole::Reader), - &location.connect_action(), - )?), + connection: Some(connect_store(py, location, ConnectionRole::Reader)?), columns: HashMap::new(), notices: Vec::new(), }; - let observed = store.require_database(py).and_then(|()| { + match store.database_exists(py) { + Ok(true) => {} + Ok(false) => { + store.close(); + return Ok(create_database_statement(&location.options) + .into_iter() + .chain(ddl_statements(Dialect::ClickHouse, &location.options)) + .collect()); + } + Err(error) => { + store.close(); + return Err(error); + } + } + let observed = { let state: StoreState = store.state(py)?; store.read_columns(py)?; let version: i64 = if state.tables.len() < ALL_TABLES.len() { @@ -700,7 +746,7 @@ impl ClickHouseReader { observed_version(store.store_version(py, &state)?) }; Ok(store.observed(&state.views, version)) - }); + }; store.close(); Ok(upgrade_statements( Dialect::ClickHouse, @@ -925,6 +971,39 @@ impl ClickHouseReader { } } +/// Why a store cannot be created when its database is missing and the writer may not create it. +pub(crate) fn missing_database_reason(database: &str) -> String { + format!("ClickHouse database `{database}` does not exist, and this user may not create it") +} + +/// Connect for `role`. When the configured database does not exist yet, connect without it, so that it can be created or reported; every statement names the database itself. +fn connect_store( + py: Python<'_>, + location: &ClickHouseLocation, + role: ConnectionRole, +) -> Result { + match calls::connect(py, (&location.config, role), &location.connect_action()) { + Err(error) if error.kind == StoreErrorKind::NotFound => { + let config: Vec<(String, ConfigValue)> = location + .config + .iter() + .map(|(key, value)| { + if key == CLICKHOUSE_DATABASE_KEY { + ( + key.clone(), + ConfigValue::String(SERVER_DEFAULT_DATABASE.to_owned()), + ) + } else { + (key.clone(), value.clone()) + } + }) + .collect(); + calls::connect(py, (&config, role), &location.connect_action()) + } + connected => connected, + } +} + /// The version `upgrade_statements` compares with: a store without a version row whose views are current or newer needs no view replacement. fn observed_version(store: StoreVersion) -> i64 { match store { diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/constants.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/constants.rs index 2b15714f83..b8c8b12949 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/constants.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/constants.rs @@ -15,6 +15,8 @@ pub(crate) const HOST_TOKEN_LENGTH: usize = 16; pub(crate) const MINIMUM_HEARTBEAT_SECONDS: f64 = 0.01; /// What a Recall closing notice says is the only thing affected. pub(crate) const RECALL_HISTORY: &str = "Recall history"; +/// The database the ClickHouse client takes as none, so the server uses the user's default: a store whose database is missing connects with it to create or report the database. +pub(crate) const SERVER_DEFAULT_DATABASE: &str = "__default__"; /// The compute-log stream index of stdout; every other output index is stderr. pub(crate) const STDOUT_STREAM_INDEX: usize = 0; pub(crate) const RECALL_DISABLED_CODE: &str = "C970"; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs index 9d1d0277bc..26dc6e54ef 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs @@ -12,7 +12,9 @@ use sqlbuild_host::entries::models::ExecutorHost; use sqlbuild_recall::records::main::format_timestamp::format_timestamp; use sqlbuild_recall::records::main::sha256_hex::sha256_hex; use sqlbuild_recall::records::models::{InvocationContext, OutputStream, RowEnvelope}; -use sqlbuild_recall::schema::main::ddl_script::ddl_script; +use sqlbuild_recall::schema::main::create_database_statement::create_database_statement; +use sqlbuild_recall::schema::main::ddl_statements::ddl_statements; +use sqlbuild_recall::schema::main::render_script::render_script; use sqlbuild_recall::schema::models::{DdlOptions, Dialect}; use sqlbuild_recall::settings::main::resolve_recall_settings::resolve_recall_settings; use sqlbuild_recall::settings::models::{ @@ -31,7 +33,7 @@ use sqlbuild_recall::writer::models::{ use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::output_capture::records::invocation_context; use crate::bindings::_helpers::recall::clickhouse_store::{ - ClickHouseLocation, ClickHouseStoreOpener, + ClickHouseLocation, ClickHouseStoreOpener, missing_database_reason, }; use crate::bindings::_helpers::recall::console::write_stderr; use crate::bindings::_helpers::recall::constants::{ @@ -289,13 +291,21 @@ fn permission_notice( if !notice_due(project_dir, &place) { return None; } - let script = ddl_script( - dialect, - &DdlOptions { - database, - retention_days: settings.retention_days, - cluster: None, - }, + let options = DdlOptions { + database, + retention_days: settings.retention_days, + cluster: None, + }; + let database_missing: bool = options + .database + .as_deref() + .is_some_and(|database| reason.starts_with(&missing_database_reason(database))); + let script = render_script( + &create_database_statement(&options) + .filter(|_| database_missing) + .into_iter() + .chain(ddl_statements(dialect, &options)) + .collect::>(), ); Some(format!( "Recall history is not being written{}: {reason}. An administrator can create the \ diff --git a/crates/sqlbuild-recall/src/schema/constants.rs b/crates/sqlbuild-recall/src/schema/constants.rs index 7a4976da9f..5d95836afd 100644 --- a/crates/sqlbuild-recall/src/schema/constants.rs +++ b/crates/sqlbuild-recall/src/schema/constants.rs @@ -4,6 +4,8 @@ use crate::schema::models::{ColumnSpec, ColumnType, Table, TableSpec}; /// The Recall schema version this build writes. Raise it when a column or view is added. pub const RECALL_SCHEMA_VERSION: i64 = 3; +/// Text that only older schema versions wrote into ClickHouse view definitions, with the last version that wrote it. Version 3 changed only view bodies, so view columns cannot tell version 2 views, which set `join_use_nulls`, from current ones. +pub const SUPERSEDED_CLICKHOUSE_VIEW_MARKERS: [(&str, i64); 1] = [("join_use_nulls", 2)]; /// The oldest SQLite library the store's views run on (ordered `group_concat`). pub const MINIMUM_SQLITE_VERSION: (u32, u32) = (3, 44); /// The `recall_meta` component of schema rows. diff --git a/crates/sqlbuild-recall/src/schema/main/create_database_statement.rs b/crates/sqlbuild-recall/src/schema/main/create_database_statement.rs new file mode 100644 index 0000000000..03a0136b46 --- /dev/null +++ b/crates/sqlbuild-recall/src/schema/main/create_database_statement.rs @@ -0,0 +1,16 @@ +//! `create_database_statement`: the statement that creates a ClickHouse store's database. + +use crate::schema::_helpers::rendering::{clickhouse_identifier, on_cluster}; +use crate::schema::models::DdlOptions; + +/// `CREATE DATABASE IF NOT EXISTS` for the configured database, which leaves an existing database and its engine alone; `None` without one. +#[must_use] +pub fn create_database_statement(options: &DdlOptions) -> Option { + options.database.as_deref().map(|database| { + format!( + "CREATE DATABASE IF NOT EXISTS {}{}", + clickhouse_identifier(database), + on_cluster(options) + ) + }) +} diff --git a/crates/sqlbuild-recall/src/schema/main/mod.rs b/crates/sqlbuild-recall/src/schema/main/mod.rs index eb5220048f..e2251bd838 100644 --- a/crates/sqlbuild-recall/src/schema/main/mod.rs +++ b/crates/sqlbuild-recall/src/schema/main/mod.rs @@ -1,11 +1,13 @@ pub mod add_column_statement; pub mod clickhouse_relation; +pub mod create_database_statement; pub mod ddl_script; pub mod ddl_statements; pub mod render_script; pub mod retention_fix_statements; pub mod schema_age; pub mod sqlite_retention_sql; +pub mod store_version_with_views; pub mod table_columns; pub mod table_spec; pub mod table_statements; diff --git a/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs b/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs new file mode 100644 index 0000000000..f37350883c --- /dev/null +++ b/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs @@ -0,0 +1,36 @@ +//! `store_version_with_views`: a ClickHouse store's version, corrected by its view definitions. + +use crate::schema::constants::{RECALL_SCHEMA_VERSION, SUPERSEDED_CLICKHOUSE_VIEW_MARKERS}; +use crate::schema::models::{SchemaAge, StoreVersion}; + +/// `store`, made older when a view definition still holds text that only an older schema version wrote: neither a version row nor view columns prove that the views were replaced. A store recorded or judged newer than this build is left alone, so its views are never downgraded. +#[must_use] +pub fn store_version_with_views(store: StoreVersion, view_definitions: &[&str]) -> StoreVersion { + let Some(views_version) = SUPERSEDED_CLICKHOUSE_VIEW_MARKERS + .iter() + .filter(|(marker, _)| { + view_definitions + .iter() + .any(|definition| definition.contains(marker)) + }) + .map(|(_, version)| *version) + .min() + else { + return store; + }; + match store { + StoreVersion::Recorded { + version, + age_seconds, + views_visible, + } if version > views_version && version <= RECALL_SCHEMA_VERSION => { + StoreVersion::Recorded { + version: views_version, + age_seconds, + views_visible, + } + } + StoreVersion::Unrecorded(SchemaAge::Current) => StoreVersion::Unrecorded(SchemaAge::Older), + other => other, + } +} diff --git a/crates/sqlbuild-recall/src/schema/tests/test_types.rs b/crates/sqlbuild-recall/src/schema/tests/test_types.rs index 00dc8d54c6..7ff543cef8 100644 --- a/crates/sqlbuild-recall/src/schema/tests/test_types.rs +++ b/crates/sqlbuild-recall/src/schema/tests/test_types.rs @@ -49,3 +49,16 @@ pub(crate) struct ViewColumnsTestCase { pub(crate) view: &'static str, pub(crate) expected_same: bool, } + +pub(crate) struct ViewDefinitionsTestCase { + pub(crate) description: &'static str, + pub(crate) store: crate::schema::models::StoreVersion, + pub(crate) definitions: &'static [&'static str], + pub(crate) expected_store: crate::schema::models::StoreVersion, +} + +pub(crate) struct CreateDatabaseTestCase { + pub(crate) description: &'static str, + pub(crate) options: crate::schema::models::DdlOptions, + pub(crate) expected_statement: Option<&'static str>, +} diff --git a/crates/sqlbuild-recall/src/schema/tests/test_version.rs b/crates/sqlbuild-recall/src/schema/tests/test_version.rs index 09f50ae617..64ebebb75f 100644 --- a/crates/sqlbuild-recall/src/schema/tests/test_version.rs +++ b/crates/sqlbuild-recall/src/schema/tests/test_version.rs @@ -1,20 +1,29 @@ use std::collections::BTreeMap; use crate::schema::_helpers::clickhouse_views::clickhouse_view_bodies; -use crate::schema::constants::{RECALL_SCHEMA_VERSION, VIEW_NAMES}; +use crate::schema::constants::{ + RECALL_SCHEMA_VERSION, SUPERSEDED_CLICKHOUSE_VIEW_MARKERS, VIEW_NAMES, +}; +use crate::schema::main::create_database_statement::create_database_statement; use crate::schema::main::schema_age::schema_age; +use crate::schema::main::store_version_with_views::store_version_with_views; use crate::schema::main::version_plan::version_plan; use crate::schema::main::view_columns::view_columns; -use crate::schema::models::{SchemaAge, StoreVersion, VersionPlan}; +use crate::schema::main::view_statements::view_statements; +use crate::schema::models::{DdlOptions, Dialect, SchemaAge, StoreVersion, VersionPlan}; use crate::schema::tests::helpers::{ changed_statements_view, clickhouse_view_columns, current_views, options, }; use crate::schema::tests::test_types::{ - SchemaAgeTestCase, VersionPlanTestCase, ViewColumnsTestCase, + CreateDatabaseTestCase, SchemaAgeTestCase, VersionPlanTestCase, ViewColumnsTestCase, + ViewDefinitionsTestCase, }; const RETENTION_DAYS: u32 = 30; const DAY: i64 = 86_400; +const PREVIOUS_VIEW: &str = + "CREATE VIEW observability.statements AS SELECT 1 AS x SETTINGS join_use_nulls = 1"; +const CURRENT_VIEW: &str = "CREATE VIEW observability.statements AS SELECT 1 AS x"; #[test] fn given_stored_view_columns_when_compared_then_the_store_is_older_current_or_newer() { @@ -247,3 +256,116 @@ fn given_each_view_when_both_dialects_are_built_then_they_expose_the_same_column ); } } + +#[test] +fn given_view_definitions_when_versioned_then_views_an_older_version_wrote_make_the_store_older() { + let recorded = |version: i64| StoreVersion::Recorded { + version, + age_seconds: DAY, + views_visible: true, + }; + let test_cases = [ + ViewDefinitionsTestCase { + description: "previous views with no version row are older, though columns match", + store: StoreVersion::Unrecorded(SchemaAge::Current), + definitions: &[CURRENT_VIEW, PREVIOUS_VIEW], + expected_store: StoreVersion::Unrecorded(SchemaAge::Older), + }, + ViewDefinitionsTestCase { + description: "previous views recorded as current count as the previous version", + store: recorded(RECALL_SCHEMA_VERSION), + definitions: &[PREVIOUS_VIEW], + expected_store: recorded(RECALL_SCHEMA_VERSION - 1), + }, + ViewDefinitionsTestCase { + description: "current views keep the recorded version", + store: recorded(RECALL_SCHEMA_VERSION), + definitions: &[CURRENT_VIEW], + expected_store: recorded(RECALL_SCHEMA_VERSION), + }, + ViewDefinitionsTestCase { + description: "current views with no version row stay current", + store: StoreVersion::Unrecorded(SchemaAge::Current), + definitions: &[CURRENT_VIEW], + expected_store: StoreVersion::Unrecorded(SchemaAge::Current), + }, + ViewDefinitionsTestCase { + description: "a store recorded by a newer build is never made older", + store: recorded(RECALL_SCHEMA_VERSION + 1), + definitions: &[PREVIOUS_VIEW], + expected_store: recorded(RECALL_SCHEMA_VERSION + 1), + }, + ViewDefinitionsTestCase { + description: "views with columns of a newer build are never made older", + store: StoreVersion::Unrecorded(SchemaAge::Newer), + definitions: &[PREVIOUS_VIEW], + expected_store: StoreVersion::Unrecorded(SchemaAge::Newer), + }, + ]; + + for test_case in test_cases { + assert_eq!( + store_version_with_views(test_case.store, test_case.definitions), + test_case.expected_store, + "{}", + test_case.description + ); + } +} + +#[test] +fn given_this_builds_clickhouse_views_when_checked_then_none_holds_a_superseded_marker() { + let statements: Vec = view_statements(Dialect::ClickHouse, &options(None)) + .into_iter() + .map(|(_, statement)| statement) + .collect(); + let marked: Vec<&str> = SUPERSEDED_CLICKHOUSE_VIEW_MARKERS + .iter() + .map(|(marker, _)| *marker) + .filter(|marker| { + statements + .iter() + .any(|statement| statement.contains(marker)) + }) + .collect(); + + assert_eq!( + (statements.len(), marked), + (VIEW_NAMES.len(), Vec::<&str>::new()) + ); +} + +#[test] +fn given_a_configured_database_when_rendered_then_it_is_created_only_if_missing() { + let test_cases = [ + CreateDatabaseTestCase { + description: "a single server creates the database if it is missing", + options: options(None), + expected_statement: Some("CREATE DATABASE IF NOT EXISTS `observability`"), + }, + CreateDatabaseTestCase { + description: "a cluster creates it on every node", + options: options(Some("main")), + expected_statement: Some( + "CREATE DATABASE IF NOT EXISTS `observability` ON CLUSTER `main`", + ), + }, + CreateDatabaseTestCase { + description: "no configured database needs no statement", + options: DdlOptions { + database: None, + ..options(None) + }, + expected_statement: None, + }, + ]; + + for test_case in test_cases { + assert_eq!( + create_database_statement(&test_case.options).as_deref(), + test_case.expected_statement, + "{}", + test_case.description + ); + } +} diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py index 5cb2a1152f..e6a82342f6 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py @@ -299,3 +299,36 @@ class ClickHouseRecallReaderSettingsTestCase: description: str user_settings: str expected_rows: list[list[object]] + + +@dataclass(frozen=True) +class ClickHouseRecallDatabaseCreationTestCase: + description: str + writer_privileges: str | None + expected_notice: str + expected_invocations: list[list[object]] + + +@dataclass(frozen=True) +class ClickHouseRecallExistingDatabaseTestCase: + description: str + expected_database_creations: tuple[tuple[object, ...], ...] + + +@dataclass(frozen=True) +class ClickHouseRecallMissingDatabaseTestCase: + description: str + writer_privileges: str + expected_fragments: tuple[str, ...] + expected_databases: tuple[tuple[object, ...], ...] + expected_invocations: list[list[object]] + + +@dataclass(frozen=True) +class ClickHouseRecallStaleViewsTestCase: + description: str + version_row: str + limited_writer: bool + expected_notices: int + expected_stale_views: tuple[tuple[object, ...], ...] + expected_version: tuple[tuple[object, ...], ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py new file mode 100644 index 0000000000..7125643583 --- /dev/null +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py @@ -0,0 +1,290 @@ +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse._test_types import ( + ClickHouseRecallDatabaseCreationTestCase, + ClickHouseRecallExistingDatabaseTestCase, + ClickHouseRecallMissingDatabaseTestCase, + ClickHouseRecallStaleViewsTestCase, +) +from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse.helpers import ( + apply_printed_ddl, + create_previous_views, + execute_clickhouse_sql, + fetch_clickhouse_rows, + limited_user, + write_recall_project, +) +from tests.integration.src.sqlbuild.cli.recall.helpers import recall_rows, run + +_STORE_MANAGER_PRIVILEGES: str = ( + "CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP TABLE, ALTER ADD COLUMN, SELECT, INSERT" +) +_CLOSING_STATUS: str = "sqb build completed successfully; only its Recall history is affected.\n" + + +def _drop_database(*, config: dict[str, object], database: str) -> None: + execute_clickhouse_sql(config=config, statements=(f"DROP DATABASE {database} SYNC",)) + + +def _stale_views(*, config: dict[str, object], database: str) -> tuple[tuple[object, ...], ...]: + return fetch_clickhouse_rows( + config=config, + sql="SELECT countIf(position(create_table_query, 'join_use_nulls') > 0) " + f"FROM system.tables WHERE database = '{database}' AND engine = 'View'", + ) + + +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallDatabaseCreationTestCase( + description="an administrator's first write creates the database and says so", + writer_privileges=None, + expected_notice="Recall: created ClickHouse database `{database}` for the store.\n", + expected_invocations=[[2]], + ), + ClickHouseRecallDatabaseCreationTestCase( + description="a writer granted the documented store rights creates it too", + writer_privileges=_STORE_MANAGER_PRIVILEGES, + expected_notice="Recall: created ClickHouse database `{database}` for the store.\n", + expected_invocations=[[2]], + ), + ], + ids=lambda case: case.description, +) +def test_given_a_missing_database_when_a_writer_may_create_it_then_recall_creates_the_store( + test_case: ClickHouseRecallDatabaseCreationTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + _drop_database(config=clickhouse_e2e_config, database=recall_database) + notice: str = test_case.expected_notice.format(database=recall_database) + + if test_case.writer_privileges is None: + project_dir: Path = write_recall_project( + parent=tmp_path, config=clickhouse_e2e_config, database=recall_database + ) + first: subprocess.CompletedProcess[bytes] = run(project_dir, "build") + second: subprocess.CompletedProcess[bytes] = run(project_dir, "build") + else: + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: + project_dir = write_recall_project( + parent=tmp_path, config=writer, database=recall_database + ) + first = run(project_dir, "build") + second = run(project_dir, "build") + + stderr: str = first.stderr.decode(errors="replace") + assert (first.returncode, second.returncode) == (0, 0), stderr + assert stderr.count(notice) == 1 + assert stderr.endswith(notice + _CLOSING_STATUS) + assert b"Recall" not in second.stderr + assert ( + recall_rows( + write_recall_project( + parent=tmp_path / "admin", config=clickhouse_e2e_config, database=recall_database + ), + "SELECT count() FROM invocations WHERE command = 'build'", + ) + == test_case.expected_invocations + ) + + +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallExistingDatabaseTestCase( + description="an existing database is used without any CREATE DATABASE", + expected_database_creations=((0,),), + ), + ], + ids=lambda case: case.description, +) +def test_given_an_existing_database_when_building_then_recall_leaves_the_database_alone( + test_case: ClickHouseRecallExistingDatabaseTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + project_dir: Path = write_recall_project( + parent=tmp_path, config=clickhouse_e2e_config, database=recall_database + ) + + built: subprocess.CompletedProcess[bytes] = run(project_dir, "build") + + execute_clickhouse_sql(config=clickhouse_e2e_config, statements=("SYSTEM FLUSH LOGS",)) + assert built.returncode == 0, built.stderr + assert b"Recall" not in built.stderr + assert ( + fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql="SELECT count() FROM system.query_log WHERE type = 'QueryStart' " + f"AND query ILIKE 'CREATE DATABASE IF NOT EXISTS%{recall_database}%'", + ) + == test_case.expected_database_creations + ) + + +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallMissingDatabaseTestCase( + description="a SELECT, INSERT writer gets the exact statements, then writes once run", + writer_privileges="SELECT, INSERT", + expected_fragments=( + "Recall history is not being written for target `dev`: ClickHouse database " + "`{database}` does not exist, and this user may not create it", + "with:\n\nCREATE DATABASE IF NOT EXISTS `{database}`;\n\n" + "CREATE TABLE IF NOT EXISTS `{database}`.`recall_meta`", + "CREATE OR REPLACE VIEW `{database}`.`invocations`", + _CLOSING_STATUS, + ), + expected_databases=((0,),), + expected_invocations=[[1]], + ), + ], + ids=lambda case: case.description, +) +def test_given_a_missing_database_when_the_writer_may_not_create_it_then_the_statement_is_printed( + test_case: ClickHouseRecallMissingDatabaseTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + _drop_database(config=clickhouse_e2e_config, database=recall_database) + + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: + project_dir: Path = write_recall_project( + parent=tmp_path, config=writer, database=recall_database + ) + refused: subprocess.CompletedProcess[bytes] = run(project_dir, "build") + printed: subprocess.CompletedProcess[bytes] = run(project_dir, "recall", "schema", "--ddl") + databases: tuple[tuple[object, ...], ...] = fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql=f"SELECT count() FROM system.databases WHERE name = '{recall_database}'", + ) + apply_printed_ddl( + config=clickhouse_e2e_config, stderr=refused.stderr.decode(errors="replace") + ) + written: subprocess.CompletedProcess[bytes] = run(project_dir, "build") + invocations: list[list[object]] = recall_rows( + project_dir, "SELECT count() FROM invocations WHERE command = 'build'" + ) + + stderr: str = refused.stderr.decode(errors="replace") + assert refused.returncode == 0, stderr + assert [ + fragment.format(database=recall_database) in stderr + for fragment in test_case.expected_fragments + ] == [True for _ in test_case.expected_fragments] + assert printed.returncode == 0, printed.stderr + assert printed.stdout.decode().startswith( + f"CREATE DATABASE IF NOT EXISTS `{recall_database}`;\n" + ) + assert databases == test_case.expected_databases + assert written.returncode == 0, written.stderr + assert b"Recall" not in written.stderr + assert invocations == test_case.expected_invocations + + +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallStaleViewsTestCase( + description="previous views without a version row are replaced by an administrator", + version_row="DELETE", + limited_writer=False, + expected_notices=0, + expected_stale_views=((0,),), + expected_version=((3,),), + ), + ClickHouseRecallStaleViewsTestCase( + description="previous views recorded as current are replaced by an administrator", + version_row="KEEP", + limited_writer=False, + expected_notices=0, + expected_stale_views=((0,),), + expected_version=((3,),), + ), + ClickHouseRecallStaleViewsTestCase( + description="previous views without a version row make a SELECT, INSERT writer warn", + version_row="DELETE", + limited_writer=True, + expected_notices=1, + expected_stale_views=((4,),), + expected_version=((0,),), + ), + ], + ids=lambda case: case.description, +) +def test_given_views_of_the_previous_version_when_building_then_they_count_as_older( + test_case: ClickHouseRecallStaleViewsTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + admin: Path = write_recall_project( + parent=tmp_path / "admin", config=clickhouse_e2e_config, database=recall_database + ) + assert run(admin, "build").returncode == 0 + create_previous_views( + config=clickhouse_e2e_config, + database=recall_database, + names=("invocations", "statements", "output_invocations", "artifacts"), + prefix="", + ) + if test_case.version_row == "DELETE": + execute_clickhouse_sql( + config=clickhouse_e2e_config, + statements=( + f"ALTER TABLE {recall_database}.recall_meta DELETE WHERE 1 " + "SETTINGS mutations_sync = 2", + ), + ) + + if test_case.limited_writer: + with limited_user(config=clickhouse_e2e_config, database=recall_database) as writer: + built: subprocess.CompletedProcess[bytes] = run( + write_recall_project( + parent=tmp_path / "writer", config=writer, database=recall_database + ), + "build", + ) + else: + built = run(admin, "build") + + stderr: str = built.stderr.decode(errors="replace") + assert built.returncode == 0, stderr + assert ( + stderr.count("is behind this SQLBuild version, and this user may not upgrade it") + == test_case.expected_notices + ) + assert _stale_views(config=clickhouse_e2e_config, database=recall_database) == ( + test_case.expected_stale_views + ) + assert ( + fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql=f"SELECT toInt64(max(toInt64OrZero(value))) FROM {recall_database}.recall_meta " + "WHERE component = 'schema'", + ) + == test_case.expected_version + ) + + +if __name__ == "__main__": + pytest.main([__file__, "-vv"]) diff --git a/website/src/content/docs/docs/cli/recall.mdx b/website/src/content/docs/docs/cli/recall.mdx index 8a3f63d21e..551bb6962f 100644 --- a/website/src/content/docs/docs/cli/recall.mdx +++ b/website/src/content/docs/docs/cli/recall.mdx @@ -91,10 +91,11 @@ Timestamps are ISO-8601 UTC text, so they compare correctly as strings. Filter o List every Recall view with its columns, types, and join keys. Accepts `--format`, `--json`, and `--target`. -With `--ddl`, print instead the SQL that creates the store, or brings an existing store up to this -SQLBuild version (missing columns, replaced views, and the schema version row), for an -administrator to run. It reads the store through `read_connection` and changes nothing; when the -store is up to date it prints nothing and says so on stderr. Run it again after every upgrade. With `--backend warehouse`, it lists SQLBuild's state tables in the target's warehouse +Recall creates and upgrades its store itself on the first write when the writer may run DDL, so +there is usually nothing to run. With `--ddl`, print instead the SQL an administrator runs when the +writer may not: what creates the store, or brings it up to this SQLBuild version (missing columns, +replaced views, and the schema version row). It reads the store through `read_connection` and +changes nothing; when the store is up to date it prints nothing and says so on stderr. With `--backend warehouse`, it lists SQLBuild's state tables in the target's warehouse instead, or describes one with `--relation `. ## sqb recall output diff --git a/website/src/content/docs/docs/concepts/observability/recall.mdx b/website/src/content/docs/docs/concepts/observability/recall.mdx index 4d4c3f29d6..97798b9d0a 100644 --- a/website/src/content/docs/docs/concepts/observability/recall.mdx +++ b/website/src/content/docs/docs/concepts/observability/recall.mdx @@ -216,8 +216,9 @@ see what last night's production build ran and printed without access to the mac first run where an audit started failing across everyone's runs, or compare a production run with your own by `target`, `git_revision` or `sqlbuild_version`. -The connection needs `adapter = "clickhouse"` when the project uses another adapter, and its -`database` must already exist; Recall never creates databases. +The connection needs `adapter = "clickhouse"` when the project uses another adapter. Recall creates +and upgrades its own store on the first write: the `database` when it does not exist yet, then its +tables, views and schema version. A writer that may run that DDL needs no other setup. ```toml [connections.history_writer] @@ -250,13 +251,16 @@ writer's password on a reader's laptop, need not be set. ### Users and grants -SQLBuild does not enforce grants; it only lets the roles be separated. A least-privilege setup: +SQLBuild does not enforce grants; it only lets the roles be separated. Give the writer the rights to +create and upgrade the store in its database, so the first command sets it up and later SQLBuild +versions upgrade it without anyone running DDL by hand: ```sql -- once, as an administrator: the users CREATE USER recall_writer IDENTIFIED BY '...'; CREATE USER recall_reader IDENTIFIED BY '...'; -GRANT SELECT, INSERT ON observability.* TO recall_writer; +GRANT CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP VIEW, ALTER ADD COLUMN, SELECT, INSERT + ON observability.* TO recall_writer; GRANT SELECT ON observability.* TO recall_reader; ``` @@ -270,23 +274,17 @@ read-only; a user that may not set the row limit gets the same truncated result, reading. A setting that Recall needs and the user may not change fails with an error naming it; set it in the user's profile, or give the user `readonly = 2`, which keeps it read-only. -Then, as the administrator, create the store with the DDL that `sqb recall schema --ddl` prints -(any connection that can read the database works, so the reader's is enough): - -```bash -sqb recall schema --ddl --target prod > recall.sql # review, then run it with an admin user -``` - -Repeat the same step after upgrading SQLBuild: `sqb recall schema --ddl` reads the store and prints -only what this version adds (`ALTER TABLE … ADD COLUMN`, replaced views and the schema version row), -or says that the store is up to date. Changes are always additive, and older SQLBuild versions keep -writing to an upgraded store. `--ddl` prints SQL, so it cannot be combined with `--json` or -`--format`. - -- **Setup.** A writer allowed to run DDL creates the `recall_*` MergeTree tables and the views above - in that database on first use, and adds the columns and views a newer SQLBuild introduces. If the - user may not create the store, the command still succeeds and prints the complete DDL for an - administrator to run; a store created from it accepts writers limited to `SELECT` and `INSERT`. +A writer limited to `SELECT` and `INSERT` also works, once the store exists: it writes history but +cannot create or upgrade the store. When such a writer finds the store missing or behind, Recall +prints the exact statements an administrator should run, and `sqb recall schema --ddl` prints them +again. + +- **Setup.** On first use the writer creates the database if it is missing, printing + ``Recall: created ClickHouse database `observability` for the store.`` once, then the `recall_*` + MergeTree tables and the views above. An existing database is used as it is, whatever its engine. + If the user may not create the store, the command still succeeds and prints the statements, + starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from + them accepts writers limited to `SELECT` and `INSERT`. - **Upgrades.** When a store is behind this SQLBuild version and the writer may not upgrade it, the command still runs and exits as it otherwise would. History is still written, to the columns that exist, and Recall prints one warning (at most once a day) with the exact statements an @@ -295,7 +293,9 @@ writing to an upgraded store. `--ddl` prints SQL, so it cannot be combined with other. Writers append it again once it is older than half the retention, with the highest version recorded. When it has expired anyway, a writer compares the stored tables' and views' columns with its own: it upgrades an older store, records its version on a current one, and never - replaces the views of a store a newer SQLBuild version created. + replaces the views of a store a newer SQLBuild version created. Views whose definitions show an + older version created them, such as views that set `join_use_nulls`, count as older whatever the + version row says. - **Writes.** History is sent in batches of up to 5,000 rows about every five seconds, one insert per table, so a long command adds few parts; everything still queued is sent when it ends, with the same queue, shutdown budget and exit-code guarantees as the local store. Many concurrent From 09984f02dafba560cf7f41d32f951edced0741c3 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:11:42 +0100 Subject: [PATCH 2/6] docs: lead the Recall docs and skill with the intended workflow Record an integration's external context as JSON, so the documented job query finds orchestrated runs. --- .../_helpers/recall/clickhouse_store.rs | 2 +- .../src/bindings/_helpers/recall/session.rs | 8 +- .../src/settings/_helpers/layers.rs | 4 +- src/sqlbuild/.agents/skills/sqlbuild/SKILL.md | 22 +++- .../sqlbuild/references/docs/cli/recall.md | 9 +- .../docs/concepts/adapters/clickhouse.md | 2 +- .../docs/concepts/observability/recall.md | 103 +++++++++++++----- .../references/exploring-a-project.md | 2 +- .../sqlbuild/references/investigating-runs.md | 17 +-- .../commands/main/clickhouse/_test_types.py | 8 ++ .../test_clickhouse_recall_setup.py | 32 ++++++ .../src/sqlbuild/cli/recall/_test_types.py | 8 ++ .../src/sqlbuild/cli/recall/helpers.py | 20 ++++ .../recall/test_recall_invocation_facts.py | 69 ++++++++++++ .../docs/concepts/adapters/clickhouse.mdx | 2 +- .../docs/concepts/observability/recall.mdx | 61 ++++++++++- 16 files changed, 319 insertions(+), 50 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs index 6c1a2614dc..4b91c8e645 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs @@ -344,7 +344,7 @@ impl ClickHouseStore { observed } - /// Create a store whose tables are missing. On existing tables add missing columns, create missing views, and replace views on a version upgrade; when retention removed the version row, the views' columns tell whether the store is older, current or newer, and a newer store's views are never replaced. A user who may not run that DDL writes the columns that exist and gets the statements an administrator must run. A version is recorded only once the views are settled, never lower than the store holds, and again before retention removes the newest row. + /// Create a store whose tables are missing. On existing tables add missing columns, create missing views, and replace views on a version upgrade; when retention removed the version row, the views' columns tell whether the store is older, current or newer, and a newer store's views are never replaced; views whose definitions show an older version wrote them make the store older either way. A user who may not run that DDL writes the columns that exist and gets the statements an administrator must run. A version is recorded only once the views are settled, never lower than the store holds, and again before retention removes the newest row. fn ensure_schema( &mut self, py: Python<'_>, diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs index 26dc6e54ef..3ab2797ef6 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use std::time::{Duration, Instant, SystemTime}; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python}; +use pyo3::types::{PyDict, PyDictMethods}; use sqlbuild_cli::dispatch::main::closing_status::closing_status; use sqlbuild_cli::dispatch::models::CommandFacts; use sqlbuild_host::entries::models::ExecutorHost; @@ -30,6 +31,7 @@ use sqlbuild_recall::writer::models::{ DeliverySummary, RecallSession, SessionConfig, SessionStore, SetupOutcome, }; +use crate::bindings::_helpers::boundary::python_calls::imported; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::output_capture::records::invocation_context; use crate::bindings::_helpers::recall::clickhouse_store::{ @@ -194,7 +196,11 @@ fn external_context(py: Python<'_>) -> PyResult> { if !context.is_truthy()? { return Ok(None); } - Ok(dumped_json(&context)?.map(|encoded| encoded.to_string())) + let options = PyDict::new(py); + options.set_item("default", py.import("builtins")?.getattr("dict")?)?; + let encoded = imported(py, "json", "dumps")?.call((&context,), Some(&options))?; + let plain = imported(py, "json", "loads")?.call1((encoded,))?; + Ok(dumped_json(&plain)?.map(|encoded| encoded.to_string())) } fn heartbeat_interval() -> Duration { diff --git a/crates/sqlbuild-recall/src/settings/_helpers/layers.rs b/crates/sqlbuild-recall/src/settings/_helpers/layers.rs index 7e90eb6997..570122df90 100644 --- a/crates/sqlbuild-recall/src/settings/_helpers/layers.rs +++ b/crates/sqlbuild-recall/src/settings/_helpers/layers.rs @@ -127,8 +127,8 @@ fn clickhouse_location( } Some(_) => { return Err(SettingsError::invalid_configuration(format!( - "connections.{name}.database must name the existing ClickHouse database Recall \ - writes to" + "connections.{name}.database must name the ClickHouse database Recall writes \ + to" ))); } }; diff --git a/src/sqlbuild/.agents/skills/sqlbuild/SKILL.md b/src/sqlbuild/.agents/skills/sqlbuild/SKILL.md index 3f57f46e06..22d9ebd854 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/SKILL.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/SKILL.md @@ -79,11 +79,13 @@ printed. Add `--backend warehouse` to check live warehouse state with the same c Read [references/investigating-runs.md](references/investigating-runs.md) before writing recall queries; `statements` and `audit_results` use `outcome`, not `status`. -Check `read_connection` in `[recall]` or `[targets..recall]` to see which store you are -reading: +Recall reads the store of the selected target: the default target unless you pass `--target`. +Check that target's `read_connection` in `[recall]` or `[targets..recall]`: - **Local (the default).** A SQLite file, `logs/recall.sqlite3`, holding this checkout's own runs. - Use it for "what did my last build do?" and to compare runs during your session. + Use it for "what did my last build do?" and to compare runs during your session. Development + targets usually record here, so pass `--target ` to see what a shared deployment + target ran. - **Shared ClickHouse store.** One history for every machine that writes to it: teammates, CI, and scheduled or orchestrated production runs. This is far more powerful: you can see what prod actually ran and printed last night without access to the machine, find the first run where an @@ -92,6 +94,20 @@ reading: ClickHouse SQL, and `--json` reports `rows_read` and `bytes_read`; filter on `invocation_id` or `invocation_started_at` and keep a `LIMIT` on large stores. +The default `long` format is fine in most cases: read it as it is. If it is too verbose, select +fewer columns or add a `LIMIT` rather than changing the format or reshaping the output. Use +`--json` only when a program does something meaningful with the result, such as feeding it into +further processing, not just to print rows more compactly. + +Every command except `sqb recall` is recorded in the store of the target it runs against, +including `sqb debug` and failed commands. To check a shared target, prefer `sqb recall` reads: +anything else you run from your machine with `--target ` appears in that target's history. + +Recall creates and upgrades its own store (database, tables, views and schema version) on the first +write, so there is usually nothing to set up, even for a new shared store. Run the statements from +`sqb recall schema --ddl` yourself only when the writer is not allowed to run DDL and Recall printed +them for an administrator, or when an administrator asks you to. + Read [references/docs/concepts/observability/recall.md](references/docs/concepts/observability/recall.md). ## Reach for these tools diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/recall.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/recall.md index 20c58781f3..b58f94671e 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/recall.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/cli/recall.md @@ -100,10 +100,11 @@ Timestamps are ISO-8601 UTC text, so they compare correctly as strings. Filter o List every Recall view with its columns, types, and join keys. Accepts `--format`, `--json`, and `--target`. -With `--ddl`, print instead the SQL that creates the store, or brings an existing store up to this -SQLBuild version (missing columns, replaced views, and the schema version row), for an -administrator to run. It reads the store through `read_connection` and changes nothing; when the -store is up to date it prints nothing and says so on stderr. Run it again after every upgrade. With `--backend warehouse`, it lists SQLBuild's state tables in the target's warehouse +Recall creates and upgrades its store itself on the first write when the writer may run DDL, so +there is usually nothing to run. With `--ddl`, print instead the SQL an administrator runs when the +writer may not: what creates the store, or brings it up to this SQLBuild version (missing columns, +replaced views, and the schema version row). It reads the store through `read_connection` and +changes nothing; when the store is up to date it prints nothing and says so on stderr. With `--backend warehouse`, it lists SQLBuild's state tables in the target's warehouse instead, or describes one with `--relation `. ## sqb recall output diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/clickhouse.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/clickhouse.md index 7e497fa203..7c86acc95c 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/clickhouse.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/adapters/clickhouse.md @@ -198,7 +198,7 @@ ClickHouse returns a type's default value (`0`, the epoch, an empty string) from A ClickHouse connection can also hold [Recall](../observability/recall.md)'s command history for a whole team, whichever adapter the project builds with. Set `[recall] write_connection` and `read_connection` to it, one connection or separate writer and -reader users. Recall creates its tables on first use, keeps every output byte by default, and needs +reader users. Recall creates its database, tables and views on first use, keeps every output byte by default, and needs only `SELECT` and `INSERT` once the tables exist; readers need only `SELECT` and may be `readonly = 1` users. See [Sharing history in ClickHouse](../observability/recall.md#sharing-history-in-clickhouse). diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md index 40bad05a42..4c08e2225c 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md @@ -233,8 +233,9 @@ see what last night's production build ran and printed without access to the mac first run where an audit started failing across everyone's runs, or compare a production run with your own by `target`, `git_revision` or `sqlbuild_version`. -The connection needs `adapter = "clickhouse"` when the project uses another adapter, and its -`database` must already exist; Recall never creates databases. +The connection needs `adapter = "clickhouse"` when the project uses another adapter. Recall creates +and upgrades its own store on the first write: the `database` when it does not exist yet, then its +tables, views and schema version. A writer that may run that DDL needs no other setup. ```toml [connections.history_writer] @@ -252,7 +253,7 @@ password = "${env:RECALL_READER_PASSWORD}" database = "observability" [recall] -write_connection = "history_writer" # every command writes history (INSERT, SELECT) +write_connection = "history_writer" # every command writes history; creates the store on first use read_connection = "history_reader" # sqb recall query, schema and output (SELECT only) retention = "30d" ``` @@ -267,16 +268,22 @@ writer's password on a reader's laptop, need not be set. ### Users and grants -SQLBuild does not enforce grants; it only lets the roles be separated. A least-privilege setup: +SQLBuild does not enforce grants; it only lets the roles be separated. Give the writer the rights to +create and upgrade the store in its database, so the first command sets it up and later SQLBuild +versions upgrade it without anyone running DDL by hand: ```sql -- once, as an administrator: the users CREATE USER recall_writer IDENTIFIED BY '...'; CREATE USER recall_reader IDENTIFIED BY '...'; -GRANT SELECT, INSERT ON observability.* TO recall_writer; +GRANT CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP TABLE, ALTER ADD COLUMN, SELECT, INSERT + ON observability.* TO recall_writer; GRANT SELECT ON observability.* TO recall_reader; ``` +`DROP TABLE` is there because ClickHouse replaces a view through a temporary table; Recall itself +never drops a table. + The reader may be a standard read-only user (`SETTINGS readonly = 1`, or a settings profile with it), which may not change any setting. Such a user's queries run with the server's own settings; every other reader gets the @@ -287,23 +294,17 @@ read-only; a user that may not set the row limit gets the same truncated result, reading. A setting that Recall needs and the user may not change fails with an error naming it; set it in the user's profile, or give the user `readonly = 2`, which keeps it read-only. -Then, as the administrator, create the store with the DDL that `sqb recall schema --ddl` prints -(any connection that can read the database works, so the reader's is enough): - -```bash -sqb recall schema --ddl --target prod > recall.sql # review, then run it with an admin user -``` - -Repeat the same step after upgrading SQLBuild: `sqb recall schema --ddl` reads the store and prints -only what this version adds (`ALTER TABLE … ADD COLUMN`, replaced views and the schema version row), -or says that the store is up to date. Changes are always additive, and older SQLBuild versions keep -writing to an upgraded store. `--ddl` prints SQL, so it cannot be combined with `--json` or -`--format`. - -- **Setup.** A writer allowed to run DDL creates the `recall_*` MergeTree tables and the views above - in that database on first use, and adds the columns and views a newer SQLBuild introduces. If the - user may not create the store, the command still succeeds and prints the complete DDL for an - administrator to run; a store created from it accepts writers limited to `SELECT` and `INSERT`. +A writer limited to `SELECT` and `INSERT` also works, once the store exists: it writes history but +cannot create or upgrade the store. When such a writer finds the store missing or behind, Recall +prints the exact statements an administrator should run, and `sqb recall schema --ddl` prints them +again. + +- **Setup.** On first use the writer creates the database if it is missing, printing + ``Recall: created ClickHouse database `observability` for the store.`` once, then the `recall_*` + MergeTree tables and the views above. An existing database is used as it is, whatever its engine. + If the user may not create the store, the command still succeeds and prints the statements, + starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from + them accepts writers limited to `SELECT` and `INSERT`. - **Upgrades.** When a store is behind this SQLBuild version and the writer may not upgrade it, the command still runs and exits as it otherwise would. History is still written, to the columns that exist, and Recall prints one warning (at most once a day) with the exact statements an @@ -312,7 +313,9 @@ writing to an upgraded store. `--ddl` prints SQL, so it cannot be combined with other. Writers append it again once it is older than half the retention, with the highest version recorded. When it has expired anyway, a writer compares the stored tables' and views' columns with its own: it upgrades an older store, records its version on a current one, and never - replaces the views of a store a newer SQLBuild version created. + replaces the views of a store a newer SQLBuild version created. Views whose definitions show an + older version created them, such as views that set `join_use_nulls`, count as older whatever the + version row says. - **Writes.** History is sent in batches of up to 5,000 rows about every five seconds, one insert per table, so a long command adds few parts; everything still queued is sent when it ends, with the same queue, shutdown budget and exit-code guarantees as the local store. Many concurrent @@ -368,7 +371,10 @@ def recall_output(record: CommandOutputRecord, writer: RecallWriter) -> None: ## Querying history -Use [`sqb recall`](../../cli/recall.md) to read the store. Queries are read-only `SELECT` statements, +Use [`sqb recall`](../../cli/recall.md) to read the store: `sqb recall query` for questions and +`sqb recall output` for what a command printed. The views fold the raw `recall_*` tables for you +(duplicate deliveries, terminal facts, retention), so start there; query the store's database +directly only for something the views do not expose. Queries are read-only `SELECT` statements, in the store's SQL dialect, against these views: | View | One row per | @@ -384,12 +390,59 @@ in the store's SQL dialect, against these views: | `output_tail` | Recent output chunk | | `conflicts` | Record written twice with different content | -Run `sqb recall schema` for every column. +Run `sqb recall schema` for every column, and check the values a column holds before filtering on +it, for example `SELECT DISTINCT resource_kind, purpose FROM statements`: a model's +`resource_kind` is its materialization, such as `table` or `view`, and its build statements have +`purpose = 'model_build'`. Every view has `invocation_id` and `invocation_started_at`. Filter on them to keep a query cheap: on a ClickHouse store, a filter on either reaches the table reads, so a query about one invocation or the last day reads that part of the store instead of folding the whole retention window. +### Common questions + +These run as written on the local SQLite store; on ClickHouse, read JSON with +`JSONExtractString(external_context, 'integration', 'job_name')`. + +The latest runs of an orchestrated job, by the context its integration records (the Dagster +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`): + +```bash +sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms + FROM invocations + WHERE json_extract(external_context, '$.integration.job_name') = 'daily_models' + ORDER BY invocation_started_at DESC LIMIT 5" +``` + +The slowest models in a run: + +```bash +sqb recall query "SELECT resource_name, resource_kind, sum(duration_ms) AS total_ms + FROM statements WHERE invocation_id = '' AND purpose = 'model_build' + GROUP BY resource_name, resource_kind ORDER BY total_ms DESC LIMIT 10" +``` + +Audits that did not pass: + +```bash +sqb recall query "SELECT invocation_started_at, audit_name, outcome, violation_count, execution_error + FROM audit_results WHERE outcome <> 'pass' + ORDER BY invocation_started_at DESC LIMIT 20" +``` + +What a run printed at the end: + +```bash +sqb recall output --tail 50 +``` + +### From a statement to the warehouse + +`statements.query_id` is the warehouse's own ID for the statement, where the adapter reports one: +the Snowflake query ID, or the `query_id` ClickHouse logs in `system.query_log`. It is `NULL` on +adapters without one, such as DuckDB. Look it up in the warehouse's query history to see what +Recall does not record, such as the warehouse size, queueing and spilling behind a slow statement. + ## Relationship to compute logs Compute logs (`logs///stdout.log`, `stderr.log`, and `diagnostics.jsonl`) are diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/exploring-a-project.md b/src/sqlbuild/.agents/skills/sqlbuild/references/exploring-a-project.md index 9b492a696d..e50053ad11 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/exploring-a-project.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/exploring-a-project.md @@ -99,7 +99,7 @@ explain it. ## Looking at data: sqb query ```bash -sqb query "SELECT status, COUNT(*) FROM analytics.orders GROUP BY 1" --format table +sqb query "SELECT status, COUNT(*) FROM analytics.orders GROUP BY 1" sqb query --file /tmp/check.sql --limit 100 ``` diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/investigating-runs.md b/src/sqlbuild/.agents/skills/sqlbuild/references/investigating-runs.md index d9976dad03..97918e64e6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/investigating-runs.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/investigating-runs.md @@ -63,6 +63,9 @@ what they printed. `sqb recall query` runs one read-only `SELECT` over a set of `statements.status`. - On a shared ClickHouse store write ClickHouse SQL (`now() - INTERVAL 1 DAY`, `countIf`), not SQLite SQL (`strftime`, `datetime('now', '-1 day')`). +- Check values before filtering on a kind or status column: `SELECT DISTINCT resource_kind, purpose + FROM statements`. A model's `resource_kind` is its materialization (`table`, `view` or a custom + one), never `model`; its build statements have `purpose = 'model_build'`. - Keep queries few and targeted: select the columns you need, filter on `invocation_id` or `invocation_started_at`, and add `LIMIT`. `SELECT *` over a view on a large store is slow. - Filter on a literal `invocation_id`. On ClickHouse, `invocation_id = (SELECT ...)` does not reach @@ -75,13 +78,13 @@ what they printed. `sqb recall query` runs one read-only `SELECT` over a set of ## Output format -- Read results in the default `long` format: every value in full, one record at a time. -- When there are too many rows to read, narrow the query (fewer columns, `LIMIT`, aggregation); - don't switch format. -- Use `--json` only when a program consumes the output, for example to chain an `invocation_id` - into the next query, or when you need `statistics` (`rows_read`, `bytes_read`, elapsed time). - Don't use `--json` by default "to be safe". -- Use `--format csv` for export. `--format table` is deprecated. +- The default `long` format is fine in most cases: read it as it is, every value in full, one + record at a time. +- If it is too verbose, select fewer columns or add a `LIMIT` (or aggregate) rather than changing + the format or reshaping the output. +- Use `--json` only when a program does something meaningful with the result, for example to chain + an `invocation_id` into the next query, or when you need `statistics` (`rows_read`, + `bytes_read`, elapsed time). Not just to print rows more compactly. ## Worked investigations diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py index e6a82342f6..444911aeb2 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py @@ -332,3 +332,11 @@ class ClickHouseRecallStaleViewsTestCase: expected_notices: int expected_stale_views: tuple[tuple[object, ...], ...] expected_version: tuple[tuple[object, ...], ...] + + +@dataclass(frozen=True) +class ClickHouseRecallExternalContextTestCase: + description: str + external_context: str + sql: str + expected_rows: list[list[object]] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py index 7125643583..bf6050ad15 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py @@ -8,6 +8,7 @@ from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse._test_types import ( ClickHouseRecallDatabaseCreationTestCase, ClickHouseRecallExistingDatabaseTestCase, + ClickHouseRecallExternalContextTestCase, ClickHouseRecallMissingDatabaseTestCase, ClickHouseRecallStaleViewsTestCase, ) @@ -286,5 +287,36 @@ def test_given_views_of_the_previous_version_when_building_then_they_count_as_ol ) +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallExternalContextTestCase( + description="an orchestrated job's runs are found with the documented JSON spelling", + external_context='{"integration": {"name": "dagster", "job_name": "daily_models"}}', + sql="SELECT status FROM invocations " + "WHERE JSONExtractString(external_context, 'integration', 'job_name') = 'daily_models'", + expected_rows=[["completed"]], + ), + ], + ids=lambda case: case.description, +) +def test_given_an_orchestrated_build_when_queried_on_clickhouse_then_its_context_is_json( + test_case: ClickHouseRecallExternalContextTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + project_dir: Path = write_recall_project( + parent=tmp_path, config=clickhouse_e2e_config, database=recall_database + ) + + built: subprocess.CompletedProcess[bytes] = run( + project_dir, "build", SQLBUILD_INVOCATION_CONTEXT_JSON=test_case.external_context + ) + + assert built.returncode == 0, built.stderr + assert recall_rows(project_dir, test_case.sql) == test_case.expected_rows + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) diff --git a/tests/integration/src/sqlbuild/cli/recall/_test_types.py b/tests/integration/src/sqlbuild/cli/recall/_test_types.py index 4109cb2f7d..03f8503d9d 100644 --- a/tests/integration/src/sqlbuild/cli/recall/_test_types.py +++ b/tests/integration/src/sqlbuild/cli/recall/_test_types.py @@ -310,3 +310,11 @@ class RecallDiagnosticsTestCase: class UnsetWriterVariableTestCase: description: str expected_exit_code: int + + +@dataclass(frozen=True) +class DocumentedQueryTestCase: + description: str + question: int + columns: tuple[str, ...] + expected_rows: list[list[object]] diff --git a/tests/integration/src/sqlbuild/cli/recall/helpers.py b/tests/integration/src/sqlbuild/cli/recall/helpers.py index 7624e25b30..baa8b2771f 100644 --- a/tests/integration/src/sqlbuild/cli/recall/helpers.py +++ b/tests/integration/src/sqlbuild/cli/recall/helpers.py @@ -6,6 +6,7 @@ import operator import os import re +import shlex import signal import sqlite3 import stat @@ -570,3 +571,22 @@ def worktree_project(parent: Path) -> tuple[Path, str]: repository, revision = committed_repository(parent) _ = git(repository, "worktree", "add", "-q", "-b", "orders", str(parent / "linked")) return parent / "linked" / "orders_shop", revision + + +RECALL_CONCEPT_PAGE: Path = ( + Path(__file__).resolve().parents[6] + / "website/src/content/docs/docs/concepts/observability/recall.mdx" +) +_COMMON_QUESTIONS: str = "### Common questions" +_BASH_BLOCK: re.Pattern[str] = re.compile(r"```bash\n(.*?)```", re.DOTALL) + + +def documented_commands(*, invocation_id: str) -> list[list[str]]: + """The arguments of each command under the Recall page's common questions, for one invocation.""" + + page: str = RECALL_CONCEPT_PAGE.read_text(encoding="utf-8") + section: str = page.split(_COMMON_QUESTIONS, 1)[1].split("\n### ", 1)[0] + return [ + shlex.split(block.replace("", invocation_id))[1:] + for block in _BASH_BLOCK.findall(section) + ] diff --git a/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py b/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py index f7b689c63f..ef58685f3a 100644 --- a/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py +++ b/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py @@ -1,9 +1,12 @@ +import json import subprocess from pathlib import Path +from typing import Any import pytest from tests.integration.src.sqlbuild.cli.recall._test_types import ( + DocumentedQueryTestCase, FailureCauseTestCase, GitRevisionTestCase, ) @@ -11,6 +14,7 @@ DEPLOYED_REVISION, checkout_project, deployed_project, + documented_commands, recall_rows, run, worktree_project, @@ -25,6 +29,71 @@ 'schema = "dev"\n' ) +ORCHESTRATED_CONTEXT: str = ( + '{"integration": {"name": "dagster", "job_name": "daily_models", "run_id": "run-7"}}' +) +WARNING_AUDITS: str = '\n[settings]\ndefault_audit_severity = "warn"\n' +CUSTOMERS_MODEL: str = ( + 'MODEL (\n description "One row per customer",\n materialized table,\n' + " columns (email (audits [not_null])),\n);\n\n" + "SELECT 1 AS customer_id, CAST(NULL AS VARCHAR) AS email\n" +) + + +@pytest.mark.parametrize( + "test_case", + [ + DocumentedQueryTestCase( + description="an orchestrated job's runs are found by their external context", + question=0, + columns=("status",), + expected_rows=[["completed"]], + ), + DocumentedQueryTestCase( + description="the slowest models of a run are its model builds by resource", + question=1, + columns=("resource_name", "resource_kind"), + expected_rows=[["customers", "table"], ["orders", "table"]], + ), + DocumentedQueryTestCase( + description="audits that did not pass are listed", + question=2, + columns=("audit_name", "outcome", "violation_count"), + expected_rows=[["not_null", "warn", 1]], + ), + ], + ids=lambda case: case.description, +) +def test_given_an_orchestrated_build_when_running_the_documented_queries_then_they_answer( + test_case: DocumentedQueryTestCase, tmp_path: Path +) -> None: + project_dir: Path = write_project(tmp_path / "orders_shop", extra_toml=WARNING_AUDITS) + _ = (project_dir / "models" / "customers.sql").write_text(CUSTOMERS_MODEL, encoding="utf-8") + built: subprocess.CompletedProcess[bytes] = run( + project_dir, "build", SQLBUILD_INVOCATION_CONTEXT_JSON=ORCHESTRATED_CONTEXT + ) + invocation_id: str = str( + recall_rows(project_dir, "SELECT invocation_id FROM invocations")[0][0] + ) + commands: list[list[str]] = documented_commands(invocation_id=invocation_id) + + answered: subprocess.CompletedProcess[bytes] = run( + project_dir, *commands[test_case.question], "--json" + ) + printed: subprocess.CompletedProcess[bytes] = run(project_dir, *commands[-1]) + + assert built.returncode == 0, built.stderr + assert len(commands) == 4 + assert answered.returncode == 0, answered.stderr + result: dict[str, list[Any]] = json.loads(answered.stdout) + positions: list[int] = [result["columns"].index(column) for column in test_case.columns] + assert [ + [row[position] for position in positions] for row in result["rows"] + ] == test_case.expected_rows + assert printed.returncode == 0, printed.stderr + assert len(printed.stdout.splitlines()) == 50 + assert printed.stdout.endswith(built.stdout.splitlines(keepends=True)[-1]) + @pytest.mark.parametrize( "test_case", diff --git a/website/src/content/docs/docs/concepts/adapters/clickhouse.mdx b/website/src/content/docs/docs/concepts/adapters/clickhouse.mdx index 91367a362d..27fb8d29cb 100644 --- a/website/src/content/docs/docs/concepts/adapters/clickhouse.mdx +++ b/website/src/content/docs/docs/concepts/adapters/clickhouse.mdx @@ -184,7 +184,7 @@ ClickHouse returns a type's default value (`0`, the epoch, an empty string) from A ClickHouse connection can also hold [Recall](/docs/concepts/observability/recall)'s command history for a whole team, whichever adapter the project builds with. Set `[recall] write_connection` and `read_connection` to it, one connection or separate writer and -reader users. Recall creates its tables on first use, keeps every output byte by default, and needs +reader users. Recall creates its database, tables and views on first use, keeps every output byte by default, and needs only `SELECT` and `INSERT` once the tables exist; readers need only `SELECT` and may be `readonly = 1` users. See [Sharing history in ClickHouse](/docs/concepts/observability/recall#sharing-history-in-clickhouse). diff --git a/website/src/content/docs/docs/concepts/observability/recall.mdx b/website/src/content/docs/docs/concepts/observability/recall.mdx index 97798b9d0a..b5a505ee21 100644 --- a/website/src/content/docs/docs/concepts/observability/recall.mdx +++ b/website/src/content/docs/docs/concepts/observability/recall.mdx @@ -236,7 +236,7 @@ password = "${env:RECALL_READER_PASSWORD}" database = "observability" [recall] -write_connection = "history_writer" # every command writes history (INSERT, SELECT) +write_connection = "history_writer" # every command writes history; creates the store on first use read_connection = "history_reader" # sqb recall query, schema and output (SELECT only) retention = "30d" ``` @@ -259,11 +259,14 @@ versions upgrade it without anyone running DDL by hand: -- once, as an administrator: the users CREATE USER recall_writer IDENTIFIED BY '...'; CREATE USER recall_reader IDENTIFIED BY '...'; -GRANT CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP VIEW, ALTER ADD COLUMN, SELECT, INSERT +GRANT CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP TABLE, ALTER ADD COLUMN, SELECT, INSERT ON observability.* TO recall_writer; GRANT SELECT ON observability.* TO recall_reader; ``` +`DROP TABLE` is there because ClickHouse replaces a view through a temporary table; Recall itself +never drops a table. + The reader may be a standard read-only user (`SETTINGS readonly = 1`, or a settings profile with it), which may not change any setting. Such a user's queries run with the server's own settings; every other reader gets the @@ -352,7 +355,10 @@ def recall_output(record: CommandOutputRecord, writer: RecallWriter) -> None: ## Querying history -Use [`sqb recall`](/docs/cli/recall) to read the store. Queries are read-only `SELECT` statements, +Use [`sqb recall`](/docs/cli/recall) to read the store: `sqb recall query` for questions and +`sqb recall output` for what a command printed. The views fold the raw `recall_*` tables for you +(duplicate deliveries, terminal facts, retention), so start there; query the store's database +directly only for something the views do not expose. Queries are read-only `SELECT` statements, in the store's SQL dialect, against these views: | View | One row per | @@ -368,12 +374,59 @@ in the store's SQL dialect, against these views: | `output_tail` | Recent output chunk | | `conflicts` | Record written twice with different content | -Run `sqb recall schema` for every column. +Run `sqb recall schema` for every column, and check the values a column holds before filtering on +it, for example `SELECT DISTINCT resource_kind, purpose FROM statements`: a model's +`resource_kind` is its materialization, such as `table` or `view`, and its build statements have +`purpose = 'model_build'`. Every view has `invocation_id` and `invocation_started_at`. Filter on them to keep a query cheap: on a ClickHouse store, a filter on either reaches the table reads, so a query about one invocation or the last day reads that part of the store instead of folding the whole retention window. +### Common questions + +These run as written on the local SQLite store; on ClickHouse, read JSON with +`JSONExtractString(external_context, 'integration', 'job_name')`. + +The latest runs of an orchestrated job, by the context its integration records (the Dagster +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`): + +```bash +sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms + FROM invocations + WHERE json_extract(external_context, '$.integration.job_name') = 'daily_models' + ORDER BY invocation_started_at DESC LIMIT 5" +``` + +The slowest models in a run: + +```bash +sqb recall query "SELECT resource_name, resource_kind, sum(duration_ms) AS total_ms + FROM statements WHERE invocation_id = '' AND purpose = 'model_build' + GROUP BY resource_name, resource_kind ORDER BY total_ms DESC LIMIT 10" +``` + +Audits that did not pass: + +```bash +sqb recall query "SELECT invocation_started_at, audit_name, outcome, violation_count, execution_error + FROM audit_results WHERE outcome <> 'pass' + ORDER BY invocation_started_at DESC LIMIT 20" +``` + +What a run printed at the end: + +```bash +sqb recall output --tail 50 +``` + +### From a statement to the warehouse + +`statements.query_id` is the warehouse's own ID for the statement, where the adapter reports one: +the Snowflake query ID, or the `query_id` ClickHouse logs in `system.query_log`. It is `NULL` on +adapters without one, such as DuckDB. Look it up in the warehouse's query history to see what +Recall does not record, such as the warehouse size, queueing and spilling behind a slow statement. + ## Relationship to compute logs Compute logs (`logs///stdout.log`, `stderr.log`, and `diagnostics.jsonl`) are From be747a39c8e8af0efd23079329f308516474ff29 Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:30:51 +0100 Subject: [PATCH 3/6] fix: satisfy the repository's static rules for the Recall setup changes --- .../src/bindings/_helpers/recall/session.rs | 36 ++++---- .../schema/main/store_version_with_views.rs | 12 +-- .../src/schema/tests/test_types.rs | 2 +- .../src/schema/tests/test_version.rs | 56 +++++------- .../commands/main/clickhouse/_test_types.py | 6 +- .../cli/commands/main/clickhouse/helpers.py | 16 ++++ .../test_clickhouse_recall_setup.py | 89 ++++++++----------- .../src/sqlbuild/cli/recall/helpers.py | 13 ++- .../recall/test_recall_invocation_facts.py | 11 ++- 9 files changed, 123 insertions(+), 118 deletions(-) diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs index 3ab2797ef6..621e57d7d1 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs @@ -5,16 +5,17 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::{Duration, Instant, SystemTime}; +use pyo3::exceptions::PyValueError; use pyo3::prelude::{Bound, PyAny, PyAnyMethods, PyResult, Python}; -use pyo3::types::{PyDict, PyDictMethods}; use sqlbuild_cli::dispatch::main::closing_status::closing_status; use sqlbuild_cli::dispatch::models::CommandFacts; +use sqlbuild_core::command_output::constants::INVOCATION_CONTEXT_ENV; use sqlbuild_host::entries::models::ExecutorHost; use sqlbuild_recall::records::main::format_timestamp::format_timestamp; use sqlbuild_recall::records::main::sha256_hex::sha256_hex; use sqlbuild_recall::records::models::{InvocationContext, OutputStream, RowEnvelope}; use sqlbuild_recall::schema::main::create_database_statement::create_database_statement; -use sqlbuild_recall::schema::main::ddl_statements::ddl_statements; +use sqlbuild_recall::schema::main::ddl_script::ddl_script; use sqlbuild_recall::schema::main::render_script::render_script; use sqlbuild_recall::schema::models::{DdlOptions, Dialect}; use sqlbuild_recall::settings::main::resolve_recall_settings::resolve_recall_settings; @@ -31,7 +32,6 @@ use sqlbuild_recall::writer::models::{ DeliverySummary, RecallSession, SessionConfig, SessionStore, SetupOutcome, }; -use crate::bindings::_helpers::boundary::python_calls::imported; use crate::bindings::_helpers::native_host::host; use crate::bindings::_helpers::output_capture::records::invocation_context; use crate::bindings::_helpers::recall::clickhouse_store::{ @@ -46,7 +46,6 @@ use crate::bindings::_helpers::recall::constants::{ use crate::bindings::_helpers::recall::events::escaped_text; use crate::bindings::_helpers::recall::replacements::{RecallReplacements, in_replacement_sink}; use crate::bindings::_helpers::records::fields::field as native_field; -use crate::bindings::_helpers::rules::config_json::dumped_json; /// What the invocation records: its session, when enabled, and the resolved target and adapter. #[derive(Clone, Debug, Default)] @@ -182,7 +181,7 @@ fn host_token() -> Option { Some(sha256_hex(name.trim().as_bytes())[..HOST_TOKEN_LENGTH].to_owned()) } -/// The orchestrator context as JSON; an invalid environment context records none. +/// The orchestrator context as JSON, read from the environment once it is validated; an invalid one records none. fn external_context(py: Python<'_>) -> PyResult> { let context: Bound<'_, PyAny> = match invocation_context(py, None) { Ok(context) => context, @@ -196,11 +195,16 @@ fn external_context(py: Python<'_>) -> PyResult> { if !context.is_truthy()? { return Ok(None); } - let options = PyDict::new(py); - options.set_item("default", py.import("builtins")?.getattr("dict")?)?; - let encoded = imported(py, "json", "dumps")?.call((&context,), Some(&options))?; - let plain = imported(py, "json", "loads")?.call1((encoded,))?; - Ok(dumped_json(&plain)?.map(|encoded| encoded.to_string())) + let Some(text) = environment(INVOCATION_CONTEXT_ENV) else { + return Ok(None); + }; + serde_json::from_str::(&text) + .map(|value| Some(value.to_string())) + .map_err(|error| { + PyValueError::new_err(format!( + "{INVOCATION_CONTEXT_ENV} must contain valid JSON: {error}" + )) + }) } fn heartbeat_interval() -> Duration { @@ -306,13 +310,11 @@ fn permission_notice( .database .as_deref() .is_some_and(|database| reason.starts_with(&missing_database_reason(database))); - let script = render_script( - &create_database_statement(&options) - .filter(|_| database_missing) - .into_iter() - .chain(ddl_statements(dialect, &options)) - .collect::>(), - ); + let script: String = ddl_script(dialect, &options); + let script: String = match create_database_statement(&options).filter(|_| database_missing) { + Some(statement) => format!("{}\n{script}", render_script(&[statement])), + None => script, + }; Some(format!( "Recall history is not being written{}: {reason}. An administrator can create the \ store {place} with:\n\n{script}\n", diff --git a/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs b/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs index f37350883c..64bfb3b3d5 100644 --- a/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs +++ b/crates/sqlbuild-recall/src/schema/main/store_version_with_views.rs @@ -8,11 +8,7 @@ use crate::schema::models::{SchemaAge, StoreVersion}; pub fn store_version_with_views(store: StoreVersion, view_definitions: &[&str]) -> StoreVersion { let Some(views_version) = SUPERSEDED_CLICKHOUSE_VIEW_MARKERS .iter() - .filter(|(marker, _)| { - view_definitions - .iter() - .any(|definition| definition.contains(marker)) - }) + .filter(|(marker, _)| holds(view_definitions, marker)) .map(|(_, version)| *version) .min() else { @@ -34,3 +30,9 @@ pub fn store_version_with_views(store: StoreVersion, view_definitions: &[&str]) other => other, } } + +fn holds(view_definitions: &[&str], marker: &str) -> bool { + view_definitions + .iter() + .any(|definition| definition.contains(marker)) +} diff --git a/crates/sqlbuild-recall/src/schema/tests/test_types.rs b/crates/sqlbuild-recall/src/schema/tests/test_types.rs index 7ff543cef8..58effc024b 100644 --- a/crates/sqlbuild-recall/src/schema/tests/test_types.rs +++ b/crates/sqlbuild-recall/src/schema/tests/test_types.rs @@ -53,7 +53,7 @@ pub(crate) struct ViewColumnsTestCase { pub(crate) struct ViewDefinitionsTestCase { pub(crate) description: &'static str, pub(crate) store: crate::schema::models::StoreVersion, - pub(crate) definitions: &'static [&'static str], + pub(crate) definitions: Vec, pub(crate) expected_store: crate::schema::models::StoreVersion, } diff --git a/crates/sqlbuild-recall/src/schema/tests/test_version.rs b/crates/sqlbuild-recall/src/schema/tests/test_version.rs index 64ebebb75f..0ef34b26e2 100644 --- a/crates/sqlbuild-recall/src/schema/tests/test_version.rs +++ b/crates/sqlbuild-recall/src/schema/tests/test_version.rs @@ -1,9 +1,7 @@ use std::collections::BTreeMap; use crate::schema::_helpers::clickhouse_views::clickhouse_view_bodies; -use crate::schema::constants::{ - RECALL_SCHEMA_VERSION, SUPERSEDED_CLICKHOUSE_VIEW_MARKERS, VIEW_NAMES, -}; +use crate::schema::constants::{RECALL_SCHEMA_VERSION, VIEW_NAMES}; use crate::schema::main::create_database_statement::create_database_statement; use crate::schema::main::schema_age::schema_age; use crate::schema::main::store_version_with_views::store_version_with_views; @@ -268,44 +266,60 @@ fn given_view_definitions_when_versioned_then_views_an_older_version_wrote_make_ ViewDefinitionsTestCase { description: "previous views with no version row are older, though columns match", store: StoreVersion::Unrecorded(SchemaAge::Current), - definitions: &[CURRENT_VIEW, PREVIOUS_VIEW], + definitions: vec![CURRENT_VIEW.to_owned(), PREVIOUS_VIEW.to_owned()], expected_store: StoreVersion::Unrecorded(SchemaAge::Older), }, ViewDefinitionsTestCase { description: "previous views recorded as current count as the previous version", store: recorded(RECALL_SCHEMA_VERSION), - definitions: &[PREVIOUS_VIEW], + definitions: vec![PREVIOUS_VIEW.to_owned()], expected_store: recorded(RECALL_SCHEMA_VERSION - 1), }, ViewDefinitionsTestCase { description: "current views keep the recorded version", store: recorded(RECALL_SCHEMA_VERSION), - definitions: &[CURRENT_VIEW], + definitions: vec![CURRENT_VIEW.to_owned()], expected_store: recorded(RECALL_SCHEMA_VERSION), }, ViewDefinitionsTestCase { description: "current views with no version row stay current", store: StoreVersion::Unrecorded(SchemaAge::Current), - definitions: &[CURRENT_VIEW], + definitions: vec![CURRENT_VIEW.to_owned()], expected_store: StoreVersion::Unrecorded(SchemaAge::Current), }, + ViewDefinitionsTestCase { + description: "this build's own ClickHouse views keep the current version", + store: recorded(RECALL_SCHEMA_VERSION), + definitions: view_statements(Dialect::ClickHouse, &options(None)) + .into_iter() + .map(|(_, statement)| statement) + .collect(), + expected_store: recorded(RECALL_SCHEMA_VERSION), + }, ViewDefinitionsTestCase { description: "a store recorded by a newer build is never made older", store: recorded(RECALL_SCHEMA_VERSION + 1), - definitions: &[PREVIOUS_VIEW], + definitions: vec![PREVIOUS_VIEW.to_owned()], expected_store: recorded(RECALL_SCHEMA_VERSION + 1), }, ViewDefinitionsTestCase { description: "views with columns of a newer build are never made older", store: StoreVersion::Unrecorded(SchemaAge::Newer), - definitions: &[PREVIOUS_VIEW], + definitions: vec![PREVIOUS_VIEW.to_owned()], expected_store: StoreVersion::Unrecorded(SchemaAge::Newer), }, ]; for test_case in test_cases { assert_eq!( - store_version_with_views(test_case.store, test_case.definitions), + store_version_with_views( + test_case.store, + &test_case + .definitions + .iter() + .map(String::as_str) + .collect::>() + ), test_case.expected_store, "{}", test_case.description @@ -313,28 +327,6 @@ fn given_view_definitions_when_versioned_then_views_an_older_version_wrote_make_ } } -#[test] -fn given_this_builds_clickhouse_views_when_checked_then_none_holds_a_superseded_marker() { - let statements: Vec = view_statements(Dialect::ClickHouse, &options(None)) - .into_iter() - .map(|(_, statement)| statement) - .collect(); - let marked: Vec<&str> = SUPERSEDED_CLICKHOUSE_VIEW_MARKERS - .iter() - .map(|(marker, _)| *marker) - .filter(|marker| { - statements - .iter() - .any(|statement| statement.contains(marker)) - }) - .collect(); - - assert_eq!( - (statements.len(), marked), - (VIEW_NAMES.len(), Vec::<&str>::new()) - ); -} - #[test] fn given_a_configured_database_when_rendered_then_it_is_created_only_if_missing() { let test_cases = [ diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py index 444911aeb2..cd222f4613 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py @@ -304,7 +304,7 @@ class ClickHouseRecallReaderSettingsTestCase: @dataclass(frozen=True) class ClickHouseRecallDatabaseCreationTestCase: description: str - writer_privileges: str | None + writer_privileges: str expected_notice: str expected_invocations: list[list[object]] @@ -327,8 +327,8 @@ class ClickHouseRecallMissingDatabaseTestCase: @dataclass(frozen=True) class ClickHouseRecallStaleViewsTestCase: description: str - version_row: str - limited_writer: bool + deleted_version_rows: str + writer_privileges: str expected_notices: int expected_stale_views: tuple[tuple[object, ...], ...] expected_version: tuple[tuple[object, ...], ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/helpers.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/helpers.py index 5fbce5caae..4bf97ac346 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/helpers.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/helpers.py @@ -1027,3 +1027,19 @@ def representative_store(project_dir: Path) -> subprocess.Popen[bytes]: timeout=120, ) return running + + +def drop_database(*, config: dict[str, object], database: str) -> None: + """Drop `database`, so a writer finds it missing.""" + + execute_clickhouse_sql(config=config, statements=(f"DROP DATABASE {database} SYNC",)) + + +def stale_views(*, config: dict[str, object], database: str) -> tuple[tuple[object, ...], ...]: + """How many views in `database` still depend on `join_use_nulls`, as the previous version's.""" + + return fetch_clickhouse_rows( + config=config, + sql="SELECT countIf(position(create_table_query, 'join_use_nulls') > 0) " + f"FROM system.tables WHERE database = '{database}' AND engine = 'View'", + ) diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py index bf6050ad15..d0f30a1c78 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py @@ -15,9 +15,11 @@ from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse.helpers import ( apply_printed_ddl, create_previous_views, + drop_database, execute_clickhouse_sql, fetch_clickhouse_rows, limited_user, + stale_views, write_recall_project, ) from tests.integration.src.sqlbuild.cli.recall.helpers import recall_rows, run @@ -28,24 +30,12 @@ _CLOSING_STATUS: str = "sqb build completed successfully; only its Recall history is affected.\n" -def _drop_database(*, config: dict[str, object], database: str) -> None: - execute_clickhouse_sql(config=config, statements=(f"DROP DATABASE {database} SYNC",)) - - -def _stale_views(*, config: dict[str, object], database: str) -> tuple[tuple[object, ...], ...]: - return fetch_clickhouse_rows( - config=config, - sql="SELECT countIf(position(create_table_query, 'join_use_nulls') > 0) " - f"FROM system.tables WHERE database = '{database}' AND engine = 'View'", - ) - - @pytest.mark.parametrize( "test_case", [ ClickHouseRecallDatabaseCreationTestCase( - description="an administrator's first write creates the database and says so", - writer_privileges=None, + description="a writer with every right on the database creates it and says so", + writer_privileges="ALL", expected_notice="Recall: created ClickHouse database `{database}` for the store.\n", expected_invocations=[[2]], ), @@ -64,26 +54,19 @@ def test_given_a_missing_database_when_a_writer_may_create_it_then_recall_create clickhouse_e2e_config: dict[str, object], recall_database: str, ) -> None: - _drop_database(config=clickhouse_e2e_config, database=recall_database) + drop_database(config=clickhouse_e2e_config, database=recall_database) notice: str = test_case.expected_notice.format(database=recall_database) - if test_case.writer_privileges is None: + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: project_dir: Path = write_recall_project( - parent=tmp_path, config=clickhouse_e2e_config, database=recall_database + parent=tmp_path, config=writer, database=recall_database ) first: subprocess.CompletedProcess[bytes] = run(project_dir, "build") second: subprocess.CompletedProcess[bytes] = run(project_dir, "build") - else: - with limited_user( - config=clickhouse_e2e_config, - database=recall_database, - privileges=test_case.writer_privileges, - ) as writer: - project_dir = write_recall_project( - parent=tmp_path, config=writer, database=recall_database - ) - first = run(project_dir, "build") - second = run(project_dir, "build") stderr: str = first.stderr.decode(errors="replace") assert (first.returncode, second.returncode) == (0, 0), stderr @@ -162,7 +145,7 @@ def test_given_a_missing_database_when_the_writer_may_not_create_it_then_the_sta clickhouse_e2e_config: dict[str, object], recall_database: str, ) -> None: - _drop_database(config=clickhouse_e2e_config, database=recall_database) + drop_database(config=clickhouse_e2e_config, database=recall_database) with limited_user( config=clickhouse_e2e_config, @@ -207,24 +190,24 @@ def test_given_a_missing_database_when_the_writer_may_not_create_it_then_the_sta [ ClickHouseRecallStaleViewsTestCase( description="previous views without a version row are replaced by an administrator", - version_row="DELETE", - limited_writer=False, + deleted_version_rows="1", + writer_privileges="ALL", expected_notices=0, expected_stale_views=((0,),), expected_version=((3,),), ), ClickHouseRecallStaleViewsTestCase( description="previous views recorded as current are replaced by an administrator", - version_row="KEEP", - limited_writer=False, + deleted_version_rows="0", + writer_privileges="ALL", expected_notices=0, expected_stale_views=((0,),), expected_version=((3,),), ), ClickHouseRecallStaleViewsTestCase( description="previous views without a version row make a SELECT, INSERT writer warn", - version_row="DELETE", - limited_writer=True, + deleted_version_rows="1", + writer_privileges="SELECT, INSERT", expected_notices=1, expected_stale_views=((4,),), expected_version=((0,),), @@ -248,33 +231,33 @@ def test_given_views_of_the_previous_version_when_building_then_they_count_as_ol names=("invocations", "statements", "output_invocations", "artifacts"), prefix="", ) - if test_case.version_row == "DELETE": - execute_clickhouse_sql( - config=clickhouse_e2e_config, - statements=( - f"ALTER TABLE {recall_database}.recall_meta DELETE WHERE 1 " - "SETTINGS mutations_sync = 2", + execute_clickhouse_sql( + config=clickhouse_e2e_config, + statements=( + f"ALTER TABLE {recall_database}.recall_meta " + f"DELETE WHERE {test_case.deleted_version_rows} SETTINGS mutations_sync = 2", + ), + ) + + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: + built: subprocess.CompletedProcess[bytes] = run( + write_recall_project( + parent=tmp_path / "writer", config=writer, database=recall_database ), + "build", ) - if test_case.limited_writer: - with limited_user(config=clickhouse_e2e_config, database=recall_database) as writer: - built: subprocess.CompletedProcess[bytes] = run( - write_recall_project( - parent=tmp_path / "writer", config=writer, database=recall_database - ), - "build", - ) - else: - built = run(admin, "build") - stderr: str = built.stderr.decode(errors="replace") assert built.returncode == 0, stderr assert ( stderr.count("is behind this SQLBuild version, and this user may not upgrade it") == test_case.expected_notices ) - assert _stale_views(config=clickhouse_e2e_config, database=recall_database) == ( + assert stale_views(config=clickhouse_e2e_config, database=recall_database) == ( test_case.expected_stale_views ) assert ( diff --git a/tests/integration/src/sqlbuild/cli/recall/helpers.py b/tests/integration/src/sqlbuild/cli/recall/helpers.py index baa8b2771f..64cc99889a 100644 --- a/tests/integration/src/sqlbuild/cli/recall/helpers.py +++ b/tests/integration/src/sqlbuild/cli/recall/helpers.py @@ -16,6 +16,7 @@ from collections.abc import Iterator from contextlib import contextmanager from pathlib import Path +from typing import Any PYTHON_CLI: Path = Path(sys.executable).with_name("sqlbuild-python") NATIVE_SQB: Path = Path(sys.executable).with_name("sqb") @@ -582,7 +583,7 @@ def worktree_project(parent: Path) -> tuple[Path, str]: def documented_commands(*, invocation_id: str) -> list[list[str]]: - """The arguments of each command under the Recall page's common questions, for one invocation.""" + """Each command's arguments under the Recall page's common questions, for one invocation.""" page: str = RECALL_CONCEPT_PAGE.read_text(encoding="utf-8") section: str = page.split(_COMMON_QUESTIONS, 1)[1].split("\n### ", 1)[0] @@ -590,3 +591,13 @@ def documented_commands(*, invocation_id: str) -> list[list[str]]: shlex.split(block.replace("", invocation_id))[1:] for block in _BASH_BLOCK.findall(section) ] + + +def selected_columns(*, result: dict[str, Any], columns: tuple[str, ...]) -> list[list[object]]: + """The `columns` of each row of a `--json` query result, in that order.""" + + positions: list[int] = [result["columns"].index(column) for column in columns] + rows: list[list[object]] = [] + for row in result["rows"]: + rows.append([row[position] for position in positions]) + return rows diff --git a/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py b/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py index ef58685f3a..b06b81c621 100644 --- a/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py +++ b/tests/integration/src/sqlbuild/cli/recall/test_recall_invocation_facts.py @@ -1,7 +1,6 @@ import json import subprocess from pathlib import Path -from typing import Any import pytest @@ -17,6 +16,7 @@ documented_commands, recall_rows, run, + selected_columns, worktree_project, write_project, ) @@ -85,11 +85,10 @@ def test_given_an_orchestrated_build_when_running_the_documented_queries_then_th assert built.returncode == 0, built.stderr assert len(commands) == 4 assert answered.returncode == 0, answered.stderr - result: dict[str, list[Any]] = json.loads(answered.stdout) - positions: list[int] = [result["columns"].index(column) for column in test_case.columns] - assert [ - [row[position] for position in positions] for row in result["rows"] - ] == test_case.expected_rows + assert ( + selected_columns(result=json.loads(answered.stdout), columns=test_case.columns) + == test_case.expected_rows + ) assert printed.returncode == 0, printed.stderr assert len(printed.stdout.splitlines()) == 50 assert printed.stdout.endswith(built.stdout.splitlines(keepends=True)[-1]) From 5bd0595b2485bd17dfeadc947deca4cb3c4ce56c Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 19:59:52 +0100 Subject: [PATCH 4/6] fix: keep the Recall database creation notice through failed setups and close it plainly --- .../src/dispatch/main/closing_status.rs | 13 +- .../src/dispatch/tests/test_closing_status.rs | 17 ++- .../src/dispatch/tests/test_types.rs | 1 + .../bindings/_helpers/cli/observability.rs | 2 +- .../_helpers/recall/clickhouse_store.rs | 25 +++- .../src/bindings/_helpers/recall/session.rs | 36 +++-- crates/sqlbuild-recall/src/store/types.rs | 5 + .../src/writer/_helpers/delivery.rs | 7 +- .../src/writer/_helpers/queue.rs | 3 + .../src/writer/_helpers/session.rs | 6 + .../src/writer/tests/helpers.rs | 23 ++- .../src/writer/tests/test_session.rs | 47 ++++++- .../src/writer/tests/test_types.rs | 63 ++++++++- .../docs/concepts/observability/recall.md | 17 ++- .../commands/main/clickhouse/_test_types.py | 18 +++ .../test_clickhouse_recall_setup.py | 131 +++++++++++++++++- .../docs/concepts/observability/recall.mdx | 17 ++- 17 files changed, 390 insertions(+), 41 deletions(-) diff --git a/crates/sqlbuild-cli/src/dispatch/main/closing_status.rs b/crates/sqlbuild-cli/src/dispatch/main/closing_status.rs index 53b8d3a897..69b38753f8 100644 --- a/crates/sqlbuild-cli/src/dispatch/main/closing_status.rs +++ b/crates/sqlbuild-cli/src/dispatch/main/closing_status.rs @@ -1,13 +1,14 @@ //! The closing line after an incomplete-delivery notice, so the command's own outcome is the last word. -/// `sqb `'s outcome by exit code, naming the only thing a preceding notice affects. +/// `sqb `'s outcome by exit code, naming the only thing a preceding notice affects; with nothing `affected`, as after a purely informational notice, success is reported plainly. #[must_use] -pub fn closing_status(command: &str, exit_code: Option, affected: &str) -> String { - match exit_code { - Some(0) => { +pub fn closing_status(command: &str, exit_code: Option, affected: Option<&str>) -> String { + match (exit_code, affected) { + (Some(0), Some(affected)) => { format!("sqb {command} completed successfully; only its {affected} is affected.\n") } - Some(code) => format!("sqb {command} failed with exit code {code}.\n"), - None => format!("sqb {command} failed.\n"), + (Some(0), None) => format!("sqb {command} completed successfully.\n"), + (Some(code), _) => format!("sqb {command} failed with exit code {code}.\n"), + (None, _) => format!("sqb {command} failed.\n"), } } diff --git a/crates/sqlbuild-cli/src/dispatch/tests/test_closing_status.rs b/crates/sqlbuild-cli/src/dispatch/tests/test_closing_status.rs index 188b2b2071..6b730bcffb 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/test_closing_status.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/test_closing_status.rs @@ -7,23 +7,38 @@ fn given_command_outcome_when_closing_a_notice_then_the_outcome_is_the_last_line ClosingStatusTestCase { description: "success names the only affected output", exit_code: Some(0), + affected: Some("lifecycle event export"), expected_status: "sqb audit completed successfully; only its lifecycle event export \ is affected.\n", }, ClosingStatusTestCase { description: "failure reports the exit code", exit_code: Some(2), + affected: Some("lifecycle event export"), expected_status: "sqb audit failed with exit code 2.\n", }, ClosingStatusTestCase { description: "unknown exit code reports failure", exit_code: None, + affected: Some("lifecycle event export"), expected_status: "sqb audit failed.\n", }, + ClosingStatusTestCase { + description: "success after an informational notice names nothing affected", + exit_code: Some(0), + affected: None, + expected_status: "sqb audit completed successfully.\n", + }, + ClosingStatusTestCase { + description: "failure after an informational notice still reports the exit code", + exit_code: Some(1), + affected: None, + expected_status: "sqb audit failed with exit code 1.\n", + }, ]; for test_case in test_cases { assert_eq!( - closing_status("audit", test_case.exit_code, "lifecycle event export"), + closing_status("audit", test_case.exit_code, test_case.affected), test_case.expected_status, "{}", test_case.description diff --git a/crates/sqlbuild-cli/src/dispatch/tests/test_types.rs b/crates/sqlbuild-cli/src/dispatch/tests/test_types.rs index 7f2415c4b8..b914ebdf4a 100644 --- a/crates/sqlbuild-cli/src/dispatch/tests/test_types.rs +++ b/crates/sqlbuild-cli/src/dispatch/tests/test_types.rs @@ -23,5 +23,6 @@ pub(super) struct EventExportWarningTestCase { pub(super) struct ClosingStatusTestCase { pub(super) description: &'static str, pub(super) exit_code: Option, + pub(super) affected: Option<&'static str>, pub(super) expected_status: &'static str, } diff --git a/crates/sqlbuild-python/src/bindings/_helpers/cli/observability.rs b/crates/sqlbuild-python/src/bindings/_helpers/cli/observability.rs index fb81e25eea..6a26b63e16 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/cli/observability.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/cli/observability.rs @@ -211,7 +211,7 @@ fn warn_incomplete_export( return Ok(()); }; let closing: String = if command_succeeded { - closing_status(command, Some(0), LIFECYCLE_EVENT_EXPORT) + closing_status(command, Some(0), Some(LIFECYCLE_EVENT_EXPORT)) } else { String::new() }; diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs index 4b91c8e645..4cc6440c5d 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/clickhouse_store.rs @@ -2,6 +2,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::{Path, PathBuf}; +use std::sync::{Mutex, PoisonError}; use std::time::{Duration, Instant, SystemTime}; use pyo3::prelude::Python; @@ -95,6 +96,8 @@ pub(crate) struct ClickHouseStoreOpener { pub(crate) location: ClickHouseLocation, pub(crate) retention_notice: fn(&Path, &str) -> bool, pub(crate) project_dir: PathBuf, + /// What opening did that nothing is wrong with, such as creating the database, kept until the session takes it even when a later step fails. + pub(crate) information: Mutex>, } /// The Recall relations present: tables with their `CREATE` queries, views, and the stored schema version. @@ -120,6 +123,7 @@ struct ClickHouseStore { connection: Option, columns: HashMap>, notices: Vec, + information: Vec, } fn text(value: Option<&RecallValue>) -> String { @@ -279,7 +283,7 @@ impl ClickHouseStore { }; match self.command(py, &statement, "create the Recall database") { Ok(()) => { - self.notices.push(format!( + self.information.push(format!( "created ClickHouse database `{}` for the store", self.location.database() )); @@ -634,8 +638,14 @@ impl RecallStoreOpener for ClickHouseStoreOpener { connection: Some(connect_store(py, &self.location, ConnectionRole::Writer)?), columns: HashMap::new(), notices: Vec::new(), + information: Vec::new(), }; - match store.ensure_schema(py, envelope) { + let ensured = store.ensure_schema(py, envelope); + self.information + .lock() + .unwrap_or_else(PoisonError::into_inner) + .append(&mut store.information); + match ensured { Ok(ReadyStore { tables, behind }) => { if !behind.is_empty() && (self.retention_notice)( @@ -663,6 +673,15 @@ impl RecallStoreOpener for ClickHouseStoreOpener { }) } + fn take_information(&self) -> Vec { + std::mem::take( + &mut *self + .information + .lock() + .unwrap_or_else(PoisonError::into_inner), + ) + } + fn batch_rows(&self) -> usize { BATCH_ROWS } @@ -688,6 +707,7 @@ impl ClickHouseReader { )?), columns: HashMap::new(), notices: Vec::new(), + information: Vec::new(), }; Ok(Self { store }) } @@ -722,6 +742,7 @@ impl ClickHouseReader { connection: Some(connect_store(py, location, ConnectionRole::Reader)?), columns: HashMap::new(), notices: Vec::new(), + information: Vec::new(), }; match store.database_exists(py) { Ok(true) => {} diff --git a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs index 621e57d7d1..9dadbec08f 100644 --- a/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs +++ b/crates/sqlbuild-python/src/bindings/_helpers/recall/session.rs @@ -2,7 +2,7 @@ use std::io::ErrorKind; use std::path::{Path, PathBuf}; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant, SystemTime}; use pyo3::exceptions::PyValueError; @@ -383,6 +383,7 @@ fn started_session( location: ClickHouseLocation::new(config, database, settings.retention_days), retention_notice: notice_due, project_dir: project_dir.to_path_buf(), + information: Mutex::default(), })), StoreLocation::Unsupported { connection, @@ -464,18 +465,21 @@ pub(crate) fn start_invocation_recall( }) } +/// `notice` as one Recall line, or block when it already ends with a newline. +fn prefixed(notice: &str) -> String { + if notice.ends_with('\n') { + format!("Recall: {notice}") + } else { + format!("Recall: {notice}.\n") + } +} + /// What Recall must tell the user about this run once delivery has finished. fn closing_notices(recall: &InvocationRecall, session: &RecallSession) -> Vec { let mut notices: Vec = session .notices() - .into_iter() - .map(|notice| { - if notice.ends_with('\n') { - format!("Recall: {notice}") - } else { - format!("Recall: {notice}.\n") - } - }) + .iter() + .map(|notice| prefixed(notice)) .collect(); if let (Some(SetupOutcome::PermissionDenied { reason }), Some((settings, project_dir))) = (session.wait_for_setup(Duration::ZERO), &recall.settings) @@ -514,11 +518,21 @@ pub(crate) fn finish_invocation_recall( return Err(interrupt); } } + let information: Vec = session + .information() + .iter() + .map(|text| prefixed(text)) + .collect(); let notices: Vec = closing_notices(recall, session); - if !notices.is_empty() { + if !information.is_empty() || !notices.is_empty() { + let affected: Option<&str> = (!notices.is_empty()).then_some(RECALL_HISTORY); notify( py, - Some(notices.concat() + &closing_status(&recall.command, exit_code, RECALL_HISTORY)), + Some( + information.concat() + + ¬ices.concat() + + &closing_status(&recall.command, exit_code, affected), + ), ); } Ok(()) diff --git a/crates/sqlbuild-recall/src/store/types.rs b/crates/sqlbuild-recall/src/store/types.rs index 8789e007cf..7330233b2f 100644 --- a/crates/sqlbuild-recall/src/store/types.rs +++ b/crates/sqlbuild-recall/src/store/types.rs @@ -32,6 +32,11 @@ pub trait RecallStoreOpener: Send + Sync + std::fmt::Debug { give_up: Instant, ) -> Result, StoreError>; + /// What opening did that the user should know although nothing is wrong, such as a database it created, gathered since the last call. Kept across failed and retried opens, so each is reported exactly once. + fn take_information(&self) -> Vec { + Vec::new() + } + /// Most rows one insert batch takes. fn batch_rows(&self) -> usize; diff --git a/crates/sqlbuild-recall/src/writer/_helpers/delivery.rs b/crates/sqlbuild-recall/src/writer/_helpers/delivery.rs index 976362ed7f..dc9f69ab2f 100644 --- a/crates/sqlbuild-recall/src/writer/_helpers/delivery.rs +++ b/crates/sqlbuild-recall/src/writer/_helpers/delivery.rs @@ -113,7 +113,12 @@ fn open_store(shared: &Shared, config: &SessionConfig) -> Result }), - SessionStore::Hosted(opener) => open_hosted(shared, config, opener.as_ref()), + SessionStore::Hosted(opener) => { + let opened = open_hosted(shared, config, opener.as_ref()); + let information: Vec = opener.take_information(); + locked(&shared.state).information.extend(information); + opened + } } } diff --git a/crates/sqlbuild-recall/src/writer/_helpers/queue.rs b/crates/sqlbuild-recall/src/writer/_helpers/queue.rs index d2d55a7f7d..be5c858d56 100644 --- a/crates/sqlbuild-recall/src/writer/_helpers/queue.rs +++ b/crates/sqlbuild-recall/src/writer/_helpers/queue.rs @@ -104,6 +104,8 @@ pub(crate) struct WriterState { pub(crate) degraded: Option, /// Store notices the user should see, such as a corrupt store moved aside. pub(crate) notices: Vec, + /// Informational store notices that report no problem, such as a database Recall created. + pub(crate) information: Vec, /// The context row's content while its replacement-filter flag is still unknown. pub(crate) held_context: Option, capacity: (usize, usize), @@ -151,6 +153,7 @@ impl WriterState { rejecting: false, degraded: None, notices: Vec::new(), + information: Vec::new(), held_context: None, capacity, } diff --git a/crates/sqlbuild-recall/src/writer/_helpers/session.rs b/crates/sqlbuild-recall/src/writer/_helpers/session.rs index e36d47770b..240150540a 100644 --- a/crates/sqlbuild-recall/src/writer/_helpers/session.rs +++ b/crates/sqlbuild-recall/src/writer/_helpers/session.rs @@ -279,4 +279,10 @@ impl RecallSession { pub fn notices(&self) -> Vec { locked(&self.shared.state).notices.clone() } + + /// Informational store notices, such as a database Recall created; they report no problem. + #[must_use] + pub fn information(&self) -> Vec { + locked(&self.shared.state).information.clone() + } } diff --git a/crates/sqlbuild-recall/src/writer/tests/helpers.rs b/crates/sqlbuild-recall/src/writer/tests/helpers.rs index c26f04538d..22b2ed1912 100644 --- a/crates/sqlbuild-recall/src/writer/tests/helpers.rs +++ b/crates/sqlbuild-recall/src/writer/tests/helpers.rs @@ -17,8 +17,8 @@ use crate::writer::models::{ DeliveryStream, DeliverySummary, Priority, SessionConfig, SessionStore, SetupOutcome, }; use crate::writer::tests::test_types::{ - CopyingReplacement, OutputBytesStore, PartialStore, PrefixReplacement, RecordedSession, - RecordingReplacement, + CopyingReplacement, CreatingStore, OutputBytesStore, PartialStore, PrefixReplacement, + RecordedSession, RecordingReplacement, }; /// The envelope of invocation `invocation_id`. @@ -586,3 +586,22 @@ pub(crate) fn replacement_order(priorities: &[Priority]) -> Vec { drop(Materializer::new(envelope("ordered")).build(batch, None)); std::mem::take(&mut *seen.lock().expect("the record is not poisoned")) } + +/// The informational notices of a session whose store opens as `failures` say after creating its database, and whether setup was refused for permission. +pub(crate) fn creation_notices( + failures: Vec>, +) -> (Vec, bool) { + let session = start_recall_session(SessionConfig { + store: SessionStore::Hosted(Arc::new(CreatingStore { + failures, + ..CreatingStore::default() + })), + ..config(PathBuf::from("unused"), "creating") + }); + let setup = session.wait_for_setup(Duration::from_secs(10)); + drop(session.close()); + ( + session.information(), + matches!(setup, Some(SetupOutcome::PermissionDenied { .. })), + ) +} diff --git a/crates/sqlbuild-recall/src/writer/tests/test_session.rs b/crates/sqlbuild-recall/src/writer/tests/test_session.rs index ab99a7a55e..c8a9c5d218 100644 --- a/crates/sqlbuild-recall/src/writer/tests/test_session.rs +++ b/crates/sqlbuild-recall/src/writer/tests/test_session.rs @@ -2,12 +2,13 @@ use std::time::Duration; use crate::records::models::{OutputStream, RecallValue}; use crate::schema::models::Table; +use crate::store::errors::StoreErrorKind; use crate::writer::models::{Priority, SetupOutcome}; #[cfg(unix)] use crate::writer::tests::helpers::unwritable_session; use crate::writer::tests::helpers::{ - artifact_counts, close_under_held_lock, event_counts, insert_heartbeat, + artifact_counts, close_under_held_lock, creation_notices, event_counts, insert_heartbeat, invocation_during_discovery, output_stored_while_running, partially_inserted, pending_event_counts, record_run, recorded_session, replaced_session, replacement_order, sent_before_the_window, session_on_corrupt_store, written_to_core_release_store, @@ -15,10 +16,10 @@ use crate::writer::tests::helpers::{ #[cfg(unix)] use crate::writer::tests::test_types::UnwritableTestCase; use crate::writer::tests::test_types::{ - ArtifactSizeTestCase, CoreReleaseStoreTestCase, CorruptStoreTestCase, DiscoveryTestCase, - EarlyBatchTestCase, EventBuildTestCase, HeldLockTestCase, PartialInsertTestCase, - PendingEventTestCase, RecordedSession, ReplacementOrderTestCase, ReplacementTestCase, - SessionTestCase, StoredWhileRunningTestCase, + ArtifactSizeTestCase, CoreReleaseStoreTestCase, CorruptStoreTestCase, CreationNoticeTestCase, + DiscoveryTestCase, EarlyBatchTestCase, EventBuildTestCase, HeldLockTestCase, + PartialInsertTestCase, PendingEventTestCase, RecordedSession, ReplacementOrderTestCase, + ReplacementTestCase, SessionTestCase, StoredWhileRunningTestCase, }; const CLOSE_SLACK_SECONDS: f64 = 0.3; @@ -399,3 +400,39 @@ fn given_a_byte_heavy_burst_when_the_window_is_long_then_it_is_sent_early_and_no ); } } + +#[test] +fn given_a_created_database_when_later_setup_fails_or_retries_then_its_notice_is_kept_once() { + let test_cases = [ + CreationNoticeTestCase { + description: "creation followed by refused DDL keeps the notice", + failures: vec![Some(StoreErrorKind::Permission)], + expected_information: vec!["created database `history`".to_owned()], + expected_permission_denied: true, + }, + CreationNoticeTestCase { + description: "a busy retry after creation reports it once", + failures: vec![Some(StoreErrorKind::Busy), None], + expected_information: vec!["created database `history`".to_owned()], + expected_permission_denied: false, + }, + CreationNoticeTestCase { + description: "a store opened at once reports the creation", + failures: vec![None], + expected_information: vec!["created database `history`".to_owned()], + expected_permission_denied: false, + }, + ]; + + for test_case in test_cases { + assert_eq!( + creation_notices(test_case.failures), + ( + test_case.expected_information, + test_case.expected_permission_denied + ), + "{}", + test_case.description + ); + } +} diff --git a/crates/sqlbuild-recall/src/writer/tests/test_types.rs b/crates/sqlbuild-recall/src/writer/tests/test_types.rs index f240a46e1d..67ab3e837c 100644 --- a/crates/sqlbuild-recall/src/writer/tests/test_types.rs +++ b/crates/sqlbuild-recall/src/writer/tests/test_types.rs @@ -3,7 +3,7 @@ use std::time::{Duration, Instant}; use crate::records::models::{OutputChunk, OutputStream, RecallRow, RecallValue, RowEnvelope}; use crate::schema::models::Table; -use crate::store::errors::StoreError; +use crate::store::errors::{StoreError, StoreErrorKind}; use crate::store::models::InsertOutcome; use crate::store::types::{RecallStore, RecallStoreOpener}; use crate::writer::models::{ @@ -288,3 +288,64 @@ impl PendingLifecycleEvent for RecordingReplacement { ReplacedEvents::Published(Vec::new()) } } + +pub(crate) struct CreationNoticeTestCase { + pub(crate) description: &'static str, + /// The failure of each open after the first creates the database; `None` opens the store. + pub(crate) failures: Vec>, + pub(crate) expected_information: Vec, + pub(crate) expected_permission_denied: bool, +} + +/// A hosted store whose first open creates its database, then fails or opens as `failures` say. +#[derive(Debug, Default)] +pub(crate) struct CreatingStore { + pub(crate) failures: Vec>, + pub(crate) attempts: Mutex, + pub(crate) information: Mutex>, +} + +impl RecallStoreOpener for CreatingStore { + fn open( + &self, + _envelope: &RowEnvelope, + _give_up: Instant, + ) -> Result, StoreError> { + let mut attempts = self + .attempts + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let attempt: usize = *attempts; + *attempts += 1; + if attempt == 0 { + self.information + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push("created database `history`".to_owned()); + } + match self.failures.get(attempt).copied().flatten() { + Some(kind) => Err(StoreError { + kind, + message: "the store could not be set up".to_owned(), + }), + None => Ok(Box::new(OutputBytesStore::default())), + } + } + + fn take_information(&self) -> Vec { + std::mem::take( + &mut *self + .information + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner), + ) + } + + fn batch_rows(&self) -> usize { + 1_000 + } + + fn batch_window(&self) -> Duration { + Duration::from_millis(10) + } +} diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md index 4c08e2225c..370ed6ead2 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md @@ -302,6 +302,10 @@ again. - **Setup.** On first use the writer creates the database if it is missing, printing ``Recall: created ClickHouse database `observability` for the store.`` once, then the `recall_*` MergeTree tables and the views above. An existing database is used as it is, whatever its engine. + The database is created on the server the writer connects to, never `ON CLUSTER`: on a + self-managed cluster whose default database engine is not `Replicated`, create the database + yourself first (`ENGINE = Replicated(...)` or `ON CLUSTER`), and Recall leaves it alone. + ClickHouse Cloud needs nothing. If the user may not create the store, the command still succeeds and prints the statements, starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from them accepts writers limited to `SELECT` and `INSERT`. @@ -405,7 +409,9 @@ These run as written on the local SQLite store; on ClickHouse, read JSON with `JSONExtractString(external_context, 'integration', 'job_name')`. The latest runs of an orchestrated job, by the context its integration records (the Dagster -integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`): +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). This +finds runs recorded by SQLBuild 0.168.0 or later; earlier versions stored the context in a form +the JSON functions cannot read, so those runs are skipped without an error: ```bash sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms @@ -438,10 +444,11 @@ sqb recall output --tail 50 ### From a statement to the warehouse -`statements.query_id` is the warehouse's own ID for the statement, where the adapter reports one: -the Snowflake query ID, or the `query_id` ClickHouse logs in `system.query_log`. It is `NULL` on -adapters without one, such as DuckDB. Look it up in the warehouse's query history to see what -Recall does not record, such as the warehouse size, queueing and spilling behind a slow statement. +`statements.query_id` is the warehouse's own ID for the statement on Snowflake (the query ID) and +ClickHouse (the `query_id` in `system.query_log`). Look it up in the warehouse's query history to +see what Recall does not record, such as the warehouse size, queueing and spilling behind a slow +statement. It is `NULL` on DuckDB and BigQuery; a BigQuery statement's job ID is in its +`statement_submitted` event instead, as `payload` field `job_id` in `events`. ## Relationship to compute logs diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py index cd222f4613..6283ccbede 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/_test_types.py @@ -340,3 +340,21 @@ class ClickHouseRecallExternalContextTestCase: external_context: str sql: str expected_rows: list[list[object]] + + +@dataclass(frozen=True) +class ClickHouseRecallRefusedTablesTestCase: + description: str + writer_privileges: str + expected_fragments: tuple[str, ...] + expected_databases: tuple[tuple[object, ...], ...] + + +@dataclass(frozen=True) +class ClickHouseRecallDocumentedGrantsTestCase: + description: str + writer_privileges: str + dropped_column: tuple[str, str] + expected_columns: tuple[tuple[object, ...], ...] + expected_stale_views: tuple[tuple[object, ...], ...] + expected_version: tuple[tuple[object, ...], ...] diff --git a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py index d0f30a1c78..b0080f9766 100644 --- a/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py +++ b/tests/e2e/src/sqlbuild/cli/commands/main/clickhouse/test_clickhouse_recall_setup.py @@ -7,9 +7,11 @@ from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse._test_types import ( ClickHouseRecallDatabaseCreationTestCase, + ClickHouseRecallDocumentedGrantsTestCase, ClickHouseRecallExistingDatabaseTestCase, ClickHouseRecallExternalContextTestCase, ClickHouseRecallMissingDatabaseTestCase, + ClickHouseRecallRefusedTablesTestCase, ClickHouseRecallStaleViewsTestCase, ) from tests.e2e.src.sqlbuild.cli.commands.main.clickhouse.helpers import ( @@ -28,6 +30,7 @@ "CREATE DATABASE, CREATE TABLE, CREATE VIEW, DROP TABLE, ALTER ADD COLUMN, SELECT, INSERT" ) _CLOSING_STATUS: str = "sqb build completed successfully; only its Recall history is affected.\n" +_SUCCESS_STATUS: str = "sqb build completed successfully.\n" @pytest.mark.parametrize( @@ -71,7 +74,7 @@ def test_given_a_missing_database_when_a_writer_may_create_it_then_recall_create stderr: str = first.stderr.decode(errors="replace") assert (first.returncode, second.returncode) == (0, 0), stderr assert stderr.count(notice) == 1 - assert stderr.endswith(notice + _CLOSING_STATUS) + assert stderr.endswith(notice + _SUCCESS_STATUS) assert b"Recall" not in second.stderr assert ( recall_rows( @@ -301,5 +304,131 @@ def test_given_an_orchestrated_build_when_queried_on_clickhouse_then_its_context assert recall_rows(project_dir, test_case.sql) == test_case.expected_rows +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallRefusedTablesTestCase( + description="a writer that may create the database but not tables still hears of it", + writer_privileges="CREATE DATABASE, SELECT, INSERT", + expected_fragments=( + "Recall: created ClickHouse database `{database}` for the store.\n", + "Recall history is not being written for target `dev`: cannot create the Recall " + "tables", + "CREATE TABLE IF NOT EXISTS `{database}`.`recall_meta`", + _CLOSING_STATUS, + ), + expected_databases=((1,),), + ), + ], + ids=lambda case: case.description, +) +def test_given_a_writer_that_may_create_only_the_database_when_building_then_both_are_reported( + test_case: ClickHouseRecallRefusedTablesTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + drop_database(config=clickhouse_e2e_config, database=recall_database) + + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: + built: subprocess.CompletedProcess[bytes] = run( + write_recall_project(parent=tmp_path, config=writer, database=recall_database), "build" + ) + + stderr: str = built.stderr.decode(errors="replace") + assert built.returncode == 0, stderr + assert [ + stderr.count(fragment.format(database=recall_database)) + for fragment in test_case.expected_fragments + ] == [1 for _ in test_case.expected_fragments] + assert stderr.endswith(_CLOSING_STATUS) + assert ( + fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql=f"SELECT count() FROM system.databases WHERE name = '{recall_database}'", + ) + == test_case.expected_databases + ) + + +@pytest.mark.parametrize( + "test_case", + [ + ClickHouseRecallDocumentedGrantsTestCase( + description="the documented writer grants replace older views and add a column", + writer_privileges=_STORE_MANAGER_PRIVILEGES, + dropped_column=("recall_invocations", "argv_digest"), + expected_columns=((1,),), + expected_stale_views=((0,),), + expected_version=((3,),), + ), + ], + ids=lambda case: case.description, +) +def test_given_the_documented_writer_grants_when_the_store_is_behind_then_the_writer_upgrades_it( + test_case: ClickHouseRecallDocumentedGrantsTestCase, + tmp_path: Path, + clickhouse_e2e_config: dict[str, object], + recall_database: str, +) -> None: + table, column = test_case.dropped_column + admin: Path = write_recall_project( + parent=tmp_path / "admin", config=clickhouse_e2e_config, database=recall_database + ) + assert run(admin, "build").returncode == 0 + create_previous_views( + config=clickhouse_e2e_config, + database=recall_database, + names=("invocations", "statements", "output_invocations", "artifacts"), + prefix="", + ) + execute_clickhouse_sql( + config=clickhouse_e2e_config, + statements=( + f"ALTER TABLE {recall_database}.{table} DROP COLUMN {column}", + f"ALTER TABLE {recall_database}.recall_meta UPDATE value = '2' " + "WHERE component = 'schema' SETTINGS mutations_sync = 2", + ), + ) + + with limited_user( + config=clickhouse_e2e_config, + database=recall_database, + privileges=test_case.writer_privileges, + ) as writer: + built: subprocess.CompletedProcess[bytes] = run( + write_recall_project( + parent=tmp_path / "writer", config=writer, database=recall_database + ), + "build", + ) + + assert built.returncode == 0, built.stderr + assert b"Recall" not in built.stderr + assert ( + fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql=f"SELECT count() FROM system.columns WHERE database = '{recall_database}' " + f"AND table = '{table}' AND name = '{column}'", + ) + == test_case.expected_columns + ) + assert stale_views(config=clickhouse_e2e_config, database=recall_database) == ( + test_case.expected_stale_views + ) + assert ( + fetch_clickhouse_rows( + config=clickhouse_e2e_config, + sql=f"SELECT toInt64(max(toInt64OrZero(value))) FROM {recall_database}.recall_meta " + "WHERE component = 'schema'", + ) + == test_case.expected_version + ) + + if __name__ == "__main__": pytest.main([__file__, "-vv"]) diff --git a/website/src/content/docs/docs/concepts/observability/recall.mdx b/website/src/content/docs/docs/concepts/observability/recall.mdx index b5a505ee21..bb6674bb92 100644 --- a/website/src/content/docs/docs/concepts/observability/recall.mdx +++ b/website/src/content/docs/docs/concepts/observability/recall.mdx @@ -285,6 +285,10 @@ again. - **Setup.** On first use the writer creates the database if it is missing, printing ``Recall: created ClickHouse database `observability` for the store.`` once, then the `recall_*` MergeTree tables and the views above. An existing database is used as it is, whatever its engine. + The database is created on the server the writer connects to, never `ON CLUSTER`: on a + self-managed cluster whose default database engine is not `Replicated`, create the database + yourself first (`ENGINE = Replicated(...)` or `ON CLUSTER`), and Recall leaves it alone. + ClickHouse Cloud needs nothing. If the user may not create the store, the command still succeeds and prints the statements, starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from them accepts writers limited to `SELECT` and `INSERT`. @@ -389,7 +393,9 @@ These run as written on the local SQLite store; on ClickHouse, read JSON with `JSONExtractString(external_context, 'integration', 'job_name')`. The latest runs of an orchestrated job, by the context its integration records (the Dagster -integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`): +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). This +finds runs recorded by SQLBuild 0.168.0 or later; earlier versions stored the context in a form +the JSON functions cannot read, so those runs are skipped without an error: ```bash sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms @@ -422,10 +428,11 @@ sqb recall output --tail 50 ### From a statement to the warehouse -`statements.query_id` is the warehouse's own ID for the statement, where the adapter reports one: -the Snowflake query ID, or the `query_id` ClickHouse logs in `system.query_log`. It is `NULL` on -adapters without one, such as DuckDB. Look it up in the warehouse's query history to see what -Recall does not record, such as the warehouse size, queueing and spilling behind a slow statement. +`statements.query_id` is the warehouse's own ID for the statement on Snowflake (the query ID) and +ClickHouse (the `query_id` in `system.query_log`). Look it up in the warehouse's query history to +see what Recall does not record, such as the warehouse size, queueing and spilling behind a slow +statement. It is `NULL` on DuckDB and BigQuery; a BigQuery statement's job ID is in its +`statement_submitted` event instead, as `payload` field `job_id` in `events`. ## Relationship to compute logs From 18fd7466174bff0cc5a170248136088f0fe2374a Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:20:29 +0100 Subject: [PATCH 5/6] docs: describe which runs the orchestrated-job query finds without naming a release --- .../references/docs/concepts/observability/recall.md | 6 +++--- .../src/content/docs/docs/concepts/observability/recall.mdx | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md index 370ed6ead2..9754805bd6 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md @@ -409,9 +409,9 @@ These run as written on the local SQLite store; on ClickHouse, read JSON with `JSONExtractString(external_context, 'integration', 'job_name')`. The latest runs of an orchestrated job, by the context its integration records (the Dagster -integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). This -finds runs recorded by SQLBuild 0.168.0 or later; earlier versions stored the context in a form -the JSON functions cannot read, so those runs are skipped without an error: +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). Runs +recorded before this context was stored as JSON hold it in a form the JSON functions cannot read, +so this query skips them without an error: ```bash sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms diff --git a/website/src/content/docs/docs/concepts/observability/recall.mdx b/website/src/content/docs/docs/concepts/observability/recall.mdx index bb6674bb92..3d9ee8095a 100644 --- a/website/src/content/docs/docs/concepts/observability/recall.mdx +++ b/website/src/content/docs/docs/concepts/observability/recall.mdx @@ -393,9 +393,9 @@ These run as written on the local SQLite store; on ClickHouse, read JSON with `JSONExtractString(external_context, 'integration', 'job_name')`. The latest runs of an orchestrated job, by the context its integration records (the Dagster -integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). This -finds runs recorded by SQLBuild 0.168.0 or later; earlier versions stored the context in a form -the JSON functions cannot read, so those runs are skipped without an error: +integration records `run_id`, `job_name`, `step_key`, `retry_number` and `partition_key`). Runs +recorded before this context was stored as JSON hold it in a form the JSON functions cannot read, +so this query skips them without an error: ```bash sqb recall query "SELECT invocation_id, invocation_started_at, status, duration_ms From 3b998758b0d6ff237217be7e00b2a2115e5064eb Mon Sep 17 00:00:00 2001 From: Kevin Longe <273747162+kvlonge@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:39:44 +0100 Subject: [PATCH 6/6] docs: recommend only a Replicated database for Recall on self-managed clusters --- .../sqlbuild/references/docs/concepts/observability/recall.md | 2 +- website/src/content/docs/docs/concepts/observability/recall.mdx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md index 9754805bd6..b16583b2b9 100644 --- a/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md +++ b/src/sqlbuild/.agents/skills/sqlbuild/references/docs/concepts/observability/recall.md @@ -304,7 +304,7 @@ again. MergeTree tables and the views above. An existing database is used as it is, whatever its engine. The database is created on the server the writer connects to, never `ON CLUSTER`: on a self-managed cluster whose default database engine is not `Replicated`, create the database - yourself first (`ENGINE = Replicated(...)` or `ON CLUSTER`), and Recall leaves it alone. + yourself first with `ENGINE = Replicated(...)`, and Recall leaves it alone. ClickHouse Cloud needs nothing. If the user may not create the store, the command still succeeds and prints the statements, starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from diff --git a/website/src/content/docs/docs/concepts/observability/recall.mdx b/website/src/content/docs/docs/concepts/observability/recall.mdx index 3d9ee8095a..7511bbf4c2 100644 --- a/website/src/content/docs/docs/concepts/observability/recall.mdx +++ b/website/src/content/docs/docs/concepts/observability/recall.mdx @@ -287,7 +287,7 @@ again. MergeTree tables and the views above. An existing database is used as it is, whatever its engine. The database is created on the server the writer connects to, never `ON CLUSTER`: on a self-managed cluster whose default database engine is not `Replicated`, create the database - yourself first (`ENGINE = Replicated(...)` or `ON CLUSTER`), and Recall leaves it alone. + yourself first with `ENGINE = Replicated(...)`, and Recall leaves it alone. ClickHouse Cloud needs nothing. If the user may not create the store, the command still succeeds and prints the statements, starting with `CREATE DATABASE IF NOT EXISTS` when the database is missing; a store created from