From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Magnus Hagander <magnus(at)hagander(dot)net> |
Cc: | pgsql-hackers(at)postgreSQL(dot)org |
Subject: | Fairly serious bug induced by latest guc enum changes |
Date: | 2008-05-12 20:22:12 |
Message-ID: | 26433.1210623732@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Magnus Hagander | 2008-05-12 20:33:33 | Re: Fairly serious bug induced by latest guc enum changes |
Previous Message | Alvaro Herrera | 2008-05-12 20:02:53 | Re: bloated heapam.h |