Thread: [PATCH] EnableDisableTrigger Cleanup & Questions
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
Attachment
"Jonah H. Harris" <jonah.harris@gmail.com> writes:
> 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.
It would have been useful to make this criticism before 8.3 was
released. I don't think it's reasonable to change it now.
regards, tom lane
On Thu, Nov 6, 2008 at 9:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It would have been useful to make this criticism before 8.3 was > released. I don't think it's reasonable to change it now. Well, I didn't have time to review code back in the 8.3 days, and ugly is ugly regardless of when it was originally committted. I'm not saying it needs to be an 8.4 fix, just that as a whole, several of the components of that patch (including rewrite) seem to be a little hackish and that they could be cleaned up in 8.5. I would imagine someone will be working on trigger-related code in 8.5, and just thought it would be nice to clean it up if one had the time to do so. -- Jonah H. Harris, Senior DBA myYearbook.com
"Jonah H. Harris" <jonah.harris@gmail.com> writes:
> On Thu, Nov 6, 2008 at 9:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It would have been useful to make this criticism before 8.3 was
>> released. I don't think it's reasonable to change it now.
> Well, I didn't have time to review code back in the 8.3 days, and ugly
> is ugly regardless of when it was originally committted. I'm not
> saying it needs to be an 8.4 fix, just that as a whole, several of the
> components of that patch (including rewrite) seem to be a little
> hackish and that they could be cleaned up in 8.5.
I have no objection to cleaning up the backend internals, but system
catalog definitions are client-visible. I don't think we should thrash
the catalog definitions for minor aesthetic improvements. Since 8.3 is
already out, that means client-side code (like pg_dump and psql, and
probably other programs we don't control) is going to have to deal with
the existing definition for the foreseeable future. Dealing with this
definition *and* a slightly cleaner one isn't a net improvement from the
client standpoint.
regards, tom lane
On Thu, Nov 6, 2008 at 10:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I have no objection to cleaning up the backend internals, but system > catalog definitions are client-visible. I don't think we should thrash > the catalog definitions for minor aesthetic improvements. Since 8.3 is > already out, that means client-side code (like pg_dump and psql, and > probably other programs we don't control) is going to have to deal with > the existing definition for the foreseeable future. Dealing with this > definition *and* a slightly cleaner one isn't a net improvement from the > client standpoint. Well, it didn't seem like anyone had an issue changing the definition at 8.3 time. As for pg_dump/psql, those changes are fairly simple. And, there aren't that many PG utilities out there. PGAdmin looks like it would require a 1-3 line change (depending on coding preferences) and I don't see anything that checks it in Slony. I'm fine with cleaning up the internal-side, I just don't think there's that much relying on tgenabled. In fact, Google code search seems to show more things relying on a boolean tgenabled rather than the current implementation. Oh well, it was just a thought. -- Jonah H. Harris, Senior DBA myYearbook.com
On Thu, Nov 6, 2008 at 12:03 AM, Jonah H. Harris <jonah.harris@gmail.com> wrote:
Was there a reason that this cleanup patch wasn't applied?
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.
Was there a reason that this cleanup patch wasn't applied?
--
Jonah H. Harris, Senior DBA
myYearbook.com
On Wed, Jan 21, 2009 at 6:17 AM, Jonah H. Harris <jonah.harris@gmail.com> wrote: > On Thu, Nov 6, 2008 at 12:03 AM, Jonah H. Harris <jonah.harris@gmail.com> > wrote: >> >> 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. > > Was there a reason that this cleanup patch wasn't applied? 1. It was submitted after the deadline for CommitFest:November. 2. It sounded like you had given up: > Oh well, it was just a thought. 3. Tom Lane objected to it. http://archives.postgresql.org/message-id/20096.1225984128@sss.pgh.pa.us If you want it to be considered further, you might add it here: http://wiki.postgresql.org/wiki/CommitFest_2009-First ...Robert
Robert Haas wrote: > On Wed, Jan 21, 2009 at 6:17 AM, Jonah H. Harris <jonah.harris@gmail.com> wrote: >> On Thu, Nov 6, 2008 at 12:03 AM, Jonah H. Harris <jonah.harris@gmail.com> >> wrote: >>> 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. >> Was there a reason that this cleanup patch wasn't applied? > > 1. It was submitted after the deadline for CommitFest:November. Well, it's just comment changes... > 2. It sounded like you had given up: That's the impression I had, until I just went and read the thread in detail. >> Oh well, it was just a thought. > > 3. Tom Lane objected to it. > > http://archives.postgresql.org/message-id/20096.1225984128@sss.pgh.pa.us If I understood the discussion correctly, Tom objected to the more drastic change of renaming the catalog column. But the patch Jonah posted didn't do that, it only changed the comments, precisely because he felt that others might not want the more drastic change, (I haven't checked whether the comment changes are a good idea. But they probably are..) -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas escribió: > (I haven't checked whether the comment changes are a good idea. But they > probably are..) The original comments are broken, the new ones seem good. I think this patch should just be applied. The only possible gripe I have is that the grammar in the second hunk seems strange or broken, but maybe it's just that I don't know the language enough. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera escribió: > The only possible gripe I have is that the grammar in the second hunk > seems strange or broken, but maybe it's just that I don't know the > language enough. Oh, it makes sense if you consider "states" as a noun rather than a verb. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>>> Was there a reason that this cleanup patch wasn't applied? >> >> 1. It was submitted after the deadline for CommitFest:November. > > Well, it's just comment changes... Oh, didn't realize that. That's what I get for replying without reading the patch... ...Robert
On Wed, Jan 21, 2009 at 2:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Yes :)
>>> Was there a reason that this cleanup patch wasn't applied?Oh, didn't realize that. That's what I get for replying without
>>
>> 1. It was submitted after the deadline for CommitFest:November.
>
> Well, it's just comment changes...
reading the patch...
Yes :)
--
Jonah H. Harris, Senior DBA
myYearbook.com
Jonah H. Harris wrote: > On Wed, Jan 21, 2009 at 2:02 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>>>> Was there a reason that this cleanup patch wasn't applied? >>>> 1. It was submitted after the deadline for CommitFest:November. >>> Well, it's just comment changes... >> Oh, didn't realize that. That's what I get for replying without >> reading the patch... > > Yes :) Committed, thanks. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com