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