Re: exposing pg_controldata and pg_config as functions

On Sat, Feb 20, 2016 at 12:12 PM, Joe Conway <mail@joeconway.com> wrote:
> On 01/17/2016 04:10 PM, Joe Conway wrote:
>> On 01/16/2016 06:02 AM, Michael Paquier wrote:
>>> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
>>>> 3) Adds new functions, more or less in line with previous discussions:
>>>>    * pg_checkpoint_state()
>>>>    * pg_controldata_state()
>>>>    * pg_recovery_state()
>>>>    * pg_init_state()
>>>
>>> Taking the opposite direction of Josh upthread, why is this split
>>> actually necessary? Isn't the idea to provide a SQL interface of what
>>> pg_controldata shows? If this split proves to be useful, shouldn't we
>>> do it as well for pg_controldata?
>>
>> The last discussion moved strongly in the direction of separate
>> functions, and that being different from pg_controldata was not a bad
>> thing. That said, I'm still of the opinion that there are legitimate
>> reasons to want the command line pg_controldata and the SQL functions to
>> produce equivalent, if not identical, results. I just wish we could get
>> a clear consensus one way or the other.
>
> I've assumed that we are sticking with the separate functions. As such,
> here is a rebased patch, with documentation and other fixes such as
> Copyright year, Mkvcbuild support, and some cruft removal.

Looking again at this thread I guess that this is consensus, based on
the proposal from Josh and seeing no other ideas around. Another idea
would be to group all the fields that into a single function
pg_control_data().

> Is there general consensus that we want this feature, and that we want
> it in this form? Any other comments?

I had a look at this patch.

+   <indexterm>
+    <primary>pg_checkpoint_state</primary>
+   </indexterm>
+   <para>
+    <function>pg_checkpoint_state</> returns a record containing
+    checkpoint_location, prior_location, redo_location, redo_wal_file,
+    timeline_id, prev_timeline_id, full_page_writes, next_xid, next_oid,
+    next_multixact_id, next_multi_offset, oldest_xid, oldest_xid_dbid,
+    oldest_active_xid, oldest_multi_xid, oldest_multi_dbid,
+    oldest_commit_ts_xid, newest_commit_ts_xid, and checkpoint_time.
+   </para>
This is bit unreadable. The only entry in the documentation that
adopts a similar style is pg_stat_file, and with six fields that feels
as being enough. I would suggest using a table instead with the type
of the field and its name.

Regarding the naming of the functions, I think that it would be good
to get something consistent with the concept of those being "Control
Data functions" by having them share the same prefix, say pg_control_
- pg_control_checkpoint
- pg_control_init
- pg_control_system
- pg_control_recovery

+       snprintf (controldata_name, CONTROLDATANAME_LEN,
+                 "%s:", controldata[i].name);
Nitpick: extra space.

+static const char *const controldata_names[] =
+{
+   gettext_noop("pg_control version number"),
+   gettext_noop("Catalog version number"),
+   gettext_noop("Database system identifier"),
Is this complication really necessary? Those identifiers are used only
in the frontend and the footprint of this patch on pg_controldata is
really large. What I think we should do is have in src/common the
following set of routines that work directly on ControlFileData:
- checkControlFile, to perform basic sanity checks on the control file
(CRC, see for example pg_rewind.c)
- getControlFile(dataDir), that simply returns a palloc'd
ControlFileData to the caller after looking at global/pg_control.
pg_rewind could perhaps make use of the one to check the control file
CRC, to fetch ControlFileData there is some parallel logic for the
source server if it is either remote or local so it would be better to
not use getControlFile in this case.
-- 
Michael