SEO

I see that assign_xlog_sync_method() is still assigning to sync_method.
This is 100% wrong: an assign hook is chartered to set derived values,
but not to set the GUC variable itself. This will for example result
in set_config_option() stacking the wrong value (the already-updated
one) as the value to roll back to if the transaction aborts.

You could just remove the assignment from assign_xlog_sync_method,
although it might look a bit odd to be setting open_sync_bit but
not sync_method. It also bothers me slightly that the derived and
main values wouldn't be set at exactly the same point --- problems
inside guc.c might lead to them getting out of sync.

Another possibility is to stick with something equivalent to the former
design: what GUC thinks is the variable is just a dummy static integer
in guc.c, and the assign hook is still setting the "real" value that the
rest of the code looks at. A minor advantage of this second way is that
the "real" value could still be declared as enum rather than int.

Please fix this, and take another look at the prior WAL enum changes
to see if the same problem hasn't been created elsewhere.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> I see that assign_xlog_sync_method() is still assigning to
> sync_method. This is 100% wrong: an assign hook is chartered to set
> derived values, but not to set the GUC variable itself. This will
> for example result in set_config_option() stacking the wrong value
> (the already-updated one) as the value to roll back to if the
> transaction aborts.

Hm. I never quite did get my head around how the stacking work, that's
probably why :-)


> You could just remove the assignment from assign_xlog_sync_method,
> although it might look a bit odd to be setting open_sync_bit but
> not sync_method. It also bothers me slightly that the derived and
> main values wouldn't be set at exactly the same point --- problems
> inside guc.c might lead to them getting out of sync.
>
> Another possibility is to stick with something equivalent to the
> former design: what GUC thinks is the variable is just a dummy static
> integer in guc.c, and the assign hook is still setting the "real"
> value that the rest of the code looks at. A minor advantage of this
> second way is that the "real" value could still be declared as enum
> rather than int.

That value never was an enum, so that part is not an advantage.

I still think going with the older method would be the safest, though,
for the other reasons. You agree?


> Please fix this, and take another look at the prior WAL enum changes
> to see if the same problem hasn't been created elsewhere.

(I assume you mean GUC enum here, that seems fairly obvious)


The only other assign hooks are assign_syslog_facility and
assign_session_replication_role. The changes there are:

In these, the value was previously derived from a string and set in the
hook. It's now set directly by the GUC code, and the hook only updates
"other things" (setting the actual syslog facility, and resetting the
cache, respectively).

I think that means there are no bugs there.

//Magnus

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Magnus Hagander <magnus@hagander.net> writes:
> I still think going with the older method would be the safest, though,
> for the other reasons. You agree?

Seems reasonable to me.

> (I assume you mean GUC enum here, that seems fairly obvious)

Sorry, was writing in too much of a hurry.

> In these, the value was previously derived from a string and set in the
> hook. It's now set directly by the GUC code, and the hook only updates
> "other things" (setting the actual syslog facility, and resetting the
> cache, respectively).

> I think that means there are no bugs there.

Yeah, that's fine. I think though that I may have created a bug inside
GUC itself: the new stacking code could conceivably fail (palloc error)
between success return from the assign hook and setting up the stack
entry that is needed to undo the assignment on abort. In this situation
the assign hook would've made its "other thing" changes but there is no
GUC state to cause the hook to be called again to undo 'em. I need to
fix it so that any palloc'ing needed is done before calling the assign
hook.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> I still think going with the older method would be the safest, though,
>> for the other reasons. You agree?
>
> Seems reasonable to me.

Since it didn't really sound like a nice option, here's a third one I
came up with later. Basically, this one splits things apart so we only
use one variable, which is sync_method. Instead of using a macro to get
the open sync bit, it uses a function. This makes the assign hook only
responsible for flushing and closing the old file.

Thoughts? And if you like it, is it enough to expect the compiler to
figure out to inline it or should we explicitly inline it?

>> In these, the value was previously derived from a string and set in the
>> hook. It's now set directly by the GUC code, and the hook only updates
>> "other things" (setting the actual syslog facility, and resetting the
>> cache, respectively).
>
>> I think that means there are no bugs there.
>
> Yeah, that's fine. I think though that I may have created a bug inside
> GUC itself: the new stacking code could conceivably fail (palloc error)
> between success return from the assign hook and setting up the stack
> entry that is needed to undo the assignment on abort. In this situation
> the assign hook would've made its "other thing" changes but there is no
> GUC state to cause the hook to be called again to undo 'em. I need to
> fix it so that any palloc'ing needed is done before calling the assign
> hook.

Ok. I'll leave that to you for now I still need to figure out how the
stacking works because I want to add the "this setting came from file
X line Y" field to pg_settings, but that's a separate issue.

//Magnus


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Magnus Hagander <magnus@hagander.net> writes:
> Since it didn't really sound like a nice option, here's a third one I
> came up with later. Basically, this one splits things apart so we only
> use one variable, which is sync_method. Instead of using a macro to get
> the open sync bit, it uses a function. This makes the assign hook only
> responsible for flushing and closing the old file.

Okay, but you failed to correctly reproduce the conditions for closing
the old file.

> Thoughts? And if you like it, is it enough to expect the compiler to
> figure out to inline it or should we explicitly inline it?

I don't think we care that much, since it's only invoked when we're
about to do a moderately expensive kernel call.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:
> Okay, but you failed to correctly reproduce the conditions for closing
> the old file.

A more bulletproof solution might involve passing sync_method to
get_sync_bit as an explicit parameter, and then the assign hook
could do
if (get_sync_bit(sync_method) != get_sync_bit(new_sync_method))
XLogFileClose();

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> I wrote:
> > Okay, but you failed to correctly reproduce the conditions for
> > closing the old file.
>
> A more bulletproof solution might involve passing sync_method to
> get_sync_bit as an explicit parameter, and then the assign hook
> could do
> if (get_sync_bit(sync_method) !=
> get_sync_bit(new_sync_method)) XLogFileClose();

Right, but I still need the other part of the check, right? This one
still fails the same check as my patch, no? Because I assume the hole
you found there was that get_sync_bit() will return 0 for two different
sync methods as long as none of them are O_SYNC or O_DSYNC...

//Magnus

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Magnus Hagander <magnus@hagander.net> writes:
> Right, but I still need the other part of the check, right? This one
> still fails the same check as my patch, no? Because I assume the hole
> you found there was that get_sync_bit() will return 0 for two different
> sync methods as long as none of them are O_SYNC or O_DSYNC...

No, my point was that there are three possible states of sync_bit and
your patch only accounted for transitions between two of 'em. For
instance, if sync_bit goes to 0 we must close and reopen the file,
else we'll be doing both O_SYNC flush and whatever flush method
is supposed to be getting used.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
> > Right, but I still need the other part of the check, right? This one
> > still fails the same check as my patch, no? Because I assume the hole
> > you found there was that get_sync_bit() will return 0 for two different
> > sync methods as long as none of them are O_SYNC or O_DSYNC...
>
> No, my point was that there are three possible states of sync_bit and
> your patch only accounted for transitions between two of 'em. For
> instance, if sync_bit goes to 0 we must close and reopen the file,
> else we'll be doing both O_SYNC flush and whatever flush method
> is supposed to be getting used.

Did this every get addressed? I don't see a commit for it.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> No, my point was that there are three possible states of sync_bit and
>> your patch only accounted for transitions between two of 'em.

> Did this every get addressed? I don't see a commit for it.

I thought it got fixed here:

2008-05-14 10:02 mha

* src/backend/access/transam/xlog.c: Remove the special variable
for open_sync_bit used in O_SYNC and O_DSYNC modes, replacing it
with a call to a function that derives it from the sync_method
variable, now that it has distinct values for these two cases.

This means that assign_xlog_sync_method() no longer changes any
settings, thus fixing the bug introduced in the change to use a guc
enum for wal_sync_method.

Hmm ... or at least more or less fixed. Seems like there's no provision
to close and reopen the file if enableFsync changes. Not sure if that's
worth worrying about.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers