From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Justin Pryzby <pryzby(at)telsasoft(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [PATCH v1] pg_ls_tmpdir to show directories |
Date: | 2019-12-27 17:50:24 |
Message-ID: | alpine.DEB.2.21.1912271845000.27864@pseudo |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> Re-added -hackers.
Indeed, I left it out by accident. I tried to bounce the original mail but
it did not work.
> Thanks for reviewing.
>
> On Fri, Dec 27, 2019 at 05:22:47PM +0100, Fabien COELHO wrote:
>> The implementation simply extends an existing functions with a boolean to
>> allow for sub-directories. However, the function does not seem to show
>> subdir contents recursively. Should it be the case?
>
>> STM that "//"-comments are not project policy.
>
> Sure, but the patch is less important than the design, which needs to be
> addressed first. The goal is to somehow show tmpfiles (or at least dirs) used
> by parallel workers. I mentioned a few possible ways, of which this was the
> simplest to implement. Showing files beneath the dir is probably good, but
> need to decide how to present it. Should there be a column for the dir (null
> if not a shared filesets)? Or some other presentation, like a boolean column
> "is_shared_fileset".
Why not simply showing the files underneath their directories?
/path/to/tmp/file1
/path/to/tmp/subdir1/file2
In which case probably showing the directory itself is not useful,
and the is_dir column could be dropped?
>> I'm unconvinced by the skipping condition:
>>
>> + if (!S_ISREG(attrib.st_mode) &&
>> + (!dir_ok && S_ISDIR(attrib.st_mode)))
>> continue;
>>
>> which is pretty hard to read. ISTM you meant "not A and not (B and C)"
>> instead?
>
> I can write it as two ifs.
Hmmm. Not sure it would help that much. At least the condition must be
right. Also, the comment should be updated.
> And, it's probably better to say:
> if (!ISDIR() || !dir_ok)
I cannot say I like it. dir_ok is cheaper to test so could come first.
> ..which is same as: !(B && C), as you said.
Ok, so you confirm that the condition was wrong.
>> Catalog update should be a date + number? Maybe this is best left to
>> the committer?
>
> Yes, but I preferred to include it, causing a deliberate conflict, to ensure
> it's not forgotten.
Ok.
--
Fabien.
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-12-27 17:59:04 | Re: Allow cluster owner to bypass authentication |
Previous Message | Stephen Frost | 2019-12-27 17:49:46 | Re: Allow cluster owner to bypass authentication |