Date: Sat, 7 Mar 2020 11:10:36 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Fabien COELHO <coelho@cri.ensmp.fr>
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>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets
Message-ID: <20200307171036.GA1357@telsasoft.com>
References: <20200303200117.GI684@telsasoft.com>
 <20200303202313.GA28076@alvherre.pgsql>
 <20200305161838.GJ684@telsasoft.com>
 <20200306233507.GN684@telsasoft.com>
 <alpine.DEB.2.21.2003071125340.21542@pseudo>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.21.2003071125340.21542@pseudo>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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