Code cleanup patch submission for extended_stats.c
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+3-3
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
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+3-3
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
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