Tighten ACL check in repack_is_permitted_for_relation()
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253304psql -h localhost -U postgresBuilt from patchset v4 (message #4), August 18, 2026 at 05:20 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253304_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253304_4 && git checkout t253304_4Patchset v4 (message #4) is on t253304_4
Hi,
(CC-ing Álvaro Herrera for some thoughts)
repack_is_permitted_for_relation() uses pg_class_aclcheck_ext() to
silently skip a concurrently-dropped relation. That's wrong for a
caller that may already hold a lock on the relation whose ACL is
checked, where missing a relation is not fine, and it makes the
single-relation REPACK cases more brittle
(/messages/by-id/akPhEffRipH4isWF@nathan). So
only detect a missing relation where that's expected, following the
fix for vacuum_is_permitted_for_relation() in commit 824d5f6. The new
missing_ok behavior is limited to get_tables_to_repack() and
get_tables_to_repack_partitioned(). All other callers of
repack_is_permitted_for_relation() hold a lock on the relation that
prevents it from being concurrently dropped, so this commit also adds
an assertion to that effect.
I posted this in the thread but starting a new discussion to get some
quick thoughts:
/messages/by-id/CALj2ACX3pyuRS8++6L20cJUMRTf_qbbVp69J1btJ3y6=77e5gw@mail.gmail.com
Please find the attached patch for review.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
On Tue, Aug 04, 2026 at 01:13:00AM -0700, Bharath Rupireddy wrote:
I posted this in the thread but starting a new discussion to get some
quick thoughts:
/messages/by-id/CALj2ACX3pyuRS8++6L20cJUMRTf_qbbVp69J1btJ3y6=77e5gw@mail.gmail.comPlease find the attached patch for review.
Alvaro, could you comment please?
Perhaps this is something that should be an open item for v19 due to
the fact that it is new code and it can make a REPACK operation more
disruptive depending on unlocky circumstances and concurrent activity?
If Alvaro thinks differently, that's fine, as REPACK is his feature.
it just seems to me like one possible option on the table.
--
Michael
Hi,
On Thu, Aug 13, 2026 at 8:51 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 04, 2026 at 01:13:00AM -0700, Bharath Rupireddy wrote:
I posted this in the thread but starting a new discussion to get some
quick thoughts:
/messages/by-id/CALj2ACX3pyuRS8++6L20cJUMRTf_qbbVp69J1btJ3y6=77e5gw@mail.gmail.comPlease find the attached patch for review.
Alvaro, could you comment please?
Perhaps this is something that should be an open item for v19 due to
the fact that it is new code and it can make a REPACK operation more
disruptive depending on unlocky circumstances and concurrent activity?
If Alvaro thinks differently, that's fine, as REPACK is his feature.
it just seems to me like one possible option on the table.
I added this as an open item for PG19 to get some thoughts before the
release. Thanks.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
Hello
On 2026-Aug-04, Bharath Rupireddy wrote:
repack_is_permitted_for_relation() uses pg_class_aclcheck_ext() to
silently skip a concurrently-dropped relation. That's wrong for a
caller that may already hold a lock on the relation whose ACL is
checked, where missing a relation is not fine, and it makes the
single-relation REPACK cases more brittle
(/messages/by-id/akPhEffRipH4isWF@nathan). So
only detect a missing relation where that's expected, following the
fix for vacuum_is_permitted_for_relation() in commit 824d5f6.
That makes sense. I think "missing OK" is a bit weird as an argument
here though; I prefer it as "already locked", inverting the boolean.
What do you think of this formulation?
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
https://twitter.com/thingskatedid/status/1456027786158776329
Hi,
On Mon, Aug 17, 2026 at 8:56 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
On 2026-Aug-04, Bharath Rupireddy wrote:
repack_is_permitted_for_relation() uses pg_class_aclcheck_ext() to
silently skip a concurrently-dropped relation. That's wrong for a
caller that may already hold a lock on the relation whose ACL is
checked, where missing a relation is not fine, and it makes the
single-relation REPACK cases more brittle
(/messages/by-id/akPhEffRipH4isWF@nathan). So
only detect a missing relation where that's expected, following the
fix for vacuum_is_permitted_for_relation() in commit 824d5f6.That makes sense. I think "missing OK" is a bit weird as an argument
here though; I prefer it as "already locked", inverting the boolean.
What do you think of this formulation?
Thanks. That works for me. The v3 patch LGTM. pgindent, make check,
and make check-world are all good.
--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com
On 2026-Aug-17, Bharath Rupireddy wrote:
Thanks. That works for me. The v3 patch LGTM. pgindent, make check,
and make check-world are all good.
Thanks! Pushed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Curiosity is the hope that something wondrous waits just out of sight,
for we are surrounded by a garden of miracles" (Namron)
(Marjorie Liu and Sana Takeda, Monstress: Inferno)