Re: exposing pg_controldata and pg_config as functions

On 02/28/2016 05:16 AM, Michael Paquier wrote:
> +        Returns information about current controldata file state.
> s/controldata/control data?
>
> +    <tgroup cols="2">
> +     <thead>
> +      <row>
> +       <entry>Column Name</entry>
> +       <entry>Data Type</entry>
> +      </row>
> +     </thead>
> +
> Having a description of each field would be nice.

Might be nice, but the pg_controldata section of the docs does not have
any description either, and I don't feel compelled to improve on that
just for the sake of this patch. I'll put it on my TODO to improve both
at some point though.

> + * pg_controldata.c
> + *     Expose select pg_controldata output, except via SQL functions
> I am not much getting the meaning of this sentence. What about the following:
> "Set of routines exposing the contents of the control data file in a
> set of SQL functions".

Ok, reworded to something similar.

> +   /* Populate the values and null arrays */
> +   values[0] = LSNGetDatum(ControlFile->checkPoint);
> +   nulls[0] = false;
> +
> +   values[1] = LSNGetDatum(ControlFile->prevCheckPoint);
> +   nulls[1] = false;
> Instead of setting each flag of nulls[] one by one, just calling once
> MemSet would be fine. Any method is fine though.

I prefer the current style and I believe it is more consistent
with what is done elsewhere. In any case this is not exactly a
performance critical path.

> +   /* get a copy of the control file */
> +   ControlFile = get_controlfile(DataDir, progname);
> Some whitespaces here. git diff is showing in red here.

fixed

> +   if (ControlFile->pg_control_version % 65536 == 0 &&
> +       ControlFile->pg_control_version / 65536 != 0)
> +       elog(ERROR, _("byte ordering mismatch"));
> You may want to put this check directly in get_controlfile(). it is
> repeated 4 times in the backend, and has an equivalent in
> pg_controldata.

makes sense - done


>     our @pgcommonallfiles = qw(
> -     config_info.c exec.c pg_lzcompress.c pgfnames.c psprintf.c
> +     config_info.c controldata_utils.c exec.c pg_lzcompress.c pgfnames.c
>       relpath.c rmtree.c string.c username.c wait_error.c);
> "psprintf.c" has been removed from this list. This causes the build
> with MSVC to fail.

good catch -- fixed

If there are no other complaints or comments, I will commit the attached
sometime this coming week (the the requisite catversion bump).

Thanks!

Joe

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