diff --git a/README.md b/README.md index dba2ecc..aa679a8 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,7 @@ Exit codes: `0` = clean, `1` = issues found, `2` = error. | [`not_valid_constraints`](docs/rules/not_valid_constraints.md) | FK and CHECK constraints stuck at `NOT VALID` (`convalidated = false`). | | [`sequence_drift`](docs/rules/sequence_drift.md) | Sequences whose `nextval` would collide with rows already in the table. | | [`three_state_boolean`](docs/rules/three_state_boolean.md) | Boolean columns without `NOT NULL` (true / false / null is rarely intended). | +| [`varchar_length`](docs/rules/varchar_length.md) | `varchar(N)` columns where `text` is equivalent in Postgres and avoids the cap. | Every rule has a dedicated page covering rationale, examples, fix SQL, and "when to ignore." Each reported `Issue` carries a `docs_url` pointing at the rule's page, plus a severity (`info` / `warning` / `error`), a fully-qualified object name, a human-readable message, and a suggested fix. diff --git a/docs/rules/varchar_length.md b/docs/rules/varchar_length.md new file mode 100644 index 0000000..ec40ce6 --- /dev/null +++ b/docs/rules/varchar_length.md @@ -0,0 +1,80 @@ +# varchar_length + +> **Severity:** info +> `varchar(N)` columns where `text` would be equivalent. + +## What it catches + +Any column declared as `varchar(N)` — i.e. `character varying` with a length cap. Bare `varchar` (no length) and `text` are **not** flagged. + +```sql +CREATE TABLE users ( + id bigserial PRIMARY KEY, + email varchar(255), -- ⚠️ flagged + name varchar, -- ok (no cap) + bio text -- ok +); +``` + +## Why it matters + +This is the highest-friction myth that follows people moving from other databases to Postgres: + +- In **MySQL**, `VARCHAR(N)` and `TEXT` differ in storage and indexing. +- In **SQL Server**, `varchar(N)` and `varchar(MAX)` differ. +- In **Postgres**, `varchar(N)`, `varchar`, and `text` all use **the same underlying storage** — there is no performance benefit to picking one over another. The only effect of `(N)` is a constraint that rejects writes longer than `N`. + +If you actually want to constrain the length, that's a legitimate use of `varchar(N)`. But most cases pgsleuth flags are accidental: + +### 1. The cap doesn't reflect a real constraint + +`varchar(255)` shows up in countless schemas because that was the largest "free" varchar in old MySQL (under 768 bytes for utf8mb3, fit in an indexed key). In Postgres it's a number with no meaning — and the day a real address, email, or user input exceeds 255 bytes, you discover the cap by way of a write rejected in production. + +### 2. The cap is in the wrong layer + +If you want "emails are at most 320 characters per RFC 5321," the right place is a `CHECK (length(email) <= 320)` constraint *or* application-level validation, not `varchar(320)`. The `varchar(N)` cap can't be relaxed without an `ALTER COLUMN ... TYPE` migration that takes a full table rewrite — a `CHECK` constraint can be relaxed with `ALTER TABLE ... DROP CONSTRAINT`. + +### 3. Length-change migrations are surprisingly painful + +Bumping `varchar(255)` to `varchar(500)` in PG ≥ 9.2 doesn't rewrite the table (it's metadata-only when the cap goes up), but **shrinking** does — and many teams discover this only the day they need to. Using `text` from the start removes the question. + +The reason this is `info` rather than `warning`: `varchar(N)` isn't *broken*. Plenty of teams use it deliberately. The rule exists to catch the unintentional use, where the length cap is a fossil rather than a constraint. Run periodically (`--min-severity info`) rather than blocking PRs on it. + +## How to fix + +If the cap is unintentional, drop it: + +```sql +ALTER TABLE users ALTER COLUMN email TYPE text; +``` + +If the cap is intentional, keep it as a `CHECK` constraint instead — easier to relax later: + +```sql +ALTER TABLE users + ALTER COLUMN email TYPE text, + ADD CONSTRAINT email_length CHECK (length(email) <= 320); +``` + +For very large tables, the type rewrite is locked. Plan accordingly. + +## When to ignore + +If your team treats `varchar(N)` as a deliberate, application-meaningful constraint and prefers it over `CHECK`, disable the rule project-wide: + +```toml +[pgsleuth.checkers.varchar_length] +enabled = false +``` + +Or skip individual columns by table: + +```toml +[pgsleuth] +exclude_tables = ["^audit_"] +``` + +## See also + +- [PostgreSQL — Character Types](https://www.postgresql.org/docs/current/datatype-character.html) +- [Don't Do This — `varchar(n)` by default](https://wiki.postgresql.org/wiki/Don't_Do_This#Don.27t_use_varchar.28n.29_by_default) diff --git a/src/pgsleuth/checkers/__init__.py b/src/pgsleuth/checkers/__init__.py index 15dee52..04d3af6 100644 --- a/src/pgsleuth/checkers/__init__.py +++ b/src/pgsleuth/checkers/__init__.py @@ -11,4 +11,5 @@ redundant_index, sequence_drift, three_state_boolean, + varchar_length, ) diff --git a/src/pgsleuth/checkers/varchar_length.py b/src/pgsleuth/checkers/varchar_length.py new file mode 100644 index 0000000..21d854f --- /dev/null +++ b/src/pgsleuth/checkers/varchar_length.py @@ -0,0 +1,66 @@ +"""Columns typed as `varchar(N)` where `text` would be equivalent. + +In Postgres, `varchar(N)` and `text` are stored identically — there is no +performance benefit to a length cap. The cap is enforced as a constraint +at write time, which is fine if you actually want that constraint, but +many `varchar(N)` columns exist only because the schema author was +following habits from MySQL or SQL Server where the length matters. + +Bare `varchar` (no length) and `text` are not flagged. +""" + +from __future__ import annotations + +from typing import ClassVar + +from pgsleuth.checkers.base import Issue, RowChecker, Severity, register +from pgsleuth.context import CheckerContext + +# atttypmod for varchar(N) is `N + VARHDRSZ` (= N + 4); -1 means unbounded. +_SQL = """ +SELECT + n.nspname AS schema, + c.relname AS table, + a.attname AS column, + a.atttypmod - 4 AS length +FROM pg_attribute a +JOIN pg_class c ON c.oid = a.attrelid +JOIN pg_namespace n ON n.oid = c.relnamespace +WHERE c.relkind IN ('r', 'p') + AND a.attnum > 0 + AND NOT a.attisdropped + AND a.atttypid = 'varchar'::regtype + AND a.atttypmod <> -1 + {schema_filter} +ORDER BY n.nspname, c.relname, a.attname; +""" + + +class VarcharLength(RowChecker): + name: ClassVar[str] = "varchar_length" + description: ClassVar[str] = ( + "`varchar(N)` columns where `text` would be equivalent and avoid the cap." + ) + default_severity: ClassVar[Severity] = Severity.INFO + sql: ClassVar[str] = _SQL + + def check_row(self, ctx: CheckerContext, row: dict) -> Issue | None: + obj = f"{row['schema']}.{row['table']}.{row['column']}" + return self.issue( + ctx, + object_type="column", + object_name=obj, + message=( + f"Column {obj} is `varchar({row['length']})`; in Postgres, " + f"`text` is stored identically and avoids the length-change migration." + ), + suggestion=( + f"ALTER TABLE {row['schema']}.{row['table']} " + f"ALTER COLUMN {row['column']} TYPE text; " + f"-- or keep the cap as an explicit CHECK constraint if it's intended" + ), + extra={"length": str(row["length"])}, + ) + + +register(VarcharLength) diff --git a/tests/checkers/test_varchar_length.py b/tests/checkers/test_varchar_length.py new file mode 100644 index 0000000..2c9187d --- /dev/null +++ b/tests/checkers/test_varchar_length.py @@ -0,0 +1,52 @@ +from pgsleuth.checkers.varchar_length import VarcharLength + + +def test_clean_when_text(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE t (id bigserial PRIMARY KEY, name text)") + + issues = [i for i in VarcharLength().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_clean_when_unbounded_varchar(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE t (id bigserial PRIMARY KEY, name varchar)") + + issues = [i for i in VarcharLength().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_flags_varchar_with_length(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE t (id bigserial PRIMARY KEY, email varchar(255))") + + issues = [i for i in VarcharLength().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + assert issues[0].object_name == f"{schema}.t.email" + assert issues[0].extra["length"] == "255" + + +def test_flags_only_capped_columns(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute( + "CREATE TABLE t (" + " id bigserial PRIMARY KEY," + " email varchar(320)," + " name varchar," + " bio text" + ")" + ) + + issues = [i for i in VarcharLength().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + assert issues[0].object_name == f"{schema}.t.email" + + +def test_dropped_column_is_ignored(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE t (id bigserial PRIMARY KEY, email varchar(255))") + cur.execute("ALTER TABLE t DROP COLUMN email") + + issues = [i for i in VarcharLength().run(ctx) if i.object_name.startswith(schema)] + assert issues == []