Re: Avoid orphaned objects dependencies, take 3

Hi,

On Mon, Jun 15, 2026 at 06:19:19PM -0700, Jeff Davis wrote:
> On Wed, 2026-06-10 at 15:16 +0000, Bertrand Drouvot wrote:
> > PFA a new version of v26, it adds a new test as compared to the v26
> > previously
> > shared.
>
> I'd like to avoid adding lines to AbortTransaction(). Also, I think it
> might miss subtransaction aborts, which could be relevant in complex
> cases with SPI.
>
> Can you use a structure like:
>
> ProcessUtility()
> {
> TrackAclTable *prevTrackAclTable = CurrentTrackAclTable;
> /* allocates in CurrentMemoryContext */
> CurrentTrackAclTable = NewTrackAclTable();
>
> PG_TRY();
> {
> ... rest of ProcessUtility ...
> }
> PG_FINALLY();
> {
> FreeTrackAclTable(CurrentTrackAclTable);
> CurrentTrackAclTable = prevTrackAclTable;
> }
> PG_END_TRY();
> }
>
> That would avoid the need to create a special memory context; you could
> just repalloc() the chunk allocated for the table. It would also mean
> you don't have to track the stack frames manually with a counter, just
> use a local variable.

The advantage of v26 is that it also checked for the outer while checking the
nested (that way a revoke that impacted the outer would fail sooner). That said,
I like your idea: it's more readable and probably less error-prone (don't use
global variable in multiple places and a dedicated counter for nested checks).

So, the attached implements it that way.
It's in 0001. The reason for multiple sub-patches:

> Also, are you sure that the two call sites for aclcheck_track_record()
> are enough? Or do we need checks in e.g. pg_attribute_aclcheck() as
> well?

Currently, the only caller of pg_attribute_aclcheck_ext() that also records
dependencies on the checked object is checkFkeyPermissions(), which already
holds ShareRowExclusiveLock on the referenced table. While ShareRowExclusiveLock
prevents direct REVOKE on the table, it does not prevent role membership revokes.
So 0003 adds pg_attribute_aclcheck_ext to ACL tracking for dependency recording,
which also protects against new DDL callers.

The remaining aclcheck functions (pg_parameter_aclcheck and
pg_largeobject_aclcheck_snapshot) do not need tracking: pg_parameter_aclcheck
checks GUC parameters which are not dependency targets, and
pg_largeobject_aclcheck_snapshot is only called from inv_open() and
has_largeobject_privilege(), neither of which records dependencies.

That said, while looking at checkFkeyPermissions(), I realized that we have a
corner case in 0001.

Indeed, checkFkeyPermissions() first tries pg_class_aclcheck() (which fails for
column-level grants), then falls through to pg_attribute_aclcheck(). But as
aclcheck_track_record() is called for both successful and failed checks, then
the tracked failed entry could trigger a 'permission denied' in recheckAcl() if
catalog invalidations arrived between the check and dependency recording.

0002: fixes it by moving aclcheck_track_record() to after the permission check
succeeds in object_aclcheck_ext() and pg_class_aclcheck_ext(). Indeed, there is
no need to track failed permission checks. It also adds a test that reproduces
the issue by injecting invalidations while the FK creation is paused after the
failed table-level check. The test would fail without the fix in 0002 applied.

0003: adds pg_attribute_aclcheck_ext to ACL tracking for dependency recording.
It adds an attnum field to AclCheckEntry so that recheckAcl() can distinguish
column-level checks from table-level checks and call the appropriate recheck
function. InvalidAttrNumber means whole-object check.

The sub-patches are to ease the review. They should probably be merged before
being pushed.

Regards,

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