diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a79d9e..c700fc82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -209,6 +209,33 @@ true until the next version shipped. function and a wrong-arity call (`42883`), a null table name (`22004`) and `1/0` (`22012`). +- A parallel export refuses a destination too long to hold the names it generates + (#863). + + **`pgcolumnar.parallel_export_parquet` checked the destination's length not at + all.** Worker paths cross shared memory in `MAXPGPATH` buffers and have a + generated name appended, and `snprintf` truncates rather than failing, so a long + destination produced short names and the export completed as though nothing had + happened. + + Two distinct failures, both silent. A destination of 1000 bytes or more truncated + the part names themselves: the run wrote `part-0000.parqu` instead of + `part-0000.parquet`, stamped `_SUCCESS` beside it, and reported success for an + export `pgcolumnar.read_parquet` does not recognise. A destination of 994 to 999 + bytes wrote correct part names but truncated the path that + `pexport_remove_outputs` composes for its cleanup scan -- `"%s/%s"` from the same + directory and the sink's in-flight `part-NNNN.parquet.tmp.`, which is 30 + bytes past the directory with a seven-digit pid -- so the scan unlinked a + truncated path and left the temporary file behind. + + The destination is now probed against the longest name the file constructs, + `"/part-%04d.parquet.tmp.%d"` at `INT_MAX` for both, which is 39 bytes past the + directory against the final part name's 24. Anything that does not fit is refused + with `54000` before the destination directory is created, because a truncated run + publishes a differently named object and still stamps `_SUCCESS` over it. + + The sink itself was never at risk and is unchanged: `columnar_sink.c` builds its + temporary name with `psprintf`, which allocates. - 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/src/columnar_parallel_export.c b/src/columnar_parallel_export.c index 5b609384..73ea1b1f 100644 --- a/src/columnar_parallel_export.c +++ b/src/columnar_parallel_export.c @@ -606,6 +606,7 @@ pgcolumnar_parallel_export_parquet(PG_FUNCTION_ARGS) cur; int64 total = 0; int failed = -1; + char pathProbe[MAXPGPATH]; if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) ereport(ERROR, @@ -700,6 +701,33 @@ pgcolumnar_parallel_export_parquet(PG_FUNCTION_ARGS) errmsg("relation \"%s\" is not a columnar table or a partitioned table with columnar partitions", RelationGetRelationName(rel)))); + /* + * Worker paths cross DSM in MAXPGPATH buffers and add a generated part + * name. Refuse a destination that cannot hold the longest possible name; + * truncating it would publish a differently named object and still stamp + * _SUCCESS over an unreadable export. + * + * The longest such name is NOT the final part name. pexport_remove_outputs() + * composes "%s/%s" from this directory and a directory entry into a + * MAXPGPATH buffer, and the entries it acts on include the sink's in-flight + * form part-NNNN.parquet.tmp.. Measured: "/part-0000.parquet.tmp.PID" + * with a 7-digit pid (this platform's pid_max is 4194304) is dir + 30, while + * the final part name "/part-2147483647.parquet" is only dir + 24, so + * probing the part name alone left a window -- dir lengths 994..999 at + * MAXPGPATH 1024 -- where the destination was accepted and the cleanup scan + * then truncated a path it unlinks. Probe the longest form this file + * constructs instead. The part index and the pid are both ints, so INT_MAX + * bounds each, and the probe is 39 bytes wide against the part name's 24. + */ + if (snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet.tmp.%d", + dir, INT_MAX, INT_MAX) >= (int) sizeof(pathProbe)) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("parallel export destination is too long"), + errdetail("The destination, the generated part name and the " + "in-flight temporary suffix must fit in %d bytes.", + MAXPGPATH))); + pexport_prepare_dir(dir); /* diff --git a/test/parallel_export_parquet.sh b/test/parallel_export_parquet.sh index f16d2d87..f08e698e 100755 --- a/test/parallel_export_parquet.sh +++ b/test/parallel_export_parquet.sh @@ -190,6 +190,157 @@ check "retry into the cleaned directory succeeds" \ check "the retry writes a fresh _SUCCESS marker" \ "$([ -f "$DCX/_SUCCESS" ] && echo yes || echo no)" yes +# ---- destination-length guard (#863) ---------------------------------------- +# A destination that fits by itself but not with a generated suffix used to be +# silently truncated: the export returned success and stamped _SUCCESS beside a +# clipped part name, which read_parquet ignores. +# +# The guard has to probe the LONGEST path this file builds into a fixed +# MAXPGPATH buffer, and that is NOT the final part name. pexport_remove_outputs() +# composes "%s/%s" from the directory and a directory entry, and the entries it +# unlinks include the sink's in-flight form part-NNNN.parquet.tmp.: +# +# final part name "/part-2147483647.parquet" dir + 24 +# cleanup scan "/part-0000.parquet.tmp.1234567" dir + 30 +# +# 7-digit pids are reachable here (/proc/sys/kernel/pid_max is 4194304), so a +# guard that probes only the part name accepts dir lengths 994..999 while the +# cleanup scan's buffer truncates a path it then unlinks. The probe must be the +# longest constructed form, "/part-.parquet.tmp.", which is 39 bytes +# at INT_MAX for both numbers -- so the longest acceptable destination is +# MAXPGPATH - 1 - 39. +# +# Both boundaries are asserted: one byte under must still export (this class of +# fix breaks by becoming over-broad and rejecting legal paths) and one byte over +# must be refused with ERRCODE_PROGRAM_LIMIT_EXCEEDED, by SQLSTATE -- "it +# failed" also passes for a typo, a missing table or a dead server. + +PEXPORT_TOO_LONG_SQLSTATE=54000 # ERRCODE_PROGRAM_LIMIT_EXCEEDED + +# premise 1: the arithmetic is written for MAXPGPATH == 1024. +PGC_MAXPGPATH="$(sed -n 's/^#define[[:space:]]\{1,\}MAXPGPATH[[:space:]]\{1,\}\([0-9]\{1,\}\).*/\1/p' \ + "$("$PGC_PG_CONFIG" --includedir-server)/pg_config_manual.h" | head -1)" +if [ "$PGC_MAXPGPATH" != "1024" ]; then + echo "PREMISE FAILED: MAXPGPATH is [$PGC_MAXPGPATH]; these arms are written for 1024" >&2 + exit 1 +fi + +# premise 2: the cleanup scan still builds dir + "/" + entry into a fixed +# buffer, and the sink still appends ".tmp." to the part name. If either +# moves, the +30 above is stale and these lengths measure nothing. +pexport_anchor="$(grep -c 'snprintf(fp, sizeof(fp), "%s/%s", dir, de->d_name);' \ + "$PGC_SRCDIR/src/columnar_parallel_export.c")" +sink_anchor="$(grep -c 'psprintf("%s.tmp.%d", path, MyProcPid)' \ + "$PGC_SRCDIR/src/columnar_sink.c")" +if [ "$pexport_anchor" != "1" ] || [ "$sink_anchor" != "1" ]; then + echo "PREMISE FAILED: cleanup-scan anchor [$pexport_anchor] sink .tmp anchor [$sink_anchor], wanted 1 and 1" >&2 + exit 1 +fi + +# Longest destination that still holds the longest generated path, and the +# shortest destination at which the cleanup scan's "%s/%s" truncates with a +# 7-digit pid (len("/part-0000.parquet.tmp.1234567") == 30). +PEXPORT_LEN_OK=$(( PGC_MAXPGPATH - 1 - 39 )) # 984 +PEXPORT_LEN_OVER=$(( PEXPORT_LEN_OK + 1 )) # 985 +PEXPORT_LEN_WINDOW=$(( PGC_MAXPGPATH - 30 )) # 994 + +# Build a directory path of an EXACT length and create its parents 777, so the +# server (running as postgres) can create the final component itself. Sets +# LP_PATH; it does not echo, because an exit inside a command substitution would +# leave the suite running with an ungated premise. +LP_PATH="" +path_of_len() { + local want="$1" p="$PGC_WORKDIR/lp" rem + while [ $(( want - ${#p} - 1 )) -gt 255 ]; do + p="$p/$(printf 'a%.0s' $(seq 1 200))" + done + rem=$(( want - ${#p} - 1 )) + if [ "$rem" -lt 1 ]; then + echo "PREMISE FAILED: cannot build a ${want}-byte path under $PGC_WORKDIR" >&2 + exit 1 + fi + p="$p/$(printf 'a%.0s' $(seq 1 "$rem"))" + if [ "${#p}" -ne "$want" ]; then + echo "PREMISE FAILED: built a ${#p}-byte path, wanted $want" >&2 + exit 1 + fi + mkdir -p "$(dirname "$p")" || { echo "PREMISE FAILED: mkdir -p for $want failed" >&2; exit 1; } + chmod -R 777 "$PGC_WORKDIR/lp" || { echo "PREMISE FAILED: chmod for $want failed" >&2; exit 1; } + if [ ! -d "$(dirname "$p")" ] || [ -e "$p" ]; then + echo "PREMISE FAILED: parent missing or target already present for $want" >&2 + exit 1 + fi + LP_PATH="$p" +} + +# Run one statement ONCE and record its 5-char SQLSTATE, "none" when it +# succeeded, or HANG on the wall-clock cap. VERBOSITY verbose puts the SQLSTATE +# and the message on the same ERROR line, so one run yields both, and a caller +# can assert the message without executing the statement a second time -- a +# second run of an accepted export hits the require-empty check and returns a +# DIFFERENT sqlstate (55000), which is exactly how "it failed" lies. +# +# Sets globals rather than echoing: a global assigned inside $( ) is assigned in +# a subshell and never reaches the caller, so SQLSTATE_LAST_OUT would arrive +# empty and its message check would silently compare nothing. +SQLSTATE_LAST="" +SQLSTATE_LAST_OUT="" +run_sqlstate() { + local rc + SQLSTATE_LAST="" + SQLSTATE_LAST_OUT="$(timeout -s KILL 180 env PATH="$PGC_BINDIR:$PATH" psql \ + -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -qtA 2>&1 <