GUC for data checksums

Started by Bernd Helmleover 12 years ago6 messages
#1Bernd Helmle
mailings@oopsware.de
1 attachment(s)

Attached is a small patch to add a new GUC to report wether data checksums
for a particular cluster are enabled. The only way to get this info afaik
is to look into pg_control and the version number used, but i'd welcome a
way to access this remotely, too. If there aren't any objections i'll add
this to the CF.

--
Thanks

Bernd

Attachments:

data_checksums_guc.patchapplication/octet-stream; name=data_checksums_guc.patchDownload
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
new file mode 100644
index 23ebc11..5a1d9ac
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** dynamic_library_path = 'C:\tools\postgre
*** 6169,6174 ****
--- 6169,6188 ----
        </listitem>
       </varlistentry>
  
+      <varlistentry id="guc-data-checksums" xreflabel="data_checksums">
+       <term><varname>data_checksums</varname> (<type>boolean</type>)</term>
+       <indexterm>
+        <primary><varname>data_checksums</> configuration parameter</primary>
+       </indexterm>
+       <listitem>
+        <para>
+         Reports wether data checksums are turned on or off for this
+         particular cluster. See <xref linkend="app-initdb-data-checksums"> for more
+         information.
+        </para>
+       </listitem>
+      </varlistentry>
+ 
       <varlistentry id="guc-integer-datetimes" xreflabel="integer_datetimes">
        <term><varname>integer_datetimes</varname> (<type>boolean</type>)</term>
        <indexterm>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 7d297bc..f2fccc4
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static bool check_application_name(char 
*** 199,204 ****
--- 199,205 ----
  static void assign_application_name(const char *newval, void *extra);
  static const char *show_unix_socket_permissions(void);
  static const char *show_log_file_mode(void);
+ static const char *show_data_checksums(void);
  
  static char *config_enum_get_options(struct config_enum * record,
  						const char *prefix, const char *suffix,
*************** static int	max_identifier_length;
*** 466,471 ****
--- 467,473 ----
  static int	block_size;
  static int	segment_size;
  static int	wal_block_size;
+ static bool data_checksums;
  static int	wal_segment_size;
  static bool integer_datetimes;
  static int	effective_io_concurrency;
*************** static struct config_bool ConfigureNames
*** 1457,1462 ****
--- 1459,1476 ----
  		NULL, NULL, NULL
  	},
  
+ 	{
+ 		{"data_checksums", PGC_INTERNAL, PRESET_OPTIONS,
+ 		 gettext_noop("Shows wether data checksums are turned on for this cluster"),
+ 		 NULL,
+ 		 GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ 		},
+ 		&data_checksums,
+ 		false,
+ 		NULL, NULL, show_data_checksums
+ 	},
+ 
+ 
  	/* End-of-list marker */
  	{
  		{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
*************** assign_tcp_keepalives_idle(int newval, v
*** 8656,8661 ****
--- 8670,8684 ----
  }
  
  static const char *
+ show_data_checksums(void)
+ {
+ 	if (DataChecksumsEnabled())
+ 		return "on";
+ 	else
+ 		return "off";
+ }
+ 
+ static const char *
  show_tcp_keepalives_idle(void)
  {
  	/* See comments in assign_tcp_keepalives_idle */
#2Andres Freund
andres@2ndquadrant.com
In reply to: Bernd Helmle (#1)
Re: GUC for data checksums

Hi,

On 2013-09-14 18:33:38 +0200, Bernd Helmle wrote:

Attached is a small patch to add a new GUC to report wether data checksums
for a particular cluster are enabled. The only way to get this info afaik is
to look into pg_control and the version number used, but i'd welcome a way
to access this remotely, too. If there aren't any objections i'll add this
to the CF.

Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Bernd Helmle
mailings@oopsware.de
In reply to: Andres Freund (#2)
Re: GUC for data checksums

--On 15. September 2013 00:25:34 +0200 Andres Freund
<andres@2ndquadrant.com> wrote:

Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?

It's along the line with the other informational variables like block_size
et al. Do you want to have a function instead or what's your intention?

One benefit is to have 'em all in SHOW ALL which can be used to compare
database/cluster settings, to mention one use case i have in mind.

--
Thanks

Bernd

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Andres Freund
andres@2ndquadrant.com
In reply to: Bernd Helmle (#3)
Re: GUC for data checksums

On 2013-09-15 03:34:53 +0200, Bernd Helmle wrote:

--On 15. September 2013 00:25:34 +0200 Andres Freund
<andres@2ndquadrant.com> wrote:

Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?

It's along the line with the other informational variables like block_size
et al. Do you want to have a function instead or what's your intention?

Well, you've added a "data_checksums" variable that won't ever get used,
right? You can't set the variable and the show hook doesn't actually use
it.
The reason you presumably did so is that there is no plain variable that
contains information about data checksums, we first need to read the
control file to know whether it's enabled and GUCs are initialized way
earlier than that.

A quick look unfortunately shows that there's no support for GUCs
without an actual underlying variable, so unless somebody adds that,
there doesn't seem to be much choice.

I think a comment documenting that the data_checksums variable is not
actually used would be appropriate.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Andres Freund (#4)
Re: GUC for data checksums

On 15.09.2013 17:05, Andres Freund wrote:

On 2013-09-15 03:34:53 +0200, Bernd Helmle wrote:

--On 15. September 2013 00:25:34 +0200 Andres Freund
<andres@2ndquadrant.com> wrote:

Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?

It's along the line with the other informational variables like block_size
et al. Do you want to have a function instead or what's your intention?

Well, you've added a "data_checksums" variable that won't ever get used,
right? You can't set the variable and the show hook doesn't actually use
it.
The reason you presumably did so is that there is no plain variable that
contains information about data checksums, we first need to read the
control file to know whether it's enabled and GUCs are initialized way
earlier than that.

A quick look unfortunately shows that there's no support for GUCs
without an actual underlying variable, so unless somebody adds that,
there doesn't seem to be much choice.

I think a comment documenting that the data_checksums variable is not
actually used would be appropriate.

Surprisingly we don't have any other gucs that would be set at initdb
time, and not changed after that. But we used to have two, lc_collate
and lc_ctype, until we changed them to be per-database settings. We used
to do this in ReadControlFile:

/* Make the fixed locale settings visible as GUC variables, too */
SetConfigOption("lc_collate", ControlFile->lc_collate,
PGC_INTERNAL, PGC_S_OVERRIDE);
SetConfigOption("lc_ctype", ControlFile->lc_ctype,
PGC_INTERNAL, PGC_S_OVERRIDE);

I did the same for data_checksums, and committed. Thanks for the patch.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Andres Freund
andres@2ndquadrant.com
In reply to: Heikki Linnakangas (#5)
Re: GUC for data checksums

On 2013-09-16 14:43:27 +0300, Heikki Linnakangas wrote:

On 15.09.2013 17:05, Andres Freund wrote:

On 2013-09-15 03:34:53 +0200, Bernd Helmle wrote:

--On 15. September 2013 00:25:34 +0200 Andres Freund
<andres@2ndquadrant.com> wrote:

Looks like a good idea to me. The implementation looks sane as well,
except that I am not sure if we really need to introduce that faux
variable. If the variable cannot be set and we have a SHOW hook, do we
need it?

It's along the line with the other informational variables like block_size
et al. Do you want to have a function instead or what's your intention?

Well, you've added a "data_checksums" variable that won't ever get used,
right? You can't set the variable and the show hook doesn't actually use
it.
The reason you presumably did so is that there is no plain variable that
contains information about data checksums, we first need to read the
control file to know whether it's enabled and GUCs are initialized way
earlier than that.

A quick look unfortunately shows that there's no support for GUCs
without an actual underlying variable, so unless somebody adds that,
there doesn't seem to be much choice.

I think a comment documenting that the data_checksums variable is not
actually used would be appropriate.

Surprisingly we don't have any other gucs that would be set at initdb time,
and not changed after that. But we used to have two, lc_collate and
lc_ctype, until we changed them to be per-database settings. We used to do
this in ReadControlFile:

/* Make the fixed locale settings visible as GUC variables, too */
SetConfigOption("lc_collate", ControlFile->lc_collate,
PGC_INTERNAL, PGC_S_OVERRIDE);
SetConfigOption("lc_ctype", ControlFile->lc_ctype,
PGC_INTERNAL, PGC_S_OVERRIDE);

I did the same for data_checksums, and committed. Thanks for the patch.

I don't think it's fatal - as you say we've done so before - but note
that both the committed and Bernd's version don't report the correct
value on postgres -C data_checksums because we haven't read the control
file yet...
Maybe we should do that earlier?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers