Re: exposing pg_controldata and pg_config as functions

From: Joe Conway <mail(at)joeconway(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Andrew Dunstan <andrew(at)dunslane(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Peter Eisentraut <peter_e(at)gmx(dot)net>
Subject: Re: exposing pg_controldata and pg_config as functions
Date: 2016-02-16 14:36:10
Message-ID: 56C333DA.3060708@joeconway.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 02/15/2016 11:20 PM, Michael Paquier wrote:
> Here are just a couple of cosmetic comments.

> Missing markup <productname></> around PostgreSQL.

Added, but I'll note that there are tons of locations in the docs where
we do not do that. Maybe they should all be made consistent.

> + Application. There is a System Information Function
> Why is using upper-case characters necessary here? This could just say
> system function.

It is capitalized because it refers to a section in the docs. I followed
it with <xref linkend="functions-info"> as well, so it ends up reading:
"There is a System Information Function (Section 9.25) called pg_config
which underlies this view."

I guess I could be convinced to lower case it, but I thought this looked
better.

> The paragraph in func.sgml is a copy-paste of the one in
> catalogs.sgml. We may want to remove the duplication.

The paragraphs are mostly copy-paste, but slightly different (toward the
end). There is both a function and a system view -- why would we not
want a description in both places?

> + /* let the caller know we're sending back a tuplestore */
> + rsinfo->returnMode = SFRM_Materialize;
> I guess one can recognize your style here for SRF functions :)

Old habits die hard ;-)

> @@ -61,7 +74,7 @@ libpgcommon_srv.a: $(OBJS_SRV)
> # a hack that might fail someday if there is a *_srv.o without a
> # corresponding *.o, but it works for now.
> %_srv.o: %.c %.o
> - $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@
> + $(CC) $(CFLAGS) $(subst -DFRONTEND ,, $(CPPFLAGS)) -c $< -o $@
> Diff noise?

No, intentional. The original version leaves a white space at the
beginning of CPPFLAGS after removing -DFRONTEND.

> --- /dev/null
> +++ b/src/common/config_info.c
> [...]
> + * IDENTIFICATION
> + * src/common/controldata_utils.c
> This is incorrect.

Right -- found and fixed several of these with Alvaro's help after
posting. Also fixed Copyright dates (s/2015/2016/) on the new files.

> + * IDENTIFICATION
> + * src/backend/utils/misc/pg_config.c
> + *
> + */
> I am nitpicking here but this header block should have a long
> "----------------" at its bottom.

Fair enough -- fixed.

Thanks!

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Attachment Content-Type Size
pg_config-2016.02.16.01.diff text/x-diff 32.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2016-02-16 14:38:20 Re: pglogical - logical replication contrib module
Previous Message Fabien COELHO 2016-02-16 14:12:18 Re: extend pgbench expressions with functions