Re: [HACKERS] split builtins.h to quote.h
On 11/08/2014 12:37 AM, Michael Paquier wrote: On Sat, Nov 8, 2014 at 5:55 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Michael Paquier wrote: On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. Moving everything to quote.h is done in-line with what Tom and Robert suggested, builtins.h being a refuge for function declarations that have nowhere else to go. Suggestion from here: http://www.postgresql.org/message-id/ca+tgmozf3dkptua6ex6gxlnnd-nms-fbjcxroitwffh-+6y...@mail.gmail.com Did you miss this one? http://www.postgresql.org/message-id/31728.1413209...@sss.pgh.pa.us Well, yes :) I missed that. Note that I am leaning to Robert's direction as well to do a clear separation... Now if the final consensus is different, then let's use the patch attached that puts the SQL functions to builtins.h, and the rest in quote.h. I am unlcear about what the consensus is on this, and don't have strong feelings either way. Do we need a vote? It's not of earth-shattering importance, but my slight inclination would be to do the minimally invasive thing where there is disagreement. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
Andrew Dunstan and...@dunslane.net writes: On 11/08/2014 12:37 AM, Michael Paquier wrote: Well, yes :) I missed that. Note that I am leaning to Robert's direction as well to do a clear separation... Now if the final consensus is different, then let's use the patch attached that puts the SQL functions to builtins.h, and the rest in quote.h. I am unlcear about what the consensus is on this, and don't have strong feelings either way. Do we need a vote? It's not of earth-shattering importance, but my slight inclination would be to do the minimally invasive thing where there is disagreement. Well, the minimally invasive thing would be to reject the patch altogether. Do we really need this? In a quick look, the patch seems to result in strictly increasing the number of #include's needed, which ISTM is not a positive sign for a refactoring, especially given the number of files it hits. If there had been some #include's removed as well, I'd be happier. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Sun, Dec 14, 2014 at 1:00 AM, Tom Lane t...@sss.pgh.pa.us wrote: Andrew Dunstan and...@dunslane.net writes: On 11/08/2014 12:37 AM, Michael Paquier wrote: Well, yes :) I missed that. Note that I am leaning to Robert's direction as well to do a clear separation... Now if the final consensus is different, then let's use the patch attached that puts the SQL functions to builtins.h, and the rest in quote.h. I am unlcear about what the consensus is on this, and don't have strong feelings either way. Do we need a vote? It's not of earth-shattering importance, but my slight inclination would be to do the minimally invasive thing where there is disagreement. Well, the minimally invasive thing would be to reject the patch altogether. Do we really need this? In a quick look, the patch seems to result in strictly increasing the number of #include's needed, which ISTM is not a positive sign for a refactoring, especially given the number of files it hits. If there had been some #include's removed as well, I'd be happier. Let's do so then. I marked it as rejected. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
Michael Paquier wrote: On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. Moving everything to quote.h is done in-line with what Tom and Robert suggested, builtins.h being a refuge for function declarations that have nowhere else to go. Suggestion from here: http://www.postgresql.org/message-id/ca+tgmozf3dkptua6ex6gxlnnd-nms-fbjcxroitwffh-+6y...@mail.gmail.com Did you miss this one? http://www.postgresql.org/message-id/31728.1413209...@sss.pgh.pa.us -- Álvaro Herrerahttp://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
Re: [HACKERS] split builtins.h to quote.h
On Fri, Nov 7, 2014 at 3:55 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Michael Paquier wrote: On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. Moving everything to quote.h is done in-line with what Tom and Robert suggested, builtins.h being a refuge for function declarations that have nowhere else to go. Suggestion from here: http://www.postgresql.org/message-id/ca+tgmozf3dkptua6ex6gxlnnd-nms-fbjcxroitwffh-+6y...@mail.gmail.com Did you miss this one? http://www.postgresql.org/message-id/31728.1413209...@sss.pgh.pa.us I personally think that's getting our priorities backwards, but there's clearly a spectrum in terms of how much people care about the cost of partial compiles, and I'm clearly all the way on one end of it. I don't like having to think hard about where a function prototype is or should be, and getting more consistency there would, for me, outweigh all other considerations. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
Robert Haas wrote: I personally think that's getting our priorities backwards, but there's clearly a spectrum in terms of how much people care about the cost of partial compiles, and I'm clearly all the way on one end of it. I don't like having to think hard about where a function prototype is or should be, and getting more consistency there would, for me, outweigh all other considerations. fmgr.h is a nasty header which would do well to avoid including in other headers as much as possible; it makes compilation in frontend environment impossible. For headers that don't otherwise need fmgr.h, my preference is to keep the SQL-callable declarations in builtins.h or some other dedicated header. -- Álvaro Herrerahttp://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
Re: [HACKERS] split builtins.h to quote.h
On Fri, Nov 7, 2014 at 4:42 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas wrote: I personally think that's getting our priorities backwards, but there's clearly a spectrum in terms of how much people care about the cost of partial compiles, and I'm clearly all the way on one end of it. I don't like having to think hard about where a function prototype is or should be, and getting more consistency there would, for me, outweigh all other considerations. fmgr.h is a nasty header which would do well to avoid including in other headers as much as possible; it makes compilation in frontend environment impossible. For headers that don't otherwise need fmgr.h, my preference is to keep the SQL-callable declarations in builtins.h or some other dedicated header. Well, there's something to that argument. I can live with whatever you decide to do here, but given my druthers, the prototypes for src/backend/X/Y/Z.c would all be in src/include/X/Z.h. If that means shuffling some code around, I'd rather do that than have the prototypes in strange places. But if I get outvoted, oh well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Sat, Nov 8, 2014 at 5:55 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Michael Paquier wrote: On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. Moving everything to quote.h is done in-line with what Tom and Robert suggested, builtins.h being a refuge for function declarations that have nowhere else to go. Suggestion from here: http://www.postgresql.org/message-id/ca+tgmozf3dkptua6ex6gxlnnd-nms-fbjcxroitwffh-+6y...@mail.gmail.com Did you miss this one? http://www.postgresql.org/message-id/31728.1413209...@sss.pgh.pa.us Well, yes :) I missed that. Note that I am leaning to Robert's direction as well to do a clear separation... Now if the final consensus is different, then let's use the patch attached that puts the SQL functions to builtins.h, and the rest in quote.h. Regards, -- Michael From f69abec98e96dcbaff3476a07a9d6536d9d5b5e5 Mon Sep 17 00:00:00 2001 From: Michael Paquier michael@otacoo.com Date: Sat, 11 Oct 2014 22:28:05 +0900 Subject: [PATCH] Move all quote-related functions into a single header quote.h Note that this impacts as well contrib modules, and mainly external modules particularly for quote_identifier. --- contrib/dblink/dblink.c | 1 + contrib/postgres_fdw/deparse.c| 1 + contrib/postgres_fdw/postgres_fdw.c | 1 + contrib/tablefunc/tablefunc.c | 1 + contrib/test_decoding/test_decoding.c | 1 + contrib/worker_spi/worker_spi.c | 2 +- src/backend/catalog/namespace.c | 1 + src/backend/catalog/objectaddress.c | 1 + src/backend/commands/explain.c| 1 + src/backend/commands/matview.c| 2 +- src/backend/commands/trigger.c| 1 + src/backend/parser/parse_utilcmd.c| 1 + src/backend/utils/adt/format_type.c | 1 + src/backend/utils/adt/quote.c | 110 ++ src/backend/utils/adt/regproc.c | 1 + src/backend/utils/adt/ri_triggers.c | 1 + src/backend/utils/adt/ruleutils.c | 109 + src/backend/utils/adt/varlena.c | 1 + src/backend/utils/cache/ts_cache.c| 1 + src/backend/utils/misc/guc.c | 1 + src/include/utils/builtins.h | 6 -- src/include/utils/quote.h | 27 + src/pl/plpgsql/src/pl_gram.y | 1 + src/pl/plpython/plpy_plpymodule.c | 1 + 24 files changed, 158 insertions(+), 116 deletions(-) create mode 100644 src/include/utils/quote.h diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index d77b3ee..cbbc513 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -56,6 +56,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/tqual.h diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 2045774..0009cd3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -50,6 +50,7 @@ #include parser/parsetree.h #include utils/builtins.h #include utils/lsyscache.h +#include utils/quote.h #include utils/syscache.h diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 4c49776..feb41f5 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -36,6 +36,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h PG_MODULE_MAGIC; diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c index 10ee8c7..66e5ccf 100644 --- a/contrib/tablefunc/tablefunc.c +++ b/contrib/tablefunc/tablefunc.c @@ -41,6 +41,7 @@ #include lib/stringinfo.h #include miscadmin.h #include utils/builtins.h +#include utils/quote.h #include tablefunc.h diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c index fdbd313..b168fc3 100644 --- a/contrib/test_decoding/test_decoding.c +++ b/contrib/test_decoding/test_decoding.c @@ -25,6 +25,7 @@ #include utils/builtins.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/relcache.h #include utils/syscache.h diff --git a/contrib/worker_spi/worker_spi.c b/contrib/worker_spi/worker_spi.c index 328c722..ec6c9fa 100644 --- a/contrib/worker_spi/worker_spi.c +++ b/contrib/worker_spi/worker_spi.c @@ -37,7 +37,7 @@ #include fmgr.h #include lib/stringinfo.h #include pgstat.h -#include utils/builtins.h +#include utils/quote.h #include utils/snapmgr.h #include tcop/utility.h diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 911f015..76a50ac 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -54,6 +54,7 @@ #include utils/inval.h #include
Re: [HACKERS] split builtins.h to quote.h
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Le 14/10/2014 10:00, Michael Paquier a écrit : On Mon, Oct 13, 2014 at 11:14 PM, Robert Haas robertmh...@gmail.com wrote: IMHO, putting some prototypes for a .c file in one header and others in another header is going to make it significantly harder to figure out which files you need to #include when. Keeping a simple rule there seems essential to me. OK. Let's make the separation clear then. The attached does so, and moves the remaining quote_* functions previously in builtins.h to quote.h. I am adding it to the upcoming CF for tracking the effort on it. Hi Michael, I just reviewed this patch : * applies cleanly to master(d2b8a2c7) * all regression tests pass As it's only moving functions from builtins.h to quote.h and update impacted files, nothing special to add. It will probably break some user extensions using quote_* functions. Otherwise it looks ready to commit. Regards. - -- Julien Rouhaud http://dalibo.com - http://dalibo.org -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.11 (GNU/Linux) iQEcBAEBAgAGBQJUW6wEAAoJELGaJ8vfEpOqCn4H/AydFB5ERI0x9R6l8T/Qkx9/ Tm0ZgSSsfG39lHkbspZaUwTxmNPam1+EueQrw2VQcT3JXjWaHWvurWjFDBdSi8Ar fgcD4WOTcyenxsckq5/XwT++ZfOZa1h2qBABoC4nx1qcO4pNBW51ywL11Fmcb7O8 CkwKZ3FfUlVFLsh2Novqa7HtEWdi872zzb4TSxQjsShMuFNX/6PF6RFJVZAy61VA sbVQ/0rRQ9bOcJgh+DDaIMVQAnOUWsvYdR9VbRCbgcZuBlZKeW7gt9qGgevgjcun PYB+ZuSC92WiS9y3OhcGKp9cYQpcsOD5ZDxe1Cw7q4YNZNgUtbKpDW6GsTC+vp0= =NH1j -END PGP SIGNATURE- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
Julien Rouhaud wrote: I just reviewed this patch : * applies cleanly to master(d2b8a2c7) * all regression tests pass As it's only moving functions from builtins.h to quote.h and update impacted files, nothing special to add. It will probably break some user extensions using quote_* functions. Otherwise it looks ready to commit. I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. -- Álvaro Herrerahttp://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
Re: [HACKERS] split builtins.h to quote.h
On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Julien Rouhaud wrote: I just reviewed this patch : * applies cleanly to master(d2b8a2c7) * all regression tests pass As it's only moving functions from builtins.h to quote.h and update impacted files, nothing special to add. It will probably break some user extensions using quote_* functions. Otherwise it looks ready to commit. I thought the consensus was that the SQL-callable function declarations should remain in builtins.h -- mainly so that quote.h does not need to include fmgr.h. Moving everything to quote.h is done in-line with what Tom and Robert suggested, builtins.h being a refuge for function declarations that have nowhere else to go. Suggestion from here: http://www.postgresql.org/message-id/ca+tgmozf3dkptua6ex6gxlnnd-nms-fbjcxroitwffh-+6y...@mail.gmail.com -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Mon, Oct 13, 2014 at 11:14 PM, Robert Haas robertmh...@gmail.com wrote: IMHO, putting some prototypes for a .c file in one header and others in another header is going to make it significantly harder to figure out which files you need to #include when. Keeping a simple rule there seems essential to me. OK. Let's make the separation clear then. The attached does so, and moves the remaining quote_* functions previously in builtins.h to quote.h. I am adding it to the upcoming CF for tracking the effort on it. -- Michael From d36cb786e15a296ed9b1a2b3d84741821aaa01df Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Sat, 11 Oct 2014 22:28:05 +0900 Subject: [PATCH] Move all quote-related functions into a single header quote.h Note that this impacts as well contrib modules, and mainly external modules particularly for quote_identifier. --- contrib/dblink/dblink.c | 1 + contrib/postgres_fdw/deparse.c| 1 + contrib/postgres_fdw/postgres_fdw.c | 1 + contrib/tablefunc/tablefunc.c | 1 + contrib/test_decoding/test_decoding.c | 1 + contrib/worker_spi/worker_spi.c | 2 +- src/backend/catalog/namespace.c | 1 + src/backend/catalog/objectaddress.c | 1 + src/backend/commands/explain.c| 1 + src/backend/commands/extension.c | 1 + src/backend/commands/matview.c| 2 +- src/backend/commands/trigger.c| 1 + src/backend/commands/tsearchcmds.c| 1 + src/backend/parser/parse_utilcmd.c| 1 + src/backend/utils/adt/format_type.c | 1 + src/backend/utils/adt/quote.c | 110 ++ src/backend/utils/adt/regproc.c | 1 + src/backend/utils/adt/ri_triggers.c | 1 + src/backend/utils/adt/ruleutils.c | 109 + src/backend/utils/adt/varlena.c | 1 + src/backend/utils/cache/ts_cache.c| 1 + src/backend/utils/misc/guc.c | 1 + src/include/utils/builtins.h | 11 src/include/utils/quote.h | 31 ++ src/pl/plpgsql/src/pl_gram.y | 1 + src/pl/plpython/plpy_plpymodule.c | 1 + 26 files changed, 164 insertions(+), 121 deletions(-) create mode 100644 src/include/utils/quote.h diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index d77b3ee..cbbc513 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -56,6 +56,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/tqual.h diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 2045774..0009cd3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -50,6 +50,7 @@ #include parser/parsetree.h #include utils/builtins.h #include utils/lsyscache.h +#include utils/quote.h #include utils/syscache.h diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 4c49776..feb41f5 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -36,6 +36,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h PG_MODULE_MAGIC; diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c index 10ee8c7..66e5ccf 100644 --- a/contrib/tablefunc/tablefunc.c +++ b/contrib/tablefunc/tablefunc.c @@ -41,6 +41,7 @@ #include lib/stringinfo.h #include miscadmin.h #include utils/builtins.h +#include utils/quote.h #include tablefunc.h diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c index fdbd313..b168fc3 100644 --- a/contrib/test_decoding/test_decoding.c +++ b/contrib/test_decoding/test_decoding.c @@ -25,6 +25,7 @@ #include utils/builtins.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/relcache.h #include utils/syscache.h diff --git a/contrib/worker_spi/worker_spi.c b/contrib/worker_spi/worker_spi.c index 328c722..ec6c9fa 100644 --- a/contrib/worker_spi/worker_spi.c +++ b/contrib/worker_spi/worker_spi.c @@ -37,7 +37,7 @@ #include fmgr.h #include lib/stringinfo.h #include pgstat.h -#include utils/builtins.h +#include utils/quote.h #include utils/snapmgr.h #include tcop/utility.h diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 5eb8fd4..7f8f54b 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -53,6 +53,7 @@ #include utils/inval.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/syscache.h diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index b69b75b..d7a1ec7 100644 --- a/src/backend/catalog/objectaddress.c +++ b/src/backend/catalog/objectaddress.c @@ -74,6 +74,7 @@ #include utils/builtins.h #include utils/fmgroids.h
Re: [HACKERS] split builtins.h to quote.h
On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost sfr...@snowman.net wrote: * Noah Misch (n...@leadboat.com) wrote: On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. That seems reasonable to me also- though I'd caveat it as for now and make sure to make a note of the reason it's included in the comments... Yuck. I think if we're going to break it, we should just break it. No significant advantage will be gained by splitting it out and then #including it; nobody's really going to fix their module builds until they actually break. On the general substance of the issue, our usual convention is that src/backend/X/Y/Z.c has its prototypes in src/include/X/Z.h. If this proposal moves us closer to that, I'm OK with enduring the module breakage that will result. If, on the other hand, it in moves us further away from that, then I'm against it. What I find strange about the actual patch is that it moves some but not all of the prototypes for the stuff that ends up in quote.c into quote.h. That doesn't seem right. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Mon, Oct 13, 2014 at 10:04 PM, Robert Haas robertmh...@gmail.com wrote: On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost sfr...@snowman.net wrote: * Noah Misch (n...@leadboat.com) wrote: On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. That seems reasonable to me also- though I'd caveat it as for now and make sure to make a note of the reason it's included in the comments... Yuck. I think if we're going to break it, we should just break it. No significant advantage will be gained by splitting it out and then #including it; nobody's really going to fix their module builds until they actually break. On the general substance of the issue, our usual convention is that src/backend/X/Y/Z.c has its prototypes in src/include/X/Z.h. If this proposal moves us closer to that, I'm OK with enduring the module breakage that will result. If, on the other hand, it in moves us further away from that, then I'm against it. That's a 2/2 tie then AFAIK: Noah and Stephen express concerns about the breakage, you and I would be fine with a clear breakage to make code more organized (correct me if you don't feel this way). What I find strange about the actual patch is that it moves some but not all of the prototypes for the stuff that ends up in quote.c into quote.h. That doesn't seem right. Are you referring to the Datum quote_*(PG_FUNCTION_ARGS) that are still let in builtins.h? That was let on purpose to let all the SQL functions within builtins.h but I'd be happy to move everything to quote.h to make the separation clearer. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
* Robert Haas (robertmh...@gmail.com) wrote: On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost sfr...@snowman.net wrote: * Noah Misch (n...@leadboat.com) wrote: On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. That seems reasonable to me also- though I'd caveat it as for now and make sure to make a note of the reason it's included in the comments... Yuck. I think if we're going to break it, we should just break it. I'm fine with that, for my part- was simply looking for a compromise, and having a deprecated period of time seemed reasonable. No significant advantage will be gained by splitting it out and then #including it; nobody's really going to fix their module builds until they actually break. Well, hopefully folks on -hackers would, though I agree that others aren't likely to. What I find strange about the actual patch is that it moves some but not all of the prototypes for the stuff that ends up in quote.c into quote.h. That doesn't seem right. Agreed. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] split builtins.h to quote.h
Michael Paquier michael.paqu...@gmail.com writes: On Mon, Oct 13, 2014 at 10:04 PM, Robert Haas robertmh...@gmail.com wrote: No significant advantage will be gained by splitting it out and then #including it; nobody's really going to fix their module builds until they actually break. On the general substance of the issue, our usual convention is that src/backend/X/Y/Z.c has its prototypes in src/include/X/Z.h. If this proposal moves us closer to that, I'm OK with enduring the module breakage that will result. If, on the other hand, it in moves us further away from that, then I'm against it. That's a 2/2 tie then AFAIK: Noah and Stephen express concerns about the breakage, you and I would be fine with a clear breakage to make code more organized (correct me if you don't feel this way). FWIW, we break code *all the time* in the direction of requiring new #include's. I think the argument that this particular case should be sacrosanct is pretty thin. If we were back-patching then the considerations would be different --- but as long as it's 9.5 material I have no problem with it. What I find strange about the actual patch is that it moves some but not all of the prototypes for the stuff that ends up in quote.c into quote.h. That doesn't seem right. Are you referring to the Datum quote_*(PG_FUNCTION_ARGS) that are still let in builtins.h? That was let on purpose to let all the SQL functions within builtins.h but I'd be happy to move everything to quote.h to make the separation clearer. I agree with Robert on this one. builtins.h is really just a refuge for declaring SQL-level functions that have no other natural home. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
I wrote: Michael Paquier michael.paqu...@gmail.com writes: Are you referring to the Datum quote_*(PG_FUNCTION_ARGS) that are still let in builtins.h? That was let on purpose to let all the SQL functions within builtins.h but I'd be happy to move everything to quote.h to make the separation clearer. I agree with Robert on this one. builtins.h is really just a refuge for declaring SQL-level functions that have no other natural home. Actually, on second thought I'm not so sure. What we've done in other modules is to put SQL function declarations into builtins.h rather than a module-specific header, because otherwise we'd have had to #include fmgr.h in the module-specific header, and that did not seem like a win. If there is some independent reason why quote.h needs to include fmgr.h then we may as well move the SQL functions there too; but if not, I'd just as soon keep down the amount of header inclusion needed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Mon, Oct 13, 2014 at 9:24 AM, Michael Paquier michael.paqu...@gmail.com wrote: That's a 2/2 tie then AFAIK: Noah and Stephen express concerns about the breakage, you and I would be fine with a clear breakage to make code more organized (correct me if you don't feel this way). Well, I think that the long-standing agglomeration of prototypes from many header files into builtins.h is kind of a wart. But I also think that it's a justified wart, to the extent that we wish to avoid creating many header files with only a tiny handful of prototypes. So I'm not in a tremendous hurry to change it, but I'm OK with changing it if other people feel there's a benefit. The main reason to separate out quote.h, AIUI, is that unlike a lot of the stuff in builtins.h, those functions are actually called from quite a few places. What I find strange about the actual patch is that it moves some but not all of the prototypes for the stuff that ends up in quote.c into quote.h. That doesn't seem right. Are you referring to the Datum quote_*(PG_FUNCTION_ARGS) that are still let in builtins.h? That was let on purpose to let all the SQL functions within builtins.h but I'd be happy to move everything to quote.h to make the separation clearer. IMHO, putting some prototypes for a .c file in one header and others in another header is going to make it significantly harder to figure out which files you need to #include when. Keeping a simple rule there seems essential to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Sat, Oct 11, 2014 at 10:44:39AM -0300, Alvaro Herrera wrote: Started a new thread to raise awareness. Ref: this comes from http://www.postgresql.org/message-id/cab7npqr1ivd5r_qn_ngmkbolqmagbosj4wnpo8eybnn6we_...@mail.gmail.com Thanks. You can assume I'm -1 on every header split proposal not driving a substantial compile duration improvement: http://www.postgresql.org/message-id/flat/20130917163056.ga327...@tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On Sat, Oct 11, 2014 at 05:19:27PM -0400, Noah Misch wrote: On Sat, Oct 11, 2014 at 10:44:39AM -0300, Alvaro Herrera wrote: Started a new thread to raise awareness. Ref: this comes from http://www.postgresql.org/message-id/cab7npqr1ivd5r_qn_ngmkbolqmagbosj4wnpo8eybnn6we_...@mail.gmail.com Thanks. You can assume I'm -1 on every header split proposal not driving a substantial compile duration improvement: http://www.postgresql.org/message-id/flat/20130917163056.ga327...@tornado.leadboat.com Uh, I think clarity is also a reasonable reason to split headers. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
On 2014-10-11 17:19:27 -0400, Noah Misch wrote: On Sat, Oct 11, 2014 at 10:44:39AM -0300, Alvaro Herrera wrote: Started a new thread to raise awareness. Ref: this comes from http://www.postgresql.org/message-id/cab7npqr1ivd5r_qn_ngmkbolqmagbosj4wnpo8eybnn6we_...@mail.gmail.com Thanks. You can assume I'm -1 on every header split proposal not driving a substantial compile duration improvement: I don't know. Isn't it, from a aesthetic POV, wrong to have all that stuff in builtins.h? The stuff split of really doesn't seem to belong there? I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Greetings, Andres Freund -- Andres Freund 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
Re: [HACKERS] split builtins.h to quote.h
* Bruce Momjian (br...@momjian.us) wrote: On Sat, Oct 11, 2014 at 05:19:27PM -0400, Noah Misch wrote: On Sat, Oct 11, 2014 at 10:44:39AM -0300, Alvaro Herrera wrote: Started a new thread to raise awareness. Ref: this comes from http://www.postgresql.org/message-id/cab7npqr1ivd5r_qn_ngmkbolqmagbosj4wnpo8eybnn6we_...@mail.gmail.com Thanks. You can assume I'm -1 on every header split proposal not driving a substantial compile duration improvement: http://www.postgresql.org/message-id/flat/20130917163056.ga327...@tornado.leadboat.com Uh, I think clarity is also a reasonable reason to split headers. I've only glanced at the actual patch, but I agree with Bruce. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] split builtins.h to quote.h
On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: On 2014-10-11 17:19:27 -0400, Noah Misch wrote: On Sat, Oct 11, 2014 at 10:44:39AM -0300, Alvaro Herrera wrote: Started a new thread to raise awareness. Ref: this comes from http://www.postgresql.org/message-id/cab7npqr1ivd5r_qn_ngmkbolqmagbosj4wnpo8eybnn6we_...@mail.gmail.com Thanks. You can assume I'm -1 on every header split proposal not driving a substantial compile duration improvement: I don't know. Isn't it, from a aesthetic POV, wrong to have all that stuff in builtins.h? The stuff split of really doesn't seem to belong there? Yes, the status quo is aesthetically wrong. Still, any clarity improvement from this split is vaporous. The cost of breaking module builds is real. I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] split builtins.h to quote.h
* Noah Misch (n...@leadboat.com) wrote: On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. That seems reasonable to me also- though I'd caveat it as for now and make sure to make a note of the reason it's included in the comments... Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] split builtins.h to quote.h
On Sun, Oct 12, 2014 at 7:09 AM, Stephen Frost sfr...@snowman.net wrote: * Noah Misch (n...@leadboat.com) wrote: On Sat, Oct 11, 2014 at 11:43:46PM +0200, Andres Freund wrote: I personally wouldn't object plaing a #include for the splitof file into builtin.h to address backward compat concerns. Would imo still be an improvement. Agreed. If the patch preserved compatibility by having builtins.h include quote.h, I would not object. That seems reasonable to me also- though I'd caveat it as for now and make sure to make a note of the reason it's included in the comments... This seems like a consensus. I updated the patch with the following things: - Declaration of quote.h in builtins.h, with a comment on top of builtins.h. - Removal of declaration of builtins.h where it is not necessary anymore, replaced by quote.h. I thought there would be more places, but a lot of files still depend on the cstring and format_type. Perhaps this could be as well separated, keeping only the function declarations of the type Datum foo(PG_FUNCTION_ARGS) in builtins.h... Regards, -- Michael From 02b45db20b9893f4517f96b084fb898bb0c41bfc Mon Sep 17 00:00:00 2001 From: Michael Paquier mich...@otacoo.com Date: Sat, 11 Oct 2014 22:28:05 +0900 Subject: [PATCH] Move all quote-related functions into a single header quote.h Note that this impacts as well contrib modules, and mainly external modules particularly for quote_identifier. --- contrib/dblink/dblink.c | 1 + contrib/postgres_fdw/deparse.c| 1 + contrib/postgres_fdw/postgres_fdw.c | 1 + contrib/test_decoding/test_decoding.c | 1 + contrib/worker_spi/worker_spi.c | 2 +- src/backend/catalog/namespace.c | 1 + src/backend/catalog/objectaddress.c | 1 + src/backend/commands/explain.c| 1 + src/backend/commands/extension.c | 1 + src/backend/commands/matview.c| 2 +- src/backend/commands/trigger.c| 1 + src/backend/commands/tsearchcmds.c| 1 + src/backend/parser/parse_utilcmd.c| 1 + src/backend/utils/adt/format_type.c | 1 + src/backend/utils/adt/quote.c | 110 ++ src/backend/utils/adt/regproc.c | 1 + src/backend/utils/adt/ri_triggers.c | 1 + src/backend/utils/adt/ruleutils.c | 109 + src/backend/utils/adt/varlena.c | 1 + src/backend/utils/cache/ts_cache.c| 1 + src/backend/utils/misc/guc.c | 1 + src/include/utils/builtins.h | 10 ++-- src/include/utils/quote.h | 28 + src/pl/plpgsql/src/pl_gram.y | 1 + src/pl/plpython/plpy_plpymodule.c | 1 + 25 files changed, 164 insertions(+), 116 deletions(-) create mode 100644 src/include/utils/quote.h diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c index d77b3ee..cbbc513 100644 --- a/contrib/dblink/dblink.c +++ b/contrib/dblink/dblink.c @@ -56,6 +56,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/tqual.h diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 2045774..0009cd3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -50,6 +50,7 @@ #include parser/parsetree.h #include utils/builtins.h #include utils/lsyscache.h +#include utils/quote.h #include utils/syscache.h diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 4c49776..feb41f5 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -36,6 +36,7 @@ #include utils/guc.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h PG_MODULE_MAGIC; diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c index fdbd313..b168fc3 100644 --- a/contrib/test_decoding/test_decoding.c +++ b/contrib/test_decoding/test_decoding.c @@ -25,6 +25,7 @@ #include utils/builtins.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/rel.h #include utils/relcache.h #include utils/syscache.h diff --git a/contrib/worker_spi/worker_spi.c b/contrib/worker_spi/worker_spi.c index 328c722..ec6c9fa 100644 --- a/contrib/worker_spi/worker_spi.c +++ b/contrib/worker_spi/worker_spi.c @@ -37,7 +37,7 @@ #include fmgr.h #include lib/stringinfo.h #include pgstat.h -#include utils/builtins.h +#include utils/quote.h #include utils/snapmgr.h #include tcop/utility.h diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 5eb8fd4..7f8f54b 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -53,6 +53,7 @@ #include utils/inval.h #include utils/lsyscache.h #include utils/memutils.h +#include utils/quote.h #include utils/syscache.h diff --git a/src/backend/catalog/objectaddress.c
