MemberJunction / MemberJunction/MJ

SQLConverter: dedup mutates committed .pg.sql files; CTE-DELETE + sys.indexes-guard transpile gaps

Open
#3,166 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary
Three `@memberjunction/sql-converter` (`mj migrate convert`) defects surfaced during the v5.48 PG conversion. The `/pg-migrate-experimental` runbook now *defends* against all three (see #3165), but the root fixes belong in the converter.

### 1. The dedup pass mutates COMMITTED `.pg.sql` files (highest priority)
Every `mj migrate convert` run ends with a global "EntityField Sequence deduplication" pass that rewrites `Sequence` values **inside already-committed `.pg.sql` files**, in place (prints `Fixed N sequence collision(s)` naming each file). Committed PG migrations are a deployed, immutable historical ledger — the converter must never touch them.

**Repro (v5.48):** converting `V202607142047__v5.48.x__Metadata_Sync.sql` reported 5 collisions and modified `V202604241700…`, `V202605190912…`, `V202605251204…`, `V202606051200…`, `V202606121723…` (5.30–5.41). The fresh-DB gate then applied cleanly *without* those edits — i.e. the dedup was spurious.

**Ask:** the dedup should only ever adjust sequences in the NEW migration being generated, never rewrite prior committed files. If a genuine cross-migration collision exists, fail loudly and point at the new migration.

### 2. CTE-targeted DELETE transpiles to invalid PG
`;WITH cte AS (...) DELETE FROM cte WHERE ...` (valid T-SQL) transpiles to `DELETE FROM "cte" ...`, which PG rejects (you can't DELETE from a CTE). Correct PG form is `DELETE FROM t USING () d WHERE t."ID" = d."ID" AND d.rn > 1`. Seen on `ListDetail_Index_Optimization` — had to hand-author the whole `.pg.sql`.

### 3. `IF EXISTS (SELECT 1 FROM sys.indexes …) BEGIN DROP INDEX … END` is dropped as an unhandled gap
The T-SQL idempotent-index-drop pattern lands as a `.needs-hand`/gap comment. The PG equivalent is trivial and could be templated: `DROP INDEX IF EXISTS __mj."IndexName";` (and `CREATE UNIQUE INDEX IF NOT EXISTS …`).

## Context
Discovered during the v5.48.0 release PG conversion. Runbook mitigations landed in #3165; this issue tracks the underlying `packages/SQLConverter` fixes (each warrants its own PR + regression test).

Contributor guide

Open the contributing guide

Research direction

Start in packages/SQLConverter and reproduce the defects with mj migrate convert, using the /pg-migrate-experimental runbook and #3165 for the affected scenarios. Trace the deduplication pass and the CTE-DELETE and sys.indexes patterns, then add regression tests showing that only the new migration changes and that generated PostgreSQL is valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
postgresql, sql
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.