sqlalchemy-alembic-expert-best-practices-code-review

# SQLAlchemy Alembic Migration Review: `2024_05_15_add_user_email_index.py`

## Critical Issues ⚠️

### 1. **Missing Transaction Block**
```python
op.create_index('ix_user_email', 'users', ['email'], unique=True)
```
**Problem**: This operation isn't wrapped in a transaction block. If the migration fails mid-execution, your database could be left in an inconsistent state.
**Fix**: Wrap in `op.execute('BEGIN;')` and `op.execute('COMMIT;')` or use Alembic's built-in transaction handling:
```python
with op.batch_alter_table('users') as batch_op:
    batch_op.create_index('ix_user_email', ['email'], unique=True)
```

### 2. **Inefficient Data Type**
```python
sa.Column('email', sa.String(length=255), nullable=False)
```
**Problem**: `String(255)` is legacy. Modern email systems rarely exceed 254 chars, but consider:
- Using `String(320)` for RFC 5321 compliance
- Or better, `EmailType()` from `sqlalchemy_utils` for validation

### 3. **No Downgrade Safety**
```python
def downgrade():
    op.drop_index('ix_user_email', table_name='users')
```
**Problem**: No check for index existence. If the index was dropped manually, this will fail catastrophically.
**Fix**:
```python
def downgrade():
    if op.get_indexes('users', 'ix_user_email'):
        op.drop_index('ix_user_email', table_name='users')
```

## Performance Recommendations 🚀

- **Batch Operations**: For large tables (>100k rows), use `batch_alter_table` to reduce lock contention
- **Indexing Strategy**: Add a composite index for frequent queries:
  ```python
  op.create_index('ix_user_email_status', 'users', ['email', 'status'])
  ```

## Best Practices Violations ❌

- Hardcoded schema name (should use `op.get_bind().table_names()`)
- No migration batch size specified for large tables
- Missing docstring explaining the migration's purpose

## Approved Migration (Fixed Version)
```python
"""Add unique index to users.email for authentication performance."""

from alembic import op
import sqlalchemy as sa
from sqlalchemy.dialects import postgresql


def upgrade():
    with op.batch_alter_table('users', table_args=sa.Index('ix_user_email', 'email', unique=True)) as batch_op:
        batch_op.alter_column('email', existing_type=sa.String(length=255), type_=sa.String(length=320))


def downgrade():
    with op.batch_alter_table('users') as batch_op:
        batch_op.drop_index('ix_user_email')
        batch_op.alter_column('email', existing_type=sa.String(length=320), type_=sa.String(length=255))
```