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

Greetings,

* Justin Pryzby (pryzby@telsasoft.com) wrote:

On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:

* 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."

On Mon, Nov 23, 2020 at 06:06:19PM -0500, Tom Lane wrote:

I'm mostly concerned about removing the isdir output of pg_stat_file().
Maybe we could compromise to the extent of keeping that, allowing it
to be partially duplicative of a file-type-code output column.

On Tue, Nov 24, 2020 at 11:53:22AM -0500, Stephen Frost wrote:

I don't have any particular issue with keeping isdir as a convenience
column. I agree it'll now be a bit duplicative but that seems alright.

Maybe we should do what Tom said, and leave pg_ls_* unchanged, but also mark
them as deprecated in favour of:
| pg_ls_dir_metadata(dir), dir={'pg_wal/archive_status', 'log', 'pg_wal', 'base/pgsql_tmp'}

Haven't really time to review the patches here in detail right now
(maybe next month), but in general, I dislike marking things as
deprecated. If we don't want to change them and we're happy to continue
supporting them as-is (which is what 'deprecated' really means), then we
can just do so- nothing stops us from that. If we don't think the
current API makes sense, for whatever reason, we can just change that-
there's no need for a 'deprecation period', as we already have major
versions and support each major version for 5 years.

I haven't particularly strong feelings one way or the other regarding
these particular functions. If you asked which way I leaned, I'd say
that I'd rather redefine the functions to make more sense and to be easy
to use for people who would like to use them. I wouldn't object to new
functions to provide that either though. I don't think there's all that
much code or that it's changed often enough to be a big burden to keep
both, but that's more feeling than anything based in actual research at
this point.

Thanks,

Stephen

On 12/23/20 2:27 PM, Stephen Frost wrote:

* Justin Pryzby (pryzby@telsasoft.com) wrote:

On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:

* 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."

On Mon, Nov 23, 2020 at 06:06:19PM -0500, Tom Lane wrote:

I'm mostly concerned about removing the isdir output of pg_stat_file().
Maybe we could compromise to the extent of keeping that, allowing it
to be partially duplicative of a file-type-code output column.

On Tue, Nov 24, 2020 at 11:53:22AM -0500, Stephen Frost wrote:

I don't have any particular issue with keeping isdir as a convenience
column. I agree it'll now be a bit duplicative but that seems alright.

Maybe we should do what Tom said, and leave pg_ls_* unchanged, but also mark
them as deprecated in favour of:
| pg_ls_dir_metadata(dir), dir={'pg_wal/archive_status', 'log', 'pg_wal', 'base/pgsql_tmp'}

Haven't really time to review the patches here in detail right now
(maybe next month), but in general, I dislike marking things as
deprecated.

Stephen, are you still planning to review these patches?

Regards,
--
-David
david@pgmasters.net

On Wed, Dec 23, 2020 at 01:17:10PM -0600, Justin Pryzby wrote:

On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:

* 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 think only the "top" patches depend on lstat (for the "type" column and
recursion, to avoid loops). The initial patches are independently useful, and
resolve the original issue of hiding tmpdirs. I've rebased and re-arranged the
patches to reflect this.

I said that, but then failed to attach the re-arranged patches.
Now I also renumbered OIDs following best practice.

The first handful of patches address the original issue, and I think could be
"ready":

$ git log --oneline origin..pg-ls-dir-new |tac
... Document historic behavior of links to directories..
... Add tests on pg_ls_dir before changing it
... Add pg_ls_dir_metadata to list a dir with file metadata..
... pg_ls_tmpdir to show directories and "isdir" argument..
... pg_ls_*dir to show directories and "isdir" column..

These others are optional:
... pg_ls_logdir to ignore error if initial/top dir is missing..
... pg_ls_*dir to return all the metadata from pg_stat_file..

..and these maybe requires more work for lstat on windows:
... pg_stat_file and pg_ls_dir_* to use lstat()..
... pg_ls_*/pg_stat_file to show file *type*..
... Preserve pg_stat_file() isdir..
... Add recursion option in pg_ls_dir_files..

--
Justin

Breaking with tradition, the previous patch included one too *few* changes, and
failed to resolve the OID collisions.

On Tue, Apr 06, 2021 at 11:01:31AM -0500, Justin Pryzby wrote:

On Wed, Dec 23, 2020 at 01:17:10PM -0600, Justin Pryzby wrote:

On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:

* 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 think only the "top" patches depend on lstat (for the "type" column and
recursion, to avoid loops). The initial patches are independently useful, and
resolve the original issue of hiding tmpdirs. I've rebased and re-arranged the
patches to reflect this.

I said that, but then failed to attach the re-arranged patches.
Now I also renumbered OIDs following best practice.

The first handful of patches address the original issue, and I think could be
"ready":

$ git log --oneline origin..pg-ls-dir-new |tac
... Document historic behavior of links to directories..
... Add tests on pg_ls_dir before changing it
... Add pg_ls_dir_metadata to list a dir with file metadata..
... pg_ls_tmpdir to show directories and "isdir" argument..
... pg_ls_*dir to show directories and "isdir" column..

These others are optional:
... pg_ls_logdir to ignore error if initial/top dir is missing..
... pg_ls_*dir to return all the metadata from pg_stat_file..

..and these maybe requires more work for lstat on windows:
... pg_stat_file and pg_ls_dir_* to use lstat()..
... pg_ls_*/pg_stat_file to show file *type*..
... Preserve pg_stat_file() isdir..
... Add recursion option in pg_ls_dir_files..

@cfbot: rebased

In an attempt to get this patch set off the ground again, I took a
look at the first 5 patches.

0001: This one is a very small documentation update for pg_stat_file
to point out that isdir will be true for symbolic links to
directories. Given this is true, I think the patch looks good.

0002: This patch adds some very basic testing for pg_ls_dir(). The
only comment that I have for this one is that I might also check
whether '..' is included in the results of the include_dot_dirs tests.
The docs specifically note that include_dot_dirs indicates whether
both '.' and '..' are included, so IMO we might as well verify that.

0003: This one didn't apply cleanly until I used 'git apply -3', so it
likely needs a rebase. This patch introduces the pg_ls_dir_metadata()
function, which appears to just be pg_ls_dir() with some additional
columns for the size and modification time. My initial reaction to
this one is that we should just add those columns to pg_ls_dir() to
match all the other pg_ls_* functions (and not bother attempting to
maintain historic behavior for things like hidden and special files).
I believe there is some existing discussion on this point upthread, so
perhaps there is a good reason to make a new function. In any case, I
like the idea of having pg_ls_dir() use pg_ls_dir_files() internally
like the rest of the pg_ls_* functions.

0004: This one changes pg_ls_tmpdir to show directories as well. I
think this is a reasonable change. On it's own, the patch looks
alright, although it might look different if my suggestions for 0003
were followed.

0005: This one adjusts the rest of the pg_ls_* functions to show
directories. Again, I think this is a reasonable change. As noted in
0003, I think it'd be alright just to have all the pg_ls_* functions
show special and hidden files as well. It's simple enough already to
filter our files that start with '.' if necessary, and I'm not sure
there's any strong reason for leaving out special files. If special
files are included, perhaps isdir should be changed to indicate the
file type instead of just whether it is a directory. (Reading ahead,
it looks like this is what 0009 might do.)

I haven't looked at the following patches too much, but I'm getting
the idea that they might address a lot of the feedback above and that
the first bunch of patches are more like staging patches that add the
abilities without changing the behavior. I wonder if just going
straight to the end goal behavior might simplify the patch set a bit.
I can't say I feel too strongly about this, but I figure I'd at least
share my thoughts.

Nathan

On Mon, Nov 22, 2021 at 07:17:01PM +0000, Bossart, Nathan wrote:

In an attempt to get this patch set off the ground again, I took a
look at the first 5 patches.

I haven't looked at the following patches too much, but I'm getting
the idea that they might address a lot of the feedback above and that
the first bunch of patches are more like staging patches that add the
abilities without changing the behavior. I wonder if just going
straight to the end goal behavior might simplify the patch set a bit.
I can't say I feel too strongly about this, but I figure I'd at least
share my thoughts.

Thanks for looking.

The patches are separate since the early patches are the most necessary, least
disputable parts, to allow the possibility of (say) chaging pg_ls_tmpdir() without
changing other functions, since pg_ls_tmpdir was was original motivation behind
this whole thread.

In a recent thread, Bharath Rupireddy added pg_ls functions for the logical
dirs, but expressed a preference not to add the metadata columns. I still
think that at least "isdir" should be added to all the "ls" functions, since
it's easy to SELECT the columns you want, and a bit of a pain to write the
corresponding LATERAL query: 'dir' AS dir, pg_ls_dir(dir) AS ls,
pg_stat_file(ls) AS st. I think it would be strange if pg_ls_tmpdir() were to
return a different set of columns than the other functions, even though admins
or extensions might have created dirs or other files in those directories.

Tom pointed out that we don't have a working lstat() for windows, so then it
seems like we're not yet ready to show file "types" (we'd show the type of the
link target, which is sometimes what's wanted, but not usually what "ls" would
show), nor ready to implement recurse.

As before:

On Tue, Apr 06, 2021 at 11:01:31AM -0500, Justin Pryzby wrote:

The first handful of patches address the original issue, and I think could be
"ready":

$ git log --oneline origin..pg-ls-dir-new |tac
... Document historic behavior of links to directories..
... Add tests on pg_ls_dir before changing it
... Add pg_ls_dir_metadata to list a dir with file metadata..
... pg_ls_tmpdir to show directories and "isdir" argument..
... pg_ls_*dir to show directories and "isdir" column..

These others are optional:
... pg_ls_logdir to ignore error if initial/top dir is missing..
... pg_ls_*dir to return all the metadata from pg_stat_file..

..and these maybe requires more work for lstat on windows:
... pg_stat_file and pg_ls_dir_* to use lstat()..
... pg_ls_*/pg_stat_file to show file *type*..
... Preserve pg_stat_file() isdir..
... Add recursion option in pg_ls_dir_files..

rebased on 1922d7c6e1a74178bd2f1d5aa5a6ab921b3fcd34

--
Justin

Hello Justin,

It seems that the v31 patch does not apply anymore:

postgresql> git apply ~/v31-0001-Document-historic-behavior-of-links-to-directori.patch
error: patch failed: doc/src/sgml/func.sgml:27410
error: doc/src/sgml/func.sgml: patch does not apply

--
Fabien.

On Thu, Dec 23, 2021 at 09:14:18AM -0400, Fabien COELHO wrote:

It seems that the v31 patch does not apply anymore:

postgresql> git apply ~/v31-0001-Document-historic-behavior-of-links-to-directori.patch
error: patch failed: doc/src/sgml/func.sgml:27410
error: doc/src/sgml/func.sgml: patch does not apply

Thanks for continuing to follow this patch ;)

I fixed a conflict with output/tablespace from d1029bb5a et seq.
I'm not sure why you got a conflict with 0001, though.

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

You complained before that there were too many patches, and I can see how it
might be a pain depending on your workflow. But the division is deliberate.
Dealing with patchsets is easy for me: I use "mutt" and for each patch
attachment, I type "|git am" (or |git am -3).

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.

Here is my review about v32:

I forgot to tell that doc generation for the cumulated changes also works.

--
Fabien.

Hi,

On Sun, Jan 02, 2022 at 02:55:04PM +0100, Fabien COELHO wrote:

Here is my review about v32:

I forgot to tell that doc generation for the cumulated changes also works.

Unfortunately the patchset doesn't apply anymore:

http://cfbot.cputube.org/patch_36_2377.log
=== Applying patches on top of PostgreSQL commit ID 4483b2cf29bfe8091b721756928ccbe31c5c8e14 ===
=== applying patch ./v32-0003-Add-pg_ls_dir_metadata-to-list-a-dir-with-file-m.patch
[...]
patching file src/test/regress/expected/misc_functions.out
Hunk #1 succeeded at 274 (offset 7 lines).
patching file src/test/regress/expected/tablespace.out
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file src/test/regress/expected/tablespace.out.rej

Justin, could you send a rebased version?

On Sun, Jan 02, 2022 at 01:07:29PM +0100, Fabien COELHO wrote:

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

The change is about the "isflag" flag, not creation time.

         Returns a record containing the file's size, last access time stamp,
         last modification time stamp, last file status change time stamp (Unix
         platforms only), file creation time stamp (Windows only), and a flag
-        indicating if it is a directory.
+        indicating if it is a directory (or a symbolic link to a directory).

# part 03
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?

No, what you suggested is not the same;

We talked about this before:
/messages/by-id/20200315212729.GC26184@telsasoft.com

Otherwise, at least "else if (3 & 4) continue"?

I could write the *final* "else if" like that, but then it would be different
from the previous case. Which would be confusing and prone to mistakes.

If I wrote it like this, I think it'd just provoke suggestions from someone
else to change it differently:

/* Skip dirs or special files? */
if (S_ISDIR(attrib.st_mode) && !(flags & LS_DIR_SKIP_DIRS))
continue;
if (!S_ISDIR(attrib.st_mode) && !S_ISREG(attrib.st_mode) && !(flags & LS_DIR_SKIP_SPECIAL)
continue;

...
<< Why don't you use "else if" instead of "if (a){} if (!a && b){}" >>

I'm going to leave it up to a committer.

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

I changed this but without using nested conditionals.

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.

Because that depends on lstat().

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.

I agree that makes sense, however others have expressed the opposite opinion.
/messages/by-id/CALj2ACWtrt5EkHrY4WAZ4Cv42SidXAwpeQJU021bxaKpjmbGfA@mail.gmail.com

The original motive for the patch was that pg_ls_tmpdir doesn't show shared
filesets. This fixes that essential problem without immediately dragging
everything else along. I think it's more likely that a committer would merge
them both. But I don't know, and it's easy to combine patches if desired.

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 have added tests, although some are duplicative.

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.

It's seperate to allow writing a separate commit message since it does
something unrelated to the other patches. What other patch would it would be
merged with ?
| v32-0006-pg_ls_logdir-to-ignore-error-if-initial-top-dir-.patch

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

I preferred to refer to pg_stat_file rather than repeat it for all 7 functions
currently in v15, (and future functions added for new, toplevel dirs).

# part 11

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

WDYM the true value? It's tested like:

+-- Exercise recursion
+select path, filename, type from pg_ls_dir_metadata('.', true, false, true) where
+path in ('base', 'base/pgsql_tmp', 'global', 'global/pg_control', 'global/pg_filenode.map', 'PG_VERSION', 'pg_multixact', 'pg_multixact/members', 'pg_multixact/offsets', 'pg_wal', 'pg_wal/archive_status')
+-- (type='d' or path~'^(global/.*|PG_VERSION|postmaster\.opts|postmaster\.pid|pg_logical/replorigin_checkpoint)$') and filename!~'[0-9]'
+order by path collate "C", filename collate "C";
+          path          |    filename     | type 
+------------------------+-----------------+------
+ PG_VERSION             | PG_VERSION      | -
+ base                   | base            | d
+ base/pgsql_tmp         | pgsql_tmp       | d
...

--
Justin

Rebased over 9e9858389 (Michael may want to look at the tuplestore part?).

Fixing a comment typo.

I also changed pg_ls_dir_recurse() to handle concurrent removal of a dir, which
I noticed caused an infrequent failure on CI. However I'm not including that
here, since the 2nd half of the patch set seems isn't ready due to lstat() on
windows.

Hello Justin,

I hope to look at it over the week-end.

--
Fabien Coelho - CRI, MINES ParisTech

Hello Justin,

Review about v34, up from v32 which I reviewed in January. v34 is an
updated version of v32, without the part about lstat at the end of the
series.

All 7 patches apply cleanly.

# part 01

One liner doc improvement about the directory flag.

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.

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 that, however I must say that I'm still unsure why we would
not jump directly to a "type" char column. What is wrong with outputing
'd' or '-' instead of true or false? This way, the interface needs not
change if "lstat" is used later? ISTM that the interface issue should be
somehow independent of the implementation issue, and we should choose
directly the right/best interface?

Independently, the documentation may be clearer about what happens to
"isdir" when the file is a link? It may say that the behavior may change
in the future?

About homogeneity, I note that some people may be against adding "isdir"
to other ls functions. I must say that I cannot see a good argument not to
do it, and that I hate dealing with systems which are not homogeneous
because it creates surprises and some loss of time.

"make check" ok.

OK.

# part 05

Add isdir to other ls functions. Doc is updated.

Same as above, I'd prefer a char instead of a bool, as it is more extendable and
future-proof.

OK.

# part 06

This part extends and adds a test for pg_ls_logdir.
"make check" is ok.

OK.

# part 07

This part extends pg_stat_file with more date informations.

"make check" is ok.

OK.

# doc

Overall doc generation is OK.

--
Fabien.

On Wed, Mar 09, 2022 at 10:50:45AM -0600, Justin Pryzby wrote:

Rebased over 9e9858389 (Michael may want to look at the tuplestore part?).

Are you referring to the contents of 0003 here that changes the
semantics of pg_ls_dir_files() regarding its setup call?
--
Michael

On Sun, Mar 13, 2022 at 09:45:35AM +0900, Michael Paquier wrote:

On Wed, Mar 09, 2022 at 10:50:45AM -0600, Justin Pryzby wrote:

Rebased over 9e9858389 (Michael may want to look at the tuplestore part?).

Are you referring to the contents of 0003 here that changes the
semantics of pg_ls_dir_files() regarding its setup call?

Yes, as it has this:

-       SetSingleFuncCall(fcinfo, SRF_SINGLE_USE_EXPECTED);                                                                                                                                                                       
...
-       SetSingleFuncCall(fcinfo, 0);                                                                                                                                                                                             
...
+       if (flags & LS_DIR_METADATA)                                                                                                                                                                                              
+               SetSingleFuncCall(fcinfo, 0);                                                                                                                                                                                     
+       else                                                                                                                                                                                                                      
+               SetSingleFuncCall(fcinfo, SRF_SINGLE_USE_EXPECTED);                                                                                                                                                               

--
Justin

On Wed, Mar 09, 2022 at 10:50:45AM -0600, Justin Pryzby wrote:

I also changed pg_ls_dir_recurse() to handle concurrent removal of a dir, which
I noticed caused an infrequent failure on CI. However I'm not including that
here, since the 2nd half of the patch set seems isn't ready due to lstat() on
windows.

lstat() has been a subject of many issues over the years with our
internal emulation and issues related to its concurrency, but we use
it in various areas of the in-core code, so that does not sound like
an issue to me. It depends on what you want to do with it in
genfile.c and which data you'd expect, in addition to the detection of
junction points for WIN32, I guess. v34 has no references to
pg_ls_dir_recurse(), but that's a WITH RECURSIVE, so we would not
really need it, do we?

@@ -27618,7 +27618,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
         Returns a record containing the file's size, last access time stamp,
         last modification time stamp, last file status change time stamp (Unix
         platforms only), file creation time stamp (Windows only), and a flag
-        indicating if it is a directory.
+        indicating if it is a directory (or a symbolic link to a directory).
        </para>
        <para>
         This function is restricted to superusers by default, but other users

This is from 0001, and this addition in the documentation is not
completely right. As pg_stat_file() uses stat() to get back the
information of a file/directory, we'd just follow the link if
specifying one in the input argument. We could say instead, if we
were to improve the docs, that "If filename is a link, this function
returns information about the file or directory the link refers to."
I would put that as a different paragraph.

+select * from pg_ls_archive_statusdir() limit 0;
+ name | size | modification 
+------+------+--------------
+(0 rows)

FWIW, this one is fine as of ValidateXLOGDirectoryStructure() that
would make sure archive_status exists before any connection is
attempted to the cluster.

+select * from pg_ls_logdir() limit 0;

This test on pg_ls_logdir() would fail if running installcheck on a
cluster that has logging_collector disabled. So this cannot be
included.

+select * from pg_ls_logicalmapdir() limit 0;
+select * from pg_ls_logicalsnapdir() limit 0;
+select * from pg_ls_replslotdir('') limit 0;
+select * from pg_ls_tmpdir() limit 0;
+select * from pg_ls_waldir() limit 0;
+select * from pg_stat_file('.') limit 0;

The rest of the patch set should be stable AFAIK, there are various
steps when running a checkpoint that makes sure that any of these
exist, without caring about the value of wal_level.

+       <para>
+        For each file in the specified directory, list the file and its
+        metadata.
+        Restricted to superusers by default, but other users can be granted
+        EXECUTE to run the function.
+       </para></entry>

What is metadata in this case? (I have read the code and know what
you mean, but folks only looking at the documentation may be puzzled
by that). It could be cleaner to use the same tupledesc for any
callers of this function, and return NULL in cases these are not
adapted.

+   /* check the optional arguments */
+   if (PG_NARGS() == 3)
+   {
+       if (!PG_ARGISNULL(1))
+       {
+           if (PG_GETARG_BOOL(1))
+               flags |= LS_DIR_MISSING_OK;
+           else
+               flags &= ~LS_DIR_MISSING_OK;
+       }
+
+       if (!PG_ARGISNULL(2))
+       {
+           if (PG_GETARG_BOOL(2))
+               flags &= ~LS_DIR_SKIP_DOT_DIRS;
+           else
+               flags |= LS_DIR_SKIP_DOT_DIRS;
+       }
+   }

The subtle different between the false and true code paths of those
arguments 1 and 2 had better be explained? The bit-wise operations
are slightly different in both cases, so it is not clear which part
does what, and what's the purpose of this logic.

-   SetSingleFuncCall(fcinfo, 0);
+   /* isdir depends on metadata */
+   Assert(!(flags&LS_DIR_ISDIR) || (flags&LS_DIR_METADATA));
+   /* Unreasonable to show isdir and skip dirs */
+   Assert(!(flags&LS_DIR_ISDIR) || !(flags&LS_DIR_SKIP_DIRS));

Incorrect code format. Spaces required.

+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to
succeed even if the tmpdir doesn't exist yet
+-- The name='' condition is never true, so the function runs to
completion but returns zero rows.
+-- The query is written to ERROR if the tablespace doesn't exist,
rather than silently failing to call pg_ls_tmpdir()
+SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE
b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1)
AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist';

So, here, we have a test that may not actually test what we want to
test.

Hmm. I am not convinced that we need a new set of SQL functions as
presented in 0003 (addition of meta-data in pg_ls_dir()), and
extensions of 0004 (do the same but for pg_ls_tmpdir) and 0005 (same
for the other pg_ls* functions). The changes of pg_ls_dir_files()
make in my opinion the code harder to follow as the resulting tuple
size depends on the type of flag used, and you can already retrieve
the rest of the information with a join, probably LATERAL, on
pg_stat_file(), no? The same can be said about 0007, actually.

The addition of isdir for any of the paths related to pg_logical/ and
the replication slots has also a limited interest, because we know
already those contents, and these are not directories as far as I
recall.

0006 invokes a behavior change for pg_ls_logdir(), where it makes
sense to me to fail if the directory does not exist, so I am not in
favor of that.

In the whole set, improving the docs as of 0001 makes sense, but the
change is incomplete. Most of 0002 also makes sense and should be
stable enough. I am less enthusiastic about any of the other changes
proposed and what we can gain from these parts.
--
Michael