Skip to content

Remove the per-object advisory lock now that {revision_key} is required in key templates #1492

Open

Description

@KonstantinMirin

Problem

ReportingRowStorageV1 takes a transaction-scoped advisory lock per object in two places: for every adopted object inside the ledger commit (lock_and_verify_adopted), and for every object a sweep or prune deletes (delete_unreferenced). The module's own comment gives the reason: a custom key template that omits an identity placeholder lets two row sets name the same object. #1485 closed that case. Every writable object binding's template must include {revision_key}, validated at configuration and at readiness, so two row sets can no longer name one object. The lock now serializes a conflict that cannot occur, and it adds one pg_advisory_xact_lock round trip per adopted object inside the commit transaction and per deleted object in retention.

@bokelley's reply on adcp#7996 commits to this change ("With unique object keys required everywhere, though, the cross-row-set object lock can go. We'll take that simplification."). This issue tracks it on the Python side.

Evidence

All references are to main at 47084e83a, src/adcp/reporting/ledger/row_storage.py unless stated.

  • Lines 1043-1051, the design comment:

    Chunks are adopted create-only, so a custom key template that omits an identity placeholder lets two row sets name the same (binding, key, version). Every path that deletes an object and every commit that adopts one serialize on a transaction-scoped advisory lock per object ...

  • Lines 1054-1055: _object_lock_text returns f"adcp-reporting-object:{binding_id}:{object_ref}".

  • Lines 1074-1078, in lock_and_verify_adopted:

    for locator in adopted:
        await connection.execute(
            "SELECT pg_advisory_xact_lock(hashtextextended(%s, 0))",
            (self._object_lock_text(binding_id, locator.object_ref),),
        )
  • Lines 1147-1150: the same lock at the top of each per-object transaction in delete_unreferenced.

  • src/adcp/reporting/ledger/row_write.py:350-351: persist_revision_row_set calls lock_and_verify_adopted inside the ledger transaction, after the account lock.

  • src/adcp/reporting/ledger/row_storage_object.py:217-232: assert_reporting_row_key_template_v1(template, *, require_revision_key=True) appends "{revision_key}" to the required placeholders; row_storage.py:292-301 applies it to every writable binding.

  • JS SDK, adcp-client main at 7c7f730f4: src/lib/reporting/ledger/postgres.ts:5471-5474 defines objectLock with the same text, used at :2462 and :2636.

Suggested fix

  • Delete _object_lock_text and the two lock loops. Keep the version-pinned stat verification that feat(reporting)!: align row storage with AdCP stored names and the period date rule #1485 added (rename lock_and_verify_adopted to verify_adopted), and keep the reference check and the provider delete together in the per-object transaction of delete_unreferenced.
  • Confirm two cases before the lock goes, and cover each with a test:
    • Read-only bindings. They are exempt from the {revision_key} rule, so objects written under an older template can still be shared by two row sets. The reference check in delete_unreferenced then has to decide alone when two prunes, or a prune and a sweep, reach the same object.
    • A retry that adopts its own earlier object. upload adopts an existing object (created=False, row_storage.py:836-866) when a retry of the same row set finds its key taken. The sweep claims the intent under the account lock (row_retention.py:131-141) and the retry takes the intent over only while it is open (row_write.py:254-257), so the intent row looks sufficient; a test that races a sweep against a retry's commit settles it.
  • Coordinate with the JS SDK so both remove the lock in the same release; the lock text is shared, and a one-sided removal is harmless but confusing in a shared database.

Introduced in #1467.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpythonPull requests that update python code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions