Re: [PATCH] Teach pg_waldump to extract FPIs from the WAL

On Wed, Nov 9, 2022 at 5:08 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)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.

2. I'm unable to understand the use-case for --fixup-fpi option.
pg_waldump is supposed to be just WAL reader, and must not return any
modified information, with --fixup-fpi option, the patch violates this
principle i.e. it sets page LSN and returns. Without actually
replaying the WAL record on the page, how is it correct to just set
the LSN? How will it be useful? ISTM, we must ignore this option
unless there's a strong use-case.
3.
+ if (fork >= 0 && fork <= MAX_FORKNUM)
+ {
+ if (fork)
+ sprintf(forkname, "_%s", forkNames[fork]);
+ else
+ forkname[0] = 0;
+ }
+ else
+ pg_fatal("found invalid fork number: %u", fork);

Isn't the above complex? What's the problem with something like below?
Why do we need if (fork) - else block?

if (fork >= 0 && fork <= MAX_FORKNUM)
sprintf(forkname, "_%s", forkNames[fork]);
else
pg_fatal("found invalid fork number: %u", fork);

3.
+ char page[BLCKSZ] = {0};
I think when writing to a file, we need PGAlignedBlock rather than a
simple char array of bytes, see the description around PGAlignedBlock
for why it is so.

4.
+ if (pg_pwrite(fileno(OPF), page, BLCKSZ, 0) != BLCKSZ)
Why pg_pwrite(), why not just fwrite()? If fwrite() is used, you can
avoid fileno() system calls, no? Do you need the file position to
remain the same after writing, hence pg_pwrite()?

5.
+ if (!RestoreBlockImage(record, block_id, page))
+ continue;
+
+ /* we have our extracted FPI, let's save it now */
After extracting the page from the WAL record, do we need to perform a
checksum on it?

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

7.
+ if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
+ fsync(fileno(OPF));
+ fclose(OPF);
Since you're creating the directory in case it's not available, you
need to fsync the directory too?

8.
+ case 'W':
+ case 'X':
+ config.fixup_fpw = (option == 'X');
+ config.save_fpw_path = pg_strdup(optarg);
+ break;
Just set config.fixup_fpw = false before the switch block starts,
like the other variables, and then perhaps doing like below is more
readable:
case 'W':
config.save_fpw_path = pg_strdup(optarg);
case 'X':
config.fixup_fpw = true;
config.save_fpw_path = pg_strdup(optarg);

9.
+ if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
Should we use pg_mkdir_p() instead of mkdir()?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com