Date: Mon, 1 Jun 2026 09:21:39 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Robert Haas <robertmhaas@gmail.com>, Jeff Davis <pgsql@j-davis.com>,
	Roman Eskin <r.eskin@arenadata.io>,
	Michael Paquier <michael@paquier.xyz>,
	Alexander Lakhin <exclusion@gmail.com>,
	pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Avoid orphaned objects dependencies, take 3
Message-ID: <ah1PIyqtT+427dWb@bdtpg>
References: 
 <CA+TgmoZh8yXoQ2AF-VFSTswhin0o+BZ78AaOsWZskCW1GHBd6g@mail.gmail.com>
 <aEJ+oNANWutG8aDv@ip-10-97-1-34.eu-west-3.compute.internal>
 <9f5f0dd4-96d5-4d90-9436-62d96e0298f9@arenadata.io>
 <ad/awXLnMuc6Rf5M@bdtpg>
 <afCXPVNuvpf5ECGZ@bdtpg>
 <CA+TgmobtzkG9HzGtDXbgb3KVE46qHuSvGcuKHgAGQ1aejCoLLw@mail.gmail.com>
 <agsCqlLTytZCudMv@bdtpg>
 <aebf583b-b904-4035-a1c1-a4c5ad064dec@iki.fi>
 <ahYAKgbx05IQdjwR@bdtpg>
 <3963e573-4852-4d25-b741-7c5c7e1f9140@iki.fi>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="qDVpkCoSYWu3RcCv"
Content-Disposition: inline
In-Reply-To: <3963e573-4852-4d25-b741-7c5c7e1f9140@iki.fi>
Archived-At: 
 <https://www.postgresql.org/message-id/ah1PIyqtT%2B427dWb%40bdtpg>
Precedence: bulk


--qDVpkCoSYWu3RcCv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Wed, May 27, 2026 at 06:53:35PM +0300, Heikki Linnakangas wrote:
> Fixed these, and did some more copy-editing on the comments. With that,
> committed and backpatched.

Thanks!

Now that we avoid orphaned objects dependencies, I resumed working on Robert's
concern about the TOCTOU window where a REVOKE could land between the original
permission check and the dependency recording.

Based on our discussion during PGConf.dev, PFA a new patch that uses the same
approach as RangeVarGetRelidExtended(): record SharedInvalidMessageCounter at the
time of the original aclcheck, then before locking compare the current counter to
the saved value. If it changed, recheck permission before acquiring the lock.
After the lock wait, if more invalidations arrived, release and retry.

Remarks:

The tracking array lives in a dedicated AclCheckTrackContext memory context
(child of TopMemoryContext). The context is reset at the start of each
top-level utility statement, which frees all prior allocations and provides
clean lifetime management.

Recording is gated by aclcheck_tracking_active, which is set to true only
during top-level utility statement execution. This ensures DML and queries pay
no cost. The flag is cleared both at normal completion of ProcessUtility and in
AbortTransaction to handle the error path.

The patch adds a test that would fail without the TOCTOU protection in place.

Alternatives considered:

- To avoid allocating memory for each statement, keep the array in
TopMemoryContext and never free it (only resetting the count). But that left the
high-water mark allocated for the lifetime of the backend.

- Passing privilege info (roleId, mode) as extra arguments through the
dependency recording APIs (recordDependencyOn, recordMultipleDependencies,
etc.). It was discarded because expression-based dependencies (recordDependencyOnExpr,
find_expr_references_walker) discover objects by walking expression trees: the
caller never sees individual objects and cannot attach privilege info to them.

That's why I believe the tracking approach that is in the attached sounds like
a right approach. Bonus point, it would also help if we want to "ensure" that
we always do the acl check prior the dependency recording: that would avoid
cases like the one mentioned in [1] (for example, when we can create a view and
then record a dependency based on a function we don't have exec privilege on).

[1]: https://postgr.es/m/agsCqlLTytZCudMv%40bdtpg

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

--qDVpkCoSYWu3RcCv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v23-0001-Recheck-permissions-after-lock-acquisition-in-de.patch"