On 2021/04/01 0:09, Kohei KaiGai wrote:
> What does the "permission checks" mean in this context?
> The permission checks on the foreign tables involved are already checked
> at truncate_check_rel(), by PostgreSQL's standard access control.
I meant that's the permission check that happens in the remote server side.
For example, when the foreign table is defined on postgres_fdw and truncated,
TRUNCATE command is issued to the remote server via postgres_fdw and
it checks the permission of the table before performing actual truncation.
> Please assume an extreme example below:
> 1. I defined a foreign table with file_fdw onto a local CSV file.
> 2. Someone tries to scan the foreign table, and ACL allows it.
> 3. I disallow the read remission of the CSV using chmod, after the above step,
> but prior to the query execution.
> 4. Someone's query moved to the execution stage, then IterateForeignScan()
> raises an error due to OS level permission checks.
>
> FDW is a mechanism to convert from/to external data sources to/from PostgreSQL's
> structured data, as literal. Once we checked the permissions of
> foreign-tables by
> database ACLs, any other permission checks handled by FDW driver are a part of
> execution (like, OS permission check when file_fdw read(2) the
> underlying CSV files).
> And, we have no reliable way to check entire permissions preliminary,
> like OS file
> permission check or database permission checks by remote server. Even
> if a checker
> routine returned a "hint", it may be changed at the execution time.
> Somebody might
> change the "truncate" permission at the remote server.
>
> How about your opinions?
I agree that something like checker routine might not be so useful and
also be overkill. I was thinking that it's better to truncate the foreign tables first
and the local ones later. Otherwise it's a waste of cycles to truncate
the local tables if the truncation on foreign table causes an permission error
on the remote server.
But ISTM that the order of tables to truncate that the current patch provides
doesn't cause any actual bugs. So if you think the current order is better,
I'm ok with that for now. In this case, the comments for ExecuteTruncate()
should be updated.
BTW, the latest patch doesn't seem to be applied cleanly to the master
because of commit 27e1f14563. Could you rebase it?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
