feat(storage): add PostgreSQL dialect nucleus to the shared store core

[Kiran01bm]

Summary

Adds the PostgreSQL nucleus to the shared sqlstore core: a PostgresDialect implementing the binder, SQL-rendering, and identity seams that the dialect-parameterized store already consumes, plus the first cross-dialect parity lanes proving the shared SQL runs unmodified on PostgreSQL.

Stacked on the SQL-portability and joined-DML seam PRs (#1007, #1009); base branch is the joined-DML branch.

What

• PostgresDialect.Rebind rewrites store-native ? placeholders to $n ordinals, leaving ? untouched inside single-quoted strings (including '' escapes), double-quoted identifiers, dollar-quoted bodies ($$…$$, $tag$…$tag$ — tags follow identifier rules, so a digit-leading $1 stays a parameter), -- line comments, and nested /* */ block comments.
• PostgreSQL rendering for every dialect seam:
  • EXCLUDED.col upserts via ON CONFLICT … DO UPDATE, now() timestamps (microsecond precision is native), and interval arithmetic (now() - ? * interval '1 second').
  • InsertIfAbsent via ON CONFLICT (cols) DO NOTHING (conflict reports zero affected rows, same contract as INSERT IGNORE).
  • JSONBooleanIsTrue via jsonb_extract_path(...) IS NOT DISTINCT FROM 'true'::jsonb — null-safe like MySQL's <=>, so missing paths, JSON null, and SQL NULL yield false rather than NULL.
  • IndexHint renders empty — PostgreSQL has no index-hint syntax.
  • JoinedUpdate via UPDATE … SET … FROM … WHERE (join) AND (predicate); SET assignments stay textually ahead of the predicate so ?→$n ordinal rebinding preserves the MySQL rendering's argument order.
• Identity insertion via RETURNING id — InsertID scans the generated key; InsertGuardedID maps the no-row case of a guarded INSERT … SELECT to inserted=false instead of an error.
• NewWithDependencies now injects the configured identity/dialect into the task, apply-log, plan-comment, and settings stores, which previously pinned MySQLDialect{}. MySQL callers pass MySQLDialect{} for every seam; the one MySQL-visible change is that settings.Set now renews updated_at on same-value writes, so the column tracks the last write rather than the last value change — deliberate, documented in code, and covered by tests.
• First parity lanes: the shared storagetest.TestSettings suite and an apply-logs round-trip run against a real PostgreSQL container, plus pgx stdlib value-contract pins (boolean, jsonb []byte, timestamptz round-trip precision, RowsAffected matched-rows semantics).

Why

The store core is dialect-parameterized but until now only had a MySQL implementation, so the seams were exercised on one side only. This nucleus makes the second dialect real and lets each remaining store family be brought onto PostgreSQL in small, individually verifiable slices.

Deliberately out of scope (follow-up slices): joined DML for the operation/task/apply stores, growing the PostgreSQL parity lanes to the remaining store families, and the public PostgreSQL store constructor.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[morgo]

🤖 Approving on Morgan's behalf (automated review, escalation rules apply).

Verified the risk areas: every ON CONFLICT target has an exactly matching unique index in pkg/schema/postgres (so PG's stricter named-target requirement can't throw); the ?→$n rebinder correctly skips escaped strings, quoted identifiers, $$/tagged dollar quotes, and both comment styles in a single left-to-right pass so ordinals stay aligned with argument order; RETURNING id identity insertion mirrors MySQL's RowsAffected==0 guarded-insert semantics; de-pinning tasks/applyLogs/planComments/settings from the hardwired MySQLDialect is identity-preserving for MySQL; and the PG integration tests genuinely run in CI against postgres:16 (integration job passed, not skipped).

Non-blocking findings:

• Nothing pins timezone=UTC on PG sessions — server-side now() cast into the schema's plain timestamp columns goes through session TimeZone, so lease-expiry/staleness predicates would skew on a non-UTC server, and the postgres:16 container's UTC default means this lane can't catch it. Suggest pinning timezone=UTC in the DSN in the public-constructor follow-up (#1012).
• The PR body's "MySQL behavior is unchanged" slightly overclaims: settings.Set's upsert now stamps updated_at on a same-value write where the old ODKU left the row untouched. Deliberate, documented in-code, and covered — just worth a wording fix.
• Rebind's dollar-quote scanner accepts digit-leading tags ($1$…$1$) that the PG lexer would treat as a parameter; unreachable with the store's ?-only corpus.

See the fencing note on #1011 — the PostgresDialect JoinedUpdate introduced here is the rendering in question.

[Copilot]

Pull request overview

Adds a PostgreSQL dialect “nucleus” to the shared pkg/storage/internal/sqlstore core, implementing PostgreSQL placeholder rebinding, SQL rendering seams, and identity insertion behavior so the same store logic can run against PostgreSQL, plus initial integration parity tests.

Changes:

• Introduces PostgresDialect implementing rebinding (? → $n) and dialect SQL seams (upserts, joined UPDATE, JSON boolean predicate, relative time, etc.).
• Adds PostgreSQL identity insertion via RETURNING id, including guarded-insert semantics.
• Adds first PostgreSQL integration parity lanes for Settings + ApplyLogs and pgx stdlib value contract tests; updates the updated_at stamping lint to account for dialect-rendered JoinedUpdate call sites.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/storage/internal/sqlstore/updated_at_lint_test.go	Extends the source-scan lint to also validate updated_at stamping in dialect-rendered JoinedUpdate call sites.
pkg/storage/internal/sqlstore/storage.go	Ensures all store families receive injected dialect/identity dependencies (removes hardwired MySQLDialect{} in a few stores).
pkg/storage/internal/sqlstore/settings.go	Clarifies updated_at stamping expectations for upserts across dialects.
pkg/storage/internal/sqlstore/postgres_integration_test.go	Adds PostgreSQL integration parity tests (Settings + ApplyLogs) and pgx stdlib value contract assertions.
pkg/storage/internal/sqlstore/identity.go	Adds PostgreSQL identity insertion using RETURNING id, including guarded-insert no-row handling.
pkg/storage/internal/sqlstore/dialect.go	Implements PostgresDialect (rebinding, upserts, joined UPDATE rendering, etc.).
pkg/storage/internal/sqlstore/dialect_test.go	Adds unit tests covering PostgresDialect rebinding and SQL seam renderings.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) AI code review assessment agent (Amp / Claude Opus 4.5)

Summary: All three non-blocking findings are addressed — the dollar-quote tag fix landed on this branch, the UTC session pin is raised as a standalone PR, and the PR body wording is corrected; the fencing note will be answered on #1011.

#	Finding	Status	Explanation
1	No timezone=UTC pinning on PG sessions — server-side now() into plain timestamp columns skews lease-expiry/staleness predicates on a non-UTC server	fixed	#1018 pins timezone=UTC in postgresconn's connection config (single choke point covering Open, OpenReloadable, and the credential-reload path) rather than the #1012 constructor, which takes an already-open *sql.DB. An explicit DSN timezone wins, mirroring sslmode. An integration test proves the session reports UTC even when the database default is overridden to a different timezone.
2	PR body's "MySQL behavior is unchanged" overclaims — settings.Set now stamps updated_at on same-value writes	fixed	PR body updated: it now calls out the one MySQL-visible change (updated_at tracks the last write rather than the last value change, deliberate and covered by tests).
3	Rebind's dollar-quote scanner accepts digit-leading tags ($1$…$1$) that the PG lexer treats as a parameter	fixed	913bd72 — tag validation now follows identifier rules (no leading digit), with unit tests pinning both the digit-leading ($1$ stays a parameter) and underscore-leading ($_1$ still quotes) cases.
—	Fencing note on #1011 (JoinedUpdate rendering introduced here)	deferred	The fencing concern belongs to the #1011 review and needs a design decision (row locking vs. documented acceptance); it will be assessed and answered there.

Verified-correct section (ON CONFLICT targets, rebind ordinal alignment, RETURNING id semantics, identity-preserving de-pinning, CI lane genuinely running): no action — thanks for the confirmation.

Source: #1010 review 4919081588, posted by morgo (automated review on Morgan's behalf).