Subject: Re: [PATCH v1] pg_ls_tmpdir to show directories

From: David Steele <[email-masked]>
Date: Tue, 3 Mar 2020 14:51:54 -0500

Hi Fabien,

On 1/16/20 9:38 AM, Justin Pryzby wrote:
> On Thu, Jan 16, 2020 at 09:34:32AM +0100, Fabien COELHO wrote:
>> Also, I'm not fully sure why ".*" files should be skipped, maybe it should
>> be an option? Or the user can filter it with SQL if it does not want them?
> 
> I think if someone wants the full generality, they can do this:
> 
> postgres=# SELECT name, s.size, s.modification, s.isdir FROM (SELECT 'base/pgsql_tmp'p)p, pg_ls_dir(p)name, pg_stat_file(p||'/'||name)s;
>   name | size |      modification      | isdir
> ------+------+------------------------+-------
>   .foo | 4096 | 2020-01-16 08:57:04-05 | t
> 
> In my mind, pg_ls_tmpdir() is for showing tmpfiles, not just a shortcut to
> SELECT pg_ls_dir((SELECT 'base/pgsql_tmp'p)); -- or, for all tablespaces:
> WITH x AS (SELECT format('/PG_%s_%s', split_part(current_setting('server_version'), '.', 1), catalog_version_no) suffix FROM pg_control_system()), y AS (SELECT a, pg_ls_dir(a) AS d FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(oid),'')||suffix, 'base') a FROM pg_tablespace,x)a) SELECT a, pg_ls_dir(a||'/pgsql_tmp') FROM y WHERE d='pgsql_tmp';
> 
> I think changing dotfiles is topic for another patch.
> That would also affect pg_ls_dir, and everything else that uses the backing
> function pg_ls_dir_files_recurse.  I'd have to ask why not also show . and .. ?
> 
> (In fact, if I were to change anything, I would propose to limit pg_ls_tmpdir()
> to files matching PG_TEMP_FILE_PREFIX).

We seem to be at an impasse on this patch.  What do you think of 
Justin's comments here?

Do you still believe a different implementation is required?

Regards,
-- 
-David
[email-masked]

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 3 Mar 2020 14:01:17 -0600

On Tue, Mar 03, 2020 at 02:51:54PM -0500, David Steele wrote:
> We seem to be at an impasse on this patch.  What do you think of Justin's
> comments here?

Actually, I found Fabien's comment regarding extensions use of tmp dir to be
convincing.  And I'm willing to update the patch to use a stack to show
arbitrarily-deep files/dirs rather than just one level deep (as used for shared
filesets in core postgres).

But I don't think it makes sense to go through more implementation/review
cycles without some agreement from a larger group regarding the
desired/intended interface.  Should there be a column for "parent dir" ?  Or a
column for "is_dir" ?  Should dirs be shown at all, or only files ?

-- 
Justin

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 3 Mar 2020 17:23:13 -0300

> column for "is_dir" ?  Should dirs be shown at all, or only files ?

IMO: is_dir should be there (and subdirs should be listed), but
parent_dir should not appear.  Also, the "path" should show the complete
pathname, including containing dirs, starting from whatever the "root"
is for the operation.

So for the example in the initial email, it would look like

path					isdir
pgsql_tmp11025.0.sharedfileset/		t
pgsql_tmp11025.0.sharedfileset/0.0	f
pgsql_tmp11025.0.sharedfileset/1.0	f

plus additional columns, same as pg_ls_waldir et al.

I'd rather not have the code assume that there's a single level of
subdirs, or assuming that an entry in the subdir cannot itself be a dir;
that might end up hiding files for no good reason.

I don't understand what purpose is served by having pg_ls_waldir() hide
directories.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 5 Mar 2020 10:18:38 -0600

> that might end up hiding files for no good reason.

Thanks for your input, see attached.

I'm not sure if prefer the 0002 patch alone (which recurses into dirs all at
once during the initial call), or 0002+3+4, which incrementally reads the dirs
on each call (but requires keeping dirs opened).

> I don't understand what purpose is served by having pg_ls_waldir() hide

We could talk about whether the other functions should show dirs, if it's worth
breaking their return type.  Or if they should show hidden or special files,
which doesn't require breaking the return.  But until then I am to leave the
behavior alone.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 6 Mar 2020 17:35:07 -0600

> on each call (but requires keeping dirs opened).

I fixed an issue that leading dirs were being shown which should not have been,
which was easier in the 0004 patch, so squished.  And fixed a bug that
"special" files weren't excluded, and "missing_ok" wasn't effective.

> which doesn't require breaking the return.  But until then I am to leave the

I don't see why any of the functions would exclude dirs, but ls_tmpdir deserves
to be fixed since core postgres dynamically creates dirs there.

Also ... I accidentally changed the behavior: master not only doesn't decend
into dirs, it hides them - that was my original complaint.  I propose to *also*
change at least tmpdir and logdir to show dirs, but don't decend.  I left
waldir alone for now.

Since v12 ls_tmpdir and since v10 logdir and waldir exclude dirs, I think we
should backpatch documentation to say so.

ISTM pg_ls_tmpdir and ls_logdir should be called with missing_ok=true, since
they're not created until they're used.

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Sat, 7 Mar 2020 15:14:37 +0100 (CET)

Hello Justin,

Some feedback about the v7 patch set.

About v7.1, seems ok.

About v7.2 & v7.3 seems ok, altought the two could be merged.

About v7.4:

The documentation sentences could probably be improved "for for", "used 
... used". Maybe:

   For the temporary directory for <parameter>tablespace</parameter>, ...
->
   For <parameter>tablespace</parameter> temporary directory, ...

   Directories are used for temporary files used by parallel
   processes, and are shown recursively.
->
   Directories holding temporary files used by parallel
   processes are shown recursively.

It seems that lists are used as FIFO structures by appending, fetching & 
deleting last, all of which are O(n). ISTM it would be better to use the 
head of the list by inserting, getting and deleting first, which are O(1).

ISTM that several instances of: "pg_ls_dir_files(..., true, false);" 
should be "pg_ls_dir_files(..., true, DIR_HIDE);".

About v7.5 looks like a doc update which should be merged with v7.4.

Alas, ISTM that there are no tests on any of these functions:-(

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 7 Mar 2020 11:10:36 -0600

> Some feedback about the v7 patch set.

Thanks for looking again

> About v7.2 & v7.3 seems ok, altought the two could be merged.

These are separate since I proprose that one should be backpatched to v12 and
the other to v10.

> About v7.4:
...
> head of the list by inserting, getting and deleting first, which are O(1).

I think you're referring to linked lists, but pglists are now arrays, for which
that's backwards.  See 1cff1b95a and d97b714a2.  For example, list_delete_last
says:
 * This is the opposite of list_delete_first(), but is noticeably cheaper
 * with a long list, since no data need be moved.

> ISTM that several instances of: "pg_ls_dir_files(..., true, false);" should
> be "pg_ls_dir_files(..., true, DIR_HIDE);".

Oops, that affects an intermediate commit and maybe due to merge conflict.
Thanks.

> About v7.5 looks like a doc update which should be merged with v7.4.

No, v7.5 updates pg_proc.dat and changes the return type of two functions.
It's a short commit since all the infrastructure is implemented to make the
functions do whatever we want.  But it's deliberately separate since I'm
proposing a breaking change, and one that hasn't been discussed until now.

> Alas, ISTM that there are no tests on any of these functions:-(

Yeah.  Everything that includes any output is going to include timestamps;
those could be filtered out.  waldir is going to have random filenames, and a
differing number of rows.  But we should exercize pg_ls_dir_files at least
once..

My previous version had a bug with ignore_missing with pg_ls_tmpdir, which
would've been caught by a test like:
SELECT FROM pg_ls_tmpdir() WHERE name='Does not exist'; -- Never true, so the function runs to completion but returns zero rows.

The 0006 commit changes that for logdir, too.  Without 0006, that will ERROR if
the dir doesn't exist (which I think would be the default during regression
tests).

It'd be nice to run pg_ls_tmpdir before the tmpdir exists, and again
afterwards.  But I'm having trouble finding a single place to put it.  The
closest I can find is dbsize.sql.  Any ideas ?

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Sat, 7 Mar 2020 18:40:08 +0100 (CET)

> I think you're referring to linked lists, but pglists are now arrays,

Ok… I forgot about this change, so my point is void, you took the right 
one.

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 7 Mar 2020 15:40:10 -0600

On Sat, Mar 07, 2020 at 03:14:37PM +0100, Fabien COELHO wrote:
> The documentation sentences could probably be improved "for for", "used ...
> used". Maybe:

> Alas, ISTM that there are no tests on any of these functions:-(

Addressed these.

And reordered the last two commits to demonstrate and exercize the behavior
change in regress test.

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Sun, 8 Mar 2020 09:02:19 +0100 (CET)

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.

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 10 Mar 2020 13:30:37 -0500

I took a step back, and I wondered whether we should add a generic function for
listing a dir with metadata, possibly instead of changing the existing
functions.  Then one could do pg_ls_dir_metadata('pg_wal',false,false);

Since pg8.1, we have pg_ls_dir() to show a list of files.  Since pg10, we've
had pg_ls_logdir and pg_ls_waldir, which show not only file names but also
(some) metadata (size, mtime).  And since pg12, we've had pg_ls_tmpfile and
pg_ls_archive_statusdir, which also show metadata.

...but there's no a function which lists the metadata of an directory other
than tmp, wal, log.

One can do this:
|SELECT b.*, c.* FROM (SELECT 'base' a)a, LATERAL (SELECT a||'/'||pg_ls_dir(a.a)b)b, pg_stat_file(b)c;
..but that's not as helpful as allowing:
|SELECT * FROM pg_ls_dir_metadata('.',true,true);

There's also no function which recurses into an arbitrary directory, so it
seems shortsighted to provide a function to recursively list a tmpdir.

Also, since pg_ls_dir_metadata indicates whether the path is a dir, one can
write a SQL function to show the dir recursively.  It'd be trivial to plug in
wal/log/tmp (it seems like tmpdirs of other tablespace's are not entirely
trivial).
|SELECT * FROM pg_ls_dir_recurse('base/pgsql_tmp');

Also, on a neighboring thread[1], Tom indicated that the pg_ls_* functions
should enumerate all files during the initial call, which sounds like a bad
idea when recursively showing directories.  If we add a function recursing into
a directory, we'd need to discuss all the flags to expose to it, like recurse,
ignore_errors, one_filesystem?, show_dotfiles (and eventually bikeshed all the
rest of the flags in find(1)).

My initial patch [2] changed ls_tmpdir to show metadata columns including
is_dir, but not decend.  It's pretty unfortunate if a function called
pg_ls_tmpdir hides shared filesets, so maybe it really is best to change that
(it's new in v12).

I'm interested to in feedback on the alternative approach, as attached.  The
final patch to include all the rest of columns shown by pg_stat_file() is more
of an idea/proposal and not sure if it'll be desirable.  But pg_ls_tmpdir() is
essentially the same as my v1 patch.

This is intended to be mostly independent of any fix to the WARNING I reported
[1].  Since my patch collapses pg_ls_dir into pg_ls_dir_files, we'd only need
to fix one place.  I'm planning to eventually look into Tom's suggestion of
returning tuplestore to fix that, and maybe rebase this patchset on top of
that.

-- 
Justin

[1] https://www.postgresql.org/message-id/flat/20200308173103.GC1357%40telsasoft.com
[2] https://www.postgresql.org/message-id/20191214224735.GA28433%40telsasoft.com

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 13 Mar 2020 08:12:32 -0500

@cfbot: rebased onto 085b6b6679e73b9b386f209b4d625c7bc60597c0

The merge conflict presents another opportunity to solicit comments on the new
approach.  Rather than making "recurse into tmpdir" the end goal:

  - add a function to show metadata of an arbitrary dir;
  - add isdir arguments to pg_ls_* functions (including pg_ls_tmpdir but not
    pg_ls_dir).
  - maybe add pg_ls_dir_recurse, which satisfies the original need;
  - retire pg_ls_dir (does this work with tuplestore?)
  - profit

The alternative seems to be to go back to Alvaro's earlier proposal:
 - not only add "isdir", but also recurse;

I think I would insist on adding a general function to recurse into any dir.
And *optionally* change ps_ls_* to recurse (either by accepting an argument, or
by making that a separate patch to debate).  tuplestore is certainly better
than keeping a stack/List of DIRs for this.

---

From: Fabien COELHO <[email-masked]>
Date: Sun, 15 Mar 2020 18:15:02 +0100 (CET)

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.

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 15 Mar 2020 16:27:29 -0500

> Some feedback on v10:

Thanks for looking.  I'm hoping to hear from Alvaro what he thinks of this
approach (all functions to show isdir, rather than one function which lists
recursively).

> would be less than 9 patches…

I kept them separate to allow the earlier patches to be applied.
And intended to make easier to review, even if it's more work for me..

If you mean that it's a pain to apply 9 patches, I will suggest to use:
|git am -3 ./mailbox
where ./mailbox is either a copy of the mail you received, or retrieved from
the web interface.

To test that each one works (compiles, passes tests, etc), I use git rebase -i
HEAD~11 and "e"edit the target (set of) patches.

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

As I mentioned, separate since I'm proposing that they're backpatched to
different releases.  Those could be applied now (and Tom already applied a
patch identical to 0001 in a prior patchset).

> 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?

The tmpdir *doesn't* exist until someone creates tmpfiles there.
As it mentions:
+-- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet

> local. I'd suggest "LS_DIR_*", maybe, as a more specific prefix.

Done.

> FLAG_MISSING_OK)" and other instances.

Partially took your suggestion.

>  if (S_ISDIR(attrib.st_mode) && (flags & FLAG_SKIP_DIRS))

That's not the same - if SKIP_DIRS isn't set, your way would fail that test for
dirs, and then hit the !ISREG test, and skip them anyway.
|else if (!S_ISREG(attrib.st_mode))
|	continue

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

Done

>        Assert(!(flags&FLAG_ISDIR) || (flags&FLAG_METADATA));

It's not supported to show isdir without showing metadata (because that case
isn't needed to support the old and the new behaviors).

+               if (flags & FLAG_METADATA)
+               {
+                       values[1] = Int64GetDatum((int64) attrib.st_size);
+                       values[2] = TimestampTzGetDatum(time_t_to_timestamptz(attrib.st_mtime));
+                       if (flags & FLAG_ISDIR)
+                               values[3] = BoolGetDatum(S_ISDIR(attrib.st_mode));
+               }

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

It's unimportant, but testing for intended use of flags during development.

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

Maybe.  I put them back...but it means they're not being exercized by any
*used* case.

> caught more cleanly later in the conditional structure.

This went away when I put back the SKIP_DIRS flag.

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

Why you think it's contrived?  Listing a tmpdir recursively is the initial
motivation of this patch.  Maybe you think I should list just the tmpdir for
one tablespace ?  Note that for temp_tablespaces parameter:

|When there is more than one name in the list, PostgreSQL chooses a random member of the list each time a temporary object is to be created; except that within a transaction, successively created temporary objects are placed in successive tablespaces from the list.

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

I used "" because of this:
| x.name||'/'||a.name
I don't know if there's a better way to join paths in SQL, or if that suggests
this is a bad way to do it.

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

I used: ./src/include/catalog/unused_oids (maybe not correctly).

> I do not understand why UNION ALL and not UNION.

In general, union ALL can avoid a "distinct" plan node, but it doesn't seem to
have any effect here.

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

Done

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

fixed, thanks.

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

I added something which would've caught the issue with number of arguments.

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

I think it makes sense to allow ls_logdir to succeed even if ./log doesn't
exist, since it isn't created by initdb or during postmaster start, and since
we already using MISSING_OK for tmpdir.

But a separate patch since we didn't previous discuss changing logdir.  

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

This is the last patch in the series, since I think it's least likely to be
agreed on.

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Mon, 16 Mar 2020 16:20:21 +0100 (CET)

About v11, ISTM that the recursive function should check for symbolic 
links and possibly avoid them:

  sh> cd data/base
  sh> ln -s .. foo

  psql> SELECT * FROM pg_ls_dir_recurse('.');
  ERROR:  could not stat file "./base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo/base/foo": Too many levels of symbolic links
  CONTEXT:  SQL function "pg_ls_dir_recurse" statement 1

This probably means using lstat instead of (in supplement to?) stat, and 
probably tell if something is a link, and if so not recurse in them.

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 16 Mar 2020 10:41:36 -0500

On Mon, Mar 16, 2020 at 04:20:21PM +0100, Fabien COELHO wrote:
> 
> About v11, ISTM that the recursive function should check for symbolic links
> and possibly avoid them:
> 
>  sh> cd data/base
>  sh> ln -s .. foo
> 

Thanks for looking.

I think that opens up a can of worms.  I don't want to go into the business of
re-implementing all of find(1) - I count ~128 flags (most of which take
arguments).  You're referring to find -L vs find -P, and some people would want
one and some would want another.  And don't forget about find -H...

pg_stat_file doesn't expose the file type (I guess because it's not portable?),
and I think it's outside the scope of this patch to change that.  Maybe it
suggests that the pg_ls_dir_recurse patch should be excluded.

ISTM if someone wants to recursively list a directory, they should avoid
putting cycles there, or permission errors, or similar.  Or they should write
their own C extension that borrows from pg_ls_dir_files but handles more
arguments.

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Mon, 16 Mar 2020 19:21:06 +0100 (CET)

Hello Justin,

> one and some would want another.  And don't forget about find -H...

This is not the point. The point is that a link can change a finite tree 
into cyclic graph, and you do not want to delve into that, ever.

The "find" command, by default, does not recurse into a link because of 
said problem, and the user *must* ask for it and assume the infinite loop 
if any.

So if you implement one behavior, it should be not recursing into links. 
Franckly, I would not provide the recurse into link alternative, but it 
could be implemented if someone wants it, and the problem that come with 
it.

> pg_stat_file doesn't expose the file type (I guess because it's not portable?),

You are right that Un*x and Windows are not the same wrt link. It seems 
that there is already something about that in port:

   "./src/port/dirmod.c:pgwin32_is_junction(const char *path)"

So most of the details are already hidden.

> suggests that the pg_ls_dir_recurse patch should be excluded.

IMHO, I really think that it should be included. Dealing with links is no 
big deal, but you need an additional column in _metadata to tell it is a 
link, and there is a ifdef because testing is a little different between 
unix and windows. I'd guess around 10-20 lines of code added.

> putting cycles there, or permission errors, or similar.

Hmmm. I'd say the user should like to be able to call the function and 
never have a bad experience with it such as a failure on an infinite loop.

> Or they should write their own C extension that borrows from 
> pg_ls_dir_files but handles more arguments.

ISTM that the point of your patch is to provide the basic tool needed to 
list directories contents, and handling links somehow is a necessary part 
of that.

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 16 Mar 2020 16:48:06 -0500

On Mon, Mar 16, 2020 at 07:21:06PM +0100, Fabien COELHO wrote:
> link

Instead of showing another column, I changed to show links with isdir=false.
At the cost of two more patches, to allow backpatching docs and maybe separate
commit to make the subtle change obvious in commit history, at least.

I see a few places in the backend and a few more in the fronted using the same
logic that I used for islink(), but I'm not sure if there's a good place to put
that to allow factoring out at least the other backend ones.

-- 
Justin

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 16 Mar 2020 19:17:36 -0300

I pushed 0001 and 0003 (as a single commit).  archive_statusdir didn't
get here until 12, so your commit message was mistaken.  Also, pg10 is
slightly different so it didn't apply there, so I left it alone.

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 16 Mar 2020 22:14:27 -0500

> slightly different so it didn't apply there, so I left it alone.

Thanks, I appreciate it (and I'm sure Fabien will appreciate having two fewer
patches...).

@cfbot: rebased onto b4570d33aa045df330bb325ba8a2cbf02266a555

I realized that if I lstat() a file to make sure links to dirs show as
isdir=false, it's odd to then show size and timestamps of the dir.  So changed
to use lstat ... and squished.

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Tue, 17 Mar 2020 10:21:48 +0100 (CET)

About v13, seens as one patch:

Function "pg_ls_dir_metadata" documentation suggests a variable number of 
arguments with brackets, but parameters are really mandatory.

  postgres=# SELECT pg_ls_dir_metadata('.');
  ERROR:  function pg_ls_dir_metadata(unknown) does not exist
  LINE 1: SELECT pg_ls_dir_metadata('.');
                ^
  HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
  postgres=# SELECT pg_ls_dir_metadata('.', true, true);
  …

The example in the documentation could be better indented. Also, ISTM that 
there are two implicit laterals (format & pg_ls_dir_recurse) that I would 
make explicit. I'd use the pcs alias explicitely. I'd use meaningful 
aliases (eg ts instead of b, …).

On reflection, I think that a boolean "isdir" column is a bad idea because 
it is not extensible. I'd propose to switch to the standard "ls" approach 
of providing the type as one character: '-' for regular, 'd' for 
directory, 'l' for link, 's' for socket, 'c' for character special…

ISTM that "lstat" is not available on windows, which suggests to call 
"stat" always, and then "lstat" on un*x and pg ports stuff on win.

I'm wondering about the restriction on directories only. Why should it not 
work on a file? Can it be easily extended to work on a simple file? If so, 
it could be just "pg_ls".

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 17 Mar 2020 14:04:01 -0500

> arguments with brackets, but parameters are really mandatory.

Fixed, and added tests on 1 and 3 arg versions of both pg_ls_dir() and
pg_ls_dir_metadata().

It seems like the only way to make variable number of arguments is is with
multiple entries in pg_proc.dat, one for each "number of" arguments.  Is that
right ?

> make explicit. I'd use the pcs alias explicitely. I'd use meaningful aliases
> (eg ts instead of b, …).

> it is not extensible. I'd propose to switch to the standard "ls" approach of
> providing the type as one character: '-' for regular, 'd' for directory, 'l'
> for link, 's' for socket, 'c' for character special…

I think that's outside the scope of the patch, since I'd want to change
pg_stat_file; that's where I borrowed "isdir" from, for consistency.

Note that both LS_DIR_HISTORIC and LS_DIR_MODERN include LS_DIR_SKIP_SPECIAL,
so only pg_ls_dir itself show specials, so they way to do it would be to 1)
change pg_stat_file to expose the file's "type", 2) use pg_ls_dir() AS a,
lateral pg_stat_file(a) AS b, 3) then consider also changing LS_DIR_MODERN and
all the existing pg_ls_*.

> ISTM that "lstat" is not available on windows, which suggests to call "stat"
> always, and then "lstat" on un*x and pg ports stuff on win.

I believe that's handled here.
src/include/port/win32_port.h:#define lstat(path, sb) stat(path, sb)

> it could be just "pg_ls".

I think that's a good idea, except it doesn't fit with what the code does:
AllocDir() and ReadDir().  Instead, use pg_stat_file() for that.

Hm, I realized that the existing pg_ls_dir_metadata was skipping links to dirs,
since !ISREG().  So changed to use both stat() and lstat().

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Tue, 17 Mar 2020 15:11:01 -0400

> multiple entries in pg_proc.dat, one for each "number of" arguments.  Is that

Another way to do it is to have one entry, put the full set of arguments
into the initial pg_proc.dat data, and then use CREATE OR REPLACE FUNCTION
later during initdb to install some defaults.  See existing cases in
system_views.sql, starting about line 1180.  Neither way is especially
pretty, so take your choice.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 31 Mar 2020 15:08:12 -0500

> all the existing pg_ls_*.

The patch intends to fix the issue of "failing to show failed filesets"
(because dirs are skipped) while also generalizing existing functions (to show
directories and "isdir" column) and providing some more flexible ones (to list
file and metadata of a dir, which is currently possible [only] for "special"
directories, or by recursively calling pg_stat_file).

I'm still of the opinion that supporting arbitrary file types is out of scope,
but I changed the "isdir" to show "type".  I'm only supporting '[-dl]'.  I
don't want to have to check #ifdef S_ISDOOR or whatever other vendors have.  I
insist that it is a separate patch, since it depends on everything else, and I
have no feedback from anybody else as to whether any of that is desired.

template1=# SELECT * FROM pg_ls_waldir();
           name           |   size   |         access         |      modification      |         change         | creation | type 
--------------------------+----------+------------------------+------------------------+------------------------+----------+------
 barr                     |        0 | 2020-03-31 14:43:11-05 | 2020-03-31 14:43:11-05 | 2020-03-31 14:43:11-05 |          | ?
 baz                      |     4096 | 2020-03-31 14:39:18-05 | 2020-03-31 14:39:18-05 | 2020-03-31 14:39:18-05 |          | d
 foo                      |        0 | 2020-03-31 14:39:37-05 | 2020-03-31 14:39:37-05 | 2020-03-31 14:39:37-05 |          | -
 archive_status           |     4096 | 2020-03-31 14:38:20-05 | 2020-03-31 14:38:18-05 | 2020-03-31 14:38:18-05 |          | d
 000000010000000000000001 | 16777216 | 2020-03-31 14:42:53-05 | 2020-03-31 14:43:08-05 | 2020-03-31 14:43:08-05 |          | -
 bar                      |        3 | 2020-03-31 14:39:16-05 | 2020-03-31 14:39:01-05 | 2020-03-31 14:39:01-05 |          | l

---

From: Fabien COELHO <[email-masked]>
Date: Sun, 12 Apr 2020 13:53:40 +0200 (CEST)

Hello Justin,

About v15, seen as one patch.

Patches serie applies cleanly, compiles, "make check" ok.

Documentation:
  - indent documentation text around 80 cols, as done around?
  - indent SQL example for readability and capitalize keywords
    (pg_ls_dir_metadata)
  - "For each file in a directory, list the file and its metadata."
    maybe: "List files and their metadata in a directory"?

Code:
  - Most pg_ls_*dir* functions call pg_ls_dir_files(), which looks like
    reasonable refactoring, ISTM that the code is actually smaller.
  - please follow pg style, eg not "} else {"
  - there is a "XXX" (meaning fixme?) tag remaining in a comment.
  - file types: why not do block & character devices, fifo and socket
    as well, before the unkown case?
  - I'm wondering whether could pg_stat_file call pg_ls_dir_files without
    too much effort? ISTM that the output structure nearly the same. I do
    not like much having one function specialized for files and one for
    directories.

Tests:
  - good, there are some!
  - indent SQL code, eg by starting a new line on new clauses?
  - put comments on separate lines (I'm not against it on principle, I do
    that, but I do not think that it is done much in test files).

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 2 May 2020 21:42:15 -0500

Thanks for looking.

>   not like much having one function specialized for files and one for

I refactored but not like that.  As I mentioned in the commit message, I don't
see a good way to make a function operate on a file when the function's primary
data structure is a DIR*.  Do you ?  I don't think it should call stat() and
then conditionally branch off to pg_stat_file().

There are two functions because they wrap two separate syscalls, which see as
good, transparent goal.  If we want a function that does what "ls -al" does,
that would also be a good example to follow, except that we already didn't
follow it.

/bin/ls first stat()s the path, and then either outputs its metadata (if it's a
file or if -d was specified) or lists a dir.  It's essentially a wrapper around
*two* system calls (stat and readdir/getdents).

Maybe we could invent a new pg_ls() which does that, and then refactor existing
code.  Or, maybe it would be a SQL function which calls stat() and then
conditionally calls pg_ls_dir if isdir=True (or type='d').  That would be easy
if we merge the commit which outputs all stat fields.

I'm still hoping for confirmation from a committer if this approach is worth
pursuing:

https://www.postgresql.org/message-id/20200310183037.GA29065%40telsasoft.com
https://www.postgresql.org/message-id/20200313131232.GO29065%40telsasoft.com
|Rather than making "recurse into tmpdir" the end goal:
|
|  - add a function to show metadata of an arbitrary dir;
|  - add isdir arguments to pg_ls_* functions (including pg_ls_tmpdir but not
|    pg_ls_dir).
|  - maybe add pg_ls_dir_recurse, which satisfies the original need;
|  - retire pg_ls_dir (does this work with tuplestore?)
|  - profit

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 7 May 2020 10:08:35 -0500

Rebased onto 1ad23335f36b07f4574906a8dc66a3d62af7c40c

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 25 May 2020 21:10:03 -0500

Rebased onto 7b48f1b490978a8abca61e9a9380f8de2a56f266 and renumbered OIDs.

---

From: Fabien COELHO <[email-masked]>
Date: Sun, 7 Jun 2020 10:07:19 +0200 (CEST)

Hello Justin,

> Rebased onto 7b48f1b490978a8abca61e9a9380f8de2a56f266 and renumbered OIDs.

Some feedback about v18, seen as one patch.

Patch applies cleanly & compiles. "make check" is okay.

pg_stat_file() and pg_stat_dir_files() now return a char type, as well as 
the function which call them, but the documentation does not seem to say 
that it is the case.

I must admit that I'm not a fan on the argument management of 
pg_ls_dir_metadata and pg_ls_dir_metadata_1arg and others. I understand 
that it saves a few lines though, so maybe let it be.

There is a comment in pg_ls_dir_files which talks about pg_ls_dir.

Could pg_ls_*dir functions C implementations be dropped in favor of a pure 
SQL implementation, like you did with recurse?

If so, ISTM that pg_ls_dir_files() could be significantly simplified by 
moving its filtering flag to SQL conditions on "type" and others. That 
could allow not to change the existing function output a keep the "isdir" 
column defined as "type = 'd'" where it was used previously, if someone 
complains, but still have the full capability of "ls". That would also 
allow to drop the "*_1arg" hacks. Basically I'm advocating having 1 or 2 
actual C functions, and all other variants managed at the SQL level.

-- 
Fabien.

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 21 Jun 2020 20:53:25 -0500

> > Rebased onto 7b48f1b490978a8abca61e9a9380f8de2a56f266 and renumbered OIDs.

Rebased again on whatever broke func.sgml.

> the function which call them, but the documentation does not seem to say

Fixed, thanks

> pg_ls_dir_metadata and pg_ls_dir_metadata_1arg and others. I understand that
> it saves a few lines though, so maybe let it be.

I think you're saying that you don't like the _1arg functions, but they're
needed to allow the regression tests to pass:

| * note: this wrapper is necessary to pass the sanity check in opr_sanity,
| * which checks that all built-in functions that share the implementing C
| * function take the same number of arguments

> SQL implementation, like you did with recurse?

I'm still waiting to hear feedback from a commiter if this is a good idea to
put this into the system catalog.  Right now, ts_debug is the only nontrivial
function.

> moving its filtering flag to SQL conditions on "type" and others. That could
> allow not to change the existing function output a keep the "isdir" column
> defined as "type = 'd'" where it was used previously, if someone complains,
> but still have the full capability of "ls". That would also allow to drop
> the "*_1arg" hacks. Basically I'm advocating having 1 or 2 actual C
> functions, and all other variants managed at the SQL level.

You want to get rid of the 1arg stuff and just have one function.

I see your point, but I guess the C function would still need to accept a
"missing_ok" argument, so we need two functions, so there's not much utility in
getting rid of the "include_dot_dirs" argument, which is there for consistency
with pg_ls_dir.

Conceivably we could 1) get rid of pg_ls_dir, and 2) get rid of the
include_dot_dirs argument and 3) maybe make "missing_ok" a required argument;
and, 4) get rid of the C wrapper functions, and replace with a bunch of stuff
like this:

SELECT name, size, access, modification, change, creation, type='d' AS isdir
FROM pg_ls_dir_metadata('pg_wal') WHERE substring(name,1,1)!='.' AND type!='d';

Where the defaults I changed in this patchset still remain to be discussed:
with or without metadata, hidden files, dotdirs.

As I'm still waiting for committer feedback on the first 10 patches, so not
intending to add more.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 14 Jul 2020 22:08:39 -0500

> put this into the system catalog.  Right now, ts_debug is the only nontrivial

I'm still missing feedback from committers about the foundation of this
approach.

But I finally looked into the pg_rewind test failure 

That led met to keep the "dir" as a separate column, since that's what's needed
there, and it's more convenient to have a separate column than to provide a
column needing to be parsed.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Sat, 18 Jul 2020 15:15:32 -0500

> I'm still missing feedback from committers about the foundation of this

Now rebased on top of fix for my own bug report (1d09fb1f).

I also changed argument handling for pg_ls_dir_recurse().

Passing '.' gave an initial path of . (of course) but then every other path
begins with './' which I didn't like, since it's ambiguous with empty path, or
.// or ././ ...  And one could pass './' which gives different output (like
././).  

So I specially handled the input of '.'.  Maybe the special value should be
NULL instead of ''.  But it looks no other system functions are currently
non-strict.

For pg_rewind testcase, getting the output path+filename uses a coalesce, since
the rest of the test does stuff like strcmp("pg_wal").

Still waiting for feedback from a committer.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 8 Sep 2020 14:51:26 -0500

> Still waiting for feedback from a committer.

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.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 28 Oct 2020 14:34:02 -0500

> least ready for senior review.

@cfbot: rebased

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 5 Nov 2020 07:51:57 -0600

> > least ready for senior review.

Rebased on e152506adef4bc503ea7b8ebb4fedc0b8eebda81

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Mon, 23 Nov 2020 16:14:18 -0500

>> 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

---

From: Stephen Frost <[email-masked]>
Date: Mon, 23 Nov 2020 18:00:31 -0500

Greetings,

> surely not OK --- there is no possible way that doesn't break existing

I disagree that we need to stress over this- we pretty routinely change
the signature of various catalogs and functions and anyone using these
is already of the understanding that we are free to make such changes
between major versions.  If anything, we should be strongly discouraging
the notion of "don't break user queries" when it comes to administrative
and monitoring functions like these because, otherwise, we end up with
things like the mess that is pg_start/stop_backup() (and just contrast
that to what we did to recovery.conf when thinking about "well, do we
need to 'deprecate' or keep around the old stuff so we don't break
things for users who use these functions?" or the changes made in v10,
neither of which have produced much in the way of complaints).

Let's focus on working towards cleaner APIs and functions, accepting a
break when it makes sense to, which seems to be the case with this patch
(though I agree about using a TAP test suite and about performing the
directory recursion in C instead), and not pull forward cruft that we
then are telling ourselves we have to maintain compatibility of
indefinitely and at the expense of sensible APIs.

Thanks,

Stephen

---

From: Tom Lane <[email-masked]>
Date: Mon, 23 Nov 2020 18:06:19 -0500

Stephen Frost <[email-masked]> writes:
>> pg_ls_logdir() et al.

> between major versions.

Well, like I said, just MHO.  Anybody else want to weigh in?

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.

			regards, tom lane

---

From: Stephen Frost <[email-masked]>
Date: Tue, 24 Nov 2020 11:53:22 -0500

Greetings,

> to be partially duplicative of a file-type-code output column.

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.

Thanks,

Stephen

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 29 Nov 2020 11:21:15 -0600

> standpoints.  Surely it can't be hard to build a recursion capability

Thanks.  WITH RECURSIVE was the "new approach" I took early this year.  Of
course we can recurse in C, now that I know (how) to use the tuplestore.
Working on that patch was how I ran into the "LIMIT 1" SRF bug.

I don't see how security is relevant though, though, since someone can run a
the WITH query directly.  The function just needs to be restricted to
superusers, same as pg_ls_dir().

Anyway, I've re-ordered commits so this the last patch, since earlier commits
don't need to depend on it.  I don't think it's even essential to provide a
recursive function (anyone could write the CTE), so long as we don't hide dirs
and show isdir or type.

I implemented it first as a separate function and then as an optional argument
to pg_ls_dir_files().  If it's implemented as an optional "mode" of an existing
function, there's the constraint that returning a "path" argument has to be
after all other arguments (the ones that are useful without recursion) or else
it messes up other functions (like pg_ls_waldir()) that also call
pg_ls_dir_files().

> implies LS_DIR_SKIP_DOT_DIRS.  Do we want that?

Yes it's implied.  Those options exist to support the pre-existing behavior.
pg_ls_dir can optionaly show dotdirs, but pg_ls_*dir skip all hidden files
(which is documented since 8b6d94cf6).  I'm happy to implement something else
if a different behavior is desirable.

-- 
Justin

---

From: Tom Lane <[email-masked]>
Date: Fri, 04 Dec 2020 12:23:23 -0500

Justin Pryzby <[email-masked]> writes:
[ v24-0001-Document-historic-behavior-of-links-to-directori.patch ]

The cfbot is unhappy with one of the test cases you added:

6245@@ -259,9 +259,11 @@
6246 select path, filename, type from pg_ls_dir_metadata('.', true, false, true) where path!~'[0-9]|pg_internal.init|global.tmp' order by 1;
6247                path               |       filename        | type 
6248 ----------------------------------+-----------------------+------
6249+ PG_VERSION                       | PG_VERSION            | -
6250  base                             | base                  | d
6251  base/pgsql_tmp                   | pgsql_tmp             | d
6252  global                           | global                | d
6253+ global/config_exec_params        | config_exec_params    | -
6254  global/pg_control                | pg_control            | -
6255  global/pg_filenode.map           | pg_filenode.map       | -
6256  pg_commit_ts                     | pg_commit_ts          | d
6257@@ -285,7 +287,6 @@
6258  pg_subtrans                      | pg_subtrans           | d
6259  pg_tblspc                        | pg_tblspc             | d
6260  pg_twophase                      | pg_twophase           | d
6261- PG_VERSION                       | PG_VERSION            | -
6262  pg_wal                           | pg_wal                | d
6263  pg_wal/archive_status            | archive_status        | d
6264  pg_xact                          | pg_xact               | d
6265@@ -293,7 +294,7 @@
6266  postgresql.conf                  | postgresql.conf       | -
6267  postmaster.opts                  | postmaster.opts       | -
6268  postmaster.pid                   | postmaster.pid        | -
6269-(34 rows)
6270+(35 rows)

This shows that (a) the test is sensitive to prevailing collation and
(b) it's not filtering out enough temporary files.  Even if those things
were fixed, though, the test would break every time we added/removed
some PGDATA substructure.  Worse, it'd also break if say somebody had
edited postgresql.conf and left an editor backup file behind, or when
running in an installation where the configuration files are someplace
else.  I think this is way too fragile to be acceptable.

Maybe it could be salvaged by reversing the sense of the WHERE condition
so that instead of trying to blacklist stuff, you whitelist just a small
number of files that should certainly be there.

			regards, tom lane

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 9 Dec 2020 10:37:43 -0600

> number of files that should certainly be there.

Yes, I had noticed this one.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 23 Dec 2020 13:17:10 -0600

> 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'}

However, pg_ls_tmpdir is special since it handles tablespace tmpdirs, which it
seems is not trivial to get from sql:

+SELECT * FROM (SELECT DISTINCT COALESCE(NULLIF(pg_tablespace_location(b.oid),'')||suffix, 'base/pgsql_tmp') AS dir
+FROM pg_tablespace b, pg_control_system() pcs,
+LATERAL format('/PG_%s_%s', left(current_setting('server_version_num'), 2), pcs.catalog_version_no) AS suffix) AS dir,
+LATERAL pg_ls_dir_recurse(dir) AS a;

For context, the line of reasoning that led me to this patch series was
something like this:

0) Why can't I list shared tempfiles (dirs) using pg_ls_tmpdir() ?
1) Implement recursion for pg_ls_tmpdir();
2) Eventually realize that it's silly to implement a function to recurse into
   one particular directory when no general feature exists;
3) Implement generic facility;

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

-- 
Justin

---

From: Stephen Frost <[email-masked]>
Date: Wed, 23 Dec 2020 14:27:32 -0500

Greetings,

> | 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

---

From: David Steele <[email-masked]>
Date: Mon, 15 Mar 2021 08:47:17 -0400

On 12/23/20 2:27 PM, Stephen Frost wrote:
> deprecated.  
Stephen, are you still planning to review these patches?

Regards,
-- 
-David
[email-masked]

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 6 Apr 2021 11:01:31 -0500

> resolve the original issue of hiding tmpdirs.  I've rebased and re-arranged the

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

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 8 Apr 2021 23:14:32 -0500

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

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 2 Jul 2021 14:16:37 -0500

@cfbot: rebased

---

From: "Bossart, Nathan" <[email-masked]>
Date: Mon, 22 Nov 2021 19:17:01 +0000

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

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 23 Nov 2021 18:04:44 -0600

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:

> ... Add recursion option in pg_ls_dir_files..

rebased on 1922d7c6e1a74178bd2f1d5aa5a6ab921b3fcd34

-- 
Justin

---

From: Fabien COELHO <[email-masked]>
Date: Thu, 23 Dec 2021 09:14:18 -0400 (AST)

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.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 23 Dec 2021 11:36:31 -0600

>   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).

---

From: Michael Paquier <[email-masked]>
Date: Wed, 23 Mar 2022 15:17:35 +0900

On Mon, Mar 21, 2022 at 06:28:28PM -0700, Andres Freund wrote:
> Doesn't apply cleanly anymore: http://cfbot.cputube.org/patch_37_2377.log
> 
> Marked as waiting-on-author.

FWIW, per my review the bit of the patch set that I found the most
relevant is the addition of a note in the docs of pg_stat_file() about
the case where "filename" is a link, because the code internally uses
stat().   The function name makes that obvious, but that's not
commonly known, I guess.  Please see the attached, that I would be
fine to apply.
--
Michael
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8a802fb225..ca111f0745 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -27620,6 +27620,10 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
         platforms only), file creation time stamp (Windows only), and a flag
         indicating if it is a directory.
        </para>
+       <para>
+        If <parameter>filename</parameter> is a link, this function returns
+        information about the file or directory the link refers to.
+       </para>
        <para>
         This function is restricted to superusers by default, but other users
         can be granted EXECUTE to run the function.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 26 Mar 2022 20:23:54 +0900

> commonly known, I guess.  Please see the attached, that I would be

Hmm.  I am having second thoughts on this one, as on Windows we rely
on GetFileInformationByHandle() for the emulation of stat() in
win32stat.c, and it looks like this returns some information about the
junction point and not the directory or file this is pointing to, it
seems.  At the end, it looks better to keep things simple here, so
let's drop it.
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 28 Mar 2022 21:13:52 -0500

On Sat, Mar 26, 2022 at 08:23:54PM +0900, Michael Paquier wrote:
> seems.

Where did you find that ?  What metadata does it return about the junction
point ?  We only care about a handful of fields.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 31 Mar 2022 18:42:00 -0500

On Mon, Mar 14, 2022 at 09:37:25PM -0500, Justin Pryzby wrote:
> The original, minimal goal of this patch was to show shared tempdirs in
> pg_ls_tmpfile() - rather than hiding them misleadingly as currently happens.
> [email-masked]
> [email-masked]
> 
> I added the metadata function 2 years ago since it's silly to show metadata for
> tmpdir but not other, arbitrary directories.
> [email-masked]
> [email-masked]
> [email-masked]

I renamed the CF entry to make even more clear the original motive for the
patches (I'm not maintaining the patch to add the metadata function just to
avoid writing a lateral join).

> > 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.
> 
> It is frustrating to hear this feedback now, after the patch has gone through
> multiple rewrites over 2 years - based on other positive feedback and review.
> I went to the effort to ask, numerous times, whether to write the patch and how
> its interfaces should look.  Now, I'm hearing that not only the implementation
> but its goals are wrong.  What should I have done to avoid that ?
> 
> [email-masked]
> [email-masked]
> [email-masked]
> [email-masked]

Michael said he's not enthusiastic about the patch.  But I haven't heard a
suggestion about how else to address the issue that pg_ls_tmpdir() hides shared
filesets.

> point ?  We only care about a handful of fields.

Pending your feedback, I didn't modify this beyond your original suggestion -
which seemed like a good one.

This also adds some comments you requested and fixes your coding style
complaints, and causes cfbot to test my proposed patch rather than your doc
patch.

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 23 Jun 2022 23:35:49 -0500

This thread has been going for 2.5 years, so here's a(nother) recap.

This omits the patches for recursion, since they're optional and evidently a
distraction from the main patches.

On Fri, Dec 27, 2019 at 11:02:20AM -0600, Justin Pryzby wrote:
> The goal is to somehow show tmpfiles (or at least dirs) used by parallel
> workers.

> It's pretty unfortunate if a function called

On Fri, Mar 13, 2020 at 08:12:32AM -0500, Justin Pryzby wrote:
> by making that a separate patch to debate).

On Tue, Jan 25, 2022 at 01:27:55PM -0600, Justin Pryzby wrote:
> The original motive for the patch was that pg_ls_tmpdir doesn't show shared
> filesets.

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 27 Oct 2022 19:38:15 -0500

On Fri, Dec 13, 2019 at 03:03:47PM +1300, Thomas Munro wrote:
> > Actually, I tried using pg_ls_tmpdir(), but it unconditionally masks
> > non-regular files and thus shared filesets.  Maybe that's worth
> > discussion on a new thread ?
> >
> > src/backend/utils/adt/genfile.c
> >                 /* Ignore anything but regular files */
> >                 if (!S_ISREG(attrib.st_mode))
> >                         continue;
> 
> +1, that's worth fixing.

@cfbot: rebased on eddc128be.

---

From: Peter Smith <[email-masked]>
Date: Mon, 22 Jan 2024 12:16:13 +1100

2024-01 Commitfest.

Hi, this patch was marked in CF as "Needs Review", but there has been
no activity on this thread for 14+ months.

Since there seems not much interest, I have changed the status to
"Returned with Feedback" [1]. Feel free to propose a stronger use case
for the patch and add an entry for the same.

======
[1] https://commitfest.postgresql.org/46/2377/

Kind Regards,
Peter Smith.