Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
> Hi Amit
> I have reviewed and tested the patch, Following are my observations and
comments.
Thank you for the review.
I need some clarification regarding some of the comments
> Observations and Comments
> -------------------------------------------
>- I think when finding the max LSN of single file the utility should
consider all relation segments.
Would you like to find for all relation related segments:
Like 12345, 12345.1 ... 12345.n Or
12345, 12345.1 ... and 12345_vm, 12345.1_vm
But how about if user asks for 12345.4, do we need to find all greater
than 12345.4?
IMHO, as this utility is not aware of relation or any other logical entity
and deals with only file or directory,
it is okay to find only for that file.
> - For -p {file | dir} option the utility expects the file path relative
to the specified data directory path which makes thing little
> confusing
> for example
> ./pg_computemaxlsn -p data/base/12286/12200 data
> pg_computemaxlsn: file or directory "data/base/12286/12200" does not
exists
> although data/base/12286/12200 is valid file path but we gets file does
not exists error
Is changing path to "data/data/base/12286/12200" in error message will make
things more clear?
>Code Review
>---------------------
> - While finding the max LSN from single file, you are not verifying that
the file actually exists inside the data directory path
> provided. and can end up looking
> at wrong data directory for checking if the server is running.
> For example:
> bin/pg_computemaxlsn -p $PWD/data/base/12286/12200
$PWD/otherinstallation/data
> Maximum LSN found is: 0, WAL segment file name (fileid,seg):
0000000000000000
>- Code is not verifying, if the provided data directory is the valid data
directory, This could make pg_computemaxlsn to compute max LSN
> from the running server file.
> For example:
> bin/pg_computemaxlsn -p $PWD/data/base/12286/12200 NONDATADIR/
> Maximum LSN found is: 0, WAL segment file name (fileid,seg):
0000000000000000
I think both of the above can be addressed if we allow only relative path
for file or directory and check if that is relative and below data directory
with function path_is_relative_and_below_cwd().
>Questions
> -----------------
>- I am wondering why are you not following the link inside the "pg_tblspc"
directory to get to the table space location instead of >building the
directory path. I am thinking if you follow the link instead, The utility
can be used across the different versions of PG.
This is good suggestion. I shall take care of this in updated patch.
With Regards,
Amit Kapila.
Regards,
Muhammad Usama
