Re: Fairly serious bug induced by latest guc enum changes

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