From: Tom Lane <tgl@sss.pgh.pa.us>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: code cleanups
Date: Wed, 23 Nov 2022 12:52:57 -0500
Message-ID: <3722577.1669225977@sss.pgh.pa.us> (raw)
In-Reply-To: <20221123172436.GJ11463@telsasoft.com>
References: <20221123172436.GJ11463@telsasoft.com>
Justin Pryzby <pryzby@telsasoft.com> 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:
- bool nulls[PG_STAT_GET_RECOVERY_PREFETCH_COLS];
+ bool nulls[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
bool nulls[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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pryzby@telsasoft.com
Subject: Re: code cleanups
In-Reply-To: <3722577.1669225977@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox