[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
Attachments:
endisable_trig_fctn_commnt_cleanup.patchapplication/octet-stream; name=endisable_trig_fctn_commnt_cleanup.patchDownload
diff -cr pgsql/src/backend/commands/trigger.c pgsql-trigcommentclean/src/backend/commands/trigger.c
*** pgsql/src/backend/commands/trigger.c Fri Oct 24 19:42:35 2008
--- pgsql-trigcommentclean/src/backend/commands/trigger.c Wed Nov 5 23:32:27 2008
***************
*** 1016,1027 ****
/*
* EnableDisableTrigger()
*
! * Called by ALTER TABLE ENABLE/DISABLE TRIGGER
* to change 'tgenabled' field for the specified trigger(s)
*
* rel: relation to process (caller must hold suitable lock on it)
* tgname: trigger to process, or NULL to scan all triggers
! * enable: new value for tgenabled field
* skip_system: if true, skip "system" triggers (constraint triggers)
*
* Caller should have checked permissions for the table; here we also
--- 1016,1029 ----
/*
* EnableDisableTrigger()
*
! * Called by ALTER TABLE ENABLE/DISABLE [ REPLICA | ALWAYS ] TRIGGER
* to change 'tgenabled' field for the specified trigger(s)
*
* rel: relation to process (caller must hold suitable lock on it)
* tgname: trigger to process, or NULL to scan all triggers
! * fires_when: when the trigger should be fired. In addition to generic
! * enablement/disablement, this also defines when the trigger
! * should be fired in session replication roles (assigned to tgenabled)
* skip_system: if true, skip "system" triggers (constraint triggers)
*
* Caller should have checked permissions for the table; here we also
diff -cr pgsql/src/include/commands/trigger.h pgsql-trigcommentclean/src/include/commands/trigger.h
*** pgsql/src/include/commands/trigger.h Fri Oct 24 19:42:35 2008
--- pgsql-trigcommentclean/src/include/commands/trigger.h Wed Nov 5 23:16:31 2008
***************
*** 95,100 ****
--- 95,103 ----
#define SESSION_REPLICATION_ROLE_LOCAL 2
extern PGDLLIMPORT int SessionReplicationRole;
+ /*
+ * States at which a trigger can be fired.
+ */
#define TRIGGER_FIRES_ON_ORIGIN 'O'
#define TRIGGER_FIRES_ALWAYS 'A'
#define TRIGGER_FIRES_ON_REPLICA 'R'
"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:
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:
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 :)
--
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