From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Beena Emerson <memissemerson(at)gmail(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: increasing the default WAL segment size |
Date: | 2017-09-06 02:07:07 |
Message-ID: | 20170906020707.hu6kilwnc6idnykb@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
I was looking to commit this, but the changes I made ended up being
pretty large. Here's what I changed in the attached:
- split GUC_UNIT_BYTE into a separate commit, squashed rest
- renamed GUC_UNIT_BYT to GUC_UNIT_BYTE, don't see why we'd have such a
weird abbreviation?
- bumped control file version, otherwise things wouldn't work correctly
- wal_segment_size text still said "Shows the number of pages per write
ahead log segment."
- I still feel strongly that exporting XLogSegSize, which previously was
a macro and now a integer variable, is a bad idea. Hence I've renamed
it to wal_segment_size.
- There still were comments referencing XLOG_SEG_SIZE
- IsPowerOf2 regarded 0 as a valid power of two
- ConvertToXSegs() depended on a variable not passed as arg, bad idea.
- As previously mentioned, I don't think it's ok to rely on vars like
XLogSegSize to be defined both in backend and frontend code.
- I don't think XLogReader can rely on XLogSegSize, needs to be
parametrized.
- pg_rewind exported another copy of extern int XLogSegSize
- streamutil.h had a extern uint32 WalSegsz; but used
RetrieveXlogSegSize, that seems needlessly different
- moved wal_segment_size (aka XLogSegSize) to xlog.h
- pg_standby included xlogreader, not sure why?
- MaxSegmentsPerLogFile still had a conflicting naming scheme
- you'd included "sys/stat.h", that's not really appropriate for system
headers, should be <sys/stat.h> (and then grouped w/ rest)
- pg_controldata's warning about an invalid segsize missed newlines
Unresolved:
- this needs some new performance tests, the number of added instructions
isn't trivial. Don't think there's anything, but ...
- read through it again, check long lines
- pg_standby's RetrieveWALSegSize() does too much for it's name. It
seems quite weird that a function named that way has the section below
"/* check if clean up is necessary */"
- the way you redid the ReadControlFile() invocation doesn't quite seem
right. Consider what happens if XLOGbuffers isn't -1 - then we
wouldn't read the control file, but you unconditionally copy it in
XLOGShmemInit(). I think we instead should introduce something like
XLOGPreShmemInit() that reads the control file unless in bootstrap
mode. Then get rid of the second ReadControlFile() already present.
- In pg_resetwal.c:ReadControlFile() we ignore the file contents if
there's an invalid segment size, but accept the contents as guessed if
there's a crc failure - that seems a bit weird?
- verify EXEC_BACKEND does the right thing
- not this commit/patch, but XLogReadDetermineTimeline() could really
use some simplifying of repetitive expresssions
- XLOGShmemInit shouldn't memcpy to temp_cfile and such, why not just
save previous pointer in a local variable?
- could you fill in the Reviewed-By: line in the commit message?
Running out of concentration / time now.
- Andres
Attachment | Content-Type | Size |
---|---|---|
0001-Introduce-BYTES-unit-for-GUCs.patch | text/x-diff | 2.4 KB |
0002-Make-wal-segment-size-configurable-at-initdb-time.patch | text/x-diff | 126.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-09-06 02:30:58 | atomics/arch-x86.h is stupider than atomics/generic-gcc.h? |
Previous Message | Amit Langote | 2017-09-06 00:56:13 | Re: Copyright in partition.h and partition.c |