Re: Add CREATE support to event triggers

On Thu, Oct 9, 2014 at 6:26 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> About patch 1, which I just pushed, there were two reviews:
>
> Andres Freund wrote:
> > On 2014-09-25 18:59:31 -0300, Alvaro Herrera wrote:
> > > diff --git a/src/include/utils/ruleutils.h
> b/src/include/utils/ruleutils.h
> > > new file mode 100644
> > > index 0000000..520b066
> > > --- /dev/null
> > > +++ b/src/include/utils/ruleutils.h
>
> > I wondered for a minute whether any of these are likely to cause
> > problems for code just including builtins.h - but I think there will be
> > sufficiently few callers for them to make that not much of a concern.
>
> Great.
>
> Michael Paquier wrote:
>
> > Patch 1: I still like this patch as it gives a clear separation of the
> > built-in functions and the sub-functions of ruleutils.c that are
> > completely independent. Have you considered adding the external
> > declaration of quote_all_identifiers as well? It is true that this
> > impacts extensions (some of my stuff as well), but my point is to bite
> > the bullet and make the separation cleaner between builtins.h and
> > ruleutils.h. Attached is a patch that can be applied on top of patch 1
> > doing so... Feel free to discard for the potential breakage this would
> > create though.
>
> Yes, I did consider moving the quoting function definitions and the
> global out of builtins.h. It is much more disruptive, however, because
> it affects a lot of external code not only our own; and given the looks
> I got after people had to mess with adding the htup_details.h header to
> external projects due to the refactoring I did to htup.h in an earlier
> release, I'm not sure about it.
>
> However, if I were to do it, I would instead create a quote.h file and
> would also add the quote_literal_cstr() prototype to it, perhaps even
> move the implementations from their current ruleutils.c location to
> quote.c. (Why is half the stuff in ruleutils.c rather than quote.c is
> beyond me.)
>
Yes, an extra header file would be cleaner. Now if we are worried about
creating much incompatibilities with existing extension (IMO that's not
something core should worry much about), then it is better not to do it.
Now, if I can vote on it, I think it is worth doing in order to have
builtins.h only contain only Datum foo(PG_FUNCTION_ARGS), and move all the
quote-related things in quote.c.
--
Michael