On 1/5/17 5:38 AM, Beena Emerson wrote:
On Tue, Jan 3, 2017 at 2:53 AM, Jim Nasby <jim.na...@bluetreble.com
<mailto:jim.na...@bluetreble.com>> wrote:
General comments:
There was some discussion about the impact of this on small
installs, particularly around min_wal_size. The concern was that
...
The patch maintains the current XLOG_SEG_SIZE of 16MB as the default.
Only the capability to change its value has been moved for configure to
initdb.
Ah, I missed that. Thanks for clarifying.
It's not clear from the thread that there is consensus that this
feature is desired. In particular, the performance aspects of
changing segment size from a C constant to a variable are in
question. Someone with access to large hardware should test that.
Andres[1] and Robert[2] did suggest that the option could be changed
to a bitshift, which IMHO would also solve some sanity-checking issues.
Are you going to change to a bitshift in the next patch?
+ * 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.
That doesn't seem like a good idea to me. If anything, the backend
should sanity-check and initdb just rely on that. Perhaps this is
how other initdb options work, but it still seems bogus. In
particular, verifying the size is a power of 2 seems important, as
failing that would probably be ReallyBad(tm).
The patch also blindly trusts the value read from the control file;
I'm not sure if that's standard procedure or not, but ISTM it'd be
worth sanity-checking that as well.
There is a CRC check to detect error in the file. I think all the
ControlFile values are used directly and not re-verified.
Sounds good. I do still think the variable from initdb should be
sanity-checked.
The patch leaves the base GUC units for min_wal_size and
max_wal_size as the # of segments. I'm not sure if that's a great idea.
I think we can leave it as is. This is used in
CalculateCheckpontSegments and in XLOGfileslop to calculate the segment
numbers.
My concern here is that we just changed from segments to KB for all the
checkpoint settings, and this is introducing segments back in, but ...
I agree pretty_print_kb would have been a better for this function.
However, I have realised that using the show hook and this function is
not suitable and have found a better way of handling the removal of
GUC_UNIT_XSEGS which no longer needs this function : using the
GUC_UNIT_KB, convert the value in bytes to wal segment count instead in
the assign hook. The next version of patch will use this.
... it sounds like you're going back to exposing KB to users, and that's
all that really matters.
IMHO it'd be better to use the n & (n-1) check detailed at [3].
See my other email about that.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
