Re: wal_sync_method as enum

From: Magnus Hagander <magnus(at)hagander(dot)net>
To: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org
Subject: Re: wal_sync_method as enum
Date: 2008-04-08 08:18:02
Message-ID: 20080408101802.0b8ebabc@mha-laptop
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

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

In response to

Browse pgsql-patches by date

  From Date Subject
Next Message Zoltan Boszormenyi 2008-04-08 09:09:39 Re: [HACKERS] TRUNCATE TABLE with IDENTITY
Previous Message Magnus Hagander 2008-04-08 07:50:10 Re: [PATCHES] Integer datetime by default