Re: Review: Patch to compute Max LSN of Data Pages

On Fri, Oct 18, 2013 at 9:01 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Oct 9, 2013 at 2:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Sat, Sep 14, 2013 at 3:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>>On Monday, July 08, 2013 5:16 PM Andres Freund wrote:
>>>>>On 2013-07-08 17:10:43 +0530, Amit Kapila wrote:
>>>>>> On Monday, July 08, 2013 4:26 PM Andres Freund wrote:
>>>>>> > On 2013-07-08 16:17:54 +0530, Hari Babu wrote:
>> I am OK with this patch in its current form, modulo some grammar
>> issues in the documentation which I can fix before committing.
>> However, I'm unclear whether there was sufficient consensus to proceed
>> with this. Can others weigh in? If there is too much residual
>> unhappiness with this, then we should just mark this as Rejected and
>> stop wasting time on it; it can be pushed to PGXN or similar even if
>> we don't put it in core.
>
> I didn't hear any other votes. Anyone else have an opinion about
> this? If I can't get a +1 from anyone who wasn't involved in writing
> the patch, I'm inclined to think we don't have sufficient consensus to
> commit this.
>
> On further review of the patch, I also found a number of other issues
> that I think need to be fixed before we could consider committing it:
>
> - Per a previous request of mine, the patch has three different modes:
> it can be run on an individual file, on a directory potentially
> containing multiple relation files, or on an entire data directory.
> This is not explained in the documentation.

There is some explanation about it, but I think you want to see in
different format or wording.
I will change it to explain it more clearly in next update of this patch.

+ <title>Description</title>
+ <para>
+ <command>pg_computemaxlsn</command> computes maximun LSN from database pages
+ in the specified list of files or directories.
+ </para>
+
+ <para>
+ If user doesn't provide the file or directory to find the max lsn then
+ <command>pg_computemaxlsn</command> use the environment variable
<envar>PGDATA</>
+ if exists otherwise reports an error.
+ </para>

> - The patch skips printing an error if attempting to open a file
> returns ENOENT. I don't see why that case shouldn't print an error.
> Yeah, it could be legit if you're executing this against a running
> server, but why are you doing that? And even if you are (e.g. for
> corruption detection), printing a error message and proceeding makes
> more sense than proceeding without printing anything, at least IMHO.
>
> - Some of the static functions in this file preceed main and others
> follow it. And they use different naming conventions. I suggest
> putting all of them after main() and using names like check_data_dir,
> check_data_file_name (instead of validateRelfilenodename; note that
> the comment for that function is also incorrect),
> find_max_lsn_in_file, find_max_lsn_in_directory,
> find_max_lsn_in_pgdata.
>
> - Since the patch goes to some pains to exit with 0 if no errors were
> encountered and 1 if any were, that probably ought to be documented.
> But I think instead of passing a "result" argument back up the call
> stack it would be better to just keep a global variable called
> "errors_encountered". When we encounter an error, bump that value.
> Then you can just do exit(errors_encountered > 0 ? 1 : 0) and not
> bother passing all of this stuff around via the return value. The
> current behavior is inconsistent in another way, too: if you encounter
> an error while scanning through a particular directory, you finish the
> whole directory. But if multiple command-line arguments were passed,
> you don't proceed to any subsequent command-line arguments. I think
> you should continue always, which the above-mentioned change will take
> care of basically for free.
>
> - The description of the -q option is not very clear. A reader could
> be forgiven for thinking that the option suppresses all output, which
> would be quite useless, or at least left in doubt about what output
> will still be provided.

Thank you for your feedback.
I will update it in next version of patch if there is a consensus to
proceed for this utility.

> A broader complaint I have with this patch is that it almost but
> not-quite solves a problem I've had a few times in the past: namely,
> searching through the data directory for data blocks which have LSNs
> in the future. This has come up a few times for me, and this tool
> would make it easier, because I'd be able to run it and look through
> the output to see which relations have high max-LSN values. However,
> it wouldn't be quite enough, because it'd only tell me about the block
> with the highest LSN in each file, whereas what I'd really want to
> find is every block with an LSN greater than some threshold value.
> Maybe I'm pushing the envelope too much by trying to fit that into the
> framework of this patch, but I can't help thinking we're not going to
> want both pg_computemaxlsn and pg_findlsnsaftersomethreshold that are
> 95% the same code, so maybe we ought to rename the utility to
> something slightly more generic than "pg_computemaxlsn".
>
> In some ways this is almost a plausible shell for a generic
> block-corruption checker. If we wanted to validate, for example, that
> every page in a relation (or a whole data directory) had the expected
> page version number, we'd want almost exactly this same code
> structure, just with more checks added in.

Okay, I have some idea to make the use case of this utility bit
broader, so that it can be helpful in many more cases to detect
corruption.
This utility can be extended to validate database in below ways:
1. It will validate the blocks based on checksum, if database has checksums
2. Can validate the version number in pages
3. Can validate if all data files exist
4. Can validate if block contains all zeros
5. It can validate the blocks based on whether rows are intact in a
block, doesn't have clear idea at this point for how to achieve it.
6. It can report empty pages in a file.

more ideas can be thought of to validate the database, but I think we
can start with something minimal like
a. its current usage of computing LSN and extend it find LSN greater
than threshold
b. validate the checksum

> I'm going to mark this "Returned with Feedback" in the CF app for now.
> I still think that there's promise in this idea but, on further
> reflection, committing what we have here now doesn't feel like the
> right decision.

Thank you very much for giving valuable suggestions and supporting the idea.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com