New review from Zsolt Parragi (Percona) — v5 code review
A new reviewer, Zsolt Parragi (Percona), posted a detailed code-level review of v5. This is the first substantive review feedback from someone other than Munro and introduces several concrete defects, including one genuine correctness bug that the prior analysis/review rounds missed.
1. Integer overflow in the return expression (real bug)
The most significant finding. The code:
return fst.f_bavail * fst.f_frsize; /* available blocks times fragment size */
Parragi points out (with a Godbolt link demonstrating the generated assembly) that the multiplication happens in the type of the operands before the implicit conversion to the 64-bit return value. On platforms where f_bavail and f_frsize are 32-bit unsigned long (notably 32-bit Linux, where POSIX only requires fsblkcnt_t/unsigned long width), this silently truncates for any filesystem larger than ~4 GB × fragment size. The fix is a cast on at least one operand, e.g. (uint64) fst.f_bavail * fst.f_frsize.
This matters because the whole point of the feature is multi-TB tablespaces — exactly the regime where 32-bit overflow bites. Neither Berg nor Munro flagged this across five patch revisions.
2. Windows error-handling inconsistency
The #ifdef WIN32 branch does elog(ERROR, "GetDiskFreeSpaceEx failed: error code %lu", GetLastError()), while the POSIX branch uses ereport with errcode_for_file_access() and %m. Parragi asks for symmetry:
- Use
_dosmaperr(GetLastError())to translate the Win32 error intoerrno, then use the sameereport/errcode_for_file_access()machinery. This is the standard PostgreSQL pattern (seesrc/port/*,dirmod.c, etc.) and gives consistent SQLSTATE values across platforms. - There is also a semantic divergence: on
ENOENTthe POSIX path returns -1 (→ NULL), but the Windows path raises ERROR. After the v4 change to raise errors onstatvfsfailure the POSIX side arguably no longer does this either, but the two branches should at minimum behave identically for "directory does not exist."
3. Error message wording
"could not statvfs directory \"%s\"" leaks the syscall name into a user-facing message. Parragi suggests "could not get free disk space for directory \"%s\"". This matches PostgreSQL's general style — user-facing messages describe the intent, not the syscall (compare "could not stat file" vs. "could not fsync file", where the latter is arguably a style bug of its own, but statvfs is considerably more obscure).
4. Documentation wording
"Returns the available disk space in the tablespace" is imprecise — a tablespace is a logical PG object, not a storage pool with its own free space. The reported number is the filesystem's free space, which may be shared with other tablespaces, pg_wal, the OS, etc. Suggested rewording: "returns the space on the filesystem hosting the tablespace." This also ties back to the unresolved pg_wal design question from the last round — the docs need to be honest that the number is per-filesystem, not per-tablespace.
5. Typo in the psql query
wHERE (lowercase w) in the v5 \db+ query. Minor, but indicates the v5 psql change was not compile-tested against a non-superuser path, or was hand-edited after testing.
Assessment
Parragi's review is the first from a reviewer who read the code rather than the design. The overflow bug is a must-fix before commit and somewhat undermines the earlier "go for it" trajectory — v6 is required. The Windows/POSIX error-path asymmetry is a reasonable portability concern that the previous review cycle glossed over (Munro's error-handling pushback was all about POSIX). No position shifts yet; Berg has not responded.