While working on the join elimination patch, I was going through the trigger code and found quite a bit of nastiness in regard to naming and variable repurposing related to the addition of replication roles in 8.3. The most obvious issue is that tgenabled was switched from a bool to char to support replication roles. From a naming standpoint, the term "enabled" generally implies boolean and is fairly consistently used as such in other functions within the core. My initial preference would be to return tgenabled to its original boolean for use only in enabling/disabling triggers. Then, I'd probably add another boolean entry (tgreplica?) for use in determining whether the trigger should be fired on origin/local or replica. Otherwise, the naming of EnableDisableTrigger and friends seems a bit contradictory due to the fact that it has the ability to convert a trigger into a replica trigger. Similarly, I can't see any reason for keeping the structure member name the same, especially when the change from bool to char broke backward compatibility anyway. Thoughts?
As I wasn't sure whether anyone agrees with my distaste for repurposing tgenabled as mentioned above, I have attached is a patch which minimally corrects the function comment for EnableDisableTrigger where fires_when is concerned. -- Jonah H. Harris, Senior DBA myYearbook.com
endisable_trig_fctn_commnt_cleanup.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
