integration-test/race_condition/test_race_condition_parallel.sh (1)

74-98: Same output interleaving issue in remaining parallel checks.

The same output interleaving problem from lines 62-72 also affects the parallel checks after 1s wait and 5s wait. Apply similar fixes to all three sections to properly capture and display results.

internal/info.go (1)

26-26: Core Version constant bumped to v1.4.7 — verified OK.

Verification confirms no leftover v1.4.6 references and public API docs correctly reflect v1.4.7. For future releases, consider driving doc versions from a single source (or CI inject) to prevent drift across documentation.

integration-test/race_condition/docker-compose-test.yml (1)

2-8: Document test-only credentials.

The hardcoded credentials (pg/pg) are appropriate for a local test environment. Consider adding a comment to clarify these are test-only and should not be used in production.

Apply this diff to add clarifying comments:

+  # Test environment only - uses simple credentials for local testing
   postgres:
     image: postgres:17-alpine
     environment:

integration-test/race_condition/README.md (1)

14-22: Consider adding expected outcomes and prerequisites.

The test instructions are clear, but could be enhanced with:

1. Expected test results (what success looks like)
2. Note about making scripts executable (chmod +x *.sh)
3. Prerequisites (Docker, Docker Compose, curl, jq)

Consider adding a section like:

### Prerequisites
- Docker and Docker Compose
- `curl` and `jq` installed

### Expected Results
Both tests should demonstrate that permissions are correctly isolated after concurrent deletions. The `read` permission should remain granted while `comment` and `edit` should be denied after deletion.

**Note:** Make scripts executable before running: `chmod +x *.sh`

integration-test/race_condition/test_race_condition.sh (3)

1-7: Consider adding error handling and validation.

The script correctly uses set -e but doesn't validate API responses. Consider capturing and checking HTTP status codes or response content to ensure operations succeed.

Example enhancement:

#!/bin/bash
set -e

# Write schema
RESPONSE=$(curl -sS -w '\n%{http_code}' 'http://localhost:3477/v1/tenants/t1/schemas/write' \
     -H "Content-Type: application/json" \
     -d "$(cat "schema.perm" | jq -Rs '{schema: . }')")

HTTP_CODE=$(echo "$RESPONSE" | tail -n1)
if [ "$HTTP_CODE" -ne 200 ]; then
    echo "Schema write failed with HTTP $HTTP_CODE"
    exit 1
fi

---

22-33: Clarify the metadata depth parameter.

The depth: 20 parameter is used but not documented. Consider adding a comment explaining why this depth is chosen for the race condition test, or confirm if this is necessary for the test scenario.

 function check_access() {
     ACTION=$1
-
+    # depth: 20 ensures deep permission resolution across potential relation chains
     curl -sS -w '\n' 'http://localhost:3477/v1/tenants/t1/permissions/check' \

---

50-76: Test lacks programmatic validation.

The test outputs results but doesn't validate them programmatically. Consider adding assertions to verify:

1. Read permission remains granted throughout
2. Comment and edit permissions are denied after deletion
3. Results are stable after the wait period

Example validation:

# After DELETE checks
READ_RESULT=$(check_access 'read' | jq -r '.can')
COMMENT_RESULT=$(check_access 'comment' | jq -r '.can')
EDIT_RESULT=$(check_access 'edit' | jq -r '.can')

if [ "$READ_RESULT" != "CHECK_RESULT_ALLOWED" ]; then
    echo "ERROR: Read permission should still be granted"
    exit 1
fi

if [ "$COMMENT_RESULT" = "CHECK_RESULT_ALLOWED" ]; then
    echo "ERROR: Comment permission should be denied"
    exit 1
fi

integration-test/race_condition/test_race_condition_parallel.sh (1)

1-46: Consider extracting shared code to reduce duplication.

Lines 1-46 are identical to test_race_condition.sh. Consider extracting the shared functions and setup into a common file (e.g., test_common.sh) that both scripts can source.

Example structure:

test_common.sh:

#!/bin/bash

BASE_URL="http://localhost:3477"
TENANT="t1"

function check_access() { ... }
function drop_access() { ... }
function setup_schema() { ... }
function grant_permissions() { ... }

Then in both test scripts:

#!/bin/bash
set -e
source "$(dirname "$0")/test_common.sh"

setup_schema
grant_permissions
# ... test-specific logic

internal/storage/postgres/data_reader.go (2)

36-38: RepeatableRead + snapshot-aware filters LGTM; reduce repeated type assertions

• Isolation change to RepeatableRead is appropriate for stable reads.
• Passing both xid and snapshot into utils.SnapshotQuery is consistent with the new API.
• Minor: store st as a concrete snapshot.Token once to avoid repeated assertions.

Example:

- var st token.SnapToken
- st, err = snapshot.EncodedToken{Value: snap}.Decode()
+ var st token.SnapToken
+ st, err = snapshot.EncodedToken{Value: snap}.Decode()
  if err != nil { /* ... */ }
- builder = utils.SnapshotQuery(builder, st.(snapshot.Token).Value.Uint, st.(snapshot.Token).Snapshot)
+ tok := st.(snapshot.Token)
+ builder = utils.SnapshotQuery(builder, tok.Value.Uint, tok.Snapshot)

Also applies to: 60-60, 135-135, 223-223, 281-281, 369-369, 482-482

---

559-575: Add supporting index for HeadSnapshot query

Create the composite index to avoid seq scans and reduce latency under load:

CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_transactions_tenant_id_id
ON transactions(tenant_id, id DESC);

Add a reversible migration.

Also applies to: 586-587

internal/storage/postgres/utils/common_test.go (1)

28-40: Good coverage; add case for xid > xmax

Please add a test where xid > xmax to validate createFinalSnapshot adjusts xmax and xip_list appropriately.

Example:

It("Case 4: New mode with snapshot (xid > xmax)", func() {
  sl := squirrel.Select("column").From("table")
  revision := uint64(50)
  snapshot := "30:40:35" // xid(50) > xmax(40) -> expect adjusted final snapshot
  query := utils.SnapshotQuery(sl, revision, snapshot)
  sql, args, err := query.ToSql()
  Expect(err).ShouldNot(HaveOccurred())
  Expect(sql).Should(Equal(
    "SELECT column FROM table WHERE (pg_visible_in_snapshot(created_tx_id, ?) = true OR created_tx_id = ?::xid8) AND ((pg_visible_in_snapshot(expired_tx_id, ?) = false OR expired_tx_id = ?::xid8) AND expired_tx_id <> ?::xid8)",
  ))
  Expect(args[0]).ShouldNot(Equal(snapshot)) // final snapshot should change
  Expect(args).Should(HaveLen(5))
})

Also applies to: 42-55, 56-69

internal/storage/postgres/snapshot/token_test.go (1)

41-54: Extend tests: comparator with snapshots and invalid new-format

• Verify Eg/Gt/Lt ignore Snapshot by adding cases where Snapshot differs but Value equals.
• Add a failing decode case for base64-valid but invalid format (no colon), e.g., base64("123") -> expect error.

Happy to draft these tests if you want.

Also applies to: 116-127, 136-151, 153-165

internal/storage/postgres/snapshot/token.go (1)

37-55: Token format handling looks solid; tighten errors and doc comment

• Return a typed sentinel error for invalid token formats to simplify caller handling, e.g., var ErrInvalidTokenFormat = errors.New("invalid token format"), and use it instead of fmt.Errorf.
• Fix the comment on EncodedToken.String() (it returns the encoded value, doesn’t decode).
• Optional: consider prefixing new formats (e.g., "v2:xid:snapshot") to future‑proof beyond the len==8 heuristic.

Also applies to: 75-119, 121-124

internal/storage/postgres/utils/common.go (1)

121-121: Cast snapshot parameter to pg_snapshot for type safety.

Passing a raw text parameter relies on implicit casts. Be explicit to avoid version/driver quirks.

Apply:

-    squirrel.Expr("pg_visible_in_snapshot(created_tx_id, ?) = true", finalSnapshot),
+    squirrel.Expr("pg_visible_in_snapshot(created_tx_id, ?::pg_snapshot) = true", finalSnapshot),
@@
-      squirrel.Expr("pg_visible_in_snapshot(expired_tx_id, ?) = false", finalSnapshot),
+      squirrel.Expr("pg_visible_in_snapshot(expired_tx_id, ?::pg_snapshot) = false", finalSnapshot),

Also applies to: 128-128

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro