On Mon, Aug 29, 2022 at 2:20 AM Matthias van de Meent
<boekewurm+postgres@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