Skip to content

Conversation

@mathix420
Copy link

pretty useful when connecting the ledger to an external source

@pgr0ss
Copy link
Owner

pgr0ss commented Jun 27, 2025

I love the idea of adding optional idempotency, thanks! I don't really want to mess with the transfer_id, though. I prefer these stay pgledger generated to keep them in a consistent format and monotonic. I am planning to add a metadata column soon. Maybe idempotency key can go in there as well? We could make that a special field in the metadata json and index it.

@aabbdev
Copy link

aabbdev commented Jul 7, 2025

I don't think we should include an idempotency key in the transfers table.
Instead, we could use a combination of:

  • An optional ULID/UUID (transfer_id), either auto-generated or client-provided
  • An external table like pgledger_idempotency_keys with a 24h expiration policy

That said, I believe idempotency keys should be handled at the application level, not within the library.

@mathix420
Copy link
Author

  1. Could work but idempotency keys are not always ULIDs or UUIDs therefore not time sortable. And we will lose consistency as @pgr0ss mentioned.
  2. Will not work in many usecases, ex: bridging an external ledger that may only be connected from time to time.

I think it might still be useful to have idempotency especially considering the little to no side effects it implies. You didn't really provide any arguments on why it should stay in the application logic, can you elaborate your pov?

@aabbdev
Copy link

aabbdev commented Jul 7, 2025

  1. The id should have a fixed size to ensure consistent and predictable performance. If you need to include a transfer as part of a larger workflow, you can always use a two-phase commit strategy.
  2. Upserting on the idempotency_key column locks rows, which can introduce contention and slow down the system under load.
  3. Idempotency keys are arbitrary text, which means they can be large and unstructured. Since they should only be stored temporarily (e.g. 24 hours), it’s best to avoid polluting the core ledger table. Idempotency handling is also orthogonal to the domain of the ledger API and should be managed separately.

@pgr0ss
Copy link
Owner

pgr0ss commented Jul 8, 2025

I think we can still have idempotency in the database, but do it outside pgledger core. Here's a quick example, not fully developed or tested:

Create an idempotency table:

CREATE TABLE idempotency_keys (
    key TEXT PRIMARY KEY,
    transfer_id TEXT NOT NULL REFERENCES pgledger_transfers (id),
    created_at TIMESTAMPTZ NOT NULL
);

Now, you can insert and check for conflicts, and only create transfers if the idempotency key doesn't exist yet. This could be wrapped up in a nice function or a big insert statement with CTEs (common table expressions):

INSERT INTO idempotency_keys (key, transfer_id, created_at)
SELECT 'key', id, now()
FROM pgledger_create_transfer(:'acct1_id', :'acct2_id', 12.34)
ON CONFLICT (key) DO UPDATE
  SET transfer_id = idempotency_keys.transfer_id -- no op update so RETURNING works
RETURNING *, (xmax = 0) AS inserted;

SELECT t.*
FROM idempotency_keys i
JOIN pgledger_transfers_view t ON i.transfer_id = t.id
WHERE key = 'key';

Here's how it looks the first time for a new idempotency key:

-[ RECORD 1 ]--------------------------------
key         | key
transfer_id | pglt_01JZKTW8FYEH3BSJXMFVD4RZ2K
created_at  | 2025-07-08 01:37:18.330647+00
inserted    | t

-[ RECORD 1 ]---+--------------------------------
id              | pglt_01JZKTW8FYEH3BSJXMFVD4RZ2K
from_account_id | pgla_01JZKTVR64FWNRRRACD4HHAT21
to_account_id   | pgla_01JZKTVSFHFCQV5EQE10TB17MZ
amount          | 12.34
created_at      | 2025-07-08 01:37:18.330647+00
event_at        | 2025-07-08 01:37:18.330647+00

And the second time:

-[ RECORD 1 ]--------------------------------
key         | key
transfer_id | pglt_01JZKTW8FYEH3BSJXMFVD4RZ2K
created_at  | 2025-07-08 01:37:18.330647+00
inserted    | f

-[ RECORD 1 ]---+--------------------------------
id              | pglt_01JZKTW8FYEH3BSJXMFVD4RZ2K
from_account_id | pgla_01JZKTVR64FWNRRRACD4HHAT21
to_account_id   | pgla_01JZKTVSFHFCQV5EQE10TB17MZ
amount          | 12.34
created_at      | 2025-07-08 01:37:18.330647+00
event_at        | 2025-07-08 01:37:18.330647+00

You'd also probably want something to periodically clear out the old idempotency_keys.

@mathix420
Copy link
Author

Yeah it make sense to do it separately and then get rid of the idempotency key in most use-cases. I'm doing something similar to an open-banking connector so in my case the idempotency key is never expiring and correspond to the transaction id of the original source. But I get that it's a very specific use-case and it's probably closer to being a metadata of the transaction rather than idempotency in the classical way.

@pgr0ss
Copy link
Owner

pgr0ss commented Jul 8, 2025

Yeah it make sense to do it separately and then get rid of the idempotency key in most use-cases. I'm doing something similar to an open-banking connector so in my case the idempotency key is never expiring and correspond to the transaction id of the original source. But I get that it's a very specific use-case and it's probably closer to being a metadata of the transaction rather than idempotency in the classical way.

In that case, I think it might make sense to use the upcoming metadata field on the transfer, and then add a unique index for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants