Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2021-Sep-29, Ranier Vilela wrote:
>> Em qua., 29 de set. de 2021 às 08:12, Drouvot, Bertrand <bdrouvot@amazon.com>
>> escreveu:
>> Duplicating functions is very bad for maintenance and bloats the code
>> unnecessarily, IMHO.
> Well, there are 42 calls of PushActiveSnapshot currently, and only 6 are
> updated in the patch. Given that six sevenths of the calls continue to
> use the existing function and that it is less verbose than the new one,
> that seems sufficient argument to keep it.
Seeing that we have to back-patch this, changing the ABI of
PushActiveSnapshot seems like a complete non-starter.
The idea I'd had to avoid code duplication was to make
PushActiveSnapshot a wrapper for the extended function:
void
PushActiveSnapshot(Snapshot snap)
{
PushActiveSnapshotWithLevel(snap, GetCurrentTransactionNestLevel());
}
This would add one function call to the common code path, but there
are enough function calls in PushActiveSnapshot that I don't think
that's a big concern.
Another point is that this'd also add the as_level ordering assertion
to the common code path, but on the whole I think that's good not bad.
BTW, this is not great code:
+ if (ActiveSnapshot != NULL && ActiveSnapshot->as_next != NULL)
+ Assert(as_level >= ActiveSnapshot->as_next->as_level);
You want it all wrapped in the Assert, so that there's not any code
left in a non-assert build (which the compiler may or may not optimize
away, perhaps after complaining about a side-effect-free statement).
Actually, it's plain wrong, because you should be looking at the
top as_level not the next one. So more like
Assert(ActiveSnapshot == NULL ||
snap_level >= ActiveSnapshot->as_level);
regards, tom lane