diff --git a/CHANGELOG.md b/CHANGELOG.md index a13222c83..672afdd14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -401,6 +401,14 @@ true until the next version shipped. ### Fixed +- **The Iceberg wrapper no longer reads server files while planning for a role the scan + would refuse.** `EXPLAIN` on an Iceberg foreign table read its `metadata_path` for any + role holding SELECT on it. That showed a role without `pg_read_server_files` the + table's row count, and a `metadata_path` that did not exist failed planning with + `58P01`, which probed whether a server path exists. The planner now reads the metadata + only for a role the scan would admit, and gives everyone else the flat estimate, as the + Parquet wrapper has since #330. `test/fdw_plan_privilege.sh` and + `test_fdw_plan_privilege.py` pin both wrappers' gates. - Changing a text column's collation without a rewrite no longer gives wrong answers (#1327). `ALTER COLUMN ... TYPE text COLLATE ` keeps the table's files, so the zone maps kept minima and maxima ordered under the old diff --git a/src/columnar_iceberg_fdw.c b/src/columnar_iceberg_fdw.c index fa468586a..8edb83842 100644 --- a/src/columnar_iceberg_fdw.c +++ b/src/columnar_iceberg_fdw.c @@ -299,6 +299,21 @@ icefdwGetForeignRelSize(PlannerInfo *root, RelOptInfo *baserel, * restriction clauses (baserestrictinfo is populated by now), so a filtered * scan estimates its filtered rows rather than the whole table. */ + /* + * ONLY FOR A ROLE THE SCAN WOULD ADMIT. The executor refuses a caller without + * pg_read_server_files at icefdwBeginForeignScan, but planning ran first and + * read the metadata for anyone: EXPLAIN then told such a role how many rows a + * server-side table holds, and a metadata_path that did not exist failed + * planning with 58P01, so planning probed whether a server path exists. The + * Parquet wrapper has gated its planner the same way since #330. Others get + * the flat estimate and learn nothing from the plan. + */ + if (!has_privs_of_role(GetUserId(), ROLE_PG_READ_SERVER_FILES)) + { + baserel->tuples = 1000.0; + set_baserel_size_estimates(root, baserel); + return; + } if (mdpath != NULL) est = PgColumnarIcebergEstimateRows(mdpath); baserel->tuples = (est > 0) ? (double) est : 1.0; diff --git a/test/check_ledger.tsv b/test/check_ledger.tsv index 6c59a597d..af03b6af9 100644 --- a/test/check_ledger.tsv +++ b/test/check_ledger.tsv @@ -551,6 +551,14 @@ encode_post_codec encode_post_codec the tail column holds no row the heap does n encode_post_codec encode_post_codec the tail column preserves its checksum 15;16;17;18;19 2026-10-13 the reader returning row 5 of every row group as NULL encode_post_codec encode_post_codec the tail column reads back exactly what the heap holds 15;16;17;18;19 2026-10-13 the reader returning row 5 of every row group as NULL encode_post_codec encode_post_codec while a column where encoding genuinely wins still encodes 15;16;17;18;19 never - +fdw_plan_privilege fdw_plan_privilege a privileged planner estimates the Iceberg table from its manifests 15;16;17;18;19 never - +fdw_plan_privilege fdw_plan_privilege a privileged planner estimates the Parquet table from the file's size 15;16;17;18;19 never - +fdw_plan_privilege fdw_plan_privilege an unprivileged planner gets the flat Parquet estimate 15;16;17;18;19 2026-10-14 the Parquet planner stats the file for any role (no pg_read_server_files gate) +fdw_plan_privilege fdw_plan_privilege an unprivileged planner gets the flat estimate, not the Iceberg table's row count 15;16;17;18;19 2026-10-14 the Iceberg planner reads metadata_path for any role, as before this fix +fdw_plan_privilege fdw_plan_privilege and a metadata_path that does not exist plans the same, so planning probes nothing 15;16;17;18;19 2026-10-14 the Iceberg planner reads metadata_path for any role, as before this fix +fdw_plan_privilege fdw_plan_privilege premise: the Parquet file was written 15;16;17;18;19 never - +fdw_plan_privilege fdw_plan_privilege premise: the reader is not a member of pg_read_server_files 15;16;17;18;19 never - +fdw_plan_privilege fdw_plan_privilege the unprivileged Iceberg scan is still refused (42501) 15;16;17;18;19 never - harness_selftest 030-assertions nothing leaked into the squatter 15;16;17;18;19 never - harness_selftest 030-assertions pgc_port_free says the squatter's port is busy 15;16;17;18;19 never - harness_selftest 030-assertions squatter survived untouched 15;16;17;18;19 never - diff --git a/test/check_ledger_budget.txt b/test/check_ledger_budget.txt index c726b914c..e1faa6895 100644 --- a/test/check_ledger_budget.txt +++ b/test/check_ledger_budget.txt @@ -1991,4 +1991,15 @@ suites_not_covered 242 # never 1918 + 7 = 1925 # dated 579 + 1 = 580 # 1925 + 580 == 2505 -checks_never_observed_red 1925 +# +# fdw_plan_privilege (jdatcmd): 1925 -> 1930. A new suite, seeded from five-major runs +# (8/8 on each of 15-19). Three arms arrive dated: the two Iceberg ones by the planner +# reading metadata_path for any role (main's code), the Parquet one by its +# pg_read_server_files gate removed. Five arrive never: two premises, two privileged +# controls, and the scan's 42501, which neither mutation touches. +# +# ledger rows 2505 + 8 = 2513 +# never 1925 + 8 - 3 = 1930 +# dated 580 + 3 = 583 +# 1930 + 583 == 2513 +checks_never_observed_red 1930 diff --git a/test/fdw_plan_privilege.sh b/test/fdw_plan_privilege.sh new file mode 100755 index 000000000..c8c58d49b --- /dev/null +++ b/test/fdw_plan_privilege.sh @@ -0,0 +1,73 @@ +#!/usr/bin/env bash +# +# The foreign-data wrappers' PLANNING path must not read server files for a role the +# scan itself would refuse (#330 for Parquet; the Iceberg wrapper had no such gate). +# A planner estimate is visible through EXPLAIN, so a wrapper that reads the file at +# plan time for anyone tells a role without pg_read_server_files how many rows a +# server-side table holds, and whether a path exists at all. +# +# Usage: test/fdw_plan_privilege.sh [PG_CONFIG] +# Written fresh for pgColumnar. + +set -uo pipefail +. "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +pgc_setup "${1:-/usr/local/pg17/bin/pg_config}" + +FX="$(dirname "${BASH_SOURCE[0]}")/fixtures/iceberg" +[ -f "$(ls "$FX"/warehouse/db/events/data/*/*.parquet 2>/dev/null | head -1)" ] \ + || pgc_skip fixture "iceberg warehouse data files are missing" +DEST="$PGC_WORKDIR/wh"; rm -rf "$DEST"; mkdir -p "$DEST" +cp -r "$FX/warehouse/db" "$DEST/db"; chmod -R a+rX "$DEST" +MD="$(ls "$DEST"/db/events/metadata/*.metadata.json | sort | tail -1)" +PQ="$PGC_WORKDIR/fp.parquet" + +# est ROLE SQL -> the top node's row estimate, or the SQLSTATE if EXPLAIN failed +est() { + local out + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" -d "$PGC_DB" \ + -At -v VERBOSITY=sqlstate -v ON_ERROR_STOP=1 -c "EXPLAIN $2" 2>&1)" \ + || { sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' <<<"$out" | head -1; return; } + grep -oE 'rows=[0-9]+' <<<"$out" | head -1 | cut -d= -f2 +} +state_as() { + local out + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" -d "$PGC_DB" \ + -At -v VERBOSITY=sqlstate -v ON_ERROR_STOP=1 -c "$2" 2>&1)" && { echo ok; return; } + sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' <<<"$out" | head -1 +} + +psql_run "CREATE ROLE fp_reader NOSUPERUSER LOGIN;" +psql_run "CREATE SERVER fp_ice FOREIGN DATA WRAPPER pgcolumnar_iceberg;" +psql_run "CREATE FOREIGN TABLE fp_ev (id bigint, region text, amount int) SERVER fp_ice OPTIONS (metadata_path '$MD');" +psql_run "CREATE FOREIGN TABLE fp_gone (id bigint, region text, amount int) SERVER fp_ice OPTIONS (metadata_path '$DEST/no/such/v1.metadata.json');" +psql_run "CREATE TABLE fp_src (id int, v text) USING pgcolumnar;" +psql_run "INSERT INTO fp_src SELECT g, md5(g::text) || md5((g * 7)::text) FROM generate_series(1, 20000) g;" +psql_run "SELECT pgcolumnar.export_parquet('fp_src', '$PQ');" +psql_run "CREATE SERVER fp_pq FOREIGN DATA WRAPPER pgcolumnar_parquet;" +psql_run "CREATE FOREIGN TABLE fp_pqt (id int, v text) SERVER fp_pq OPTIONS (path '$PQ');" +psql_run "GRANT SELECT ON fp_ev, fp_gone, fp_pqt TO fp_reader;" + +check "premise: the Parquet file was written" \ + "$(q "SELECT ((pg_stat_file('$PQ')).size > 0)::text")" "true" +check "premise: the reader is not a member of pg_read_server_files" \ + "$(q "SELECT pg_has_role('fp_reader', 'pg_read_server_files', 'USAGE')::text")" "false" +_ice_rows="$(q 'SELECT count(*) FROM fp_ev')" + +# ---- Iceberg --------------------------------------------------------------------- +check "a privileged planner estimates the Iceberg table from its manifests" \ + "$(est postgres "SELECT * FROM fp_ev")" "$_ice_rows" +check "an unprivileged planner gets the flat estimate, not the Iceberg table's row count" \ + "$(est fp_reader "SELECT * FROM fp_ev")" "1000" +check "and a metadata_path that does not exist plans the same, so planning probes nothing" \ + "$(est fp_reader "SELECT * FROM fp_gone")" "1000" +check "the unprivileged Iceberg scan is still refused (42501)" \ + "$(state_as fp_reader "SELECT count(*) FROM fp_ev;")" "42501" + +# ---- Parquet (#330) --------------------------------------------------------------- +_pq_priv="$(est postgres "SELECT * FROM fp_pqt")" +check "a privileged planner estimates the Parquet table from the file's size" \ + "$([ -n "$_pq_priv" ] && [ "$_pq_priv" != 1000 ] && echo sized || echo "flat ($_pq_priv)")" "sized" +check "an unprivileged planner gets the flat Parquet estimate" \ + "$(est fp_reader "SELECT * FROM fp_pqt")" "1000" + +pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 98acbe2b3..6c1cbb09b 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -152,6 +152,7 @@ behaviour, the source of that number is named. - [104. test_build_needs_pg_config.py: a build with no pg_config refuses, it does not wait on stdin](#104-test_build_needs_pg_configpy-a-build-with-no-pg_config-refuses-it-does-not-wait-on-stdin) - [105. test_import_export_privilege.py: import and export check the table's privilege, serial and parallel](#105-test_import_export_privilegepy-import-and-export-check-the-tables-privilege-serial-and-parallel) - [106. test_every_suite_job_installs_the_extensions.py: every job that runs the suites installs the extension packages](#106-test_every_suite_job_installs_the_extensionspy-every-job-that-runs-the-suites-installs-the-extension-packages) +- [107. test_fdw_plan_privilege.py: planning a foreign table reads no server file for a role the scan would refuse](#107-test_fdw_plan_privilegepy-planning-a-foreign-table-reads-no-server-file-for-a-role-the-scan-would-refuse) ## 1. How to read a test in here @@ -6848,3 +6849,13 @@ Needs no cluster and no driver. | `test_the_detector_names_what_it_finds_and_what_it_cannot_read` | on workflows written to fail, the detector names the job that lacks a package, a workflow it cannot read, and a workflow with no suite job | | `test_the_suite_jobs_are_the_five_known` | the suite-running jobs are exactly `ci.yml`'s `pytest-cluster` and `suites`, and `nightly.yml`'s `coverage`, `pytest-cluster` and `suites`, so a rename is red | | `test_every_suite_job_installs_every_extension_package` | no suite job lacks a package another installs | + +## 107. test_fdw_plan_privilege.py: planning a foreign table reads no server file for a role the scan would refuse + +The pytest twin of `test/fdw_plan_privilege.sh`. A planner estimate is visible through EXPLAIN, so a wrapper that reads its file at plan time for anyone tells a role without `pg_read_server_files` how many rows a server-side table holds. If the path doesn't exist, the planning error tells it that instead. The Iceberg wrapper's planner read `metadata_path` for every role: the unprivileged planner saw the true row count (5), and a missing path failed planning with `58P01`. Its planner now reads only for a role the scan would admit, as the Parquet wrapper's has since #330. That gate had no test either. + +### Every test + +| test | what it holds | +| --- | --- | +| `test_fdw_planning_reads_no_server_file_for_an_unprivileged_role` | a role without `pg_read_server_files` gets the flat estimate from both wrappers and plans a missing Iceberg path the same as a real one, while the scan still refuses it (42501); a privileged role gets the manifest count and the size estimate (controls) | diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 6bc6c4a00..a78088982 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -644,4 +644,7 @@ guard_tests 433 # in test_catalog_plan_index.py, by collection over the cluster files on this tree. # cluster_tests 541 -> 542 for #1394's test_a_delete_whose_qual_deletes_another_row_matches_heap # in test_native_delete_visibility_paths.py, by collection over the cluster files on this tree. -cluster_tests 542 +# +# cluster_tests 542 -> 543 for test_fdw_plan_privilege.py, one test, the twin of +# test/fdw_plan_privilege.sh, by collection over the cluster files on this branch. +cluster_tests 543 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index 955a74117..6948ebdc7 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -86,6 +86,7 @@ # The shape is `SHELL_REFERENCES`' in `test_harness_deps.py`, asserted in both # directions for the same reason: a one-way list rots into a permanent exemption. COMPLETE = ["advisory_lock_class", + "fdw_plan_privilege", "analyze_differential", "analyze_function", "analyze_reltuples", diff --git a/test/pytest/test_fdw_plan_privilege.py b/test/pytest/test_fdw_plan_privilege.py new file mode 100644 index 000000000..f25d36315 --- /dev/null +++ b/test/pytest/test_fdw_plan_privilege.py @@ -0,0 +1,116 @@ +"""The foreign-data wrappers' PLANNING path reads no server file for a role the scan +would refuse (the Iceberg gate; #330 for Parquet). + +A planner estimate is visible through EXPLAIN. A wrapper that reads the file at plan +time for anyone tells a role without pg_read_server_files how many rows a server-side +table holds, and whether a path exists at all. Independent of +test/fdw_plan_privilege.sh: own fixtures, own driver; check names match. +""" + +import os +import pathlib +import re +import shutil +import stat + +import psycopg + +HERE = pathlib.Path(__file__).resolve().parent +FIXTURE = HERE.parent / "fixtures" / "iceberg" / "warehouse" / "db" / "events" + + +def _open(root): + """Readable and writable by the server process, every parent included.""" + for parent in [root] + list(root.parents): + try: + os.chmod(parent, os.stat(parent).st_mode | stat.S_IROTH | stat.S_IXOTH) + except (PermissionError, FileNotFoundError): + break + for where, _dirs, files in os.walk(root): + os.chmod(where, 0o777) + for f in files: + os.chmod(os.path.join(where, f), 0o644) + + +def _est(conn, sql, role=None): + """-> the top node's row estimate as text, or the SQLSTATE if EXPLAIN failed.""" + with conn.cursor() as cur: + try: + if role: + cur.execute(f"SET ROLE {role}") + cur.execute("EXPLAIN " + sql) + plan = "\n".join(r[0] for r in cur.fetchall()) + return re.search(r"rows=(\d+)", plan).group(1) + except psycopg.Error as e: + return e.sqlstate + finally: + cur.execute("RESET ROLE") + + +def _state(conn, sql, role): + with conn.cursor() as cur: + try: + cur.execute(f"SET ROLE {role}") + cur.execute(sql) + return "ok" + except psycopg.Error as e: + return e.sqlstate + finally: + cur.execute("RESET ROLE") + + +def _one(conn, sql): + with conn.cursor() as cur: + cur.execute(sql) + return cur.fetchone()[0] + + +def test_fdw_planning_reads_no_server_file_for_an_unprivileged_role(pgc_conn, tmp_path, expect): + if not any((FIXTURE / "data").rglob("*.parquet")): + expect.cannot_run("MISSING_DEPENDENCY", "the committed Iceberg warehouse is not in this tree", + name="iceberg warehouse data files are missing") + return + stage = tmp_path / "fpp" + shutil.copytree(FIXTURE, stage / "events") + _open(stage) + md = sorted((stage / "events" / "metadata").glob("*.metadata.json"))[-1] + pq = stage / "fp.parquet" + with pgc_conn.cursor() as cur: + cur.execute("SELECT 1 FROM pg_roles WHERE rolname = 'fp_reader'") + if cur.fetchone() is None: + cur.execute("CREATE ROLE fp_reader NOSUPERUSER") + cur.execute("SELECT current_schema()") + cur.execute(f'GRANT USAGE ON SCHEMA "{cur.fetchone()[0]}" TO fp_reader') + cur.execute("CREATE SERVER fp_ice FOREIGN DATA WRAPPER pgcolumnar_iceberg") + cur.execute("CREATE FOREIGN TABLE fp_ev (id bigint, region text, amount int)" + f" SERVER fp_ice OPTIONS (metadata_path '{md}')") + cur.execute("CREATE FOREIGN TABLE fp_gone (id bigint, region text, amount int)" + f" SERVER fp_ice OPTIONS (metadata_path '{stage}/no/such/v1.metadata.json')") + cur.execute("CREATE TABLE fp_src (id int, v text) USING pgcolumnar") + cur.execute("INSERT INTO fp_src SELECT g, md5(g::text) || md5((g * 7)::text)" + " FROM generate_series(1, 20000) g") + cur.execute(f"SELECT pgcolumnar.export_parquet('fp_src', '{pq}')") + cur.execute("CREATE SERVER fp_pq FOREIGN DATA WRAPPER pgcolumnar_parquet") + cur.execute(f"CREATE FOREIGN TABLE fp_pqt (id int, v text) SERVER fp_pq OPTIONS (path '{pq}')") + cur.execute("GRANT SELECT ON fp_ev, fp_gone, fp_pqt TO fp_reader") + + expect.text(str(_one(pgc_conn, f"SELECT (pg_stat_file('{pq}')).size > 0")), "True", + "premise: the Parquet file was written") + expect.text(str(_one(pgc_conn, "SELECT pg_has_role('fp_reader', 'pg_read_server_files', 'USAGE')")), + "False", "premise: the reader is not a member of pg_read_server_files") + rows = str(_one(pgc_conn, "SELECT count(*) FROM fp_ev")) + + expect.text(_est(pgc_conn, "SELECT * FROM fp_ev"), rows, + "a privileged planner estimates the Iceberg table from its manifests") + expect.text(_est(pgc_conn, "SELECT * FROM fp_ev", "fp_reader"), "1000", + "an unprivileged planner gets the flat estimate, not the Iceberg table's row count") + expect.text(_est(pgc_conn, "SELECT * FROM fp_gone", "fp_reader"), "1000", + "and a metadata_path that does not exist plans the same, so planning probes nothing") + expect.text(_state(pgc_conn, "SELECT count(*) FROM fp_ev", "fp_reader"), "42501", + "the unprivileged Iceberg scan is still refused (42501)") + + priv = _est(pgc_conn, "SELECT * FROM fp_pqt") + expect.text("sized" if priv.isdigit() and priv != "1000" else f"flat ({priv})", "sized", + "a privileged planner estimates the Parquet table from the file's size") + expect.text(_est(pgc_conn, "SELECT * FROM fp_pqt", "fp_reader"), "1000", + "an unprivileged planner gets the flat Parquet estimate") diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 619564202..9171883be 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -89,6 +89,7 @@ SUITES=( entry_point_privilege estimate_deleted export_sink + fdw_plan_privilege fk_referencing fsst_margin fsst_verdict_cache