Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) - Mailing list pgsql-hackers

Hello Justin,

Happy new year!

> I think the 2nd half of the patches are still waiting for fixes to lstat() on
> windows.

Not your problem?

Here is my review about v32:

All patches apply cleanly.

# part 01

One liner doc improvement to tell that creation time is only available on windows.
It is indeed not available on Linux.

OK.

# part 02

Add tests for various options on pg_ls_dir, and for pg_stat_file, which were not
exercised before.  "make check" is ok.

OK.

# part 03

This patch adds a new pg_ls_dir_metadata.  Internally, this is an extension of
pg_ls_dir_files function which is used by other pg_ls functions.  Doc ok.

About the code:

ISTM that the "if (1) { if (2) continue; } else if(3) { if (4) continue; }" structure"
may be simplified to "if (1 && 2) continue; if (3 && 4) continue;", at least if
IS_DIR and IS_REG are incompatible? Otherwise, at least "else if (3 & 4) continue"?

The ifdef WIN32 (which probably detects windows 64 bits…) overwrites values[3]. ISTM
it could be reordered so that there is no overwrite, and simpler single assignements.

   #ifndef WIN32
     v = ...;
   #else
     v = ... ? ... : ...;
   #endif

New tests are added which check that the result columns are configured as required,
including error cases.

"make check" is ok.

OK.

# part 04

Add a new "isdir" column to "pg_ls_tmpdir" output.  This is a small behavioral
change.  I'm ok with it, however I'm unsure why we would not jump directly to
the "type" char column done later in the patch series.  ISTM all such functions
should be extended the same way for better homogeneity? That would also impact
"waldir", "archive_status", "logical_*", "replslot" variants. "make check" ok.

OK.

# part 05

This patch applies my previous advice:-) ISTM that parts 4 and 5 should be one
single patch. The test changes show that only waldir has a test. Would it be
possible to add minimal tests to other variants as well?  "make check" ok.

I'd consider add such tests with part 02.

OK.

# part 06

This part extends and adds a test for pg_ls_logdir. ISTM that it should
be merged with the previous patches.  "make check" is ok.

OK.

# part 07

This part extends pg_stat_file with more date informations.

ISTM that the documentation should be clear about windows vs unix/cygwin specific
data provided (change/creation).

The code adds a new value_from_stat function to avoid code duplication.
Fine with me.

All pg_ls_*dir functions are impacted. Fine with me.

"make check" is ok.

OK.

# part 08

This part substitutes lstat to stat. Fine with me.  "make check" is ok.
I guess that lstat does something under windows even if the concept of
link is somehow different there. Maybe the doc should say so somewhere?

OK.

# part 09

This part switches the added "isdir" to a "type" column.  "make check" is ok.
This is a definite improvement.

OK.

# part 10

This part adds a redundant "isdir" column. I do not see the point.
"make check" is ok.

NOT OK.

# part 11

This part adds a recurse option. Why not. However, the true value does not
seem to be tested? "make check" is ok.

My opinion is unclear.

Overall, ignoring part 10, this makes a definite improvement to postgres ls
capabilities. I do not seen any reason why not to add those.

-- 
Fabien.