From ec460103a581d7cee1133dda87f4e220276f7b5d Mon Sep 17 00:00:00 2001 From: Ivan Histand Date: Tue, 16 Jun 2026 19:27:37 -0500 Subject: [PATCH] fix(postgres): auto-name unnamed indexes + release pg client once (1.4.1) #31: a `postgres.indexes` entry without a `name` generated `CREATE [UNIQUE] INDEX "" ...` -> "zero-length delimited identifier", failing table creation. Derive a name (`__idx`, `_key` for unique, truncated to 63 chars) when omitted, mirroring Postgres's own default naming. #32: on any failing statement the pg client was released twice (query/client error handler + the finally block), so pg-pool's throwOnDoubleRelease printed a confusing "Release called on client which has already been released" ahead of the real error. Release exactly once via a guard. Tests: unit test for the derived index name (execution_sql_test); integration tests against real Postgres for the unnamed-index create and for no double-release noise on a failing statement (postgres.spec). Bumps SQLANVIL_VERSION 1.4.0 -> 1.4.1. Closes #31 Closes #32 Co-Authored-By: Claude Opus 4.8 (1M context) --- cli/api/dbadapters/postgres_execution_sql.ts | 15 +++++- cli/api/execution_sql_test.ts | 26 ++++++++++ cli/api/utils/postgres.ts | 40 +++++++++------- tests/integration/postgres.spec.ts | 50 ++++++++++++++++++++ version.bzl | 2 +- 5 files changed, 113 insertions(+), 20 deletions(-) diff --git a/cli/api/dbadapters/postgres_execution_sql.ts b/cli/api/dbadapters/postgres_execution_sql.ts index 8f32f905..2937355f 100644 --- a/cli/api/dbadapters/postgres_execution_sql.ts +++ b/cli/api/dbadapters/postgres_execution_sql.ts @@ -240,10 +240,23 @@ export class PostgresExecutionSql implements IExecutionSql { ? ` include (${index.include.map(c => `"${c}"`).join(", ")})` : ""; const where = index.where ? ` where (${index.where})` : ""; - return `create ${unique}index "${index.name}" on ${target} using ${method} (${columns})${include}${where}`; + const indexName = + index.name || this.defaultIndexName(table.target.name, index.columns, !!index.unique); + return `create ${unique}index "${indexName}" on ${target} using ${method} (${columns})${include}${where}`; }); } + // When an index config omits `name`, derive one from the table + columns + // (mirroring Postgres's own `
__idx` default), truncated to the + // 63-char identifier limit. Without this, an unnamed index emits + // `create index "" ...` -> "zero-length delimited identifier". + private defaultIndexName(tableName: string, columns: string[], unique: boolean): string { + const parts = [tableName, ...(columns || [])].filter(Boolean); + const base = parts.join("_"); + const suffix = unique ? "_key" : "_idx"; + return `${base}${suffix}`.slice(0, 63); + } + private indexMethodAsSql(method?: sqlanvil.PostgresOptions.Index.Method): string { switch (method) { case sqlanvil.PostgresOptions.Index.Method.HASH: diff --git a/cli/api/execution_sql_test.ts b/cli/api/execution_sql_test.ts index 20186c26..1615c61e 100644 --- a/cli/api/execution_sql_test.ts +++ b/cli/api/execution_sql_test.ts @@ -216,6 +216,32 @@ suite("ExecutionSql with Postgres/Supabase", () => { ); }); + test("create index without a name derives one (no zero-length identifier)", () => { + const table: sqlanvil.ITable = { + ...baseTable, + postgres: { + indexes: [ + { columns: ["id"] }, + { columns: ["field1", "id"], unique: true } + ] + } + }; + const statements = executionSql + .publishTasks(table, { fullRefresh: false }) + .build() + .map(t => t.statement); + // Names are derived as
__idx (or _key for unique) -- never "". + expect(statements).to.not.include.members([ + 'create index "" on "my_db"."public"."my_table" using btree ("id")' + ]); + expect(statements[2]).to.equal( + 'create index "my_table_id_idx" on "my_db"."public"."my_table" using btree ("id")' + ); + expect(statements[3]).to.equal( + 'create unique index "my_table_field1_id_key" on "my_db"."public"."my_table" using btree ("field1", "id")' + ); + }); + test("incremental fresh-create emits postgres indexes", () => { const incTable: sqlanvil.ITable = { ...baseTable, diff --git a/cli/api/utils/postgres.ts b/cli/api/utils/postgres.ts index 61334d68..b4d0e1f7 100644 --- a/cli/api/utils/postgres.ts +++ b/cli/api/utils/postgres.ts @@ -65,18 +65,32 @@ export class PgPoolExecutor { }) => Promise ) { const client = await this.pool.connect(); + // The client can be released from several places: the client "error" handler, + // the query "error" handler (which passes the error to release() so the bad + // connection is closed, not returned to the pool), and the finally block. + // Release exactly once -- otherwise pg-pool's throwOnDoubleRelease fires and a + // confusing "Release called on client which has already been released" surfaces + // ahead of the real error on every failing statement. + let released = false; + const releaseOnce = (err?: Error) => { + if (released) { + return; + } + released = true; + try { + client.release(err); + } catch (e) { + // tslint:disable-next-line: no-console + console.error("Error thrown when releasing pg.Client", e.message, e.stack); + } + }; try { client.on("error", err => { // tslint:disable-next-line: no-console console.error("pg.Client client error", err.message, err.stack); // Errored connections cause issues when released back to the pool. Instead, close the connection // by passing the error to release(). https://github.com/dataform-co/dataform/issues/914 - try { - client.release(err); - } catch (e) { - // tslint:disable-next-line: no-console - console.error("Error thrown when releasing errored pg.Client", e.message, e.stack); - } + releaseOnce(err); }); return await callback({ @@ -114,12 +128,7 @@ export class PgPoolExecutor { // Errors don't cause "end" to fire, additionally errored connections // cause issues when released back to the pool. Instead, close the connection // by passing the error to release(). https://github.com/dataform-co/dataform/issues/914 - try { - client.release(err); - } catch (e) { - // tslint:disable-next-line: no-console - console.error("Error thrown when releasing errored pg.Query", e.message, e.stack); - } + releaseOnce(err); reject(err); }); query.on("end", () => { @@ -129,12 +138,7 @@ export class PgPoolExecutor { } }); } finally { - try { - client.release(); - } catch (e) { - // tslint:disable-next-line: no-console - console.error("Error thrown when releasing ended pg.Client", e.message, e.stack); - } + releaseOnce(); } } diff --git a/tests/integration/postgres.spec.ts b/tests/integration/postgres.spec.ts index b02b8901..c6559199 100644 --- a/tests/integration/postgres.spec.ts +++ b/tests/integration/postgres.spec.ts @@ -66,6 +66,56 @@ suite("@sqlanvil/integration/postgres", { parallel: true }, ({ before, after }) ); }); + test("a failing statement rejects with the real error and no double-release noise", async () => { + // Capture console.error so we can assert the pg client isn't released twice + // (which would print "Release called on client which has already been released"). + // tslint:disable-next-line: no-console + const originalError = console.error; + const logged: string[] = []; + // tslint:disable-next-line: no-console + console.error = (...args: any[]) => logged.push(args.join(" ")); + let err: Error | undefined; + try { + await dbadapter.execute("selct 1"); + } catch (e) { + err = e; + } finally { + // tslint:disable-next-line: no-console + console.error = originalError; + } + expect(err, "a bad statement should reject").to.be.an("error"); + expect(err.message.toLowerCase()).to.contain("syntax error"); + expect( + logged.join("\n"), + "the pg client must be released exactly once" + ).to.not.match(/already been released/i); + }); + + test("a table with an unnamed postgres index creates (derived index name)", async () => { + const schema = "sa_integration_test_unnamed_index"; + try { + await dbadapter.execute(`drop schema if exists "${schema}" cascade`); + } catch (e) { + // ignore + } + await dbadapter.execute(`create schema "${schema}"`); + const table: sqlanvil.ITable = { + enumType: sqlanvil.TableType.TABLE, + target: { schema, name: "with_idx" }, + query: "select 1 as id", + postgres: { indexes: [{ columns: ["id"], unique: true }] } + }; + const adapter = new ExecutionSql({ warehouse: "postgres" }, "1.4.8"); + for (const task of adapter.publishTasks(table, { fullRefresh: true }).build()) { + await dbadapter.execute(task.statement); + } + const { rows } = await dbadapter.execute( + `select indexname from pg_indexes where schemaname = '${schema}' and tablename = 'with_idx'` + ); + expect(rows.map((r: any) => r.indexname)).to.include("with_idx_id_key"); + await dbadapter.execute(`drop schema if exists "${schema}" cascade`); + }); + test("run", { timeout: 60000 }, async () => { const compiledGraph = await compile("tests/integration/postgres_project", "project_e2e"); diff --git a/version.bzl b/version.bzl index 06d455c0..e642ecac 100644 --- a/version.bzl +++ b/version.bzl @@ -2,5 +2,5 @@ # SemVer line). DF_VERSION is the upstream dataform-co/dataform release this fork # is synced to — surfaced as metadata (e.g. `sqlanvil --version`), not the package # version. Bump SQLANVIL_VERSION for sqlanvil releases; bump DF_VERSION on upstream syncs. -SQLANVIL_VERSION = "1.4.0" +SQLANVIL_VERSION = "1.4.1" DF_VERSION = "3.0.59"