Parquet path resolution: a dangling symlink or unreadable entry is silently skipped

[jdatcmd]

Follow-up from the review of #113, merged with the finding open.

pq_is_regular_file (src/columnar_parquet_reader.c) returns false on every stat failure, not only for a directory. A part-3.parquet that is a broken symlink, or one whose parent denies search, is now skipped silently: the query succeeds and returns the remaining files' rows. Before #113 the path was appended and the reader raised could not open file.

Silence is defensible for a directory listing, where a foo.parquet subdirectory is a normal thing to find in a Hive-style tree. It is harder to defend for a glob, because the user wrote the pattern that matched the name, and in both cases the failure mode is missing rows rather than an error.

Proposed change: skip S_ISDIR only, and let every other entry fall through to the reader's open, which already produces a clear per-path message.

Also open from the same review:

• The pattern "%s" matched no regular files path added in Phase G multifile: skip non-regular-file matches; add the suite to the matrix #113 has no test. It is reachable only when a glob matches at least one path and every match is filtered out; a glob over a directory holding only *.parquet subdirectories would cover it, and would pin whichever behaviour the above settles on.
• When a directory holds only *.parquet subdirectories, the error reads directory "..." contains no .parquet files, which is not what the user sees when they list it. contains no readable .parquet files describes it accurately.

[ChronicallyJD]

The finding is real and I had it in front of me during the #113 review: I noted
that a dangling symlink would now be dropped by the stat failure and judged it
acceptable, on the reasoning that skipping anything not provably a regular file
was uniform and defensible. Framing it as missing rows versus an error is the
better test, and by that test my call was wrong. A query that silently returns
two files' worth of rows out of three is the one outcome this reader is not
allowed to produce.

Confirmed the mechanism and the claim about the fallback message.
pq_is_regular_file is stat(path, &st) == 0 && S_ISREG(st.st_mode), so ENOENT
on a broken link, EACCES on a locked-down parent, and ELOOP on a symlink cycle
all land in the same bucket as "this is a directory". And the open path really
does report per-path with errno: AllocateFile failure raises could not open file "%s" for reading: %m in pq_slurp_and_parse, so a dangling part-3.parquet
would come back as a plain "No such file or directory" naming the file.

One case the proposal as written would regress

"Skip S_ISDIR only, let everything else fall through to the reader's open"
also lets a FIFO through, and AllocateFile(path, PG_BINARY_R) on a FIFO blocks
in open(2) until some process opens the write end. Since PostgreSQL installs
its signal handlers with SA_RESTART, the open restarts rather than returning
EINTR, so query cancel does not reliably get the backend back. A mkfifo part-3.parquet in a read directory would hang the session instead of erroring.

That is not hypothetical enough to ignore, because #113 is what started
excluding it: before that commit the FIFO was appended and hung, and S_ISREG
is what made it stop.

The distinction that actually matters is not is it a regular file but do we
know what it is:

if (stat(full, &st) != 0)
    keep it;              /* unknown: let the open report ENOENT/EACCES/ELOOP */
else if (S_ISDIR(st.st_mode))
    skip;                 /* a foo.parquet directory is normal in a Hive tree */
else if (!S_ISREG(st.st_mode))
    skip;                 /* FIFO, socket, device: known not a file, and a FIFO hangs */
else
    keep it;

That gets the whole of what the issue asks for -- nothing is dropped because it
could not be examined -- while keeping the one exclusion that prevents a hang.
If you would rather a FIFO be loud than silent, the third branch can ereport
instead of skipping; what it must not do is reach AllocateFile.

The two follow-ups interact with the choice

• Error wording. contains no readable .parquet files describes a state that
  the proposed change makes unreachable: once a stat failure is kept rather
  than filtered, unreadability is no longer a reason anything is missing from the
  list, and the only entries still filtered are ones that are not files at all.
  Under the branch above, matched no regular files stays exactly accurate for
  the glob, and the directory message is fine as it stands -- or, if you want it
  to say why a name the user can see did not count, something like contains no .parquet files (2 matching names are directories) reports the real reason.
  readable would send someone to check permissions that were never the problem.
• The untested branch. Agreed, and it is worth writing after this settles
  rather than before, because the reachable set changes: today it is any glob
  whose matches are all non-regular, and under the change it is a glob whose
  matches are all directories (plus FIFOs, if those keep being skipped). A
  directory holding only *.parquet subdirectories covers it either way, so the
  test can be written to whichever error string the fix lands on.

Happy to review the patch. The stat-failure half is a clear improvement over
what I approved.

[jdatcmd]

Patch is ready on fix/parquet-path-classify (addeb67), gated green on PostgreSQL 18.4 and 19beta2, all 61 suites. Opening the PR is currently failing with an HTTP 500 from the GitHub API; will retry.

[ChronicallyJD]

Reviewed addeb67 on fix/parquet-path-classify directly, since the PR is
blocked on the API. Posting here so it is not held up; I will re-confirm on the
PR when it opens.

The branch is up to date with main and the change is correct. No blocking
findings.

Verified

• The classification is the right shape. pq_path_is_candidate returns true
  on stat failure and S_ISREG(st.st_mode) otherwise, so the three cases are
  now distinct: unknown (kept, open reports it by name with errno), directory
  (skipped), any other known non-regular entry (skipped). Both call sites moved
  over, so directory and glob resolution classify identically.
• The FIFO exclusion survives, which was the whole risk in the simpler
  version of the fix. A mkfifo x.parquet still never reaches AllocateFile.
• Dangling symlink, EACCES parent, and symlink loop all now surface. ENOENT,
  EACCES and ELOOP are stat failures, so each is kept and hits could not open file "%s" for reading: %m in pq_slurp_and_parse, naming the file. That is
  the behaviour the issue asked for, and it restores the pre-Phase G multifile: skip non-regular-file matches; add the suite to the matrix #113 outcome for
  exactly those cases without restoring it for directories.
• A glob whose matches are all dangling symlinks does the right thing too:
  they are kept rather than filtered, so files is non-empty and the read errors
  by name instead of hitting matched no regular files. That message now means
  what it says, since the only entries dropped are ones that are known not to be
  files.
• errdetail_plural usage is correct -- singular and plural forms, skipped
  as the count and again as the %d argument, and the cond ? errdetail_plural(...) : 0
  ternary is the standard idiom in an ereport argument list. The detail only
  appears when the list came out empty, so a directory with two real files and one
  foo.parquet subdirectory stays quiet, which is right.
• The FIFO test's harness use is sound. PGC_BINDIR, PGC_PORT and PGC_DB
  are all set by pgc_setup, and the invocation matches q()'s except for the
  timeout wrapper and 2>&1. fifo_rc=$? does capture the command
  substitution's status, so the 124 branch works. Using the timeout as the real
  assertion is the right call: a regression fails the suite instead of hanging
  it.
• The dangling-symlink cases use read_parquet rather than the FDW, so nothing
  can quietly skip the open; broken.parquet also sorts ahead of part-0.parquet,
  so it is the first file opened either way.

Two suggestions, neither blocking

1. The new tests assert that an error happened, not which error. errs greps
   for ERROR, so "glob matching only directories errors" would pass on any
   failure, and the errdetail_plural branch this PR adds is not pinned by
   anything -- a future edit could drop the detail, or break the plural, with the
   suite still green. Matching on the message (matched no regular files, and
   not regular file for the detail) would tie these to the behaviour rather
   than to failure in general. The dangling-symlink checks would benefit most:
   right now they would pass if the read failed for an unrelated reason, which is
   the one thing that would hide a regression of this exact fix.
2. The glob branch has no counterpart to the new detail. The directory error
   now explains why a name the user can see did not count; the glob error still
   just says matched no regular files. Same information is available there
   (g.gl_pathc minus the kept count), if you want the two paths to read alike.

On the gate

18.4 and 19beta2 green covers the risk here; errdetail_plural and the ternary
form are old enough that 15 through 17 are not in question. Worth having in the
next full-matrix run at feature-set completion rather than a reason to run one
now.

Approving as soon as the PR exists.

[jdatcmd]

PR is up: #116. It takes the refinement from the review above rather than the fix this issue originally proposed, so a FIFO is still excluded and only stat failures change behaviour. Gated on PostgreSQL 18.4 and 19beta2, all 61 suites, and both dangling-symlink checks are verified to fail on the pre-fix code.