Re: Add CREATE support to event triggers
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.)
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: CAB7nPqQTZZpWVZXPAoZ9VsZH2bFg-DxxGTYE19uohUwUAQSRFg@mail.gmail.com20140925235724.GH16581@awork2.anarazel.de
On Thu, Oct 9, 2014 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.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.hb/src/include/utils/ruleutils.h
new file mode 100644 index 0000000..520b066 --- /dev/null +++ b/src/include/utils/ruleutils.hI 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
On Thu, Oct 9, 2014 at 8:29 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Thu, Oct 9, 2014 at 6:26 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote: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.
The attached patch implements this idea, creating a new header quote.h
in include/utils that groups all the quote-related functions. This
makes the code more organized.
Regards,
--
Michael