Re: Review: Patch to compute Max LSN of Data Pages - Mailing list pgsql-hackers

<p dir="LTR"><span lang="en-us"></span><p dir="LTR"><span lang="en-us"><font face="Consolas">Friday, November 23, 2012
5:38AM</font></span><span lang="en-us"><font face="Consolas"></font></span><span lang="en-us"> <font
face="Consolas">MuhammadUsama</font></span><span lang="en-us"><font face="Consolas"> wrote:</font></span><span
lang="en-us"></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">Hi Amit</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span
lang="en-us"><font face="Consolas">I have reviewed and tested the patch, Following are my observations and
comments.</font></span><pdir="LTR"><span lang="en-us">        </span><p dir="LTR"><span lang="en-us"><font
face="Consolas"> Thank you for the review.</font></span><span lang="en-us"><font face="Consolas"></font></span><span
lang="en-us"></span><p dir="LTR"><span lang="en-us"><font face="Consolas"> </font></span><span lang="en-us"> <font
face="Consolas">Ineed some clarification regarding some of the comments</font></span><span lang="en-us"></span><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">Observationsand Comments</font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"> <font
face="Consolas">-------------------------------------------</font></span><br/><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"><font face="Consolas">- I think when finding the max LSN of single
filethe utility should consider all relation segments.</font></span><span lang="en-us"></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">  Would you like to find for all</font></span><span lang="en-us"><font
face="Consolas">relation related segments:</font></span><span lang="en-us"></span><p dir="LTR"><span lang="en-us"><font
face="Consolas"> </font></span><spanlang="en-us"> <font face="Consolas">Like 12345, 12345.1 ... 12345.n   
Or</font></span><spanlang="en-us"> </span><p dir="LTR"><span lang="en-us"><font face="Consolas"> </font></span><span
lang="en-us"><font face="Consolas">12345, 12345.1 ... and 12345_vm, 12345.1_vm</font></span><span
lang="en-us"></span><pdir="LTR"><span lang="en-us"><font face="Consolas">  </font></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">  But how about if user asks for 12345.4, do we need to find all greater
than</font></span><spanlang="en-us"> <font face="Consolas">12345</font></span><span lang="en-us"><font
face="Consolas">.4</font></span><spanlang="en-us"><font face="Consolas">?</font></span><p dir="LTR"><span
lang="en-us"></span><pdir="LTR"><span lang="en-us"><font face="Consolas">  IMHO, as this utility is not aware of
relationor any other logical entity and deals with only file or directory,</font></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas"> </font></span><span lang="en-us"> <font face="Consolas">i</font></span><span
lang="en-us"><fontface="Consolas">t is okay to find only for that file.</font></span><span lang="en-us"></span><br /><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font face="Consolas">- For
-p {file| dir}  option the utility expects the file path relative to the specified data directory path which makes
thinglittle</font></span><span lang="en-us"> </span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"> <font face="Consolas">confusing</font></span><span
lang="en-us">    </span><p dir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us">
<fontface="Consolas">for example </font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"> <font face="Consolas">./pg_computemaxlsn -p data/base/12286/12200
data</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">pg_computemaxlsn:file or directory "data/base/12286/12200" does not exists</font></span><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">although data/base/12286/12200is valid file path but we gets file does not exists
error</font></span><spanlang="en-us"></span><p dir="LTR"><span lang="en-us"></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">Is changing path</font></span><span lang="en-us"> <font
face="Consolas">to</font></span><spanlang="en-us"> <font face="Consolas">"</font></span><span lang="en-us"><font
face="Consolas">data/</font></span><spanlang="en-us"><font face="Consolas">data/base/12286/12200"</font></span><span
lang="en-us"><font face="Consolas">in error me</font></span><span lang="en-us"><font
face="Consolas">s</font></span><spanlang="en-us"><font face="Consolas">sage</font></span><span lang="en-us"> <font
face="Consolas">willmake things more clear?</font></span><span lang="en-us"></span><p dir="LTR"><span
lang="en-us"></span><pdir="LTR"><span lang="en-us">        </span><p dir="LTR"><span lang="en-us"><font
face="Consolas">  </font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span
lang="en-us"><fontface="Consolas">Code Review</font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"><font face="Consolas">---------------------</font></span><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font face="Consolas">- While
findingthe max LSN from single file, you are not verifying that the file actually exists inside the data directory
path</font></span><spanlang="en-us"> </span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"> <font face="Consolas">provided. and can end up
looking</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">atwrong data directory for checking if the server is running.</font></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">></font></span><span lang="en-us"> <font face="Consolas">For
example:</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">bin/pg_computemaxlsn-p $PWD/data/base/12286/12200 $PWD/otherinstallation/data</font></span><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font face="Consolas">Maximum
LSNfound is: 0, WAL segment file name (fileid,seg): 0000000000000000</font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"><font face="Consolas">- Code is not verifying, if the provided data
directoryis the valid data directory, This could make pg_computemaxlsn to compute max LSN</font></span><span
lang="en-us"></span><p dir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">fromthe running server file.</font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"> <font face="Consolas">For example: </font></span><p
dir="LTR"><spanlang="en-us"><font face="Consolas">></font></span><span lang="en-us"> <font
face="Consolas">bin/pg_computemaxlsn-p $PWD/data/base/12286/12200 NONDATADIR/</font></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">></font></span><span lang="en-us"> <font face="Consolas">Maximum LSN found is: 0,
WALsegment file name (fileid,seg): 0000000000000000</font></span><span lang="en-us"></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">I think both of the above can be addressed if we allow only relative path for file or
directoryand check if that is rel</font></span><span lang="en-us"><font face="Consolas">ative and below data directory
withfunction path_is_</font></span><span lang="en-us"><font face="Consolas">relative_and_below_cwd</font></span><span
lang="en-us"><fontface="Consolas">()</font></span><span lang="en-us"><font face="Consolas">.</font></span><span
lang="en-us"></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span lang="en-us"><font
face="Consolas">Questions</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">></font></span><span
lang="en-us"><font face="Consolas">-----------------</font></span><p dir="LTR"><span lang="en-us"><font
face="Consolas">></font></span><spanlang="en-us"><font face="Consolas">- I am wondering why are you not following
thelink inside the "pg_tblspc" directory to get to the table space location instead of</font></span><span lang="en-us">
<fontface="Consolas">></font></span><span lang="en-us"><font face="Consolas">building the directory path. I am
thinkingif you follow the link instead, The utility can be used across the different versions of PG.</font></span><span
lang="en-us"></span><pdir="LTR"><span lang="en-us"><font face="Consolas">This is good suggestion. I shall take care
of</font></span><spanlang="en-us"> <font face="Consolas">this in updated patch.</font></span><p dir="LTR"><span
lang="en-us"><fontface="Consolas">With Regards,</font></span><p dir="LTR"><span lang="en-us"><font face="Consolas">Amit
Kapila.</font></span><spanlang="en-us"></span><br /><br /><p dir="LTR"><span lang="en-us"><font
face="Consolas">Regards,</font></span><pdir="LTR"><span lang="en-us"><font face="Consolas">Muhammad
Usama</font></span><spanlang="en-us"></span>