public inbox for [email protected]
help / color / mirror / Atom feedRe: [PATCH v1] pg_ls_tmpdir to show directories
60+ messages / 10 participants
[nested] [flat]
* Re: [PATCH v1] pg_ls_tmpdir to show directories
@ 2020-03-03 19:51 David Steele <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: David Steele @ 2020-03-03 19:51 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Justin Pryzby <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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 protected]
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: [PATCH v1] pg_ls_tmpdir to show directories
@ 2020-03-03 20:01 Justin Pryzby <[email protected]>
parent: David Steele <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-03 20:01 UTC (permalink / raw)
To: David Steele <[email protected]>; +Cc: Fabien COELHO <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Tue, Mar 03, 2020 at 02:51:54PM -0500, David Steele wrote:
> 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?
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: [PATCH v1] pg_ls_tmpdir to show directories
@ 2020-03-03 20:23 Alvaro Herrera <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Alvaro Herrera @ 2020-03-03 20:23 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: David Steele <[email protected]>; Fabien COELHO <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On 2020-Mar-03, Justin Pryzby wrote:
> 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 ?
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets
@ 2020-03-05 16:18 Justin Pryzby <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-05 16:18 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: David Steele <[email protected]>; Fabien COELHO <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Tue, Mar 03, 2020 at 05:23:13PM -0300, Alvaro Herrera wrote:
> On 2020-Mar-03, Justin Pryzby wrote:
>
> > 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 ?
>
> 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.
>
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
> directories.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets
@ 2020-03-06 23:35 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-06 23:35 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: David Steele <[email protected]>; Fabien COELHO <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Thu, Mar 05, 2020 at 10:18:38AM -0600, Justin Pryzby wrote:
> 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 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.
> > I don't understand what purpose is served by having pg_ls_waldir() hide
> > directories.
>
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets
@ 2020-03-07 14:14 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Fabien COELHO @ 2020-03-07 14:14 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets
@ 2020-03-07 17:10 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-07 17:10 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Sat, Mar 07, 2020 at 03:14:37PM +0100, Fabien COELHO wrote:
> Some feedback about the v7 patch set.
Thanks for looking again
> About v7.1, seems ok.
>
> 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:
...
> 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).
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets
@ 2020-03-07 17:40 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Fabien COELHO @ 2020-03-07 17:40 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
>> 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).
>
> 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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-07 21:40 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
1 sibling, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-03-07 21:40 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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:
> ISTM that several instances of: "pg_ls_dir_files(..., true, false);" should
> be "pg_ls_dir_files(..., true, DIR_HIDE);".
> 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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-08 08:02 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 0 replies; 60+ messages in thread
From: Fabien COELHO @ 2020-03-08 08:02 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-10 18:30 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-10 18:30 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-13 13:12 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-13 13:12 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
@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.
On Tue, Mar 10, 2020 at 01:30:37PM -0500, Justin Pryzby wrote:
> 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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-15 17:15 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Fabien COELHO @ 2020-03-15 17:15 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-15 21:27 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-15 21:27 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Sun, Mar 15, 2020 at 06:15:02PM +0100, Fabien COELHO wrote:
> 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).
> All patches apply cleanly, one on top of the previous. I really wish there
> 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.1 doc change: ok
>
> * 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).
> * 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?
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
> * v10.4 at least, some code!
> 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.
Done.
> 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.
Partially took your suggestion.
> 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;
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
> 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)
Done
> I'm not sure about these asserts:
>
> /* isdir depends on metadata */
> Assert(!(flags&FLAG_ISDIR) || (flags&FLAG_METADATA));
>
> Hmmm. Why?
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));
+ }
> /* 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.
It's unimportant, but testing for intended use of flags during development.
> * 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?
Maybe. I put them back...but it means they're not being exercized by any
*used* case.
> I do not like much the "if (...) /* empty */;" code. Maybe it could be
> caught more cleanly later in the conditional structure.
This went away when I put back the SKIP_DIRS flag.
> * v10.7 adds "pg_ls_dir_recurse" function
> 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
> (text,bool,bool,bool).
fixed, thanks.
> 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.
I added something which would've caught the issue with number of arguments.
> * v10.8 change flags & add test on pg_ls_logdir().
>
> 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.
> * 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.
This is the last patch in the series, since I think it's least likely to be
agreed on.
--
Justin
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-16 15:20 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Fabien COELHO @ 2020-03-16 15:20 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-16 15:41 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-16 15:41 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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
>
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-16 18:21 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Fabien COELHO @ 2020-03-16 18:21 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
Hello Justin,
>> 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.
>
> 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...
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.
> 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.
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.
> ISTM if someone wants to recursively list a directory, they should avoid
> 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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-16 21:48 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-16 21:48 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Mon, Mar 16, 2020 at 04:20:21PM +0100, Fabien COELHO wrote:
> 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.
On Mon, Mar 16, 2020 at 07:21:06PM +0100, Fabien COELHO wrote:
> 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
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-16 22:17 Alvaro Herrera <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Alvaro Herrera @ 2020-03-16 22:17 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Fabien COELHO <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-17 03:14 Justin Pryzby <[email protected]>
parent: Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-17 03:14 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Fabien COELHO <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Mon, Mar 16, 2020 at 07:17:36PM -0300, Alvaro Herrera wrote:
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-17 09:21 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Fabien COELHO @ 2020-03-17 09:21 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-17 19:04 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-03-17 19:04 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, Mar 17, 2020 at 10:21:48AM +0100, Fabien COELHO wrote:
>
> 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.
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 ?
> 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…
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)
> 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".
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-17 19:11 Tom Lane <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 0 replies; 60+ messages in thread
From: Tom Lane @ 2020-03-17 19:11 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Justin Pryzby <[email protected]> writes:
> 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 ?
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-03-31 20:08 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-03-31 20:08 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, Mar 17, 2020 at 02:04:01PM -0500, Justin Pryzby wrote:
> > 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…
>
> 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_*.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-04-12 11:53 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Fabien COELHO @ 2020-04-12 11:53 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-05-03 02:42 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-05-03 02:42 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Sun, Apr 12, 2020 at 01:53:40PM +0200, Fabien COELHO wrote:
> About v15, seen as one patch.
Thanks for looking.
> - 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-05-07 15:08 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 0 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-05-07 15:08 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
Rebased onto 1ad23335f36b07f4574906a8dc66a3d62af7c40c
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-05-26 02:10 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-05-26 02:10 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
Rebased onto 7b48f1b490978a8abca61e9a9380f8de2a56f266 and renumbered OIDs.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-06-07 08:07 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Fabien COELHO @ 2020-06-07 08:07 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-06-22 01:53 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-06-22 01:53 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Sun, Jun 07, 2020 at 10:07:19AM +0200, Fabien COELHO wrote:
> Hello Justin,
> > Rebased onto 7b48f1b490978a8abca61e9a9380f8de2a56f266 and renumbered OIDs.
Rebased again on whatever broke func.sgml.
> 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.
Fixed, thanks
> 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.
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
> 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?
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.
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-07-15 03:08 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-07-15 03:08 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Sun, Jun 21, 2020 at 08:53:25PM -0500, Justin Pryzby wrote:
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-07-18 20:15 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-07-18 20:15 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, Jul 14, 2020 at 10:08:39PM -0500, Justin Pryzby wrote:
> I'm still missing feedback from committers about the foundation of this
> approach.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-09-08 19:51 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-09-08 19:51 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Sat, Jul 18, 2020 at 03:15:32PM -0500, Justin Pryzby wrote:
> 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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-10-28 19:34 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-10-28 19:34 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Tue, Sep 08, 2020 at 02:51:26PM -0500, Justin Pryzby wrote:
> On Sat, Jul 18, 2020 at 03:15:32PM -0500, Justin Pryzby wrote:
> > 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.
@cfbot: rebased
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-05 13:51 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 0 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-11-05 13:51 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Tom Lane <[email protected]>
On Wed, Oct 28, 2020 at 02:34:02PM -0500, Justin Pryzby wrote:
> On Tue, Sep 08, 2020 at 02:51:26PM -0500, Justin Pryzby wrote:
> > On Sat, Jul 18, 2020 at 03:15:32PM -0500, Justin Pryzby wrote:
> > > 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.
>
> @cfbot: rebased
Rebased on e152506adef4bc503ea7b8ebb4fedc0b8eebda81
--
Justin
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-23 21:14 Tom Lane <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 3 replies; 60+ messages in thread
From: Tom Lane @ 2020-11-23 21:14 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Justin Pryzby <[email protected]> writes:
>> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-23 23:00 Stephen Frost <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 1 reply; 60+ messages in thread
From: Stephen Frost @ 2020-11-23 23:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Greetings,
* Tom Lane ([email protected]) wrote:
> Justin Pryzby <[email protected]> writes:
> >> 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.
>
> 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 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
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-23 23:06 Tom Lane <[email protected]>
parent: Stephen Frost <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Tom Lane @ 2020-11-23 23:06 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Stephen Frost <[email protected]> writes:
> * Tom Lane ([email protected]) wrote:
>> 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.
> 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.
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-24 16:53 Stephen Frost <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Stephen Frost @ 2020-11-24 16:53 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Greetings,
* Tom Lane ([email protected]) wrote:
> Stephen Frost <[email protected]> writes:
> > * Tom Lane ([email protected]) wrote:
> >> 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.
>
> > 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.
>
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-11-29 17:21 Justin Pryzby <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2020-11-29 17:21 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Mon, Nov 23, 2020 at 04:14:18PM -0500, Tom Lane wrote:
> Justin Pryzby <[email protected]> writes:
> >> 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.
>
> 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
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().
> 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?
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-12-04 17:23 Tom Lane <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Tom Lane @ 2020-12-04 17:23 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Justin Pryzby <[email protected]> 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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-12-09 16:37 Justin Pryzby <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-12-09 16:37 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On Fri, Dec 04, 2020 at 12:23:23PM -0500, Tom Lane wrote:
> Justin Pryzby <[email protected]> writes:
> [ v24-0001-Document-historic-behavior-of-links-to-directori.patch ]
>
> The cfbot is unhappy with one of the test cases you added:
> 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.
Yes, I had noticed this one.
--
Justin
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-12-23 19:17 Justin Pryzby <[email protected]>
parent: Tom Lane <[email protected]>
2 siblings, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2020-12-23 19:17 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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'}
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;
> * 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.
--
Justin
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2020-12-23 19:27 Stephen Frost <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Stephen Frost @ 2020-12-23 19:27 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Tom Lane <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Greetings,
* Justin Pryzby ([email protected]) 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
Attachments:
[application/pgp-signature] signature.asc (819B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-03-15 12:47 David Steele <[email protected]>
parent: Stephen Frost <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: David Steele @ 2021-03-15 12:47 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; Justin Pryzby <[email protected]>; +Cc: Tom Lane <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
On 12/23/20 2:27 PM, Stephen Frost wrote:
> * Justin Pryzby ([email protected]) 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
[email protected]
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-04-06 16:01 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2021-04-06 16:01 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-04-09 04:14 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 0 replies; 60+ messages in thread
From: Justin Pryzby @ 2021-04-09 04:14 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
Breaking with tradition, the previous patch included one too *few* changes, and
failed to resolve the OID collisions.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-07-02 19:16 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2021-07-02 19:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* [PATCH 1/2] Complete type names after ALTER TABLE … ADD [COLUMN] …
@ 2021-08-30 13:25 Dagfinn Ilmari Mannsåker <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Dagfinn Ilmari Mannsåker @ 2021-08-30 13:25 UTC (permalink / raw)
Since COLUMN is optional, we need to keep the list of object types to
complete after ADD in sync with the list of words not to complete type
names after. Add a comment to this effect.
---
src/bin/psql/tab-complete.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7c6af435a9..197f4c736c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2025,8 +2025,14 @@ psql_completion(const char *text, int start, int end)
"DETACH PARTITION", "FORCE ROW LEVEL SECURITY");
/* ALTER TABLE xxx ADD */
else if (Matches("ALTER", "TABLE", MatchAny, "ADD"))
+ /* make sure to keep this list and the !Matches() below in sync */
COMPLETE_WITH("COLUMN", "CONSTRAINT", "CHECK", "UNIQUE", "PRIMARY KEY",
"EXCLUDE", "FOREIGN KEY");
+ /* ATER TABLE xxx ADD [COLUMN] yyy */
+ else if (Matches("ALTER", "TABLE", MatchAny, "ADD", "COLUMN", MatchAny) ||
+ (Matches("ALTER", "TABLE", MatchAny, "ADD", MatchAny) &&
+ !Matches("ALTER", "TABLE", MatchAny, "ADD", "COLUMN|CONSTRAINT|CHECK|UNIQUE|PRIMARY|EXCLUDE|FOREIGN")))
+ COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_datatypes, NULL);
/* ALTER TABLE xxx ADD CONSTRAINT yyy */
else if (Matches("ALTER", "TABLE", MatchAny, "ADD", "CONSTRAINT", MatchAny))
COMPLETE_WITH("CHECK", "UNIQUE", "PRIMARY KEY", "EXCLUDE", "FOREIGN KEY");
--
2.30.2
--=-=-=--
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-11-22 19:17 Bossart, Nathan <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Bossart, Nathan @ 2021-11-22 19:17 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-11-24 00:04 Justin Pryzby <[email protected]>
parent: Bossart, Nathan <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2021-11-24 00:04 UTC (permalink / raw)
To: Bossart, Nathan <[email protected]>; +Cc: Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Fabien COELHO <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-12-23 13:14 Fabien COELHO <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Fabien COELHO @ 2021-12-23 13:14 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2021-12-23 17:36 Justin Pryzby <[email protected]>
parent: Fabien COELHO <[email protected]>
0 siblings, 2 replies; 60+ messages in thread
From: Justin Pryzby @ 2021-12-23 17:36 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; +Cc: Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
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).
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-03-23 06:17 Michael Paquier <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Michael Paquier @ 2022-03-23 06:17 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Fabien COELHO <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
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
Attachments:
[text/x-diff] doc-pgstatfile.patch (710B, ../../[email protected]/2-doc-pgstatfile.patch)
download | inline diff:
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.
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-03-26 11:23 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Michael Paquier @ 2022-03-26 11:23 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Fabien COELHO <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
On Wed, Mar 23, 2022 at 03:17:35PM +0900, Michael Paquier wrote:
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-03-29 02:13 Justin Pryzby <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2022-03-29 02:13 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Fabien COELHO <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>
On Sat, Mar 26, 2022 at 08:23:54PM +0900, Michael Paquier wrote:
> On Wed, Mar 23, 2022 at 03:17:35PM +0900, Michael Paquier wrote:
> > 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.
>
> 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.
Where did you find that ? What metadata does it return about the junction
point ? We only care about a handful of fields.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-03-31 23:42 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Justin Pryzby @ 2022-03-31 23:42 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Bossart, Nathan <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>; pgsql-hackers; Thomas Munro <[email protected]>; Andres Freund <[email protected]>
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 protected]
> [email protected]
>
> I added the metadata function 2 years ago since it's silly to show metadata for
> tmpdir but not other, arbitrary directories.
> [email protected]
> [email protected]
> [email protected]
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 protected]
> [email protected]
> [email protected]
> [email protected]
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.
On Mon, Mar 28, 2022 at 09:13:52PM -0500, Justin Pryzby wrote:
> On Sat, Mar 26, 2022 at 08:23:54PM +0900, Michael Paquier wrote:
> > On Wed, Mar 23, 2022 at 03:17:35PM +0900, Michael Paquier wrote:
> > > 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.
> >
> > 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.
>
> Where did you find that ? What metadata does it return about the junction
> 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
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-06-24 04:35 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
1 sibling, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2022-06-24 04:35 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>
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.
On Thu, Jan 16, 2020 at 08:38:46AM -0600, Justin Pryzby wrote:
> 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';
On Tue, Mar 10, 2020 at 01:30:37PM -0500, Justin Pryzby wrote:
> 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');
> 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).
On Fri, Mar 13, 2020 at 08:12:32AM -0500, Justin Pryzby wrote:
> 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).
On Tue, Mar 31, 2020 at 03:08:12PM -0500, Justin Pryzby wrote:
> 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).
On Wed, Dec 23, 2020 at 01:17:10PM -0600, Justin Pryzby wrote:
> 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;
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..
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2022-10-28 00:38 Justin Pryzby <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 1 reply; 60+ messages in thread
From: Justin Pryzby @ 2022-10-28 00:38 UTC (permalink / raw)
To: Fabien COELHO <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; +Cc: pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
* Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
@ 2024-01-22 01:16 Peter Smith <[email protected]>
parent: Justin Pryzby <[email protected]>
0 siblings, 0 replies; 60+ messages in thread
From: Peter Smith @ 2024-01-22 01:16 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Fabien COELHO <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; pgsql-hackers; Bossart, Nathan <[email protected]>; Thomas Munro <[email protected]>; Alvaro Herrera <[email protected]>; David Steele <[email protected]>
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.
^ permalink raw reply [nested|flat] 60+ messages in thread
end of thread, other threads:[~2024-01-22 01:16 UTC | newest]
Thread overview: 60+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 19:51 Re: [PATCH v1] pg_ls_tmpdir to show directories David Steele <[email protected]>
2020-03-03 20:01 ` Justin Pryzby <[email protected]>
2020-03-03 20:23 ` Alvaro Herrera <[email protected]>
2020-03-05 16:18 ` Re: pg_ls_tmpdir to show directories and shared filesets Justin Pryzby <[email protected]>
2020-03-06 23:35 ` Re: pg_ls_tmpdir to show directories and shared filesets Justin Pryzby <[email protected]>
2020-03-07 14:14 ` Re: pg_ls_tmpdir to show directories and shared filesets Fabien COELHO <[email protected]>
2020-03-07 17:10 ` Re: pg_ls_tmpdir to show directories and shared filesets Justin Pryzby <[email protected]>
2020-03-07 17:40 ` Re: pg_ls_tmpdir to show directories and shared filesets Fabien COELHO <[email protected]>
2020-03-07 21:40 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-08 08:02 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-03-10 18:30 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-13 13:12 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-15 17:15 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-03-15 21:27 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-16 15:20 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-03-16 15:41 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-16 18:21 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-03-16 21:48 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-16 22:17 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Alvaro Herrera <[email protected]>
2020-03-17 03:14 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-17 09:21 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-03-17 19:04 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-03-17 19:11 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Tom Lane <[email protected]>
2020-03-31 20:08 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-04-12 11:53 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-05-03 02:42 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-05-07 15:08 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-05-26 02:10 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-06-07 08:07 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2020-06-22 01:53 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-07-15 03:08 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-07-18 20:15 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-09-08 19:51 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-10-28 19:34 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-11-05 13:51 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-11-23 21:14 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Tom Lane <[email protected]>
2020-11-23 23:00 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Stephen Frost <[email protected]>
2020-11-23 23:06 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Tom Lane <[email protected]>
2020-11-24 16:53 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Stephen Frost <[email protected]>
2020-11-29 17:21 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-12-04 17:23 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Tom Lane <[email protected]>
2020-12-09 16:37 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-12-23 19:17 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2020-12-23 19:27 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Stephen Frost <[email protected]>
2021-03-15 12:47 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) David Steele <[email protected]>
2021-04-06 16:01 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2021-04-09 04:14 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2021-07-02 19:16 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2021-11-22 19:17 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Bossart, Nathan <[email protected]>
2021-11-24 00:04 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2021-12-23 13:14 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Fabien COELHO <[email protected]>
2021-12-23 17:36 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2022-03-23 06:17 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Michael Paquier <[email protected]>
2022-03-26 11:23 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Michael Paquier <[email protected]>
2022-03-29 02:13 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2022-03-31 23:42 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2022-06-24 04:35 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2022-10-28 00:38 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Justin Pryzby <[email protected]>
2024-01-22 01:16 ` Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*) Peter Smith <[email protected]>
2021-08-30 13:25 [PATCH 1/2] Complete type names after ALTER TABLE … ADD [COLUMN] … Dagfinn Ilmari Mannsåker <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox