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

Hello Justin,

Some feedback on v10:

All patches apply cleanly, one on top of the previous. I really wish there
would be less than 9 patches…

* v10.1 doc change: ok

* v10.2 doc change: ok, not sure why it is not merged with previous

* v10.3 test add: could be merge with both previous

Tests seem a little contrived. I'm wondering whether something more
straightforward could be proposed. For instance, once the tablespace is
just created but not used yet, probably we do know that the tmp file
exists and is empty?

* v10.4 at least, some code!

Compiles, make check ok.

pg_ls_dir_files: I'm fine with the flag approach given the number of
switches and the internal nature of the function.

I'm not sure of the "FLAG_" prefix which seems too generic, even if it is
local. I'd suggest "LS_DIR_*", maybe, as a more specific prefix.

ISTM that Pg style requires spaces around operators. I'd suggest some
parenthesis would help as well, eg: "flags&FLAG_MISSING_OK" -> "(flags &
FLAG_MISSING_OK)" and other instances.

About:

if (S_ISDIR(attrib.st_mode)) {
if (flags&FLAG_SKIP_DIRS)
continue;
}

and similars, why not the simpler:

if (S_ISDIR(attrib.st_mode) && (flags & FLAG_SKIP_DIRS))
continue;

Especially that it is done like that in previous cases.

Maybe I'd create defines for long and common flag specs, eg:

#define ..._LS_SIMPLE (FLAG_SKIP_DIRS|FLAG_SKIP_HIDDEN|FLAG_SKIP_SPECIAL|FLAG_METADATA)

No attempt at recursing.

I'm not sure about these asserts:

/* isdir depends on metadata */
Assert(!(flags&FLAG_ISDIR) || (flags&FLAG_METADATA));

Hmmm. Why?

/* Unreasonable to show isdir and skip dirs */
Assert(!(flags&FLAG_ISDIR) || !(flags&FLAG_SKIP_DIRS));

Hmmm. Why would I prevent that, even if it has little sense, it should
work. I do not see having false on the isdir column as an actual issue.

* v10.5 add is_dir column, a few tests & doc.

Ok.

* v10.6 behavior change for existing functions, always show isdir column,
and removal of IS_DIR flag.

I'm unsure why the features are removed, some use case may benefit from
the more complete function?

Maybe flags defs should not be changed anyway?

I do not like much the "if (...) /* empty */;" code. Maybe it could be
caught more cleanly later in the conditional structure.

* v10.7 adds "pg_ls_dir_recurse" function

Using sql recurse to possibly to implement the feature is pretty elegant
and limits open directories to one at a time, which is pretty neat.

Doc looks incomplete and the example is very contrived and badly indented.

The function definition does not follow the style around: uppercase
whereas all others are lowercase, "" instead of '', no "as"…

I do not understand why oid 8511 is given to the new function.

I do not understand why UNION ALL and not UNION.

I would have put the definition after "pg_ls_dir_metadata" definition.

pg_ls_dir_metadata seems defined as (text,bool,bool) but called as
(text,bool,bool,bool).

Maybe a better alias could be given instead of x?

There are no tests for the new function. I'm not sure it would work.

* v10.8 change flags & add test on pg_ls_logdir().

I'm unsure why it is done at this stage.

* v10.9 change some ls functions and fix patch 10.7 issue

I'm unsure why it is done at this stage. "make check" ok.

--
Fabien.