Fairly serious bug induced by latest guc enum changes

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

Responses

Browse pgsql-hackers by date

  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