Re: scary patch contest

Hi,

On 2026-08-25 12:48:25 -0400, Robert Haas wrote:
> 2. REPACK / REPACK CONCURRENTLY (ac58465e061, 28d534e2ae0, Antonin
> Houska c/b Álvaro Herrera): 28 fixes including data loss (missingvals
> discarded, also affecting VACUUM FULL/CLUSTER), broken toasted-tuple
> and generated-column handling, and two security-flavored ACL fixes.
> Its prerequisite (database-specific logical replication snapshots,
> 0d3dba38c777) was reverted as fundamentally flawed, leaving v19's
> REPACK CONCURRENTLY restricted to one process cluster-wide.

What worries me here is that something like 0d3dba38c777 was committed, it was
pretty obviously broken. I'm also worried that it was committed with stuff
like the deadlock dangers not even being close to actually resolved.

> 3. Online data checksums (f19c0eccae9, Daniel Gustafsson and Magnus
> Hagander): ~25 substantive fixes, mostly state-machine holes — invalid
> transitions in checkpoints and promotion, initial state missing from
> the control file, basebackup false checksum failures, wrong WAL for
> unlogged relations. A corruption-detection feature producing false
> positives is exactly the wrong failure mode.

I'm not *too* concerned about this one. In contrast to most of the other
features here it's a super user only feature that you're not going to use
continuously. If it turns out to be too problematic, disabling it is a few
lines.

> 4. UPDATE/DELETE FOR PORTION OF (8e72d914c52, Paul Jungwirth c/b Peter
> Eisentraut): 17 fixes, three of them security — leftover-row inserts
> bypassed INSERT RLS policies, WITH CHECK OPTION went unenforced on
> DELETE, and UPDATE permission on the range column wasn't required. The
> interaction matrix (views, generated columns, inheritance, rules,
> WHERE CURRENT OF, FDW partitions) had to be forbidden or fixed piece
> by piece after freeze.

What worries me here is that several of these interactions seem like fairly
obvious ones. Not recognizing & testing some complicated interaction, race
conditions, etc are one thing, but the oversights here seem to be pretty
... broad.

I also wonder if we need some more scalable testing here, to test the
combination of various interacting features. E.g. something like a perl script
that generates the combinatorial combination of features around this being
[not] used and verifies that results stay consistent. That could makes it much
cheaper (in human time, not necessarily test runtime), to test some of the
interactions here.

> 5. SQL/PGQ property graphs (2f094e7ac69, Peter Eisentraut and Ashutosh
> Bapat): 17 fixes, but lower severity — wrong collation, unresolved
> literals, broken LATERAL references, deparse and pg_dump ACL bugs,
> plus a series of after-the-fact prohibitions.

I kind of unintentionally looked at this recently, after working on some
nearby code:
https://www.postgresql.org/message-id/dqa5mstx5mna3i7s23pdwl4m6bek7gqsgfccef44wjpswizufi%403aa6vzri3cat

I'm a bit shocked at the density of problems that I, without even trying or
knowing any of the relevant code, found and then what AI & others
found. That'd be a lot for the feature just after being merged, but we're
getting close to the release of 19.

The fact that ALTER PROPERTY GRAPH doesn't even verify that it's operating on
a property graph seems to indicate that it's pretty far from ready.

> I don't currently have a firm position on what we should do here. I
> think it's pretty clear that none of these were as robust at commit
> time as we would like, but that doesn't mean that they're still
> broken. I personally think #4 and #6 are relatively lower risk on a
> going-forward basis, because they're only going to come up in fairly
> niche situations. That somewhat applies to #5 as well, but it's a
> bigger feature so I'm more concerned.

I think e.g. something like #4 (PORTION OF) and #5 (PGQ) are on the high-risk
end, due to the broad exposure to users. If there's a bunch of security bugs -
and there clearly are in #5 - a server admin / host can't really restrict the
use of the feature, there's no separate permission to do so. Whereas
something like #3 (online checksums) is fundamentally superuser only. With
#4/#5 an attacker doing something intentionally adverse is possible with
relatively low privileges, whereas the adversarial scenarios for #3 are a lot
more far fetched, due to almost exlusively being superuser only operations.

> #2 has been cut back in scope enough that I don't think we would have
> accepted the feature originally if it had been subject to those limitations,
> which IMHO is a reasonable argument for a revert but also not
> conclusive.

Leaving aside everything else, I think we probably *should* have gone for the
much more limited version first. Incremental development is a virtue.

Greetings,

Andres Freund