dolthub / dolthub/doltlite

ALTER TABLE ADD COLUMN ... DEFAULT stores NULL on rows whose dropped trailing column was NULL

Closed
#2,902 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
268
Forks
18
Avg merge
2h 27m
Merged PRs (30d)
447

Description

Found in a full-repo review at `0ba280f06f`.

`ALTER TABLE ... ADD COLUMN ... DEFAULT ` reads back NULL instead of the default, on exactly those rows whose previously-dropped **trailing** column was NULL. The wrong value is durable: it commits, survives a merge, and `PRAGMA integrity_check` reports `ok`.

```sql
CREATE TABLE t(k TEXT PRIMARY KEY, a INT, x TEXT);
INSERT INTO t(k,a) VALUES('n1',1); -- x left NULL
ALTER TABLE t DROP COLUMN x;
ALTER TABLE t ADD COLUMN y INT DEFAULT 7;
SELECT y FROM t;
-- EXPECTED (stock 3.54.0): 7
-- GOT: NULL
```

`UPDATE t SET a=a`, `UPDATE t SET y=y`, `VACUUM` and a `dolt_commit` round trip all leave it NULL. Only `REINDEX` or a PK-rewriting update repairs it.

The `NOT NULL` form does not merely return the wrong value, it refuses a legal statement:

```sql
ALTER TABLE t ADD COLUMN y INT NOT NULL DEFAULT 0;
-- EXPECTED: column added, y = 0
-- GOT: Error: constraint failed (column not added)
```

Two rows of the same table disagree, which is what makes it easy to miss:

```sql
CREATE TABLE t(k TEXT PRIMARY KEY, a INT, x TEXT);
INSERT INTO t(k,a) VALUES('n1',1),('n2',2);
UPDATE t SET x='v' WHERE k='n2';
SELECT dolt_commit('-A','-m','base');
ALTER TABLE t DROP COLUMN x; SELECT dolt_commit('-A','-m','drop x');
ALTER TABLE t ADD COLUMN y INT DEFAULT 7; SELECT dolt_commit('-A','-m','add y');
SELECT group_concat(k||'='||coalesce(y,'NULL'),' ') FROM t;
-- GOT: n1=NULL n2=7
-- PRAGMA integrity_check -> ok
-- dolt_at_t at HEAD agrees with the table, and the value survives a subsequent merge.
```

Scope, measured against stock as a control:

| case | result |
| --- | --- |
| clustered TEXT PK, trailing col NULL | **diverges** |
| rowid INTEGER PK, trailing col NULL | **diverges** |
| keyless table, trailing col NULL | **diverges** |
| `NOT NULL DEFAULT` after the drop | **refused** |
| `TEXT DEFAULT 'dflt'` after the drop | **diverges** (NULL) |
| second `ADD COLUMN` after the poisoned one | correct |
| dropped column was not trailing | correct |
| dropped column held a value | correct |
| no `DROP COLUMN` at all | correct |

## Cause

`src/prolly_btree_mutation.c:831-833` (and the sortkey path at `:948-951`) substitute the **old** record bytes whenever `prollyValuesEqual()` reports the new record equal:

```c
rc = prollyValuesEqual(pOld, nOld, pData, nData, &equal);
if( rc!=SQLITE_OK ) return rc;
if( equal ){
pData = pOld;
nData = nOld;
}
```

`diffRecordsEqualFieldwise` (`src/prolly_diff.c:185-187`) pads a *missing* trailing field with serial type 0, i.e. NULL:

```c
nField = aInfo.nField > bInfo.nField ? aInfo.nField : bInfo.nField;
for(i=0; i

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with src/prolly_btree_mutation.c at the record-preservation short-circuits around lines 831-833 and 948-951, then read diffRecordsEqualFieldwise in src/prolly_diff.c. Use the reported DROP COLUMN and ADD COLUMN SQL reproduction as a regression case. Done means the default is returned after the trailing NULL column is dropped while the #2611 implicit-NULL case remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
c, sqlite
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.