Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)

Justin Pryzby <pryzby(at)telsasoft(dot)com> writes:
>> This patch has been waiting for input from a committer on the approach I've
>> taken with the patches since March 10, so I'm planning to set to "Ready" - at
>> least ready for senior review.

I took a quick look through this. This is just MHO, of course:

* I don't think it's okay to change the existing signatures of
pg_ls_logdir() et al. Even if you can make an argument that it's
not too harmful to add more output columns, replacing pg_stat_file's
isdir output with something of a different name and datatype is most
surely not OK --- there is no possible way that doesn't break existing
user queries.

I think possibly a more acceptable approach is to leave these functions
alone but add documentation explaining how to get the additional info.
You could say things along the lines of "pg_ls_waldir() is the same as
pg_ls_dir_metadata('pg_wal') except for showing fewer columns."

* I'm not very much on board with implementing pg_ls_dir_recurse()
as a SQL function that depends on a WITH RECURSIVE construct.
I do not think that's okay from either performance or security
standpoints. Surely it can't be hard to build a recursion capability
into the C code? We could then also debate whether this ought to be
a separate function at all, instead of something you invoke via an
additional boolean flag parameter to pg_ls_dir_metadata().

* I'm fairly unimpressed with the testing approach, because it doesn't
seem like you're getting very much coverage. It's hard to do better while
still having the totally-fixed output expected by our regular regression
test framework, but to me that just says not to test these functions in
that framework. I'd consider ripping all of that out in favor of a
TAP test.

While I didn't read the C code in any detail, a couple of things stood
out to me:

* I noticed that you did s/stat/lstat/. That's fine on Unix systems,
but it won't have any effect on Windows systems (cf bed90759f),
which means that we'll have to document a platform-specific behavioral
difference. Do we want to go there? Maybe this patch needs to wait
on somebody fixing our lack of real lstat() on Windows. (I assume BTW
that this means the WIN32 code in get_file_type() is unreachable.)

* This bit:

+ /* Skip dot dirs? */
+ if (flags & LS_DIR_SKIP_DOT_DIRS &&
+ (strcmp(de->d_name, ".") == 0 ||
+ strcmp(de->d_name, "..") == 0))
+ continue;
+
+ /* Skip hidden files? */
+ if (flags & LS_DIR_SKIP_HIDDEN &&
+ de->d_name[0] == '.')
continue;

doesn't seem to have thought very carefully about the interaction
of those two flags, ie it seems like LS_DIR_SKIP_HIDDEN effectively
implies LS_DIR_SKIP_DOT_DIRS. Do we want that?

regards, tom lane