Subject: post-freeze damage control

From: Robert Haas <[email-masked]>
Date: Mon, 8 Apr 2024 15:47:34 -0400

On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email-masked]> 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=[email-masked]

- I also wrote separately about the flurry of recent table AM changes.
http://postgr.es/m/[email-masked]

- 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

---

From: Thomas Munro <[email-masked]>
Date: Tue, 9 Apr 2024 09:35:00 +1200

> consequences. But if it had gone in sooner, we'd have had more time to

OK, let me give an update on this work stream (pun intended).

One reason for the delay in committing was precisely that we were
fretting about regressions risks.  We tried pretty hard to identify
and grind down every regression we could find, and cases with
outstanding not-fully-understood or examined problems in that area
have been booted into the next cycle for more work: streaming bitmap
heapscan, several streaming vacuum patches, and more, basically things
that seem to have more complex interactions with other machinery.  The
only three places using streaming I/O that went in were:

041b9680: Use streaming I/O in ANALYZE.
b7b0f3f2: Use streaming I/O in sequential scans.
3a352df0: Use streaming I/O in pg_prewarm.

The first is a good first exercise in streaming random blocks;
hopefully no one would be too upset about an unexpected small
regression in ANALYZE, but as it happens it goes faster hot and cold
according to all reports.  The second is a good first exercise in
streaming sequential blocks, and it ranges from faster to no
regression, according to testing and reports.  The third is less
important, but it also goes faster.

Of those, streaming seq scan is clearly the most relevant to real
workloads that someone might be upset about, and I made a couple of
choices that you might say had damage control in mind:

* A conservative choice not to get into the business of the issuing
new hints to the kernel for random jumps in cold scans, even though we
think we probably should for better performance: more research needed
precisely to avoid unexpected interactions (cf the booted bitmap
heapscan where that sort of thing seems to be afoot).
* A GUC to turn off I/O combining if it somehow upsets your storage in
ways we didn't foresee (io_combine_limit=1).

For fully cached hot scans, it does seem to be quite sensitive to tiny
changes in a hot code path that I and others spent a lot of time
optimising and testing during the CF.  Perhaps it is possible that
someone else's microarchitecture or compiler could show a regression
that I don't see, and I will certainly look into it with vim and
vigour if so.  In that case we could consider a tiny
micro-optimisation that I've shared already (it seemed a little novel
so I'd rather propose it in the new cycle if I can), or, if it comes
to it based on evidence and inability to address a problem quickly,
reverting just b7b0f3f2 which itself is a very small patch.

An aspect you didn't mention is correctness.  I don't actually know
how to prove that buffer manager protocols are correct beyond thinking
and torture testing, ie what kind of new test harness machinery could
be used to cross-check more things about buffer pool state explicitly,
and that is a weakness I'm planning to look into.

I realise that "these are the good ones, you should see all the stuff
we decided not to commit!" is not an argument, I'm just laying out how
I see the patches that went in and why I thought they were good.  It's
almost an architectural change, but done in tiny footsteps.  I
appreciate that people would have liked to see those particular tiny
footsteps in some of the other fine months available for patching the
tree, and some of the earlier underpinning patches that were part of
the same patch series did go in around New Year, but clearly my
"commit spreading" didn't go as well as planned after that (not helped
by Jan/Feb summer vacation season down here).

Mr Paquier this year announced his personal code freeze a few weeks
back on social media, which seemed like an interesting idea I might
adopt.  Perhaps that is what some other people are doing without
saying so, and perhaps the time they are using for that is the end of
the calendar year.  I might still be naturally inclined to crunch-like
behaviour, but it wouldn't be at the same time as everyone else,
except all the people who follow the same advice.

---

From: Michael Paquier <[email-masked]>
Date: Tue, 9 Apr 2024 07:58:07 +0900

> except all the people who follow the same advice.

That's more linked to the fact that I was going silent without a
laptop for a few weeks before the end of the release cycle, and a way
to say to not count on me, while I was trying to keep my room clean to
avoid noise for others who would rush patches.  It is a vacation
period for schools in Japan as the fiscal year finishes at the end of
March, while the rest of the world still studies/works, so that makes 
trips much easier with areas being less busy when going abroad.  If
you want to limit commit activity during this period, the answer is
simple then: require that all the committers live in Japan.

Jokes apart, I really try to split commit effort across the year and
not rush things at the last minute.  If something's not agreed upon
and commit-ready by the 15th of March, the chances that I would apply
it within the release cycle are really slim.  That's a kind of
personal policy I have in place for a few years now.
--
Michael

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 9 Apr 2024 01:16:02 +0200

On 4/8/24 21:47, Robert Haas wrote:
> ...

I don't feel too particularly worried about this. Yes, backups are super
important because it's often the only thing you have left when things go
wrong, and the incremental aspect is all new. The code I've seen while
doing the CoW-related patches seemed very precise and careful, and the
one bug we found & fixed does not make it bad.

Sure, I can't rule there being more bugs, but I've been doing some
pretty extensive stress testing of this (doing incremental backups +
combinebackup, and comparing the results against the source, and that
sort of stuff). And so far only that single bug this way. I'm still
doing this randomized stress testing, with more and more complex
workloads etc. and I'll let keep doing that for a while.

Maybe I'm a bit too happy-go-lucky, but IMO the risk here is limited.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Mon, 08 Apr 2024 22:12:16 -0400

> endorsement.

I have another one that I'm not terribly happy about:

    Author: Alexander Korotkov <[email-masked]>
    Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300

        Transform OR clauses to ANY expression

I don't know that I'd call it scary exactly, but I do think it
was premature.  A week ago there was no consensus that it was
ready to commit, but Alexander pushed it (or half of it, anyway)
despite that.  A few concrete concerns:

* Yet another planner GUC.  Do we really need or want that?

* What the medical community would call off-label usage of
query jumbling.  I'm not sure this is even correct as-used,
and for sure it's using that code for something never intended.
Nor is the added code adequately (as in, at all) documented.

* Patch refuses to group anything but Consts into the SAOP
transformation.  I realize that if you want to produce an
array Const you need Const inputs, but I wonder why it
wasn't considered to produce an ARRAY[] construct if there
are available clauses with pseudo-constant (eg Param)
comparison values.

* I really, really dislike jamming this logic into prepqual.c,
where it has no business being.  I note that it was shoved
into process_duplicate_ors without even the courtesy of
expanding the header comment:

 * process_duplicate_ors
 *	  Given a list of exprs which are ORed together, try to apply
 *	  the inverse OR distributive law.

Another reason to think this wasn't a very well chosen place is
that the file's list of #include's went from 4 entries to 11.
Somebody should have twigged to the idea that this was off-topic
for prepqual.c.

* OrClauseGroupKey is not a Node type, so why does it have
a NodeTag?  I wonder what value will appear in that field,
and what will happen if the struct is passed to any code
that expects real Nodes.

I could probably find some other nits if I spent more time
on it, but I think these are sufficient to show that this
was not commit-ready.

			regards, tom lane

---

From: Andrei Lepikhov <[email-masked]>
Date: Tue, 9 Apr 2024 12:37:31 +0700

>          Transform OR clauses to ANY expression
Because I'm primary author of the idea, let me answer.
> * Yet another planner GUC.  Do we really need or want that?
It is the most interesting question here. Looking around planner 
features designed but not applied for the same reason because they can 
produce suboptimal plans in corner cases, I think about inventing 
flag-type parameters and hiding some features that work better for 
different load types under such flagged parameters.
> Nor is the added code adequately (as in, at all) documented.
I agree with documentation and disagree with critics on the expression 
jumbling. It was introduced in the core. Why don't we allow it to be 
used to speed up machinery with some hashing?
> are available clauses with pseudo-constant (eg Param)
Good point. I think, we can consider that in the future.
> expanding the header comment:
Yeah, I preferred to do it in parse_expr.c with the assumption of some 
'minimal' or 'canonical' tree form. You can see this code in the 
previous version. I think we don't have any bugs here, but we have 
different opinions on how it should work.
> and what will happen if the struct is passed to any code
It's a hack authored by Alexander. I guess He can provide additional 
reasons in support of that.
> on it, but I think these are sufficient to show that this
It's up to you. On the one hand, I don't see any bugs or strong 
performance issues, and all the issues can be resolved further; on the 
other hand, I've got your good review and some ideas to work out.

-- 
regards,
Andrei Lepikhov
Postgres Professional

---

From: Tom Lane <[email-masked]>
Date: Tue, 09 Apr 2024 01:55:39 -0400

> used to speed up machinery with some hashing?

I would back up from that a good deal: why do we need to hash here in
the first place?  There's no evidence I'm aware of that it's needful
from a performance standpoint.

> 'minimal' or 'canonical' tree form.

That seems quite the wrong direction to me.  AFAICS, the argument
for making this transformation depends on being able to convert
to an indexscan condition, so I would try to apply it even later,
when we have a set of restriction conditions to apply to a particular
baserel.  (This would weaken the argument that we need hashing
rather than naive equal() tests even further, I think.)  Applying
the transform to join quals seems unlikely to be a win.

			regards, tom lane

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 9 Apr 2024 22:47:37 +0300

> and what will happen if the struct is passed to any code

I used that to put both not-subject-of-transform nodes together with
hash entries into the same list.  This is used to save the order of
clauses.  I think this is an important property, and I have already
expressed it in [1].  That could be achieved without adding NodeTag to
hash entries, but that would require a level of indirection.  It's not
passed to code that expects real Nodes, it doesn't go to anything
except lists.

Links.
1. https://www.postgresql.org/message-id/CAPpHfdutHt31sdt2rfU%3D4fsDMWxf6tvtnHARgCzLY2Tf21%2Bfgw%40mail.gmail.com

------
Regards,
Alexander Korotkov

---

From: Tom Lane <[email-masked]>
Date: Tue, 09 Apr 2024 16:37:31 -0400

> expressed it in [1].

What exactly is the point of having a NodeTag in the struct though?
If you don't need it to be a valid Node, that seems pointless and
confusing.  We certainly have plenty of other lists that contain
plain structs without tags, so I don't buy that the List
infrastructure is making you do that.

			regards, tom lane

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 9 Apr 2024 23:48:12 +0300

> infrastructure is making you do that.

This code mixes Expr's and hash entries in the single list.  The point
of having a NodeTag in the struct is the ability to distinguish them
later.

------
Regards,
Alexander Korotkov