Em sex., 28 de ago. de 2020 às 03:04, Noah Misch <n...@leadboat.com> escreveu:
> On Thu, Aug 27, 2020 at 11:11:47PM -0400, Tom Lane wrote: > > Noah Misch <n...@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-4ce1cdc9e...@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
