Skip to content

feat(clients): expose atomic batch send#161

Merged
NikolayS merged 10 commits into
mainfrom
feat/atomic-batch-interfaces
May 2, 2026
Merged

feat(clients): expose atomic batch send#161
NikolayS merged 10 commits into
mainfrom
feat/atomic-batch-interfaces

Conversation

@NikolayS
Copy link
Copy Markdown
Owner

@NikolayS NikolayS commented May 1, 2026

Summary

Expose atomic batch send as a first-class producer API in Go and TypeScript, document the existing Python batch API, and add comparable producer benchmarks for all three client libraries.

This PR is about the final client interfaces: application code should not need to hand-roll loops over send() when it wants to publish many messages as one logical batch.

Before

Python had a batch method:

client.send_batch("orders", "order.created", [
    {"order_id": 1},
    {"order_id": 2},
])

But Go and TypeScript users had to write loops over send():

for _, order := range orders {
    _, err := client.Send(ctx, "orders", pgque.Event{
        Type: "order.created",
        Payload: order,
    })
    if err != nil {
        return err
    }
}
for (const order of orders) {
  await client.send("orders", {
    type: "order.created",
    payload: order,
  });
}

Problems:

  • one client/server round trip per message;
  • no single logical batch boundary in the client API;
  • harder retry/error handling at the app layer;
  • Go/TS were less capable than Python despite the SQL API already supporting batch sends;
  • users could easily miss that pgque.send_batch(...) exists.

After

Go:

ids, err := client.SendBatch(ctx, "orders", "order.created", []any{
    map[string]any{"order_id": 1},
    map[string]any{"order_id": 2},
})

TypeScript:

const ids = await client.sendBatch("orders", "order.created", [
  { order_id: 1 },
  { order_id: 2 },
]);

Python docs now show the already-existing equivalent:

ids = client.send_batch("orders", "order.created", [
    {"order_id": 1},
    {"order_id": 2},
])

All three point at the same SQL-level contract:

select pgque.send_batch(queue, type, payloads[])

Contract

A batch is one logical publish operation:

  • one queue;
  • one event type;
  • many payloads;
  • returned event IDs in input order;
  • empty event type defaults to default;
  • empty payload list returns an empty ID list;
  • PostgreSQL executes the call atomically inside the caller's transaction.

If a payload cannot be encoded or the database rejects the call, no partial batch is reported as successful.

Why this is better

1. Batch is visible in all final client interfaces

Batching becomes a normal producer operation, not something only SQL/Python users discover.

2. The API matches the actual product model

sendBatch / send_batch / SendBatch mirrors the existing safe SQL API instead of exposing table internals.

3. Atomicity is explicit

The client docs/comments say the important part plainly: the call is all-or-nothing inside the caller's transaction.

4. It removes client-side loops from the happy path

Go/TS users can make one call and let PgQue handle the batch boundary.

5. It composes with #159 instead of competing with it

This PR intentionally uses the existing same-type SQL shape:

pgque.send_batch(queue, type, payloads[])

If/when #159 (or its successor from #160) makes send_batch() set-based internally, these client APIs automatically benefit without changing public interfaces.

Producer benchmarks

Added env-gated producer benchmarks for all three clients:

  • Python: clients/python/bench_producer.py
  • Go: TestProducerBenchmarks gated by PGQUE_RUN_PRODUCER_BENCH=1
  • TypeScript: bun run bench:producer

Each benchmark compares:

  1. loop over send(): loop over send() / Send() / client.send();
  2. batch API call: send_batch() / SendBatch() / sendBatch();

for batch sizes 1, 100, 1000. Each measurement creates a fresh queue, publishes N events, verifies inserted row count, drops the queue, and reports median latency + events/sec over 3 repeats.

Latest GitHub Actions client-smoke benchmark results (PostgreSQL 18 runner):

PgQue client producer throughput: send loop vs batch API

language method batch_size median_ms events_per_sec
python loop over send() 1 1.609 622
python send_batch 1 1.267 790
python loop over send() 100 31.822 3,142
python send_batch 100 6.735 14,847
python loop over send() 1000 300.820 3,324
python send_batch 1000 55.683 17,959
go loop over Send() 1 0.887 1,127
go send_batch 1 0.899 1,112
go loop over Send() 100 43.665 2,290
go send_batch 100 5.707 17,522
go loop over Send() 1000 423.223 2,363
go send_batch 1000 49.425 20,233
typescript loop over send() 1 2.041 490
typescript send_batch 1 1.447 691
typescript loop over send() 100 61.150 1,635
typescript send_batch 100 6.258 15,980
typescript loop over send() 1000 523.057 1,912
typescript send_batch 1000 52.023 19,222

Relationship to #159 and #160

This PR does not rewrite server-side send_batch().

Intended layering:

  1. send() — simple single-message API.
  2. sendBatch / send_batch / SendBatch — safe high-level app batching.
  3. direct current_event_table() insert/COPY — expert throughput path.

Changed files

  • Go:
    • Client.SendBatch(ctx, queue, type, payloads)
    • live tests for IDs/order/payloads and edge cases
    • producer benchmark for 1/100/1000
    • README quickstart example
  • TypeScript:
    • client.sendBatch(queue, type, payloads)
    • live tests for IDs/order/payloads and edge cases
    • producer benchmark for 1/100/1000 using Bun
    • README API + quickstart example
  • Python:
    • README quickstart now shows existing send_batch(...)
    • producer benchmark for 1/100/1000
    • send_batch docs clarify payload encoding and transaction semantics
  • CI:
    • client-smoke runs all three producer benchmarks and prints tables in logs.

Tests

Latest evidence is posted in comments after the final rebase/fixes:

  • CI green: claude-review, client-smoke, PG 14–18 matrix, verify
  • REV via interactive Claude Code/tmux: no blocking issues
  • Local TypeScript:
    • bun run check
    • PGQUE_TEST_DSN=... bun run test → 28 passed
    • PGQUE_TEST_DSN=... bun src/smoke.ts
    • PGQUE_TEST_DSN=... bun run bench:producer
  • Local Python:
    • PGQUE_TEST_DSN=... pytest -q clients/python/tests/test_send.py clients/python/tests/test_smoke.py → 20 passed
    • PGQUE_TEST_DSN=... python clients/python/bench_producer.py
  • Go local tests were not run on this host because no go binary is installed; GitHub client-smoke covers Go and passed.

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: ✅ All checks passing (PG 14–18, verify, client-smoke, claude-review)

BLOCKING ISSUES (3)

HIGH clients/go/pgque_test.goTestSendBatch does not verify payload contents

The test checks that 3 IDs are returned in ascending order and that message types equal "batch.test", but never decodes and asserts the actual payload values {"n":1}, {"n":2}, {"n":3}. A batch that silently reorders or corrupts payload bytes would still pass. The TypeScript counterpart correctly does expect(msgs.map(m => JSON.parse(m.payload))).toEqual([{n:1},{n:2},{n:3}]) — the Go test should match that standard.
Fix: After receiving messages, unmarshal each msg.Payload (e.g. via json.Unmarshal) and assert n == 1, 2, 3 in order.

HIGH clients/go/pgque_test.go, clients/typescript/test/client.test.ts — Empty batch (payloads=[]) is untested

sendBatch(queue, type, []) is a valid call (the SQL loop runs zero iterations and returns '{}'). Neither the Go nor TypeScript test covers this. An empty-array call with a malformed client parameter binding could return nil/undefined or panic rather than an empty slice. The contract ("empty batch returns empty IDs, no error") should be pinned.
Fix: ids, err := client.SendBatch(ctx, q, "t", []any{}) → assert len(ids)==0, err==nil. Equivalent in TypeScript.

HIGH clients/go/pgque_test.go, clients/typescript/test/client.test.tstype="" defaulting to "default" is untested for the new batch APIs

The Go SendBatch godoc says "An empty typ defaults to 'default'" and the implementation has the guard. The TypeScript sendBatch also has the same client-side normalisation. Neither test calls sendBatch(queue, "", payloads) and asserts the received message type is "default". The existing TestSend_DefaultEventType / defaults event type to "default" when omitted tests cover Send/send but not the new batch path.
Fix: Add TestSendBatch_DefaultEventType (Go) and a parallel TS test: call with type="", tick, receive, assert msg.Type == "default".


NON-BLOCKING (4)

HIGH clients/go/README.md_ = ids discards the returned event IDs in the quickstart example

The PR description explicitly calls out "returned event IDs in input order" as a key benefit. The README example immediately discards them with _ = ids, teaching users to ignore a useful value. The Python and TypeScript examples make the same mistake (no ids = client.send_batch(...) assignment).
Suggestion: Use log.Printf("queued %d events, first id=%d", len(ids), ids[0]) or similar. Update the Python and TypeScript quickstarts to assign the return value too.

HIGH clients/typescript/README.md — API table row for sendBatch undersells the constraints

"Publish a same-type batch atomically; returns event ids (bigint[])." The phrase "same-type" is easy to miss. Users who pass mixed-type payloads and expect per-type routing will get a silent error or wrong behaviour. The atomicity note doesn't clarify the "all-or-nothing" implication.
Suggestion: "Publish multiple payloads of the **same** event type in one atomic call. All payloads must share \type`. If any payload is rejected, none are inserted. Returns event ids (`bigint[]`) in input order."`

MEDIUM clients/typescript/test/client.test.ts — Test is named "atomically" but never verifies it

The test name is 'sendBatch publishes multiple payloads atomically' yet no assertion confirms that all messages arrive in the same PgQ batch (same batchId). Atomicity here means one logical batch boundary — verifiable by checking that all received msg.batchId values are equal. Without this, the test name is misleading.
Suggestion: const batchIds = new Set(msgs.map(m => m.batchId)); expect(batchIds.size).toBe(1);

MEDIUM clients/typescript/src/smoke.ts — Addition of await client.ticker(queue) after forceTick is unexplained

The smoke test now calls forceTick and then ticker back-to-back. The bug-hunter agent confirmed this is actually a correct fix (per docs/tutorial.md, force_tick alone doesn't complete the tick cycle) — but there is no comment explaining why two calls are needed. A future reader will likely remove the "duplicate" and reintroduce the bug.
Suggestion: Add a one-line comment: // ticker() must follow force_tick() to complete the batch rotation.


POTENTIAL ISSUES (3)

MEDIUM clients/typescript/test/client.test.tsnull payload in a batch is untested (confidence: 8/10)

JSON.stringify(null) returns "null" (valid), but JSON.stringify(undefined) returns undefined, which the ?? 'null' fallback catches. The Python client has an explicit test_send_batch_none_payload_produces_json_null regression test for this edge case; neither Go nor TypeScript has an equivalent. The fallback path ?? 'null' is untested code.
Suggestion: await client.sendBatch(queue, 't', [null]) → receive → expect(JSON.parse(msgs[0]!.payload)).toBeNull().

LOW clients/go/producer_benchmark_test.go — Queue leaks when verifyProducerCount fails (confidence: 7/10)

verifyProducerCount calls t.Fatal on count mismatch, which exits the goroutine before the subsequent drop_queue call is reached. The fn() error path does call drop_queue before t.Fatal, so it's inconsistent. In a flaky test environment orphan queues accumulate.
Suggestion: Register cleanup with t.Cleanup(func(){ client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) }) immediately after create_queue.

LOW clients/go/producer_benchmark_test.go, clients/typescript/src/producer_bench.ts, clients/python/bench_producer.pycurrent_event_table() result used unquoted in SQL string concatenation (confidence: 4/10)

All three benchmark verifyCount helpers do "select count(*) from " + table where table is the raw return value of pgque.current_event_table(). The queue names in these files are generated internally (low real exploitability), but the pattern diverges from pgque's own codebase which consistently wraps this via pgque.quote_fqname() before EXECUTE. Low risk in practice; worth making consistent.
Suggestion: In Go use pgx.Identifier sanitization; in TS/Python use select count(*) from pgque.quote_fqname(pgque.current_event_table($1)) directly.


Summary

Area Findings Potential Filtered
CI 0
Security 0 1 2
Bugs 0 1 3
Tests 3 1 2
Guidelines 0 0 3
Docs 4 0 3

REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS NikolayS force-pushed the feat/atomic-batch-interfaces branch from b2f2b65 to 3659a2c Compare May 1, 2026 22:10
@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: 🟡 running (client-smoke in progress; all core tests pass)

BLOCKING ISSUES (8)

HIGH clients/go/pgque.go — 4-line godoc violates CLAUDE.md "max one short line" comment rule

// SendBatch publishes multiple payloads with the same event type in one SQL
// call and returns event IDs in input order. PostgreSQL executes the call
// atomically inside the caller's transaction: if any payload is rejected, no
// message from the batch is inserted. An empty typ defaults to "default".

Fix: Collapse to one line: // SendBatch publishes multiple payloads atomically; returns event IDs in input order.

HIGH clients/go/producer_benchmark_test.go — 8-line godoc on TestProducerBenchmarks violates the one-short-line-max rule

// TestProducerBenchmarks compares producer latency/throughput for the default
// Go client API paths:
//   - loop over Client.Send(...)
//   - one Client.SendBatch(...) call
//
// It is opt-in because it is a microbenchmark-style integration test:
//
//	PGQUE_RUN_PRODUCER_BENCH=1 ...

Fix: // TestProducerBenchmarks is an opt-in integration benchmark; set PGQUE_RUN_PRODUCER_BENCH=1 to run.

HIGH clients/typescript/src/client.ts — 5-line JSDoc on sendBatch violates the one-short-line-max rule

/**
 * Publish multiple payloads with the same event type in one SQL call. Returns
 * event IDs in input order. PostgreSQL executes the call atomically inside
 * the caller's transaction: if any payload is rejected, no message from the
 * batch is inserted.
 */

Fix: /** Publish multiple payloads atomically; returns event IDs in input order. */

HIGH clients/python/bench_producer.py — multi-paragraph module docstring violates the one-short-line-max rule

The 12-line docstring with usage section and bullet list is a multi-paragraph docstring.
Fix: """Producer microbenchmarks: send() loop vs send_batch() for pgque-py."""

HIGH clients/typescript/src/producer_bench.ts — multi-paragraph module JSDoc block violates the one-short-line-max rule

10-line block with bullet list and Usage section.
Fix: // Producer microbenchmarks: send() loop vs sendBatch() for the TypeScript client.

HIGH clients/python/README.md:33commit() placement makes send() appear to auto-commit while send_batch() does not

client.send("orders", {"order_id": 42}, type="order.created")
client.send_batch("orders", "order.created", [
    {"order_id": 43},
    {"order_id": 44},
])
client.conn.commit()

The single commit() commits both operations (correct), but a reader skimming the example will interpret it as send() being auto-committing and send_batch() requiring an explicit commit.
Fix: Add a comment: # commit sends above — both send and send_batch are in the same transaction directly before client.conn.commit().

HIGH clients/go/pgque_test.goTestSendBatch uses setupQueue (hardcoded gotest_queue) instead of setupFreshQueue, causing queue-name collision with TestSend, TestSendAndReceive, and TestNack in parallel runs

setupQueue(t, client)
ids, err := client.SendBatch(ctx, "gotest_queue", "batch.test", ...

Every other new test in this PR uses connectOrSkip(t) / setupFreshQueue(t, client) which generates a unique suffix. TestSendBatch is the only one on the old helper.
Fix: Rewrite TestSendBatch to use connectOrSkip + setupFreshQueue (consistent with the test suite pattern).

HIGH clients/go/pgque_test.goTestSendBatch does not verify payload content; only message count and type are checked, not payload values

for _, msg := range msgs {
    if msg.Type != "batch.test" {
        t.Fatalf("expected type batch.test, got %s", msg.Type)
    }
}

The TypeScript test correctly does expect(msgs.map(m => JSON.parse(m.payload))).toEqual([{n:1},{n:2},{n:3}]). The Go test skips this.
Fix: Unmarshal each msg.Payload and compare against the expected {"n":1}, {"n":2}, {"n":3} values.


NON-BLOCKING (7)

HIGH clients/go/producer_benchmark_test.go, clients/typescript/src/producer_bench.ts, clients/python/bench_producer.py — SQL injection via unparameterized table name in verifyProducerCount/verifyCount/verify_count

All three use the table name returned by pgque.current_event_table() and concatenate/interpolate it directly into a subsequent SQL string. Risk is low today (table name comes from a trusted DB function with random suffixes), but it establishes an unsafe pattern.
Fix (Go): pgx.Identifier{parts...}.Sanitize(). Fix (TS): pg-format %I. Fix (Python): psycopg.sql.Identifier.

MEDIUM clients/go/producer_benchmark_test.go:68medianDuration will panic on an empty slice if all repeat iterations fail before appending

mid := len(sorted) / 2
return sorted[mid]  // index out of range if len==0

Same pattern exists in TypeScript median(). Both functions are called unconditionally after the loop.
Fix: Guard with if len(sorted) == 0 { return 0 } (Go) / if (sorted.length === 0) throw new Error(...) (TypeScript).

MEDIUM clients/python/bench_producer.py:84 — if client.conn.commit() raises, elapsed is never assigned and the failed iteration is silently skipped; the finally block's rollback() then swallows the commit error

fn(queue, payloads)
client.conn.commit()
elapsed = time.perf_counter() - start  # unreachable if commit fails
verify_count(client, queue, n)
durations.append(elapsed)

Fix: Move elapsed = time.perf_counter() - start to immediately after fn(queue, payloads), before commit().

MEDIUM clients/go/README.md:57_ = ids discards the return value without any explanation; readers learn nothing about what event IDs are for

_ = ids

Fix: Replace with log.Println("batch event IDs:", ids) or add a comment: // ids contains the event IDs in insertion order.

MEDIUM clients/typescript/src/client.ts:110sendBatch JSDoc is missing the empty-type-defaults-to-"default" guarantee that the Go godoc states explicitly

The implementation applies the default (const eventType = type && type.length > 0 ? type : 'default') but the JSDoc does not document it, unlike the Go comment which says "An empty typ defaults to 'default'".
Fix: Add to the JSDoc: An empty \type` defaults to `"default"`.`

LOW clients/typescript/src/producer_bench.ts:25Number.parseInt(env ?? '3', 10) for repeats is not validated; NaN or a negative value propagates silently to the loop

const repeats = Number.parseInt(process.env.PGQUE_BENCH_REPEATS ?? '3', 10);

The Go benchmark already validates this: if _, err := fmt.Sscanf(raw, "%d", &repeats); err != nil || repeats < 1 { t.Fatalf(...) }.
Fix: Add if (!Number.isFinite(repeats) || repeats < 1) throw new Error('PGQUE_BENCH_REPEATS must be a positive integer');

LOW clients/go/README.md:5 — intro description omits SendBatch from the named function list

Line 5: A thin, idiomatic wrapper over the pgque-api SQL functions: send, receive, ack, nack.
Fix: Add SendBatch to the list: send, send_batch, receive, ack, nack.


POTENTIAL ISSUES (5)

HIGH clients/go/pgque_test.go — no test for the marshal-error path in SendBatch; the %d-indexed error message is completely untested (confidence: 8/10)

TestSend_UnmarshalablePayload covers Send but there is no equivalent for SendBatch.
Suggestion: TestSendBatch_UnmarshalablePayload: pass []any{map[string]any{"ok":1}, make(chan int)}, assert err != nil and err.Error() contains "batch payload 1".

HIGH clients/typescript/test/client.test.ts — test name claims "atomically" but only the success path is exercised; no test injects a mid-batch failure (confidence: 7/10)

Suggestion: Add a test that sends a batch where one payload is pre-encoded as invalid JSON text, then verify via receive that zero events were delivered.

MEDIUM clients/go/pgque_test.go, clients/typescript/test/client.test.ts, clients/python/tests/ — no empty-batch test in any language (confidence: 9/10)

SendBatch(ctx, q, t, []any{}) / sendBatch(q, t, []) / send_batch(q, t, []) are all untested; the behaviour on an empty slice (empty array → SQL → empty ID list) is unverified.
Suggestion: Add one empty-batch test per client, asserting len(ids) == 0 with no error.

MEDIUM clients/go/pgque_test.goTestSendBatch does not call client.Ack after receiving, leaving the batch open (confidence: 9/10)

Every other Receive test calls Ack. An open batch may cause issues on queue teardown.
Suggestion: Add client.Ack(ctx, msgs[0].BatchID) before the test returns.

LOW clients/go/pgque_test.go:117 — strict monotonic ordering assertion is stronger than the API contract (confidence: 7/10)

if ids[0] >= ids[1] || ids[1] >= ids[2] { t.Fatalf("expected IDs in input order, got %v", ids) }

The API contract says "returned event IDs in input order" — the sequence guarantees strict ascending order within a session, so this is technically safe today, but will break if the implementation ever changes. Consider asserting distinct IDs (ids[0] != ids[1] && ids[1] != ids[2]) as a more resilient alternative.


Summary

Area Blocking Non-Blocking Potential Filtered
CI
Security 1 2
Bugs 2 1 4
Tests 2 4 10
Guidelines 5 2 2
Docs 1 2 2

REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: 🔄 running (test/14: ✅, verify: ✅; test/15–18, client-smoke: in progress)

BLOCKING ISSUES (14)

Guidelines

HIGH clients/go/pgque.go:85 — Uppercase SELECT in new SendBatch SQL string

"SELECT pgque.send_batch($1, $2, $3::jsonb[])" — violates the CLAUDE.md rule "Lowercase SQL keywords". The entire Go client (Send, Receive, Ack, Nack) already uses uppercase, so this PR continues that pre-existing violation.
Fix: "select pgque.send_batch($1, $2, $3::jsonb[])". Consider fixing the full file in a follow-up since all Go SQL strings share this problem.


Tests — Go SendBatch (5 gaps)

Per CLAUDE.md §TDD: "Red/green TDD for all new code. Applies to client libraries." The errors_test.go file has TestSend_MissingQueue, TestSend_UnmarshalablePayload, and TestSend_AfterClose for Send, but none of these parallel tests exist for SendBatch.

HIGH clients/go/pgque_test.go — Missing nil payload → JSON null test for SendBatch

Python has test_send_batch_none_payload_produces_json_null as an explicit regression guard (this exact bug previously existed in the Python client — SQL NULL vs JSON null asymmetry). Go json.Marshal(nil) produces "null", but this round-trip is never verified.
Fix: Add TestSendBatch_NilPayloadProducesJsonNull: call SendBatch(ctx, queue, "t", []any{nil}), tick, receive, assert msgs[0].Payload == "null".

HIGH clients/go/pgque_test.go — Missing empty-slice test for SendBatch

SendBatch(ctx, queue, "t", []any{}) exercises QueryRow with an empty jsonb[]. Whether the DB function accepts it or errors is not verified; the client may return nil or empty []int64.
Fix: Add TestSendBatch_EmptySlice: assert err == nil && len(ids) == 0 (or document if empty batch is rejected).

HIGH clients/go/pgque_test.go — Missing nonexistent-queue error test for SendBatch

The error-wrapping path fmt.Errorf("pgque: send batch: %w", err) is untested. TestSend_MissingQueue covers this for Send.
Fix: Add TestSendBatch_MissingQueue: call against a queue that does not exist, assert err != nil.

HIGH clients/go/pgque_test.go — Missing unmarshalable payload test for SendBatch

The json.Marshal error path (lines 72–76 of pgque.go) is untested. TestSend_UnmarshalablePayload covers Send.
Fix: Add TestSendBatch_UnmarshalablePayload: pass []any{make(chan int)}, assert a non-nil error is returned.

HIGH clients/go/pgque_test.go — Missing empty-type-defaulting test for SendBatch

The if typ == "" { typ = "default" } branch (line 81–83) is never executed in any test.
Fix: Add TestSendBatch_EmptyTypeDefaultsToDefault: call with typ = "", receive, assert msgs[0].Type == "default".


Tests — TypeScript sendBatch (5 gaps)

The TypeScript client's existing test suite has 'rejects empty queue name on send', 'defaults event type to "default" when omitted', 'send to nonexistent queue raises PgqueQueueNotFoundError' for send, but no equivalents for sendBatch.

MEDIUM clients/typescript/test/client.test.ts — Missing empty queue validation test

sendBatch throws PgqueSqlError when !queue, but this guard is untested.
Fix: Add 'sendBatch rejects empty queue name': call sendBatch('', 't', [{}]), assert PgqueSqlError.

MEDIUM clients/typescript/test/client.test.ts — Missing empty type → "default" test

const eventType = type && type.length > 0 ? type : 'default' is untested.
Fix: Add 'sendBatch defaults type to "default" when empty': call sendBatch(queue, '', [{x:1}]), tick, assert msgs[0].type === 'default'.

MEDIUM clients/typescript/test/client.test.ts — Missing empty payloads array test

Whether pgque.send_batch accepts an empty jsonb[] is not verified.
Fix: Add 'sendBatch with empty array returns empty ids': assert result is [] or document the rejection.

MEDIUM clients/typescript/test/client.test.ts — Missing nonexistent queue error-mapping test

The mapPgError('sendBatch', err, { queue }) path is untested.
Fix: Add 'sendBatch to nonexistent queue raises PgqueQueueNotFoundError'.

MEDIUM clients/typescript/test/client.test.ts — Missing non-serializable payload error test

The per-element try { JSON.stringify(payload) } catch path (lines 113–119 of client.ts) is untested.
Fix: Add a unit test passing [() => {}] (non-serializable), assert PgqueSqlError before DB is touched.


Documentation

HIGH clients/python/pgque/client.py:158send_batch docstring stripped of all API contract documentation

Removed: Args (queue, type, payloads including the str-must-be-valid-JSON-text and NoneJSON null encoding rules), Returns, and the SQL primitive it maps to. CLAUDE.md says "Only add [a comment] when the WHY is non-obvious" — the encoding rules for mixed str/dict/None payloads are exactly that.
Fix: Restore the Args/Returns block, or at minimum document the str pass-through contract and Nonenull behaviour in a condensed form.

HIGH clients/python/tests/test_send.py:163 — Regression test docstring stripped of the bug explanation

Removed: "Regression test for send/send_batch asymmetry: send(None) coerces to JSON null via "null" string, but send_batch previously passed Python None through psycopg as SQL NULL, bypassing the ::jsonb cast." This is precisely the context CLAUDE.md asks to preserve: a "workaround for a specific bug, behaviour that would surprise a reader."
Fix: Restore the regression context. Future contributors may "simplify" the None → "null" path without understanding why it's there.


NON-BLOCKING (3)

LOW clients/go/producer_benchmark_test.go, clients/python/bench_producer.py, clients/typescript/src/producer_bench.ts — SQL identifier interpolation of current_event_table result in three benchmark verifyCount helpers

All three use string concatenation/f-string/template literals to build select count(*) from <table> where <table> comes from pgque.current_event_table(). Risk is low because it is a trusted server-side function, but the pattern is poor hygiene and should not be copy-pasted into production paths.
Suggestion: Use pg_quote_ident server-side, or validate the table name against ^[a-z_][a-z0-9_.]*$ before interpolation.


POTENTIAL ISSUES (4)

MEDIUM clients/go/producer_benchmark_test.go:~330 — Queue not dropped if verifyProducerCount calls t.Fatal (confidence: 7/10)

If the insert count is wrong, t.Fatal terminates the goroutine mid-loop; the trailing drop_queue is never reached and the queue leaks across test runs.
Suggestion: Register t.Cleanup(func() { client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) }) immediately after create_queue.

MEDIUM clients/go/doc.goSendBatch not listed in package godoc intro or examples_test.go (confidence: 8/10)

doc.go description still reads "send, receive, ack, nack"; examples_test.go has ExampleClient_NewConsumer but no ExampleClient_SendBatch. Discovery of the new API relies on README alone.
Suggestion: Add SendBatch to the package description and a testable ExampleClient_SendBatch function.

MEDIUM clients/typescript/src/client.ts:123JSON.stringify(payload) ?? 'null' silently encodes undefined as the JSON literal "null" (confidence: 5/10)

JSON.stringify(undefined) returns undefined (not a string), which the ?? 'null' coerces to 'null'. This erases type information silently; callers passing undefined entries will get null events rather than an error. Behaviour may be intentional to match send(undefined), but it is not documented.
Suggestion: Either explicitly reject undefined or add a JSDoc note that undefined is stored as JSON null.

LOW (commit e5cf0cc)bench: is not a recognised Conventional Commit type per CLAUDE.md (confidence: 8/10)

CLAUDE.md lists feat:, fix:, docs:, refactor:, chore: only.
Suggestion: Retype as chore(clients): compare producer batch APIs in a follow-up (or tolerate as a one-off given the project is pre-1.0).


Summary

Area Blocking Potential Filtered
CI
Security 0 0 1
Bugs 0 2 2
Tests 10 0 1
Guidelines 1 1 2
Docs 3 1 2

The new SendBatch/sendBatch APIs are cleanly designed and the happy-path tests pass. The main concerns are test coverage parity with Send in both Go and TypeScript (the project's own TDD rule applies here), two documentation regressions in the Python client (a stripped API-contract docstring and a stripped regression-bug explanation), and one style violation (uppercase SELECT in new Go SQL).


REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: ⚠️ in progress — test (15/16/17/18), client-smoke running; test (14) and verify passed

BLOCKING ISSUES (2)

MEDIUM clients/python/pgque/client.py:158 — send_batch docstring condensation removes load-bearing API contract

The previous multi-line docstring documented three non-obvious encoding rules (dict/list → JSON-encoded; str → must already be valid JSON text; None → JSON null) and the return semantics (list of event IDs in input order). The replacement one-liner retains only two of the three rules and drops return type documentation entirely. For a public library method with non-obvious str/None edge cases, this is the only canonical prose documentation available to users outside of reading the source.
Fix: Restore the key contract as a compact but complete docstring: """Send same-type payloads in one SQL call. dict/list entries are JSON-encoded automatically; str must be valid JSON text; None is stored as JSON null. Returns event IDs in input order."""

MEDIUM clients/go/pgque.go:69 — SendBatch godoc is a minimal single-liner, inconsistent with every other exported function

Every other exported function — Connect, Send, Receive, Ack, Nack — has a multi-sentence godoc explaining parameters and return semantics. SendBatch gets one line that documents the type-default behavior but says nothing about: what happens to an empty payloads slice, that returned []int64 are in input order, or that each payload is individually JSON-marshalled. The return-order guarantee is an important API contract and has no other documentation home.
Fix: Expand to match the pattern: // SendBatch publishes same-type payloads atomically. Each payload is JSON-marshalled individually; an empty typ defaults to "default". Returns event IDs in input order; an empty payloads slice returns nil, nil.


NON-BLOCKING (5)

MEDIUM clients/python/bench_producer.py:75 — unconditional rollback() in finally block after a successful commit is a misleading no-op

After client.conn.commit() succeeds, the finally: client.conn.rollback() starts and immediately ends a new empty transaction — it cannot undo the committed inserts. On failure paths it also cannot help, because the drop_queue + commit in the same finally handles cleanup. The rollback gives a false impression that failures are being undone.
Suggestion: Remove the rollback() from finally, or move it into an except branch to only run when the inner block raises before commit.

LOW clients/typescript/src/client.ts:114JSON.stringify(payload) ?? 'null' silently stores undefined values as JSON null

JSON.stringify(undefined) returns undefined (not a string), so the ?? fallback fires and stores 'null' without any error. This means callers that accidentally pass undefined get silent data loss instead of a clear serialization error. Python and Go both surface encoding errors; TypeScript is inconsistent for this specific case.
Suggestion: Throw PgqueSqlError when JSON.stringify returns undefined rather than falling back to 'null': const s = JSON.stringify(payload); if (s === undefined) throw new PgqueSqlError('sendBatch', { cause: new Error(\payload at index ${index} is not JSON-serializable`) }); return s;`

LOW clients/typescript/README.md:39 — sendBatch quickstart example discards the return value

The Go and Python quickstarts both capture and print the returned IDs (ids, batch_ids). The TypeScript quickstart calls await client.sendBatch(...) without capturing the result. Callers who want to correlate inserted events won't see this pattern demonstrated.
Suggestion: Add const batchIds = await client.sendBatch(...) and a console.log('batch ids:', batchIds) line to match the other language quickstarts.

LOW clients/typescript/README.md:84 — Errors section does not mention which errors sendBatch can throw

The Errors section documents PgqueQueueNotFoundError and PgqueConsumerNotFoundError for receive, but sendBatch can throw PgqueQueueNotFoundError (unknown queue) and PgqueSqlError (non-serializable payload). The API table row says "atomically" but gives no error guidance.
Suggestion: Add a note to the sendBatch table row, e.g. throws PgqueQueueNotFoundError for unknown queues.

INFO clients/typescript/src/producer_bench.ts:79 — drop_queue cleanup swallows all errors silently

.catch(() => undefined) hides any drop_queue failure, including connection errors that would then corrupt subsequent iterations of the bench loop.
Suggestion: At minimum log: .catch((err) => console.warn('drop_queue cleanup failed:', err)).


POTENTIAL ISSUES (8)

MEDIUM clients/typescript/test/client.test.ts:69 — test named "atomically" but does not verify partial-failure rollback (confidence: 8/10)

The test checks happy-path ID ordering but does not assert that if the DB rejects one payload, none are inserted. The "atomically" description promises all-or-nothing semantics.
Suggestion: Add a test that forces a DB-level failure mid-batch (e.g. passes an invalid payload that exceeds a column constraint inside a transaction that is aborted) and asserts the event table is empty afterwards.

MEDIUM clients/go/pgque_test.go:98 — no atomicity failure test for SendBatch (confidence: 8/10)

Same gap as TypeScript — the godoc says "atomically" but the test suite only covers happy paths.
Suggestion: Add TestSendBatchAtomicRollback that wraps SendBatch in a transaction the test deliberately rolls back and asserts no events were inserted.

MEDIUM clients/typescript/test/client.test.ts:166undefined element behavior in sendBatch is untested (confidence: 7/10)

JSON.stringify(undefined) returns undefined, which is the exact case the ?? 'null' fallback (line 114) handles silently. There is no test asserting whether undefined should throw or store JSON null. The circular-ref test covers the throw path; the silent-null path is uncovered.
Suggestion: Add skipIfNoDb('sendBatch stores undefined payload as JSON null', ...) to pin the current behavior, or change the implementation to throw and add a test for that.

MEDIUM clients/python/tests/test_send.py — send_batch missing: empty list, empty type defaulting, missing queue error (confidence: 7/10)

Go and TypeScript each have explicit tests for [], "" type defaulting, and nonexistent queue. Python's test suite covers only the None→JSON-null regression. This creates inconsistent coverage parity across the three clients.
Suggestion: Add test_send_batch_empty_list, test_send_batch_empty_type_defaults_to_default, and test_send_batch_missing_queue_raises.

LOW clients/go/producer_benchmark_test.go:74 — division by zero if median is 0 (confidence: 6/10)

eps := float64(n) / median.Seconds() produces +Inf when median == 0. TypeScript and Python both guard with medianMs > 0 ? ... : Infinity; Go does not.
Suggestion: Add if median > 0 { eps = float64(n) / median.Seconds() } else { eps = math.Inf(1) }.

LOW clients/typescript/src/producer_bench.ts:122median([]) returns NaN when PGQUE_BENCH_REPEATS=0 (confidence: 6/10)

The Go benchmark validates repeats < 1 and fails fast; Python's statistics.median([]) raises StatisticsError. TypeScript silently produces NaN rows.
Suggestion: Validate at startup: if (!Number.isFinite(repeats) || repeats < 1) throw new Error('PGQUE_BENCH_REPEATS must be >= 1').

LOW clients/go/producer_benchmark_test.go:62 — cleanup Exec on error path ignores its own error (confidence: 7/10)

client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) — the returned error and CommandTag are discarded. If cleanup fails (e.g. cancelled context), the queue silently leaks.
Suggestion: if _, dErr := client.Pool().Exec(...); dErr != nil { t.Logf("cleanup drop_queue failed: %v", dErr) }.

LOW benchmark/charts/client_producer_batch_api.svg:83 — "Ty" abbreviation for TypeScript in Panel B is ambiguous (confidence: 6/10)

Panel A's legend uses full language names; Panel B uses "Py"/"Go"/"Ty". "Ty" is not a standard TypeScript abbreviation; "TS" is universal.
Suggestion: Replace "Ty" with "TS" in all four occurrences in Panel B.


Summary

Area Blocking Non-Blocking Potential Filtered
CI
Security 0 0 0 0
Bugs 0 1 3 1
Tests 0 0 4 1
Guidelines 1 1 1 1
Docs 1 3 0 1

REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: ✅ all checks passing (claude-review, client-smoke, test 14–18, verify)

POTENTIAL ISSUES (7)

LOW clients/typescript/test/client.test.ts:69 — TypeScript lacks a null/undefined-payload regression test for sendBatch, while Go (TestSendBatchNilPayloadProducesJSONNull) and Python (test_send_batch_none_payload_produces_json_null) both have one. (confidence: 7/10)

The TS implementation relies on JSON.stringify(payload) ?? 'null' to coerce undefined into JSON null. A regression dropping the ?? 'null' would not be caught.
Suggestion: Add skipIfNoDb('sendBatch null/undefined payload stored as JSON null', ...) calling sendBatch(queue, 'batch.null', [null, undefined]), advancing, and asserting both received payload === 'null'.

LOW clients/typescript/src/client.ts:104sendBatch JSDoc is a single terse line, sparser than the Go godoc and Python docstring. (confidence: 6/10)

Doesn't mention the return type (bigint[] in input order), atomicity scope, or thrown error classes (PgqueSqlError / PgqueQueueNotFoundError).
Suggestion: Expand to ~3 lines mirroring the Go godoc: return order, server-side atomicity, default-type behavior, error class.

LOW clients/go/producer_benchmark_test.go:419client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) discards both return values. (confidence: 5/10)

errcheck/staticcheck flag this; the cleanup error is swallowed silently before t.Fatal.
Suggestion: _, _ = client.Pool().Exec(...) or log the cleanup error.

LOW clients/go/producer_benchmark_test.go:422 — Test queue can leak when verifyProducerCount calls t.Fatal. (confidence: 5/10)

If verification fails, t.Fatal ends the test before the post-verify drop_queue at line 424 runs. The queue created at line 412 stays behind. CI uses an ephemeral DB so impact is bounded, but a flaky run litters gobench_* queues.
Suggestion: t.Cleanup(func(){ _, _ = client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) }) immediately after create_queue.

LOW clients/typescript/src/producer_bench.ts:848eventsPerSec formats as Infinity in Markdown/CSV when medianMs == 0. (confidence: 4/10)

Number.POSITIVE_INFINITY.toFixed(0) === 'Infinity'; downstream CSV parsers may break. Same in Python (float('inf')'inf').
Suggestion: Emit a sentinel (blank or 0) for unmeasurable rows, or guard formatting.

LOW clients/python/bench_producer.py:586 — Unconditional client.conn.rollback() in finally: runs even after the commit() on line 581. (confidence: 4/10)

Harmless on psycopg3 (no-op after commit) but misleading; suggests the producer transaction may need rollback when it's already committed.
Suggestion: Move the rollback into an except: branch, or drop it — commit() + the subsequent drop_queue + commit() already manage state.

LOW clients/python/README.md:27 — Quickstart comment "commit once to publish both calls atomically" assumes reader knows psycopg defaults to autocommit-off. (confidence: 4/10)

Suggestion: Optionally add a parenthetical, e.g. "(pgque.connect opens a psycopg connection with autocommit off, so both calls share one transaction until commit())." — or leave as-is, this is a small ergonomic note rather than a doc bug.


Summary

Area Findings Potential Filtered
CI 0
Security 0 0 0
Bugs 0 4 0
Tests 0 1 0
Guidelines 0 0 3 (INFO, dropped)
Docs 0 2 0

No blocking issues. Implementation, error handling, and test coverage are sound. The atomic-batch contract is consistent across Go/TypeScript/Python, and benchmarks fail loud on count mismatch. Items above are quality-of-life polish.


REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 1, 2026

Final testing / review evidence for latest head e51ecb0776ef4ab1ad910e6f2eb87d761aa69136:

Rebased on latest main after #162 and adjusted to the Bun workflow.

Local verification:

cd clients/typescript
bun run check
PGQUE_TEST_DSN=postgresql://postgres:pgque_test@localhost/pgque_batch_test bun run test
PGQUE_TEST_DSN=postgresql://postgres:pgque_test@localhost/pgque_batch_test bun src/smoke.ts
PGQUE_TEST_DSN=postgresql://postgres:pgque_test@localhost/pgque_batch_test bun run bench:producer

cd ../..
PGQUE_TEST_DSN=postgresql://postgres:pgque_test@localhost/pgque_batch_test /tmp/pgque-pr161-venv/bin/pytest -q clients/python/tests/test_send.py clients/python/tests/test_smoke.py
PGQUE_TEST_DSN=postgresql://postgres:pgque_test@localhost/pgque_batch_test /tmp/pgque-pr161-venv/bin/python clients/python/bench_producer.py

Results:

  • TypeScript check ✅
  • TypeScript DB tests: 28 passed ✅
  • TypeScript smoke: pgque TypeScript smoke test: OK
  • TypeScript producer benchmark ✅
  • Python DB tests: 20 passed ✅
  • Python producer benchmark ✅
  • Go local tests not run here because this host has no go binary; GitHub client-smoke covers Go and passed ✅

CI:

  • claude-review
  • client-smoke
  • PG matrix test (14)
  • PG matrix test (15)
  • PG matrix test (16)
  • PG matrix test (17)
  • PG matrix test (18)
  • verify

REV:

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 2, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: `feat/atomic-batch-interfaces`
  • CI: ⏳ running (verify ✅; client-smoke, PG matrix, claude-review in progress)

BLOCKING ISSUES (2)

HIGH `clients/go/pgque_test.go:260-271` — TestSendBatch payload-order assertion will flake

`for i, msg := range msgs { if payload["n"] != i+1 }` relies on `client.Receive()` returning messages in insertion order. PgQ's `batch_event_sql` builds a range query over `ev_txid` without `ORDER BY ev_id`. All 3 events share the same txid (inserted by one `send_batch` call), so message order is heap-scan order — not guaranteed. The test passes consistently on a fresh queue today but will produce a false failure once the table is bloated or vacuumed.

Fix: Replace the per-index check with an order-independent assertion — collect the `n` values, sort, and compare to `[1, 2, 3]`. Alternatively sort `msgs` by `MsgID` first (ascending IDs are guaranteed by the sequence).

MEDIUM `clients/go/pgque.go:187` — "inside the caller's transaction" is misleading

The comment says "PostgreSQL executes the call atomically inside the caller's transaction." But `SendBatch` uses `c.pool.QueryRow`, which acquires an implicit single-statement transaction — it does not inherit any caller-managed `pgx.Tx`. The atomicity comes from the fact that `pgque.send_batch()` is a single SQL call (PL/pgSQL function = one statement = one implicit tx). A Go caller who already has a transaction open does not automatically get `SendBatch` to join it; they need to call `pgque.send_batch` directly through `pgx.Tx.QueryRow`.

Fix: Rewrite to: "The SQL function executes as a single statement and is therefore atomic: either all payloads are inserted or none are. To combine this with other writes in one transaction, use `pgx.Tx` and call the SQL function directly."


NON-BLOCKING (3)

INFO `clients/typescript/src/client.ts:104` — `sendBatch` JSDoc is a bare one-liner

Every other public method (`send`, `receive`, `nack`, etc.) has a full JSDoc block with `@param` descriptions, `@returns`, and payload-encoding caveats. `sendBatch` gets `/** Publish same-type payloads atomically; empty type defaults to `default`. */`. TypeScript hover-doc and API generators will produce sparse output.

Suggestion: Add `@param`/`@returns` tags and document the `undefined → null` coercion (same as `send`'s existing warning).

INFO `clients/go/README.md:1-5` — `SendBatch` not mentioned in package prose

The Go README prose enumerates the wrapped SQL functions (`send`, `receive`, `ack`, `nack`) but doesn't include `send_batch` now that `SendBatch` is a first-class method.

Suggestion: Append `send_batch` to the prose list.

INFO `clients/typescript/README.md:66` — "event ids" casing inconsistent

The API table uses lowercase "event ids" / "event id" while the prose immediately below and the Go README both use "IDs". Small but visually inconsistent.

Suggestion: Capitalize to "event ID" / "event IDs" in the table.


POTENTIAL ISSUES (7)

MEDIUM `clients/python/bench_producer.py:74-77` — `finally` block can mask original exception (confidence: 7/10)

If the benchmark `fn()` raises, `rollback()` recovers the connection, then `drop_queue` runs. If `drop_queue` itself fails, its exception propagates and completely replaces the original measurement error. Cleanup failure also leaks the queue across runs.

Suggestion: Wrap the cleanup block in `try/except` and log (not raise) cleanup failures so the original error is preserved.

MEDIUM `clients/typescript/src/producer_bench.ts:122-127` — `median([])` returns `undefined` when all iterations fail (confidence: 7/10)

If every iteration throws before `durations.push()`, `median([]))` returns `sorted[undefined]` = `undefined` (the non-null assertion `!` lies). Then `medianMs.toFixed(3)` throws a `TypeError`. Unreachable when `repeats >= 1` and errors rethrow, but the function is latently unsafe.

Suggestion: Add `if (sorted.length === 0) return NaN;` guard in `median()`.

MEDIUM `clients/python/pgque/client.py:158` — new `send_batch` docstring drops explicit empty-list and required-`type` behavior (confidence: 7/10)

The old docstring had an `Args:`/`Returns:` block that explicitly said: all parameters, the `type` has no default (unlike `send`), and an empty list returns `[]`. The new compact docstring omits all of this. A caller who scans only the docstring won't know `type` is required or that passing `[]` is safe.

Suggestion: Add at minimum: "An empty `payloads` list returns `[]`. The `type` argument is required and has no default (unlike `send`)."

MEDIUM `clients/typescript/src/client.ts:114` — `undefined` payload entries silently become JSON `null` with no test (confidence: 7/10)

`JSON.stringify(undefined)` returns JS `undefined`, so `undefined ?? 'null'` = `'null'`. A caller who passes `[undefined]` gets a JSON-null message without any warning. The behavior is consistent with `send(null)` but is undocumented in the JSDoc and not covered by a test (unlike Go's `TestSendBatchNilPayloadProducesJSONNull`).

Suggestion: Add a test `sendBatch with undefined entry produces JSON null` mirroring the Go test, and document the coercion in the JSDoc.

LOW `clients/go/pgque_test.go:332` — `TestSendBatchMissingQueue` only asserts `err != nil` (confidence: 7/10)

The corresponding `TestSend_MissingQueue` checks that the error message references the queue name. `TestSendBatchMissingQueue` accepts any non-nil error, so a driver-level connection error would pass the test even if the queue-not-found path were broken.

Suggestion: Assert the error type (e.g., check for `queue not found` in message, or use a typed `QueueNotFoundError` if the client exposes one).

LOW `clients/python/tests/test_send.py` — no test for `send_batch` with an empty list (confidence: 6/10)

Go (`TestSendBatchEmptySlice`) and TypeScript (`sendBatch accepts an empty array`) both test the empty-batch edge case. Python doesn't. The psycopg3 empty-array round-trip is driver-specific.

Suggestion: Add `test_send_batch_empty_list` asserting `ids == []` and no messages after tick.

LOW Commit `3659a2c` — subject is 57 characters, 7 over the 50-char limit (confidence: 9/10)

`docs(clients/typescript): update producer bench bun usage` (57 chars). CLAUDE.md requires subject < 50 chars.

Suggestion: Shorten on the next commit, e.g. `docs(clients/typescript): update bench bun usage` (48 chars).


Summary

Area Blocking Non-Blocking Potential Filtered
CI
Security 0 0 0 0
Bugs 1 0 2 2
Tests 1 0 3 1
Guidelines 0 0 1 3
Docs 0 3 1 0

REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS
Copy link
Copy Markdown
Owner Author

NikolayS commented May 2, 2026

REV Code Review Report

  • PR: NikolayS/pgque#161 — feat(clients): expose atomic batch send
  • Author: @NikolayS
  • Branch: feat/atomic-batch-interfaces
  • CI: ⏳ running (test-15, test-18, client-smoke, claude-review in progress; verify ✅, test-14/16/17 ✅)

BLOCKING ISSUES (0)

No blocking issues found.


NON-BLOCKING (7)

MEDIUM clients/go/pgque.go — SendBatch doc comment omits empty-slice and nil-payload behaviors

Empty payloads and nil element behavior are tested but not documented in the public comment. Both are tested by TestSendBatchEmptySlice and TestSendBatchNilPayloadProducesJSONNull but invisible to API consumers.
Suggestion: Add two sentences to the doc comment: one for empty-slice short-circuit (An empty payloads slice returns nil without a database round-trip.), one for nil→JSON null.

MEDIUM clients/typescript/src/client.ts:104 — sendBatch JSDoc is a one-liner stub

/** Publish same-type payloads atomically; empty type defaults to \default`. */— omits: empty-array returns[], non-serializable payloads throw PgqueSqlErrorbefore hitting the DB, empty queue throws, and serialization caveats. The parallelsend()has a fuller JSDoc. **Suggestion:** Expand to a multi-line JSDoc documenting the edge cases that are already tested inclient.test.ts`.

MEDIUM clients/typescript/src/client.ts:759JSON.stringify(undefined/function/Symbol) returns undefined, silently coerced to 'null' via ?? 'null'

return JSON.stringify(payload) ?? 'null';JSON.stringify(undefined) returns undefined (does not throw), so the ?? is the only guard. No test covers this path; Go and Python both error on non-serializable values before they reach the DB. The behavior is correct but undocumented and untested.
Suggestion: Add a test asserting the behavior (undefined element → stored as JSON null), or explicitly throw PgqueSqlError for undefined/function inputs to match Go/Python error behavior.

MEDIUM clients/typescript/test/client.test.ts:934 — sendBatch test doesn't assert returned IDs are bigint

expect(ids[0]).toBeLessThan(ids[1]!) — numeric comparison passes for both bigint and number. The send test explicitly asserts expect(typeof eid).toBe('bigint'). This would silently pass if the bigint parser registration broke.
Suggestion: Add expect(typeof ids[0]).toBe('bigint') consistent with the send test.

LOW clients/go/producer_benchmark_test.go, clients/python/bench_producer.py, clients/typescript/src/producer_bench.ts — SQL identifier from current_event_table() interpolated directly

Go: "select count(*) from "+table / Python: f"select count(*) from {table}" / TS: `select count(*) from ${table}` — not exploitable (table name from trusted server function), but establishes an unsafe pattern in code that tends to be copied.
Suggestion: Use pgx.Identifier{schema, name}.Sanitize() (Go), psycopg.sql.Identifier (Python), or a schema.table regex check (TS) to make the trust boundary explicit.

LOW clients/typescript/README.md:67 — sendBatch API table row omits edge-case contract

Publish a same-type batch atomically; returns event ids (\bigint[]`).— missing: empty array →[], empty type → 'default', IDs in input order. **Suggestion:** Publish a same-type batch atomically; returns event IDs (`bigint[]`) in input order. Empty array returns `[]`. Empty `type` defaults to `'default'`.`

LOW clients/typescript/README.md:67 — "event ids" should be "event IDs"

Inconsistent with the rest of the file (lines 78, 80 use "IDs").
Suggestion: idsIDs.


POTENTIAL ISSUES (4)

MEDIUM clients/python/bench_producer.py:68 — measure() finally block calls rollback after already-committed transaction (confidence: 6/10)

On success client.conn.commit() runs before the finally, so the rollback is a no-op. If verify_count raises RuntimeError, elapsed is never appended; if all repeats fail, statistics.median([]) raises StatisticsError on the next call. Only affects the benchmark tool.
Suggestion: Append elapsed before verify_count, or guard statistics.median against an empty list.

LOW clients/python/bench_producer.py:41 — lambdas close over outer client instead of using measure's parameter (confidence: 5/10)

lambda q, payloads: send_loop(client, q, payloads) — harmless today since client is never reassigned, but the measure function's own client parameter is unused, creating a silent coupling.
Suggestion: functools.partial(send_loop, client) or pass client through fn's signature.

LOW clients/go/producer_benchmark_test.go:~420 — drop_queue error silently discarded on the failure cleanup path (confidence: 4/10)

client.Pool().Exec(ctx, "select pgque.drop_queue($1, true)", queue) result ignored before t.Fatal. A failed cleanup leaves a dangling bench queue.
Suggestion: if _, derr := client.Pool().Exec(...); derr != nil { t.Logf("cleanup drop failed: %v", derr) }.

LOW clients/go/pgque_test.go — TestSendBatchNilPayloadProducesJSONNull doesn't call Ack after receiving (confidence: 4/10)

All other receive-asserting tests in the file call Ack; this one doesn't. Harmless but inconsistent.
Suggestion: Add client.Ack(context.Background(), msgs[0].BatchID) after the assertion.


Summary

Area Findings Potential Filtered
CI
Security 0 1 0
Bugs 1 3 0
Tests 1 0 5
Guidelines 0 0 4
Docs 5 0 3

REV-assisted review (AI analysis by postgres-ai/rev)

@NikolayS NikolayS merged commit 1b3814d into main May 2, 2026
8 checks passed
@NikolayS NikolayS deleted the feat/atomic-batch-interfaces branch May 2, 2026 02:36
@NikolayS NikolayS mentioned this pull request May 7, 2026
@NikolayS NikolayS mentioned this pull request May 24, 2026
22 tasks
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.

1 participant