wal_sync_method as enum

Magnus Hagander wrote:

Attached patch implements wal_sync_method as an enum. I'd like someone
to look it over before I apply it - I don't have the machines to test
all codepaths (and some of it is hard to test - better to read it and
make sure it's right).

In order to implement the enum guc, I had to break out a new
SYNC_METHOD_OPEN_DSYNC out of SYNC_METHOD_OPEN where it was previously
overloaded. This is one of the parts where I'm slightly worried I have
made a mistake.

I took a look at this and I like it -- it seems correct as far as I can
tell, and it has the nice property that we can display the list of
values that the current platform actually support.

This thing looked a bit dubious to me at first:

! switch (new_sync_method)
{
! case SYNC_METHOD_FSYNC:
! case SYNC_METHOD_FSYNC_WRITETHROUGH:
! case SYNC_METHOD_FDATASYNC:

because at least some of the values are only present on some platforms.
However, I think realized that the actual values are present on all
platforms, independent of whether they represent a supported fsync
method, so this is OK. Perhaps it would be good to add a comment on top
of the switch statement explaining this. Otherwise looks fine.

Well, and the ereport("FIXME") needs to be improved :-)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

Magnus Hagander wrote:

Attached patch implements wal_sync_method as an enum. I'd like
someone to look it over before I apply it - I don't have the
machines to test all codepaths (and some of it is hard to test -
better to read it and make sure it's right).

In order to implement the enum guc, I had to break out a new
SYNC_METHOD_OPEN_DSYNC out of SYNC_METHOD_OPEN where it was
previously overloaded. This is one of the parts where I'm slightly
worried I have made a mistake.

I took a look at this and I like it -- it seems correct as far as I
can tell, and it has the nice property that we can display the list of
values that the current platform actually support.

Yeah, this was one of the "most helpful ones" I thought of when I
started working on the enum stuff. It just also happened to be the one
that required the most changes :-S It'll be very good for tools like
[php]pgadmin to be able to show which values are actually supported.

This thing looked a bit dubious to me at first:

! switch (new_sync_method)
{
! case SYNC_METHOD_FSYNC:
! case SYNC_METHOD_FSYNC_WRITETHROUGH:
! case SYNC_METHOD_FDATASYNC:

because at least some of the values are only present on some
platforms. However, I think realized that the actual values are
present on all platforms, independent of whether they represent a
supported fsync method, so this is OK. Perhaps it would be good to
add a comment on top of the switch statement explaining this.
Otherwise looks fine.

Will add comment. I used to have #ifdefs there from the old code, but
it's a lot more readable this way... Even more readable with the
comment, of course.

Well, and the ereport("FIXME") needs to be improved :-)

Pah, details, details :-) It was just a copy/paste after all...

//Magnus