Review: Patch to compute Max LSN of Data Pages
Hi Amit
I have reviewed and tested the patch, Following are my observations and
comments.
Basic stuff
-----------------
- Patch applies OK
- Compiles cleanly with no warnings
- All regression tests pass.
Observations and Comments
-------------------------------------------
- If no option is given to pg_computemaxlsn utility then we get a wrong
error message
./pg_computemaxlsn ../data
pg_computemaxlsn: file or directory "(null)" exists
In my opinion we should consider the -P (Print max LSN from whole data
directory) as default in case no option is specified, or otherwise throw a
more descriptive error.
- Options -p and -P should be mutually exclusive
- Long options missing for -p and -P
- Help string for -P should be something like "print max LSN from whole
data directory" instead of "print max LSN from whole database"
- Help menu is silent about -V or --version option
- I think when finding the max LSN of single file the utility should
consider all relation segments.
- There is no check for extra arguments
e.g
./pg_computemaxlsn -P . ../data/ ../data/base/ ../data2/
- 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
- Utility should print the max LSN number in hexadecimal notation and LSN
format (logid/segno) for consistency with PG, and may also print the file
name which contains that LSN.
- When finding max LSN from directory, the utility does not considers the
sub directories, which I think could be useful in some cases, like if we
want to find the max LSN in tablespace directory. With the current
implementation we would require to run the utility multiple times, once for
each database.
Code Review
---------------------
Code generally looks good, I have few small points.
- In main() function variable maxLSN is defined as uint64,
Although XLogRecPtr and uint64 are the same thing, but I think we should
use XLogRecPtr instead.
- Just a small thing although will hardly improve anything but still for
sake of saving few cycles, I think you should use XLByteLT ( < ) instead
of XLByteLE ( <= )
to find if the previous max LSN is lesser then current.
- '\n' is missing from one the printf in the code :-)
fprintf(stderr, _("%s: file or directory \"%s\" does not exists"),
progname, LsnSearchPath);
- 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
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.
Regards,
Muhammad Usama
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
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
Observations and Comments
-------------------------------------------
- If no option is given to pg_computemaxlsn utility then we get a wrong
error message
./pg_computemaxlsn ../data
pg_computemaxlsn: file or directory "(null)" exists
In my opinion we should consider the -P (Print max LSN from whole data
directory) as default in case no option is specified, or otherwise throw a
more descriptive error.
Fixed. By considering default option as Whole data directory.
- Options -p and -P should be mutually exclusive
Fixed. Both options will not be allowed at the same time.
- Long options missing for -p and -P
Fixed. introduced long options as --path & --data-directory.
- Help string for -P should be something like "print max LSN from whole
data directory" instead of "print max LSN from whole database"
Fixed, by changing the message as suggested.
- Help menu is silent about -V or --version option
Fixed.
- I think when finding the max LSN of single file the utility should
consider all relation segments.
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.
- There is no check for extra arguments
e.g
./pg_computemaxlsn -P . ../data/ ../data/base/ ../data2/
Fixed.
- Extra options gives error.
- Multiple -p options also gives error.
Eg:
./pg_computemaxlsn -p base/12286/12200 -p base/12286/12201 data
- 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
As the path of file is relative to data directory, that's why in error it prints the path as "data/base/12286/12200".
Do you have any suggestion what should be done for this?
- Utility should print the max LSN number in hexadecimal notation and LSN
format (logid/segno) for consistency with PG,
Fixed
and may also print the file name which contains that LSN.
It might be confusing to users for the cases where it has to follow link (pg_tblspc) and then print some absolute path which is not related to relative path given by user.
Also I am not sure if it will be of any use to user.
- When finding max LSN from directory, the utility does not considers the
sub directories, which I think could be useful in some cases, like if we
want to find the max LSN in tablespace directory. With the current
implementation we would require to run the utility multiple times, once for
each database.
It will consider sub-directories as well.
Code Review
---------------------
Code generally looks good, I have few small points.
- In main() function variable maxLSN is defined as uint64,
Although XLogRecPtr and uint64 are the same thing, but I think we should
use XLogRecPtr instead.
Changed as per suggestion.
- Just a small thing although will hardly improve anything but still for
sake of saving few cycles, I think you should use XLByteLT ( < ) instead
of XLByteLE ( <= )
to find if the previous max LSN is lesser then current.
Changed as per suggestion.
- '\n' is missing from one the printf in the code :-)
fprintf(stderr, _("%s: file or directory \"%s\" does not exists"),
progname, LsnSearchPath);
Changed as per suggestion.
- 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
Fixed, by allowing 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.
Changed as per suggestion.
Apart from above, updated the documentation.
With Regards,
Amit Kapila.
Attachments:
pg_computemaxlsn.v2.patchapplication/octet-stream; name=pg_computemaxlsn.v2.patchDownload
*** a/contrib/Makefile
--- b/contrib/Makefile
***************
*** 31,36 **** SUBDIRS = \
--- 31,37 ----
passwordcheck \
pg_archivecleanup \
pg_buffercache \
+ pg_computemaxlsn \
pg_freespacemap \
pg_standby \
pg_stat_statements \
*** /dev/null
--- b/contrib/pg_computemaxlsn/Makefile
***************
*** 0 ****
--- 1,22 ----
+ # contrib/pg_computemaxlsn/Makefile
+
+ PGFILEDESC = "pg_computemaxlsn - an utility to find max LSN from data pages"
+ PGAPPICON = win32
+
+ PROGRAM = pg_computemaxlsn
+ OBJS = pg_computemaxlsn.o $(WIN32RES)
+
+ PG_CPPFLAGS = -I$(srcdir)
+ PG_LIBS = $(libpq_pgport)
+
+
+ ifdef USE_PGXS
+ PG_CONFIG = pg_config
+ PGXS := $(shell $(PG_CONFIG) --pgxs)
+ include $(PGXS)
+ else
+ subdir = contrib/pg_computemaxlsn
+ top_builddir = ../..
+ include $(top_builddir)/src/Makefile.global
+ include $(top_srcdir)/contrib/contrib-global.mk
+ endif
*** /dev/null
--- b/contrib/pg_computemaxlsn/pg_computemaxlsn.c
***************
*** 0 ****
--- 1,545 ----
+ /*-------------------------------------------------------------------------
+ *
+ * pg_computemaxlsn.c
+ * A utility to compute the maximum LSN in data pages
+ *
+ * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * contrib/pg_computemaxlsn/pg_computemaxlsn.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+ /*
+ * We have to use postgres.h not postgres_fe.h here, because there's so much
+ * backend-only stuff for reading data files we need. But we need a
+ * frontend-ish environment otherwise. Hence this ugly hack.
+ */
+ #define FRONTEND 1
+
+ #include "postgres.h"
+
+ #include <dirent.h>
+ #include <fcntl.h>
+ #include <locale.h>
+ #include <sys/stat.h>
+ #include <sys/time.h>
+ #include <time.h>
+ #include <unistd.h>
+ #ifdef HAVE_GETOPT_H
+ #include <getopt.h>
+ #endif
+
+ #include "access/xlog_internal.h"
+ #include "catalog/catalog.h"
+ #include "storage/bufpage.h"
+ #include "storage/fd.h"
+
+ /* Page header size */
+ #define PAGEHDRSZ (sizeof(PageHeaderData))
+
+ /*
+ * relfile nodename validation allow only file name start with digit
+ */
+ #define validateRelfilenodename(name) ((name[0] >= '0') && (name[0] <= '9'))
+ #define validateTablespaceDir(name) ((strlen(name) > 3) && (name[0] == 'P') && (name[1] == 'G') && (name[2] == '_'))
+
+
+ extern int optind;
+ extern char *optarg;
+ static const char *progname;
+
+ static int FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn);
+ static int FindMaxLSNinDir(char *path, XLogRecPtr *maxlsn, bool is_fromlink);
+ static int FindMaxLSNinPgData(XLogRecPtr *maxlsn);
+ static void usage(void);
+ static int getLinkPath(struct stat * statbuf, char *path, char *linkpath, int length);
+
+
+ int
+ main(int argc, char *argv[])
+ {
+ static struct option long_options[] = {
+ {"path", required_argument, NULL, 'p'},
+ {"data-directory", no_argument, NULL, 'P'},
+ {NULL, 0, NULL, 0}
+ };
+
+ int optindex;
+ int c;
+ char *DataDir;
+ int fd;
+ char path[MAXPGPATH];
+ bool print_max_lsn = false;
+ bool print_pgdata_max_lsn = false;
+ char *LsnSearchPath = NULL;
+ XLogRecPtr maxLSN = 0;
+ XLogSegNo logSegNo = 0;
+ int result;
+
+ set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_computemaxlsn"));
+
+ progname = get_progname(argv[0]);
+
+ if (argc > 1)
+ {
+ if (strcmp(argv[1], "--help") == 0 || strcmp(argv[1], "-?") == 0)
+ {
+ usage();
+ exit(0);
+ }
+ if (strcmp(argv[1], "--version") == 0 || strcmp(argv[1], "-V") == 0)
+ {
+ puts("pg_computemaxlsn (PostgreSQL) " PG_VERSION);
+ exit(0);
+ }
+ }
+
+ while ((c = getopt_long(argc, argv, "p:P", long_options, &optindex)) != -1)
+ {
+ switch (c)
+ {
+ case 'p':
+ if (print_max_lsn)
+ {
+ fprintf(stderr, _("%s: multiple -p options are not supported.\n"), progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+ print_max_lsn = true;
+ LsnSearchPath = strdup(optarg);
+ if (!path_is_relative_and_below_cwd(LsnSearchPath))
+ {
+ fprintf(stderr, _("%s: Path \"%s\" should be relative and"
+ " must be in or below the data directory.\n"),
+ progname, LsnSearchPath);
+ exit(1);
+ }
+ break;
+
+ case 'P':
+ if (print_pgdata_max_lsn)
+ {
+ fprintf(stderr, _("%s: multiple -P options are not supported.\n"), progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+ print_pgdata_max_lsn = true;
+ break;
+
+ default:
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+ }
+
+ if (print_max_lsn && print_pgdata_max_lsn)
+ {
+ fprintf(stderr, _("%s: both options -P and -p can not be combined.\n"), progname);
+ exit(1);
+ }
+
+ if (optind == argc)
+ {
+ fprintf(stderr, _("%s: no data directory specified.\n"), progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ if ((optind + 1) != argc)
+ {
+ fprintf(stderr, _("%s: mutiple data directories not supported.\n"), progname);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
+ exit(1);
+ }
+
+ /*
+ * 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
+
+ DataDir = argv[optind];
+
+ if (chdir(DataDir) < 0)
+ {
+ fprintf(stderr, _("%s: could not change directory to \"%s\": %s\n"),
+ progname, DataDir, strerror(errno));
+ exit(1);
+ }
+
+ /*
+ * 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, "postmaster.pid");
+
+ if ((fd = open(path, O_RDONLY, 0)) < 0)
+ {
+ if (errno != ENOENT)
+ {
+ fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
+ progname, path, strerror(errno));
+ exit(1);
+ }
+ }
+ else
+ {
+ fprintf(stderr, _("%s: lock file \"%s\" exists\n"
+ "Is a server running? If not, delete the lock file and try again.\n"),
+ progname, path);
+ exit(1);
+ }
+
+ /* By default we need to compute max lsn for database */
+ if ((print_max_lsn == false) || (0 == strcmp(LsnSearchPath, ".")))
+ {
+ result = FindMaxLSNinPgData(&maxLSN);
+ }
+ else
+ {
+ struct stat fst;
+
+ if (lstat(LsnSearchPath, &fst) < 0)
+ {
+ if (errno == ENOENT)
+ {
+ fprintf(stderr, _("%s: file or directory \"%s\" does not exists.\n"),
+ progname, LsnSearchPath);
+ }
+ else
+ {
+ fprintf(stderr, _("%s: could not stat file or directory \"%s\": %s\n"),
+ progname, LsnSearchPath, strerror(errno));
+ }
+ exit(1);
+ }
+
+ if (getLinkPath(&fst, LsnSearchPath, path, sizeof(path)) > 0)
+ {
+ result = FindMaxLSNinDir(path, &maxLSN, true);
+ }
+ else if (S_ISDIR(fst.st_mode))
+ {
+ result = FindMaxLSNinDir(LsnSearchPath, &maxLSN, false);
+ }
+ else
+ {
+ result = FindMaxLSNinFile(LsnSearchPath, &maxLSN);
+ }
+ }
+
+ if (0 == result)
+ {
+ XLByteToSeg(maxLSN, logSegNo);
+
+ printf("Maximum LSN found is: %lX \n"
+ "WAL segment file name (fileid,seg): %X/%X\n",
+ maxLSN, (uint32) (logSegNo >> 32), (uint32) (logSegNo));
+ }
+
+ return 0;
+ }
+
+
+ /*
+ * PageHeaderIsValid: Check page is valid or not
+ */
+ bool
+ PageHeaderIsValid(PageHeader page)
+ {
+ char *pagebytes;
+ int i;
+
+ /* Check normal case */
+ if (PageGetPageSize(page) == BLCKSZ &&
+ PageGetPageLayoutVersion(page) == PG_PAGE_LAYOUT_VERSION &&
+ (page->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
+ page->pd_lower >= SizeOfPageHeaderData &&
+ page->pd_lower <= page->pd_upper &&
+ page->pd_upper <= page->pd_special &&
+ page->pd_special <= BLCKSZ &&
+ page->pd_special == MAXALIGN(page->pd_special))
+ return true;
+
+ /*
+ * Check all-zeroes till page header; this is used only to log the page
+ * details even we detect invalid page we will continue to nex pages
+ */
+ pagebytes = (char *) page;
+ for (i = 0; i < PAGEHDRSZ; i++)
+ {
+ if (pagebytes[i] != 0)
+ return false;
+ }
+ return true;
+ }
+
+
+ /*
+ * Read the maximum LSN number in the one of data file (relnode file).
+ *
+ */
+ static int
+ FindMaxLSNinFile(char *filename, XLogRecPtr *maxlsn)
+ {
+ XLogRecPtr pagelsn;
+ off_t len,
+ seekpos;
+ uint32 nblocks,
+ blocknum;
+ char buffer[PAGEHDRSZ];
+ int nbytes;
+ int fd;
+
+ if ((fd = open(filename, O_RDONLY | PG_BINARY, 0)) < 0)
+ {
+ /*
+ * If file does not exist or or we can't read it. give error
+ */
+ fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
+ progname, filename, strerror(errno));
+ return -1;
+ }
+
+ /* Calculate the number of pages in file */
+ len = lseek(fd, 0L, SEEK_END);
+ if (len < 0)
+ {
+ close(fd);
+ fprintf(stderr, _("%s: .. file \"%s\" for seeking: %s\n"),
+ progname, filename, strerror(errno));
+ return -1;
+ }
+
+ nblocks = (len / BLCKSZ);
+ if (nblocks > RELSEG_SIZE)
+ {
+ /*
+ * In one relfilenode file length can't be more that RELSEG_SIZE
+ */
+ close(fd);
+ fprintf(stderr, _("%s: .. file \"%s\" length is more than segment size: %d.\n"),
+ progname, filename, RELSEG_SIZE);
+ return -1;
+ }
+
+ /*
+ * Read the only page header and validate; if we find invalid page log the
+ * details of page and continue to next page.
+ */
+ seekpos = 0;
+ for (blocknum = 0; blocknum < nblocks; blocknum++)
+ {
+ len = lseek(fd, seekpos, SEEK_SET);
+ if (len != seekpos)
+ {
+ close(fd);
+ fprintf(stderr, _("%s: could not seek to next page \"%s\": %s\n"),
+ progname, filename, strerror(errno));
+ return -1;
+ }
+
+ nbytes = read(fd, buffer, PAGEHDRSZ);
+ if (nbytes < 0)
+ {
+ close(fd);
+ fprintf(stderr, _("%s: could not read file \"%s\": %s\n"),
+ progname, filename, strerror(errno));
+ return -1;
+ }
+
+ if (PageHeaderIsValid((PageHeader) buffer))
+ {
+ pagelsn = PageGetLSN(buffer);
+ if (XLByteLT(*maxlsn, pagelsn))
+ {
+ *maxlsn = pagelsn;
+ }
+ }
+ else
+ {
+ /*
+ * If page is invalid log the error and continue
+ */
+ fprintf(stderr, _("%s: Invalid page found in file \"%s\" pagid:%d\n"),
+ progname, filename, blocknum);
+ }
+ seekpos += (off_t) BLCKSZ;
+ }
+
+ close(fd);
+ return 0;
+ }
+
+ /*
+ * Read the maximum LSN number in current directory; including sub directories
+ * and links.
+ */
+ static int
+ FindMaxLSNinDir(char *path, XLogRecPtr *maxlsn, bool is_fromlink)
+ {
+ DIR *dir;
+ struct dirent *de;
+ char pathbuf[MAXPGPATH];
+ struct stat statbuf;
+ char linkpath[MAXPGPATH];
+ int result;
+
+ dir = opendir(path);
+ if (NULL == dir)
+ {
+ fprintf(stderr, _("%s: could not open directory \"%s\": %s\n"),
+ progname, path, strerror(errno));
+ return -1;
+ }
+
+ while ((de = readdir(dir)) != NULL)
+ {
+ /* Skip special stuff */
+ if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0)
+ continue;
+
+ /* Skip temporary files */
+ if (strncmp(de->d_name,
+ PG_TEMP_FILE_PREFIX,
+ strlen(PG_TEMP_FILE_PREFIX)) == 0)
+ continue;
+
+ /*
+ * Skip all the local/global temporary files, and read and read all
+ * reamining relfinenode files
+ */
+ if (is_fromlink)
+ {
+ /* If directory is link then only allow PG_* path only */
+ if (!validateTablespaceDir(de->d_name))
+ continue;
+ }
+ else if (!validateRelfilenodename(de->d_name))
+ continue;
+
+ snprintf(pathbuf, MAXPGPATH, "%s/%s", path, de->d_name);
+
+ if (lstat(pathbuf, &statbuf) != 0)
+ {
+ if (errno != ENOENT)
+ {
+ fprintf(stderr, _("%s: could not stat file or directory \"%s\": %s\n"),
+ progname, pathbuf, strerror(errno));
+ }
+ /* If the file went away while scanning, it's no error. */
+ continue;
+ }
+
+ result = getLinkPath(&statbuf, pathbuf, linkpath, sizeof(linkpath));
+ if (result < 0)
+ {
+ continue;
+ }
+ else if (result > 0)
+ {
+ (void) FindMaxLSNinDir(linkpath, maxlsn, true);
+ }
+ else if (S_ISDIR(statbuf.st_mode))
+ (void) FindMaxLSNinDir(pathbuf, maxlsn, false);
+ else
+ (void) FindMaxLSNinFile(pathbuf, maxlsn);
+ }
+
+ closedir(dir);
+ return 0;
+ }
+
+ /*
+ * Get the link path.
+ * On success, returns length of link filename.
+ * and return zero incase of is file is not a link type.
+ * On failure, returns -1.
+ */
+ static int
+ getLinkPath(struct stat * statbuf, char *path, char *linkpath, int length)
+ {
+ int rllen;
+
+ if (
+ #ifndef WIN32
+ S_ISLNK(statbuf->st_mode)
+ #else
+ pgwin32_is_junction(path)
+ #endif
+ )
+ {
+ #if defined(HAVE_READLINK) || defined(WIN32)
+
+ rllen = readlink(path, linkpath, length);
+ if (rllen < 0)
+ {
+ fprintf(stderr, _("%s: could not read symbolic link \"%s\", so skipping file.\n"),
+ progname, path);
+ return -1;
+ }
+
+ if (rllen >= length)
+ {
+ fprintf(stderr, _("%s: symbolic link \"%s\" target is too long, so skipping file.\n"),
+ progname, path);
+
+ return -1;
+ }
+
+ linkpath[rllen] = '\0';
+
+ return rllen;
+ #else
+ /* tablespaces are not supported on this platform */
+ return -1;
+ #endif /* HAVE_READLINK */
+ }
+
+ return 0;
+ }
+
+
+ /*
+ * Read the maximum LSN number in the DATA directory.
+ */
+ static int
+ FindMaxLSNinPgData(XLogRecPtr *maxlsn)
+ {
+ /* scan all the relfilenodes in data directory */
+ if (0 != FindMaxLSNinDir("global", maxlsn, false))
+ return -1;
+ if (0 != FindMaxLSNinDir("base", maxlsn, false))
+ return -1;
+ if (0 != FindMaxLSNinDir("pg_tblspc", maxlsn, false))
+ return -1;
+
+ return 0;
+ }
+
+ static void
+ usage(void)
+ {
+ printf(_("%s compute the maximum LSN in PostgreSQL data pages.\n\n"), progname);
+ printf(_("Usage:\n %s [OPTION]... DATADIR\n\n"), progname);
+ printf(_("Options:\n"));
+ printf(_(" -p, --path=RELATIVE_PATH print max LSN from file or directory name\n"));
+ printf(_(" -P, --data-directory print max LSN from whole data directory\n"));
+ printf(_(" -V, --version output version information, then exit\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
+ printf(_("\nReport bugs to <pgsql-bugs@postgresql.org>.\n"));
+ }
*** a/doc/src/sgml/ref/allfiles.sgml
--- b/doc/src/sgml/ref/allfiles.sgml
***************
*** 177,182 **** Complete list of usable sgml source files in this directory.
--- 177,183 ----
<!ENTITY pgDumpall SYSTEM "pg_dumpall.sgml">
<!ENTITY pgReceivexlog SYSTEM "pg_receivexlog.sgml">
<!ENTITY pgResetxlog SYSTEM "pg_resetxlog.sgml">
+ <!ENTITY pgComputemaxlsn SYSTEM "pg_computemaxlsn.sgml">
<!ENTITY pgRestore SYSTEM "pg_restore.sgml">
<!ENTITY postgres SYSTEM "postgres-ref.sgml">
<!ENTITY postmaster SYSTEM "postmaster.sgml">
*** /dev/null
--- b/doc/src/sgml/ref/pg_computemaxlsn.sgml
***************
*** 0 ****
--- 1,79 ----
+ <!--
+ doc/src/sgml/ref/pg_computemaxlsn.sgml
+ PostgreSQL documentation
+ -->
+
+ <refentry id="APP-PGCOMPUTEMAXLSN">
+ <refmeta>
+ <refentrytitle><application>pg_computemaxlsn</application></refentrytitle>
+ <manvolnum>1</manvolnum>
+ <refmiscinfo>Application</refmiscinfo>
+ </refmeta>
+
+ <refnamediv>
+ <refname>pg_computemaxlsn</refname>
+ <refpurpose>computes the maximum LSN in database of a <productname>PostgreSQL</productname> database cluster</refpurpose>
+ </refnamediv>
+
+ <indexterm zone="app-pgcomputemaxlsn">
+ <primary>pg_computemaxlsn</primary>
+ </indexterm>
+
+ <refsynopsisdiv>
+ <cmdsynopsis>
+ <command>pg_computemaxlsn</command>
+ <arg choice="opt"><option>-P</option></arg>
+ <arg choice="opt"><option>-p</option> <replaceable class="parameter">file-name</replaceable> | <replaceable class="parameter">folder-name</replaceable></arg>
+ <arg choice="plain"><replaceable>datadir</replaceable></arg>
+ </cmdsynopsis>
+ </refsynopsisdiv>
+
+ <refsect1 id="R1-APP-PGCOMPUTEMAXLSN-1">
+ <title>Description</title>
+ <para>
+ <command>pg_computemaxlsn</command> computes maximun LSN from database pages.
+ </para>
+
+ <para>
+ This utility can only be run by the user who installed the server, because
+ it requires read/write access to the data directory.
+ 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</>.
+ </para>
+
+ <para>
+ The <option>-P</> or <option>--data-directory</> for computing maximum LSN from all the pages in data directory.
+ This is the default option if none of the options are provided.
+ </para>
+
+ <para>
+ The <option>-p <replaceable class="parameter">file-name | folder-name</replaceable></option> or
+ <option>--path=<replaceable class="parameter">file-name | folder-name</replaceable></option> for computing
+ maximun LSN from specific file or folder. File or folder should be relative and in or below the data directory.
+ </para>
+
+ <para>
+ The <option>-V</> and <option>--version</> options print
+ the <application>pg_computemaxlsn</application> version and exit. The
+ options <option>-?</> and <option>--help</> show supported arguments,
+ and exit.
+ </para>
+
+ </refsect1>
+
+ <refsect1>
+ <title>Notes</title>
+
+ <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>
+ </refsect1>
+
+ </refentry>
*** a/doc/src/sgml/ref/pg_resetxlog.sgml
--- b/doc/src/sgml/ref/pg_resetxlog.sgml
***************
*** 135,140 **** PostgreSQL documentation
--- 135,150 ----
largest entry in <filename>pg_xlog</>, use <literal>-l 00000001000000320000004B</> or higher.
</para>
+ <para>
+ If <command>pg_resetxlog</command> complains that it cannot determine
+ valid data for <filename>pg_control</>, and if you do not have or corrupted
+ WAL segment files in the directory <filename>pg_xlog</> under the data directory,
+ then to identify larger WAL segment file from data files we can use utility <command>pg_computemaxlsn</command>
+ with <option>-P</> option for finding maximum LSN from the data directory or
+ for from specific file or folder <option>-p <filename>file-name | folder-name</></>.
+ Once larger WAL segment file is found use <option>-l</> option for setting the value.
+ </para>
+
<note>
<para>
<command>pg_resetxlog</command> itself looks at the files in
*** a/doc/src/sgml/reference.sgml
--- b/doc/src/sgml/reference.sgml
***************
*** 248,253 ****
--- 248,254 ----
&pgControldata;
&pgCtl;
&pgResetxlog;
+ &pgComputemaxlsn;
&postgres;
&postmaster;
Import Notes
Resolved by subject fallback
Amit Kapila escribió:
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
- 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_vmBut 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.
Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it
should consider that it is a relfilenode, and so it should get a list of
all segments for all forks of it. So if I give "12345" it should get
12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on.
However, if what I give it is a path, i.e. it contains a slash, I think
it should only consider the specific file mentioned. In that light, I'm
not sure that command line options chosen are the best set.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wednesday, November 28, 2012 3:36 AM Alvaro Herrera wrote:
Amit Kapila escribió:
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
- 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_vmBut 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.Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it
should consider that it is a relfilenode, and so it should get a list of
all segments for all forks of it. So if I give "12345" it should get
12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on.
Yes, I think this can be done either by having it as new option or
internally code can interpret as you suggest.
Also, can't we think of it as an extended usecase and implement it on top of
base, if this usecase is not an urgent?
However, if what I give it is a path, i.e. it contains a slash, I think
it should only consider the specific file mentioned. In that light, I'm
not sure that command line options chosen are the best set.
Yes for sure command line options can be improved/extended based on usecase.
Please feel free to give suggestion regarding command line option if you
feel current option
is not an extendable option.
One way is -p should be for file path and may be -r for relfile number.
With Regards,
Amit Kapila.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 27, 2012 at 5:52 PM, Amit kapila <amit.kapila@huawei.com> wrote:
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
Observations and Comments
-------------------------------------------- If no option is given to pg_computemaxlsn utility then we get a wrong
error message
./pg_computemaxlsn ../data
pg_computemaxlsn: file or directory "(null)" exists
In my opinion we should consider the -P (Print max LSN from whole data
directory) as default in case no option is specified, or otherwise throwa
more descriptive error.
Fixed. By considering default option as Whole data directory.
- Options -p and -P should be mutually exclusive
Fixed. Both options will not be allowed at the same time.
- Long options missing for -p and -P
Fixed. introduced long options as --path & --data-directory.
- Help string for -P should be something like "print max LSN from whole
data directory" instead of "print max LSN from whole database"Fixed, by changing the message as suggested.
- Help menu is silent about -V or --version option
Fixed.
- I think when finding the max LSN of single file the utility should
consider all relation segments.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.
- There is no check for extra arguments
e.g
./pg_computemaxlsn -P . ../data/ ../data/base/ ../data2/Fixed.
- Extra options gives error.
- Multiple -p options also gives error.
Eg:
./pg_computemaxlsn -p base/12286/12200 -p base/12286/12201 data- 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 notexists
although data/base/12286/12200 is valid file path but we gets file does
not
exists error
As the path of file is relative to data directory, that's why in
error it prints the path as "data/base/12286/12200".
Do you have any suggestion what should be done for this?
I think we should expect provided path to be relative to current directory
or may consider it to be relative to either one of Data or CWD.
Because normally we expect path to be relative to CWD if some program is
asking for path in command line. And also tab-complete doesn't works on
terminal if path is not absolute or relative to the current directory.
To handle this I think we should not change current directory to the data
directory and generate the file path (like for postmaster.pid) where
required by appending data directory path.
And before throwing an error should check if path is valid for either DATA
or CWD something like
if (lstat(LsnSearchPath, &fst) < 0)
{
if (errno == ENOENT)
{
/* Check if the path is relative to provided data directory */
snprintf(path, MAXPGPATH, "%s/%s",DataDir,LsnSearchPath);
if (lstat(path, &fst) < 0)
{
if (errno == ENOENT)
{
/* Check if the path is relative to provided data directory */
fprintf(stderr, _("%s: file or directory \"%s\" does not exists.\n"),
progname, LsnSearchPath);
}
else
{
fprintf(stderr, _("%s: could not stat file or directory \"%s\": %s\n"),
progname, path, strerror(errno));
}
exit(1);
}
else /* Path is relative to data directory*/
LsnSearchPath = strdup(path);
}
else
{
fprintf(stderr, _("%s: could not stat file or directory \"%s\": %s\n"),
progname, LsnSearchPath, strerror(errno));
exit(1);
}
}
- Utility should print the max LSN number in hexadecimal notation and LSN
format (logid/segno) for consistency with PG,Fixed
and may also print the file name which contains that LSN.
It might be confusing to users for the cases where it has to follow link
(pg_tblspc) and then print some absolute path which is not related to
relative path given by user.
Also I am not sure if it will be of any use to user.- When finding max LSN from directory, the utility does not considers the
sub directories, which I think could be useful in some cases, like if we
want to find the max LSN in tablespace directory. With the current
implementation we would require to run the utility multiple times, oncefor
each database.
It will consider sub-directories as well.
Code Review
---------------------Code generally looks good, I have few small points.
- In main() function variable maxLSN is defined as uint64,
Although XLogRecPtr and uint64 are the same thing, but I think we should
use XLogRecPtr instead.Changed as per suggestion.
- Just a small thing although will hardly improve anything but still for
sake of saving few cycles, I think you should use XLByteLT ( < ) instead
of XLByteLE ( <= )
to find if the previous max LSN is lesser then current.Changed as per suggestion.
- '\n' is missing from one the printf in the code :-)
fprintf(stderr, _("%s: file or directory \"%s\" does not exists"),
progname, LsnSearchPath);Changed as per suggestion.
- 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):
0000000000000000Fixed, by allowing 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.Changed as per suggestion.
Apart from above, updated the documentation.
With Regards,
Amit Kapila.
With Regards
Muhammad Usama
On Wed, Nov 28, 2012 at 3:06 AM, Alvaro Herrera <alvherre@2ndquadrant.com>wrote:
Show quoted text
Amit Kapila escribió:
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
- 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_vmBut 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.Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it
should consider that it is a relfilenode, and so it should get a list of
all segments for all forks of it. So if I give "12345" it should get
12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on.
However, if what I give it is a path, i.e. it contains a slash, I think
it should only consider the specific file mentioned. In that light, I'm
not sure that command line options chosen are the best set.+1
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Wednesday, November 28, 2012 11:49 AM Muhammad Usama wrote:
On Tue, Nov 27, 2012 at 5:52 PM, Amit kapila <amit.kapila@huawei.com> wrote:
Friday, November 23, 2012 5:38 AM Muhammad Usama wrote:
- 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
As the path of file is relative to data directory, that's why in error it
prints the path as "data/base/12286/12200".
Do you have any suggestion what should be done for this?
I think we should expect provided path to be relative to current directory
or may consider it to be relative to either one of Data or CWD.
Because normally we expect path to be relative to CWD if some program is
asking for path in command line. And also tab-complete doesn't >works on
terminal if path is not absolute or relative to the current directory.
To handle this I think we should not change current directory to the data
directory and generate the file path (like for postmaster.pid) >where
required by appending data directory path.
And before throwing an error should check if path is valid for either DATA
or CWD something like
Apart from that, I think it needs to also check if the path is under data
directory path in either case, else it can get LSN for some running server
as pointed in your other point.
Also shouldn't we support relative to one of CWD or data directory rather
than both?
Any opinions from others what is the best to support in such a use case.
a. File/Folder path Relative to CWD (Current working directory)
b. File/Folder path Relative to data directory
c. to both a and b
With Regards,
Amit Kapila.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers