From f1546d305631bddaf21bbb62ede3b37214c5391c Mon Sep 17 00:00:00 2001 From: Bauken Baqytuly Date: Sun, 26 Apr 2026 02:02:50 +0500 Subject: [PATCH] Fix DDL bugs in schema, enums, indexes, and triggers tools Five bugs uncovered by smoke-testing pg_manage_schema, pg_manage_indexes and pg_manage_triggers against PostgreSQL 15: 1. create_table / alter_table ignored the `schema` parameter and always wrote to public. Added schema to the input zod schema and qualified identifiers as "schema"."table". 2. create_enum used `$1` placeholders inside CREATE TYPE, which PostgreSQL rejects in DDL ("syntax error at or near $1"). Switched to escaped string literals. 3. create_enum with ifNotExists=true emitted `CREATE TYPE IF NOT EXISTS`, which PostgreSQL does not support. Wrapped the statement in a DO block that checks pg_type/pg_namespace before EXECUTE. 4. get_indexes / analyze_index_usage queried pg_stat_user_indexes using `tablename` and `indexname` columns that do not exist on that view (the actual columns are `relname` / `indexrelname`). Aliased them correctly and qualified WHERE clauses with `psi.`. 5. create_trigger wrapped the entire function name in a single pair of double quotes, so `public.touch_updated_at` became the literal identifier `"public.touch_updated_at"`. Quote each dotted part separately, and default bare names to the trigger's own schema. Verified by re-running the failing scenarios against PostgreSQL 15.17 in an isolated schema. --- src/tools/enums.ts | 35 +++++++--- src/tools/indexes.ts | 94 +++++++++++++------------ src/tools/schema.ts | 159 +++++++++++++++++++++++------------------- src/tools/triggers.ts | 6 +- 4 files changed, 168 insertions(+), 126 deletions(-) diff --git a/src/tools/enums.ts b/src/tools/enums.ts index e97c23a..4ede611 100644 --- a/src/tools/enums.ts +++ b/src/tools/enums.ts @@ -84,17 +84,34 @@ async function executeCreateEnum( const db = DatabaseConnection.getInstance(); try { await db.connect(resolvedConnectionString); - // Manually quote identifiers using double quotes - const qualifiedSchema = `"${schema || 'public'}"`; + const schemaName = schema || 'public'; + const qualifiedSchema = `"${schemaName}"`; const qualifiedEnumName = `"${enumName}"`; const fullEnumName = `${qualifiedSchema}.${qualifiedEnumName}`; - // Use parameterized query for values and add explicit types to map - const valuesPlaceholders = values.map((_: string, i: number) => `$${i + 1}`).join(', '); - const ifNotExistsClause = ifNotExists ? 'IF NOT EXISTS' : ''; - - const query = `CREATE TYPE ${ifNotExistsClause} ${fullEnumName} AS ENUM (${valuesPlaceholders});`; - - await db.query(query, values); + // PostgreSQL doesn't accept parameter placeholders ($1) inside DDL like CREATE TYPE. + // Embed values as escaped string literals instead. + const literalValues = values.map(v => `'${v.replace(/'/g, "''")}'`).join(', '); + const createSql = `CREATE TYPE ${fullEnumName} AS ENUM (${literalValues})`; + + if (ifNotExists) { + // PostgreSQL has no native CREATE TYPE IF NOT EXISTS; emulate via DO block. + const schemaLit = `'${schemaName.replace(/'/g, "''")}'`; + const enumLit = `'${enumName.replace(/'/g, "''")}'`; + const guardedSql = `DO $do$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = ${schemaLit} AND t.typname = ${enumLit} AND t.typtype = 'e' + ) THEN + EXECUTE $sql$${createSql}$sql$; + END IF; +END +$do$;`; + await db.query(guardedSql); + } else { + await db.query(createSql); + } return { schema, enumName, values }; } catch (error) { console.error("Error creating ENUM:", error); diff --git a/src/tools/indexes.ts b/src/tools/indexes.ts index d869ff2..6bf4718 100644 --- a/src/tools/indexes.ts +++ b/src/tools/indexes.ts @@ -49,26 +49,27 @@ async function executeGetIndexes( await db.connect(resolvedConnectionString); if (includeStats) { + // pg_stat_user_indexes exposes table/index names as relname/indexrelname (not tablename/indexname). const statsQuery = ` - SELECT - schemaname, - tablename, - indexname, - idx_scan as scans, - idx_tup_read as tuples_read, - idx_tup_fetch as tuples_fetched, - pg_relation_size(indexrelname::regclass) as size_bytes, - pg_size_pretty(pg_relation_size(indexrelname::regclass)) as size_pretty, - indisunique as is_unique, - indisprimary as is_primary, - CASE - WHEN idx_scan = 0 THEN 0 - ELSE round((idx_tup_fetch::numeric / idx_tup_read::numeric) * 100, 2) - END as usage_ratio + SELECT + psi.schemaname, + psi.relname AS tablename, + psi.indexrelname AS indexname, + psi.idx_scan AS scans, + psi.idx_tup_read AS tuples_read, + psi.idx_tup_fetch AS tuples_fetched, + pg_relation_size(psi.indexrelid) AS size_bytes, + pg_size_pretty(pg_relation_size(psi.indexrelid)) AS size_pretty, + pi.indisunique AS is_unique, + pi.indisprimary AS is_primary, + CASE + WHEN psi.idx_scan = 0 THEN 0 + ELSE round((psi.idx_tup_fetch::numeric / NULLIF(psi.idx_tup_read::numeric, 0)) * 100, 2) + END AS usage_ratio FROM pg_stat_user_indexes psi JOIN pg_index pi ON psi.indexrelid = pi.indexrelid - WHERE schemaname = $1 - ${tableName ? 'AND tablename = $2' : ''} + WHERE psi.schemaname = $1 + ${tableName ? 'AND psi.relname = $2' : ''} ORDER BY size_bytes DESC, scans DESC `; @@ -359,28 +360,29 @@ async function executeAnalyzeIndexUsage( await db.connect(resolvedConnectionString); // Get all index usage stats + // pg_stat_user_indexes exposes table/index names as relname/indexrelname (not tablename/indexname). const usageQuery = ` - SELECT - schemaname, - tablename, - indexname, - idx_scan as scans, - idx_tup_read as tuples_read, - idx_tup_fetch as tuples_fetched, - pg_relation_size(indexrelname::regclass) as size_bytes, - pg_size_pretty(pg_relation_size(indexrelname::regclass)) as size_pretty, - indisunique as is_unique, - indisprimary as is_primary, - CASE - WHEN idx_scan = 0 THEN 0 - ELSE round((idx_tup_fetch::numeric / NULLIF(idx_tup_read::numeric, 0)) * 100, 2) - END as usage_ratio + SELECT + psi.schemaname, + psi.relname AS tablename, + psi.indexrelname AS indexname, + psi.idx_scan AS scans, + psi.idx_tup_read AS tuples_read, + psi.idx_tup_fetch AS tuples_fetched, + pg_relation_size(psi.indexrelid) AS size_bytes, + pg_size_pretty(pg_relation_size(psi.indexrelid)) AS size_pretty, + pi.indisunique AS is_unique, + pi.indisprimary AS is_primary, + CASE + WHEN psi.idx_scan = 0 THEN 0 + ELSE round((psi.idx_tup_fetch::numeric / NULLIF(psi.idx_tup_read::numeric, 0)) * 100, 2) + END AS usage_ratio FROM pg_stat_user_indexes psi JOIN pg_index pi ON psi.indexrelid = pi.indexrelid - WHERE schemaname = $1 - ${tableName ? 'AND tablename = $2' : ''} - AND pg_relation_size(indexrelname::regclass) >= $${tableName ? '3' : '2'} - AND NOT indisprimary -- Exclude primary key indexes from analysis + WHERE psi.schemaname = $1 + ${tableName ? 'AND psi.relname = $2' : ''} + AND pg_relation_size(psi.indexrelid) >= $${tableName ? '3' : '2'} + AND NOT pi.indisprimary ORDER BY size_bytes DESC `; @@ -395,18 +397,18 @@ async function executeAnalyzeIndexUsage( if (showDuplicates) { // Find potentially duplicate indexes by comparing column definitions const duplicateQuery = ` - SELECT - schemaname, - tablename, - array_agg(indexname) as index_names, - string_agg(pg_get_indexdef(indexrelid), ' | ') as definitions, - count(*) as index_count + SELECT + psi.schemaname, + psi.relname AS tablename, + array_agg(psi.indexrelname) AS index_names, + string_agg(pg_get_indexdef(psi.indexrelid), ' | ') AS definitions, + count(*) AS index_count FROM pg_stat_user_indexes psi JOIN pg_index pi ON psi.indexrelid = pi.indexrelid - WHERE schemaname = $1 - ${tableName ? 'AND tablename = $2' : ''} - AND NOT indisprimary - GROUP BY schemaname, tablename, indkey + WHERE psi.schemaname = $1 + ${tableName ? 'AND psi.relname = $2' : ''} + AND NOT pi.indisprimary + GROUP BY psi.schemaname, psi.relname, pi.indkey HAVING count(*) > 1 `; diff --git a/src/tools/schema.ts b/src/tools/schema.ts index 472ac80..682afad 100644 --- a/src/tools/schema.ts +++ b/src/tools/schema.ts @@ -46,6 +46,7 @@ interface EnumInfo { const GetSchemaInfoInputSchema = z.object({ connectionString: z.string().optional(), tableName: z.string().optional().describe("Optional table name to get detailed schema for"), + schema: z.string().optional().default('public').describe("Schema name (defaults to public)"), }); type GetSchemaInfoInput = z.infer; @@ -55,20 +56,21 @@ async function executeGetSchemaInfo( ): Promise { // Return type depends on whether tableName is provided const resolvedConnectionString = getConnectionString(input.connectionString); const db = DatabaseConnection.getInstance(); - const { tableName } = input; - + const { tableName, schema = 'public' } = input; + try { await db.connect(resolvedConnectionString); - + if (tableName) { - return await getTableInfo(db, tableName); + return await getTableInfo(db, tableName, schema); } - + const tables = await db.query<{ table_name: string }>( - `SELECT table_name - FROM information_schema.tables - WHERE table_schema = 'public' AND table_type = 'BASE TABLE' -- Ensure only base tables - ORDER BY table_name` + `SELECT table_name + FROM information_schema.tables + WHERE table_schema = $1 AND table_type = 'BASE TABLE' + ORDER BY table_name`, + [schema] ); return tables.map(t => t.table_name); @@ -90,9 +92,10 @@ export const getSchemaInfoTool: PostgresTool = { } try { const result = await executeGetSchemaInfo(validationResult.data, getConnectionString); - const message = validationResult.data.tableName - ? `Schema information for table ${validationResult.data.tableName}` - : 'List of tables in database'; + const sch = validationResult.data.schema ?? 'public'; + const message = validationResult.data.tableName + ? `Schema information for table ${sch}.${validationResult.data.tableName}` + : `List of tables in schema ${sch}`; return { content: [{ type: 'text', text: message }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } catch (error) { const errorMessage = error instanceof McpError ? error.message : (error instanceof Error ? error.message : String(error)); @@ -114,39 +117,34 @@ const CreateTableInputSchema = z.object({ connectionString: z.string().optional(), tableName: z.string(), columns: z.array(CreateTableColumnSchema).min(1), - // primaryKeyColumns: z.array(z.string()).optional(), // Alternative for PKs + schema: z.string().optional().default('public').describe("Schema name (defaults to public)"), }); type CreateTableInput = z.infer; async function executeCreateTable( input: CreateTableInput, getConnectionString: GetConnectionStringFn -): Promise<{ tableName: string; columns: z.infer[] }> { +): Promise<{ tableName: string; schema: string; columns: z.infer[] }> { const resolvedConnectionString = getConnectionString(input.connectionString); const db = DatabaseConnection.getInstance(); - const { tableName, columns } = input; + const { tableName, columns, schema = 'public' } = input; try { await db.connect(resolvedConnectionString); - + const columnDefs = columns.map(col => { let def = `"${col.name}" ${col.type}`; if (col.nullable === false) def += ' NOT NULL'; if (col.default !== undefined) def += ` DEFAULT ${col.default}`; - // if (col.primaryKey) def += ' PRIMARY KEY'; // If using column-level PK return def; }).join(', '); - - // const primaryKeyDef = input.primaryKeyColumns && input.primaryKeyColumns.length > 0 - // ? `, PRIMARY KEY (${input.primaryKeyColumns.map(pk => `"${pk}"`).join(', ')})` - // : ''; - // const createTableSQL = `CREATE TABLE IF NOT EXISTS "${tableName}" (${columnDefs}${primaryKeyDef})`; - const createTableSQL = `CREATE TABLE IF NOT EXISTS "${tableName}" (${columnDefs})`; - + const qualifiedTable = `"${schema}"."${tableName}"`; + const createTableSQL = `CREATE TABLE IF NOT EXISTS ${qualifiedTable} (${columnDefs})`; + await db.query(createTableSQL); - - return { tableName, columns }; + + return { tableName, schema, columns }; } catch (error) { throw new McpError(ErrorCode.InternalError, `Failed to create table: ${error instanceof Error ? error.message : String(error)}`); } finally { @@ -165,7 +163,7 @@ export const createTableTool: PostgresTool = { } try { const result = await executeCreateTable(validationResult.data, getConnectionString); - return { content: [{ type: 'text', text: `Table ${result.tableName} created successfully (if not exists).` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; + return { content: [{ type: 'text', text: `Table ${result.schema}.${result.tableName} created successfully (if not exists).` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } catch (error) { const errorMessage = error instanceof McpError ? error.message : (error instanceof Error ? error.message : String(error)); return { content: [{ type: 'text', text: `Error creating table: ${errorMessage}` }], isError: true }; @@ -186,38 +184,37 @@ const AlterTableInputSchema = z.object({ connectionString: z.string().optional(), tableName: z.string(), operations: z.array(AlterTableOperationSchema).min(1), + schema: z.string().optional().default('public').describe("Schema name (defaults to public)"), }); type AlterTableInput = z.infer; async function executeAlterTable( input: AlterTableInput, getConnectionString: GetConnectionStringFn -): Promise<{ tableName: string; operations: z.infer[] }> { +): Promise<{ tableName: string; schema: string; operations: z.infer[] }> { const resolvedConnectionString = getConnectionString(input.connectionString); const db = DatabaseConnection.getInstance(); - const { tableName, operations } = input; + const { tableName, operations, schema = 'public' } = input; + const qualifiedTable = `"${schema}"."${tableName}"`; try { await db.connect(resolvedConnectionString); - + await db.transaction(async (client: PoolClient) => { for (const op of operations) { let sql = ''; const colNameQuoted = `"${op.columnName}"`; - + switch (op.type) { case 'add': if (!op.dataType) throw new Error('Data type is required for ADD operation'); - sql = `ALTER TABLE "${tableName}" ADD COLUMN ${colNameQuoted} ${op.dataType}`; + sql = `ALTER TABLE ${qualifiedTable} ADD COLUMN ${colNameQuoted} ${op.dataType}`; if (op.nullable === false) sql += ' NOT NULL'; if (op.default !== undefined) sql += ` DEFAULT ${op.default}`; break; - + case 'alter': { - // PostgreSQL requires separate ALTER COLUMN clauses for different alterations - // This simplified version might need to be split into multiple statements for complex alters - // Or use a more specific action like 'set data type', 'set default', 'set not null' etc. - sql = `ALTER TABLE "${tableName}" ALTER COLUMN ${colNameQuoted}`; + sql = `ALTER TABLE ${qualifiedTable} ALTER COLUMN ${colNameQuoted}`; const alterActions: string[] = []; if (op.dataType) alterActions.push(`TYPE ${op.dataType}`); // May need USING clause for some type changes if (op.nullable !== undefined) { @@ -242,7 +239,7 @@ async function executeAlterTable( } case 'drop': - sql = `ALTER TABLE "${tableName}" DROP COLUMN ${colNameQuoted}`; + sql = `ALTER TABLE ${qualifiedTable} DROP COLUMN ${colNameQuoted}`; break; } if (sql) { // Ensure sql is not empty, e.g. if alterActions was empty @@ -250,8 +247,8 @@ async function executeAlterTable( } } }); - - return { tableName, operations }; + + return { tableName, schema, operations }; } catch (error) { throw new McpError(ErrorCode.InternalError, `Failed to alter table: ${error instanceof Error ? error.message : String(error)}`); } finally { @@ -270,7 +267,7 @@ export const alterTableTool: PostgresTool = { } try { const result = await executeAlterTable(validationResult.data, getConnectionString); - return { content: [{ type: 'text', text: `Table ${result.tableName} altered successfully.` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; + return { content: [{ type: 'text', text: `Table ${result.schema}.${result.tableName} altered successfully.` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } catch (error) { const errorMessage = error instanceof McpError ? error.message : (error instanceof Error ? error.message : String(error)); return { content: [{ type: 'text', text: `Error altering table: ${errorMessage}` }], isError: true }; @@ -281,7 +278,7 @@ export const alterTableTool: PostgresTool = { /** * Get detailed information about a specific table */ -async function getTableInfo(db: DatabaseConnection, tableName: string): Promise { +async function getTableInfo(db: DatabaseConnection, tableName: string, schema = 'public'): Promise { // Get column information const columns = await db.query<{ column_name: string; @@ -291,11 +288,11 @@ async function getTableInfo(db: DatabaseConnection, tableName: string): Promise< }>( `SELECT column_name, data_type, is_nullable, column_default FROM information_schema.columns - WHERE table_schema = 'public' AND table_name = $1 + WHERE table_schema = $1 AND table_name = $2 ORDER BY ordinal_position`, - [tableName] + [schema, tableName] ); - + // Get constraint information const constraints = await db.query<{ constraint_name: string; @@ -315,10 +312,10 @@ async function getTableInfo(db: DatabaseConnection, tableName: string): Promise< FROM pg_constraint c JOIN pg_namespace n ON n.oid = c.connamespace JOIN pg_class cl ON cl.oid = c.conrelid - WHERE n.nspname = 'public' AND cl.relname = $1`, - [tableName] + WHERE n.nspname = $1 AND cl.relname = $2`, + [schema, tableName] ); - + // Get index information const indexes = await db.query<{ indexname: string; @@ -331,8 +328,8 @@ async function getTableInfo(db: DatabaseConnection, tableName: string): Promise< JOIN pg_class c ON c.oid = x.indrelid JOIN pg_class i ON i.oid = x.indexrelid JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE c.relkind = 'r' AND n.nspname = 'public' AND c.relname = $1`, - [tableName] + WHERE c.relkind = 'r' AND n.nspname = $1 AND c.relname = $2`, + [schema, tableName] ); return { @@ -410,12 +407,30 @@ async function executeCreateEnumInSchema( const qualifiedSchema = `"${schema}"`; const qualifiedEnumName = `"${enumName}"`; const fullEnumName = `${qualifiedSchema}.${qualifiedEnumName}`; - const valuesPlaceholders = values.map((_: string, i: number) => `$${i + 1}`).join(', '); - const ifNotExistsClause = ifNotExists ? 'IF NOT EXISTS' : ''; - - const query = `CREATE TYPE ${ifNotExistsClause} ${fullEnumName} AS ENUM (${valuesPlaceholders});`; - - await db.query(query, values); + // PostgreSQL doesn't accept parameter placeholders ($1) inside DDL like CREATE TYPE. + // Embed values as escaped string literals instead. + const literalValues = values.map(v => `'${v.replace(/'/g, "''")}'`).join(', '); + const createSql = `CREATE TYPE ${fullEnumName} AS ENUM (${literalValues})`; + + if (ifNotExists) { + // PostgreSQL has no native CREATE TYPE IF NOT EXISTS; emulate via DO block. + const schemaLit = `'${schema.replace(/'/g, "''")}'`; + const enumLit = `'${enumName.replace(/'/g, "''")}'`; + const guardedSql = `DO $do$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE n.nspname = ${schemaLit} AND t.typname = ${enumLit} AND t.typtype = 'e' + ) THEN + EXECUTE $sql$${createSql}$sql$; + END IF; +END +$do$;`; + await db.query(guardedSql); + } else { + await db.query(createSql); + } return { schema, enumName, values }; } catch (error) { throw new McpError(ErrorCode.InternalError, `Failed to create ENUM ${enumName}: ${error instanceof Error ? error.message : String(error)}`); @@ -498,42 +513,46 @@ export const manageSchemaTools: PostgresTool = { case 'get_info': { const result = await executeGetSchemaInfo({ connectionString: connStringArg, - tableName + tableName, + schema: schema || 'public' }, getConnectionStringVal); - const message = tableName - ? `Schema information for table ${tableName}` - : 'List of tables in database'; + const fqTable = tableName ? `${schema || 'public'}.${tableName}` : null; + const message = fqTable + ? `Schema information for table ${fqTable}` + : `List of tables in schema ${schema || 'public'}`; return { content: [{ type: 'text', text: message }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } case 'create_table': { if (!tableName || !columns || columns.length === 0) { - return { - content: [{ type: 'text', text: 'Error: tableName and columns are required for create_table operation' }], - isError: true + return { + content: [{ type: 'text', text: 'Error: tableName and columns are required for create_table operation' }], + isError: true }; } const result = await executeCreateTable({ connectionString: connStringArg, tableName, - columns + columns, + schema: schema || 'public' }, getConnectionStringVal); - return { content: [{ type: 'text', text: `Table ${result.tableName} created successfully (if not exists).` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; + return { content: [{ type: 'text', text: `Table ${result.schema}.${result.tableName} created successfully (if not exists).` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } case 'alter_table': { if (!tableName || !operations || operations.length === 0) { - return { - content: [{ type: 'text', text: 'Error: tableName and operations are required for alter_table operation' }], - isError: true + return { + content: [{ type: 'text', text: 'Error: tableName and operations are required for alter_table operation' }], + isError: true }; } const result = await executeAlterTable({ connectionString: connStringArg, tableName, - operations + operations, + schema: schema || 'public' }, getConnectionStringVal); - return { content: [{ type: 'text', text: `Table ${result.tableName} altered successfully.` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; + return { content: [{ type: 'text', text: `Table ${result.schema}.${result.tableName} altered successfully.` }, { type: 'text', text: JSON.stringify(result, null, 2) }] }; } case 'get_enums': { diff --git a/src/tools/triggers.ts b/src/tools/triggers.ts index b3b1ae3..d91af96 100644 --- a/src/tools/triggers.ts +++ b/src/tools/triggers.ts @@ -382,7 +382,11 @@ async function executeCreateTrigger( const createOrReplace = replace ? 'CREATE OR REPLACE' : 'CREATE'; const qualifiedTableName = `"${schema}"."${tableName}"`; - const qualifiedFunctionName = `"${functionName}"`; // Assuming functionName might also need quoting or schema qualification + // Support both bare ("touch_updated_at") and schema-qualified ("public.touch_updated_at") function names. + // Quote each identifier part separately so PostgreSQL resolves it correctly. + const qualifiedFunctionName = functionName.includes('.') + ? functionName.split('.').map(p => `"${p}"`).join('.') + : `"${schema}"."${functionName}"`; let sql = ` ${createOrReplace} TRIGGER "${triggerName}"