Re: Introduce contributor badges

On Fri, 16 Oct 2026 at 08:56, Christoph Berg <myon(at)debian(dot)org> wrote:

> Re: Magnus Hagander
> > I will point out that I *explicitly* asked that this system not be used
> > fully
>
> Fwiw, it is not fully in use yet. There are no links to badges or to
> the people page. There is not going to be a huge spike of requests
> because no one has these URLs yet.
>
> We are doing this precisely to allow looking at how it works in
> practise before the world runs over it.
>
> > until I had completed my review. I have *not* withdrawn this
> > requirement. AFAIK nobody has done the full review ont he impact this
> will
> > have on our caching setups and the production systems.
>
> Afaict the proper purge rules are in place, and the images have Etag
> headers. We can check all that once a badge is approved. (I had
> already submitted "PostgreSQL 18 Contributor" before your mail.)
>
> > If that is wrong,
> > then please point me to that. But if not, then my objection still stands,
> > and I am *not* happy that my *explicit* request to review production
> impact
> > was not honored.
>
> I still believe that we addressed all concerns. I was aware that you
> wanted more review, that's why we did that here. If that review was
> not enough going into that direction, I'm sorry - that's why we did it
> here openly on the list. Yes I know it's conference preparation time
> for you, but it should have been obvious that we were moving to
> putting this live, so just a "please do more review of X" line would
> have helped.
>

I haven't had time to review it properly yet, but I just had Claude Opus
review it, and it seems to think there are a few issues that need fixing. I
told it to pay particular attention to performance:

Commits reviewed: `bfbb841`, `1993c00`, `8eb38fb`, `5cdc3ed`, `7257af2`,
`c93898e`, `73aab74`.

The only ones with performance or caching implications are `bfbb841` (image
field) and `8eb38fb` (contributor badges). The download-page edits, the
pycodestyle fix, the OneToOneField migration and the copy-editing commit
are all fine. `manage.py check` reports no issues.

## Varnish cache invalidation

The site relies on Varnish holding pages for a long time and on purges to
clear them (`docs/frontend.rst`). Several changes in the badges feature
never send a purge, so stale pages stay up until the default time-to-live
(TTL) runs out, and longer in grace mode.

1. **Removing a holder purges nothing for that person.** `BadgeForm.save`
calls `badge.holders.remove()` (`pgweb/contributors/forms.py`). That
deletes the through-table rows without any `post_save`, and pgweb only
hooks `post_save` (`pgweb/util/signals.py:169`). The badge page still gets
purged, because `simple_form` saves the badge afterwards. The removed
person's `/community/people/<username>/` page keeps showing the badge.
2. **Deletes never purge.** This applies to deleting a `Badge`,
`Badgeholder` or `Contributor` in the admin. The rest of pgweb has the same
gap, but it matters more here: a deleted contributor's public profile page,
which can include their email, stays cached. If someone asks for their
profile to be removed, it isn't really gone.
3. **Approving or editing a badge doesn't purge its holders' profile
pages.** `Badge.purge_urls` says profiles "get refreshed when their holder
rows are edited", but approval, unapproval, a title change and an image
added or removed all leave the profiles stale. After unapproval, profiles
keep linking to `/community/badge/<slug>/image/`, which now returns a 404
that Varnish may also cache. The fix is xkeys, which the docs already
describe as the better option: tag the badge, people and profile responses
with something like `pgbadge_<id>` and give `Badge` a `purge_xkeys`. That
also replaces the per-holder URL loops.
4. **The image URL never changes when the image does.**
`/community/badge/<slug>/image/` has no file extension and no version in
it. The docs say Varnish applies a longer policy to images "because they
are considered never to change". Varnish itself is purged, but if the
Varnish configuration (VCL) also sends long browser cache headers for
images, browsers will keep showing the old image. The VCL isn't in this
repo, so I can't check which behaviour applies. Adding a short hash of the
image to the URL, as `?v=` or in the path, avoids the question.

## Database and app server

5. **The `has_image` annotation reads every image out of TOAST**,
PostgreSQL's out-of-line storage for large values
(`BadgeManager.with_has_image`). The comment says the value is "never
transferred", which is true for the network, but `imagedata > ''` compares
the full bytea, so PostgreSQL has to fetch and reassemble the whole value.
A test of 200 rows of about 96 KB each on the local database showed the
comparison reading about 3,000 buffers in 31 ms, whilst `octet_length(img)
> 0` read 2 buffers in 0.04 ms, because it only reads the TOAST header.
Switching to `Length('imagedata') > 0` is a one-line fix. It runs on every
cache miss of the people page (up to 200 badges) and on every account
profile page load, which is never cached.
6. **Oversized images are decoded in full (a "decompression bomb").**
`ImageBinaryField.to_python` decodes the entire image with Pillow before
rescaling. A PNG under 1 MB can still be about 170 megapixels before Pillow
refuses it, which is several hundred MB of RAM plus a bicubic resize on the
app server. `ModelForm` full-clean also re-parses the stored image on every
badge edit, even when the image hasn't changed. Only managers of an
approved organisation can upload, so the risk is moderate, but checking
`img.size` before decoding would be cheap.
7. **Minor, and only on cache misses:**
- `badge.html` uses `holders|length` and then `holders.all`, which runs
the holder query and its two prefetches a second time.
- `select_related` would do better than `prefetch_related` for the
foreign-key lookups in `peoplelist` and `badge_view`.
- `badge_image` fetches the image and computes its MD5 before it checks
`If-None-Match`; storing the hash would avoid both.
- Every account profile save that has a contributor record purges
`/community/contributors/`, `/community/people/` and all of that user's
badge pages, even when nothing changed.

## Suggested order of fixes

Start with the xkey purging (items 1 to 3) and the `Length()` change (item
5).

--
Dave Page
pgAdmin: https://www.pgadmin.org
PostgreSQL: https://www.postgresql.org
pgEdge: https://www.pgedge.com