Date: Sun, 15 Mar 2020 18:15:02 +0100 (CET)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Justin Pryzby <pryzby@telsasoft.com>
cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
    David Steele <david@pgmasters.net>, pgsql-hackers@postgresql.org,
    "Bossart, Nathan" <bossartn@amazon.com>,
    Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and
 pg_ls_*)
In-Reply-To: <20200313131232.GO29065@telsasoft.com>
Message-ID: <alpine.DEB.2.21.2003151146370.12715@pseudo>
References: <20200303200117.GI684@telsasoft.com>
 <20200303202313.GA28076@alvherre.pgsql> <20200305161838.GJ684@telsasoft.com>
 <20200306233507.GN684@telsasoft.com>
 <alpine.DEB.2.21.2003071125340.21542@pseudo>
 <20200307214010.GB1357@telsasoft.com>
 <20200310183037.GA29065@telsasoft.com> <20200313131232.GO29065@telsasoft.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: multipart/mixed;
 BOUNDARY="2056645051-1912256913-1584276379=:12715"
Content-ID: <alpine.DEB.2.21.2003151745570.12715@pseudo>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--2056645051-1912256913-1584276379=:12715
Content-Type: text/plain; FORMAT=flowed; CHARSET=UTF-8
Content-Transfer-Encoding: 8BIT
Content-ID: <alpine.DEB.2.21.2003151414221.12715@pseudo>


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.
--2056645051-1912256913-1584276379=:12715--