From 1071479e66208327aa43242d56dfec8f598f2dbb Mon Sep 17 00:00:00 2001 From: Chase Granberry Date: Fri, 11 Sep 2026 17:00:01 +0000 Subject: [PATCH 1/2] The DuckLake catalog answers a call that exits as an error, so retention, GC and the compactor keep their state (T-464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every Catalog callback already promised {:error, term()}, but every statement the DuckLake implementation ran was a bare GenServer.call: a read that timed out behind a busy connection, or found the connection gone, exited its caller. The compactor had grown its own catch for that (T-460); Retention.run/1 and GC.run/1 still crashed, and GC's crash reset its grace-period candidates, so a catalog engine timing out each sweep meant GC never deleted anything. Engine.try_transaction/3 joins try_query/4, and DuckLake's query/4 and its five transaction sites go through the try layer, so an exit comes back as {:error, %CallExited{}} to every caller. The compactor's listing and read wrappers go; its swap tags the error {:swap_failed, ...} so the log names the phase, and one catch stays for an exit the catalog never sees — a store put whose HTTP pool died mid-upload. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_014K63Kxq4pf7eD9K1416Eju --- lib/smolquery/catalog/ducklake.ex | 25 ++++++++++--- lib/smolquery/engine.ex | 15 ++++++++ lib/smolquery/storage_service/compactor.ex | 37 ++++++++++++------- test/smolquery/catalog/ducklake_test.exs | 12 ++++++ test/smolquery/engine_test.exs | 22 +++++++++++ .../storage_service/compactor_test.exs | 3 +- .../storage_service/retention_test.exs | 12 ++++++ 7 files changed, 104 insertions(+), 22 deletions(-) diff --git a/lib/smolquery/catalog/ducklake.ex b/lib/smolquery/catalog/ducklake.ex index d3d87699..5840213e 100644 --- a/lib/smolquery/catalog/ducklake.ex +++ b/lib/smolquery/catalog/ducklake.ex @@ -98,6 +98,19 @@ defmodule Smolquery.Catalog.DuckLake do way: a relative path returned as-is would match no store location, and GC would again see every committed segment as unreferenced. + ## A call that exits is an error here, never a crash upstream + + Every statement this module runs goes through `Smolquery.Engine.try_query/4` + or `Smolquery.Engine.try_transaction/3`, so a call that times out on a busy + connection, or finds the connection gone, comes back as + `{:error, %Smolquery.Engine.CallExited{}}` like any other failure (T-464). + Every `Smolquery.Catalog` callback already promises `{:error, term()}`, and + the callers that matter are sweeps: the compactor, retention and GC each + visit every table in one long-lived process, and an exit from one table's + read used to take the whole sweep down and, for GC, its grace-period + bookkeeping with it. The abandoned statement keeps running on the + connection either way; what changes is that the caller keeps its state. + `ducklake_merge_adjacent_files/2` must never be called on a smolquery table: over externally-registered files it crashes DuckDB fatally (ducklake `67480b1d`, format 0.4), and a fatal error invalidates the whole database. @@ -426,7 +439,7 @@ defmodule Smolquery.Catalog.DuckLake do "INSERT INTO #{materialized_table(config.catalog)} VALUES (#{Enum.join(values, ", ")})" end) - Engine.transaction(config.engine, statements) + Engine.try_transaction(config.engine, statements) end @impl Catalog @@ -676,7 +689,7 @@ defmodule Smolquery.Catalog.DuckLake do defp swap(config, ref, add, drop) do with {:ok, name} <- table_name(config, ref), :ok <- - Engine.transaction( + Engine.try_transaction( config.engine, [delete_statement(name, drop), add_statement(config, ref, add)], config.swap_timeout_ms @@ -815,7 +828,7 @@ defmodule Smolquery.Catalog.DuckLake do :ok <- maybe_ensure_retention_table(config, options) do case option_statements(config, dataset, table, options) do [] -> :ok - statements -> Engine.transaction(config.engine, statements) + statements -> Engine.try_transaction(config.engine, statements) end end end @@ -970,7 +983,7 @@ defmodule Smolquery.Catalog.DuckLake do defp transact(config, statements) do with_commit_retries(fn -> - case Engine.transaction(config.engine, statements) do + case Engine.try_transaction(config.engine, statements) do :ok -> {:ok, :committed} {:error, _error} = failure -> failure end @@ -1123,7 +1136,7 @@ defmodule Smolquery.Catalog.DuckLake do now = System.system_time(:millisecond) created_at = connection.created_at || now - Engine.transaction(config.engine, [ + Engine.try_transaction(config.engine, [ delete_connection_sql(config, connection.name), "INSERT INTO #{connections_table(config.catalog)} " <> "(name, host, port, database_name, username, secret, sslmode, created_at, updated_at) " <> @@ -1353,7 +1366,7 @@ defmodule Smolquery.Catalog.DuckLake do end defp query(config, sql, params \\ [], timeout \\ 30_000), - do: Engine.query(config.engine, sql, params, timeout) + do: Engine.try_query(config.engine, sql, params, timeout) defp engine_extensions do :smolquery diff --git a/lib/smolquery/engine.ex b/lib/smolquery/engine.ex index d780d705..c2c9dc53 100644 --- a/lib/smolquery/engine.ex +++ b/lib/smolquery/engine.ex @@ -239,6 +239,21 @@ defmodule Smolquery.Engine do :exit, reason -> {:error, CallExited.new(reason)} end + @doc """ + Same as `transaction/3`, but an exit from the call comes back as an error — + `try_query/4`'s contract for a transaction (T-464). + + A transaction whose call exits is still running on the connection, and + commits or rolls back on its own time; the caller learns only that it did + not hear the answer. + """ + @spec try_transaction(handle(), [String.t()], timeout()) :: :ok | {:error, Exception.t()} + def try_transaction(handle, statements, timeout \\ 30_000) do + transaction(handle, statements, timeout) + catch + :exit, reason -> {:error, CallExited.new(reason)} + end + @doc """ Same as `query/3` but raises on error. """ diff --git a/lib/smolquery/storage_service/compactor.ex b/lib/smolquery/storage_service/compactor.ex index 2260562a..14a325e2 100644 --- a/lib/smolquery/storage_service/compactor.ex +++ b/lib/smolquery/storage_service/compactor.ex @@ -183,13 +183,16 @@ defmodule Smolquery.StorageService.Compactor do the whole compactor down with it. A crash forgets the backoff, the row caps and the quarantine, so the same group was re-planned and re-merged next sweep and timed out again, forty-five times in ten hours, while each - abandoned transaction kept running on the catalog connection (T-460). Here - an exit is caught where it happens: the swap's transaction becomes - `{:swap_failed, %CallExited{}}`, any other exit during a table's - compaction — a catalog read, or a store put whose HTTP pool died — - `{:call_exited, %CallExited{}}`, and a listing that exits fails the sweep. - All three back the table off like any other failure and none recycles the - compaction engine, whose statement did not exit. + abandoned transaction kept running on the catalog connection (T-460). The + catalog now answers such an exit as `{:error, %CallExited{}}` itself + (`Smolquery.Catalog.DuckLake`, T-464), so a listing that exits fails the + sweep with that error, a read that exits fails its table with it, and the + swap's transaction is tagged `{:swap_failed, %CallExited{}}` so the log + says which phase. One catch remains here, for an exit the catalog never + sees: a store put whose HTTP pool died mid-upload becomes + `{:call_exited, %CallExited{}}`. Every one of these backs the table off + like any other failure and none recycles the compaction engine, whose + statement did not exit. The sweep also stops at the first such exit. The call that exited is still running on the catalog's compaction connection, which serializes its @@ -311,7 +314,7 @@ defmodule Smolquery.StorageService.Compactor do defp run(state) do runtime = state.runtime - with {:ok, tables} <- catalog_call(:listing_failed, fn -> Catalog.tables(runtime.catalog) end) do + with {:ok, tables} <- Catalog.tables(runtime.catalog) do {cooling, due} = Enum.split_with(tables, &cooling_down?(state.cooldowns, &1)) {outcomes, deferred} = sweep_due(runtime, state, due) swept = due -- deferred @@ -758,6 +761,8 @@ defmodule Smolquery.StorageService.Compactor do end end + defp call_exited?({:failed, %{reason: %CallExited{}}}), do: true + defp call_exited?({:failed, %{reason: {step, %CallExited{}}}}) when step in [:swap_failed, :call_exited], do: true @@ -767,7 +772,7 @@ defmodule Smolquery.StorageService.Compactor do defp compact_table(runtime, quarantined_groups, table_ref) do started_at = System.monotonic_time(:microsecond) - case catalog_call(:call_exited, fn -> + case exit_safe(:call_exited, fn -> compact_listed(runtime, quarantined_groups, table_ref, started_at) end) do {:error, reason} -> failed(runtime, table_ref, reason, started_at) @@ -799,7 +804,7 @@ defmodule Smolquery.StorageService.Compactor do end end - defp catalog_call(step, call) do + defp exit_safe(step, call) do call.() catch :exit, reason -> {:error, {step, CallExited.new(reason)}} @@ -1041,15 +1046,19 @@ defmodule Smolquery.StorageService.Compactor do end defp swapped(runtime, table_ref, segment, paths) do - with {:ok, snapshot} <- - catalog_call(:swap_failed, fn -> - Catalog.replace_segments(runtime.catalog, table_ref, [segment], paths) - end), + with {:ok, snapshot} <- replaced(runtime, table_ref, segment, paths), :ok <- verify_retired(runtime, table_ref, paths) do {:ok, snapshot} end end + defp replaced(runtime, table_ref, segment, paths) do + case Catalog.replace_segments(runtime.catalog, table_ref, [segment], paths) do + {:error, %CallExited{} = exited} -> {:error, {:swap_failed, exited}} + other -> other + end + end + defp verify_retired(runtime, table_ref, dropped) do with {:ok, current} <- Catalog.segments(runtime.catalog, table_ref, :current) do listed = MapSet.new(current) diff --git a/test/smolquery/catalog/ducklake_test.exs b/test/smolquery/catalog/ducklake_test.exs index 1b14efde..0deef68e 100644 --- a/test/smolquery/catalog/ducklake_test.exs +++ b/test/smolquery/catalog/ducklake_test.exs @@ -544,6 +544,18 @@ defmodule Smolquery.Catalog.DuckLakeTest do end end + describe "a call that exits (T-464)" do + test "comes back as {:error, %CallExited{}} from a read and from a commit" do + catalog = DuckLake.new(engine: @engine) + Process.unregister(Engine.connection_name(@engine)) + + assert Catalog.tables(catalog) == {:error, %Smolquery.Engine.CallExited{reason: :noproc}} + + assert Catalog.drop_segments(catalog, @table, ["/nowhere.parquet"]) == + {:error, %Smolquery.Engine.CallExited{reason: :noproc}} + end + end + describe "replace_segments/4" do test "one snapshot both adds the replacement and retires the inputs", %{ catalog: catalog, diff --git a/test/smolquery/engine_test.exs b/test/smolquery/engine_test.exs index d25f2a32..6e32f9e4 100644 --- a/test/smolquery/engine_test.exs +++ b/test/smolquery/engine_test.exs @@ -189,6 +189,28 @@ defmodule Smolquery.EngineTest do end end + describe "try_transaction/3" do + test "commits like transaction/3 and returns its errors" do + assert Engine.try_transaction(@engine, ["CREATE TABLE try_txn (n INTEGER)"]) == :ok + + assert {:error, %Adbc.Error{}} = + Engine.try_transaction(@engine, ["SELECT * FROM no_such_table"]) + end + + test "returns CallExited instead of exiting when the connection is down or busy (T-464)" do + assert {:error, %CallExited{reason: :noproc}} = + Engine.try_transaction(__MODULE__.Missing, ["SELECT 1"]) + + busy = Process.whereis(Engine.connection_name(@engine)) + :ok = :sys.suspend(busy) + + assert {:error, %CallExited{reason: :timeout}} = + Engine.try_transaction(@engine, ["SELECT 1"], 50) + + :ok = :sys.resume(busy) + end + end + describe "transaction/2" do test "commits every statement together" do assert Engine.transaction(@engine, [ diff --git a/test/smolquery/storage_service/compactor_test.exs b/test/smolquery/storage_service/compactor_test.exs index 37db7474..3a25a194 100644 --- a/test/smolquery/storage_service/compactor_test.exs +++ b/test/smolquery/storage_service/compactor_test.exs @@ -469,8 +469,7 @@ defmodule Smolquery.StorageService.CompactorTest do compactor = Process.whereis(Runtime.compactor(context.storage)) Process.unregister(Engine.connection_name(Runtime.catalog_engine(context.storage), 2)) - assert Compactor.sweep(context.storage) == - {:error, {:listing_failed, %CallExited{reason: :noproc}}} + assert Compactor.sweep(context.storage) == {:error, %CallExited{reason: :noproc}} assert Process.alive?(compactor) end diff --git a/test/smolquery/storage_service/retention_test.exs b/test/smolquery/storage_service/retention_test.exs index 36d345dc..bc21e108 100644 --- a/test/smolquery/storage_service/retention_test.exs +++ b/test/smolquery/storage_service/retention_test.exs @@ -13,6 +13,7 @@ defmodule Smolquery.StorageService.RetentionTest do alias Smolquery.Catalog alias Smolquery.Catalog.DuckLake alias Smolquery.Engine + alias Smolquery.Engine.CallExited alias Smolquery.Engine.Result alias Smolquery.Schema alias Smolquery.Segments.Id @@ -145,6 +146,17 @@ defmodule Smolquery.StorageService.RetentionTest do assert File.exists?(aged.path) end + test "a catalog call that exits fails the sweep instead of crashing the sweeper (T-464)", + context do + runtime = start_retention(context, []) + seal(runtime, context.catalog, 1, [~N[2026-01-01 00:00:00]]) + retention = Process.whereis(Runtime.retention(context.storage)) + Process.unregister(Engine.connection_name(Runtime.catalog_engine(context.storage))) + + assert Retention.sweep(context.storage) == {:error, %CallExited{reason: :noproc}} + assert Process.alive?(retention) + end + test "a sweep with nothing expired drops nothing and reports it", context do runtime = start_retention(context, []) seal(runtime, context.catalog, 1, [days_ago(0)]) From 4fada2c7f4197f40f5ce5bb4a074bae2ad660ecb Mon Sep 17 00:00:00 2001 From: Chase Granberry Date: Fri, 11 Sep 2026 17:50:00 +0000 Subject: [PATCH 2/2] Review of T-464: a wedged catalog is an error to pg-wire clients, retention stops at the first exit, the swap's exit keeps its bare shape, and the engine's two try functions share one catch With the catalog answering exits as errors, PgCatalog.listed_tables/1 collapsed any error to [] and rebuilt pg_namespace and pg_class empty, so a psql client asking \dt during a wedge saw a database with no tables and the empty build was cached for the TTL. A refresh that cannot read the catalog now answers {:error, {:pg_catalog_unavailable, _}} and leaves the generated tables and the timestamp as they were, which is what main's crash-and-restart gave the client. Retention.run/1 mapped every table and turned each exit into a per-table failure, so a wedged connection cost one timeout per table instead of one; it now stops at the first exit and reports the rest as deferred, like the compactor. The compactor's replaced/4 existed only to re-tag the swap's exit as :swap_failed, which nothing branched on; the bare struct is the one shape. Engine.try_query/4 and try_transaction/3 share catching_exit/1. Smolquery.Test.ExitingCatalog wraps a real catalog and answers chosen calls with %CallExited{}, since a real wedge cannot be staged deterministically. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_014K63Kxq4pf7eD9K1416Eju --- lib/smolquery/engine.ex | 10 ++-- lib/smolquery/storage_service/compactor.ex | 28 +++------ lib/smolquery/storage_service/retention.ex | 29 +++++++++- lib/smolquery_pg/pg_catalog.ex | 47 ++++++++++----- .../storage_service/compactor_test.exs | 4 +- .../storage_service/retention_test.exs | 21 ++++++- test/smolquery_pg/catalog_wire_test.exs | 30 ++++++++++ test/support/exiting_catalog.ex | 57 +++++++++++++++++++ 8 files changed, 183 insertions(+), 43 deletions(-) create mode 100644 test/support/exiting_catalog.ex diff --git a/lib/smolquery/engine.ex b/lib/smolquery/engine.ex index c2c9dc53..d6e87880 100644 --- a/lib/smolquery/engine.ex +++ b/lib/smolquery/engine.ex @@ -234,9 +234,7 @@ defmodule Smolquery.Engine do @spec try_query(handle(), String.t(), [term()], timeout()) :: {:ok, Result.t()} | {:error, Exception.t()} def try_query(handle, sql, params \\ [], timeout \\ 30_000) do - query(handle, sql, params, timeout) - catch - :exit, reason -> {:error, CallExited.new(reason)} + catching_exit(fn -> query(handle, sql, params, timeout) end) end @doc """ @@ -249,7 +247,11 @@ defmodule Smolquery.Engine do """ @spec try_transaction(handle(), [String.t()], timeout()) :: :ok | {:error, Exception.t()} def try_transaction(handle, statements, timeout \\ 30_000) do - transaction(handle, statements, timeout) + catching_exit(fn -> transaction(handle, statements, timeout) end) + end + + defp catching_exit(call) do + call.() catch :exit, reason -> {:error, CallExited.new(reason)} end diff --git a/lib/smolquery/storage_service/compactor.ex b/lib/smolquery/storage_service/compactor.ex index 14a325e2..b18fbd7a 100644 --- a/lib/smolquery/storage_service/compactor.ex +++ b/lib/smolquery/storage_service/compactor.ex @@ -186,13 +186,12 @@ defmodule Smolquery.StorageService.Compactor do abandoned transaction kept running on the catalog connection (T-460). The catalog now answers such an exit as `{:error, %CallExited{}}` itself (`Smolquery.Catalog.DuckLake`, T-464), so a listing that exits fails the - sweep with that error, a read that exits fails its table with it, and the - swap's transaction is tagged `{:swap_failed, %CallExited{}}` so the log - says which phase. One catch remains here, for an exit the catalog never - sees: a store put whose HTTP pool died mid-upload becomes - `{:call_exited, %CallExited{}}`. Every one of these backs the table off - like any other failure and none recycles the compaction engine, whose - statement did not exit. + sweep with that error and a read or the swap that exits fails its table + with it. One catch remains here, for an exit the catalog never sees: a + store put whose HTTP pool died mid-upload becomes + `{:call_exited, %CallExited{}}`. Both back the table off like any other + failure and neither recycles the compaction engine, whose statement did + not exit. The sweep also stops at the first such exit. The call that exited is still running on the catalog's compaction connection, which serializes its @@ -762,11 +761,7 @@ defmodule Smolquery.StorageService.Compactor do end defp call_exited?({:failed, %{reason: %CallExited{}}}), do: true - - defp call_exited?({:failed, %{reason: {step, %CallExited{}}}}) - when step in [:swap_failed, :call_exited], - do: true - + defp call_exited?({:failed, %{reason: {:call_exited, %CallExited{}}}}), do: true defp call_exited?(_outcome), do: false defp compact_table(runtime, quarantined_groups, table_ref) do @@ -1046,19 +1041,12 @@ defmodule Smolquery.StorageService.Compactor do end defp swapped(runtime, table_ref, segment, paths) do - with {:ok, snapshot} <- replaced(runtime, table_ref, segment, paths), + with {:ok, snapshot} <- Catalog.replace_segments(runtime.catalog, table_ref, [segment], paths), :ok <- verify_retired(runtime, table_ref, paths) do {:ok, snapshot} end end - defp replaced(runtime, table_ref, segment, paths) do - case Catalog.replace_segments(runtime.catalog, table_ref, [segment], paths) do - {:error, %CallExited{} = exited} -> {:error, {:swap_failed, exited}} - other -> other - end - end - defp verify_retired(runtime, table_ref, dropped) do with {:ok, current} <- Catalog.segments(runtime.catalog, table_ref, :current) do listed = MapSet.new(current) diff --git a/lib/smolquery/storage_service/retention.ex b/lib/smolquery/storage_service/retention.ex index 9f063535..d9b01a38 100644 --- a/lib/smolquery/storage_service/retention.ex +++ b/lib/smolquery/storage_service/retention.ex @@ -51,6 +51,7 @@ defmodule Smolquery.StorageService.Retention do alias Smolquery.Catalog alias Smolquery.Engine + alias Smolquery.Engine.CallExited alias Smolquery.StorageService.Routing alias Smolquery.StorageService.Runtime @@ -72,19 +73,23 @@ defmodule Smolquery.StorageService.Retention do @doc """ Sweeps now, without waiting for the interval. - Reports what was dropped per table, how many snapshots expired, and what - failed. + Reports what was dropped per table, how many snapshots expired, what + failed, and the tables the sweep left untouched behind a catalog call that + exited (`deferred`, T-464): the call that exited is still running on the + catalog connection, so every later table would queue behind it and fail + for a reason not its own. """ @spec sweep(atom(), timeout()) :: {:ok, map()} | {:error, term()} def sweep(name, timeout \\ 60_000), do: GenServer.call(Runtime.retention(name), :sweep, timeout) defp run(state) do with {:ok, tables} <- Catalog.tables(state.runtime.catalog) do - outcomes = Enum.map(tables, &sweep_table(state.runtime, &1)) + {outcomes, deferred} = sweep_tables(state.runtime, tables) report = %{ dropped: for({:ok, drop} <- outcomes, do: drop), failed: for({:failed, failure} <- outcomes, do: failure), + deferred: deferred, expired_snapshots: expire(state.runtime) } @@ -101,6 +106,24 @@ defmodule Smolquery.StorageService.Retention do end end + defp sweep_tables(_runtime, []), do: {[], []} + + defp sweep_tables(runtime, [table_ref | rest]) do + case sweep_table(runtime, table_ref) do + {:failed, %{reason: %CallExited{}}} = outcome -> + Logger.warning(fn -> + "retention sweep stopped after a catalog call exited on #{inspect(table_ref)}: " <> + "#{length(rest)} table(s) deferred to the next sweep" + end) + + {[outcome], rest} + + outcome -> + {outcomes, deferred} = sweep_tables(runtime, rest) + {[outcome | outcomes], deferred} + end + end + defp sweep_table(runtime, table_ref) do with true <- runtime.name |> Routing.resolve() |> Routing.own?(table_ref), {:ok, %{column: column, ttl_ms: ttl_ms}} <- diff --git a/lib/smolquery_pg/pg_catalog.ex b/lib/smolquery_pg/pg_catalog.ex index 54a9ccbd..dfc531a3 100644 --- a/lib/smolquery_pg/pg_catalog.ex +++ b/lib/smolquery_pg/pg_catalog.ex @@ -50,6 +50,13 @@ defmodule SmolqueryPg.PgCatalog do every call runs through this server, so a refresh never interleaves with a read. + A rebuild that cannot read the catalog — a call that timed out behind a + busy connection, or found it gone, answers `{:error, %CallExited{}}` + since T-464 — is not a rebuild: the query answers + `{:error, {:pg_catalog_unavailable, _}}`, the generated tables keep their + last shape, and the next query tries again. An empty `pg_class` would be + a wrong answer a client acts on, not a failure it can retry. + All calls take the edge's instance name; the server and its engine derive from it (`SmolqueryPg.Runtime.pg_catalog/1`). """ @@ -59,6 +66,7 @@ defmodule SmolqueryPg.PgCatalog do alias Explorer.DataFrame alias Smolquery.Catalog alias Smolquery.Engine + alias Smolquery.Engine.CallExited alias Smolquery.Engine.Frame alias Smolquery.Identifier alias SmolqueryPg.PgCatalog.Rewrite @@ -174,9 +182,8 @@ defmodule SmolqueryPg.PgCatalog do end def handle_call({:query, sql, settings, params}, _from, state) do - state = ensure_fresh(state) - - with {:ok, _ast, canonical} <- serialize(state.engine, Rewrite.pre(sql, settings)), + with {:ok, state} <- ensure_fresh(state), + {:ok, _ast, canonical} <- serialize(state.engine, Rewrite.pre(sql, settings)), {:ok, frame} <- run(state.engine, Rewrite.post(canonical), params) do {:reply, {:ok, columns(frame), rows(frame)}, state} else @@ -289,11 +296,12 @@ defmodule SmolqueryPg.PgCatalog do now = System.monotonic_time(:millisecond) if now - state.refreshed_at > @refresh_ttl_ms or state.refreshed_at == 0 do - refresh(state.engine, state.runtime.catalog) - - %{state | refreshed_at: now} + case refresh(state.engine, state.runtime.catalog) do + :ok -> {:ok, %{state | refreshed_at: now}} + {:error, reason} -> {:error, {:pg_catalog_unavailable, reason}} + end else - state + {:ok, state} end end @@ -593,7 +601,12 @@ defmodule SmolqueryPg.PgCatalog do defp refresh(_engine, nil), do: :ok defp refresh(engine, catalog) do - tables = listed_tables(catalog) + with {:ok, tables} <- listed_tables(catalog) do + rebuild(engine, tables) + end + end + + defp rebuild(engine, tables) do datasets = tables |> Enum.map(fn {dataset, _table, _schema} -> dataset end) |> Enum.uniq() Engine.transaction(engine, [ @@ -674,16 +687,24 @@ defmodule SmolqueryPg.PgCatalog do end defp listed_tables(catalog) do - case Catalog.tables(catalog) do - {:ok, refs} -> Enum.flat_map(refs, &table_entry(catalog, &1)) - {:error, _reason} -> [] + with {:ok, refs} <- Catalog.tables(catalog) do + Enum.reduce_while(refs, {:ok, []}, &collect_entry(catalog, &1, &2)) + end + end + + defp collect_entry(catalog, ref, {:ok, entries}) do + case table_entry(catalog, ref) do + {:ok, entry} -> {:cont, {:ok, [entry | entries]}} + :skip -> {:cont, {:ok, entries}} + {:error, reason} -> {:halt, {:error, reason}} end end defp table_entry(catalog, {dataset, table} = ref) do case Catalog.table_schema(catalog, ref) do - {:ok, schema} -> [{dataset, table, schema}] - {:error, _reason} -> [] + {:ok, schema} -> {:ok, {dataset, table, schema}} + {:error, %CallExited{} = exited} -> {:error, exited} + {:error, _dropped_meanwhile} -> :skip end end diff --git a/test/smolquery/storage_service/compactor_test.exs b/test/smolquery/storage_service/compactor_test.exs index 3a25a194..131b238e 100644 --- a/test/smolquery/storage_service/compactor_test.exs +++ b/test/smolquery/storage_service/compactor_test.exs @@ -423,7 +423,7 @@ defmodule Smolquery.StorageService.CompactorTest do database = Process.whereis(Engine.database_name(compact_engine)) assert {:ok, %{compacted: [], failed: [failure]}} = Compactor.sweep(context.storage) - assert %{table: @table, reason: {:swap_failed, %CallExited{reason: :timeout}}} = failure + assert %{table: @table, reason: %CallExited{reason: :timeout}} = failure assert Process.alive?(compactor) assert Process.whereis(Engine.database_name(compact_engine)) == database @@ -446,7 +446,7 @@ defmodule Smolquery.StorageService.CompactorTest do assert {:ok, report} = Compactor.sweep(context.storage) - assert [%{table: first, reason: {:swap_failed, %CallExited{reason: :timeout}}}] = + assert [%{table: first, reason: %CallExited{reason: :timeout}}] = report.failed assert [second] = report.deferred diff --git a/test/smolquery/storage_service/retention_test.exs b/test/smolquery/storage_service/retention_test.exs index bc21e108..3cc0e592 100644 --- a/test/smolquery/storage_service/retention_test.exs +++ b/test/smolquery/storage_service/retention_test.exs @@ -20,6 +20,7 @@ defmodule Smolquery.StorageService.RetentionTest do alias Smolquery.Segments.Store alias Smolquery.StorageService.Retention alias Smolquery.StorageService.Runtime + alias Smolquery.Test.ExitingCatalog alias Smolquery.Test.SegmentFixture @moduletag :integration @@ -59,7 +60,8 @@ defmodule Smolquery.StorageService.RetentionTest do catalog: context.catalog, engine_extensions: [], retention_interval_ms: 3_600_000 - ] ++ opts + ] + |> Keyword.merge(opts) ) start_supervised!({Retention, runtime}, id: {:retention, context.storage}) @@ -157,6 +159,23 @@ defmodule Smolquery.StorageService.RetentionTest do assert Process.alive?(retention) end + test "a sweep stops at the first catalog call that exits and defers the tables behind it (T-464)", + context do + other = {"analytics", "clicks"} + :ok = Catalog.create_table(context.catalog, other, schema()) + + wedged = + ExitingCatalog.new(context.catalog, retention: fn [table] -> table == other end) + + runtime = start_retention(context, catalog: wedged) + seal(runtime, context.catalog, 1, [days_ago(10)]) + + assert {:ok, report} = Retention.sweep(context.storage) + assert [%{table: ^other, reason: %CallExited{reason: :timeout}}] = report.failed + assert report.deferred == [@table] + assert report.dropped == [] + end + test "a sweep with nothing expired drops nothing and reports it", context do runtime = start_retention(context, []) seal(runtime, context.catalog, 1, [days_ago(0)]) diff --git a/test/smolquery_pg/catalog_wire_test.exs b/test/smolquery_pg/catalog_wire_test.exs index aefc3f13..6dee1912 100644 --- a/test/smolquery_pg/catalog_wire_test.exs +++ b/test/smolquery_pg/catalog_wire_test.exs @@ -10,6 +10,7 @@ defmodule SmolqueryPg.CatalogWireTest do alias Smolquery.Catalog alias Smolquery.QueryService alias Smolquery.Schema + alias Smolquery.Test.ExitingCatalog alias Smolquery.Test.FixedCatalog alias Smolquery.Test.MapCatalog alias Smolquery.Test.PgClient @@ -61,6 +62,35 @@ defmodule SmolqueryPg.CatalogWireTest do %{socket: socket, catalog: catalog} end + test "a catalog call that exits is an error to the client, not an empty catalog (T-464)", + context do + unique = :erlang.unique_integer([:positive]) + pg = :"pg_catalog_wedged_#{unique}" + wedged = ExitingCatalog.new(context.catalog, [:list_datasets]) + + start_supervised!( + {SmolqueryPg.Supervisor, + name: pg, + auth: :cleartext, + password: @password, + query_name: :"pg_catalog_query_#{unique}", + port: 0, + catalog: wedged}, + id: pg + ) + + on_exit(fn -> Runtime.delete(pg) end) + {:ok, {_ip, port}} = SmolqueryPg.Supervisor.bound(pg) + {:ok, socket, _params} = PgClient.connect(port, password: @password) + + answer = PgClient.query(socket, "SELECT relname FROM pg_catalog.pg_class") + + assert [%{"M" => message}] = answer.errors + assert message =~ "pg_catalog_unavailable" + assert message =~ "CallExited" + assert answer.results == [] + end + test "two result columns of one name both answer: the re-select labels them (T-426)", %{ socket: socket } do diff --git a/test/support/exiting_catalog.ex b/test/support/exiting_catalog.ex new file mode 100644 index 00000000..f66d31c5 --- /dev/null +++ b/test/support/exiting_catalog.ex @@ -0,0 +1,57 @@ +defmodule Smolquery.Test.ExitingCatalog do + @moduledoc """ + A `Smolquery.Catalog` that answers like the catalog it wraps, except that + chosen calls answer `{:error, %Smolquery.Engine.CallExited{}}` — what + `Smolquery.Catalog.DuckLake` answers when a call times out behind a busy + connection or finds the connection gone (T-464). + + A real wedge cannot be staged deterministically: the abandoned statement + that causes one runs on the connection's own time. This double lets a test + say which call, or which call on which table, is the one that exited, and + everything else still reaches the real catalog underneath. + + ExitingCatalog.new(catalog, [:tables]) + ExitingCatalog.new(catalog, retention: fn [table] -> table == other end) + + A bare function name exits on every call; a predicate over the call's + arguments (the config excluded) exits when it answers true. + """ + + @behaviour Smolquery.Catalog + + alias Smolquery.Catalog + alias Smolquery.Engine.CallExited + + @type exiting :: [atom()] | [{atom(), (list() -> boolean())}] + + @spec new(Catalog.t(), exiting()) :: Catalog.t() + def new(%Catalog{} = inner, exiting) when is_list(exiting) do + %Catalog{impl: __MODULE__, config: %{inner: inner, exiting: exiting}} + end + + @impl Catalog + def on_connection(%{inner: inner} = config, slot) do + %{config | inner: %{inner | config: inner.impl.on_connection(inner.config, slot)}} + end + + for {name, arity} <- Catalog.behaviour_info(:callbacks), name != :on_connection do + args = Macro.generate_arguments(arity - 1, __MODULE__) + + @impl Catalog + def unquote(name)(%{inner: inner, exiting: exiting}, unquote_splicing(args)) do + if exits?(exiting, unquote(name), [unquote_splicing(args)]) do + {:error, %CallExited{reason: :timeout}} + else + inner.impl.unquote(name)(inner.config, unquote_splicing(args)) + end + end + end + + defp exits?(exiting, name, args) do + Enum.any?(exiting, fn + ^name -> true + {^name, predicate} when is_function(predicate, 1) -> predicate.(args) + _other -> false + end) + end +end