public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fabien COELHO <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: David Steele <[email protected]>
Cc: [email protected]
Cc: Bossart, Nathan <[email protected]>
Cc: Thomas Munro <[email protected]>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
Date: Sun, 8 Mar 2020 09:02:19 +0100 (CET)
Message-ID: <alpine.DEB.2.21.2003080805240.21542@pseudo> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<alpine.DEB.2.21.2003071125340.21542@pseudo>
<[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.
view thread (60+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and pg_ls_*)
In-Reply-To: <alpine.DEB.2.21.2003080805240.21542@pseudo>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox