Non-emergency patch for bug #17679

Hi,

On 2022-11-08 11:28:08 -0500, Tom Lane wrote:

In the release team's discussion leading up to commit 0e758ae89,
Andres opined that what commit 4ab5dae94 had done to mdunlinkfork
was a mess, and I concur. It invented an entirely new code path
through that function, and required two different behaviors from the
segment-deletion loop. I think a very straight line can be drawn
between that extra complexity and the introduction of a nasty bug.
It's all unnecessary too, because AFAICS all we really need is to
apply the pre-existing behavior for temp tables and REDO mode
to binary-upgrade mode as well.

I'm not sure I understand the current code. In the binary upgrade case we
currently *do* truncate the file in the else of "Delete or truncate the first
segment.", then again truncate it in the loop and then unlink it, right?

Hence, the attached reverts everything 4ab5dae94 did to this function,
and most of 0e758ae89 too, and instead makes IsBinaryUpgrade an
additional reason to take the immediate-unlink path.

Barring objections, I'll push this after the release freeze lifts.

I wonder if it's worth aiming slightly higher. There's plenty duplicated code
between the first segment handling and the loop body. Perhaps the if at the
top just should decide whether to unlink the first segment or not, and we then
check that in the body of the loop for segno == 0?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-11-08 11:28:08 -0500, Tom Lane wrote:

Hence, the attached reverts everything 4ab5dae94 did to this function,
and most of 0e758ae89 too, and instead makes IsBinaryUpgrade an
additional reason to take the immediate-unlink path.

I wonder if it's worth aiming slightly higher. There's plenty duplicated code
between the first segment handling and the loop body. Perhaps the if at the
top just should decide whether to unlink the first segment or not, and we then
check that in the body of the loop for segno == 0?

I don't care for that. I think the point here is precisely that
we want behavior A for the first segment and behavior B for the
remaining ones, and so I'd prefer to keep the code that does A
and the code that does B distinct. It was a misguided attempt to
share that code that got us into trouble here in the first place.
Moreover, any future changes to either behavior will be that much
harder if we combine the implementations.

regards, tom lane