Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL - Mailing list pgsql-hackers

On Wed, Nov 09, 2022 at 06:00:40PM +0530, Bharath Rupireddy wrote:
> On Wed, Nov 9, 2022 at 5:08 AM David Christensen <david.christensen@crunchydata.com> wrote:
> >
> > Enclosed is v6, which squashes your refactor and adds the additional
> > recent suggestions.
> 
> Thanks for working on this feature. Here are some comments for now. I
> haven't looked at the tests, I will take another look at the code and
> tests after these and all other comments are addressed.
> 
> 1. For ease of review, please split the test patch to 0002.

This is just my opinion, but .. why ?  Since it's easy to
filter/skip/display a file, I don't think it's usually useful to have
separate patches for tests or docs.

> 6.
> +        if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
> Use pg_dir_create_mode instead of hard-coded 0007?

I think I thought of that when I first looked at the patch ...  but, I'm
not sure, since it says:

src/include/common/file_perm.h-/* Modes for creating directories and files IN THE DATA DIRECTORY */
src/include/common/file_perm.h:extern PGDLLIMPORT int pg_dir_create_mode;

I was wondering if there's any reason to do "CREATE DATABASE".  The vast
majority of TAP tests don't.

$ git grep -ho 'safe_psql[^ ]*' '*pl' |sort |uniq -c |sort -nr |head
   1435 safe_psql('postgres',
    335 safe_psql(
     23 safe_psql($connect_db,

-- 
Justin