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

Started by Justin Pryzbyabout 4 years ago2 messageshackers
Jump to latest
#1Justin Pryzby
pryzby@telsasoft.com

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.
20200310183037.GA29065@telsasoft.com
20200313131232.GO29065@telsasoft.com

I added the metadata function 2 years ago since it's silly to show metadata for
tmpdir but not other, arbitrary directories.
20200310183037.GA29065@telsasoft.com
20200313131232.GO29065@telsasoft.com
20201223191710.GR30237@telsasoft.com

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 ?

20200503024215.GJ28974@telsasoft.com
20191227195918.GF12890@telsasoft.com
20200116003924.GJ26045@telsasoft.com
20200908195126.GB18552@telsasoft.com

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

Attachments:

v35-0001-Document-historic-behavior-of-links-to-directori.patchtext/x-diff; charset=us-asciiDownload+4-1
v35-0002-Add-tests-before-changing-pg_ls_.patchtext/x-diff; charset=us-asciiDownload+74-1
v35-0003-Add-pg_ls_dir_metadata-to-list-a-dir-with-file-m.patchtext/x-diff; charset=us-asciiDownload+217-71
v35-0004-pg_ls_tmpdir-to-show-directories-and-isdir-argum.patchtext/x-diff; charset=us-asciiDownload+20-17
v35-0005-pg_ls_-dir-to-show-directories-and-isdir-column.patchtext/x-diff; charset=us-asciiDownload+57-48
v35-0006-pg_ls_logdir-to-ignore-error-if-initial-top-dir-.patchtext/x-diff; charset=us-asciiDownload+12-2
v35-0007-pg_ls_-dir-to-return-all-the-metadata-from-pg_st.patchtext/x-diff; charset=us-asciiDownload+102-83
#2Bruce Momjian
bruce@momjian.us
In reply to: Justin Pryzby (#1)

The cfbot is failing testing this patch. It seems... unlikely given
the nature of the patch modifying an admin function that doesn't even
modify the database that it should be breaking a streaming test.
Perhaps the streaming test is using this function in the testing
scaffolding?

[03:19:29.564] # Failed test 'regression tests pass'
[03:19:29.564] # at t/027_stream_regress.pl line 76.
[03:19:29.564] # got: '256'
[03:19:29.564] # expected: '0'
[03:19:29.564] # Looks like you failed 1 test of 5.
[03:19:29.565] [03:19:27] t/027_stream_regress.pl ..............
[03:19:29.565] Dubious, test returned 1 (wstat 256, 0x100)
[03:19:29.565] Failed 1/5 subtests