On Tue, Mar 22, 2022 at 11:30 PM Andres Freund <and...@anarazel.de> wrote: > > > > To me duplicating this much code from waldump seems like a bad idea from a > > > maintainability POV. > > > > Even if we were to put the above code from pg_walinspect and > > pg_waldump into, say, walutils.c or some other existing file, there we > > had to make if (pg_walinspect) appendStringInfo else if (pg_waldump) > > printf() sort of thing, isn't it clumsy? > > Why is that needed? Just use the stringinfo in both places? You're outputting > the exact same thing in both places right now. There's already a stringinfo in > XLogDumpDisplayRecord() these days (there wasn't back when pg_xlogddump was > written), so you could just convert at least the relevant printfs in > XLogDumpDisplayRecord(). > > > Also, unnecessary if > > conditions need to be executed for every record. For maintainability, > > I added a note in pg_walinspect.c and pg_waldump.c to consider fixing > > things in both places (of course this might sound dumbest way of doing > > it, IMHO, it's sensible, given the if(pg_walinspect)-else > > if(pg_waldump) sorts of code that we need to do in the common > > functions). Thoughts? > > IMO we shouldn't merge this with as much duplication as there is right now, > the notes don't change that it's a PITA to maintain.
Here's a refactoring patch that basically moves the pg_waldump's functions and stats structures to xlogreader.h/.c so that the pg_walinspect can reuse them. If it looks okay, I will send the pg_walinspect patches based on it. Regards, Bharath Rupireddy.
v13-0001-Refactor-pg_waldump-code.patch
Description: Binary data
