Re: split builtins.h to quote.h

Started a new thread to raise awareness.

Michael Paquier wrote:
> On Thu, Oct 9, 2014 at 8:29 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Thu, Oct 9, 2014 at 6:26 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

If someone wants to object to this, please speak quickly.

Ref: this comes from
http://www.postgresql.org/message-id/CAB7nPqR1iVd5r_QN_ngmkBOLQmAGBOsJ4WNPo8eybNn6WE_Kdw@mail.gmail.com

> From b14000662a52c3f5b40cf43a1f6699e0d024d1fd Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael(at)otacoo(dot)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 | 1 +
> 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 | 1 +
> 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 | 6 --
> src/include/utils/quote.h | 28 +++++++++
> src/pl/plpgsql/src/pl_gram.y | 1 +
> src/pl/plpython/plpy_plpymodule.c | 1 +
> 25 files changed, 160 insertions(+), 114 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..7ada98d 100644
> --- a/contrib/worker_spi/worker_spi.c
> +++ b/contrib/worker_spi/worker_spi.c
> @@ -38,6 +38,7 @@
> #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"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/syscache.h"
> #include "utils/tqual.h"
>
> diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
> index 49963ff..f27ad15 100644
> --- a/src/backend/commands/explain.c
> +++ b/src/backend/commands/explain.c
> @@ -27,6 +27,7 @@
> #include "utils/builtins.h"
> #include "utils/json.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/ruleutils.h"
> #include "utils/snapmgr.h"
> diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
> index 9a0afa4..0793509 100644
> --- a/src/backend/commands/extension.c
> +++ b/src/backend/commands/extension.c
> @@ -51,6 +51,7 @@
> #include "utils/builtins.h"
> #include "utils/fmgroids.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> #include "utils/tqual.h"
> diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
> index d1c8bb0..f6f3a04 100644
> --- a/src/backend/commands/matview.c
> +++ b/src/backend/commands/matview.c
> @@ -35,6 +35,7 @@
> #include "tcop/tcopprot.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> #include "utils/syscache.h"
> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index f4c0ffa..df62484 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c
> @@ -52,6 +52,7 @@
> #include "utils/inval.h"
> #include "utils/lsyscache.h"
> #include "utils/memutils.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> #include "utils/syscache.h"
> diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
> index 5d8a001..40758cc 100644
> --- a/src/backend/commands/tsearchcmds.c
> +++ b/src/backend/commands/tsearchcmds.c
> @@ -42,6 +42,7 @@
> #include "utils/builtins.h"
> #include "utils/fmgroids.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/syscache.h"
> #include "utils/tqual.h"
> diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
> index 7c1939f..a991872 100644
> --- a/src/backend/parser/parse_utilcmd.c
> +++ b/src/backend/parser/parse_utilcmd.c
> @@ -57,6 +57,7 @@
> #include "utils/acl.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/syscache.h"
> #include "utils/typcache.h"
> diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
> index e1763a3..02733c0 100644
> --- a/src/backend/utils/adt/format_type.c
> +++ b/src/backend/utils/adt/format_type.c
> @@ -22,6 +22,7 @@
> #include "catalog/pg_type.h"
> #include "utils/builtins.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/numeric.h"
> #include "utils/syscache.h"
> #include "mb/pg_wchar.h"
> diff --git a/src/backend/utils/adt/quote.c b/src/backend/utils/adt/quote.c
> index d1336b4..6fd983e 100644
> --- a/src/backend/utils/adt/quote.c
> +++ b/src/backend/utils/adt/quote.c
> @@ -13,8 +13,13 @@
> */
> #include "postgres.h"
>
> +#include "lib/stringinfo.h"
> +#include "parser/keywords.h"
> #include "utils/builtins.h"
> +#include "utils/quote.h"
>
> +/* GUC parameters */
> +bool quote_all_identifiers = false;
>
> /*
> * quote_ident -
> @@ -95,6 +100,111 @@ quote_literal(PG_FUNCTION_ARGS)
> }
>
> /*
> + * quote_identifier - Quote an identifier only if needed
> + *
> + * When quotes are needed, we palloc the required space; slightly
> + * space-wasteful but well worth it for notational simplicity.
> + */
> +const char *
> +quote_identifier(const char *ident)
> +{
> + /*
> + * Can avoid quoting if ident starts with a lowercase letter or underscore
> + * and contains only lowercase letters, digits, and underscores, *and* is
> + * not any SQL keyword. Otherwise, supply quotes.
> + */
> + int nquotes = 0;
> + bool safe;
> + const char *ptr;
> + char *result;
> + char *optr;
> +
> + /*
> + * would like to use <ctype.h> macros here, but they might yield unwanted
> + * locale-specific results...
> + */
> + safe = ((ident[0] >= 'a' && ident[0] <= 'z') || ident[0] == '_');
> +
> + for (ptr = ident; *ptr; ptr++)
> + {
> + char ch = *ptr;
> +
> + if ((ch >= 'a' && ch <= 'z') ||
> + (ch >= '0' && ch <= '9') ||
> + (ch == '_'))
> + {
> + /* okay */
> + }
> + else
> + {
> + safe = false;
> + if (ch == '"')
> + nquotes++;
> + }
> + }
> +
> + if (quote_all_identifiers)
> + safe = false;
> +
> + if (safe)
> + {
> + /*
> + * Check for keyword. We quote keywords except for unreserved ones.
> + * (In some cases we could avoid quoting a col_name or type_func_name
> + * keyword, but it seems much harder than it's worth to tell that.)
> + *
> + * Note: ScanKeywordLookup() does case-insensitive comparison, but
> + * that's fine, since we already know we have all-lower-case.
> + */
> + const ScanKeyword *keyword = ScanKeywordLookup(ident,
> + ScanKeywords,
> + NumScanKeywords);
> +
> + if (keyword != NULL && keyword->category != UNRESERVED_KEYWORD)
> + safe = false;
> + }
> +
> + if (safe)
> + return ident; /* no change needed */
> +
> + result = (char *) palloc(strlen(ident) + nquotes + 2 + 1);
> +
> + optr = result;
> + *optr++ = '"';
> + for (ptr = ident; *ptr; ptr++)
> + {
> + char ch = *ptr;
> +
> + if (ch == '"')
> + *optr++ = '"';
> + *optr++ = ch;
> + }
> + *optr++ = '"';
> + *optr = '\0';
> +
> + return result;
> +}
> +
> +/*
> + * quote_qualified_identifier - Quote a possibly-qualified identifier
> + *
> + * Return a name of the form qualifier.ident, or just ident if qualifier
> + * is NULL, quoting each component if necessary. The result is palloc'd.
> + */
> +char *
> +quote_qualified_identifier(const char *qualifier,
> + const char *ident)
> +{
> + StringInfoData buf;
> +
> + initStringInfo(&buf);
> + if (qualifier)
> + appendStringInfo(&buf, "%s.", quote_identifier(qualifier));
> + appendStringInfoString(&buf, quote_identifier(ident));
> + return buf.data;
> +}
> +
> +/*
> * quote_literal_cstr -
> * returns a properly quoted literal
> */
> diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
> index c0314ee..3760bbc 100644
> --- a/src/backend/utils/adt/regproc.c
> +++ b/src/backend/utils/adt/regproc.c
> @@ -38,6 +38,7 @@
> #include "utils/builtins.h"
> #include "utils/fmgroids.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/syscache.h"
> #include "utils/tqual.h"
>
> diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
> index c0156fa..c954b60 100644
> --- a/src/backend/utils/adt/ri_triggers.c
> +++ b/src/backend/utils/adt/ri_triggers.c
> @@ -49,6 +49,7 @@
> #include "utils/inval.h"
> #include "utils/lsyscache.h"
> #include "utils/memutils.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/snapmgr.h"
> #include "utils/syscache.h"
> diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
> index 24ade6c..6521db9 100644
> --- a/src/backend/utils/adt/ruleutils.c
> +++ b/src/backend/utils/adt/ruleutils.c
> @@ -54,6 +54,7 @@
> #include "utils/builtins.h"
> #include "utils/fmgroids.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/rel.h"
> #include "utils/ruleutils.h"
> #include "utils/snapmgr.h"
> @@ -273,9 +274,6 @@ static const char *query_getrulebyoid = "SELECT * FROM pg_catalog.pg_rewrite WHE
> static SPIPlanPtr plan_getviewrule = NULL;
> static const char *query_getviewrule = "SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND rulename = $2";
>
> -/* GUC parameters */
> -bool quote_all_identifiers = false;
> -
>
> /* ----------
> * Local functions
> @@ -8887,111 +8885,6 @@ printSubscripts(ArrayRef *aref, deparse_context *context)
> }
>
> /*
> - * quote_identifier - Quote an identifier only if needed
> - *
> - * When quotes are needed, we palloc the required space; slightly
> - * space-wasteful but well worth it for notational simplicity.
> - */
> -const char *
> -quote_identifier(const char *ident)
> -{
> - /*
> - * Can avoid quoting if ident starts with a lowercase letter or underscore
> - * and contains only lowercase letters, digits, and underscores, *and* is
> - * not any SQL keyword. Otherwise, supply quotes.
> - */
> - int nquotes = 0;
> - bool safe;
> - const char *ptr;
> - char *result;
> - char *optr;
> -
> - /*
> - * would like to use <ctype.h> macros here, but they might yield unwanted
> - * locale-specific results...
> - */
> - safe = ((ident[0] >= 'a' && ident[0] <= 'z') || ident[0] == '_');
> -
> - for (ptr = ident; *ptr; ptr++)
> - {
> - char ch = *ptr;
> -
> - if ((ch >= 'a' && ch <= 'z') ||
> - (ch >= '0' && ch <= '9') ||
> - (ch == '_'))
> - {
> - /* okay */
> - }
> - else
> - {
> - safe = false;
> - if (ch == '"')
> - nquotes++;
> - }
> - }
> -
> - if (quote_all_identifiers)
> - safe = false;
> -
> - if (safe)
> - {
> - /*
> - * Check for keyword. We quote keywords except for unreserved ones.
> - * (In some cases we could avoid quoting a col_name or type_func_name
> - * keyword, but it seems much harder than it's worth to tell that.)
> - *
> - * Note: ScanKeywordLookup() does case-insensitive comparison, but
> - * that's fine, since we already know we have all-lower-case.
> - */
> - const ScanKeyword *keyword = ScanKeywordLookup(ident,
> - ScanKeywords,
> - NumScanKeywords);
> -
> - if (keyword != NULL && keyword->category != UNRESERVED_KEYWORD)
> - safe = false;
> - }
> -
> - if (safe)
> - return ident; /* no change needed */
> -
> - result = (char *) palloc(strlen(ident) + nquotes + 2 + 1);
> -
> - optr = result;
> - *optr++ = '"';
> - for (ptr = ident; *ptr; ptr++)
> - {
> - char ch = *ptr;
> -
> - if (ch == '"')
> - *optr++ = '"';
> - *optr++ = ch;
> - }
> - *optr++ = '"';
> - *optr = '\0';
> -
> - return result;
> -}
> -
> -/*
> - * quote_qualified_identifier - Quote a possibly-qualified identifier
> - *
> - * Return a name of the form qualifier.ident, or just ident if qualifier
> - * is NULL, quoting each component if necessary. The result is palloc'd.
> - */
> -char *
> -quote_qualified_identifier(const char *qualifier,
> - const char *ident)
> -{
> - StringInfoData buf;
> -
> - initStringInfo(&buf);
> - if (qualifier)
> - appendStringInfo(&buf, "%s.", quote_identifier(qualifier));
> - appendStringInfoString(&buf, quote_identifier(ident));
> - return buf.data;
> -}
> -
> -/*
> * get_relation_name
> * Get the unqualified name of a relation specified by OID
> *
> diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
> index c3171b5..eb4c507 100644
> --- a/src/backend/utils/adt/varlena.c
> +++ b/src/backend/utils/adt/varlena.c
> @@ -28,6 +28,7 @@
> #include "utils/builtins.h"
> #include "utils/bytea.h"
> #include "utils/lsyscache.h"
> +#include "utils/quote.h"
> #include "utils/memutils.h"
> #include "utils/pg_locale.h"
> #include "utils/sortsupport.h"
> diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
> index 5ff1461..c6d539f 100644
> --- a/src/backend/utils/cache/ts_cache.c
> +++ b/src/backend/utils/cache/ts_cache.c
> @@ -45,6 +45,7 @@
> #include "utils/inval.h"
> #include "utils/lsyscache.h"
> #include "utils/memutils.h"
> +#include "utils/quote.h"
> #include "utils/syscache.h"
> #include "utils/tqual.h"
>
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 8111b93..3253235 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -79,6 +79,7 @@
> #include "utils/plancache.h"
> #include "utils/portal.h"
> #include "utils/ps_status.h"
> +#include "utils/quote.h"
> #include "utils/snapmgr.h"
> #include "utils/tzparser.h"
> #include "utils/xml.h"
> diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
> index fb1b4a4..c2df489 100644
> --- a/src/include/utils/builtins.h
> +++ b/src/include/utils/builtins.h
> @@ -666,7 +666,6 @@ extern Datum record_image_ge(PG_FUNCTION_ARGS);
> extern Datum btrecordimagecmp(PG_FUNCTION_ARGS);
>
> /* ruleutils.c */
> -extern bool quote_all_identifiers;
> extern Datum pg_get_ruledef(PG_FUNCTION_ARGS);
> extern Datum pg_get_ruledef_ext(PG_FUNCTION_ARGS);
> extern Datum pg_get_viewdef(PG_FUNCTION_ARGS);
> @@ -689,10 +688,6 @@ extern Datum pg_get_function_arguments(PG_FUNCTION_ARGS);
> extern Datum pg_get_function_identity_arguments(PG_FUNCTION_ARGS);
> extern Datum pg_get_function_result(PG_FUNCTION_ARGS);
> extern Datum pg_get_function_arg_default(PG_FUNCTION_ARGS);
> -extern const char *quote_identifier(const char *ident);
> -extern char *quote_qualified_identifier(const char *qualifier,
> - const char *ident);
> -
>
> /* tid.c */
> extern Datum tidin(PG_FUNCTION_ARGS);
> @@ -1072,7 +1067,6 @@ extern int32 type_maximum_size(Oid type_oid, int32 typemod);
> /* quote.c */
> extern Datum quote_ident(PG_FUNCTION_ARGS);
> extern Datum quote_literal(PG_FUNCTION_ARGS);
> -extern char *quote_literal_cstr(const char *rawstr);
> extern Datum quote_nullable(PG_FUNCTION_ARGS);
>
> /* guc.c */
> diff --git a/src/include/utils/quote.h b/src/include/utils/quote.h
> new file mode 100644
> index 0000000..ccf0d83
> --- /dev/null
> +++ b/src/include/utils/quote.h
> @@ -0,0 +1,28 @@
> +/*-------------------------------------------------------------------------
> + *
> + * quote.h
> + * Declarations for operations related to string quoting.
> + *
> + *
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + * src/include/utils/quote.h
> + *
> + *-------------------------------------------------------------------------
> + */
> +#ifndef QUOTE_H
> +#define QUOTE_H
> +
> +#include "fmgr.h"
> +
> +/* GUC parameter */
> +extern bool quote_all_identifiers;
> +
> +/* Functions used for quoting */
> +extern const char *quote_identifier(const char *ident);
> +extern char *quote_qualified_identifier(const char *qualifier,
> + const char *ident);
> +extern char *quote_literal_cstr(const char *rawstr);
> +
> +#endif /* QUOTE_H */
> diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
> index 893f3a4..8e9d0a0 100644
> --- a/src/pl/plpgsql/src/pl_gram.y
> +++ b/src/pl/plpgsql/src/pl_gram.y
> @@ -22,6 +22,7 @@
> #include "parser/scanner.h"
> #include "parser/scansup.h"
> #include "utils/builtins.h"
> +#include "utils/quote.h"
>
>
> /* Location tracking support --- simpler than bison's default */
> diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
> index 37ea2a4..39e70f8 100644
> --- a/src/pl/plpython/plpy_plpymodule.c
> +++ b/src/pl/plpython/plpy_plpymodule.c
> @@ -8,6 +8,7 @@
>
> #include "mb/pg_wchar.h"
> #include "utils/builtins.h"
> +#include "utils/quote.h"
>
> #include "plpython.h"
>
> --
> 2.1.2
>

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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_Kdw@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(dot)GA327792(at)tornado(dot)leadboat(dot)com

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_Kdw@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(dot)GA327792(at)tornado(dot)leadboat(dot)com

Uh, I think clarity is also a reasonable reason to split headers.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

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_Kdw@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

* Bruce Momjian (bruce(at)momjian(dot)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_Kdw@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(dot)GA327792(at)tornado(dot)leadboat(dot)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

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_Kdw@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

* Noah Misch (noah(at)leadboat(dot)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

On Sun, Oct 12, 2014 at 7:09 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Noah Misch (noah(at)leadboat(dot)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

On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Noah Misch (noah(at)leadboat(dot)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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Noah Misch (noah(at)leadboat(dot)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

On Mon, Oct 13, 2014 at 10:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Oct 11, 2014 at 6:09 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> * Noah Misch (noah(at)leadboat(dot)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

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Mon, Oct 13, 2014 at 10:04 PM, Robert Haas <robertmhaas(at)gmail(dot)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

I wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)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

On Mon, Oct 13, 2014 at 9:24 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)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

On Mon, Oct 13, 2014 at 11:14 PM, Robert Haas <robertmhaas(at)gmail(dot)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

-----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
> <robertmhaas(at)gmail(dot)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-----

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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-+6yBQQ@mail.gmail.com
--
Michael

Michael Paquier wrote:
> On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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-+6yBQQ@mail.gmail.com

Did you miss this one?
http://www.postgresql.org/message-id/31728.1413209460@sss.pgh.pa.us

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 7, 2014 at 3:55 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier wrote:
>> On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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-+6yBQQ@mail.gmail.com
>
> Did you miss this one?
> http://www.postgresql.org/message-id/31728.1413209460@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

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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Nov 7, 2014 at 4:42 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

On Sat, Nov 8, 2014 at 5:55 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier wrote:
>> On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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-+6yBQQ@mail.gmail.com
>
> Did you miss this one?
> http://www.postgresql.org/message-id/31728.1413209460@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

On 11/08/2014 12:37 AM, Michael Paquier wrote:
> On Sat, Nov 8, 2014 at 5:55 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>> Michael Paquier wrote:
>>> On Fri, Nov 7, 2014 at 2:31 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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-+6yBQQ@mail.gmail.com
>> Did you miss this one?
>> http://www.postgresql.org/message-id/31728.1413209460@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

Andrew Dunstan <andrew(at)dunslane(dot)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

On Sun, Dec 14, 2014 at 1:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)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