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

>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:
>>> > > + This utility can also be used to decide whether backup is
>>> > required or not when the data page
>>> > > + in old-master precedes the last applied LSN in old-standby
>>> > (i.e., new-master) at the
>>> > > + moment of the failover.
>>> > > + </para>
>>> > > + </refsect1>
>>> >
>>> > I don't think this is safe in any interesting set of cases. Am I
>>> > missing
>>> > something?
>>>
>>> No, you are not missing anything. It can be only used to find max LSN in
>>> database which can avoid further corruption

>>Why is the patch submitted documenting it as a use-case then? I find it
>>rather scary if the *patch authors* document a known unsafe use case as
>>one of the known use-cases.

>I got the problem which can occur with the specified use case. Removed the
>wrong use case specified above.
>Thanks for the review, please find the updated patch attached in the mail.

Patch is not getting compiled on Windows, I had made following changes:
a. updated the patch for resolving Windows build
b. few documentation changes in (pg_resetxlog.sgml) for spelling
mistake and minor line change
c. corrected year for Copyright in file pg_computemaxlsn.c

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

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:
>>>> > > + This utility can also be used to decide whether backup is
>>>> > required or not when the data page
>>>> > > + in old-master precedes the last applied LSN in old-standby
>>>> > (i.e., new-master) at the
>>>> > > + moment of the failover.
>>>> > > + </para>
>>>> > > + </refsect1>
>>>> >
>>>> > I don't think this is safe in any interesting set of cases. Am I
>>>> > missing
>>>> > something?
>>>>
>>>> No, you are not missing anything. It can be only used to find max LSN in
>>>> database which can avoid further corruption
>
>>>Why is the patch submitted documenting it as a use-case then? I find it
>>>rather scary if the *patch authors* document a known unsafe use case as
>>>one of the known use-cases.
>
>>I got the problem which can occur with the specified use case. Removed the
>>wrong use case specified above.
>>Thanks for the review, please find the updated patch attached in the mail.
>
> Patch is not getting compiled on Windows, I had made following changes:
> a. updated the patch for resolving Windows build
> b. few documentation changes in (pg_resetxlog.sgml) for spelling
> mistake and minor line change
> c. corrected year for Copyright in file pg_computemaxlsn.c

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Oct 9, 2013 at 11:58 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:
>>>>> > > + This utility can also be used to decide whether backup is
>>>>> > required or not when the data page
>>>>> > > + in old-master precedes the last applied LSN in old-standby
>>>>> > (i.e., new-master) at the
>>>>> > > + moment of the failover.
>>>>> > > + </para>
>>>>> > > + </refsect1>
>>>>> >
>>>>> > I don't think this is safe in any interesting set of cases. Am I
>>>>> > missing
>>>>> > something?
>>>>>
>>>>> No, you are not missing anything. It can be only used to find max LSN in
>>>>> database which can avoid further corruption
>>
>>>>Why is the patch submitted documenting it as a use-case then? I find it
>>>>rather scary if the *patch authors* document a known unsafe use case as
>>>>one of the known use-cases.
>>
>>>I got the problem which can occur with the specified use case. Removed the
>>>wrong use case specified above.
>>>Thanks for the review, please find the updated patch attached in the mail.
>>
>> Patch is not getting compiled on Windows, I had made following changes:
>> a. updated the patch for resolving Windows build
>> b. few documentation changes in (pg_resetxlog.sgml) for spelling
>> mistake and minor line change
>> c. corrected year for Copyright in file pg_computemaxlsn.c
>
> 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.

I would like to summarize in short about this patch.

This idea started with me proposing some solutions for cases where
user can recover from some of corruption scenario's, then Tom Lane had
suggested
this idea and I prepared a Patch for it, then you have given
suggestions about this patch at various phases during development.
Both Fujji Masao and Alvaro seems to be in favor of use case and
Alvaro has given few suggestions for patch as well. Muhammad Usama had
reviewed this patch.

It appears to me that Andres is not in favour of this Patch.

So, I think there are good number of people who had participated in
this patch and were in favour of this patch.

The above summarization is from what I remember about this Patch, so
if anybody feels that I have misunderstood, then kindly correct me.

> 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.

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

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:
>>>>> > > + This utility can also be used to decide whether backup is
>>>>> > required or not when the data page
>>>>> > > + in old-master precedes the last applied LSN in old-standby
>>>>> > (i.e., new-master) at the
>>>>> > > + moment of the failover.
>>>>> > > + </para>
>>>>> > > + </refsect1>
>>>>> >
>>>>> > I don't think this is safe in any interesting set of cases. Am I
>>>>> > missing
>>>>> > something?
>>>>>
>>>>> No, you are not missing anything. It can be only used to find max LSN in
>>>>> database which can avoid further corruption
>>
>>>>Why is the patch submitted documenting it as a use-case then? I find it
>>>>rather scary if the *patch authors* document a known unsafe use case as
>>>>one of the known use-cases.
>>
>>>I got the problem which can occur with the specified use case. Removed the
>>>wrong use case specified above.
>>>Thanks for the review, please find the updated patch attached in the mail.
>>
>> Patch is not getting compiled on Windows, I had made following changes:
>> a. updated the patch for resolving Windows build
>> b. few documentation changes in (pg_resetxlog.sgml) for spelling
>> mistake and minor line change
>> c. corrected year for Copyright in file pg_computemaxlsn.c
>
> 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.

- 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.

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.

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas escribió:

> 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".

Perhaps not coincidentally, I had a need to do this recently. Perhaps
we should turn the utility into a generic tool to report existing LSNs,
with options to 1) report only the highest one in a given file, 2)
report only those that exceed some threshold. So maybe pg_reportlsn or
pg_extractlsn.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On Fri, Oct 18, 2013 at 9:24 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas escribió:
>
>> 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".
>
> Perhaps not coincidentally, I had a need to do this recently. Perhaps
> we should turn the utility into a generic tool to report existing LSNs,
> with options to 1) report only the highest one in a given file, 2)
> report only those that exceed some threshold. So maybe pg_reportlsn or
> pg_extractlsn.

How about extending it validate database in more meaningful way and
name it as validatedb.

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