Re: code cleanups
Justin Pryzby writes:
> Some modest cleanups I've accumulated
Hi Justin.
0001:
Regarding initializer {0}, the problem is still with old compilers, which
don't initialize exactly like memset.
Only more modern compilers fill in any "holes" that may exist.
This means that as old compilers are not supported, this will no longer be
a problem.
Fast and secure solution: memset
+1 to switch from loop to memset, same as many places in the code base.
- /* initialize nulls and values */
- for (i = 0; i < Natts_pg_constraint; i++)
- {
- nulls[i] = false;
- values[i] = (Datum) NULL;
- }
+ memset(nulls, false, sizeof(nulls));
+ memset(values, 0, sizeof(values));
What made me curious about this excerpt is that the Datum values are NULL,
but aren't nulls?
Would it not be?
+ memset(nulls, true, sizeof(nulls));
+ memset(values, 0, sizeof(values));
src/backend/tcop/pquery.c:
/* single format specified, use for all columns */
-int16 format1 = formats[0];
-
-for (i = 0; i < natts; i++)
-portal->formats[i] = format1;
+ memset(portal->formats, formats[0], natts * sizeof(*portal->formats));
0002:
contrib/sslinfo/sslinfo.c
memset is faster than intercalated stores.
src/backend/replication/logical/origin.c
+1
one store, is better than three.
but, should be:
- memset(nulls, 1, sizeof(nulls));
+memset(nulls, false, sizeof(nulls));
The correct style is false, not 0.
src/backend/utils/adt/misc.c:
-1
It got worse.
It's only one store, which could be avoided by the "continue" statement.
src/backend/utils/misc/pg_controldata.c:
+1
+memset(nulls, false, sizeof(nulls));
or
nulls[0] = false;
nulls[1] = false;
nulls[2] = false;
nulls[3] = false;
Bad style, intercalated stores are worse.
0003:
+1
But you should reduce the scope of vars:
RangeTblEntry *rte
Oid userid;
+ if (varno != relid)
+ {
+ RangeTblEntry *rte;
+ Oid userid;
0005:
+1
0006:
+1
regards,
Ranier Vilela
Re: code cleanups
On Thu, Nov 24, 2022 at 12:53 AM Tom Lane wrote: > > Justin Pryzby writes: > > Some modest cleanups I've accumulated. > 0004: Right, somebody injected code in a poorly chosen place > (yet another victim of the "add at the end" anti-pattern). I've pushed 0004. -- John Naylor EDB: http://www.enterprisedb.com
Re: code cleanups
On Wed, Nov 23, 2022 at 12:53 PM Tom Lane wrote:
> at least for bool arrays, that's true of memset'ing as well. But this,
> if you decide you need something other than zeroes, is a foot-gun.
> In particular, someone whose C is a bit weak might mistakenly think that
>
> boolnulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {true};
>
> will set all the array elements to true. Nor is there a plausible
> argument that this is more efficient. So I don't care for this approach
> and I don't want to adopt it.
I don't really know what the argument is for the explicit initializer
style, but I think this argument against it is pretty weak.
It should be more than fine to assume that anyone who is hacking on
PostgreSQL is proficient in C. It's true that there might be some
people who aren't, or who aren't familiar with the limitations of the
initializer construct, and I include myself in that latter category. I
don't think it was part of C when I learned C. But if we don't possess
the collective expertise as a project to bring people who have missed
these details of the C programming language up to speed, we should
just throw in the towel now and go home.
Hacking on PostgreSQL is HARD and it relies on knowing FAR more than
just the basics of how to code in C. Put differently, if you can't
even figure out how C works, you have no chance of doing anything very
interesting with the PostgreSQL code base, because you're going to
have to figure out a lot more than the basics of the implementation
language to make a meaningful contribution.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: code cleanups
Justin Pryzby writes:
> Some modest cleanups I've accumulated.
Hmm ...
I don't especially care for either 0001 or 0002, mainly because
I do not agree that this is good style:
- boolnulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
+ boolnulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {0};
It causes the code to be far more in bed than I like with the assumption
that we're initializing to physical zeroes. The explicit loop method
can be trivially adjusted to initialize to "true" or some other value;
at least for bool arrays, that's true of memset'ing as well. But this,
if you decide you need something other than zeroes, is a foot-gun.
In particular, someone whose C is a bit weak might mistakenly think that
boolnulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {true};
will set all the array elements to true. Nor is there a plausible
argument that this is more efficient. So I don't care for this approach
and I don't want to adopt it.
0003: I agree with getting rid of the duplicated code, but did you go
far enough? Isn't the code just above those parent checks also pretty
redundant? It would be more intellectually consistent to move the full
responsibility for setting acl_ok into a subroutine. This shows in
the patch as you have it because the header comment for
recheck_parent_acl is completely out-of-context.
0004: Right, somebody injected code in a poorly chosen place
(yet another victim of the "add at the end" anti-pattern).
0005: No particular objection, but it's not much of an improvement
either. It seems maybe a shade less consistent with the following
line.
0006: These changes will cause fetching of one more source byte than
was fetched before. I'm not sure that's safe, so I don't think this
is an improvement.
regards, tom lane
code cleanups
Some modest cleanups I've accumulated.
>From 9c5846cc3f04c6797696a234fac0953815e09e4b Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 23 Oct 2022 14:52:48 -0500
Subject: [PATCH 1/6] WIP: do not loop to set an array to false
See also:
9fd45870c1436b477264c0c82eb195df52bc0919
572e3e6634e55accf95e2bcfb1340019c86a21dc
---
src/backend/access/transam/xlogprefetcher.c | 5 +---
src/backend/catalog/pg_constraint.c | 11 ++--
src/backend/commands/user.c | 6 +
src/backend/statistics/extended_stats.c | 6 +
src/backend/storage/smgr/md.c | 3 +--
src/backend/tcop/pquery.c | 3 +--
src/backend/utils/adt/arrayfuncs.c | 29 -
src/test/isolation/isolationtester.c| 3 +--
8 files changed, 19 insertions(+), 47 deletions(-)
diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index 0cf03945eec..7fcfd146525 100644
--- a/src/backend/access/transam/xlogprefetcher.c
+++ b/src/backend/access/transam/xlogprefetcher.c
@@ -832,13 +832,10 @@ pg_stat_get_recovery_prefetch(PG_FUNCTION_ARGS)
#define PG_STAT_GET_RECOVERY_PREFETCH_COLS 10
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
Datum values[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
- bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
+ bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS] = {0};
InitMaterializedSRF(fcinfo, 0);
- for (int i = 0; i < PG_STAT_GET_RECOVERY_PREFETCH_COLS; ++i)
- nulls[i] = false;
-
values[0] = TimestampTzGetDatum(pg_atomic_read_u64(>reset_time));
values[1] = Int64GetDatum(pg_atomic_read_u64(>prefetch));
values[2] = Int64GetDatum(pg_atomic_read_u64(>hit));
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 3a5d4e96439..bd8f32bf62d 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -82,8 +82,8 @@ CreateConstraintEntry(const char *constraintName,
Relation conDesc;
Oid conOid;
HeapTuple tup;
- bool nulls[Natts_pg_constraint];
- Datum values[Natts_pg_constraint];
+ bool nulls[Natts_pg_constraint] = {0};
+ Datum values[Natts_pg_constraint] = {0};
ArrayType *conkeyArray;
ArrayType *confkeyArray;
ArrayType *conpfeqopArray;
@@ -165,13 +165,6 @@ CreateConstraintEntry(const char *constraintName,
else
conexclopArray = NULL;
- /* initialize nulls and values */
- for (i = 0; i < Natts_pg_constraint; i++)
- {
- nulls[i] = false;
- values[i] = (Datum) NULL;
- }
-
conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
Anum_pg_constraint_oid);
values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 8b6543edee2..5af79792136 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1234,8 +1234,7 @@ RenameRole(const char *oldname, const char *newname)
bool isnull;
Datum repl_val[Natts_pg_authid];
bool repl_null[Natts_pg_authid];
- bool repl_repl[Natts_pg_authid];
- int i;
+ bool repl_repl[Natts_pg_authid] = {0};
Oid roleid;
ObjectAddress address;
Form_pg_authid authform;
@@ -1321,9 +1320,6 @@ RenameRole(const char *oldname, const char *newname)
}
/* OK, construct the modified tuple */
- for (i = 0; i < Natts_pg_authid; i++)
- repl_repl[i] = false;
-
repl_repl[Anum_pg_authid_rolname - 1] = true;
repl_val[Anum_pg_authid_rolname - 1] = DirectFunctionCall1(namein,
CStringGetDatum(newname));
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index ab97e71dd79..93a9d3eeafb 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -2343,7 +2343,7 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs)
VacAttrStats *stats = exprdata[exprno].vacattrstat;
Datum values[Natts_pg_statistic];
- bool nulls[Natts_pg_statistic];
+ bool nulls[Natts_pg_statistic] = {0};
HeapTuple stup;
if (!stats->stats_valid)
@@ -2359,10 +2359,6 @@ serialize_expr_stats(AnlExprData *exprdata, int nexprs)
/*
* Construct a new pg_statistic tuple
*/
- for (i = 0; i < Natts_pg_statistic; ++i)
- {
- nulls[i] = false;
- }
values[Anum_pg_statistic_starelid - 1] = ObjectIdGetDatum(InvalidOid);
values[Anum_pg_statistic_staattnum - 1] = Int16GetDatum(InvalidAttrNumber);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 14b6fa0fd90..2d5cc97eaa9 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -557,8 +557,7 @@ void
mdopen(SMgrRelation reln)
{
/* mark it not open */
- for (int forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- reln->md_num_open_segs[forknum] = 0;
+ memset(reln->md_num_open_segs, 0, (1+MAX_FORKNUM) * sizeof(*reln->md_num_open_segs));
}
/*
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index
