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

Hello Justin,

Patch series applies cleanly. The last status compiles and passes "make
check". A few more comments:

* v8.[123] ok.

* v8.4

Avoid using the type name as a field name? "enum dir_action dir_action;"
-> "enum dir_action action", or maybe rename "dir_action" enum
"dir_action_t".

About pg_ls_dir:

"if (!fctx->dirdesc)" I do not think that is true even if AllocateDir
failed, the list exists anyway. ISTM it should be linitial which is NULL
in that case.

Given the overlap between pg_ls_dir and pg_ls_dir_files, ISTM that the
former should call the slightly extended later with appropriate flags.

About populate_paths:

function is a little bit strange to me, ISTM it would deserve more
comments.

I'm not sure the name reflect what it does. For instance, ISTM that it
does one thing, but the name is plural. Maybe "move_to_next_path" or
"update_current_path" or something?

It returns an int which can only be 0 or 1, which smells like an bool.
What is this int/bool is not told in the function head comment. I guess it
is whether the path was updated. When it returns false, the list length is
down to one.

Shouldn't AllocateDir be tested for bad result? Maybe it is a dir but you
do not have perms to open it? Or give a comment about why it cannot
happen?

later, good, at least the function is called, even if it is only for an
error case. Maybe some non empty coverage tests could be added with a
"count(*) > 0" on not is_dir or maybe "count(*) = 0" on is_dir, for
instance?

(SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace'
UNION SELECT 0 ORDER BY 1 DESC LIMIT 1)b

The 'b' glued to the ')' looks pretty strange. I'd suggest ") AS b".
Reusing the same alias twice could be avoided for clarity, maybe.

* v8.[56]

I'd say that a behavior change which adds a column and a possibly a few
rows is ok, especially as the tmpdir contains subdirs now.

--
Fabien.