On Mon, Aug 29, 2022 at 2:20 AM Matthias van de Meent <boekewurm+postg...@gmail.com> wrote: > Apart from the message that this behaviour is changing, I'd prefer > some more description in the commit message as to why this needs > changing.
I usually only write a full commit message before posting a patch when it's a full patch series, where it can be helpful to be very explicit about how the parts fit together. The single line commit message is just a placeholder -- I'll definitely write a better one before commit. > Then, on to the patch itself: > > > + * XXX We don't do push back oldestMxact here, which is not > > ideal > > Do you intend to commit this marker, or is this leftover from the > development process? Ordinarily I would never commit an XXX comment, and probably wouldn't even leave one in early revisions of patches that I post to the list. This is a special case, though -- it involves the "snapshot too old" feature, which has many similar XXX/FIXME/TODO comments. I think I might leave it like that when committing. The background here is that the snapshot too old code still has lots of problems -- there is a FIXME comment that gives an overview of this in TransactionIdLimitedForOldSnapshots(). We're going to have to live with the fact that that feature isn't in good shape for the foreseeable future. I can only really work around it. > > + if (*multiXactCutoff < FirstMultiXactId) > [...] > > + if (safeOldestMxact < FirstMultiXactId) > [...] > > + if (aggressiveMXIDCutoff < FirstMultiXactId) > > I prefer !TransactionId/MultiXactIdIsValid() over '< First > [MultiXact/Transaction]Id', even though it is the same in > functionality, because it clarifies the problem we're trying to solve. > I understand that the use of < is pre-existing, but since we're > touching this code shouldn't we try to get this new code up to current > standards? I agree in principle, but there are already 40+ other places that use the same idiom in places like multixact.c. Perhaps you can propose a patch to change all of them at once, together? -- Peter Geoghegan