On 02/21/2016 05:30 AM, Michael Paquier wrote:
> 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().
I think a single function would be ridiculously wide. I like the four
separate functions better if we're going to do it this way at all.
> + <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.
Ok, changed to your suggestion.
> 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
No issues -- changed.
> + snprintf (controldata_name, CONTROLDATANAME_LEN,
> + "%s:", controldata[i].name);
> Nitpick: extra space.
I didn't understand this comment but it is moot now anyway...
> +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.
I agree with the assessment that much of what had been moved based on
the original pg_controladata() SRF no longer needs to move. This version
only puts get_controlfile() into src/common, since that is the bit that
is still shared. If checkControlFile() or something similar is useful
for pg_rewind or some other extension, I'd say that should be a separate
patch.
Oh, and the entire thing is now rebased against a git pull from a few
hours ago. I moved this to the upcoming commitfest too, although I think
it is pretty well ready to go.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development