Re: Event Triggers reduced, v1

The Windows buildfarm members don't seem too happy with the latest
patch.

regards, tom lane

On Fri, Jul 20, 2012 at 2:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The Windows buildfarm members don't seem too happy with the latest
> patch.

I'm looking at this now, but am so far mystified. Something's
obviously broken as regards how the trigger flags get set up, but if
that were broken in a trivial way it would be broken everywhere, and
it's not. Will keep searching...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 20, 2012 at 3:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 20, 2012 at 2:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The Windows buildfarm members don't seem too happy with the latest
>> patch.
>
> I'm looking at this now, but am so far mystified. Something's
> obviously broken as regards how the trigger flags get set up, but if
> that were broken in a trivial way it would be broken everywhere, and
> it's not. Will keep searching...

I think this is fixed now. Let me know if anyone sees evidence of
remaining breakage.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

I'm back to PostgreSQL development concerns after some distraction here.
First, thanks for pushing the patch to commit!

I've been reviewing your changes and here's a very small patch with some
details I would have spelled out differently. See what you think, I
mostly needed to edit some code to get back in shape :)

Coming next, catch-up with things I've missed and extending the included
support for event triggers in term of function parameters (rewritten
command string, object kind, etc), and maybe PL support too.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Mon, Aug 27, 2012 at 7:52 AM, Dimitri Fontaine
<dimitri(at)2ndquadrant(dot)fr> wrote:
> I've been reviewing your changes and here's a very small patch with some
> details I would have spelled out differently. See what you think, I
> mostly needed to edit some code to get back in shape :)

I guess I don't particularly like either of these changes. The first
one is mostly harmless, but I don't really see why it's any better,
and it does have the downside of traversing the string twice (once for
strlen and a second time in str_toupper) instead of just once. It
also makes a line wider than 80 columns, which is a bit ugly. In the
second hunk, the point is that we never have to do CreateCommandTag()
here at all unless either casserts are enabled or EventCacheLookup
returns a non-empty list. That means that in a non-assert-enabled
build, we get to skip that work altogether in the presumably-common
case where there are no relevant event triggers. Your proposed change
would avoid doing it twice when asserts are disabled, but the cost
would be that we'd have to do it once when asserts were disabled even
if no event triggers exist. I don't think that's a good trade-off.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I guess I don't particularly like either of these changes. The first

Fair enough.

> one is mostly harmless, but I don't really see why it's any better,
> and it does have the downside of traversing the string twice (once for
> strlen and a second time in str_toupper) instead of just once. It
> also makes a line wider than 80 columns, which is a bit ugly. In the

It's much easier to read, I think. The line is 79 columns here given the
project Emacs setup wrt tabs, see src/tools/editors/emacs.samples.

> second hunk, the point is that we never have to do CreateCommandTag()
> here at all unless either casserts are enabled or EventCacheLookup
> returns a non-empty list. That means that in a non-assert-enabled
> build, we get to skip that work altogether in the presumably-common
> case where there are no relevant event triggers. Your proposed change
> would avoid doing it twice when asserts are disabled, but the cost
> would be that we'd have to do it once when asserts were disabled even
> if no event triggers exist. I don't think that's a good trade-off.

Well I needed more exercise before sending a patch then, I just missed
that.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support