diff --git a/CHANGELOG.md b/CHANGELOG.md index f9d2e43c..fb8bc05d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,45 @@ true until the next version shipped. ### Fixed +- `pgcolumnar.import_arrow` rejects an out-of-range Arrow temporal value instead + of storing an invalid `Datum` (#862). + + Arrow carries `date32`, `time64`, and `timestamp` as bare integers over the + carrier's full range. PostgreSQL's `date`, `time`, and `timestamp` are + narrower. The importer converted without checking, so a foreign file could put + a value into a columnar table that no PostgreSQL operator can read back. + Import now raises `22008` (`ERRCODE_DATETIME_VALUE_OUT_OF_RANGE`) on four + conditions: + + - a `date32` outside `IS_VALID_DATE`, + - a `time64` outside `[0, USECS_PER_DAY)`, + - a `timestamp` whose epoch shift would underflow `int64`, + - a `timestamp` outside `IS_VALID_TIMESTAMP` after that shift. + + **The suite asserts the SQLSTATE, not merely that it failed.** `22008` is the + claim. "The statement errored" is also true of a wrong path, a missing table, + or a malformed IPC stream. It is true of this file's own dictionary-encoding + rejection too. So each arm was measured red with its own guard reverted. Each + was green with the other three guards intact. The four runs used four + separately fingerprinted builds. Each red reports an absent SQLSTATE, not a + different one. + + Two things the arms measured along the way. First, `IS_VALID_TIMESTAMP`'s + **upper** bound is unreachable through this path. `ts >= END_TIMESTAMP` needs + an Arrow value of at least 9224318016000000000. That exceeds `INT64_MAX`, so + any such value underflows the subtraction first and the underflow guard takes + it. Only the lower bound is testable, and the arm uses it. + + Second, the underflow guard is not merely hygiene against undefined behaviour. + With it removed, the `INT64_MIN` row imports successfully and reads back as + `294247-01-10 04:00:54.775808`. That is measured, not derived. Whatever the + wrapped subtraction produced, `IS_VALID_TIMESTAMP` accepted it. The guard is + the only thing between a hostile file and a stored garbage date. + + When pyarrow is absent the four arms cannot run, so they report `UNRUN` + (`MISSING_DEPENDENCY`) and the suite exits `INCOMPLETE`. They previously would + have vanished inside a suite still reporting `PASSED`. + - A shebang and the execute bit go together, and every directory that documents a command is swept (#856). Two things were left over from #852. diff --git a/docs/sql-reference.md b/docs/sql-reference.md index 37ba96dd..23db6a54 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -475,6 +475,11 @@ Inserts the rows of an Arrow IPC stream file at `path` into the existing table `rel`. The column types of the table define the types that the function accepts. Returns the number of rows inserted. +Arrow carries `date32`, `time64`, and `timestamp` as bare integers. Their range +is wider than what PostgreSQL's `date`, `time`, and `timestamp` hold. A value +the target type cannot represent raises `22008` (`datetime_field_overflow`). +The import fails and stores nothing. + ### pgcolumnar.import_parquet(rel regclass, path text) returns bigint Inserts the rows of a Parquet file at `path` into the existing table `rel`. The diff --git a/src/columnar_arrow.c b/src/columnar_arrow.c index cfa01631..fbca75f3 100644 --- a/src/columnar_arrow.c +++ b/src/columnar_arrow.c @@ -1541,24 +1541,44 @@ imp_scalar_at(ImpNode *n, const uint8 *body, const int64 *bufOff, case A_DATE32: { int32 v; + int64 d; memcpy(&v, vp, 4); - return DateADTGetDatum((DateADT) (v - PG_TO_UNIX_DAYS)); + d = (int64) v - PG_TO_UNIX_DAYS; + if (!IS_VALID_DATE(d)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("columnar.import_arrow: date value is out of range"))); + return DateADTGetDatum((DateADT) d); } case A_TIME64: { int64 v; memcpy(&v, vp, 8); + if (v < 0 || v >= USECS_PER_DAY) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("columnar.import_arrow: time value is out of range"))); return TimeADTGetDatum(v); } case A_TIMESTAMP: case A_TIMESTAMPTZ: { int64 v; + Timestamp ts; memcpy(&v, vp, 8); - return TimestampGetDatum((Timestamp) (v - PG_TO_UNIX_USECS)); + if (v < PG_INT64_MIN + PG_TO_UNIX_USECS) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("columnar.import_arrow: timestamp value is out of range"))); + ts = (Timestamp) (v - PG_TO_UNIX_USECS); + if (!IS_VALID_TIMESTAMP(ts)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("columnar.import_arrow: timestamp value is out of range"))); + return TimestampGetDatum(ts); } case A_UUID: { diff --git a/test/arrow_import.sh b/test/arrow_import.sh index c39a6a6e..12271468 100755 --- a/test/arrow_import.sh +++ b/test/arrow_import.sh @@ -45,6 +45,13 @@ fifo_release() { exec 9<>"$1" 2>/dev/null; exec 9>&- 2>/dev/null; } have_pyarrow=1 python3 -c 'import pyarrow' 2>/dev/null || have_pyarrow=0 +# One name per arm, used by the arm and by its unrunnable counterpart below, so +# the two cannot drift apart. +OOB_DATE_ARM="reject out-of-range Arrow date as 22008" +OOB_TIME_ARM="reject out-of-range Arrow time as 22008" +OOB_TS_LOW_ARM="reject underflowing Arrow timestamp as 22008" +OOB_TS_RANGE_ARM="reject Arrow timestamp before the PostgreSQL minimum as 22008" + expect_error() { local label="$1" sql="$2" if psql_run "$sql" >/dev/null 2>&1; then @@ -144,6 +151,95 @@ with ipc.new_stream(pa.OSFile(sys.argv[1], 'wb'), t.schema) as w: PY psql_run "CREATE TABLE ri_d (c text) USING pgcolumnar;" expect_error "reject dictionary-encoded file" "SELECT pgcolumnar.import_arrow('ri_d', '$DICTF');" + + # Arrow temporal values span the integer carrier's full range, while + # PostgreSQL's date, time, and timestamp types have narrower valid ranges. + # Import must reject those values instead of overflowing or storing an + # invalid internal Datum. + # + # "it failed" is not the claim. The claim is that the value is refused as OUT + # OF RANGE, which is ERRCODE_DATETIME_VALUE_OUT_OF_RANGE = 22008. An arm that + # only asserts a non-zero exit also passes for a wrong path, a missing table, + # a malformed IPC stream, a different pyarrow, or this file's own + # dictionary-encoding rejection -- none of which is the property under test. + # These fixtures are hand-built buffers (pa.Array.from_buffers), so that + # difference is not theoretical. Assert the SQLSTATE. + # + # Four values for four guards, one value per guard, so a revert of any single + # guard reddens exactly one arm: + # date_oob INT32_MIN days -> !IS_VALID_DATE + # time_oob -1 usec -> v < 0 || v >= USECS_PER_DAY + # timestamp_oob INT64_MIN usec -> v < PG_INT64_MIN + PG_TO_UNIX_USECS + # timestamp_pre_min -2^60 usec -> !IS_VALID_TIMESTAMP, on its LOWER bound. + # + # IS_VALID_TIMESTAMP's upper bound is not reachable through this path and no + # fixture can cover it: ts >= END_TIMESTAMP needs v >= 9224318016000000000, + # which exceeds INT64_MAX, so any such Arrow value underflows the subtraction + # first and is caught by the guard above. -2^60 is chosen because it is below + # MIN_TIMESTAMP after the epoch shift yet nowhere near the underflow + # threshold (v < -9222425352054775808), so it pins the range guard alone. + # 2^62 does NOT work here and was measured doing so: it is a perfectly valid + # PostgreSQL timestamp (about year 148000) and imports successfully. + # + # The fixture is a premise, not a printout: if the writer fails or pyarrow + # stops accepting these buffers, the arms below would assert a SQLSTATE + # against a file that does not exist and report a plausible wrong answer. Gate + # on it and stop. + if ! python3 - "$PGC_WORKDIR" <<'PY' +import os, struct, sys, pyarrow as pa, pyarrow.ipc as ipc +out = sys.argv[1] +cases = { + 'date_oob.arrows': (pa.date32(), struct.pack('&2 + exit 1 + fi + for oob_fixture in date_oob time_oob timestamp_oob timestamp_pre_min; do + if [ ! -s "$PGC_WORKDIR/$oob_fixture.arrows" ]; then + echo "arrow_import.sh: fixture $oob_fixture.arrows is missing or empty" >&2 + exit 1 + fi + done + + psql_run "CREATE TABLE ri_date_oob (v date) USING pgcolumnar; + CREATE TABLE ri_time_oob (v time) USING pgcolumnar; + CREATE TABLE ri_timestamp_oob (v timestamp) USING pgcolumnar; + CREATE TABLE ri_timestamp_pre_min (v timestamp) USING pgcolumnar;" + # sqlstate_or_hang prints the bare 5-char SQLSTATE, or HANG, or nothing at all + # when the statement SUCCEEDED. check_text refuses an empty side outright, so + # "the guard is gone and the value imported" cannot read as anything but red. + check_text "$OOB_DATE_ARM" \ + "$(sqlstate_or_hang "SELECT pgcolumnar.import_arrow('ri_date_oob', '$PGC_WORKDIR/date_oob.arrows')")" \ + "22008" + check_text "$OOB_TIME_ARM" \ + "$(sqlstate_or_hang "SELECT pgcolumnar.import_arrow('ri_time_oob', '$PGC_WORKDIR/time_oob.arrows')")" \ + "22008" + check_text "$OOB_TS_LOW_ARM" \ + "$(sqlstate_or_hang "SELECT pgcolumnar.import_arrow('ri_timestamp_oob', '$PGC_WORKDIR/timestamp_oob.arrows')")" \ + "22008" + check_text "$OOB_TS_RANGE_ARM" \ + "$(sqlstate_or_hang "SELECT pgcolumnar.import_arrow('ri_timestamp_pre_min', '$PGC_WORKDIR/timestamp_pre_min.arrows')")" \ + "22008" +else + # These four arms live inside the pyarrow block. Without pyarrow they do not + # run, and an arm that silently does not run while the suite still reports + # PASSED is the failure mode this file has had before. Name them as the third + # state instead; the suite then exits INCOMPLETE rather than claiming a pass + # for a question nobody asked. (The pre-existing dictionary-encoding arm above + # has the same exposure; it is left alone here because it is not this change.) + for oob_arm in "$OOB_DATE_ARM" "$OOB_TIME_ARM" "$OOB_TS_LOW_ARM" "$OOB_TS_RANGE_ARM"; do + check_unrunnable "$oob_arm" MISSING_DEPENDENCY "pyarrow is not importable" + done fi IXFILE="$PGC_WORKDIR/ix_roundtrip.arrows"