On Wed, Nov 9, 2022 at 6:30 AM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Wed, Nov 9, 2022 at 5:08 AM David Christensen
> <david.christen...@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.
Per later discussion seems like new feature tests are fine in the same
patch, yes?
> 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.
How I was envisioning this was for cases like extreme surgery for
corrupted pages, where you extract the page from WAL but it has lsn
and checksum set so you could do something like `dd if=fixup-block
of=relation ...`, so it *simulates* the replay of said fullpage blocks
in cases where for some reason you can't play the intermediate
records; since this is always a fullpage block, it's capturing what
would be the snapshot so you could manually insert somewhere as needed
without needing to replay (say if dealing with an incomplete or
corrupted WAL stream).
> 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);
This was to suppress any suffix for main forks, but yes, could
simplify and include the `_` in the suffix name. Will include such a
change.
> 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.
Easy enough change, and makes sense.
> 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()?
I don't recall the original motivation, TBH.
> 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?
That is there in fixup mode (or should be). Are you thinking this
should also be set if not in fixup mode? That defeats the purpose of
the raw page extract, which is to see *exactly* what the WAL stream
has.
> 6.
> + if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
> Use pg_dir_create_mode instead of hard-coded 0007?
Sure.
> 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?
Sure.
> 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);
Like separate opt processing with their own `break` statement?
Probably a bit more readable/conventional.
> 9.
> + if (dir_status == 0 && mkdir(config.save_fpw_path, 0700) < 0)
> Should we use pg_mkdir_p() instead of mkdir()?
Sure.
Thanks,
David
