fix(postgresql): no ALTER TABLE on the createIfNotExist=false path

[aiyili]

Fixes #3213

Problem

PostgresAgentStateStore ran ensureVersionColumn() after the createIfNotExist branch, so the ALTER TABLE ... ADD COLUMN IF NOT EXISTS version ... DDL executed even with createIfNotExist=false. PostgreSQL checks table ownership before honoring ADD COLUMN IF NOT EXISTS, so DML-only accounts fail with ERROR: must be owner of table ... (SQLSTATE 42501) on every startup, even when the column already exists — the least-privilege deployment createIfNotExist(false) advertises is not actually deliverable.

Fix (items 1, 2 and 4 of the evaluation above; item 3 — the autoMigrateSchema opt-in flag — left for maintainers to decide, as noted)

1. The createIfNotExist=false path now ends in a read-only verifyVersionColumnExists(), mirroring the INFORMATION_SCHEMA check MysqlAgentStateStore already performs. When the version column is missing it fails fast with an IllegalStateException that quotes the exact migration DDL to run.
2. createSchemaIfNotExist() / createTableIfNotExist() / ensureVersionColumn() now stay together under createIfNotExist=true. The DEFAULT 1 backfill value from fix(state): resolve phantom CAS conflicts on version-0 migrated rows … #3165 is preserved on that migration path. PostgresDistributedStore (which uses createIfNotExist=true) is unaffected.
3. The migration DDL string is shared between ensureVersionColumn() and the verification failure message (single versionColumnMigrationDdl() source), so the operator instructions can never drift from what the auto-path runs.

Tests

• constructorWithoutAutoCreateNeverIssuesAlter — on the false path, prepareStatement(startsWith("ALTER")) is never invoked. This closes the gap called out in the evaluation: the existing test asserted createStatement() is never called, which the prepareStatement-based DDL bypassed.
• verifyVersionColumnMissingThrowsWithMigrationDdl — schema exists, table exists, version column missing → IllegalStateException whose message contains the full ADD COLUMN IF NOT EXISTS version BIGINT NOT NULL DEFAULT 1 DDL.
• autoCreateStillEnsuresVersionColumn — the true path still issues exactly one ALTER TABLE.
• Module suite: 151/151 green (BUILD SUCCESS), PostgresAgentStateStoreTest 67/67.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Fixes #3213 by keeping ensureVersionColumn() inside the createIfNotExist=true branch and replacing it on the least-privilege path with a read-only verifyVersionColumnExists(). The root cause is real and correctly diagnosed: PostgreSQL checks table ownership before honoring ADD COLUMN IF NOT EXISTS, so an unconditional ALTER fails with SQLSTATE 42501 for DML-only accounts even when the column already exists. The structure, the shared versionColumnMigrationDdl() helper, the fail-fast message, and the regression tests (especially constructorWithoutAutoCreateNeverIssuesAlter, which closes the gap left by the old createStatement()-only assertion) are all sound. CI is green on this head and CLA is signed. No blockers — the four notes below are two docs/consistency asks and two refinements, all non-blocking.

Findings

• [Warning] PostgresAgentStateStore.java:146 — the shared DDL uses DEFAULT 1, but createTableIfNotExist() (line 216) still declares version BIGINT NOT NULL DEFAULT 0, and insertItems()/saveHash() insert without an explicit version, so freshly auto-created tables rest rows at the 0 "row absent" sentinel. Pre-existing, but the new "single source" javadoc implies a consistency the CREATE path does not have.
• [Warning] PostgresAgentStateStore.java:170 — user-visible behavior change: createIfNotExist(false) deployments whose table predates the column and whose account can do DDL used to self-heal at startup and now throw from the constructor. Please add a migration note to the postgresql-repository docs (docs/v2/en + docs/v2/zh) and the changelog. Blast radius is limited but not zero: new PostgresAgentStateStore(dataSource) defaults to false.
• [Info] PostgresAgentStateStore.java:174 — the predicate checks existence only; a version column with the wrong type/nullability passes and misbehaves later in the CAS paths. Also note MysqlAgentStateStore still auto-migrates on the same flag, so the two stores now give createIfNotExist(false) different meanings.
• [Info] PostgresAgentStateStoreTest.java:160 — the thenReturn(true, true, false) stubbing couples the test to constructor check order; keying the mock on the INFORMATION_SCHEMA.COLUMNS SQL keeps it stable if a fourth read-only check is ever added.

On the deferred item

Leaving item 3 of the evaluation (an autoMigrateSchema opt-in) to maintainers is the right split — that is a configuration-surface decision, not a bug fix, and this PR stays scoped without smuggling it in.

Cross-repo Note

agentscope-extensions-mysql (MysqlAgentStateStore) and agentscope-extensions-jdbc are the other state stores with a version-column migration. If maintainers settle on fail-fast as the contract for createIfNotExist(false), that decision should be mirrored there so the flag means one thing across stores; a follow-up issue would be cheaper for the next contributor than discovering it from a startup exception.

Thanks for the thorough write-up and for including the "why DEFAULT 1, not 0" reasoning — the PR body made this quick to review.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] This shared DDL is now quoted to operators as the migration statement, but it disagrees with what the auto-create path actually builds: createTableIfNotExist() declares version BIGINT NOT NULL DEFAULT 0 (line 216), while this ALTER uses DEFAULT 1. The consequence is real rather than cosmetic — insertItems() / saveHash() insert without an explicit version, so a freshly auto-created table defaults rows to 0, which is exactly the "row absent" sentinel that readVersion() and saveIfVersion(..., 0) interpret as missing (the failure mode the ensureVersionColumn() comment describes). Pre-existing, not introduced here, but since this PR makes the two statements share one source, could you either align CREATE TABLE to DEFAULT 1 or soften the javadoc claim that this is "the exact statement the operator must apply"? A follow-up is fine — just don't want the shared helper to imply a consistency that the CREATE path does not have.

[oss-maintainer]

[Warning] The fail-fast is the right call, but it is a user-visible behavior change worth documenting: any deployment on createIfNotExist(false) whose table predates the version column and whose account does hold DDL rights used to self-heal at startup and now throws IllegalStateException from the constructor. Blast radius is limited (the builder defaults to createIfNotExist = true, and PostgresDistributedStore passes true), but new PostgresAgentStateStore(dataSource) defaults to false. Two asks: (1) add a migration note to the postgresql-repository docs page under both docs/v2/en and docs/v2/zh; (2) mention it in the changelog/release notes so operators do not meet it as a surprise startup exception after an upgrade.

[oss-maintainer]

[Info] Two small notes on the predicate itself.

1. It checks existence only. A pre-existing version column of the wrong type, or nullable, or DEFAULT 0, passes verification and then misbehaves at runtime in the CAS paths. Adding AND DATA_TYPE = 'bigint' (and optionally IS_NULLABLE = 'NO') to the same query is free and turns "column exists" into "column is the one this code expects" — with the same fail-fast message.
2. This diverges from MysqlAgentStateStore, which still runs a check-then-ALTER (ensureVersionColumn() at its line 176-208) on the createIfNotExist=false path and therefore keeps auto-migrating. Same flag, two different contracts across the two state stores. Worth a line in the javadoc, or a follow-up issue to make MySQL fail fast too.

[oss-maintainer]

[Info] Good set of regression tests — constructorWithoutAutoCreateNeverIssuesAlter in particular closes the gap where the old assertion only checked createStatement() and the DDL went out through prepareStatement.

The stubbing is positional, though: thenReturn(true, true, false) asserts "the third ResultSet.next() of the constructor is the version check", which couples the test to check order rather than to the query. If a fourth read-only verification is added later (or the order changes), this test silently stops exercising the branch it names. Selecting on the SQL — e.g. stubbing connection.prepareStatement(contains("INFORMATION_SCHEMA.COLUMNS")) to its own ResultSet — keeps the intent intact without depending on call order. Non-blocking.

[aiyili]

Added two tests covering both previously-uncovered catch (SQLException) branches (verifyVersionColumnExists on the verify path, ensureVersionColumn on the auto-create path). Local JaCoCo now reports 0 missed lines for all three changed methods; module suite 69/69 green. Patch coverage should be 100% on the next CI run.

[oss-maintainer]

Summary

Re-review after the new commit 5aa82149 (test(postgresql): 补齐 verifyVersionColumnExists / ensureVersionColumn 异常分支覆盖). The increment is test-only — the previously approved production changes (versionColumnMigrationDdl() + read-only verifyVersionColumnExists() on the createIfNotExist=false path) are untouched — and it adds exactly the exception-branch coverage asked for: missing-column fail-fast quoting the migration DDL, SQLException from the verification query, and ALTER failure on the auto-create path. CLA signed. LGTM.

Findings

• [Info] PostgresAgentStateStoreTest.java — the new tests correctly re-stub the shared preparedStatement/resultSet mocks per test; just note they now depend on the construction-time call ordering of createSchemaIfNotExist → createTableIfNotExist → ensureVersionColumn (and schema → table → column on the verify path). If those calls are ever reordered or an extra check is added, these stub sequences will fail loudly rather than silently — acceptable, but worth knowing when touching the constructor.
• [Info] The pre-existing warnings from the previous round (DDL backtick-quoting drift for mixed-case identifiers, documenting the fail-fast behavior change, type/nullability of a pre-existing version column) remain open — non-blocking, maintainer's call.

Running the module suite on CI; local re-execution was skipped to stay within the review budget.

---

Automated review by github-manager-bot