From: | Beena Emerson <memissemerson(at)gmail(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
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 14:54:16 |
Message-ID: | CAOG9ApGy5aSbLVqL-jKsDS=CPDj8U8fSv+sWvuhBCTj9Jf9OYA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello,
On Wed, Sep 6, 2017 at 7:37 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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
>
Thank you.
> Unresolved:
> - this needs some new performance tests, the number of added instructions
> isn't trivial. Don't think there's anything, but ...
I will give out the results soon.
> - read through it again, check long lines
I have broken the long lines where necessary and applied pgindent as well.
> - 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 */"
we set 2 cleanup related variables once WalSegSize is set, namely
need_cleanup and exclusiveCleanupFileName. Does
SetWALSegSizeAndCleanupValues look good?
> - 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.
I did not think it was necessary to create a new function, I have
simply added the check and
function call within the XLOGShmemInit().
> - 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?
I have changed the behaviour to treat it as guessed and also modified
the error message.
>- verify EXEC_BACKEND does the right thing
> - not this commit/patch, but XLogReadDetermineTimeline() could really
> use some simplifying of repetitive expressions
I will check this.
> - XLOGShmemInit shouldn't memcpy to temp_cfile and such, why not just
> save previous pointer in a local variable?
done.
> - could you fill in the Reviewed-By: line in the commit message?
I have added the names in alphabetical order.
Kindly check the attached v2 patch.
--
Beena Emerson
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment | Content-Type | Size |
---|---|---|
0002-Make-wal-segment-size-configurable-at-initdb-time_v2.patch | application/octet-stream | 126.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-09-06 15:03:17 | Re: PoC plpgsql - possibility to force custom or generic plan |
Previous Message | Tom Lane | 2017-09-06 14:44:50 | Re: Rewriting the test of pg_upgrade as a TAP test |