post-freeze damage control

On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Can you elaborate, which patches you think were not ready? Let's make
> sure to capture any concrete concerns in the Open Items list.

Hi,

I'm moving this topic to a new thread for better visibility and less
admixture of concerns. I'd like to invite everyone here present to
opine on which patches we ought to be worried about. Here are a few
picks from me to start things off. My intention here is to convey "I
find these scary" rather than "these commits were irresponsible," so I
respectfully ask that you don't take the choice to list your patch
here as an attack, or the choice not to list your patch here as an
endorsement. I'm very happy if I'm wrong and these patches are not
actually scary. And likewise let's view any allegations of scariness
by others as a genuine attempt to figure out what might be broken,
rather than as an attempt to get anyone into trouble or whatever.

- As I wrote about separately, I'm concerned that the failover slots
stuff may not be in as good a shape as it needs to be for people to
get good use out of it.
http://postgr.es/m/CA+TgmoaA4oufUBR5B-4o83rnwGZ3zAA5UvwxDX=NjCm1TVgRsQ@mail.gmail.com

- I also wrote separately about the flurry of recent table AM changes.
http://postgr.es/m/CA+TgmoZpWB50GnJZYF1x8PenTqXDTFBH_Euu6ybGfzEy34o+5Q@mail.gmail.com

- The streaming read API stuff was all committed very last minute. I
think this should have been committed much sooner. It's probably not
going to break the world; it's more likely to have performance
consequences. But if it had gone in sooner, we'd have had more time to
figure that out.

- The heap pruning refactoring work, on the other hand, seems to be at
very significant risk of breaking the world. I don't doubt the
authors, but this is some very critical stuff and a lot of it happened
very quickly right at the end.

- Incremental backup could have all sorts of terrible data-corrupting
bugs. 55a5ee30cd65886ff0a2e7ffef4ec2816fbec273 was a full brown-paper
bag level fix, so the chances of there being further problems are
probably high.

- I'm slightly worried about the TID store work (ee1b30f12, 30e144287,
667e65aac35), perhaps for no reason. Actually, the results seem really
impressive, and a very quick look at some of the commits seemed kind
of reassuring. But, like the changes to pruning and freezing, this is
making some really fundamental changes to critical code. In this case,
it's code that has evolved very little over the years and seems to
have now evolved quite a lot.

- I couldn't understand why the "Operate
XLogCtl->log{Write,Flush}Result with atomics" code was correct when I
read it. That's not to say I know it to be incorrect. But, a spinlock
protecting two variables together guarantees more than atomic access
to each of those variables separately.

There's probably a lot more to worry about; there have been so
incredibly many changes recently. But this is as far as my speculation
extends, as of now. Comments welcome. Additional concerns also
welcome, as noted above. And again, no offense is intended to anyone.

--
Robert Haas
EDB: http://www.enterprisedb.com