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
zerorows.
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
