Code cleanup patch submission for extended_stats.c

Started by Mark Dilgerabout 8 years ago5 messages

Mark Dilger

hornschnorter@gmail.com

about 8 years ago

1 attachment(s)

Hackers, Alvaro,

In src/backend/statistics/extended_stats.c, in statext_store, there is a section:

Datum values[Natts_pg_statistic_ext];
bool nulls[Natts_pg_statistic_ext];
bool replaces[Natts_pg_statistic_ext];

memset(nulls, 1, Natts_pg_statistic_ext * sizeof(bool));
memset(replaces, 0, Natts_pg_statistic_ext * sizeof(bool));
memset(values, 0, Natts_pg_statistic_ext * sizeof(Datum));

It looks to me like Alvaro introduced this in the original version of the file which
was created in commit 7b504eb282ca2f5104b5c00b4f05a3ef6bb1385b. Grep'ing
through the code base, it seems the following would be more consistent with
how these initializations are handled elsewhere:

Datum values[Natts_pg_statistic_ext];
bool nulls[Natts_pg_statistic_ext];
bool replaces[Natts_pg_statistic_ext];

memset(nulls, 1, sizeof(nulls));
memset(replaces, 0, sizeof(replaces));
memset(values, 0, sizeof(values));

Patch attached as 0001_extended_stats_sizeof.patch.1

mark

Attachments:

0001_extended_stats_sizeof.patch.1application/octet-stream; name=0001_extended_stats_sizeof.patch.1Download

diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index db4987bde3..509297c9eb 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -300,9 +300,9 @@ statext_store(Relation pg_stext, Oid statOid,
 	bool		nulls[Natts_pg_statistic_ext];
 	bool		replaces[Natts_pg_statistic_ext];
 
-	memset(nulls, 1, Natts_pg_statistic_ext * sizeof(bool));
-	memset(replaces, 0, Natts_pg_statistic_ext * sizeof(bool));
-	memset(values, 0, Natts_pg_statistic_ext * sizeof(Datum));
+	memset(nulls, 1, sizeof(nulls));
+	memset(replaces, 0, sizeof(replaces));
+	memset(values, 0, sizeof(values));
 
 	/*
 	 * Construct a new pg_statistic_ext tuple, replacing the calculated stats.

Tom Lane

tgl@sss.pgh.pa.us

about 8 years ago

In reply to: Mark Dilger (#1)

Re: Code cleanup patch submission for extended_stats.c

Mark Dilger <hornschnorter@gmail.com> writes:

It looks to me like Alvaro introduced this in the original version of the file which
was created in commit 7b504eb282ca2f5104b5c00b4f05a3ef6bb1385b. Grep'ing
through the code base, it seems the following would be more consistent with
how these initializations are handled elsewhere:

memset(nulls, 1, sizeof(nulls));
memset(replaces, 0, sizeof(replaces));
memset(values, 0, sizeof(values));

+1. I'd be inclined to use "false" and "true" for the init values of
the boolean arrays, too.

regards, tom lane

Mark Dilger

hornschnorter@gmail.com

about 8 years ago

In reply to: Tom Lane (#2)

1 attachment(s)

Re: Code cleanup patch submission for extended_stats.c

On Nov 25, 2017, at 2:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mark Dilger <hornschnorter@gmail.com> writes:

It looks to me like Alvaro introduced this in the original version of the file which
was created in commit 7b504eb282ca2f5104b5c00b4f05a3ef6bb1385b. Grep'ing
through the code base, it seems the following would be more consistent with
how these initializations are handled elsewhere:

memset(nulls, 1, sizeof(nulls));
memset(replaces, 0, sizeof(replaces));
memset(values, 0, sizeof(values));

+1. I'd be inclined to use "false" and "true" for the init values of
the boolean arrays, too.

Done.

mark

Attachments:

0001_extended_stats_sizeof.patch.2application/octet-stream; name=0001_extended_stats_sizeof.patch.2Download

diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index db4987bde3..eca09217e3 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -300,9 +300,9 @@ statext_store(Relation pg_stext, Oid statOid,
 	bool		nulls[Natts_pg_statistic_ext];
 	bool		replaces[Natts_pg_statistic_ext];
 
-	memset(nulls, 1, Natts_pg_statistic_ext * sizeof(bool));
-	memset(replaces, 0, Natts_pg_statistic_ext * sizeof(bool));
-	memset(values, 0, Natts_pg_statistic_ext * sizeof(Datum));
+	memset(nulls, true, sizeof(nulls));
+	memset(replaces, false, sizeof(replaces));
+	memset(values, 0, sizeof(values));
 
 	/*
 	 * Construct a new pg_statistic_ext tuple, replacing the calculated stats.

Michael Paquier

michael.paquier@gmail.com

about 8 years ago

In reply to: Mark Dilger (#3)

Re: Code cleanup patch submission for extended_stats.c

On Sun, Nov 26, 2017 at 11:07 AM, Mark Dilger <hornschnorter@gmail.com> wrote:

On Nov 25, 2017, at 2:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mark Dilger <hornschnorter@gmail.com> writes:

It looks to me like Alvaro introduced this in the original version of the file which
was created in commit 7b504eb282ca2f5104b5c00b4f05a3ef6bb1385b. Grep'ing
through the code base, it seems the following would be more consistent with
how these initializations are handled elsewhere:

memset(nulls, 1, sizeof(nulls));
memset(replaces, 0, sizeof(replaces));
memset(values, 0, sizeof(values));

+1. I'd be inclined to use "false" and "true" for the init values of
the boolean arrays, too.

Done.

That's better practice. More places deserve the same treatment if you
grep for them:
$ git grep memset | grep nulls | grep "[1|0]"
contrib/dblink/dblink.c: memset(nulls, 0, sizeof(nulls));
contrib/pageinspect/ginfuncs.c: memset(nulls, 0, sizeof(nulls));
contrib/pageinspect/ginfuncs.c: memset(nulls, 0, sizeof(nulls));
contrib/pageinspect/ginfuncs.c: memset(nulls, 0, sizeof(nulls));
contrib/pageinspect/heapfuncs.c: memset(nulls, 0, sizeof(nulls));
contrib/pageinspect/rawpage.c: memset(nulls, 0, sizeof(nulls));
contrib/pg_stat_statements/pg_stat_statements.c: memset(nulls,
0, sizeof(nulls));
contrib/pgstattuple/pgstatapprox.c: memset(nulls, 0, sizeof(nulls));
src/backend/access/heap/heapam.c: memset(nulls, 1, sizeof(nulls));
src/backend/catalog/pg_collation.c: memset(nulls, 0, sizeof(nulls));
src/backend/catalog/pg_range.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/extension.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/extension.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/extension.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/extension.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/extension.c: memset(nulls, 0, sizeof(nulls));
src/backend/commands/sequence.c: memset(pgs_nulls, 0, sizeof(pgs_nulls));
src/backend/commands/tablecmds.c: memset(nulls, 0, sizeof(nulls));
src/backend/libpq/hba.c: memset(nulls, 0, sizeof(nulls));
src/backend/libpq/hba.c: memset(&nulls[1], true,
(NUM_PG_HBA_FILE_RULES_ATTS - 2) * sizeof(bool));
src/backend/replication/logical/logicalfuncs.c: memset(nulls, 0,
sizeof(nulls));
src/backend/replication/logical/origin.c: memset(&nulls, 0,
sizeof(nulls));
src/backend/replication/logical/origin.c: memset(nulls, 1,
sizeof(nulls));
src/backend/replication/slotfuncs.c: memset(nulls, 0, sizeof(nulls));
src/backend/replication/slotfuncs.c: memset(nulls, 0, sizeof(nulls));
src/backend/replication/walsender.c: memset(nulls, 0, sizeof(nulls));
src/backend/statistics/extended_stats.c: memset(nulls, 1,
Natts_pg_statistic_ext * sizeof(bool));
src/backend/utils/adt/genfile.c: memset(nulls, 0, sizeof(nulls));
src/backend/utils/misc/guc.c: memset(nulls, 0, sizeof(nulls));
--
Michael

Alvaro Herrera

alvherre@alvh.no-ip.org

about 8 years ago

In reply to: Mark Dilger (#3)

Re: Code cleanup patch submission for extended_stats.c

Mark Dilger wrote:

On Nov 25, 2017, at 2:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Mark Dilger <hornschnorter@gmail.com> writes:

It looks to me like Alvaro introduced this in the original version of the file which
was created in commit 7b504eb282ca2f5104b5c00b4f05a3ef6bb1385b. Grep'ing
through the code base, it seems the following would be more consistent with
how these initializations are handled elsewhere:

memset(nulls, 1, sizeof(nulls));
memset(replaces, 0, sizeof(replaces));
memset(values, 0, sizeof(values));

+1. I'd be inclined to use "false" and "true" for the init values of
the boolean arrays, too.

Done.

Pushed, thanks.

--
ï¿½lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services