On Thu, Aug 27, 2020 at 11:11:47PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Thu, Aug 27, 2020 at 12:57:20PM -0400, Alvaro Herrera wrote:
> >> Looks legit, and at least per commit 13bba02271dc we do fix such things,
> >> even if it's useless in practice.
> >> Given that no buildfarm member has ever complained, this exercise seems
> >> pretty pointless.
>
> > Later decision to stop changing such code:
> > https://postgr.es/m/flat/e1a26ece-7057-a234-d87e-4ce1cdc9eaa0@2ndquadrant.com
> I think that the actual risk here has to do not with
> what memcmp() might do, but with what gcc might do in code surrounding
> the call, once it's armed with the assumption that any pointer we pass
> to memcmp() could not be null. See
>
> https://www.postgresql.org/message-id/flat/BLU437-SMTP48A5B2099E7134AC6BE7C7F2A10%40phx.gbl
>
> It's surely not hard to visualize cases where necessary code could
> be optimized away if the compiler thinks it's entitled to assume
> such things.
Good point. We could pick from a few levels of concern:
- No concern: reject changes serving only to remove this class of deviation.
This is today's policy.
- Medium concern: accept fixes, but the buildfarm continues not to break in
the face of new deviations. This will make some code uglier, but we'll be
ready against some compiler growing the optimization you describe.
- High concern: I remove -fno-sanitize=nonnull-attribute from buildfarm member
thorntail. In addition to the drawback of the previous level, this will
create urgent work for committers introducing new deviations (or introducing
test coverage that unearths old deviations). This is our current response
to Valgrind complaints, for example.
Maybe in this specific case, the policy could be ignored, this change does not hurt.
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -293,7 +293,7 @@ TransactionIdSetPageStatus(TransactionId xid, int nsubxids,
* sub-XIDs and all of the XIDs for which we're adjusting clog should be
* on the same page. Check those conditions, too.
*/
- if (all_xact_same_page && xid == MyProc->xid &&
+ if (all_xact_same_page && subxids && xid == MyProc->xid &&
nsubxids <= THRESHOLD_SUBTRANS_CLOG_OPT &&
nsubxids == MyProc->subxidStatus.count &&
memcmp(subxids, MyProc->subxids.xids,
regards,
Ranier Vilela