PFA an updated patch. This fixes an issue reported by Tushar internally. Since the patch changes the way min and max wal_size is stored internally from segment count to size in kb, it limited the maximum size of min and max_wal_size to 2GB in 32 bit systems.
The minimum required segment is 2 and the minimum wal size is 1MB. So the lowest possible value of the min/max wal_size is 2MB. Hence, I have changed the internal representation to MB instead of KB so that we can increase the range. Also, for any wal-seg-size, it retains the default seg count as 5 and 64 for min and max wal_size. Consequently, the size of the variables increase automatically according to the wal_segment_size. This behaviour is similar to that of existing code. I have also run pg_indent on the files. On Mon, Mar 20, 2017 at 11:37 PM, Beena Emerson <memissemer...@gmail.com> wrote: > Hello, > > PFA the updated patch. > > On Fri, Mar 17, 2017 at 6:40 AM, Robert Haas <robertmh...@gmail.com> > wrote: > >> On Tue, Mar 14, 2017 at 1:44 AM, Beena Emerson <memissemer...@gmail.com> >> wrote: >> > Attached is the updated patch. It fixes the issues and also updates few >> code >> > comments. >> >> I did an initial readthrough of this patch tonight just to get a >> feeling for what's going on. Based on that, here are a few review >> comments: >> >> The changes to pg_standby seem to completely break the logic to wait >> until the file has attained the correct size. I don't know how to >> salvage that logic off-hand, but just breaking it isn't acceptable. >> > > Using, the XLogLongPageHeader->xlp_seg_size, all the original checks have > been retained. This methid is even used in pg_waldump. > > > >> >> + Note that changing this value requires an initdb. >> >> Instead, maybe say something like "Note that this value is fixed for >> the lifetime of the database cluster." >> > > Corrected. > > >> >> -int max_wal_size = 64; /* 1 GB */ >> -int min_wal_size = 5; /* 80 MB */ >> +int wal_segment_size = 2048; /* 16 MB */ >> +int max_wal_size = 1024 * 1024; /* 1 GB */ >> +int min_wal_size = 80 * 1024; /* 80 MB */ >> >> If wal_segment_size is now measured in multiple of XLOG_BLCKSZ, then >> it's not the case that 2048 is always 16MB. If the other values are >> now measured in kB, perhaps rename the variables to add _kb, to avoid >> confusion with the way it used to work (and in general). The problem >> with leaving this as-is is that any existing references to >> max_wal_size in core or extension code will silently break; you want > > it to break in a noticeable way so that it gets fixed. >> >> > The wal_segment_size now is DEFAULT_XLOG_SEG_SIZE / XLOG_BLCKSZ; > min and max wal_size have _kb postfix > > >> + * UsableBytesInSegment: It is set in assign_wal_segment_size and stores >> the >> + * number of bytes in a WAL segment usable for WAL data. >> >> The comment doesn't need to say where it gets set, and it doesn't need >> to repeat the variable name. Just say "The number of bytes in a..." >> > > Done. > > >> >> +assign_wal_segment_size(int newval, void *extra) >> >> Why does a PGC_INTERNAL GUC need an assign hook? I think the GUC >> should only be there to expose the value; it shouldn't have >> calculation logic associated with it. >> > > Removed the function and called the functions in ReadControlFile. > > >> >> /* >> + * initdb passes the WAL segment size in an environment variable. We >> don't >> + * bother doing any sanity checking, we already check in initdb that >> the >> + * user gives a sane value. >> + */ >> + XLogSegSize = pg_atoi(getenv("XLOG_SEG_SIZE"), sizeof(uint32), 0); >> >> I think we should bother. I don't like the idea of the postmaster >> crashing in flames without so much as a reasonable error message if >> this parameter-passing mechanism goes wrong. >> > > I have rechecked the XLogSegSize. > > >> >> + {"wal-segsize", required_argument, NULL, 'Z'}, >> >> When adding an option with no documented short form, generally one >> picks a number that isn't a character for the value at the end. See >> pg_regress.c or initdb.c for examples. >> > > Done. > > >> >> + wal_segment_size = atoi(str_wal_segment_size); >> >> So, you're comfortable interpreting --wal-segsize=1TB or >> --wal-segsize=1GB as 1? Implicitly, 1MB? >> > > Imitating the current behaviour of config option --with-wal-segment, I > have used strtol to throw an error if the value is not only integers. > > >> >> + * ControlFile is not accessible here so use SHOW wal_segment_size >> command >> + * to set the XLogSegSize >> >> Breaks compatibility with pre-9.6 servers. >> > > Added check for the version, the SHOW command will be run only in v10 and > above. Previous versions do not need this. > > >> >> -- > Thank you, > > Beena Emerson > > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Thank you, Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
02-initdb-walsegsize-v6.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