Hi,
On 9/26/18, 3:36 PM, "Laurenz Albe" <laurenz.albe@cybertec.at> wrote:
> The patch applies, builds without warning and passes "make check-world".
Thanks for the review!
> Since pgsql_tmp is only created when the first temp file is written,
> calling the function on a newly initdb'ed data directory fails with
>
> ERROR: could not open directory "base/pgsql_tmp": No such file or directory
>
> This should be fixed; it shouldn't be an error.
Done.
> Calling the function with a non-existing tablespace OID produces:
>
> ERROR: could not open directory "pg_tblspc/1665/PG_12_201809121/pgsql_tmp": No such file or directory
>
> Wouldn't it be better to have a check if the tablespace exists?
Done.
> About the code:
> The catversion bump shouldn't be part of the patch, it will
> rot too quickly. The committer is supposed to add it.
Removed.
> I think the idea to have such a function is fine, but I have two doubts:
>
> 1. Do we really need two functions, one without input argument
> to list the default tablespace?
> I think that anybody who uses with such a function whould
> be able to feed the OID of "pg_default".
That seems reasonable to me. I've removed the second version of the
function.
> 2. There already are functions "pg_ls_logdir" and "pg_ls_waldir",
> and I can imagine new requests, e.g. pg_ls_datafiles() to list the
> data files in a database directory.
>
> It may make sense to have a generic function like
>
> pg_ls_dir(dirname text, tablespace oid)
>
> instead. But maybe that's taking it too far...
There is an existing pg_ls_dir() function that appears to be available
only to superusers. IMO it's also nice to break out specific "safe"
directories like this for other roles (e.g. pg_monitor).
Nathan
