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

Please find the rebased Patch for Compute MAX LSN.

There was one compilation error as "undefined reference to XLByteLT " as earlier XLogRecPtr was a structure as
typedef struct XLogRecPtr
{
uint32 xlogid; /* log file #, 0 based */
uint32 xrecoff; /* byte offset of location in log file */
} XLogRecPtr;
So in order to compare two LSN, there was one macro as XLByteLT to compare both fields.
But now XLogRecPtr has been changed as just uint64 and so XLByteLT is removed.
So the change done is to replace XLByteLT(*maxlsn, pagelsn) with (*maxlsn < pagelsn).

Muhammad, Can you verify if every thing is okay, then this can be marked as "Ready for Committer"

With Regards,

Amit Kapila.

2013/1/18 Amit kapila <amit(dot)kapila(at)huawei(dot)com>:
> Please find the rebased Patch for Compute MAX LSN.

The function 'remove_parent_refernces' couldn't be called
'remove_parent_references' ?

Why not an extension in PGXN instead of a contrib?

Regards,
--
Dickson S. Guedes
mail/xmpp: guedes(at)guedesoft(dot)net - skype: guediz
http://github.com/guedes - http://guedesoft.net
http://www.postgresql.org.br

On Sunday, January 20, 2013 4:04 AM Dickson S. Guedes wrote:
2013/1/18 Amit kapila <amit(dot)kapila(at)huawei(dot)com>:
>> Please find the rebased Patch for Compute MAX LSN.

>The function 'remove_parent_refernces' couldn't be called
>'remove_parent_references' ?

Shall fix this.

> Why not an extension in PGXN instead of a contrib?

This functionality has similarity to pg_resetxlog, so we thought of putting it either in bin or in contrib.
Finally based on suggestions from other community members, we have added to contrib.

With Regards,
Amit Kapila.

2013/1/20 Amit kapila <amit(dot)kapila(at)huawei(dot)com>:
> On Sunday, January 20, 2013 4:04 AM Dickson S. Guedes wrote:
> 2013/1/18 Amit kapila <amit(dot)kapila(at)huawei(dot)com>:
>>> Please find the rebased Patch for Compute MAX LSN.
>
>>The function 'remove_parent_refernces' couldn't be called
>>'remove_parent_references' ?
>
> Shall fix this.
>
>> Why not an extension in PGXN instead of a contrib?
>
> This functionality has similarity to pg_resetxlog, so we thought of putting it either in bin or in contrib.
> Finally based on suggestions from other community members, we have added to contrib.

Indeed.

--
Dickson S. Guedes
mail/xmpp: guedes(at)guedesoft(dot)net - skype: guediz
http://github.com/guedes - http://guedesoft.net
http://www.postgresql.org.br

On Sunday, January 20, 2013 10:50 AM Amit kapila wrote:
On Sunday, January 20, 2013 4:04 AM Dickson S. Guedes wrote:
2013/1/18 Amit kapila <amit(dot)kapila(at)huawei(dot)com>:
>>> Please find the rebased Patch for Compute MAX LSN.

>>The function 'remove_parent_refernces' couldn't be called
>>'remove_parent_references' ?

> Shall fix this.

Fixed in attached version.

With Regards,
Amit Kapila.

Hackers,

Amit posted a new version of this patch on January 23rd. But last
comment on it by Tom is "not sure everyone wants this".

https://commitfest.postgresql.org/action/patch_view?id=905

... so, what's the status of this patch?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Hi,

On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> Amit posted a new version of this patch on January 23rd. But last
> comment on it by Tom is "not sure everyone wants this".
>
> https://commitfest.postgresql.org/action/patch_view?id=905

> ... so, what's the status of this patch?

That comment was referencing a mail of mine - so perhaps I better
explain:

I think the usecase for this utility isn't big enough to be included in
postgres since it really can only help in a very limited
circumstances. And I think it's too likely to be misused for stuff it's
not useable for (e.g. remastering).

The only scenario I see is that somebody deleted/corrupted
pg_controldata. In that scenario the tool is supposed to be used to find
the biggest lsn used so far so the user then can use pg_resetxlog to set
that as the wal starting point.
But that can be way much easier solved by just setting the LSN to
something very, very high. The database cannot be used for anything
reliable afterwards anyway.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> Hi,
>
> On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> > Amit posted a new version of this patch on January 23rd. But last
> > comment on it by Tom is "not sure everyone wants this".
> >
> > https://commitfest.postgresql.org/action/patch_view?id=905
>
> > ... so, what's the status of this patch?
>
> That comment was referencing a mail of mine - so perhaps I better
> explain:
>
> I think the usecase for this utility isn't big enough to be included in
> postgres since it really can only help in a very limited
> circumstances. And I think it's too likely to be misused for stuff it's
> not useable for (e.g. remastering).
>
> The only scenario I see is that somebody deleted/corrupted
> pg_controldata. In that scenario the tool is supposed to be used to
> find
> the biggest lsn used so far so the user then can use pg_resetxlog to
> set
> that as the wal starting point.
> But that can be way much easier solved by just setting the LSN to
> something very, very high. The database cannot be used for anything
> reliable afterwards anyway.

One of the main reason this was written was to make server up in case of
corruption and
user can take dump of some useful information if any.

By setting LSN very, very high user might loose the information which he
wants to take dump.
So I think in such scenario's it can be quite helpful to users, but such
scenarios are rare and so
it might not be a utility which user will need often.

With Regards,
Amit Kapila.

On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
> On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> > > Amit posted a new version of this patch on January 23rd. But last
> > > comment on it by Tom is "not sure everyone wants this".
> > >
> > > https://commitfest.postgresql.org/action/patch_view?id=905
> >
> > > ... so, what's the status of this patch?
> >
> > That comment was referencing a mail of mine - so perhaps I better
> > explain:
> >
> > I think the usecase for this utility isn't big enough to be included in
> > postgres since it really can only help in a very limited
> > circumstances. And I think it's too likely to be misused for stuff it's
> > not useable for (e.g. remastering).
> >
> > The only scenario I see is that somebody deleted/corrupted
> > pg_controldata. In that scenario the tool is supposed to be used to
> > find
> > the biggest lsn used so far so the user then can use pg_resetxlog to
> > set
> > that as the wal starting point.
> > But that can be way much easier solved by just setting the LSN to
> > something very, very high. The database cannot be used for anything
> > reliable afterwards anyway.
>
> One of the main reason this was written was to make server up in case of
> corruption and
> user can take dump of some useful information if any.
>
> By setting LSN very, very high user might loose the information which he
> wants to take dump.

Which information would that loose? We don't currently use the LSN for
tuple visibility. And you sure wouldn't do anything but dump such a
cluster.
Now you could argue that you could modify this to find the current xid
used - but that's not that easy due to the wraparound semantics of
xids. And you are more likely to be successfull by looking at pg_clog.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wednesday, June 26, 2013 1:20 PM Andres Freund wrote:
> On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
> > On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> > > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> > > > Amit posted a new version of this patch on January 23rd. But
> last
> > > > comment on it by Tom is "not sure everyone wants this".
> > > >
> > > > https://commitfest.postgresql.org/action/patch_view?id=905
> > >
> > > > ... so, what's the status of this patch?
> > >
> > > That comment was referencing a mail of mine - so perhaps I better
> > > explain:
> > >
> > > I think the usecase for this utility isn't big enough to be
> included in
> > > postgres since it really can only help in a very limited
> > > circumstances. And I think it's too likely to be misused for stuff
> it's
> > > not useable for (e.g. remastering).
> > >
> > > The only scenario I see is that somebody deleted/corrupted
> > > pg_controldata. In that scenario the tool is supposed to be used to
> > > find
> > > the biggest lsn used so far so the user then can use pg_resetxlog
> to
> > > set
> > > that as the wal starting point.
> > > But that can be way much easier solved by just setting the LSN to
> > > something very, very high. The database cannot be used for anything
> > > reliable afterwards anyway.
> >
> > One of the main reason this was written was to make server up in case
> of
> > corruption and
> > user can take dump of some useful information if any.
> >
> > By setting LSN very, very high user might loose the information which
> he
> > wants to take dump.
>
> Which information would that loose?
Information from WAL replay which can be more appropriate by selecting
LSN.
Also for a developer, guessing very high LSN might be easy, but for users
it might not be equally easy, and getting such value by utility would be
comfortable.

One more use case for which this utility was done is as below:
It will be used to decide that on new-standby (old-master) whether a full
backup is needed from
New-master(old-standby).
The backup is required 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.

> We don't currently use the LSN for
> tuple visibility. And you sure wouldn't do anything but dump such a
> cluster.
> Now you could argue that you could modify this to find the current xid
> used - but that's not that easy due to the wraparound semantics of
> xids. And you are more likely to be successfull by looking at pg_clog.

With Regards,
Amit Kapila.

Hi Amit,

On 2013-06-26 16:22:28 +0530, Amit Kapila wrote:
> On Wednesday, June 26, 2013 1:20 PM Andres Freund wrote:
> > On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
> > > On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> > > > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> > > > > Amit posted a new version of this patch on January 23rd. But
> > last
> > > > > comment on it by Tom is "not sure everyone wants this".
> > > > >
> > > > > https://commitfest.postgresql.org/action/patch_view?id=905
> > > >
> > > > > ... so, what's the status of this patch?
> > > >
> > > > That comment was referencing a mail of mine - so perhaps I better
> > > > explain:
> > > >
> > > > I think the usecase for this utility isn't big enough to be
> > included in
> > > > postgres since it really can only help in a very limited
> > > > circumstances. And I think it's too likely to be misused for stuff
> > it's
> > > > not useable for (e.g. remastering).
> > > >
> > > > The only scenario I see is that somebody deleted/corrupted
> > > > pg_controldata. In that scenario the tool is supposed to be used to
> > > > find
> > > > the biggest lsn used so far so the user then can use pg_resetxlog
> > to
> > > > set
> > > > that as the wal starting point.
> > > > But that can be way much easier solved by just setting the LSN to
> > > > something very, very high. The database cannot be used for anything
> > > > reliable afterwards anyway.
> > >
> > > One of the main reason this was written was to make server up in case
> > of
> > > corruption and
> > > user can take dump of some useful information if any.
> > >
> > > By setting LSN very, very high user might loose the information which
> > he
> > > wants to take dump.
> >
> > Which information would that loose?
> Information from WAL replay which can be more appropriate by selecting
> LSN.

Sorry, I can't follow. If wal replay still is an option you can just
look at the WAL and get a sensible value way easier. The whole tool
seems to only make sense if you've lost pg_xlog.

> Also for a developer, guessing very high LSN might be easy, but for users
> it might not be equally easy, and getting such value by utility would be
> comfortable.

Well, then we can just document some very high lsn and be done with
it. Like CF000000/00000000.
That would leave enough space for eventual writes caused while dumping
the database (say hint bit writes in a checksummed database) and cannot
yet be realistically be reached during normal operation.

> One more use case for which this utility was done is as below:
> It will be used to decide that on new-standby (old-master) whether a full
> backup is needed from
> New-master(old-standby).
> The backup is required 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.

That's exactly what I was afraid of. Unless I miss something the tool is
*NOT* sufficient to do this. Look at the mail introducing pg_rewind and
the ensuing discussion for what's necessary for that.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wednesday, June 26, 2013 4:40 PM Andres Freund wrote:
> Hi Amit,
>
> On 2013-06-26 16:22:28 +0530, Amit Kapila wrote:
> > On Wednesday, June 26, 2013 1:20 PM Andres Freund wrote:
> > > On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
> > > > On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> > > > > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> > > > > > Amit posted a new version of this patch on January 23rd. But
> > > last
> > > > > > comment on it by Tom is "not sure everyone wants this".
> > > > > >
> > > > > > https://commitfest.postgresql.org/action/patch_view?id=905
> > > > >
> > > > > > ... so, what's the status of this patch?
> > > > >
> > > > > That comment was referencing a mail of mine - so perhaps I
> better
> > > > > explain:
> > > > >
> > > > > I think the usecase for this utility isn't big enough to be
> > > included in
> > > > > postgres since it really can only help in a very limited
> > > > > circumstances. And I think it's too likely to be misused for
> stuff
> > > it's
> > > > > not useable for (e.g. remastering).
> > > > >
> > > > > The only scenario I see is that somebody deleted/corrupted
> > > > > pg_controldata. In that scenario the tool is supposed to be
> used to
> > > > > find
> > > > > the biggest lsn used so far so the user then can use
> pg_resetxlog
> > > to
> > > > > set
> > > > > that as the wal starting point.
> > > > > But that can be way much easier solved by just setting the LSN
> to
> > > > > something very, very high. The database cannot be used for
> anything
> > > > > reliable afterwards anyway.
> > > >
> > > > One of the main reason this was written was to make server up in
> case
> > > of
> > > > corruption and
> > > > user can take dump of some useful information if any.
> > > >
> > > > By setting LSN very, very high user might loose the information
> which
> > > he
> > > > wants to take dump.
> > >
> > > Which information would that loose?
> > Information from WAL replay which can be more appropriate by
> selecting
> > LSN.
>
> Sorry, I can't follow. If wal replay still is an option you can just
> look at the WAL and get a sensible value way easier.

Originally 2 parts were proposed, one was to get LSN from data pages and
other from data pages.
Original proposal is:
http://www.postgresql.org/message-id/6C0B27F7206C9E4CA54AE035729E9C382851FFA
1(at)szxeml509-mbs

The second part for looking into WAL was written but due to xlogreader
patch, it was postponed and I didn't get time after that
to pursue it.

>The whole tool
> seems to only make sense if you've lost pg_xlog.

The tool's initial intent was if pg_controldata is lost and this idea is
originated in below mail chain:
http://www.postgresql.org/message-id/4274.1340084598@sss.pgh.pa.us

> > Also for a developer, guessing very high LSN might be easy, but for
> users
> > it might not be equally easy, and getting such value by utility
> would be
> > comfortable.
>
> Well, then we can just document some very high lsn and be done with
> it. Like CF000000/00000000.
> That would leave enough space for eventual writes caused while dumping
> the database (say hint bit writes in a checksummed database) and cannot
> yet be realistically be reached during normal operation.

Can we be ultra sure, that this LSN is not reached. I think it will take
vary long to reach such LSN, but still theoretically it can be possible.
I don't have any evidence.

> > One more use case for which this utility was done is as below:
> > It will be used to decide that on new-standby (old-master) whether
> a full
> > backup is needed from
> > New-master(old-standby).
> > The backup is required 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.
>
> That's exactly what I was afraid of. Unless I miss something the tool
> is
> *NOT* sufficient to do this.

You mean to say if user knows the max LSN of data pages in old-master and
last applied LSN in new master, he cannot decide whether
Full backup is needed? It should be straightforward decision that skip a
backup if that old-master LSN is less than the new-master (i.e., last
applied LSN, IOW, timeline switch LSN).
It was proposed as a usecase in this below mail:
http://www.postgresql.org/message-id/CAHGQGwHyd1fY0hF0qKh0-uKDh-gcbYxMOFBYVk
Kh4jzji-FCfg(at)mail(dot)gmail(dot)com

> Look at the mail introducing pg_rewind and
> the ensuing discussion for what's necessary for that.

I had briefly looked into that discussion at the time it was going on, but I
will look into it more carefully.

With Regards,
Amit Kapila.

On Wed, Jun 26, 2013 at 8:57 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Wednesday, June 26, 2013 4:40 PM Andres Freund wrote:
>> Hi Amit,
>>
>> On 2013-06-26 16:22:28 +0530, Amit Kapila wrote:
>> > On Wednesday, June 26, 2013 1:20 PM Andres Freund wrote:
>> > > On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
>> > > > On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
>> > > > > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
>> > > > > > Amit posted a new version of this patch on January 23rd. But
>> > > last
>> > > > > > comment on it by Tom is "not sure everyone wants this".
>> > > > > >
>> > > > > > https://commitfest.postgresql.org/action/patch_view?id=905
>> > > > >
>> > > > > > ... so, what's the status of this patch?
>> > > > >
>> > > > > That comment was referencing a mail of mine - so perhaps I
>> better
>> > > > > explain:
>> > > > >
>> > > > > I think the usecase for this utility isn't big enough to be
>> > > included in
>> > > > > postgres since it really can only help in a very limited
>> > > > > circumstances. And I think it's too likely to be misused for
>> stuff
>> > > it's
>> > > > > not useable for (e.g. remastering).
>> > > > >
>> > > > > The only scenario I see is that somebody deleted/corrupted
>> > > > > pg_controldata. In that scenario the tool is supposed to be
>> used to
>> > > > > find
>> > > > > the biggest lsn used so far so the user then can use
>> pg_resetxlog
>> > > to
>> > > > > set
>> > > > > that as the wal starting point.
>> > > > > But that can be way much easier solved by just setting the LSN
>> to
>> > > > > something very, very high. The database cannot be used for
>> anything
>> > > > > reliable afterwards anyway.
>> > > >
>> > > > One of the main reason this was written was to make server up in
>> case
>> > > of
>> > > > corruption and
>> > > > user can take dump of some useful information if any.
>> > > >
>> > > > By setting LSN very, very high user might loose the information
>> which
>> > > he
>> > > > wants to take dump.
>> > >
>> > > Which information would that loose?
>> > Information from WAL replay which can be more appropriate by
>> selecting
>> > LSN.
>>
>> Sorry, I can't follow. If wal replay still is an option you can just
>> look at the WAL and get a sensible value way easier.
>
> Originally 2 parts were proposed, one was to get LSN from data pages and
> other from data pages.
> Original proposal is:
> http://www.postgresql.org/message-id/6C0B27F7206C9E4CA54AE035729E9C382851FFA
> 1(at)szxeml509-mbs
>
> The second part for looking into WAL was written but due to xlogreader
> patch, it was postponed and I didn't get time after that
> to pursue it.
>
>
>>The whole tool
>> seems to only make sense if you've lost pg_xlog.
>
> The tool's initial intent was if pg_controldata is lost and this idea is
> originated in below mail chain:
> http://www.postgresql.org/message-id/4274.1340084598@sss.pgh.pa.us
>
>
>> > Also for a developer, guessing very high LSN might be easy, but for
>> users
>> > it might not be equally easy, and getting such value by utility
>> would be
>> > comfortable.
>>
>> Well, then we can just document some very high lsn and be done with
>> it. Like CF000000/00000000.
>> That would leave enough space for eventual writes caused while dumping
>> the database (say hint bit writes in a checksummed database) and cannot
>> yet be realistically be reached during normal operation.
>
> Can we be ultra sure, that this LSN is not reached. I think it will take
> vary long to reach such LSN, but still theoretically it can be possible.
> I don't have any evidence.
>
>> > One more use case for which this utility was done is as below:
>> > It will be used to decide that on new-standby (old-master) whether
>> a full
>> > backup is needed from
>> > New-master(old-standby).
>> > The backup is required 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.
>>
>> That's exactly what I was afraid of. Unless I miss something the tool
>> is
>> *NOT* sufficient to do this.
>
> You mean to say if user knows the max LSN of data pages in old-master and
> last applied LSN in new master, he cannot decide whether
> Full backup is needed? It should be straightforward decision that skip a
> backup if that old-master LSN is less than the new-master (i.e., last
> applied LSN, IOW, timeline switch LSN).
> It was proposed as a usecase in this below mail:
> http://www.postgresql.org/message-id/CAHGQGwHyd1fY0hF0qKh0-uKDh-gcbYxMOFBYVk
> Kh4jzji-FCfg(at)mail(dot)gmail(dot)com

I guess he meant the commit hint bit problem.

Regards,

--
Fujii Masao

On Wednesday, June 26, 2013 10:19 PM Fujii Masao wrote:
> On Wed, Jun 26, 2013 at 8:57 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > On Wednesday, June 26, 2013 4:40 PM Andres Freund wrote:
> >> Hi Amit,
> >>
> >> On 2013-06-26 16:22:28 +0530, Amit Kapila wrote:
> >> > On Wednesday, June 26, 2013 1:20 PM Andres Freund wrote:
> >> > > On 2013-06-26 08:50:27 +0530, Amit Kapila wrote:
> >> > > > On Tuesday, June 25, 2013 11:12 PM Andres Freund wrote:
> >> > > > > On 2013-06-16 17:19:49 -0700, Josh Berkus wrote:
> >> > > > > > Amit posted a new version of this patch on January 23rd.
> But
> >> > > last
> >> > > > > > comment on it by Tom is "not sure everyone wants this".
> >> > > > > >
> >> > > > > > https://commitfest.postgresql.org/action/patch_view?id=905
> >> > > > >
> >> > > > > > ... so, what's the status of this patch?
> >> > > > >
> >> > > > > That comment was referencing a mail of mine - so perhaps I
> >> better
> >> > > > > explain:
> >> > > > >
> >> > > > > I think the usecase for this utility isn't big enough to be
> >> > > included in
> >> > > > > postgres since it really can only help in a very limited
> >> > > > > circumstances. And I think it's too likely to be misused for
> >> stuff
> >> > > it's
> >> > > > > not useable for (e.g. remastering).
> >> > > > >
> >> > > > > The only scenario I see is that somebody deleted/corrupted
> >> > > > > pg_controldata. In that scenario the tool is supposed to be
> >> used to
> >> > > > > find
> >> > > > > the biggest lsn used so far so the user then can use
> >> pg_resetxlog
> >> > > to
> >> > > > > set
> >> > > > > that as the wal starting point.
> >> > > > > But that can be way much easier solved by just setting the
> LSN
> >> to
> >> > > > > something very, very high. The database cannot be used for
> >> anything
> >> > > > > reliable afterwards anyway.
> >> > > >
> >> > > > One of the main reason this was written was to make server up
> in
> >> case
> >> > > of
> >> > > > corruption and
> >> > > > user can take dump of some useful information if any.
> >> > > >
> >> > > > By setting LSN very, very high user might loose the
> information
> >> which
> >> > > he
> >> > > > wants to take dump.
> >> > >
> >> > > Which information would that loose?
> >> > Information from WAL replay which can be more appropriate by
> >> selecting
> >> > LSN.
> >>
> >> Sorry, I can't follow. If wal replay still is an option you can just
> >> look at the WAL and get a sensible value way easier.
> >
> > Originally 2 parts were proposed, one was to get LSN from data pages
> and
> > other from data pages.
> > Original proposal is:
> > http://www.postgresql.org/message-
> id/6C0B27F7206C9E4CA54AE035729E9C382851FFA
> > 1(at)szxeml509-mbs
> >
> > The second part for looking into WAL was written but due to
> xlogreader
> > patch, it was postponed and I didn't get time after that
> > to pursue it.
> >
> >
> >>The whole tool
> >> seems to only make sense if you've lost pg_xlog.
> >
> > The tool's initial intent was if pg_controldata is lost and this idea
> is
> > originated in below mail chain:
> > http://www.postgresql.org/message-id/4274.1340084598@sss.pgh.pa.us
> >
> >
> >> > Also for a developer, guessing very high LSN might be easy, but
> for
> >> users
> >> > it might not be equally easy, and getting such value by utility
> >> would be
> >> > comfortable.
> >>
> >> Well, then we can just document some very high lsn and be done with
> >> it. Like CF000000/00000000.
> >> That would leave enough space for eventual writes caused while
> dumping
> >> the database (say hint bit writes in a checksummed database) and
> cannot
> >> yet be realistically be reached during normal operation.
> >
> > Can we be ultra sure, that this LSN is not reached. I think it will
> take
> > vary long to reach such LSN, but still theoretically it can be
> possible.
> > I don't have any evidence.
> >
> >> > One more use case for which this utility was done is as below:
> >> > It will be used to decide that on new-standby (old-master)
> whether
> >> a full
> >> > backup is needed from
> >> > New-master(old-standby).
> >> > The backup is required 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.
> >>
> >> That's exactly what I was afraid of. Unless I miss something the
> tool
> >> is
> >> *NOT* sufficient to do this.
> >
> > You mean to say if user knows the max LSN of data pages in old-master
> and
> > last applied LSN in new master, he cannot decide whether
> > Full backup is needed? It should be straightforward decision that
> skip a
> > backup if that old-master LSN is less than the new-master (i.e., last
> > applied LSN, IOW, timeline switch LSN).
> > It was proposed as a usecase in this below mail:
> > http://www.postgresql.org/message-id/CAHGQGwHyd1fY0hF0qKh0-uKDh-
> gcbYxMOFBYVk
> > Kh4jzji-FCfg(at)mail(dot)gmail(dot)com
>
> I guess he meant the commit hint bit problem.

True, after reading the thread mentioned by Andres, I got the reason he was
pointing why it is not sufficient.
So can it be useful if database has checksums enabled?

With Regards,
Amit Kapila.

On 2013-06-27 11:16:25 +0530, Amit Kapila wrote:
> On Wednesday, June 26, 2013 10:19 PM Fujii Masao wrote:
> > On Wed, Jun 26, 2013 at 8:57 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> > >> > One more use case for which this utility was done is as below:
> > >> > It will be used to decide that on new-standby (old-master)
> > whether
> > >> a full
> > >> > backup is needed from
> > >> > New-master(old-standby).
> > >> > The backup is required 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.
> > >>
> > >> That's exactly what I was afraid of. Unless I miss something the
> > tool
> > >> is
> > >> *NOT* sufficient to do this.
> > >
> > > You mean to say if user knows the max LSN of data pages in old-master
> > and
> > > last applied LSN in new master, he cannot decide whether
> > > Full backup is needed? It should be straightforward decision that
> > skip a
> > > backup if that old-master LSN is less than the new-master (i.e., last
> > > applied LSN, IOW, timeline switch LSN).
> > > It was proposed as a usecase in this below mail:
> > > http://www.postgresql.org/message-id/CAHGQGwHyd1fY0hF0qKh0-uKDh-
> > gcbYxMOFBYVk
> > > Kh4jzji-FCfg(at)mail(dot)gmail(dot)com
> >
> > I guess he meant the commit hint bit problem.
>
> True, after reading the thread mentioned by Andres, I got the reason he was
> pointing why it is not sufficient.
> So can it be useful if database has checksums enabled?

I think for that usecase its far more useful to work on getting
pg_rewind since that has a chance of working when local WAL has been
applied that hadn't yet shipped to the other side (which is frequently
the case).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thursday, June 27, 2013 11:26 AM Andres Freund wrote:
> On 2013-06-27 11:16:25 +0530, Amit Kapila wrote:
> > On Wednesday, June 26, 2013 10:19 PM Fujii Masao wrote:
> > > On Wed, Jun 26, 2013 at 8:57 PM, Amit Kapila
> <amit(dot)kapila(at)huawei(dot)com>
> > > >> > One more use case for which this utility was done is as
> below:
> > > >> > It will be used to decide that on new-standby (old-master)
> > > whether
> > > >> a full
> > > >> > backup is needed from
> > > >> > New-master(old-standby).
> > > >> > The backup is required 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.
> > > >>
> > > >> That's exactly what I was afraid of. Unless I miss something the
> > > tool
> > > >> is
> > > >> *NOT* sufficient to do this.
> > > >
> > > > You mean to say if user knows the max LSN of data pages in old-
> master
> > > and
> > > > last applied LSN in new master, he cannot decide whether
> > > > Full backup is needed? It should be straightforward decision that
> > > skip a
> > > > backup if that old-master LSN is less than the new-master (i.e.,
> last
> > > > applied LSN, IOW, timeline switch LSN).
> > > > It was proposed as a usecase in this below mail:
> > > > http://www.postgresql.org/message-id/CAHGQGwHyd1fY0hF0qKh0-uKDh-
> > > gcbYxMOFBYVk
> > > > Kh4jzji-FCfg(at)mail(dot)gmail(dot)com
> > >
> > > I guess he meant the commit hint bit problem.
> >
> > True, after reading the thread mentioned by Andres, I got the reason
> he was
> > pointing why it is not sufficient.
> > So can it be useful if database has checksums enabled?
>
> I think for that usecase its far more useful to work on getting
> pg_rewind since that has a chance of working when local WAL has been
> applied that hadn't yet shipped to the other side (which is frequently
> the case).

Aren't the use case for both is bit different
Pg_computmaxlsn - by computing max lsn for checksums enabled database, user
can made old-master follow new-master if maxlsn suggests that fullbackup is
not required.

Pg_rewind - a tool to resynchronize old-master and new-master by
copying changed blocks from new master.
I think more work might be needed in case DDL's happened
on old-master after forking of new-master.

Although for this case, both have resemblance in terms of avoiding full
backup, but I think maxlsn tool can be independently also used.
Do you think pg_rewind can be used by default for any checksum enabled
database to resynchronize old-master?

With Regards,
Amit Kapila.

On Tue, Jun 25, 2013 at 1:42 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I think the usecase for this utility isn't big enough to be included in
> postgres since it really can only help in a very limited
> circumstances. And I think it's too likely to be misused for stuff it's
> not useable for (e.g. remastering).
>
> The only scenario I see is that somebody deleted/corrupted
> pg_controldata. In that scenario the tool is supposed to be used to find
> the biggest lsn used so far so the user then can use pg_resetxlog to set
> that as the wal starting point.
> But that can be way much easier solved by just setting the LSN to
> something very, very high. The database cannot be used for anything
> reliable afterwards anyway.

I guess this is true, but I think I'm mildly in favor of including
this anyway. I think I would have used it once or twice, if it had
been there - maybe not even to feed into pg_resetxlog, but just to
check for future LSNs. We don't have anything like a suite of
diagnostic tools in bin or contrib today, for use by database
professionals in fixing things that fall strictly in the realm of
"don't try this at home", and I think we should have such a thing.
Unfortunately this covers about 0.1% of the space I'd like to see
covered, which might be a reason to reject it or at least insist that
it be enhanced first.

At any rate, I don't think this is anywhere near committable as it
stands today. Some random review comments:

remove_parent_refernces() is spelled wrong.

Why does this patch need all of this fancy path-handling logic -
specifically, remove_parent_refernces() and make_absolute_path()?
Surely its needs are not that different from pg_ctl or pg_resetxlog or
pg_controldata. If they all need it and it's duplicated, we should
fix that. Otherwise, why the special logic here?

I don't think we need getLinkPath() either. The server has no trouble
finding its files by just using a pathname that follows the symlink.
Why do we need anything different here? The whole point of symlinks
is that you can traverse them transparently, without knowing where
they point.

Duplicating PageHeaderIsValid doesn't seem acceptable. Moreover,
since the point of this is to be able to use it on a damaged cluster,
why is that logic even desirable? I think we'd be better off assuming
the pages to be valid.

The calling convention for this utility seems quite baroque. There's
no real need for all of this -p/-P stuff. I think the syntax should
just be:

pg_computemaxlsn file_or_directory...

For each argument, we determine whether it's a file or a directory.
If it's a file, we assume it's a PostgreSQL data file and scan it. If
it's a directory, we check whether it looks like a data directory. If
it does, we recurse through the whole tree structure and find the data
files, and process them. If it doesn't look like a data directory, we
scan each plain file in that directory whose name looks like a
PostgreSQL data file name. With this approach, there's no need to
limit ourselves to a single input argument and no need to specify what
kind of argument it is; the tool just figures it out.

I think it would be a good idea to have a mode that prints out the max
LSN found in *each data file* scanned, and then prints out the overall
max found at the end. In fact, I think that should perhaps be the
default mode, with -q, --quiet to disable it. When printing out the
per-file data, I think it would be useful to track and print the block
number where the highest LSN in that file was found. I have
definitely had cases where I suspected, but was not certain of,
corruption. One could use a tool like this to hunt for problems, and
then use something like pg_filedump to dump the offending blocks.
That would be a lot easier than running pg_filedump on the whole file
and grepping through the output.

Similarly, I see no reason for the restriction imposed by
check_path_belongs_to_pgdata(). I've had people mail me one data
file; why shouldn't I be able to run this tool on it? It's a
read-only utility.

- if (0 != FindMaxLSNinDir(pathbuf, maxlsn, false)) and similar is not
PostgreSQL style.

+ printf(_("%s compute the maximum LSN in PostgreSQL data
pages.\n\n"), progname);

s/compute/computes/

+ /*
+ * Don't allow pg_computemaxlsn to be run as root, to avoid overwriting
+ * the ownership of files in the data directory. We need only check for
+ * root -- any other user won't have sufficient permissions to modify
+ * files in the data directory.
+ */
+ #ifndef WIN32
+ if (geteuid() == 0)
+ {
+ fprintf(stderr, _("%s: cannot be executed by \"root\".\n"),
+ progname);
+ fprintf(stderr, _("You must run %s as the PostgreSQL
superuser.\n"),
+ progname);
+ exit(1);
+ }
+ #endif

This utility only reads files; it does not modify them. So this seems
unnecessary. I assume it's blindly copied from somewhere else.

+ fprintf(stderr, _("%s: \"%s\" not a valid data directory.\n"),

Extra space.

+ /*
+ * Check for a postmaster lock file --- if there is one, refuse to
+ * proceed, on grounds we might be interfering with a live installation.
+ */
+ snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);

Again, this might be appropriate for pg_resetxlog, but I see no reason
for the restriction here. The output might be inaccurate in that
case, but if you're using this tool you're required to know what
you're doing.

+ For safety reasons, you must specify the data directory on the
command line.
+ <command>pg_computemaxlsn</command> does not use the environment variable
+ <envar>PGDATA</>.

Same thing here. I think using PGDATA would be quite appropriate for
this utility.

+ <para>
+ This command must not be used when the server is
+ running. <command>pg_computemaxlsn</command> will refuse to start up if
+ it finds a server lock file in the data directory. If the
+ server crashed then a lock file might have been left
+ behind; in that case you can remove the lock file to allow
+ <command>pg_computemaxlsn</command> to run. But before you do
+ so, make doubly certain that there is no server process still alive.
+ </para>

More of the same paranoia.

Overall my feeling is that this can be simplified quite a lot.
There's a lot of stuff in here that's really not needed.

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

On Wednesday, July 03, 2013 1:26 AM Robert Haas wrote:
> On Tue, Jun 25, 2013 at 1:42 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
> > I think the usecase for this utility isn't big enough to be included
> in
> > postgres since it really can only help in a very limited
> > circumstances. And I think it's too likely to be misused for stuff
> it's
> > not useable for (e.g. remastering).
> >
> > The only scenario I see is that somebody deleted/corrupted
> > pg_controldata. In that scenario the tool is supposed to be used to
> find
> > the biggest lsn used so far so the user then can use pg_resetxlog to
> set
> > that as the wal starting point.
> > But that can be way much easier solved by just setting the LSN to
> > something very, very high. The database cannot be used for anything
> > reliable afterwards anyway.
>
> I guess this is true, but I think I'm mildly in favor of including
> this anyway. I think I would have used it once or twice, if it had
> been there - maybe not even to feed into pg_resetxlog, but just to
> check for future LSNs. We don't have anything like a suite of
> diagnostic tools in bin or contrib today, for use by database
> professionals in fixing things that fall strictly in the realm of
> "don't try this at home", and I think we should have such a thing.
> Unfortunately this covers about 0.1% of the space I'd like to see
> covered, which might be a reason to reject it or at least insist that
> it be enhanced first.
>
> At any rate, I don't think this is anywhere near committable as it
> stands today. Some random review comments:
>
> remove_parent_refernces() is spelled wrong.

It was corrected in last posted version.
http://www.postgresql.org/message-id/6C0B27F7206C9E4CA54AE035729E9C383BEB928
8(at)szxeml509-mbx

> Why does this patch need all of this fancy path-handling logic -
> specifically, remove_parent_refernces() and make_absolute_path()?
> Surely its needs are not that different from pg_ctl or pg_resetxlog or
> pg_controldata. If they all need it and it's duplicated, we should
> fix that. Otherwise, why the special logic here?
>
> I don't think we need getLinkPath() either. The server has no trouble
> finding its files by just using a pathname that follows the symlink.
> Why do we need anything different here? The whole point of symlinks
> is that you can traverse them transparently, without knowing where
> they point.

It is to handle negative scenario's like if there is any recursion in path.
However if you feel this is not important, it can be removed.

> Duplicating PageHeaderIsValid doesn't seem acceptable. Moreover,
> since the point of this is to be able to use it on a damaged cluster,
> why is that logic even desirable? I think we'd be better off assuming
> the pages to be valid.
>
> The calling convention for this utility seems quite baroque. There's
> no real need for all of this -p/-P stuff. I think the syntax should
> just be:
>
> pg_computemaxlsn file_or_directory...
>
> For each argument, we determine whether it's a file or a directory.
> If it's a file, we assume it's a PostgreSQL data file and scan it. If
> it's a directory, we check whether it looks like a data directory. If
> it does, we recurse through the whole tree structure and find the data
> files, and process them. If it doesn't look like a data directory, we
> scan each plain file in that directory whose name looks like a
> PostgreSQL data file name. With this approach, there's no need to
> limit ourselves to a single input argument and no need to specify what
> kind of argument it is; the tool just figures it out.
>
> I think it would be a good idea to have a mode that prints out the max
> LSN found in *each data file* scanned, and then prints out the overall
> max found at the end. In fact, I think that should perhaps be the
> default mode, with -q, --quiet to disable it.

Printing too many LSN for each file might fill user's screen and he might be
needing only overall LSN.
Should we keep -p --printall as option to print all LSN's and keep default
as overall max LSN?

With Regards,
Amit Kapila.

On Wed, Jul 3, 2013 at 8:44 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
>> Why does this patch need all of this fancy path-handling logic -
>> specifically, remove_parent_refernces() and make_absolute_path()?
>> Surely its needs are not that different from pg_ctl or pg_resetxlog or
>> pg_controldata. If they all need it and it's duplicated, we should
>> fix that. Otherwise, why the special logic here?
>>
>> I don't think we need getLinkPath() either. The server has no trouble
>> finding its files by just using a pathname that follows the symlink.
>> Why do we need anything different here? The whole point of symlinks
>> is that you can traverse them transparently, without knowing where
>> they point.
>
> It is to handle negative scenario's like if there is any recursion in path.
> However if you feel this is not important, it can be removed.

I'm having a hard time imagining a situation where that would be a
problem. If the symlink points to itself somehow, the OS will throw
an error. If your filesystem is so badly hosed that the directory
structure is more fundamentally broken than the OS's circular-symlink
detection code can handle, whether or not this utility works is a
second-order consideration. What kind of scenario are you imagining?

>> I think it would be a good idea to have a mode that prints out the max
>> LSN found in *each data file* scanned, and then prints out the overall
>> max found at the end. In fact, I think that should perhaps be the
>> default mode, with -q, --quiet to disable it.
>
> Printing too many LSN for each file might fill user's screen and he might be
> needing only overall LSN.
> Should we keep -p --printall as option to print all LSN's and keep default
> as overall max LSN?

Honestly, I have a hard time imagining the use case for that mode.
This isn't a tool that people should be running regularly, and some
output that lends a bit of confidence that the tool is doing the right
thing seems like a good thing. Keep in mind it's likely to run for
quite a while, too, and this would provide a progress indicator. I'll
defer to whatever the consensus is here but my gut feeling is that if
you don't want that extra output, there's a good chance you're
misusing the tool.

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

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: Wednesday, July 03, 2013 6:40 PM
> To: Amit Kapila
> Cc: Andres Freund; Josh Berkus; pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
>
> On Wed, Jul 3, 2013 at 8:44 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> >> Why does this patch need all of this fancy path-handling logic -
> >> specifically, remove_parent_refernces() and make_absolute_path()?
> >> Surely its needs are not that different from pg_ctl or pg_resetxlog
> or
> >> pg_controldata. If they all need it and it's duplicated, we should
> >> fix that. Otherwise, why the special logic here?
> >>
> >> I don't think we need getLinkPath() either. The server has no
> trouble
> >> finding its files by just using a pathname that follows the symlink.
> >> Why do we need anything different here? The whole point of symlinks
> >> is that you can traverse them transparently, without knowing where
> >> they point.
> >
> > It is to handle negative scenario's like if there is any recursion in
> path.
> > However if you feel this is not important, it can be removed.
>
> I'm having a hard time imagining a situation where that would be a
> problem. If the symlink points to itself somehow, the OS will throw
> an error. If your filesystem is so badly hosed that the directory
> structure is more fundamentally broken than the OS's circular-symlink
> detection code can handle, whether or not this utility works is a
> second-order consideration. What kind of scenario are you imagining?

amit(at)linux:~> md test
amit(at)linux:~> cd test
amit(at)linux:~/test> ln -s ~/test link_test
amit(at)linux:~/test> ls
link_test
amit(at)linux:~/test> cd link_test
amit(at)linux:~/test/link_test> ls
link_test
amit(at)linux:~/test/link_test> cd link_test
amit(at)linux:~/test/link_test/link_test> cd link_test
amit(at)linux:~/test/link_test/link_test/link_test> pwd
/home/amit/test/link_test/link_test/link_test
amit(at)linux:~/test/link_test/link_test/link_test>

Platform details
----------------
Suse - 11.2
Kernel - 3.0.13

This is to avoid when user has given some path where db files are present.

> >> I think it would be a good idea to have a mode that prints out the
> max
> >> LSN found in *each data file* scanned, and then prints out the
> overall
> >> max found at the end. In fact, I think that should perhaps be the
> >> default mode, with -q, --quiet to disable it.
> >
> > Printing too many LSN for each file might fill user's screen and he
> might be
> > needing only overall LSN.
> > Should we keep -p --printall as option to print all LSN's and keep
> default
> > as overall max LSN?
>
> Honestly, I have a hard time imagining the use case for that mode.
> This isn't a tool that people should be running regularly, and some
> output that lends a bit of confidence that the tool is doing the right
> thing seems like a good thing. Keep in mind it's likely to run for
> quite a while, too, and this would provide a progress indicator. I'll
> defer to whatever the consensus is here but my gut feeling is that if
> you don't want that extra output, there's a good chance you're
> misusing the tool.

With Regards,
Amit Kapila.

On Wed, Jul 3, 2013 at 9:51 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> amit(at)linux:~> md test
> amit(at)linux:~> cd test
> amit(at)linux:~/test> ln -s ~/test link_test
> amit(at)linux:~/test> ls
> link_test
> amit(at)linux:~/test> cd link_test
> amit(at)linux:~/test/link_test> ls
> link_test
> amit(at)linux:~/test/link_test> cd link_test
> amit(at)linux:~/test/link_test/link_test> cd link_test
> amit(at)linux:~/test/link_test/link_test/link_test> pwd
> /home/amit/test/link_test/link_test/link_test
> amit(at)linux:~/test/link_test/link_test/link_test>

So what?

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

On Wednesday, July 03, 2013 7:41 PM Robert Haas wrote:
> On Wed, Jul 3, 2013 at 9:51 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > amit(at)linux:~> md test
> > amit(at)linux:~> cd test
> > amit(at)linux:~/test> ln -s ~/test link_test
> > amit(at)linux:~/test> ls
> > link_test
> > amit(at)linux:~/test> cd link_test
> > amit(at)linux:~/test/link_test> ls
> > link_test
> > amit(at)linux:~/test/link_test> cd link_test
> > amit(at)linux:~/test/link_test/link_test> cd link_test
> > amit(at)linux:~/test/link_test/link_test/link_test> pwd
> > /home/amit/test/link_test/link_test/link_test
> > amit(at)linux:~/test/link_test/link_test/link_test>
>
> So what?

It can cause error "too many levels of symbolic links"

Point was that in case of symlinks we only want to allow PG_ paths, so that
such situation can never occur.
However I think this is not important to handle by this utility, so we can
remove such handling.

With Regards,
Amit Kapila.

On Wednesday, July 03, 2013 1:26 AM Robert Haas Wrote:
>On Tue, Jun 25, 2013 at 1:42 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
>> I think the usecase for this utility isn't big enough to be included in
>> postgres since it really can only help in a very limited
>> circumstances. And I think it's too likely to be misused for stuff it's
>> not useable for (e.g. remastering).
>>
>> The only scenario I see is that somebody deleted/corrupted
>> pg_controldata. In that scenario the tool is supposed to be used to find
>> the biggest lsn used so far so the user then can use pg_resetxlog to set
>> that as the wal starting point.
>> But that can be way much easier solved by just setting the LSN to
>> something very, very high. The database cannot be used for anything
>> reliable afterwards anyway.

>I guess this is true, but I think I'm mildly in favor of including
>this anyway. I think I would have used it once or twice, if it had
>been there - maybe not even to feed into pg_resetxlog, but just to
>check for future LSNs. We don't have anything like a suite of
>diagnostic tools in bin or contrib today, for use by database
>professionals in fixing things that fall strictly in the realm of
>"don't try this at home", and I think we should have such a thing.
>Unfortunately this covers about 0.1% of the space I'd like to see
>covered, which might be a reason to reject it or at least insist that
>it be enhanced first.

>At any rate, I don't think this is anywhere near committable as it
>stands today. Some random review comments:

Thanks for the detailed review.

>remove_parent_refernces() is spelled wrong.

>Why does this patch need all of this fancy path-handling logic -
>specifically, remove_parent_refernces() and make_absolute_path()?
>Surely its needs are not that different from pg_ctl or pg_resetxlog or
>pg_controldata. If they all need it and it's duplicated, we should
>fix that. Otherwise, why the special logic here?

>I don't think we need getLinkPath() either. The server has no trouble
>finding its files by just using a pathname that follows the symlink.
>Why do we need anything different here? The whole point of symlinks
>is that you can traverse them transparently, without knowing where
>they point.

Removed the special handling of path functions.

>Duplicating PageHeaderIsValid doesn't seem acceptable. Moreover,
>since the point of this is to be able to use it on a damaged cluster,
>why is that logic even desirable? I think we'd be better off assuming
>the pages to be valid.

Corrected.

>The calling convention for this utility seems quite baroque. There's
>no real need for all of this -p/-P stuff. I think the syntax should
>just be:

>pg_computemaxlsn file_or_directory...

>For each argument, we determine whether it's a file or a directory.
>If it's a file, we assume it's a PostgreSQL data file and scan it. If
>it's a directory, we check whether it looks like a data directory. If
>it does, we recurse through the whole tree structure and find the data
>files, and process them. If it doesn't look like a data directory, we
>scan each plain file in that directory whose name looks like a
>PostgreSQL data file name. With this approach, there's no need to
>limit ourselves to a single input argument and no need to specify what
>kind of argument it is; the tool just figures it out.

Changed to accept file or directory without of options.

>I think it would be a good idea to have a mode that prints out the max
>LSN found in *each data file* scanned, and then prints out the overall
>max found at the end. In fact, I think that should perhaps be the
>default mode, with -q, --quiet to disable it. When printing out the
>per-file data, I think it would be useful to track and print the block
>number where the highest LSN in that file was found. I have
>definitely had cases where I suspected, but was not certain of,
>corruption. One could use a tool like this to hunt for problems, and
>then use something like pg_filedump to dump the offending blocks.
>That would be a lot easier than running pg_filedump on the whole file
>and grepping through the output.

Corrected.

>Similarly, I see no reason for the restriction imposed by
>check_path_belongs_to_pgdata(). I've had people mail me one data
>file; why shouldn't I be able to run this tool on it? It's a
>read-only utility.

>- if (0 != FindMaxLSNinDir(pathbuf, maxlsn, false)) and similar is not
>PostgreSQL style.

>+ printf(_("%s compute the maximum LSN in PostgreSQL data
>pages.\n\n"), progname);

Fixed.

>+ /*
>+ * Don't allow pg_computemaxlsn to be run as root, to avoid
overwriting
>+ * the ownership of files in the data directory. We need only check
for
>+ * root -- any other user won't have sufficient permissions to
modify
>+ * files in the data directory.
>+ */
>+ #ifndef WIN32
>+ if (geteuid() == 0)
>+ {
>+ fprintf(stderr, _("%s: cannot be executed by \"root\".\n"),
>+ progname);
>+ fprintf(stderr, _("You must run %s as the PostgreSQL
>superuser.\n"),
>+ progname);
>+ exit(1);
>+ }
>+ #endif

>This utility only reads files; it does not modify them. So this seems
>unnecessary. I assume it's blindly copied from somewhere else.

>+ fprintf(stderr, _("%s: \"%s\" not a valid data
directory.\n"),

>Extra space.

>+ /*
>+ * Check for a postmaster lock file --- if there is one, refuse to
>+ * proceed, on grounds we might be interfering with a live
installation.
>+ */
>+ snprintf(path, MAXPGPATH, "%s/postmaster.pid", DataDir);

>Again, this might be appropriate for pg_resetxlog, but I see no reason
>for the restriction here. The output might be inaccurate in that
>case, but if you're using this tool you're required to know what
>you're doing.

Fixed.

>+ For safety reasons, you must specify the data directory on the
>command line.
>+ <command>pg_computemaxlsn</command> does not use the environment
variable
>+ <envar>PGDATA</>.

>Same thing here. I think using PGDATA would be quite appropriate for
>this utility.

Fixed.

>+ <para>
>+ This command must not be used when the server is
>+ running. <command>pg_computemaxlsn</command> will refuse to start up
if
>+ it finds a server lock file in the data directory. If the
>+ server crashed then a lock file might have been left
>+ behind; in that case you can remove the lock file to allow
>+ <command>pg_computemaxlsn</command> to run. But before you do
>+ so, make doubly certain that there is no server process still alive.
>+ </para>

>More of the same paranoia.

>Overall my feeling is that this can be simplified quite a lot.
>There's a lot of stuff in here that's really not needed.

Corrected.

Please find the updated patch attached.

Regards,
Hari babu.

On Thu, Jul 4, 2013 at 2:14 AM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> It can cause error "too many levels of symbolic links"

Sure, so you report the error and exit. No problem.

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

This looks better.

+ fprintf(stderr, _("%s: .. file \"%s\" for seeking: %s\n"),
+ progname, filename, strerror(errno));

Weird error message style - what's with the ".."?

+ fprintf(stderr, _("%s: .. file \"%s\" length is more than segment
size: %d.\n"),
+ progname, filename, RELSEG_SIZE);

Again.

+ fprintf(stderr, _("%s: could not seek to next page \"%s\": %s\n"),
+ progname, filename, strerror(errno));

I think this should be written as: could not seek to offset NUMBER in
file "PATH"

+ fprintf(stderr, _("%s: could not read file \"%s\": %s\n"),
+ progname, filename, strerror(errno));

And this one as: could not read file "PATH" at offset NUMBER

+ printf("File:%s Maximum LSN found is: %X/%X \nWAL segment file
name (fileid,seg): %X/%X\n",

I think that we don't need to display the WAL segment file name for
the per-file progress updates. Instead, let's show the block number
where that LSN was found, like this:

Highest LSN for file "%s" is %X/%X in block %u.

The overall output can stay as you have it.

+ if (0 != result)

Coding style.

+ static int
+ FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn)

It seems that this function, and a few others, use -1 for a failure
return, 0 for success, and all others undefined. Although this is a
defensible choice, I think it would be more PG-like to make the return
value bool and use true for success and false for failure.

+ if (S_ISREG(statbuf.st_mode))
+ (void) FindMaxLSNinFile(pathbuf, maxlsn);
+ else
+ (void) FindMaxLSNinDir(pathbuf, maxlsn);

I don't see why we're throwing away the return value here. I would
expect the function to have a "bool result = true" at the top and sent
result = false if one of these functions returns false. At the end,
it returns result.

+ This utility can only be run by the user who installed the server, because
+ it requires read/write access to the data directory.

False.

+ LsnSearchPath = argv[optind];
+
+ if (LsnSearchPath == NULL)
+ LsnSearchPath = getenv("PGDATA");

I think you should write the code so that the tool loops over its
input arguments; if none, it processes $PGDATA. (Don't forget to
update the syntax synopsis and documentation to match.)

I think the documentation should say something about the intended uses
of this tool, including cautions against using it for things to which
it is not well-suited. I guess the threshold question for this patch
is whether those uses are enough to justify including the tool in the
core distribution.

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

On Thursday, July 04, 2013 11:19 PM Robert Haas wrote:

>+ fprintf(stderr, _("%s: .. file \"%s\" for seeking: %s\n"),
>+ progname, filename, strerror(errno));

>Weird error message style - what's with the ".."?

>+ fprintf(stderr, _("%s: .. file \"%s\" length is more than
segment
>size: %d.\n"),
>+ progname, filename, RELSEG_SIZE);

>Again.

Corrected.

>+ fprintf(stderr, _("%s: could not seek to next page
\"%s\": %s\n"),
>+ progname, filename,
strerror(errno));

>I think this should be written as: could not seek to offset NUMBER in
>file "PATH"

>+ fprintf(stderr, _("%s: could not read file \"%s\":
%s\n"),
>+ progname, filename,
strerror(errno));

>And this one as: could not read file "PATH" at offset NUMBER

>+ printf("File:%s Maximum LSN found is: %X/%X \nWAL segment
file
>name (fileid,seg): %X/%X\n",

>I think that we don't need to display the WAL segment file name for
>the per-file progress updates. Instead, let's show the block number
>where that LSN was found, like this:

>Highest LSN for file "%s" is %X/%X in block %u.

>The overall output can stay as you have it.

Changed as per your suggestion.

>+ if (0 != result)

>Coding style.

>+ static int
>+ FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn)

>It seems that this function, and a few others, use -1 for a failure
>return, 0 for success, and all others undefined. Although this is a
>defensible choice, I think it would be more PG-like to make the return
>value bool and use true for success and false for failure.

>+ if (S_ISREG(statbuf.st_mode))
>+ (void) FindMaxLSNinFile(pathbuf, maxlsn);
>+ else
>+ (void) FindMaxLSNinDir(pathbuf, maxlsn);

>I don't see why we're throwing away the return value here. I would
>expect the function to have a "bool result = true" at the top and sent
>result = false if one of these functions returns false. At the end,
>it returns result.

Fixed.

>+ This utility can only be run by the user who installed the server,
because
>+ it requires read/write access to the data directory.

>False.

Removed.

>+ LsnSearchPath = argv[optind];
>+
>+ if (LsnSearchPath == NULL)
>+ LsnSearchPath = getenv("PGDATA");

>I think you should write the code so that the tool loops over its
>input arguments; if none, it processes $PGDATA. (Don't forget to
>update the syntax synopsis and documentation to match.)

Added the functionality of multiple file or directories parsing and printing
Max LSN for each input argument.

>I think the documentation should say something about the intended uses
>of this tool, including cautions against using it for things to which
>it is not well-suited. I guess the threshold question for this patch
>is whether those uses are enough to justify including the tool in the
>core distribution.

Added some use cases and notes regarding the tool. Please suggest if any
More information needs to be documented.

Thanks for the review, please find the updated patch attached in the mail.

Regards,
Hari babu.

On Friday, July 05, 2013 6:48 PM Hari Babu wrote:
>On Thursday, July 04, 2013 11:19 PM Robert Haas wrote:

The patch is updated with the following changes.

1.If the input data is data directory, all the errors occurred are displayed
at once instead of one error at a time.
2.Fixed the problem of replacing result of the previous file or directory
result with new one.
3.Update the docs.

Thanks for the review, please find the updated patch attached in the mail.

Regards,
Hari babu.

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?
People falsely thinking that it can be used for this is the primary
reason for me objecting the patch...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

> People falsely thinking that it can be used for this is the primary
> reason for me objecting the patch...

With Regards,
Amit Kapila.

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.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

Regards,
Hari babu.

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?

This is my mistake, I was not able to catch.
I am really sorry for it and in future will make sure such mistake doesn't
happen again

> I find it
> rather scary if the *patch authors* document a known unsafe use case as
> one of the known use-cases.

With Regards,
Amit Kapila.