Re: exposing pg_controldata and pg_config as functions

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