Re: Several issues with postgres_fdw stats import

On Sat, Sep 26, 2026 at 6:25 AM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> wrote:
> While testing the postgres_fdw statistics import feature, I also ran
> into issue (1), "User-defined functions may be executed with unexpected
> privileges".
>
> I've tested Noah's patch that Etsuro shared, and it looks good to me. It
> fixes both the domain-function case and the data disclosure case in all
> the scenarios I tried.

Thanks for the testing!

> Since the patch doesn't include regression tests, I'm attaching a
> patch with two tests added:
>
> - The first is based on Noah's example: a superuser runs ANALYZE on a
> foreign table owned by a non-superuser that has no user mapping. It
> now fails with "user mapping not found" instead of importing
> statistics the owner can't read.
>
> - The second covers the case Fujii described: a domain CHECK constraint
> on a foreign table column calls a function that reports current_user
> and search_path. It now runs as the table owner with search_path set
> to "pg_catalog, pg_temp".

I think it's a good idea to extend these to test the sampling path as
well. Note that we don't have any test cases to test that path's priv
handling.

> I also added a paragraph to the ImportForeignStatistics section on
> fdwhandler.sgml saying that the callback runs as the foreign table's
> owner in a security-restricted operation, so FDWs should use the owner's
> user mapping. This is now part of the callback's contract, and it
> differs from AnalyzeForeignTable, which is still called as the user
> running ANALYZE, so I think it's worth documenting for FDW authors.

I think the contract that the FDW should use the foreign table owner's
user mapping applies to the sampling path as well, as
AcquireSampleRowsFunc is run as the owner in that path. Also note
that postgresAnalyzeForeignTable follows the contract by doing this:

/*
* Get the connection to use. We do the remote access as the table's
* owner, even if the ANALYZE was started by some other user.
*/
table = GetForeignTable(RelationGetRelid(relation));
user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
conn = GetConnection(user, false, NULL);

I'm not sure we really need to add a note about this, but if we do so,
I think it's better to add it as a common contract for both
sampling/import paths.

Also, I recommend making this as an improvement for both paths, not
just for the import path, maybe in v20.

> Noah's code changes are unchanged; the attached patch adds only the
> tests, the docs and a commit message.

I pushed Noah's patch.

Best regards,
Etsuro Fujita