Skip to content

Commit

Permalink
Make set_transaction_id use the PK index for its lookup
Browse files Browse the repository at this point in the history
This patch fixes a longstanding (yet undiscovered) bug in the `set_transaction_id`
procedure that caused it to not use the provided index on `id` spanning `xact_id`.
Reason was the use of `CURRVAL` in the `WHERE` clause which caused the query to
evaluate every row against a new function call, turning the should-be-fast lookup
into a seq scan of the `transactions` table. Consequently, `INSERT`s scaled linearly
with the size of the `transactions` table, instead of logarithmically.

Huge 🤦
  • Loading branch information
maltoe committed Jun 27, 2024
1 parent b88aa11 commit fd29476
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 29 deletions.
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
## Unreleased

**New migration patches:** 9

### Fixed

- Fix `set_transaction_id` trigger not using the PK index on `transactions` due to being dependent on `CURRVAL` for every row. Replaced with a CTE the `EXIST` query is now an index-only scan as it was intended to be. As a consequence, scalability of inserts into the `transactions` table has been greatly improved.

## [0.12.1] - 2024-05-21

### Fixed
Expand Down
2 changes: 1 addition & 1 deletion lib/carbonite/migrations.ex
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ defmodule Carbonite.Migrations do
# --------------------------------- patch levels ---------------------------------

@initial_patch 1
@current_patch 8
@current_patch 9

@doc false
@spec initial_patch :: non_neg_integer()
Expand Down
63 changes: 35 additions & 28 deletions lib/carbonite/migrations/v4.ex
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,40 @@ defmodule Carbonite.Migrations.V4 do
:ok
end

@spec create_set_transaction_id_procedure(prefix()) :: :ok
def create_set_transaction_id_procedure(prefix) do
"""
CREATE OR REPLACE FUNCTION #{prefix}.set_transaction_id() RETURNS TRIGGER AS
$body$
BEGIN
BEGIN
/* verify that no previous INSERT within current transaction (with same id) */
IF
EXISTS(
SELECT 1 FROM #{prefix}.transactions
WHERE id = COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq'))
AND xact_id = COALESCE(NEW.xact_id, pg_current_xact_id())
)
THEN
NEW.id = COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq'));
END IF;
EXCEPTION WHEN object_not_in_prerequisite_state THEN
/* when NEXTVAL has never been called within session, we're good */
END;
NEW.id = COALESCE(NEW.id, NEXTVAL('#{prefix}.transactions_id_seq'));
NEW.xact_id = COALESCE(NEW.xact_id, pg_current_xact_id());
RETURN NEW;
END
$body$
LANGUAGE plpgsql;
"""
|> squish_and_execute()

:ok
end

@impl true
@spec up([up_option()]) :: :ok
def up(opts) do
Expand Down Expand Up @@ -161,34 +195,7 @@ defmodule Carbonite.Migrations.V4 do
"""
|> squish_and_execute()

"""
CREATE OR REPLACE FUNCTION #{prefix}.set_transaction_id() RETURNS TRIGGER AS
$body$
BEGIN
BEGIN
/* verify that no previous INSERT within current transaction (with same id) */
IF
EXISTS(
SELECT 1 FROM #{prefix}.transactions
WHERE id = COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq'))
AND xact_id = COALESCE(NEW.xact_id, pg_current_xact_id())
)
THEN
NEW.id = COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq'));
END IF;
EXCEPTION WHEN object_not_in_prerequisite_state THEN
/* when NEXTVAL has never been called within session, we're good */
END;
NEW.id = COALESCE(NEW.id, NEXTVAL('#{prefix}.transactions_id_seq'));
NEW.xact_id = COALESCE(NEW.xact_id, pg_current_xact_id());
RETURN NEW;
END
$body$
LANGUAGE plpgsql;
"""
|> squish_and_execute()
create_set_transaction_id_procedure(prefix)

# ------------- override_xact_id -------------

Expand Down
75 changes: 75 additions & 0 deletions lib/carbonite/migrations/v9.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# SPDX-License-Identifier: Apache-2.0

defmodule Carbonite.Migrations.V9 do
@moduledoc false

use Ecto.Migration
use Carbonite.Migrations.Version
alias Carbonite.Migrations.V4

@type prefix :: binary()

@spec create_set_transaction_id_procedure(prefix()) :: :ok
def create_set_transaction_id_procedure(prefix) do
"""
CREATE OR REPLACE FUNCTION #{prefix}.set_transaction_id() RETURNS TRIGGER AS
$body$
BEGIN
BEGIN
/* verify that no previous INSERT within current transaction (with same id) */
IF
EXISTS(
WITH constants AS (
SELECT
COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq')) AS id,
COALESCE(NEW.xact_id, pg_current_xact_id()) AS xact_id
)
SELECT 1 FROM #{prefix}.transactions
JOIN constants
ON constants.id = transactions.id
AND constants.xact_id = transactions.xact_id
)
THEN
NEW.id = COALESCE(NEW.id, CURRVAL('#{prefix}.transactions_id_seq'));
END IF;
EXCEPTION WHEN object_not_in_prerequisite_state THEN
/* when NEXTVAL has never been called within session, we're good */
END;
NEW.id = COALESCE(NEW.id, NEXTVAL('#{prefix}.transactions_id_seq'));
NEW.xact_id = COALESCE(NEW.xact_id, pg_current_xact_id());
RETURN NEW;
END
$body$
LANGUAGE plpgsql;
"""
|> squish_and_execute()

:ok
end

@type up_option :: {:carbonite_prefix, prefix()}

@impl true
@spec up([up_option()]) :: :ok
def up(opts) do
prefix = Keyword.get(opts, :carbonite_prefix, default_prefix())

create_set_transaction_id_procedure(prefix)

:ok
end

@type down_option :: {:carbonite_prefix, prefix()}

@impl true
@spec down([down_option()]) :: :ok
def down(opts) do
prefix = Keyword.get(opts, :carbonite_prefix, default_prefix())

V4.create_set_transaction_id_procedure(prefix)

:ok
end
end

0 comments on commit fd29476

Please sign in to comment.