Thread: 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
Attachment
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
Attachment
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