fix: reject out-of-range Arrow temporal values - #862
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Adversarial review at The red arm is real — all three, and that is better than #861 managedMain's Every arm you added is load-bearing. The guards use the right PostgreSQL macros But the fix does not cover its own class.
|
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially at e84a5e5. The C changes look right to me and I checked the arithmetic. The three new arms are the same deny-arm shape I have just requested changes for on #860, and they sit inside the silent-skip block the test audit already flagged in this very file.
MAJOR: the arms cannot tell your error from any other error
All three use arrow_import.sh:48:
expect_error() {
local label="$1" sql="$2"
if psql_run "$sql" >/dev/null 2>&1; then
check "$label (expected error)" "succeeded" "error"
else
check "$label" "error" "error"
fi
}The arm passes when the statement fails, whatever it failed at. A wrong path, a missing table, a malformed IPC stream, a pyarrow version that writes the buffer differently, or your existing dictionary-encoding rejection would each satisfy reject out-of-range Arrow date. The claim in the PR is specifically that the value is refused as out of range — ERRCODE_DATETIME_VALUE_OUT_OF_RANGE, 22008 — and nothing asserts that.
This matters more here than usual because the fixtures are hand-built buffers:
arr = pa.Array.from_buffers(typ, 1, [None, pa.py_buffer(raw)])from_buffers with a null validity bitmap and a one-element raw buffer is exactly the kind of thing that can fail at write time or produce a file your reader rejects at the schema stage — and either way the arm still says error, and still passes.
#860 has the same defect and I proved it there by running it: I replaced the call with a function that does not exist, and the arm still printed PASS. The same substitution would pass here.
Assert the SQLSTATE, and the arm becomes evidence:
state_of() { # state_of SQL -> SQLSTATE, or ACCEPTED
q "DO \$\$ BEGIN $1 RAISE NOTICE 'ACCEPTED';
EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 |
grep -oE '[0-9A-Z]{5}|ACCEPTED' | tail -1
}
check "reject out-of-range Arrow date (22008)" \
"$(state_of "PERFORM pgcolumnar.import_arrow('ri_date_oob', '$PGC_WORKDIR/date_oob.arrows');")" "22008"Note expect_error also prints a different check name on the failing branch ("$label (expected error)"), so a harness matching on check names sees one name when it passes and another when it fails. Worth fixing while you are in there.
MAJOR: all three arms are inside if [ "$have_pyarrow" = 1 ]
They land at lines 166-174, inside the block opened at 136. On a box without pyarrow the whole section disappears and the suite reports PASSED having tested none of it. The audit already raised this for this exact file: arrow_export.sh:24 and arrow_nested.sh:22 call pgc_skip for the identical dependency, which fails unless waived with PGC_ALLOW_MISSING_PYARROW=1, and arrow_import.sh is the sibling that narrows silently instead.
CI has pyarrow, so they do run there — this is about the coverage quietly vanishing elsewhere, not about CI today.
What I checked in the C and believe is right
A_DATE32: promoting toint64before subtractingPG_TO_UNIX_DAYSis the correct order — the old code could overflowint32on the way toDateADT.IS_VALID_DATEon the promoted value is the right bound.A_TIMESTAMP: guardingv < PG_INT64_MIN + PG_TO_UNIX_USECSbefore the subtraction is the right direction and the only one that can overflow, since subtracting a positive constant cannot overflow upward.IS_VALID_TIMESTAMPafterwards catches the in-range-but-invalid remainder. Both are needed and both are there.A_TIME64:v < 0 || v >= USECS_PER_DAYmatches PostgreSQL'stimedomain.
One question I am not turning into a finding, because I did not verify it
A_TIME64 and A_TIMESTAMP are commented as [us], and Arrow permits s, ms, us and ns for both. If the import path accepts a file declaring nanoseconds and reads the integers as microseconds, the new bounds would reject valid data and accept wrong data — but I did not trace the schema-parsing side far enough to claim that, and this PR is about range checks rather than units. Flagging it as a question for you rather than asserting it.
|
Answering the question jdatcmd left open, with measurements. Both halves of his
He was right not to assert it without tracing, and the trace is a run. Every file Rejects valid data: Both, in one diff, exactly as suspected. I have filed the underlying bug as #865; it is
And the sequencing argument is now much strongerMeasured across the branches, for both #864 ( #861 closes both silent-corruption classes and ships a test for neither. I Land #861 first. If #862 lands first, both corruptions stay open and #862's own On your two findings against me hereAgreed on both, and the pyarrow-block one I missed: all three arms sit inside On the SQLSTATE arm: agreed, and your |
|
Correcting my own review: I said the time bound was right and it is not. I wrote that Measured on PG 17.10:
The bound is off by one microsecond. It should be: if (v < 0 || v > USECS_PER_DAY)
This is a BLOCKING finding and it upgrades my earlier review, which had the tests as the only problem. It also has a consequence for the tests: there is no positive arm anywhere that imports a valid extreme, so nothing in the suite would have caught the over-rejection. The fixtures feed only the extreme-negative end of all three guards — My earlier points stand unchanged: the three deny arms cannot tell your error from any other, and they sit inside the |
|
Cross-reference: #870 (fixes #864/#865) adds overflow and range guards to the The guards were arrived at independently and agree on the shape: reject rather 1. The upper range bound is unreachable, for every unit. I wrote an arm Only the lower bound is reachable ( 2. A time's sign must be tested on the stored value, not the scaled one. The same laundering shape exists in Posted as OffgridwithJD; not approving, same account as the author. |
Requested on review. The three arms used expect_error(), which passes when the statement fails for any reason at all -- a wrong path, a missing table, a malformed IPC stream, a different pyarrow, or this file's own pre-existing dictionary-encoding rejection. The claim is specifically that the value is refused as out of range, and nothing asserted that. They now assert the SQLSTATE through the file's existing sqlstate_or_hang helper rather than a fourth variant. Each arm reddens when the single guard it covers is reverted, one at a time, and reports a different code or none rather than 22008.
This PR now overlaps #870 semantically, and resolving the text would decide a behaviour question by accidentNot asking for a merge decision. Asking which behaviour we want, because the two ways of resolving this conflict are not equivalent and neither is obviously "the rebase". #870 merged into
The thing that makes this worth a ruling rather than a judgement call: Either may be right. Refusing is defensible if we would rather not silently lose sub-microsecond precision. Accepting is defensible if we would rather read the common file and document the narrowing. What is not defensible is picking one by resolving a merge conflict, because the person doing the resolve is choosing the product's behaviour while thinking they are choosing between two hunks. Also note the CI status on this PR is not evidence. Its only workflow run is against a commit that is no longer its head:
GitHub Actions fired nothing between roughly 20:54Z and 01:00Z, so the pushes at 21:40–21:41 produced no runs. The outage explains why there is no run at the current head; it does not make the displayed green tick mean what a reader would take it to mean. Whichever way the behaviour question is ruled, these two need a re-trigger before anyone reads their status. Holding off on resolving until someone rules. |
jdatcmd
left a comment
There was a problem hiding this comment.
This is not a merge-order problem, and it should not be resolved as one.
This branch forks from 8b39053, before #870 landed in main as 53224e4. #870 rewrote the
same three decode sites. Checked directly in both trees:
mainhasarrow_scale_to_usecs,pg_sub_s64_overflow,IS_VALID_DATEand
IS_VALID_TIMESTAMP, all raisingERRCODE_DATETIME_FIELD_OVERFLOW.- this branch has the naive comparisons and
arrow_scale_to_usecsdoes not exist in it at
all — 0 occurrences, against 3 onmain. The wholesrcUnitmachinery (n->srcUnit, the
DateUnit/TimeUnit reads, the width selection) is absent.
That is why it is CONFLICTING. Whoever resolves that conflict is choosing between #870's
unit-aware decode and this branch's naive one, while believing they are choosing between hunks.
Taking the branch's side deletes srcUnit and reintroduces the silent corruption #864/#865
fixed. Please do not resolve it in a merge; decide the question first.
Separately, there is no CI at this head (faa313c) — zero checks, from the Actions outage.
The tick this PR displays was earned by a commit that is no longer its head.
Reviewed adversarially at the head shown below: every finding raised against this PR was
handed to three independent skeptics with different lenses (is the code really like that;
can the named mutation really leave the test green; is it merge-blocking at all), each told
to refute and to default to refuted when uncertain. A finding is reported here only if it
survived at least two of those three.
3 finding(s) survived refutation
1. All four new arms are already green on main with this PR's entire src/ diff reverted — zero removal proof
test/arrow_import.sh:221-232 — refuter votes: stands(high) stands(high) stands(high)
The author's red arm was measured against the merge base 8b39053, not against main. #870 landed in main as 53224e4 and put equivalent-or-stronger guards on the same three decode arms (src/columnar_arrow.c:1773 IS_VALID_DATE, :1805-1808 raw<0 / us>USECS_PER_DAY, :1822-1824 arrow_scale_to_usecs + pg_sub_s64_overflow + IS_VALID_TIMESTAMP), every one raising ERRCODE_DATETIME_FIELD_OVERFLOW, which is the same SQLSTATE 22008 as this PR's ERRCODE_DATETIME_VALUE_OUT_OF_RANGE (errcodes.h:68-69 both MAKE_SQLSTATE('2','2','0','0','8')). So every arm the PR adds is satisfied by code already in the tree. This is the exact shape CONTEXT.md names: a check that cannot go red for the change it claims to prove. The PR body's '19 passed, 3 failed' red arm is a true statement about a base that no longer exists, and neither the PR body nor the CHANGELOG entry has been re-measured against main.
Failure scenario / mutation: Apply only test/arrow_import.sh from faa313c onto main (b4f0a45) and revert 100% of this PR's src/columnar_arrow.c hunks. date_oob (INT32_MIN date32): main computes days = -2147483648-10957, IS_VALID_DATE false -> 22008. time_oob (-1 us): main's raw < 0 -> 22008. timestamp_oob (INT64_MIN us): pg_sub_s64_overflow(INT64_MIN, 946684800000000) -> 22008. timestamp_pre_min (-2^60 us): subtraction is fine, IS_VALID_TIMESTAMP(-1153868189406846976) false -> 22008. All four print PASS with the fix deleted.
2. Merging this reverts #870 and reintroduces two silent-corruption bugs (#864, #865) that are fixed in main
src/columnar_arrow.c:1541-1580 — refuter votes: stands(high) stands(high) stands(high)
This branch's imp_scalar_at is the pre-#870 code. A_DATE32 does memcpy(&v, vp, 4) unconditionally, with no date64 (width==8) arm; A_TIME64 does memcpy(&v, vp, 8) with no time32 arm and no unit scaling; A_TIMESTAMP reads raw microseconds with no unit scaling. main carries n->srcUnit (line 1350), imp_apply_field reading Date.unit/Time.unit/Time.bitWidth/Timestamp.unit (1588-1625), arrow_scale_to_usecs with floored ns narrowing (1509), and the date64 milliseconds-to-days floor (1749-1765). Because the PR conflicts in exactly these hunks, resolving toward this PR deletes all of it. OffgridwithJD measured the consequence on this branch and posted it: date64 2000-01-01 stores 4908285-05-04, timestamp('ns') 2000-01-01 stores 31969-04-01. Note also this PR's guards are strictly weaker even where they overlap: v < PG_INT64_MIN + PG_TO_UNIX_USECS is a hand-rolled special case of pg_sub_s64_overflow, and it is only correct because PG_TO_UNIX_USECS happens to be positive.
Failure scenario / mutation: Import a pandas-default file: pyarrow emits datetime64[ns] as timestamp('ns'). On main, arrow_scale_to_usecs(ARROW_TU_NANO, ...) floors ns to us and the value is correct. Resolve #862's conflict toward #862 and the same file stores 31969-04-01 with no error. Same for any date64 column: 2000-01-01 -> 4908285-05-04.
3. The blocking ask from the CHANGES_REQUESTED review is not addressed: v >= USECS_PER_DAY rejects legal time '24:00:00'
src/columnar_arrow.c:1559 — refuter votes: stands(high) stands(high) stands(high)
jdatcmd's 2026-09-01T18:19Z comment explicitly upgraded the review to BLOCKING on this and asked for > instead of >=, plus two arms (24:00:00 must SUCCEED, 24:00:00.000001 must fail 22008). Head faa313c still reads if (v < 0 || v >= USECS_PER_DAY). main already has the correct bound at line 1808: us > USECS_PER_DAY. This is not academic: export_arrow writes TimeADT verbatim as int64 (src/columnar_arrow.c:575-580 on main), so time '24:00:00' leaves our exporter as exactly 86400000000 = USECS_PER_DAY and this PR refuses to read our own file back. There is still no positive arm anywhere in the new tests: all four fixtures are extreme-negative (INT32_MIN, -1, INT64_MIN, -2^60), so the upper half of every new bound has zero coverage and nothing in the suite can see the over-rejection. docs/sql-reference.md:478-481, added by this PR, says a value 'the target type cannot represent' raises 22008 — false of 24:00:00, which the target type represents fine.
Failure scenario / mutation: CREATE TABLE t (v time) USING pgcolumnar; INSERT INTO t VALUES ('24:00:00'); SELECT pgcolumnar.export_arrow('t','/tmp/t.arrows'); then import that file back into a fresh columnar time column. On main it round-trips. With this PR it fails 22008 on data pgColumnar itself wrote.
Non-blocking
- CHANGELOG entry asserts measurements that are no longer true of the tree it would land in (CHANGELOG.md:38-73): The entry states 'the underflow guard is the only thing between a hostile file and a stored garbage date' and 'each arm was measured red with its own guard reverted'. Both were true against merge base 8b39053 and are false against main, where pg_sub_s64_overflow (1823) already takes that input and every arm stays green with this PR's guards gone (see finding 1). The entry would also sit directly above main's existing #870 entry, which describes the same subsystem and the same 22008 behaviour from different code — a reader gets two accounts of one fix. Per CLAUDE.md's 'a correction dies when you regenerate' rule, this needs re-measuring against main, not re-wording.
|
Closing this as superseded by #870, with the policy question now decided by the owner. Not closing it as wrong. This PR and #870 were written against the same real defect — the
The owner has decided on the first: convert, never refuse, and report what was lost. That is Three things that made this the harder call than it looked, recorded so it is not relitigated:
What was genuinely valuable here and is NOT covered by #870: the non-temporal checks. That half deserves its own issue and its own PR against current Also worth stating so it is not inherited as a surprise: this branch's head deleted its own |
|
Record of the ruling this closure rests on, which was missing here. The owner decided the policy explicitly in session, after working through always-convert-silently, refuse-the-type, lossless-or-error, convert-and-warn, and a retry loop incrementing on This PR is closed as superseded by that ruling plus #870, not as incorrect. The non-temporal half of its validation is genuinely uncovered by #870 and is now tracked as #881. |
Summary
Reproduction
On current origin/main, crafted one-element PyArrow arrays carrying INT32_MIN, -1, and INT64_MIN are all accepted for PostgreSQL date, time, and timestamp targets. With only the regression tests applied to main, all three checks fail.
Tests