Re: 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