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

On Tue, Nov 15, 2022 at 4:41 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Tue, Nov 15, 2022 at 1:29 AM David Christensen
> <david(dot)christensen(at)crunchydata(dot)com> wrote:
> >
> > Enclosed is v8, which uses the replication slot method to retain WAL
> > as well as fsync'ing the output directory when everything is done.
>
> Thanks. It mostly is in good shape. However, few more comments:
>
> 1.
> + if it does not exist. The images saved will be subject to the same
> + filtering and limiting criteria as display records, but in this
> + mode <application>pg_waldump</application> will not output any other
> + information.
> May I know what's the intention of the statement 'The images saved
> ....'? If it's not necessary and convey anything useful to the user,
> can we remove it?

Basically I mean if you're limiting to a specific relation or rmgr
type, etc, it only saves those FPIs. (So filtering is applied first
before considering whether to save the FPI or not.)

> 2.
> +#include "storage/checksum.h"
> +#include "storage/checksum_impl.h"
> I think we don't need the above includes as we got rid of verifying
> page checksums. The patch compiles without them for me.

Good catch.

> 3.
> + char *save_fpw_path;
> Can we rename the above variable to save_fpi_path, just to be in sync
> with what we expose to the user, the option name 'save-fpi'?

Sure.

> 4.
> + if (config.save_fpw_path != NULL)
> + {
> + /* Fsync our output directory */
> + fsync_fname(config.save_fpw_path, true);
> + }
> I guess adding a comment there as to why we aren't fsyncing for every
> file that gets created, but once per the directory at the end. That'd
> help clarify doubts that other members might get while looking at the
> code.

Can do.

> 5.
> + if (config.save_fpw_path != NULL)
> + {
> + /* Fsync our output directory */
> + fsync_fname(config.save_fpw_path, true);
> + }
> So, are we sure that we don't want to fsync for time_to_stop exit(0)
> cases, say when CTRL+C'ed. Looks like we handle time_to_stop safely
> meaning exiting with return code 0, shouldn't we fsync the directory?

We can. Like I've said before, since these aren't production parts of
the cluster I don't personally have much of an opinion if fsync() is
appropriate at all, so don't have strong feelings here.

> 6.
> + else if (config.save_fpw_path)
> Let's use the same convention to check non-NULLness,
> config.save_fpw_path != NULL.

Good catch.

> 7.
> +CHECKPOINT;
> +SELECT pg_switch_wal();
> +UPDATE test_table SET a = a + 1;
> +SELECT pg_switch_wal();
> I don't think switching WAL after checkpoint is necessary here,
> because the checkpoint ensures all the WAL gets flushed to disk.
> Please remove it.

The point is to ensure we have a clean WAL segment that we know will
contain the relation we are filtering by. Will test if this still
holds without the extra pg_switch_wal(), but that's the rationale.

> PS: I've seen the following code:
> +my $walfile = [sort { $a <=> $b } glob("$waldir/00*")]->[1]; # we
> want the second WAL file, which will be a complete WAL file with
> full-page writes for our specific relation.

I don't understand the question.

> 8.
> +$node->safe_psql('postgres', <<EOF);
> +EOF
> Why EOF is used here? Can't we do something like below to execute
> multiple statements?
> $node->safe_psql(
> 'postgres', qq[
> SELECT data FROM pg_logical_slot_get_changes('regression_slot1', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> SELECT data FROM pg_logical_slot_get_changes('regression_slot2', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> SELECT data FROM pg_logical_slot_get_changes('regression_slot3', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> SELECT data FROM pg_logical_slot_get_changes('regression_slot4', NULL,
> NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
> ]);
>
> Same here:
> +$node->safe_psql('postgres', <<EOQ);
> +SELECT pg_drop_replication_slot('regress_pg_waldump_slot');
> +EOQ

As a long-time perl programmer, heredocs seem more natural and easier
to read rather than a string that accomplishes the same function. If
there is an established project style I'll stick with it, but it just
rolled out that way. :-)

> 9.
> +my $walfile = [sort { $a <=> $b } glob("$waldir/00*")]->[1]; # we
> want the second WAL file, which will be a complete WAL file with
> full-page writes for our specific relation.
> Is it guaranteed that just looking at the second WAL file in the
> pg_wal directory assures WAL file with FPIs? I think we have to save
> the WAL file that contains FPIs, that is the file after, CHECKPOINT,
> UPDATE and pg_switch_wal. I think you can store output LSN of
> pg_switch_wal

Yeah, I could look at that approach; originally this test was doing a
lot more, so this is sort of residual from that original
implementation. For a single file, this would probably be an
acceptable route.

> 10.
> +$node->safe_psql('postgres', <<EOQ);
> +SELECT pg_drop_replication_slot('regress_pg_waldump_slot');
> +EOQ
> +done_testing();
>
> Do we need to explicitly drop the slot here? I think we don't
> specifically drop the replication slot in all the places, my guess is
> after done_testing(), the node would get destroyed and also the slot.
> I think it's not required.

Maybe not required, but seems good form (and the other test I based
this on did do the cleanup).

> 11.
> +# verify filename formats matches w/--save-fpi
> +for my $fullpath (glob "$tmp_folder/raw/*")
> Do we need to look for the exact match of the file that gets created
> in the save-fpi path? While checking for this is great, it makes the
> test code non-portable (may not work on Windows or other platforms,
> no?) and complex? This way, you can get rid of get_block_info() as
> well? And +for my $fullpath (glob "$tmp_folder/raw/*")
> will also get simplified.
>
> I think you can further simplify the tests by:
> create the node
> generate an FPI
> call pg_waldump with save-fpi option
> check the target directory for a file that contains the relid,
> something like '%relid%'.
>
> The above would still serve the purpose, tests the code without much complexity.

I disagree; I think there is utility in keeping the validation of the
expected output. Since we have the code that works for it (and does
work on Windows, per passing the CI tests) I'm not seeing why we
wouldn't want to continue to validate as much as possible.

Thanks,

David