Hello, On Wed, Mar 22, 2017 at 9:41 PM, Kuntal Ghosh <kuntalghosh.2...@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 3:14 PM, Beena Emerson <memissemer...@gmail.com> > wrote: > > PFA an updated patch which fixes a minor bug I found. It only increases > the > > string size in pretty_wal_size function. > > The 01-add-XLogSegmentOffset-macro.patch has also been rebased. > Thanks for the updated versions. Here is a partial review of the patch: > > In pg_standby.c and pg_waldump.c, > + XLogPageHeader hdr = (XLogPageHeader) buf; > + XLogLongPageHeader NewLongPage = (XLogLongPageHeader) hdr; > + > + XLogSegSize = NewLongPage->xlp_seg_size; > It waits until the file is at least XLOG_BLCKSZ, then gets the > expected final size from XLogPageHeader. This looks really clean > compared to the previous approach. > thank you for testing. PFA the rebased patch incorporating your comments. > > + * Verify that the min and max wal_size meet the minimum requirements. > Better to write min_wal_size and max_wal_size. > Updated wherever applicable. > > + errmsg("Insufficient value for \"min_wal_size\""))); > "min_wal_size %d is too low" may be? Use lower case for error > messages. Same for max_wal_size. > updated. > > + /* Set the XLogSegSize */ > + XLogSegSize = ControlFile->xlog_seg_size; > + > A call to IsValidXLogSegSize() will be good after this, no? > Is it necessary? We already have the CRC check for ControlFiles. Is that not enough? > > + /* Update variables using XLogSegSize */ > + check_wal_size(); > The method name looks somewhat misleading compared to the comment for > it, doesn't it? > The method name is correct since it only checks if the value provided is sufficient (at least 2 segment size). I have updated the comment to say Check and update variables dependent on XLogSegSize > This patch introduces a new guc_unit having values in MB for > max_wal_size and min_wal_size. I'm not sure about the upper limit > which is set to INT_MAX for 32-bit systems as well. Is it needed to > define something like MAX_MEGABYTES similar to MAX_KILOBYTES? > It is worth mentioning that GUC_UNIT_KB can't be used in this case > since MAX_KILOBYTES is INT_MAX / 1024(<2GB) on 32-bit systems. That's > not a sufficient value for min_wal_size/max_wal_size. > You are right. I can use the same value as upper limit for GUC_UNIT_MB, we could probably change the name of MAX_KILOBYTES to something more general for both GUC_UNIT_MB and GUC_UNIT_KB. The max size in 32-bit systems would be 2TB. > While testing with pg_waldump, I got the following error. > bin/pg_waldump -p master/pg_wal/ -s 0/01000000 > Floating point exception (core dumped) > Stack: > #0 0x00000000004039d6 in ReadPageInternal () > #1 0x0000000000404c84 in XLogFindNextRecord () > #2 0x0000000000401e08 in main () > I think that the problem is in following code: > /* parse files as start/end boundaries, extract path if not specified */ > if (optind < argc) > { > .... > + if (!RetrieveXLogSegSize(full_path)) > ... > } > In this case, RetrieveXLogSegSize is conditionally called. So, if the > condition is false, XLogSegSize will not be initialized. > > pg_waldump code has been updated. -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
02-initdb-walsegsize-v8.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers