Date: Wed, 9 Nov 2022 08:14:47 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: David Christensen <david.christensen@crunchydata.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL
Message-ID: <20221109141446.GN3775@telsasoft.com>
References: <20221104140238.GA16921@telsasoft.com>
 <4E379ADE-1FC3-4D53-BE46-562BDC77C5FE@crunchydata.com>
 <20221104183838.GD16921@telsasoft.com>
 <CAOxo6X+2-k6gqwv2okbgLR_B_MkBCVHOW3CqFomFta2wNkHYdw@mail.gmail.com>
 <CAOxo6XJt=F3eKS8-jvFYoW2MVP+DCpseHPHYiLVYGF43iET30Q@mail.gmail.com>
 <20221108224519.GH3775@telsasoft.com>
 <CAOxo6XJ_pf+cKDXZMvxprvA1x-Pi+-22fq0Tw50o9HgkXg7cTw@mail.gmail.com>
 <CAOxo6XLCwGGjLEK9--VK9SzOb4EFKiJuSrnOePSPfEyr-pLKPw@mail.gmail.com>
 <CALj2ACWU7ckJLu=9Ag0AahnWT=1E22FsPu9wmcyAEJCv04BCvA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACWU7ckJLu=9Ag0AahnWT=1E22FsPu9wmcyAEJCv04BCvA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221109141446.GN3775%40telsasoft.com>
Precedence: bulk

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