[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

"Jonah H. Harris" <jonah(dot)harris(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)harris(at)gmail(dot)com> writes:
> On Thu, Nov 6, 2008 at 9:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)harris(at)gmail(dot)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(dot)harris(at)gmail(dot)com> wrote:
> On Thu, Nov 6, 2008 at 12:03 AM, Jonah H. Harris <jonah(dot)harris(at)gmail(dot)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(dot)harris(at)gmail(dot)com> wrote:
>> On Thu, Nov 6, 2008 at 12:03 AM, Jonah H. Harris <jonah(dot)harris(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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