db: results carry their SQL type, and failures raise (#887, #888)

[InauguralPhysicist]

Two defects in one function, both silent.

#887 — everything was a string. db_query_json emitted every column with eigs_json_escape_string, so SQL false arrived as "f" — a non-empty string, and therefore truthy. The issue's own repro shows if rows[1].is_admin: passing for a non-admin. NULL and '' were both "" with no way to tell them apart, and 9 > 10 held because '9' > '1'.

#888 — every failure looked like an empty table. A syntax error, a missing table, a revoked permission and a dead connection all returned [], which is exactly what a successful query over an empty table returns. A reporting script keeps printing "0 rows" forever after a schema change; a migration that did nothing looks healthy in CI.

What changed

Values carry their column's SQL type through one classifier shared with db_query_value, so the two cannot drift:

SQL type	Arrives as
NULL (any type)	null — checked before the type
boolean	true/false → 1/0
smallint/integer/bigint/oid	number
real/double precision	number (NaN/Infinity → strings; JSON has no literal)
numeric	string — deliberate, see below
everything else	string

The mapping is a function of the column's SQL type alone, never of the row's value. A column that decoded as a number for row 1 and a string for row 100 would break row.n + 1 on data rather than on schema.

Failures raise a catchable io error carrying libpq's own first line. A genuinely empty result is still []/"" and only that. db_connect still reports by return value, so probing for a database needs no try.

Three judgment calls, and why

The filer explicitly left these open.

Straight semantics change, not a second db_query_typed builtin. The current behavior has no defenders once written down, and a parallel builtin would leave the trap installed under the name everyone reaches for first.

numeric stays a string. It is PostgreSQL's arbitrary-precision decimal — the money type — and an EigenScript number is a binary double, which holds neither numeric(38,10) nor 0.1. Silently rounding money is the exact defect class this PR exists to remove, so the digits are preserved and ::float8 is the one-token opt-in. Documented, including the note that avg()/sum(numeric) return numeric and want the cast.

bigint is a number, and one past 2^53 raises. bigint does not get numeric's treatment, because count(*) and sum(integer) return it — leaving it a string would leave the filed bug unfixed for most real numeric queries. For the values a double cannot hold, raising beats rounding: a silently rounded primary key is unrecoverable downstream, and the message names the column and the fix.

Error line 3: db: column 'id' value 9007199254740993 exceeds the exact-integer
range of a number (2^53); select it as text (id::text) to keep the digits

Kind is io, and no new db kind. libpq reports a typo, a permission denial, a missing relation and a reset connection through one channel, so claiming the runtime knows the failure was a bad argument (value) would be a guess. And DIAGNOSTICS.md's closed set classifies by the nature of a failure, not by which subsystem raised it — a db kind would break that scheme. Callers get libpq's text in the message.

Verification

tests/test_db.eigs gained two kinds of coverage:

• No-connection raise checks that run wherever the extension is compiled in. The file was previously inert without a server — every assertion was == "" or == "[]", which passes when nothing works. These now assert the raise, the kind, and the message.
• DB18–DB26 against the live CI postgres service: a genuinely empty result is still [] while a missing table and a syntax error raise with libpq's text; false is falsy in an if; NULL is distinguishable from ''; numbers order numerically; numeric keeps its scale (1.10) while ::float8 is a number; a bigint past 2^53 raises naming the column and ::text, and 2^53 itself is representable.

Local: full suite 3930/3930 on the full variant. The typing half is verified by CI's database job, not locally — this box has no PostgreSQL and no way to install one (no sudo, no docker), so the OID mapping executes for the first time in that job. Flagging that rather than implying I ran it.

Also fixed in passing: a malformed call (db_query_json of 42, db_execute of null, a param list whose first element is not SQL) used to return []/""/"error" — the same shape as a result. Those raise type_mismatch now.

Closes #887
Closes #888

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR fixes two silent failure modes in the PostgreSQL DB extension by (1) preserving SQL types when returning query results (instead of stringifying everything) and (2) raising catchable io errors on DB failures so they are distinguishable from legitimate empty results.

Changes:

• Implement a shared SQL-type classifier for db_query_json and db_query_value, emitting JSON null/booleans/numbers where appropriate and keeping numeric as a string.
• Change DB query/execute helpers to raise on connection/statement failures (rather than returning []/""), and document the behavior in BUILTINS/DIAGNOSTICS/CHANGELOG.
• Expand tests/test_db.eigs with no-connection raise checks and live-DB type/behavior coverage (DB18–DB26).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/test_db.eigs	Adds no-connection raise assertions and live-DB coverage for typed results and error signaling.
src/ext_db.c	Implements typed JSON/value emission and “failures raise” behavior for DB builtins.
docs/DIAGNOSTICS.md	Documents DB failures as io errors with an example message.
docs/BUILTINS.md	Documents raising behavior and the SQL type → EigenScript mapping details.
CHANGELOG.md	Records the breaking semantics change and its rationale.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/ext_db.c:311

• If PQexec/PQexecParams returns NULL (e.g. libpq allocation failure), no error is currently raised and db_execute returns "error" as if it were a normal result, contradicting the new "raises on failure" contract for #888.

    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("error");      /* db_build_query already raised */

src/ext_db.c:332

• If PQexec/PQexecParams returns NULL (e.g. libpq allocation failure), no error is currently raised and db_query_json returns "[]", reintroducing the #888 ambiguity between failure and an empty result set.

    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("[]");         /* db_build_query already raised */

src/ext_db.c:205

• db_check_exact_int uses strtod() to decide whether an int8/int4 value exceeds 2^53, but strtod rounds integers above 2^53 to an adjacent representable double (e.g. 9007199254740993 parses as 9007199254740992). That means the first unrepresentable bigint can slip through and be silently rounded later, defeating the purpose of the raise and likely breaking DB26.

    char *end = NULL;
    double d = strtod(text, &end);
    if (end == text) return 1;            /* not a plain integer literal; leave it */
    if (d > DB_EXACT_INT_MAX || d < -DB_EXACT_INT_MAX) {

src/ext_db.c:259

• If PQexec/PQexecParams returns NULL (e.g. libpq allocation failure), no error is currently raised and the function returns "" even though #888 intends failures to raise. The existing comment assumes db_build_query raised, but NULL can also mean a libpq exec failure without g_has_error being set.

This issue also appears in the following locations of the same file:

• line 310
• line 331

    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("");           /* db_build_query already raised */

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/ext_db.c:193

• DB_OID_NUMERIC is defined but not handled explicitly in db_classify(), relying on the default case to keep numeric as a string. Making the numeric decision explicit improves readability and reduces the risk of someone later “fixing” it into the numeric cases by mistake.

static DbColType db_classify(unsigned int oid) {
    switch (oid) {
        case DB_OID_BOOL:   return DBT_BOOL;
        case DB_OID_INT2:
        case DB_OID_INT4:
        case DB_OID_OID:
        case DB_OID_INT8:
        case DB_OID_FLOAT4:
        case DB_OID_FLOAT8: return DBT_NUM;
        default:            return DBT_STR;   /* numeric, text, date, json, ... */
    }
}

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/ext_db.c:337

• Similar to db_query_value/db_execute: for list-form calls, db_build_query's argument-shape validation runs only after db_require_conn. With no connection, a malformed list like [42] will raise io "not connected" instead of type_mismatch about the call shape. If malformed calls should consistently report programmer error regardless of connection state, validate list args before db_require_conn.

    if (!db_require_conn()) return make_str("[]");
    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("[]");         /* db_build_query already raised */

src/ext_db.c:263

• For list-form calls, the list shape/parameter validation in db_build_query currently happens only after db_require_conn. That means a malformed call like db_query_value of [42] will raise "db: not connected" when no connection is present, instead of the more accurate type_mismatch about the argument shape. Consider validating the list form before checking connectivity so program bugs are reported consistently regardless of environment.

This issue also appears on line 335 of the same file.

    }
    if (!db_require_conn()) return make_str("");
    PGresult *res = db_exec_from_arg(arg);

src/ext_db.c:315

• db_execute validates only the outer arg type before db_require_conn. For list-form calls, a malformed list (e.g., [42]) will currently raise an io "not connected" error when no connection exists, instead of type_mismatch about the list shape/params. If the goal is for malformed calls to raise type_mismatch independent of connection state, validate the list form before db_require_conn.

    if (!db_require_conn()) return make_str("error");
    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("error");      /* db_build_query already raised */

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/ext_db.c:317

• db_exec_from_arg can return NULL if PQexec/PQexecParams fails to allocate a PGresult (e.g., OOM). Here that case returns "error" without raising, which is inconsistent with the new “failures raise” contract (#888) for DB execution failures. Consider raising (unless an earlier validation already set g_has_error).

    if (!db_require_conn()) return make_str("error");
    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("error");      /* db_build_query already raised */

src/ext_db.c:338

• db_exec_from_arg can return NULL if PQexec/PQexecParams fails to allocate a PGresult (e.g., OOM). This branch currently returns "[]" without raising, which would again make that failure indistinguishable from a genuine empty result set (#888). Guard on g_has_error and otherwise raise using PQerrorMessage.

    if (!db_require_conn()) return make_str("[]");
    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("[]");         /* db_build_query already raised */
    if (PQresultStatus(res) != PGRES_TUPLES_OK) {

src/ext_db.c:266

• db_exec_from_arg can return NULL if PQexec/PQexecParams fails to allocate a PGresult (e.g., OOM). In that case this path returns "" without raising, reintroducing the “failure looks like empty result” problem (#888) for that class of failures. Only treat NULL as “already raised” when g_has_error is already set (e.g., by db_build_query).

This issue also appears in the following locations of the same file:

• line 314
• line 335

    if (!db_require_conn()) return make_str("");
    PGresult *res = db_exec_from_arg(arg);
    if (!res) return make_str("");           /* db_build_query already raised */
    if (PQresultStatus(res) != PGRES_TUPLES_OK) {
        db_raise_stmt(res, "query");

src/ext_db.c:214

• The “fix” suggestion in this error message uses %s::text where %s is PQfname (column label). For many queries that label is an alias (e.g. SELECT count(*) AS n), and n::text is not valid SQL in the same SELECT list, so the guidance can be misleading. Suggest a cast to text (::text) without implying the label itself is a castable expression.

        rt_error(EK_VALUE, 0,
                 "db: column '%s' value %s exceeds the exact-integer range of a "
                 "number (2^53); select it as text (%s::text) to keep the digits",
                 colname ? colname : "?", text, colname ? colname : "col");