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>, Josh Berkus <josh(dot)berkus(at)pgexperts(dot)com> |
Subject: | Re: exposing pg_controldata and pg_config as functions |
Date: | 2016-02-28 18:50:51 |
Message-ID: | 56D3418B.7010106@joeconway.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
pg_controldata_funcs-2016.02.28.00.diff | text/x-diff | 46.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2016-02-28 20:03:28 | WIP: Upper planner pathification |
Previous Message | Simon Riggs | 2016-02-28 18:21:57 | Re: The plan for FDW-based sharding |