Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid orphaned objects dependencies

  2. Don't try to record dependency on a dropped column's datatype

  1. Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-22T08:45:19Z

    Hi,

This new thread is a follow-up of [1] and [2].

Problem description:

We have occasionally observed objects having an orphaned dependency, the 
most common case we have seen is functions not linked to any namespaces.

Examples to produce such orphaned dependencies:

Scenario 1:

session 1: begin; drop schema schem;
session 2: create a function in the schema schem
session 1: commit;

With the above, the function created in session 2 would be linked to a non
existing schema.

Scenario 2:

session 1: begin; create a function in the schema schem
session 2: drop schema schem;
session 1: commit;

With the above, the function created in session 1 would be linked to a non
existing schema.

A patch has been initially proposed to fix this particular 
(function-to-namespace) dependency (see [1]), but there could be much 
more scenarios (like the function-to-datatype one highlighted by Gilles 
in [1] that could lead to a function having an invalid parameter datatype).

As Tom said there are dozens more cases that would need to be 
considered, and a global approach to avoid those race conditions should 
be considered instead.

A first global approach attempt has been proposed in [2] making use of a dirty
snapshot when recording the dependency. But this approach appeared to be "scary"
and it was still failing to close some race conditions (see [2] for details).

Then, Tom proposed another approach in [2] which is that "creation DDL will have
to take a lock on each referenced object that'd conflict with a lock taken by
DROP".

This is what the attached patch is trying to achieve.

It does the following:

1) A new lock (that conflicts with a lock taken by DROP) has been put in place
when the dependencies are being recorded.

Thanks to it, the drop schema in scenario 2 would be locked (resulting in an
error should session 1 committs).

2) After locking the object while recording the dependency, the patch checks
that the object still exists.

Thanks to it, session 2 in scenario 1 would be locked and would report an error
once session 1 committs (that would not be the case should session 1 abort the
transaction).

The patch also adds a few tests for some dependency cases (that would currently
produce orphaned objects):

- schema and function (as the above scenarios)
- function and type
- table and type (which is I think problematic enough, as involving a table into
the game, to fix this stuff as a whole).

[1]: https://www.postgresql.org/message-id/flat/a4f55089-7cbd-fe5d-a9bb-19adc6418ae9@darold.net#9af5cdaa9e80879beb1def3604c976e8
[2]: https://www.postgresql.org/message-id/flat/8369ff70-0e31-f194-2954-787f4d9e21dd%40amazon.com

Please note that I'm not used to with this area of the code so that the patch
might not take the option proposed by Tom the "right" way.

Adding the patch to the July CF.

Looking forward to your feedback,

Regards,

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

  2. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-04-22T10:00:00Z

    Hi Bertrand,

22.04.2024 11:45, Bertrand Drouvot wrote:
> Hi,
>
> This new thread is a follow-up of [1] and [2].
>
> Problem description:
>
> We have occasionally observed objects having an orphaned dependency, the
> most common case we have seen is functions not linked to any namespaces.
>
> ...
>
> Looking forward to your feedback,

This have reminded me of bug #17182 [1].
Unfortunately, with the patch applied, the following script:

for ((i=1;i<=100;i++)); do
   ( { for ((n=1;n<=20;n++)); do echo "DROP SCHEMA s;"; done } | psql ) >psql1.log 2>&1 &
   echo "
CREATE SCHEMA s;
CREATE FUNCTION s.func1() RETURNS int LANGUAGE SQL AS 'SELECT 1;';
CREATE FUNCTION s.func2() RETURNS int LANGUAGE SQL AS 'SELECT 2;';
CREATE FUNCTION s.func3() RETURNS int LANGUAGE SQL AS 'SELECT 3;';
CREATE FUNCTION s.func4() RETURNS int LANGUAGE SQL AS 'SELECT 4;';
CREATE FUNCTION s.func5() RETURNS int LANGUAGE SQL AS 'SELECT 5;';
   "  | psql >psql2.log 2>&1 &
   wait
   psql -c "DROP SCHEMA s CASCADE" >psql3.log
done
echo "
SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc pp
   LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL" | psql

still ends with:
server closed the connection unexpectedly
         This probably means the server terminated abnormally
         before or while processing the request.

2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|LOG:  server process (PID 1388378) was terminated by signal 11: 
Segmentation fault
2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|DETAIL:  Failed process was running: SELECT 
pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc pp
       LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL

[1] https://www.postgresql.org/message-id/flat/17182-a6baa001dd1784be%40postgresql.org

Best regards,
Alexander

  3. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-22T10:52:21Z

    Hi,

On Mon, Apr 22, 2024 at 01:00:00PM +0300, Alexander Lakhin wrote:
> Hi Bertrand,
> 
> 22.04.2024 11:45, Bertrand Drouvot wrote:
> > Hi,
> > 
> > This new thread is a follow-up of [1] and [2].
> > 
> > Problem description:
> > 
> > We have occasionally observed objects having an orphaned dependency, the
> > most common case we have seen is functions not linked to any namespaces.
> > 
> > ...
> > 
> > Looking forward to your feedback,
> 
> This have reminded me of bug #17182 [1].

Thanks for the link to the bug!

> Unfortunately, with the patch applied, the following script:
> 
> for ((i=1;i<=100;i++)); do
>   ( { for ((n=1;n<=20;n++)); do echo "DROP SCHEMA s;"; done } | psql ) >psql1.log 2>&1 &
>   echo "
> CREATE SCHEMA s;
> CREATE FUNCTION s.func1() RETURNS int LANGUAGE SQL AS 'SELECT 1;';
> CREATE FUNCTION s.func2() RETURNS int LANGUAGE SQL AS 'SELECT 2;';
> CREATE FUNCTION s.func3() RETURNS int LANGUAGE SQL AS 'SELECT 3;';
> CREATE FUNCTION s.func4() RETURNS int LANGUAGE SQL AS 'SELECT 4;';
> CREATE FUNCTION s.func5() RETURNS int LANGUAGE SQL AS 'SELECT 5;';
>   "  | psql >psql2.log 2>&1 &
>   wait
>   psql -c "DROP SCHEMA s CASCADE" >psql3.log
> done
> echo "
> SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid FROM pg_proc pp
>   LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL" | psql
> 
> still ends with:
> server closed the connection unexpectedly
>         This probably means the server terminated abnormally
>         before or while processing the request.
> 
> 2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|LOG:  server process (PID
> 1388378) was terminated by signal 11: Segmentation fault
> 2024-04-22 09:54:39.171 UTC|||662633dc.152bbc|DETAIL:  Failed process was
> running: SELECT pg_identify_object('pg_proc'::regclass, pp.oid, 0), pp.oid
> FROM pg_proc pp
>       LEFT JOIN pg_namespace pn ON pp.pronamespace = pn.oid WHERE pn.oid IS NULL
> 

Thanks for sharing the script.

That's weird, I just launched it several times with the patch applied and I'm not
able to see the seg fault (while I can see it constently failing on the master
branch).

Are you 100% sure you tested it against a binary with the patch applied?

Regards,

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

  4. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-04-22T12:00:00Z

    22.04.2024 13:52, Bertrand Drouvot wrote:
>
> That's weird, I just launched it several times with the patch applied and I'm not
> able to see the seg fault (while I can see it constently failing on the master
> branch).
>
> Are you 100% sure you tested it against a binary with the patch applied?
>

Yes, at least I can't see what I'm doing wrong. Please try my
self-contained script attached.

Best regards,
Alexander

  5. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-23T04:59:09Z

    Hi,

On Mon, Apr 22, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote:
> 22.04.2024 13:52, Bertrand Drouvot wrote:
> > 
> > That's weird, I just launched it several times with the patch applied and I'm not
> > able to see the seg fault (while I can see it constently failing on the master
> > branch).
> > 
> > Are you 100% sure you tested it against a binary with the patch applied?
> > 
> 
> Yes, at least I can't see what I'm doing wrong. Please try my
> self-contained script attached.

Thanks for sharing your script!

Yeah your script ensures the patch is applied before the repro is executed.

I do confirm that I can also see the issue with the patch applied (but I had to
launch multiple attempts, while on master one attempt is enough).

I'll have a look.

Regards,

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

  6. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-23T16:20:46Z

    Hi,

On Tue, Apr 23, 2024 at 04:59:09AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Apr 22, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote:
> > 22.04.2024 13:52, Bertrand Drouvot wrote:
> > > 
> > > That's weird, I just launched it several times with the patch applied and I'm not
> > > able to see the seg fault (while I can see it constently failing on the master
> > > branch).
> > > 
> > > Are you 100% sure you tested it against a binary with the patch applied?
> > > 
> > 
> > Yes, at least I can't see what I'm doing wrong. Please try my
> > self-contained script attached.
> 
> Thanks for sharing your script!
> 
> Yeah your script ensures the patch is applied before the repro is executed.
> 
> I do confirm that I can also see the issue with the patch applied (but I had to
> launch multiple attempts, while on master one attempt is enough).
> 
> I'll have a look.

Please find attached v2 that should not produce the issue anymore (I launched a
lot of attempts without any issues). v1 was not strong enough as it was not
always checking for the dependent object existence. v2 now always checks if the
object still exists after the additional lock acquisition attempt while recording
the dependency.

I still need to think about v2 but in the meantime could you please also give
v2 a try on you side?

Regards,

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

  7. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-24T08:38:01Z

    Hi,

On Tue, Apr 23, 2024 at 04:20:46PM +0000, Bertrand Drouvot wrote:
> Please find attached v2 that should not produce the issue anymore (I launched a
> lot of attempts without any issues). v1 was not strong enough as it was not
> always checking for the dependent object existence. v2 now always checks if the
> object still exists after the additional lock acquisition attempt while recording
> the dependency.
> 
> I still need to think about v2 but in the meantime could you please also give
> v2 a try on you side?

I gave more thought to v2 and the approach seems reasonable to me. Basically what
it does is that in case the object is already dropped before we take the new lock
(while recording the dependency) then the error message is a "generic" one (means
it does not provide the description of the "already" dropped object). I think it
makes sense to write the patch that way by abandoning the patch's ambition to
tell the description of the dropped object in all the cases.

Of course, I would be happy to hear others thought about it.

Please find v3 attached (which is v2 with more comments).

Regards,

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

  8. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-04-24T12:00:00Z

    Hi Bertrand,

24.04.2024 11:38, Bertrand Drouvot wrote:
>> Please find attached v2 that should not produce the issue anymore (I launched a
>> lot of attempts without any issues). v1 was not strong enough as it was not
>> always checking for the dependent object existence. v2 now always checks if the
>> object still exists after the additional lock acquisition attempt while recording
>> the dependency.
>>
>> I still need to think about v2 but in the meantime could you please also give
>> v2 a try on you side?
> I gave more thought to v2 and the approach seems reasonable to me. Basically what
> it does is that in case the object is already dropped before we take the new lock
> (while recording the dependency) then the error message is a "generic" one (means
> it does not provide the description of the "already" dropped object). I think it
> makes sense to write the patch that way by abandoning the patch's ambition to
> tell the description of the dropped object in all the cases.
>
> Of course, I would be happy to hear others thought about it.
>
> Please find v3 attached (which is v2 with more comments).

Thank you for the improved version!

I can confirm that it fixes that case.
I've also tested other cases that fail on master (most of them also fail
with v1), please try/look at the attached script. (There could be other
broken-dependency cases, of course, but I think I've covered all the
representative ones.)

All tested cases work correctly with v3 applied — I couldn't get broken
dependencies, though concurrent create/drop operations can still produce
the "cache lookup failed" error, which is probably okay, except that it is
an INTERNAL_ERROR, which assumed to be not easily triggered by users.

Best regards,
Alexander

  9. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-25T05:00:22Z

    Hi,

On Wed, Apr 24, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote:
> 24.04.2024 11:38, Bertrand Drouvot wrote:
> > I gave more thought to v2 and the approach seems reasonable to me. Basically what
> > it does is that in case the object is already dropped before we take the new lock
> > (while recording the dependency) then the error message is a "generic" one (means
> > it does not provide the description of the "already" dropped object). I think it
> > makes sense to write the patch that way by abandoning the patch's ambition to
> > tell the description of the dropped object in all the cases.
> > 
> > Of course, I would be happy to hear others thought about it.
> > 
> > Please find v3 attached (which is v2 with more comments).
> 
> Thank you for the improved version!
> 
> I can confirm that it fixes that case.

Great, thanks for the test!

> I've also tested other cases that fail on master (most of them also fail
> with v1), please try/look at the attached script.

Thanks for all those tests!

> (There could be other broken-dependency cases, of course, but I think I've
> covered all the representative ones.)

Agree. Furthermore the way the patch is written should be agnostic to the
object's kind that are part of the dependency. Having said that, that does not
hurt to add more tests in this patch, so v4 attached adds some of your tests (
that would fail on the master branch without the patch applied).

The way the tests are written in the patch are less "racy" that when triggered
with your script. Indeed, I think that in the patch the dependent object can not
be removed before the new lock is taken when recording the dependency. We may
want to add injection points in the game if we feel the need.

> All tested cases work correctly with v3 applied —

Yeah, same on my side, I did run them too and did not find any issues.

> I couldn't get broken
> dependencies,

Same here.

> though concurrent create/drop operations can still produce
> the "cache lookup failed" error, which is probably okay, except that it is
> an INTERNAL_ERROR, which assumed to be not easily triggered by users.

I did not see any of those "cache lookup failed" during my testing with/without
your script. During which test(s) did you see those with v3 applied?

Attached v4, simply adding more tests to v3.

Regards,

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

  10. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-04-25T06:00:00Z

    Hi,

25.04.2024 08:00, Bertrand Drouvot wrote:
>
>> though concurrent create/drop operations can still produce
>> the "cache lookup failed" error, which is probably okay, except that it is
>> an INTERNAL_ERROR, which assumed to be not easily triggered by users.
> I did not see any of those "cache lookup failed" during my testing with/without
> your script. During which test(s) did you see those with v3 applied?

You can try, for example, table-trigger, or other tests that check for
"cache lookup failed" psql output only (maybe you'll need to increase the
iteration count). For instance, I've got (with v4 applied):
2024-04-25 05:48:08.102 UTC [3638763] ERROR:  cache lookup failed for function 20893
2024-04-25 05:48:08.102 UTC [3638763] STATEMENT:  CREATE TRIGGER modified_c1 BEFORE UPDATE OF c ON t
         FOR EACH ROW WHEN (OLD.c <> NEW.c) EXECUTE PROCEDURE trigger_func('modified_c');

Or with function-function:
2024-04-25 05:52:31.390 UTC [3711897] ERROR:  cache lookup failed for function 32190 at character 54
2024-04-25 05:52:31.390 UTC [3711897] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;
--
2024-04-25 05:52:37.639 UTC [3720011] ERROR:  cache lookup failed for function 34465 at character 54
2024-04-25 05:52:37.639 UTC [3720011] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;

> Attached v4, simply adding more tests to v3.

Thank you for working on this!

Best regards,
Alexander

  11. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-04-25T07:20:05Z

    Hi,

On Thu, Apr 25, 2024 at 09:00:00AM +0300, Alexander Lakhin wrote:
> Hi,
> 
> 25.04.2024 08:00, Bertrand Drouvot wrote:
> > 
> > > though concurrent create/drop operations can still produce
> > > the "cache lookup failed" error, which is probably okay, except that it is
> > > an INTERNAL_ERROR, which assumed to be not easily triggered by users.
> > I did not see any of those "cache lookup failed" during my testing with/without
> > your script. During which test(s) did you see those with v3 applied?
> 
> You can try, for example, table-trigger, or other tests that check for
> "cache lookup failed" psql output only (maybe you'll need to increase the
> iteration count). For instance, I've got (with v4 applied):
> 2024-04-25 05:48:08.102 UTC [3638763] ERROR:  cache lookup failed for function 20893
> 2024-04-25 05:48:08.102 UTC [3638763] STATEMENT:  CREATE TRIGGER modified_c1 BEFORE UPDATE OF c ON t
>         FOR EACH ROW WHEN (OLD.c <> NEW.c) EXECUTE PROCEDURE trigger_func('modified_c');
> 
> Or with function-function:
> 2024-04-25 05:52:31.390 UTC [3711897] ERROR:  cache lookup failed for function 32190 at character 54
> 2024-04-25 05:52:31.390 UTC [3711897] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;
> --
> 2024-04-25 05:52:37.639 UTC [3720011] ERROR:  cache lookup failed for function 34465 at character 54
> 2024-04-25 05:52:37.639 UTC [3720011] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;

I see, so this is during object creation.

It's easy to reproduce this kind of issue with gdb. For example set a breakpoint
on SearchSysCache1() and during the create function f1() once it breaks on:

#0  SearchSysCache1 (cacheId=45, key1=16400) at syscache.c:221
#1  0x00005ad305beacd6 in func_get_detail (funcname=0x5ad308204d50, fargs=0x0, fargnames=0x0, nargs=0, argtypes=0x7ffff2ff9cc0, expand_variadic=true, expand_defaults=true, include_out_arguments=false, funcid=0x7ffff2ff9ba0, rettype=0x7ffff2ff9b9c, retset=0x7ffff2ff9b94, nvargs=0x7ffff2ff9ba4,
    vatype=0x7ffff2ff9ba8, true_typeids=0x7ffff2ff9bd8, argdefaults=0x7ffff2ff9be0) at parse_func.c:1622
#2  0x00005ad305be7dd0 in ParseFuncOrColumn (pstate=0x5ad30823be98, funcname=0x5ad308204d50, fargs=0x0, last_srf=0x0, fn=0x5ad308204da0, proc_call=false, location=55) at parse_func.c:266
#3  0x00005ad305bdffb0 in transformFuncCall (pstate=0x5ad30823be98, fn=0x5ad308204da0) at parse_expr.c:1474
#4  0x00005ad305bdd2ee in transformExprRecurse (pstate=0x5ad30823be98, expr=0x5ad308204da0) at parse_expr.c:230
#5  0x00005ad305bdec34 in transformAExprOp (pstate=0x5ad30823be98, a=0x5ad308204e20) at parse_expr.c:990
#6  0x00005ad305bdd1a0 in transformExprRecurse (pstate=0x5ad30823be98, expr=0x5ad308204e20) at parse_expr.c:187
#7  0x00005ad305bdd00b in transformExpr (pstate=0x5ad30823be98, expr=0x5ad308204e20, exprKind=EXPR_KIND_SELECT_TARGET) at parse_expr.c:131
#8  0x00005ad305b96b7e in transformReturnStmt (pstate=0x5ad30823be98, stmt=0x5ad308204ee0) at analyze.c:2395
#9  0x00005ad305b92213 in transformStmt (pstate=0x5ad30823be98, parseTree=0x5ad308204ee0) at analyze.c:375
#10 0x00005ad305c6321a in interpret_AS_clause (languageOid=14, languageName=0x5ad308204c40 "sql", funcname=0x5ad308204ad8 "f100", as=0x0, sql_body_in=0x5ad308204ee0, parameterTypes=0x0, inParameterNames=0x0, prosrc_str_p=0x7ffff2ffa208, probin_str_p=0x7ffff2ffa200, sql_body_out=0x7ffff2ffa210,
    queryString=0x5ad3082040b0 "CREATE FUNCTION f100() RETURNS int LANGUAGE SQL RETURN f() + 1;") at functioncmds.c:953
#11 0x00005ad305c63c93 in CreateFunction (pstate=0x5ad308186310, stmt=0x5ad308204f00) at functioncmds.c:1221

then drop function f() in another session. Then the create function f1() would:

postgres=# CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN f() + 1;
ERROR:  cache lookup failed for function 16400

This stuff does appear before we get a chance to call the new depLockAndCheckObject()
function.

I think this is what Tom was referring to in [1]:

"
So the only real fix for this would be to make every object lookup in the entire
system do the sort of dance that's done in RangeVarGetRelidExtended.
"

The fact that those kind of errors appear also somehow ensure that no orphaned
dependencies can be created.

The patch does ensure that no orphaned depencies can occur after those "initial"
look up are done (should the dependent object be dropped).

I'm tempted to not add extra complexity to avoid those kind of errors and keep the
patch as it is. All of those servicing the same goal: no orphaned depencies are
created.

[1]: https://www.postgresql.org/message-id/2872252.1630851337%40sss.pgh.pa.us

Regards,

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

  12. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-04-30T17:00:00Z

    Hi Bertrand,

25.04.2024 10:20, Bertrand Drouvot wrote:
> postgres=# CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN f() + 1;
> ERROR:  cache lookup failed for function 16400
>
> This stuff does appear before we get a chance to call the new depLockAndCheckObject()
> function.
>
> I think this is what Tom was referring to in [1]:
>
> "
> So the only real fix for this would be to make every object lookup in the entire
> system do the sort of dance that's done in RangeVarGetRelidExtended.
> "
>
> The fact that those kind of errors appear also somehow ensure that no orphaned
> dependencies can be created.

I agree; the only thing that I'd change here, is the error code.

But I've discovered yet another possibility to get a broken dependency.
Please try this script:
res=0
numclients=20
for ((i=1;i<=100;i++)); do
for ((c=1;c<=numclients;c++)); do
   echo "
CREATE SCHEMA s_$c;
CREATE CONVERSION myconv_$c FOR 'LATIN1' TO 'UTF8' FROM iso8859_1_to_utf8;
ALTER CONVERSION myconv_$c SET SCHEMA s_$c;
   " | psql >psql1-$c.log 2>&1 &
   echo "DROP SCHEMA s_$c RESTRICT;" | psql >psql2-$c.log 2>&1 &
done
wait
pg_dump -f db.dump || { echo "on iteration $i"; res=1; break; }
for ((c=1;c<=numclients;c++)); do
   echo "DROP SCHEMA s_$c CASCADE;" | psql >psql3-$c.log 2>&1
done
done
psql -c "SELECT * FROM pg_conversion WHERE connamespace NOT IN (SELECT oid FROM pg_namespace);"

It fails for me (with the v4 patch applied) as follows:
pg_dump: error: schema with OID 16392 does not exist
on iteration 1
   oid  | conname  | connamespace | conowner | conforencoding | contoencoding |      conproc      | condefault
-------+----------+--------------+----------+----------------+---------------+-------------------+------------
  16396 | myconv_6 |        16392 |       10 |              8 |             6 | iso8859_1_to_utf8 | f

Best regards,
Alexander

  13. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-09T12:20:51Z

    Hi,

On Tue, Apr 30, 2024 at 08:00:00PM +0300, Alexander Lakhin wrote:
> Hi Bertrand,
> 
> But I've discovered yet another possibility to get a broken dependency.

Thanks for the testing and the report!

> Please try this script:
> res=0
> numclients=20
> for ((i=1;i<=100;i++)); do
> for ((c=1;c<=numclients;c++)); do
>   echo "
> CREATE SCHEMA s_$c;
> CREATE CONVERSION myconv_$c FOR 'LATIN1' TO 'UTF8' FROM iso8859_1_to_utf8;
> ALTER CONVERSION myconv_$c SET SCHEMA s_$c;
>   " | psql >psql1-$c.log 2>&1 &
>   echo "DROP SCHEMA s_$c RESTRICT;" | psql >psql2-$c.log 2>&1 &
> done
> wait
> pg_dump -f db.dump || { echo "on iteration $i"; res=1; break; }
> for ((c=1;c<=numclients;c++)); do
>   echo "DROP SCHEMA s_$c CASCADE;" | psql >psql3-$c.log 2>&1
> done
> done
> psql -c "SELECT * FROM pg_conversion WHERE connamespace NOT IN (SELECT oid FROM pg_namespace);"
> 
> It fails for me (with the v4 patch applied) as follows:
> pg_dump: error: schema with OID 16392 does not exist
> on iteration 1
>   oid  | conname  | connamespace | conowner | conforencoding | contoencoding |      conproc      | condefault
> -------+----------+--------------+----------+----------------+---------------+-------------------+------------
>  16396 | myconv_6 |        16392 |       10 |              8 |             6 | iso8859_1_to_utf8 | f
> 

Thanks for sharing the test, I'm able to reproduce the issue with v4.

Oh I see, your test updates an existing dependency. v4 took care about brand new 
dependencies creation (recordMultipleDependencies()) but forgot to take care
about changing an existing dependency (which is done in another code path:
changeDependencyFor()).

Please find attached v5 that adds:

- a call to the new depLockAndCheckObject() function in changeDependencyFor().
- a test when altering an existing dependency.

With v5 applied, I don't see the issue anymore.

Regards,

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

  14. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-05-14T12:00:00Z

    Hi Bertrand,

09.05.2024 15:20, Bertrand Drouvot wrote:
> Oh I see, your test updates an existing dependency. v4 took care about brand new
> dependencies creation (recordMultipleDependencies()) but forgot to take care
> about changing an existing dependency (which is done in another code path:
> changeDependencyFor()).
>
> Please find attached v5 that adds:
>
> - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> - a test when altering an existing dependency.
>
> With v5 applied, I don't see the issue anymore.

Me too. Thank you for the improved version!
I will test the patch in the background, but for now I see no other
issues with it.

Best regards,
Alexander

  15. Re: Avoid orphaned objects dependencies, take 3

    Michael Paquier <michael@paquier.xyz> — 2024-05-15T01:14:09Z

    On Thu, May 09, 2024 at 12:20:51PM +0000, Bertrand Drouvot wrote:
> Oh I see, your test updates an existing dependency. v4 took care about brand new 
> dependencies creation (recordMultipleDependencies()) but forgot to take care
> about changing an existing dependency (which is done in another code path:
> changeDependencyFor()).
> 
> Please find attached v5 that adds:
> 
> - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> - a test when altering an existing dependency.
> 
> With v5 applied, I don't see the issue anymore.

+            if (object_description)
+                ereport(ERROR, errmsg("%s does not exist", object_description));
+            else
+                ereport(ERROR, errmsg("a dependent object does not ex

This generates an internal error code.  Is that intended?

--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec 

This introduces a module with only one single spec.  I could get
behind an extra module if splitting the tests into more specs makes
sense or if there is a restriction on installcheck.  However, for 
one spec file filed with a bunch of objects, and note that I'm OK to
let this single spec grow more for this range of tests, it seems to me
that this would be better under src/test/isolation/.

+		if (use_dirty_snapshot)
+		{
+			InitDirtySnapshot(DirtySnapshot);
+			snapshot = &DirtySnapshot;
+		}
+		else
+			snapshot = NULL;

I'm wondering if Robert has a comment about that.  It looks backwards
in a world where we try to encourage MVCC snapshots for catalog
scans (aka 568d4138c646), especially for the part related to
dependency.c and ObjectAddresses.
--
Michael

  16. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-15T08:31:43Z

    Hi,

On Wed, May 15, 2024 at 10:14:09AM +0900, Michael Paquier wrote:
> On Thu, May 09, 2024 at 12:20:51PM +0000, Bertrand Drouvot wrote:
> > Oh I see, your test updates an existing dependency. v4 took care about brand new 
> > dependencies creation (recordMultipleDependencies()) but forgot to take care
> > about changing an existing dependency (which is done in another code path:
> > changeDependencyFor()).
> > 
> > Please find attached v5 that adds:
> > 
> > - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> > - a test when altering an existing dependency.
> > 
> > With v5 applied, I don't see the issue anymore.
> 
> +            if (object_description)
> +                ereport(ERROR, errmsg("%s does not exist", object_description));
> +            else
> +                ereport(ERROR, errmsg("a dependent object does not ex
> 
> This generates an internal error code.  Is that intended?

Thanks for looking at it!

Yes, it's like when say you want to create an object in a schema that does not
exist (see get_namespace_oid()).

> --- /dev/null
> +++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec 
> 
> This introduces a module with only one single spec.  I could get
> behind an extra module if splitting the tests into more specs makes
> sense or if there is a restriction on installcheck.  However, for 
> one spec file filed with a bunch of objects, and note that I'm OK to
> let this single spec grow more for this range of tests, it seems to me
> that this would be better under src/test/isolation/.

Yeah, I was not sure about this one (the location is from take 2 mentioned
up-thread). I'll look at moving the tests to src/test/isolation/.

Regards,

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

  17. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-15T08:33:05Z

    Hi,

On Tue, May 14, 2024 at 03:00:00PM +0300, Alexander Lakhin wrote:
> Hi Bertrand,
> 
> 09.05.2024 15:20, Bertrand Drouvot wrote:
> > Oh I see, your test updates an existing dependency. v4 took care about brand new
> > dependencies creation (recordMultipleDependencies()) but forgot to take care
> > about changing an existing dependency (which is done in another code path:
> > changeDependencyFor()).
> > 
> > Please find attached v5 that adds:
> > 
> > - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> > - a test when altering an existing dependency.
> > 
> > With v5 applied, I don't see the issue anymore.
> 
> Me too. Thank you for the improved version!
> I will test the patch in the background, but for now I see no other
> issues with it.

Thanks for confirming!

Regards,

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

  18. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-15T10:31:02Z

    Hi,

On Wed, May 15, 2024 at 08:31:43AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Wed, May 15, 2024 at 10:14:09AM +0900, Michael Paquier wrote:
> > +++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec 
> > 
> > This introduces a module with only one single spec.  I could get
> > behind an extra module if splitting the tests into more specs makes
> > sense or if there is a restriction on installcheck.  However, for 
> > one spec file filed with a bunch of objects, and note that I'm OK to
> > let this single spec grow more for this range of tests, it seems to me
> > that this would be better under src/test/isolation/.
> 
> Yeah, I was not sure about this one (the location is from take 2 mentioned
> up-thread). I'll look at moving the tests to src/test/isolation/.

Please find attached v6 (only diff with v5 is moving the tests as suggested
above).

Regards,

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

  19. Re: Avoid orphaned objects dependencies, take 3

    Alexander Lakhin <exclusion@gmail.com> — 2024-05-19T08:00:00Z

    Hello Bertrand,

15.05.2024 11:31, Bertrand Drouvot wrote:
> On Wed, May 15, 2024 at 10:14:09AM +0900, Michael Paquier wrote:
>
>> +            if (object_description)
>> +                ereport(ERROR, errmsg("%s does not exist", object_description));
>> +            else
>> +                ereport(ERROR, errmsg("a dependent object does not ex
>>
>> This generates an internal error code.  Is that intended?
> Yes, it's like when say you want to create an object in a schema that does not
> exist (see get_namespace_oid()).

AFAICS, get_namespace_oid() throws not ERRCODE_INTERNAL_ERROR,
but ERRCODE_UNDEFINED_SCHEMA:

# SELECT regtype('unknown_schema.type');
ERROR:  schema "unknown_schema" does not exist
LINE 1: SELECT regtype('unknown_schema.type');
                        ^
# \echo :LAST_ERROR_SQLSTATE
3F000

Probably, it's worth to avoid ERRCODE_INTERNAL_ERROR here in light of [1]
and [2], as these errors are not that abnormal (not Assert-like).

[1] https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz
[2] https://commitfest.postgresql.org/48/4735/

Best regards,
Alexander

  20. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-21T07:22:57Z

    Hi,

On Sun, May 19, 2024 at 11:00:00AM +0300, Alexander Lakhin wrote:
> Hello Bertrand,
> 
> Probably, it's worth to avoid ERRCODE_INTERNAL_ERROR here in light of [1]
> and [2], as these errors are not that abnormal (not Assert-like).
> 
> [1] https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz
> [2] https://commitfest.postgresql.org/48/4735/
> 

Thanks for mentioning the above examples, I agree that it's worth to avoid
ERRCODE_INTERNAL_ERROR here: please find attached v7 that makes use of a new
ERRCODE: ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST 

I thought about this name as it is close enough to the already existing 
"ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST" but I'm open to suggestion too.

Regards,

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

  21. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-05-21T12:53:06Z

    On Wed, May 15, 2024 at 6:31 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> Please find attached v6 (only diff with v5 is moving the tests as suggested
> above).

I don't immediately know what to think about this patch. I've known
about this issue for a long time, but I didn't think this was how we
would fix it.

I think the problem here is basically that we don't lock namespaces
(schemas) when we're adding and removing things from the schema. So I
assumed that if we ever did something about this, what we would do
would be add a bunch of calls to lock schemas to the appropriate parts
of the code. What you've done here instead is add locking at a much
lower level - whenever we are adding a dependency on an object, we
lock the object. The advantage of that approach is that we definitely
won't miss anything. The disadvantage of that approach is that it
means we have some very low-level code taking locks, which means it's
not obvious which operations are taking what locks. Maybe it could
even result in some redundancy, like the higher-level code taking a
lock also (possibly in a different mode) and then this code taking
another one.

I haven't gone through the previous threads; it sounds like there's
already been some discussion of this, but I'm just telling you how it
strikes me on first look.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  22. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-22T10:21:34Z

    Hi,

On Tue, May 21, 2024 at 08:53:06AM -0400, Robert Haas wrote:
> On Wed, May 15, 2024 at 6:31 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Please find attached v6 (only diff with v5 is moving the tests as suggested
> > above).
> 
> I don't immediately know what to think about this patch.

Thanks for looking at it!

> I've known about this issue for a long time, but I didn't think this was how we
> would fix it.

I started initially with [1] but it was focusing on function-schema only.

Then I proposed [2] making use of a dirty snapshot when recording the dependency.
But this approach appeared to be "scary" and it was still failing to close
some race conditions.

Then, Tom proposed another approach in [2] which is that "creation DDL will have
to take a lock on each referenced object that'd conflict with a lock taken by DROP".
This is the one the current patch is trying to implement.

> What you've done here instead is add locking at a much
> lower level - whenever we are adding a dependency on an object, we
> lock the object. The advantage of that approach is that we definitely
> won't miss anything.

Right, as there is much more than the ones related to schemas, for example:

- function and arg type
- function and return type
- function and function
- domain and domain
- table and type
- server and foreign data wrapper

to name a few.

> The disadvantage of that approach is that it
> means we have some very low-level code taking locks, which means it's
> not obvious which operations are taking what locks.

Right, but the new operations are "only" the ones leading to recording or altering
a dependency.

> Maybe it could
> even result in some redundancy, like the higher-level code taking a
> lock also (possibly in a different mode) and then this code taking
> another one.

The one that is added here is in AccessShareLock mode. It could conflict with
the ones in AccessExclusiveLock means (If I'm not missing any):

- AcquireDeletionLock(): which is exactly what we want
- get_object_address()
    - get_object_address_rv()
        - ExecAlterObjectDependsStmt()
    - ExecRenameStmt()
    - ExecAlterObjectDependsStmt()
    - ExecAlterOwnerStmt()
    - RemoveObjects()
- AlterPublication()

I think there is 2 cases here:

First case: the "conflicting" lock mode is for one of our own lock then LockCheckConflicts()
would report this as a NON conflict.

Second case: the "conflicting" lock mode is NOT for one of our own lock then LockCheckConflicts()
would report a conflict. But I've the feeling that the existing code would
already lock those sessions.

One example where it would be the case:

Session 1: doing "BEGIN; ALTER FUNCTION noschemas.foo2() SET SCHEMA alterschema" would
acquire the lock in AccessExclusiveLock during ExecAlterObjectSchemaStmt()->get_object_address()->LockDatabaseObject()
(in the existing code and before the new call that would occur through changeDependencyFor()->depLockAndCheckObject()
with the patch in place).

Then, session 2: doing "alter function noschemas.foo2() owner to newrole;"
would be locked in the existing code while doing ExecAlterOwnerStmt()->get_object_address()->LockDatabaseObject()).

Means that in this example, the new lock this patch is introducing would not be
responsible of session 2 beging locked.

Was your concern about "First case" or "Second case" or both?

[1]: https://www.postgresql.org/message-id/flat/5a9daaae-5538-b209-6279-e903c3ea2157%40amazon.com
[2]: https://www.postgresql.org/message-id/flat/8369ff70-0e31-f194-2954-787f4d9e21dd%40amazon.com

Regards,

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

  23. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-05-22T14:48:12Z

    On Wed, May 22, 2024 at 6:21 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> I started initially with [1] but it was focusing on function-schema only.

Yeah, that's what I thought we would want to do. And then just extend
that to the other cases.

> Then I proposed [2] making use of a dirty snapshot when recording the dependency.
> But this approach appeared to be "scary" and it was still failing to close
> some race conditions.

The current patch still seems to be using dirty snapshots for some
reason, which struck me as a bit odd. My intuition is that if we're
relying on dirty snapshots to solve problems, we likely haven't solved
the problems correctly, which seems consistent with your statement
about "failing to close some race conditions". But I don't think I
understand the situation well enough to be sure just yet.

> Then, Tom proposed another approach in [2] which is that "creation DDL will have
> to take a lock on each referenced object that'd conflict with a lock taken by DROP".
> This is the one the current patch is trying to implement.

It's a clever idea, but I'm not sure that I like it.

> I think there is 2 cases here:
>
> First case: the "conflicting" lock mode is for one of our own lock then LockCheckConflicts()
> would report this as a NON conflict.
>
> Second case: the "conflicting" lock mode is NOT for one of our own lock then LockCheckConflicts()
> would report a conflict. But I've the feeling that the existing code would
> already lock those sessions.
>
> Was your concern about "First case" or "Second case" or both?

The second case, I guess. It's bad to take a weaker lock first and
then a stronger lock on the same object later, because it can lead to
deadlocks that would have been avoided if the stronger lock had been
taken at the outset. Here, it seems like things would happen in the
other order: if we took two locks, we'd probably take the stronger
lock in the higher-level code and then the weaker lock in the
dependency code. That shouldn't break anything; it's just a bit
inefficient. My concern was really more about the maintainability of
the code. I fear that if we add code that takes heavyweight locks in
surprising places, we might later find the behavior difficult to
reason about.

Tom, what is your thought about that concern?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  24. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-23T04:19:36Z

    Hi,

On Wed, May 22, 2024 at 10:48:12AM -0400, Robert Haas wrote:
> On Wed, May 22, 2024 at 6:21 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > I started initially with [1] but it was focusing on function-schema only.
> 
> Yeah, that's what I thought we would want to do. And then just extend
> that to the other cases.
> 
> > Then I proposed [2] making use of a dirty snapshot when recording the dependency.
> > But this approach appeared to be "scary" and it was still failing to close
> > some race conditions.
> 
> The current patch still seems to be using dirty snapshots for some
> reason, which struck me as a bit odd. My intuition is that if we're
> relying on dirty snapshots to solve problems, we likely haven't solved
> the problems correctly, which seems consistent with your statement
> about "failing to close some race conditions". But I don't think I
> understand the situation well enough to be sure just yet.

The reason why we are using a dirty snapshot here is for the cases where we are
recording a dependency on a referenced object that we are creating at the same
time behind the scene (for example, creating a composite type while creating
a relation). Without the dirty snapshot, then the object we are creating behind
the scene (the composite type) would not be visible and we would wrongly assume
that it has been dropped.

Note that the usage of the dirty snapshot is only when the object is first
reported as "non existing" by the new ObjectByIdExist() function.

> > I think there is 2 cases here:
> >
> > First case: the "conflicting" lock mode is for one of our own lock then LockCheckConflicts()
> > would report this as a NON conflict.
> >
> > Second case: the "conflicting" lock mode is NOT for one of our own lock then LockCheckConflicts()
> > would report a conflict. But I've the feeling that the existing code would
> > already lock those sessions.
> >
> > Was your concern about "First case" or "Second case" or both?
> 
> The second case, I guess. It's bad to take a weaker lock first and
> then a stronger lock on the same object later, because it can lead to
> deadlocks that would have been avoided if the stronger lock had been
> taken at the outset.

In the example I shared up-thread that would be the opposite: the Session 1 would
take an AccessExclusiveLock lock on the object before taking an AccessShareLock
during changeDependencyFor().

> Here, it seems like things would happen in the
> other order: if we took two locks, we'd probably take the stronger
> lock in the higher-level code and then the weaker lock in the
> dependency code.

Yeah, I agree.

> That shouldn't break anything; it's just a bit
> inefficient.

Yeah, the second lock is useless in that case (like in the example up-thread).

> My concern was really more about the maintainability of
> the code. I fear that if we add code that takes heavyweight locks in
> surprising places, we might later find the behavior difficult to
> reason about.
>

I think I understand your concern about code maintainability but I'm not sure
that adding locks while recording a dependency is that surprising.

> Tom, what is your thought about that concern?

+1

Regards,

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

  25. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-05-23T18:10:54Z

    On Thu, May 23, 2024 at 12:19 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> The reason why we are using a dirty snapshot here is for the cases where we are
> recording a dependency on a referenced object that we are creating at the same
> time behind the scene (for example, creating a composite type while creating
> a relation). Without the dirty snapshot, then the object we are creating behind
> the scene (the composite type) would not be visible and we would wrongly assume
> that it has been dropped.

The usual reason for using a dirty snapshot is that you want to see
uncommitted work by other transactions. It sounds like you're saying
you just need to see uncommitted work by the same transaction. If
that's true, I think using HeapTupleSatisfiesSelf would be clearer. Or
maybe we just need to put CommandCounterIncrement() calls in the right
places to avoid having the problem in the first place. Or maybe this
is another sign that we're doing the work at the wrong level.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  26. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-05-23T21:12:28Z

    Hi,

On Thu, May 23, 2024 at 02:10:54PM -0400, Robert Haas wrote:
> On Thu, May 23, 2024 at 12:19 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > The reason why we are using a dirty snapshot here is for the cases where we are
> > recording a dependency on a referenced object that we are creating at the same
> > time behind the scene (for example, creating a composite type while creating
> > a relation). Without the dirty snapshot, then the object we are creating behind
> > the scene (the composite type) would not be visible and we would wrongly assume
> > that it has been dropped.
> 
> The usual reason for using a dirty snapshot is that you want to see
> uncommitted work by other transactions. It sounds like you're saying
> you just need to see uncommitted work by the same transaction.

Right.

> If that's true, I think using HeapTupleSatisfiesSelf would be clearer.

Oh thanks! I did not know about the SNAPSHOT_SELF snapshot type (I should have
check all the snapshot types first though) and that's exactly what is needed here.

Please find attached v8 making use of SnapshotSelf instead of a dirty snapshot.

Regards,

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

  27. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-06T05:56:14Z

    Hi,

On Thu, May 23, 2024 at 02:10:54PM -0400, Robert Haas wrote:
> On Thu, May 23, 2024 at 12:19 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > The reason why we are using a dirty snapshot here is for the cases where we are
> > recording a dependency on a referenced object that we are creating at the same
> > time behind the scene (for example, creating a composite type while creating
> > a relation). Without the dirty snapshot, then the object we are creating behind
> > the scene (the composite type) would not be visible and we would wrongly assume
> > that it has been dropped.
> 
> The usual reason for using a dirty snapshot is that you want to see
> uncommitted work by other transactions. It sounds like you're saying
> you just need to see uncommitted work by the same transaction. If
> that's true, I think using HeapTupleSatisfiesSelf would be clearer. Or
> maybe we just need to put CommandCounterIncrement() calls in the right
> places to avoid having the problem in the first place. Or maybe this
> is another sign that we're doing the work at the wrong level.

Thanks for having discussed your concern with Tom last week during pgconf.dev
and shared the outcome to me. I understand your concern regarding code
maintainability with the current approach.

Please find attached v9 that:

- Acquire the lock and check for object existence at an upper level, means before
calling recordDependencyOn() and recordMultipleDependencies().

- Get rid of the SNAPSHOT_SELF snapshot usage and relies on
CommandCounterIncrement() instead to ensure new entries are visible when
we check for object existence (for the cases where we create additional object
behind the scene: like composite type while creating a relation).

- Add an assertion in recordMultipleDependencies() to ensure that we locked the
object before recording the dependency (to ensure we don't miss any cases now that
the lock is acquired at an upper level).

A few remarks:

My first attempt has been to move eliminate_duplicate_dependencies() out of
record_object_address_dependencies() so that we get the calls in this order:

eliminate_duplicate_dependencies()
depLockAndCheckObjects()
record_object_address_dependencies()

What I'm doing instead in v9 is to rename record_object_address_dependencies()
to lock_record_object_address_dependencies() and add depLockAndCheckObjects()
in it at the right place. That way the caller of
[lock_]record_object_address_dependencies() is not responsible of calling
eliminate_duplicate_dependencies() (which would have been the case with my first
attempt).

We need to setup the LOCKTAG before calling the new Assert in
recordMultipleDependencies(). So, using "#ifdef USE_ASSERT_CHECKING" here to
not setup the LOCKTAG on a non Assert enabled build.

v9 is more invasive (as it changes code in much more places) than v8 but it is
easier to follow (as it is now clear where the new lock is acquired).

Regards,

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

  28. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-06-06T20:00:23Z

    On Thu, Jun 6, 2024 at 1:56 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> v9 is more invasive (as it changes code in much more places) than v8 but it is
> easier to follow (as it is now clear where the new lock is acquired).

Hmm, this definitely isn't what I had in mind. Possibly that's a sign
that what I had in mind was dumb, but for sure it's not what I
imagined. What I thought you were going to do was add calls like
LockDatabaseObject(NamespaceRelationId, schemaid, 0, AccessShareLock)
in various places, or perhaps LockRelationOid(reloid,
AccessShareLock), or whatever the case may be. Here you've got stuff
like this:

- record_object_address_dependencies(&conobject, addrs_auto,
-    DEPENDENCY_AUTO);
+ lock_record_object_address_dependencies(&conobject, addrs_auto,
+ DEPENDENCY_AUTO);

...which to me looks like the locking is still pushed down inside the
dependency code.

And you also have stuff like this:

  ObjectAddressSet(referenced, RelationRelationId, childTableId);
+ depLockAndCheckObject(&referenced);
  recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_SEC);

But in depLockAndCheckObject you have:

+ if (object->classId == RelationRelationId || object->classId ==
AuthMemRelationId)
+ return;

That doesn't seem right, because then it seems like the call isn't
doing anything, but there isn't really any reason for it to not be
doing anything. If we're dropping a dependency on a table, then it
seems like we need to have a lock on that table. Presumably the reason
why we don't end up with dangling dependencies in such cases now is
because we're careful about doing LockRelation() in the right places,
but we're not similarly careful about other operations e.g.
ConstraintSetParentConstraint is called by DefineIndex which calls
table_open(childRelId, ...) first, but there's no logic in DefineIndex
to lock the constraint.

Thoughts?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  29. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-07T08:41:36Z

    Hi,

On Thu, Jun 06, 2024 at 04:00:23PM -0400, Robert Haas wrote:
> On Thu, Jun 6, 2024 at 1:56 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > v9 is more invasive (as it changes code in much more places) than v8 but it is
> > easier to follow (as it is now clear where the new lock is acquired).
> 
> Hmm, this definitely isn't what I had in mind. Possibly that's a sign
> that what I had in mind was dumb, but for sure it's not what I
> imagined. What I thought you were going to do was add calls like
> LockDatabaseObject(NamespaceRelationId, schemaid, 0, AccessShareLock)
> in various places, or perhaps LockRelationOid(reloid,
> AccessShareLock), or whatever the case may be.

I see what you’re saying, doing things like:

LockDatabaseObject(TypeRelationId, returnType, 0, AccessShareLock);
in ProcedureCreate() for example.

> Here you've got stuff
> like this:
> 
> - record_object_address_dependencies(&conobject, addrs_auto,
> -    DEPENDENCY_AUTO);
> + lock_record_object_address_dependencies(&conobject, addrs_auto,
> + DEPENDENCY_AUTO);
> 
> ...which to me looks like the locking is still pushed down inside the
> dependency code.

Yes but it’s now located in places where, I think, it’s easier to understand
what’s going on (as compare to v8), except maybe for:

recordDependencyOnExpr()
makeOperatorDependencies()
GenerateTypeDependencies()
makeParserDependencies()
makeDictionaryDependencies()
makeTSTemplateDependencies()
makeConfigurationDependencies()

but probably for:

heap_create_with_catalog()
StorePartitionKey()
index_create()
AggregateCreate()
CastCreate()
CreateConstraintEntry()
ProcedureCreate()
RangeCreate()
InsertExtensionTuple()
CreateTransform()
CreateProceduralLanguage()

The reasons I keep it linked to the dependency code are:

- To ensure we don’t miss anything (well, with the new Assert in place that’s
probably a tangential argument)

- It’s not only about locking the object: it’s also about 1) verifying the object
is pinned, 2) checking it still exists and 3) provide a description in the error
message if we can (in case the object does not exist anymore). Relying on an
already build object (in the dependency code) avoid to 1) define the object(s)
one more time or 2) create new functions that would do the same as isObjectPinned()
and getObjectDescription() with a different set of arguments.

That may sounds like weak arguments but it has been my reasoning.

Do you still find the code hard to maintain with v9?

> 
> And you also have stuff like this:
> 
>   ObjectAddressSet(referenced, RelationRelationId, childTableId);
> + depLockAndCheckObject(&referenced);
>   recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_SEC);
> 
> But in depLockAndCheckObject you have:
> 
> + if (object->classId == RelationRelationId || object->classId ==
> AuthMemRelationId)
> + return;
> 
> That doesn't seem right, because then it seems like the call isn't
> doing anything, but there isn't really any reason for it to not be
> doing anything. If we're dropping a dependency on a table, then it
> seems like we need to have a lock on that table. Presumably the reason
> why we don't end up with dangling dependencies in such cases now is
> because we're careful about doing LockRelation() in the right places,

Yeah, that's what I think: we're already careful when we deal with relations.

> but we're not similarly careful about other operations e.g.
> ConstraintSetParentConstraint is called by DefineIndex which calls
> table_open(childRelId, ...) first, but there's no logic in DefineIndex
> to lock the constraint.

table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
already. Not sure I understand your concern here.

Regards,

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

  30. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-06-13T14:49:34Z

    On Fri, Jun 7, 2024 at 4:41 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> Do you still find the code hard to maintain with v9?

I don't think it substantially changes my concerns as compared with
the earlier version.

> > but we're not similarly careful about other operations e.g.
> > ConstraintSetParentConstraint is called by DefineIndex which calls
> > table_open(childRelId, ...) first, but there's no logic in DefineIndex
> > to lock the constraint.
>
> table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
> already. Not sure I understand your concern here.

I believe this is not true. This would take a lock on the table, not
the constraint itself.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  31. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-13T16:52:09Z

    Hi,

On Thu, Jun 13, 2024 at 10:49:34AM -0400, Robert Haas wrote:
> On Fri, Jun 7, 2024 at 4:41 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Do you still find the code hard to maintain with v9?
> 
> I don't think it substantially changes my concerns as compared with
> the earlier version.

Thanks for the feedback, I'll give it more thoughts.

> 
> > > but we're not similarly careful about other operations e.g.
> > > ConstraintSetParentConstraint is called by DefineIndex which calls
> > > table_open(childRelId, ...) first, but there's no logic in DefineIndex
> > > to lock the constraint.
> >
> > table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
> > already. Not sure I understand your concern here.
> 
> I believe this is not true. This would take a lock on the table, not
> the constraint itself.

I agree that it would not lock the constraint itself. What I meant to say is that
, nevertheless, the constraint can not be dropped. Indeed, the "ALTER TABLE"
necessary to drop the constraint (ALTER TABLE <childRelId> DROP CONSTRAINT) would
be locked by the table_open(childRelId, ...).

That's why I don't understand your concern with this particular example. But
anyway, I'll double check your related concern:

+ if (object->classId == RelationRelationId || object->classId ==
AuthMemRelationId)
+ return;

in depLockAndCheckObject(). 

Regards,

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

  32. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-06-13T18:27:45Z

    On Thu, Jun 13, 2024 at 12:52 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> > > table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
> > > already. Not sure I understand your concern here.
> >
> > I believe this is not true. This would take a lock on the table, not
> > the constraint itself.
>
> I agree that it would not lock the constraint itself. What I meant to say is that
> , nevertheless, the constraint can not be dropped. Indeed, the "ALTER TABLE"
> necessary to drop the constraint (ALTER TABLE <childRelId> DROP CONSTRAINT) would
> be locked by the table_open(childRelId, ...).

Ah, right. So, I was assuming that, with either this version of your
patch or the earlier version, we'd end up locking the constraint
itself. Was I wrong about that?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  33. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-14T07:54:52Z

    Hi,

On Thu, Jun 13, 2024 at 02:27:45PM -0400, Robert Haas wrote:
> On Thu, Jun 13, 2024 at 12:52 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > > > table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
> > > > already. Not sure I understand your concern here.
> > >
> > > I believe this is not true. This would take a lock on the table, not
> > > the constraint itself.
> >
> > I agree that it would not lock the constraint itself. What I meant to say is that
> > , nevertheless, the constraint can not be dropped. Indeed, the "ALTER TABLE"
> > necessary to drop the constraint (ALTER TABLE <childRelId> DROP CONSTRAINT) would
> > be locked by the table_open(childRelId, ...).
> 
> Ah, right. So, I was assuming that, with either this version of your
> patch or the earlier version, we'd end up locking the constraint
> itself. Was I wrong about that?

The child contraint itself is not locked when going through
ConstraintSetParentConstraint().

While at it, let's look at a full example and focus on your concern.

Let's do that with this gdb file:

"
$ cat gdb.txt
b dependency.c:1542
command 1
        printf "Will return for: classId %d and objectId %d\n", object->classId, object->objectId
        c
end
b dependency.c:1547 if object->classId == 2606
command 2
        printf "Will lock constraint: classId %d and objectId %d\n", object->classId, object->objectId
        c
end
"

knowing that:

"
Line 1542 is the return here in depLockAndCheckObject() (your concern):

    if (object->classId == RelationRelationId || object->classId == AuthMemRelationId)
        return;

Line 1547 is the lock here in depLockAndCheckObject():

    /* assume we should lock the whole object not a sub-object */
    LockDatabaseObject(object->classId, object->objectId, 0, AccessShareLock);
"

So, with gdb attached to a session let's:

1. Create the parent relation

CREATE TABLE upsert_test (
    a   INT,
    b   TEXT
) PARTITION BY LIST (a);

gdb produces:

---
Will return for: classId 1259 and objectId 16384
Will return for: classId 1259 and objectId 16384
---

Oid 16384 is upsert_test, so I think the return (dependency.c:1542) is fine as
we are creating the object (it can't be dropped as not visible to anyone else).

2. Create another relation (will be the child)

CREATE TABLE upsert_test_2 (b TEXT, a int);

gdb produces:

---
Will return for: classId 1259 and objectId 16391
Will return for: classId 1259 and objectId 16394
Will return for: classId 1259 and objectId 16394
Will return for: classId 1259 and objectId 16391
---

Oid 16391 is upsert_test_2
Oid 16394 is pg_toast_16391

so I think the return (dependency.c:1542) is fine as we are creating those
objects (can't be dropped as not visible to anyone else).

3. Attach the partition

ALTER TABLE upsert_test ATTACH PARTITION upsert_test_2 FOR VALUES IN (2);

gdb produces:

---
Will return for: classId 1259 and objectId 16384
---

That's fine because we'd already had locked the relation 16384 through
AlterTableLookupRelation()->RangeVarGetRelidExtended()->LockRelationOid().

4. Add a constraint on the child relation

ALTER TABLE upsert_test_2 add constraint bdtc2 UNIQUE (a);

gdb produces:

---
Will return for: classId 1259 and objectId 16391
Will lock constraint: classId 2606 and objectId 16397
---

That's fine because we'd already had locked the relation 16391 through
AlterTableLookupRelation()->RangeVarGetRelidExtended()->LockRelationOid().

Oid 16397 is the constraint we're creating (bdtc2).

5. Add a constraint on the parent relation (this goes through
ConstraintSetParentConstraint())

ALTER TABLE upsert_test add constraint bdtc1 UNIQUE (a);

gdb produces:

---
Will return for: classId 1259 and objectId 16384
Will lock constraint: classId 2606 and objectId 16399
Will return for: classId 1259 and objectId 16398
Will return for: classId 1259 and objectId 16391
Will lock constraint: classId 2606 and objectId 16399
Will return for: classId 1259 and objectId 16391
---

Regarding "Will return for: classId 1259 and objectId 16384":
That's fine because we'd already had locked the relation 16384 through
AlterTableLookupRelation()->RangeVarGetRelidExtended()->LockRelationOid().

Regarding "Will lock constraint: classId 2606 and objectId 16399":
Oid 16399 is the constraint that we're creating.

Regarding "Will return for: classId 1259 and objectId 16398":
That's fine because Oid 16398 is an index that we're creating while creating
the constraint (so the index can't be dropped as not visible to anyone else).

Regarding "Will return for: classId 1259 and objectId 16391":
That's fine because 16384 we'd be already locked (as mentioned above). And
I think that's fine because trying to drop "upsert_test_2" (aka 16391) would produce
RemoveRelations()->RangeVarGetRelidExtended()->RangeVarCallbackForDropRelation()
->LockRelationOid(relid=16384, lockmode=8) and so would be locked.

Regarding this example, I don't think that the return in depLockAndCheckObject():

"
if (object->classId == RelationRelationId || object->classId == AuthMemRelationId)
     return;
"

is an issue. Indeed, the example above shows it would return for an object that
we'd be creating (so not visible to anyone else) or for an object that we'd
already have locked.

Is it an issue outside of this example?: I've the feeling it's not as we're
already careful when we deal with relations. That said, to be on the safe side
we could get rid of this return and make use of LockRelationOid() instead.

Regards,

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

  34. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-17T10:50:56Z

    Hi,

On Thu, Jun 13, 2024 at 04:52:09PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Jun 13, 2024 at 10:49:34AM -0400, Robert Haas wrote:
> > On Fri, Jun 7, 2024 at 4:41 AM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > > Do you still find the code hard to maintain with v9?
> > 
> > I don't think it substantially changes my concerns as compared with
> > the earlier version.
> 
> Thanks for the feedback, I'll give it more thoughts.

Please find attached v10 that puts the object locking outside of the dependency
code.

It's done that way except for:

recordDependencyOnExpr() 
recordDependencyOnSingleRelExpr()
makeConfigurationDependencies()

The reason is that I think that it would need part of the logic that his inside
the above functions to be duplicated and I'm not sure that's worth it.

For example, we would probably need to:

- make additional calls to find_expr_references_walker() 
- make additional scan on the config map

It's also not done outside of recordDependencyOnCurrentExtension() as:

1. I think it is clear enough that way (as it is clear that the lock is taken on
a ExtensionRelationId object class).
2. why to include "commands/extension.h" in more places (locking would
depend of "creating_extension" and "CurrentExtensionObject"), while 1.?

Remarks:

1. depLockAndCheckObject() and friends in v9 have been renamed to
LockNotPinnedObject() and friends (as the vast majority of their calls are now
done outside of the dependency code).

2. regarding the concern around RelationRelationId (discussed in [1]), v10 adds
a comment "XXX Do we need a lock for RelationRelationId?" at the places we
may want to lock this object class. I did not think about it yet (but will do),
I only added this comment at multiple places.

I think that v10 is easier to follow (as compare to v9) as we can easily see for
which object class we'll put a lock on.

Thoughts?

[1]: https://www.postgresql.org/message-id/Zmv3TPfJAyQXhIdu%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

  35. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-06-17T16:24:46Z

    On Fri, Jun 14, 2024 at 3:54 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> > Ah, right. So, I was assuming that, with either this version of your
> > patch or the earlier version, we'd end up locking the constraint
> > itself. Was I wrong about that?
>
> The child contraint itself is not locked when going through
> ConstraintSetParentConstraint().
>
> While at it, let's look at a full example and focus on your concern.

I'm not at the point of having a concern yet, honestly. I'm trying to
understand the design ideas. The commit message just says that we take
a conflicting lock, but it doesn't mention which object types that
principle does or doesn't apply to. I think the idea of skipping it
for cases where it's redundant with the relation lock could be the
right idea, but if that's what we're doing, don't we need to explain
the principle somewhere? And shouldn't we also apply it across all
object types that have the same property?

Along the same lines:

+ /*
+ * Those don't rely on LockDatabaseObject() when being dropped (see
+ * AcquireDeletionLock()). Also it looks like they can not produce
+ * orphaned dependent objects when being dropped.
+ */
+ if (object->classId == RelationRelationId || object->classId ==
AuthMemRelationId)
+ return;

"It looks like X cannot happen" is not confidence-inspiring. At the
very least, a better comment is needed here. But also, that relation
has no exception for AuthMemRelationId, only for RelationRelationId.
And also, the exception for RelationRelationId doesn't imply that we
don't need a conflicting lock here: the special case for
RelationRelationId in AcquireDeletionLock() is necessary because the
lock tag format is different for relations than for other database
objects, not because we don't need a lock at all. If the handling here
were really symmetric with what AcquireDeletionLock(), the coding
would be to call either LockRelationOid() or LockDatabaseObject()
depending on whether classid == RelationRelationId. Now, that isn't
actually necessary, because we already have relation-locking calls
elsewhere, but my point is that the rationale this commit gives for
WHY it isn't necessary seems to me to be wrong in multiple ways.

So to try to sum up here: I'm not sure I agree with this design. But I
also feel like the design is not as clear and consistently implemented
as it could be. So even if I just ignored the question of whether it's
the right design, it feels like we're a ways from having something
potentially committable here, because of issues like the ones I
mentioned in the last paragraph.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  36. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-17T17:57:05Z

    Hi,

On Mon, Jun 17, 2024 at 12:24:46PM -0400, Robert Haas wrote:
> On Fri, Jun 14, 2024 at 3:54 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > > Ah, right. So, I was assuming that, with either this version of your
> > > patch or the earlier version, we'd end up locking the constraint
> > > itself. Was I wrong about that?
> >
> > The child contraint itself is not locked when going through
> > ConstraintSetParentConstraint().
> >
> > While at it, let's look at a full example and focus on your concern.
> 
> I'm not at the point of having a concern yet, honestly. I'm trying to
> understand the design ideas. The commit message just says that we take
> a conflicting lock, but it doesn't mention which object types that
> principle does or doesn't apply to. I think the idea of skipping it
> for cases where it's redundant with the relation lock could be the
> right idea, but if that's what we're doing, don't we need to explain
> the principle somewhere? And shouldn't we also apply it across all
> object types that have the same property?

Yeah, I still need to deeply study this area and document it. 

> Along the same lines:
> 
> + /*
> + * Those don't rely on LockDatabaseObject() when being dropped (see
> + * AcquireDeletionLock()). Also it looks like they can not produce
> + * orphaned dependent objects when being dropped.
> + */
> + if (object->classId == RelationRelationId || object->classId ==
> AuthMemRelationId)
> + return;
> 
> "It looks like X cannot happen" is not confidence-inspiring.

Yeah, it is not. It is just a "feeling" that I need to work on to remove
any ambiguity and/or adjust the code as needed.

> At the
> very least, a better comment is needed here. But also, that relation
> has no exception for AuthMemRelationId, only for RelationRelationId.
> And also, the exception for RelationRelationId doesn't imply that we
> don't need a conflicting lock here: the special case for
> RelationRelationId in AcquireDeletionLock() is necessary because the
> lock tag format is different for relations than for other database
> objects, not because we don't need a lock at all. If the handling here
> were really symmetric with what AcquireDeletionLock(), the coding
> would be to call either LockRelationOid() or LockDatabaseObject()
> depending on whether classid == RelationRelationId.

Agree.

> Now, that isn't
> actually necessary, because we already have relation-locking calls
> elsewhere, but my point is that the rationale this commit gives for
> WHY it isn't necessary seems to me to be wrong in multiple ways.

Agree. I'm not done with that part yet (should have made it more clear).

> So to try to sum up here: I'm not sure I agree with this design. But I
> also feel like the design is not as clear and consistently implemented
> as it could be. So even if I just ignored the question of whether it's
> the right design, it feels like we're a ways from having something
> potentially committable here, because of issues like the ones I
> mentioned in the last paragraph.
> 

Agree. I'll now move on with the "XXX Do we need a lock for RelationRelationId?"
comments that I put in v10 (see [1]) and study all the cases around them.

Once done, I think that it will easier to 1.remove ambiguity, 2.document and
3.do the "right" thing regarding the RelationRelationId object class.

[1]: https://www.postgresql.org/message-id/ZnAVEBhlGvpDDVOD%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

  37. Re: Avoid orphaned objects dependencies, take 3

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2024-06-19T11:49:28Z

    Hi,

If the dependency is more, this can hit max_locks_per_transaction
limit very fast. Won't it? I just tried this little experiment with
and without patch.

1) created some UDTs (I have just chosen some random number, 15)
do $$
declare
    i int := 1;
    type_name text;
begin
    while i <= 15 loop
        type_name := format('ct_%s', i);

        -- check if the type already exists
        if not exists (
            select 1
            from pg_type
            where typname = type_name
        ) then
            execute format('create type %I as (f1 INT, f2 TEXT);', type_name);
        end if;

        i := i + 1;
    end loop;
end $$;

2) started a transaction and tried creating a table that uses all udts
created above:
begin;
create table dep_tab(a ct_1, b ct_2, c ct_3, d ct_4, e ct_5, f ct_6, g
ct_7, h ct_8, i ct_9, j ct_10, k ct_11, l ct_12, m ct_13, n ct_14, o
ct_15);

3) checked the pg_locks entries inside the transaction both with and
without patch:

-- with patch:
select count(*) from pg_locks;
 count
-------
    23
(1 row)

-- without patch:
select count(*) from pg_locks;
 count
-------
     7
(1 row)

With patch, it increased by 3 times. Won't that create a problem if
many concurrent sessions engage in similar activity?

--
With Regards,
Ashutosh Sharma.

  38. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2024-06-19T12:20:08Z

    On Wed, Jun 19, 2024 at 7:49 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> If the dependency is more, this can hit max_locks_per_transaction
> limit very fast.

Your experiment doesn't support this conclusion. Very few users would
have 15 separate user-defined types in the same table, and even if
they did, and dropped the table, using 23 locks is no big deal. By
default, max_locks_per_transaction is 64, so the user would need to
have more like 45 separate user-defined types in the same table in
order to use more than 64 locks. So, yes, it is possible that if every
backend in the system were simultaneously trying to drop a table and
all of those tables had an average of at least 45 or so user-defined
types, all different from each other, you might run out of lock table
space.

But probably nobody will ever do that in real life, and if they did,
they could just raise max_locks_per_transaction.

When posting about potential problems like this, it is a good idea to
first do a careful thought experiment to assess how realistic the
problem is. I would consider an issue like this serious if there were
a realistic scenario under which a small number of backends could
exhaust the lock table for the whole system, but I think you can see
that this isn't the case here. Even your original scenario is more
extreme than what most people are likely to hit in real life, and it
only uses 23 locks.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  39. Re: Avoid orphaned objects dependencies, take 3

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2024-06-19T13:35:36Z

    Hi Robert,

On Wed, Jun 19, 2024 at 5:50 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Jun 19, 2024 at 7:49 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > If the dependency is more, this can hit max_locks_per_transaction
> > limit very fast.
>
> Your experiment doesn't support this conclusion. Very few users would
> have 15 separate user-defined types in the same table, and even if
> they did, and dropped the table, using 23 locks is no big deal. By
> default, max_locks_per_transaction is 64, so the user would need to
> have more like 45 separate user-defined types in the same table in
> order to use more than 64 locks. So, yes, it is possible that if every
> backend in the system were simultaneously trying to drop a table and
> all of those tables had an average of at least 45 or so user-defined
> types, all different from each other, you might run out of lock table
> space.
>
> But probably nobody will ever do that in real life, and if they did,
> they could just raise max_locks_per_transaction.
>
> When posting about potential problems like this, it is a good idea to
> first do a careful thought experiment to assess how realistic the
> problem is. I would consider an issue like this serious if there were
> a realistic scenario under which a small number of backends could
> exhaust the lock table for the whole system, but I think you can see
> that this isn't the case here. Even your original scenario is more
> extreme than what most people are likely to hit in real life, and it
> only uses 23 locks.
>

I agree that based on the experiment I shared (which is somewhat
unrealistic), this doesn't seem to have any significant implications.
However, I was concerned that it could potentially increase the usage
of max_locks_per_transaction, which is why I wanted to mention it
here. Nonetheless, my experiment did not reveal any serious issues
related to this. Sorry for the noise.

--
With Regards,
Ashutosh Sharma.

  40. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-19T14:11:50Z

    Hi,

On Mon, Jun 17, 2024 at 05:57:05PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Jun 17, 2024 at 12:24:46PM -0400, Robert Haas wrote:
> > So to try to sum up here: I'm not sure I agree with this design. But I
> > also feel like the design is not as clear and consistently implemented
> > as it could be. So even if I just ignored the question of whether it's
> > the right design, it feels like we're a ways from having something
> > potentially committable here, because of issues like the ones I
> > mentioned in the last paragraph.
> > 
> 
> Agree. I'll now move on with the "XXX Do we need a lock for RelationRelationId?"
> comments that I put in v10 (see [1]) and study all the cases around them.

A. I went through all of them, did some testing for all, and reached the
conclusion that we must be in one of the two following cases that would already
prevent the relation to be dropped:

1. The relation is already locked (could be an existing relation or a relation
that we are creating).

2. The relation is protected indirectly (i.e an index protected by a lock on
its table, a table protected by a lock on a function that depends of
the table...).

So we don't need to add a lock if this is a RelationRelationId object class for
the cases above.

As a consequence, I replaced the "XXX" related comments that were in v10 by
another set of comments in v11 (attached) like "No need to call LockRelationOid()
(through LockNotPinnedObject())....". Reason is to make it clear in the code
and also to ease the review.

B. I explained in [1] (while sharing v10) that the object locking is now outside
of the dependency code except for (and I explained why):

recordDependencyOnExpr() 
recordDependencyOnSingleRelExpr()
makeConfigurationDependencies()

So I also did some testing, on the RelationRelationId case, for those and I
reached the same conclusion as the one shared above.

For A. and B. the testing has been done by adding a "ereport(WARNING.." at
those places when a RelationRelationId is involved. Then I run "make check"
and went to the failed tests (output were not the expected ones due to the
extra "WARNING"), reproduced them with gdb and checked for the lock on the
relation producing the "WARNING". All of those were linked to 1. or 2.

Note that adding an assertion on an existing lock would not work for the cases
described in 2.

So, I'm now confident that we must be in 1. or 2. but it's also possible
that I've missed some cases (though I think the way the testing has been done is
not that weak).

To sum up, I did not see any cases that did not lead to 1. or 2., so I think
it's safe to not add an extra lock for the RelationRelationId case. If, for any
reason, there is still cases that are outside 1. or 2. then they may lead to
orphaned dependencies linked to the RelationRelationId class. I think that's
fine to take that "risk" given that a. that would not be worst than currently
and b. I did not see any of those in our fleet currently (while I have seen a non
negligible amount outside of the RelationRelationId case).

> Once done, I think that it will easier to 1.remove ambiguity, 2.document and
> 3.do the "right" thing regarding the RelationRelationId object class.
> 

Please find attached v11, where I added more detailed comments in the commit
message and also in the code (I also removed the useless check on
AuthMemRelationId).

[1]: https://www.postgresql.org/message-id/ZnAVEBhlGvpDDVOD%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

  41. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-21T13:22:43Z

    Hi,

On Wed, Jun 19, 2024 at 02:11:50PM +0000, Bertrand Drouvot wrote:
> To sum up, I did not see any cases that did not lead to 1. or 2., so I think
> it's safe to not add an extra lock for the RelationRelationId case. If, for any
> reason, there is still cases that are outside 1. or 2. then they may lead to
> orphaned dependencies linked to the RelationRelationId class. I think that's
> fine to take that "risk" given that a. that would not be worst than currently
> and b. I did not see any of those in our fleet currently (while I have seen a non
> negligible amount outside of the RelationRelationId case).

Another thought for the RelationRelationId class case: we could check if there
is a lock first and if there is no lock then acquire one. That way that would
ensure the relation is always locked (so no "risk" anymore), but OTOH it may
add "unecessary" locking (see 2. mentioned previously).

I think I do prefer this approach to be on the safe side of thing, what do
you think?

Regards,

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

  42. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-06-26T10:24:41Z

    Hi,

On Fri, Jun 21, 2024 at 01:22:43PM +0000, Bertrand Drouvot wrote:
> Another thought for the RelationRelationId class case: we could check if there
> is a lock first and if there is no lock then acquire one. That way that would
> ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> add "unecessary" locking (see 2. mentioned previously).

Please find attached v12 implementing this idea for the RelationRelationId class
case. As mentioned, it may add unnecessary locking for 2. but I think that's
worth it to ensure that we are always on the safe side of thing. This idea is
implemented in LockNotPinnedObjectById().

A few remarks:

- there is one place where the relation is not visible (even if
CommandCounterIncrement() is used). That's in TypeCreate(), because the new 
relation Oid is _not_ added to pg_class yet.
Indeed, in heap_create_with_catalog(), AddNewRelationType() is called before
AddNewRelationTuple()). I put a comment in this part of the code explaining why
it's not necessary to call LockRelationOid() here.

- some namespace related stuff is removed from "test_oat_hooks/expected/alter_table.out".
That's due to the logic in cachedNamespacePath() and the fact that the same
namespace related stuff is added prior in alter_table.out.

- the patch touches 37 .c files, but that's mainly due to the fact that
LockNotPinnedObjectById() has to be called in a lot of places.

Regards,

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

  43. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-01T09:39:17Z

    Hi,

On Wed, Jun 26, 2024 at 10:24:41AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Fri, Jun 21, 2024 at 01:22:43PM +0000, Bertrand Drouvot wrote:
> > Another thought for the RelationRelationId class case: we could check if there
> > is a lock first and if there is no lock then acquire one. That way that would
> > ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> > add "unecessary" locking (see 2. mentioned previously).
> 
> Please find attached v12 implementing this idea for the RelationRelationId class
> case. As mentioned, it may add unnecessary locking for 2. but I think that's
> worth it to ensure that we are always on the safe side of thing. This idea is
> implemented in LockNotPinnedObjectById().

Please find attached v13, mandatory rebase due to 0cecc908e97. In passing, make
use of CheckRelationOidLockedByMe() added in 0cecc908e97.

Regards,

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

  44. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-02T05:56:23Z

    Hi,

On Mon, Jul 01, 2024 at 09:39:17AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Wed, Jun 26, 2024 at 10:24:41AM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Fri, Jun 21, 2024 at 01:22:43PM +0000, Bertrand Drouvot wrote:
> > > Another thought for the RelationRelationId class case: we could check if there
> > > is a lock first and if there is no lock then acquire one. That way that would
> > > ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> > > add "unecessary" locking (see 2. mentioned previously).
> > 
> > Please find attached v12 implementing this idea for the RelationRelationId class
> > case. As mentioned, it may add unnecessary locking for 2. but I think that's
> > worth it to ensure that we are always on the safe side of thing. This idea is
> > implemented in LockNotPinnedObjectById().
> 
> Please find attached v13, mandatory rebase due to 0cecc908e97. In passing, make
> use of CheckRelationOidLockedByMe() added in 0cecc908e97.

Please find attached v14, mandatory rebase due to 65b71dec2d5.

Regards,

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

  45. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-10T07:31:06Z

    Hi,

On Tue, Jul 02, 2024 at 05:56:23AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Jul 01, 2024 at 09:39:17AM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Wed, Jun 26, 2024 at 10:24:41AM +0000, Bertrand Drouvot wrote:
> > > Hi,
> > > 
> > > On Fri, Jun 21, 2024 at 01:22:43PM +0000, Bertrand Drouvot wrote:
> > > > Another thought for the RelationRelationId class case: we could check if there
> > > > is a lock first and if there is no lock then acquire one. That way that would
> > > > ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> > > > add "unecessary" locking (see 2. mentioned previously).
> > > 
> > > Please find attached v12 implementing this idea for the RelationRelationId class
> > > case. As mentioned, it may add unnecessary locking for 2. but I think that's
> > > worth it to ensure that we are always on the safe side of thing. This idea is
> > > implemented in LockNotPinnedObjectById().
> > 
> > Please find attached v13, mandatory rebase due to 0cecc908e97. In passing, make
> > use of CheckRelationOidLockedByMe() added in 0cecc908e97.
> 
> Please find attached v14, mandatory rebase due to 65b71dec2d5.

In [1] I mentioned that the object locking logic has been put outside of the 
dependency code except for:

recordDependencyOnExpr() 
recordDependencyOnSingleRelExpr()
makeConfigurationDependencies()

Please find attached v15 that also removes the logic outside of the 3 above 
functions. Well, for recordDependencyOnExpr() and recordDependencyOnSingleRelExpr()
that's now done in find_expr_references_walker(): It's somehow still in the
dependency code but at least it is now clear which objects we are locking (and
I'm not sure how we could do better than that for those 2 functions).

There is still one locking call in recordDependencyOnCurrentExtension() but I
think this one is clear enough and does not need to be put outside (for
the same reason mentioned in [1]).

So, to sum up:

A. Locking is now done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
so that it's now always clear what object we want to acquire a lock for. It means
we are not manipulating directly an object address or a list of objects address
as it was the case when the locking was done "directly" within the dependency code.

B. A special case is done for objects that belong to the RelationRelationId class.
For those, we should be in one of the two following cases that would already
prevent the relation to be dropped:

 1. The relation is already locked (could be an existing relation or a relation
 that we are creating).

 2. The relation is protected indirectly (i.e an index protected by a lock on
 its table, a table protected by a lock on a function that depends the table...)

To avoid any risks for the RelationRelationId class case, we acquire a lock if
there is none. That may add unnecessary lock for 2. but that seems worth it. 

[1]: https://www.postgresql.org/message-id/ZnAVEBhlGvpDDVOD%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

  46. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-08-19T15:35:14Z

    Hi,

On Wed, Jul 10, 2024 at 07:31:06AM +0000, Bertrand Drouvot wrote:
> So, to sum up:
> 
> A. Locking is now done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
> so that it's now always clear what object we want to acquire a lock for. It means
> we are not manipulating directly an object address or a list of objects address
> as it was the case when the locking was done "directly" within the dependency code.
> 
> B. A special case is done for objects that belong to the RelationRelationId class.
> For those, we should be in one of the two following cases that would already
> prevent the relation to be dropped:
> 
>  1. The relation is already locked (could be an existing relation or a relation
>  that we are creating).
> 
>  2. The relation is protected indirectly (i.e an index protected by a lock on
>  its table, a table protected by a lock on a function that depends the table...)
> 
> To avoid any risks for the RelationRelationId class case, we acquire a lock if
> there is none. That may add unnecessary lock for 2. but that seems worth it. 
> 

Please find attached v16, mandatory rebase due to 80ffcb8427.

Regards,

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

  47. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-10-28T09:30:19Z

    Hi,

On Mon, Aug 19, 2024 at 03:35:14PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Wed, Jul 10, 2024 at 07:31:06AM +0000, Bertrand Drouvot wrote:
> > So, to sum up:
> > 
> > A. Locking is now done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
> > so that it's now always clear what object we want to acquire a lock for. It means
> > we are not manipulating directly an object address or a list of objects address
> > as it was the case when the locking was done "directly" within the dependency code.
> > 
> > B. A special case is done for objects that belong to the RelationRelationId class.
> > For those, we should be in one of the two following cases that would already
> > prevent the relation to be dropped:
> > 
> >  1. The relation is already locked (could be an existing relation or a relation
> >  that we are creating).
> > 
> >  2. The relation is protected indirectly (i.e an index protected by a lock on
> >  its table, a table protected by a lock on a function that depends the table...)
> > 
> > To avoid any risks for the RelationRelationId class case, we acquire a lock if
> > there is none. That may add unnecessary lock for 2. but that seems worth it. 
> > 
> 
> Please find attached v16, mandatory rebase due to 80ffcb8427.

rebased (v17 attached).

Regards,

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

  48. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-01-02T08:15:13Z

    Hi,

On Mon, Oct 28, 2024 at 09:30:19AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Aug 19, 2024 at 03:35:14PM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Wed, Jul 10, 2024 at 07:31:06AM +0000, Bertrand Drouvot wrote:
> > > So, to sum up:
> > > 
> > > A. Locking is now done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
> > > so that it's now always clear what object we want to acquire a lock for. It means
> > > we are not manipulating directly an object address or a list of objects address
> > > as it was the case when the locking was done "directly" within the dependency code.
> > > 
> > > B. A special case is done for objects that belong to the RelationRelationId class.
> > > For those, we should be in one of the two following cases that would already
> > > prevent the relation to be dropped:
> > > 
> > >  1. The relation is already locked (could be an existing relation or a relation
> > >  that we are creating).
> > > 
> > >  2. The relation is protected indirectly (i.e an index protected by a lock on
> > >  its table, a table protected by a lock on a function that depends the table...)
> > > 
> > > To avoid any risks for the RelationRelationId class case, we acquire a lock if
> > > there is none. That may add unnecessary lock for 2. but that seems worth it. 
> > > 
> > 
> > Please find attached v16, mandatory rebase due to 80ffcb8427.
> 
> rebased (v17 attached).

rebased (v18 attached).

Regards,

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

  49. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-04T13:24:03Z

    Hi,

On Thu, Jan 02, 2025 at 08:15:13AM +0000, Bertrand Drouvot wrote:
> rebased (v18 attached).

Thanks to all of you that have discussed this patch during the developer meeting
at FOSDEM PGDay last week [1]. I'm attaching a new version to address Álvaro's
concern about calling getObjectDescription() in the new LockNotPinnedObjectById()
function. This call was being used to provide a "meaningful" error message but
we agreed to provide the object OID instead (as anyway the object is dropped).

Note that the OIDs are reported as "errdetail" to ensure the isolation tests
added in this patch remain stable (output does not depend of the actual OIDs
values).

A quick sum up about this patch:

A. Locking is done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
so that it's now always clear what object we want to acquire a lock for. It means
that we are not manipulating directly an object address or a list of objects
address as it was the case when the locking was done "directly" within the
dependency code.

B. A special case is done for objects that belong to the RelationRelationId class.
For those, we should be in one of the two following cases that would already
prevent the relation to be dropped:

 B1. The relation is already locked (could be an existing relation or a relation
 that we are creating).

 B2. The relation is protected indirectly (i.e an index protected by a lock on
 its table, a table protected by a lock on a function that depends the table...)

To avoid any risks for the RelationRelationId class case, we acquire a lock if
there is none. That may add unnecessary lock for B2. but that seems worth it. 

That's a lot of mechanical changes so that's easy to miss one or to do it
wrong but:

1. I did my best to avoid that
2. assertions are added in recordMultipleDependencies() to "ensure" the object
is locked
3. even if one case is missing (that is not catched by the assertions because 
the dependency is not covered in the tests, not sure that exists though), then it
just means that we could be in the current state (orphaned dependency), not worst
than that

During the meeting a question has been raised regarding the number of locks
increase. This has already been discussed in [2] and I think that the outcome
is that the default max_locks_per_transaction value (64) is probably still
enough in real life (and even if it is not then it can be increased to satisfy
the requirements).

[1]: https://2025.fosdempgday.org/devmeeting
[2]: https://www.postgresql.org/message-id/CA%2BTgmoaFPUubBBk52Qp2wkoL7JX7OjhewiK%2B7LSot7%3DrecbzzQ%40mail.gmail.com

Regards,

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

  50. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2025-05-19T17:02:52Z

    On Wed, 2024-05-22 at 10:48 -0400, Robert Haas wrote:
> > Then, Tom proposed another approach in [2] which is that "creation
> > DDL will have
> > to take a lock on each referenced object that'd conflict with a
> > lock taken by DROP".
> > This is the one the current patch is trying to implement.
> 
> It's a clever idea, but I'm not sure that I like it.

I'm not sure if you intended it this way, but using "clever" here
suggests that it's an unusual trick. But isn't that the kind of thing
heavyweight locks are for?

> 
> My concern was really more about the maintainability of
> the code. I fear that if we add code that takes heavyweight locks in
> surprising places, we might later find the behavior difficult to
> reason about.

FWIW, a lock while recording a dependency would not be surprising to
me. Assuming that heavyweight locks are the right approach, the locks
need to be taken somewhere. And expecting all the callers to get it
right seems error-prone.

This is a long thread so I must be missing some problem or complication
here.

Regards,
	Jeff Davis

  51. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2025-05-19T18:07:34Z

    On Mon, May 19, 2025 at 1:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
> I'm not sure if you intended it this way, but using "clever" here
> suggests that it's an unusual trick. But isn't that the kind of thing
> heavyweight locks are for?
>
> FWIW, a lock while recording a dependency would not be surprising to
> me. Assuming that heavyweight locks are the right approach, the locks
> need to be taken somewhere. And expecting all the callers to get it
> right seems error-prone.

I agree with that, but I think that it may also be error-prone to
assume that it's OK to acquire heavyweight locks on other catalog
objects at any place in the code where we record a dependency. I will
not be surprised at all if that turns out to have some negative
consequences. For example, I think it might result in acquiring the
locks on those other objects at a subtly wrong time (leading to race
conditions) or acquiring two locks on the same object with different
lock modes where we should really only acquire one. I'm all in favor
of solving this problem using heavyweight locks, but I think that
implicitly acquiring them as a side effect of recording dependencies
feels too surprising.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  52. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2025-05-20T21:12:41Z

    On Mon, 2025-05-19 at 14:07 -0400, Robert Haas wrote:
> I agree with that, but I think that it may also be error-prone to
> assume that it's OK to acquire heavyweight locks on other catalog
> objects at any place in the code where we record a dependency. I will
> not be surprised at all if that turns out to have some negative
> consequences. For example, I think it might result in acquiring the
> locks on those other objects at a subtly wrong time (leading to race
> conditions) or acquiring two locks on the same object with different
> lock modes where we should really only acquire one. I'm all in favor
> of solving this problem using heavyweight locks, but I think that
> implicitly acquiring them as a side effect of recording dependencies
> feels too surprising.

I see what you mean now, in the sense that other code that calls
LockDatabaseObject (and other variants of LockAcquire) are mostly
higher-level operations like AlterPublication(), and not side-effects
of something else.

But relation_open() is sort of an exception. There are lots of places
where that takes a lock because we happen to want something out of the
relcache, like generate_partition_qual() taking a lock on the parent or
CheckAttributeType() taking a lock on the typrelid. You could say those
are fairly obvious, but that's because we already know, and we could
make it more widely known that recording a dependency takes a lock.

One compromise might be to have recordDependencyOn() take a LOCKMODE
parameter, which would both inform the caller that a lock will be
taken, and allow the caller to do it their own way and specify NoLock
if necessary. That still results in a huge diff, but the end result
would not be any more complex than the current code.

Regards,
	Jeff Davis

  53. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-05-21T06:37:10Z

    Hi,

On Tue, May 20, 2025 at 02:12:41PM -0700, Jeff Davis wrote:
> On Mon, 2025-05-19 at 14:07 -0400, Robert Haas wrote:
> > I agree with that, but I think that it may also be error-prone to
> > assume that it's OK to acquire heavyweight locks on other catalog
> > objects at any place in the code where we record a dependency. I will
> > not be surprised at all if that turns out to have some negative
> > consequences. For example, I think it might result in acquiring the
> > locks on those other objects at a subtly wrong time (leading to race
> > conditions) or acquiring two locks on the same object with different
> > lock modes where we should really only acquire one. I'm all in favor
> > of solving this problem using heavyweight locks, but I think that
> > implicitly acquiring them as a side effect of recording dependencies
> > feels too surprising.
> 
> I see what you mean now, in the sense that other code that calls
> LockDatabaseObject (and other variants of LockAcquire) are mostly
> higher-level operations like AlterPublication(), and not side-effects
> of something else.
> 
> But relation_open() is sort of an exception. There are lots of places
> where that takes a lock because we happen to want something out of the
> relcache, like generate_partition_qual() taking a lock on the parent or
> CheckAttributeType() taking a lock on the typrelid. You could say those
> are fairly obvious, but that's because we already know, and we could
> make it more widely known that recording a dependency takes a lock.
> 
> One compromise might be to have recordDependencyOn() take a LOCKMODE
> parameter, which would both inform the caller that a lock will be
> taken, and allow the caller to do it their own way and specify NoLock
> if necessary. That still results in a huge diff, but the end result
> would not be any more complex than the current code.

Thanks for sharing your thoughts! I had in mind to "just" check if there
is an existing lock (and if so, skip acquiring a new one) but your proposal
sounds better. Indeed it would make the locking behavior explicit and also
be flexible (allowing the callers to choose the LOCKMODE).

I'll prepare a new version implementing your proposal.

Regards,

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

  54. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2025-05-21T16:55:22Z

    On Tue, May 20, 2025 at 5:12 PM Jeff Davis <pgsql@j-davis.com> wrote:
> But relation_open() is sort of an exception. There are lots of places
> where that takes a lock because we happen to want something out of the
> relcache, like generate_partition_qual() taking a lock on the parent or
> CheckAttributeType() taking a lock on the typrelid. You could say those
> are fairly obvious, but that's because we already know, and we could
> make it more widely known that recording a dependency takes a lock.

To me, relation_open() feels like the kind of operation that I would
expect to take a lock. If I open something, I must have acquired some
resource on it that I will then use for a while before closing the
object.

> One compromise might be to have recordDependencyOn() take a LOCKMODE
> parameter, which would both inform the caller that a lock will be
> taken, and allow the caller to do it their own way and specify NoLock
> if necessary. That still results in a huge diff, but the end result
> would not be any more complex than the current code.

Yeah, that's not a terrible idea. I still like the idea I thought
Bertrand was pursuing, namely, to take no lock in recordDependencyOn()
but assert that the caller has previously acquired one. However, we
could still do the Assert() check with this design when NoLock is
passed, so I think this is a reasonable alternative to that design.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  55. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2025-05-21T17:17:58Z

    On Wed, 2025-05-21 at 12:55 -0400, Robert Haas wrote:
> > ...like generate_partition_qual() taking a lock on the parent or
> > CheckAttributeType() taking a lock on the typrelid...
> 
> To me, relation_open() feels like the kind of operation that I would
> expect to take a lock. If I open something, I must have acquired some
> resource on it that I will then use for a while before closing the
> object.

Of course relation_open() takes a lock, but sometimes relation_open()
is hidden in the call stack below other functions where it's not so
obvious.

> 
> Yeah, that's not a terrible idea. I still like the idea I thought
> Bertrand was pursuing, namely, to take no lock in
> recordDependencyOn()
> but assert that the caller has previously acquired one. However, we
> could still do the Assert() check with this design when NoLock is
> passed, so I think this is a reasonable alternative to that design.

I'd have to see the patch to see whether I liked the end result. But
I'm guessing that involves a lot of non-mechanical changes in the call
sites, and also relies on test coverage for all of them.

Regards,
	Jeff Davis

  56. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2025-05-21T17:47:17Z

    On Wed, May 21, 2025 at 1:18 PM Jeff Davis <pgsql@j-davis.com> wrote:
> Of course relation_open() takes a lock, but sometimes relation_open()
> is hidden in the call stack below other functions where it's not so
> obvious.

Probably true, although some of those are probably code that could
stand to be improved.

> > Yeah, that's not a terrible idea. I still like the idea I thought
> > Bertrand was pursuing, namely, to take no lock in
> > recordDependencyOn()
> > but assert that the caller has previously acquired one. However, we
> > could still do the Assert() check with this design when NoLock is
> > passed, so I think this is a reasonable alternative to that design.
>
> I'd have to see the patch to see whether I liked the end result. But
> I'm guessing that involves a lot of non-mechanical changes in the call
> sites, and also relies on test coverage for all of them.

Sure, fair enough.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  57. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-05-22T12:15:11Z

    Hi,

On Wed, May 21, 2025 at 10:17:58AM -0700, Jeff Davis wrote:
> On Wed, 2025-05-21 at 12:55 -0400, Robert Haas wrote:
> > Yeah, that's not a terrible idea. I still like the idea I thought
> > Bertrand was pursuing, namely, to take no lock in
> > recordDependencyOn()
> > but assert that the caller has previously acquired one. However, we
> > could still do the Assert() check with this design when NoLock is
> > passed, so I think this is a reasonable alternative to that design.
> 
> I'd have to see the patch to see whether I liked the end result. But
> I'm guessing that involves a lot of non-mechanical changes in the call
> sites, and also relies on test coverage for all of them.

Thinking more about it, I'm not sure the NoLock/AccessShareLock will be easy
to make it right and maintain.

The thing is that in recordMultipleDependencies() we may iterate over multiple
referenced objects and those are the ones we want a lock on.

So if we enter recordMultipleDependencies() with "NoLock" we would need to be
100% sure that ALL the referenced objects are already locked (or that we don't
want to take a lock on ALL the referenced objects).

But that's not so easy, for example with something like:

create schema my_schema;
CREATE TABLE my_schema.test_maint(i INT);
INSERT INTO my_schema.test_maint VALUES (1), (2);
CREATE FUNCTION my_schema.fn(INT) RETURNS INT IMMUTABLE LANGUAGE plpgsql AS $$
  BEGIN
    RAISE NOTICE 'BDT';
    RETURN $1;
  END;
$$;

Then during:

CREATE MATERIALIZED VIEW my_schema.test_maint_mv AS SELECT my_schema.fn(i) FROM my_schema.test_maint;

1. The schema is locked
2. The relation is locked
3. The function is not locked

So we should pass "AccessShareLock" to recordMultipleDependencies() but that could
be easy to miss, resulting in passing "NoLock".

Furthermore, it means that even if we pass "AccessShareLock" correctly here, we'd
need to check that there is no existing lock on each referenced object (with
LockHeldByMe()/CheckRelationOidLockedByMe()) with a level > AccessShareLock (if
not, we'd add an extra lock without any good reason to do so).

With Robert's idea we could avoid to call LockDatabaseObject()/LockRelationOid()
if we know that the object/relation is already locked (or that we don't want 
a lock at this place). But it would mean being 100% sure that if there are
multiple code paths leading to the same referenced object insertion location
then each of them have the same locking behavior.

As that seems hard, a better approach would probably be to also always call
LockHeldByMe()/CheckRelationOidLockedByMe() before trying to acquire the lock.

So I think:

- Jeff's idea could be hard to reason about, as "NoLock" could mean: we are sure
that ALL the existing referenced objects are already locked and "AccessShareLock"
would mean: we know that at least one referenced object needs an AccessShareLock.
Then that would mean that "by design" we'd need to check if there is no existing
lock before trying to acquire the AccessShareLock on the referenced objects.

- Robert's idea would still require that we check whether there is any existing
lock before acquiring the AccessShareLock (to be on the safe side of things).

So I wonder if, after all, it makes sense to simply try to acquire the
AccessShareLock on a referenced object in recordMultipleDependencies() IIF
this referenced object is not already locked (basically what was initially
proposed, but with this extra check added and without the "NoLock"/"lock"
addition to recordMultipleDependencies())).

That would probably address Robert's concern [1] "acquiring two locks on the same
object with different lock modes where we should really only acquire one" but 
probably not this one "I think it might result in acquiring the
locks on those other objects at a subtly wrong time (leading to race
conditions)".

For the latter I'm not sure how that could be a subtly wrong time or how could
we determine what a subtly wrong time is (to avoid taking the lock).

Thoughts?

[1]: https://www.postgresql.org/message-id/CA%2BTgmoaCJ5-Zx5R0uN%2Bah4EZU1SamY1PneaW5O617FsNKavmfw%40mail.gmail.com

Regards,

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

  58. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2025-05-22T13:40:47Z

    On Thu, May 22, 2025 at 8:15 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> That would probably address Robert's concern [1] "acquiring two locks on the same
> object with different lock modes where we should really only acquire one" but
> probably not this one "I think it might result in acquiring the
> locks on those other objects at a subtly wrong time (leading to race
> conditions)".
>
> For the latter I'm not sure how that could be a subtly wrong time or how could
> we determine what a subtly wrong time is (to avoid taking the lock).

Well, I admit I'm not quite sure there is any timing problem here. But
I think it's possible. For example, consider RangeVarGetRelidExtended,
and in particular the callback argument. If we do permissions checking
before locking the relation, the results can change before we actually
lock the relation; if we do it afterward, users can contrive to hold
locks on relations for which they don't have permissions. That rather
ornate callback system allows us to have the best of both worlds. That
system is also concerned with the possibility that we do a name -> OID
translation before holding a lock and by the time we acquire the lock,
concurrent DDL has happened and the answer we got is no longer the
right answer. We've had security holes due to such things.

Now I'm not entirely sure that either of those things are issues here.
My first guess is that name lookup races are not an issue here,
because we're following OID links, but permissions checks seem like
they might be an issue. We might not decide to do something as
elaborate as we did with RangeVarGetRelidExtended and ... maybe that's
OK? But in general I am skeptical of the conclusion that we can just
shove all the locking down into some subordinate layer and nothing
will go wrong, because locking doesn't exist in a vacuum -- it relates
to other things that we also need to do, and whether we do the locking
before or after other steps can affect semantics and even security.

Pushing the locking down into recordDependencyOn amounts to hoping
that we don't need to study each code path in detail and decide on the
exactly right place to acquire the lock. It amounts to hoping that
wherever the recordDependencyOn call is located, it's before things
that we want the locking to happen before and after the things that we
want the locking to happen after. And maybe that's true. Or maybe it's
close enough to true that it's still better than the status quo where
we're not taking locks at all. But on the other hand, since I can't
think of any compelling reason why it HAS to be true, maybe it isn't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  59. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2025-05-22T19:38:50Z

    On Thu, 2025-05-22 at 09:40 -0400, Robert Haas wrote:

> Pushing the locking down into recordDependencyOn amounts to hoping
> that we don't need to study each code path in detail and decide on
> the
> exactly right place to acquire the lock.

There are (by my rough count) over 250 call sites modified by the v19
patch. I fear that, if each of those call sites needs to be studied for
the kinds of subtle issues you are describing, then we are likely to
make a mistake. If not now, then in the future as new features change
those call sites.

Bertrand, what pattern is safe to follow for most call sites? Which
call sites are the most interesting ones that need special attention?

Regards,
	Jeff Davis

  60. Re: Avoid orphaned objects dependencies, take 3

    jian he <jian.universality@gmail.com> — 2025-05-23T04:20:01Z

    On Tue, Feb 4, 2025 at 9:24 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Thu, Jan 02, 2025 at 08:15:13AM +0000, Bertrand Drouvot wrote:
> > rebased (v18 attached).
>
> Thanks to all of you that have discussed this patch during the developer meeting
> at FOSDEM PGDay last week [1]. I'm attaching a new version to address Álvaro's
> concern about calling getObjectDescription() in the new LockNotPinnedObjectById()
> function. This call was being used to provide a "meaningful" error message but
> we agreed to provide the object OID instead (as anyway the object is dropped).
>
> Note that the OIDs are reported as "errdetail" to ensure the isolation tests
> added in this patch remain stable (output does not depend of the actual OIDs
> values).
>
> A quick sum up about this patch:
>
> A. Locking is done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
> so that it's now always clear what object we want to acquire a lock for. It means
> that we are not manipulating directly an object address or a list of objects
> address as it was the case when the locking was done "directly" within the
> dependency code.
>
> B. A special case is done for objects that belong to the RelationRelationId class.
> For those, we should be in one of the two following cases that would already
> prevent the relation to be dropped:
>
>  B1. The relation is already locked (could be an existing relation or a relation
>  that we are creating).
>
>  B2. The relation is protected indirectly (i.e an index protected by a lock on
>  its table, a table protected by a lock on a function that depends the table...)
>
> To avoid any risks for the RelationRelationId class case, we acquire a lock if
> there is none. That may add unnecessary lock for B2. but that seems worth it.
>
> That's a lot of mechanical changes so that's easy to miss one or to do it
> wrong but:
>
> 1. I did my best to avoid that
> 2. assertions are added in recordMultipleDependencies() to "ensure" the object
> is locked
> 3. even if one case is missing (that is not catched by the assertions because
> the dependency is not covered in the tests, not sure that exists though), then it
> just means that we could be in the current state (orphaned dependency), not worst
> than that
>
> During the meeting a question has been raised regarding the number of locks
> increase. This has already been discussed in [2] and I think that the outcome
> is that the default max_locks_per_transaction value (64) is probably still
> enough in real life (and even if it is not then it can be increased to satisfy
> the requirements).
>
> [1]: https://2025.fosdempgday.org/devmeeting
> [2]: https://www.postgresql.org/message-id/CA%2BTgmoaFPUubBBk52Qp2wkoL7JX7OjhewiK%2B7LSot7%3DrecbzzQ%40mail.gmail.com
>

hi.
I plan to play around with v19.
but i can not build based on it.

related error message:

running bootstrap script ... ok
performing post-bootstrap initialization ... TRAP: failed
Assert("LockHeldByMe(&tag, AccessShareLock, false)"), File:
"../../Desktop/pg_src/src1/postgres/src/backend/catalog/pg_depend.c",
Line: 116, PID: 1118343
/home/jian/postgres/regression1/bin/postgres(ExceptionalCondition+0xf3)[0xe66090]
/home/jian/postgres/regression1/bin/postgres(recordMultipleDependencies+0x17f)[0x6ae34d]
/home/jian/postgres/regression1/bin/postgres(record_object_address_dependencies+0x4e)[0x66b333]
/home/jian/postgres/regression1/bin/postgres(ProcedureCreate+0x2443)[0x6b9ad7]
/home/jian/postgres/regression1/bin/postgres(CreateFunction+0xf91)[0x73a7ef]
/home/jian/postgres/regression1/bin/postgres[0xbcba68]
/home/jian/postgres/regression1/bin/postgres(standard_ProcessUtility+0x170c)[0xbc9fb7]
/home/jian/postgres/regression1/bin/postgres(ProcessUtility+0x184)[0xbc889b]
/home/jian/postgres/regression1/bin/postgres[0xbc6821]
/home/jian/postgres/regression1/bin/postgres[0xbc6bbd]
/home/jian/postgres/regression1/bin/postgres(PortalRun+0x3f4)[0xbc5bda]
/home/jian/postgres/regression1/bin/postgres[0xbbae37]
/home/jian/postgres/regression1/bin/postgres(PostgresMain+0x1110)[0xbc2feb]
/home/jian/postgres/regression1/bin/postgres(PostgresMain+0x0)[0xbc1edb]
/home/jian/postgres/regression1/bin/postgres(main+0x4cf)[0x8e98b1]
/lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x71d95a829d90]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x71d95a829e40]
/home/jian/postgres/regression1/bin/postgres(_start+0x25)[0x4af1c5]
Aborted (core dumped)
child process exited with exit code 134

  61. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-06-02T14:02:36Z

    Hi,

On Thu, May 22, 2025 at 09:40:47AM -0400, Robert Haas wrote:
> On Thu, May 22, 2025 at 8:15 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > That would probably address Robert's concern [1] "acquiring two locks on the same
> > object with different lock modes where we should really only acquire one" but
> > probably not this one "I think it might result in acquiring the
> > locks on those other objects at a subtly wrong time (leading to race
> > conditions)".
> >
> > For the latter I'm not sure how that could be a subtly wrong time or how could
> > we determine what a subtly wrong time is (to avoid taking the lock).
> 
> Well, I admit I'm not quite sure there is any timing problem here. But
> I think it's possible. For example, consider RangeVarGetRelidExtended,
> and in particular the callback argument. If we do permissions checking
> before locking the relation, the results can change before we actually
> lock the relation; if we do it afterward, users can contrive to hold
> locks on relations for which they don't have permissions. That rather
> ornate callback system allows us to have the best of both worlds. That
> system is also concerned with the possibility that we do a name -> OID
> translation before holding a lock and by the time we acquire the lock,
> concurrent DDL has happened and the answer we got is no longer the
> right answer. We've had security holes due to such things.

Yeah I just looked at 2ad36c4e44c and see what issues it solved.

> 
> Now I'm not entirely sure that either of those things are issues here.
> My first guess is that name lookup races are not an issue here,
> because we're following OID links,

Yeah, I think the same and think that checking that the object(s) still exist
once we tried to get the lock is probably enough (as done in the patch versions
that have been shared).

> but permissions checks seem like
> they might be an issue.

I agree that we might end up locking an object we don't have the permission
on.

I just looked at 2 examples.

1.

CREATE MATERIALIZED VIEW test_maint_mv2 AS SELECT fn(i) FROM test_maint;

Would call recordMultipleDependencies() with the function as a referenced object:

Breakpoint 3, recordMultipleDependencies
123
(gdb) p *depender
$3 = {classId = 2618, objectId = 16468, objectSubId = 0}
(gdb) p *referenced
$4 = {classId = 1255, objectId = 16388, objectSubId = 0}

postgres=# select * from pg_identify_object(1255,16388,0);
   type   | schema | name |      identity
----------+--------+------+--------------------
 function | public |      | public.fn(integer)
(1 row)

But would check the permissions after:

Breakpoint 1, object_aclcheck (classid=1255, objectid=16388, roleid=16384, mode=128)
3823            return object_aclcheck_ext(classid, objectid, roleid, mode, NULL);

Means that in that case we'd put a lock on the function *before* checking for
the permissions.

2.

OTOH it's also possible the other way around:

CREATE FUNCTION myschema1.foo() RETURNS int AS 'select 1' LANGUAGE sql;

Would call object_aclcheck() on the schema:

Breakpoint 1, object_aclcheck (classid=2615, objectid=16435, roleid=10, mode=512)
3823            return object_aclcheck_ext(classid, objectid, roleid, mode, NULL);

postgres=# select * from pg_identify_object(2615,16435,0);
  type  | schema |   name    | identity
--------+--------+-----------+-----------
 schema |        | myschema1 | myschema1
(1 row)

before adding the dependency:

Breakpoint 3, recordMultipleDependencies
123
(gdb) p *referenced
$1 = {classId = 2615, objectId = 16435, objectSubId = 0}

> We might not decide to do something as
> elaborate as we did with RangeVarGetRelidExtended

So, we currently have 2 patterns:

P1: permission checks on a referenced object is done before we call recordMultipleDependencies()
P2: permission checks on a referenced object is done after we call recordMultipleDependencies()

So, if we add locking in recordMultipleDependencies() then I think that P2 is worst 
than P1 (there is still the risk that the permissions changed by the time
we reach recordMultipleDependencies() though).

As also stated in RangeVarGetRelidExtended()'s comment:

"
and it's really best to check permissions * before locking anything!
"

We could imagine doing the permissions check in recordMultipleDependencies() (in
more or less the same way as RangeVarGetRelidExtended() is doing with callback)
because only the recordMultipleDependencies()'s "caller" could know what kind of
check is needed. But that would not work for the same reasons as Jeff's proposal
does not fly (we may manipulate multiple referenced objects that would need
distinct callbacks...).

So, it looks like that we may need some refactoring of the existing code to
ensure that the permissions checks on the referenced objects are done before
the lock is acquired (before recordMultipleDependencies() is called?).

I propose to first try to list the cases where P2 is in action (and I think those
will be the ones that need special attention that Jeff is asking for in [1]).

And then discuss once we have the cases in hand. Thoughts?

[1]: https://www.postgresql.org/message-id/d721011cd3ec3aedd57b193ef10cf541f50df325.camel%40j-davis.com

Regards,

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

  62. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-06-02T14:06:43Z

    Hi,

On Thu, May 22, 2025 at 12:38:50PM -0700, Jeff Davis wrote:
> Which
> call sites are the most interesting ones that need special attention?

I think the ones that need special attention are the ones that check for
the permissions on the referenced objects *after* recordMultipleDependencies is
called (see [1]).

Does that make sense to you? 

[1]: https://www.postgresql.org/message-id/aD2u/GR/yaRkBJdJ%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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

  63. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2025-06-03T17:27:29Z

    On Mon, Jun 2, 2025 at 10:02 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> So, we currently have 2 patterns:
>
> P1: permission checks on a referenced object is done before we call recordMultipleDependencies()
> P2: permission checks on a referenced object is done after we call recordMultipleDependencies()
>
> So, if we add locking in recordMultipleDependencies() then I think that P2 is worst
> than P1 (there is still the risk that the permissions changed by the time
> we reach recordMultipleDependencies() though).

Hmm. I don't think I agree. If I understand correctly, P2 only permits
users to take a lock on an object they shouldn't be able to touch,
permitting them to temporarily interfere with access to it. P1 permits
users to potentially perform a permanent catalog modification that
should have been blocked by the permissions system. To my knowledge,
we've never formally classified the former type of problem as a
security vulnerability, although maybe there's an argument that it is
one. We've filed CVEs for problems of the latter sort.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  64. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-06-06T05:37:36Z

    Hi,

On Tue, Jun 03, 2025 at 01:27:29PM -0400, Robert Haas wrote:
> On Mon, Jun 2, 2025 at 10:02 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > So, we currently have 2 patterns:
> >
> > P1: permission checks on a referenced object is done before we call recordMultipleDependencies()
> > P2: permission checks on a referenced object is done after we call recordMultipleDependencies()
> >
> > So, if we add locking in recordMultipleDependencies() then I think that P2 is worst
> > than P1 (there is still the risk that the permissions changed by the time
> > we reach recordMultipleDependencies() though).
> 
> Hmm. I don't think I agree.

Thanks for sharing your thoughts!

> If I understand correctly, P2 only permits
> users to take a lock on an object they shouldn't be able to touch,
> permitting them to temporarily interfere with access to it. P1 permits
> users to potentially perform a permanent catalog modification that
> should have been blocked by the permissions system. To my knowledge,
> we've never formally classified the former type of problem as a
> security vulnerability, although maybe there's an argument that it is
> one. We've filed CVEs for problems of the latter sort.

What I meant to say is that P2 is worse "by design" because it's "always" wrong
to lock an object we don't have a permission on: so the permission should be
checked first.

So we have:

P1:

 * check_permissions()
 * permissions could change here
 * Lock in recordMultipleDependencies()

P2:

 * Lock in recordMultipleDependencies()
 * FWIW, permissions could change here too (for example, one could still "
revoke usage on schema myschema1 from user1" while there is an AccessShareLock
on schema myschema1)
 * check_permissions()

But P2 sequence of events is "wrong" by design (to lock an object we may not
have permissions on) that's what I meant.

Now if we look at it from a "pure" security angle (as you did) I agree that P1 is 
the worse because it could allow catalog change that should have been blocked by
the permission check.  P2 would prevent that. I agree that we should focus on
P1 then.

Let me try to list the P1 cases (instead of the P2 ones) so that we can focus on
/discuss those. 

Regards,

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

  65. Re: Avoid orphaned objects dependencies, take 3

    Roman Eskin <r.eskin@arenadata.io> — 2025-11-09T08:33:39Z

    Hi everybody,

Apologies for jumping into this conversation.

Our customers have also encountered a similar issue with the
concurrent drop of a dependent object. In our code (in the Greengage
DB), we implemented a fix analogous to one of the first versions of
Bertrand's approach, using locking within the pg_depend code.

In the long term, however, we are interested in aligning with the
upstream Postgres code as much as possible.

Since there haven't been any recent updates in this thread, we were
wondering if there are any plans for the next steps regarding this
issue.

Thank you in advance for your insights.

Best regards,
Roman Eskin

06.06.2025 15:37, Bertrand Drouvot пишет:
> Hi,
> 
> On Tue, Jun 03, 2025 at 01:27:29PM -0400, Robert Haas wrote:
>> On Mon, Jun 2, 2025 at 10:02 AM Bertrand Drouvot
>> <bertranddrouvot.pg@gmail.com> wrote:
>>> So, we currently have 2 patterns:
>>>
>>> P1: permission checks on a referenced object is done before we call recordMultipleDependencies()
>>> P2: permission checks on a referenced object is done after we call recordMultipleDependencies()
>>>
>>> So, if we add locking in recordMultipleDependencies() then I think that P2 is worst
>>> than P1 (there is still the risk that the permissions changed by the time
>>> we reach recordMultipleDependencies() though).
>>
>> Hmm. I don't think I agree.
> 
> Thanks for sharing your thoughts!
> 
>> If I understand correctly, P2 only permits
>> users to take a lock on an object they shouldn't be able to touch,
>> permitting them to temporarily interfere with access to it. P1 permits
>> users to potentially perform a permanent catalog modification that
>> should have been blocked by the permissions system. To my knowledge,
>> we've never formally classified the former type of problem as a
>> security vulnerability, although maybe there's an argument that it is
>> one. We've filed CVEs for problems of the latter sort.
> 
> What I meant to say is that P2 is worse "by design" because it's "always" wrong
> to lock an object we don't have a permission on: so the permission should be
> checked first.
> 
> So we have:
> 
> P1:
> 
>   * check_permissions()
>   * permissions could change here
>   * Lock in recordMultipleDependencies()
> 
> P2:
> 
>   * Lock in recordMultipleDependencies()
>   * FWIW, permissions could change here too (for example, one could still "
> revoke usage on schema myschema1 from user1" while there is an AccessShareLock
> on schema myschema1)
>   * check_permissions()
> 
> But P2 sequence of events is "wrong" by design (to lock an object we may not
> have permissions on) that's what I meant.
> 
> Now if we look at it from a "pure" security angle (as you did) I agree that P1 is
> the worse because it could allow catalog change that should have been blocked by
> the permission check.  P2 would prevent that. I agree that we should focus on
> P1 then.
> 
> Let me try to list the P1 cases (instead of the P2 ones) so that we can focus on
> /discuss those.
> 
> Regards,
> 

-- 
	С уважением,
Роман Ескин | C Developer
+7-964-449-81-15 | _r.eskin@arenadata.io_ <mailto:r.eskin@arenadata.io>
_Arenadata_ <https://arenadata.tech/> | Подписывайтесь на нас в Telegram
<https://t.me/arenadata> и VK <https://vk.com/arenadata>!

<https://arenadata.tech/events/>

УВЕДОМЛЕНИЕ О КОНФИДЕНЦИАЛЬНОСТИ:
Это электронное сообщение и любые документы, приложенные к нему, могут
содержать конфиденциальную информацию и предназначаются только
адресату/адресатам. Настоящим уведомляем Вас о том, что Вы не можете
воспроизводить, распространять и разглашать содержимое данного
электронного сообщения кому-либо без письменного согласия ООО «Аренадата
Софтвер». Разглашение конфиденциальной информации строго запрещено. Если
Вы получили это сообщение по ошибке, пожалуйста, сообщите об этом
отправителю по электронной почте и немедленно удалите это сообщение.

  66. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-04-15T18:36:49Z

    Hi,

On Sun, Nov 09, 2025 at 06:33:39PM +1000, Roman Eskin wrote:
> Hi everybody,
> 
> Apologies for jumping into this conversation.

No problem at all, you're more than welcome to join it!

> Our customers have also encountered a similar issue with the
> concurrent drop of a dependent object.

Yeah, it's not something so rare that one could thought (we also see a non
negligible of them in our fleet).

> In our code (in the Greengage
> DB), we implemented a fix analogous to one of the first versions of
> Bertrand's approach, using locking within the pg_depend code.
> 
> In the long term, however, we are interested in aligning with the
> upstream Postgres code as much as possible.
> 
> Since there haven't been any recent updates in this thread, we were
> wondering if there are any plans for the next steps regarding this
> issue.

Apologies for this late reply. I plan to work on this again in the coming weeks.
The next step I've in mind is to build the list of the P1 cases described above.

Once we've that list I think we could start discussing the next steps.

Regards,

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

  67. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-04-28T11:17:17Z

    Hi,

On Wed, Apr 15, 2026 at 06:36:49PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Sun, Nov 09, 2025 at 06:33:39PM +1000, Roman Eskin wrote:
> > Hi everybody,
> > 
> > Apologies for jumping into this conversation.
> 
> No problem at all, you're more than welcome to join it!
> 
> > Our customers have also encountered a similar issue with the
> > concurrent drop of a dependent object.
> 
> Yeah, it's not something so rare that one could thought (we also see a non
> negligible of them in our fleet).
> 
> > In our code (in the Greengage
> > DB), we implemented a fix analogous to one of the first versions of
> > Bertrand's approach, using locking within the pg_depend code.
> > 
> > In the long term, however, we are interested in aligning with the
> > upstream Postgres code as much as possible.
> > 
> > Since there haven't been any recent updates in this thread, we were
> > wondering if there are any plans for the next steps regarding this
> > issue.
> 
> Apologies for this late reply. I plan to work on this again in the coming weeks.
> The next step I've in mind is to build the list of the P1 cases described above.
> 
> Once we've that list I think we could start discussing the next steps.

I've been able to identify the P1 cases (lock after acl check) thanks to 0003
attached (more on that later). With the list at hands, v20 attached is made of
3 sub patches.

0001: Avoid orphaned objects dependencies

Fix by acquiring AccessShareLock on referenced objects when recording
dependencies. This conflicts with AccessExclusiveLock taken by DROP,
preventing the race. After acquiring the lock, verify the object still
exists, if it was dropped concurrently, report an error.

The lock and check is done in both recordMultipleDependencies() and
changeDependencyFor().

Remarks:

1/ If the caller already holds a lock that conflicts with DROP (AccessShareLock
or stronger), skip the lock acquisition entirely.

2/ That does not address Robert's concern about lock after acl check cases and
that's what 0002 and 0003 are for.

0002: Lock referenced objects before permission checks in DDL commands

Add LockNotPinnedObjectById() calls before object_aclcheck() at all caller
sites where a permission check on a referenced object occurs before the
dependency on that object is recorded. This converts permission check before
lock to lock before permission check. It closes the TOCTOU window that could
allow a REVOKE to succeed between the permission check and the dependency
recording.

Note that the permission check before lock cases fixed in 0002 are already present.
0001 just makes the TOCTOU window wider in case there is a concurrent drop that
would block the dependency recording. So, I think that 0002 has merit on its own.

0003: Add Assert guard to detect permission check before lock regressions

Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
is called on a referenced object before a lock is held on it, which would widen
the TOCTOU window between the permission check and the dependency recording.

In recordMultipleDependencies() and changeDependencyFor(), for each referenced
object that had a permission check earlier in the same statement, assert that a
lock is already held. This catches any future code that adds a permission check
on a referenced object without first acquiring a lock.

This is enabled only for Assert enabled builds so that there is no overhead
for production builds.

I think that v20 is taking care of what has been discussed in this thread, mainly:

- It adds the new lock when the dependency is being recorded and thus avoids
having to modify about 250 call sites (as mentioned by Jeff in [1]).
- It addresses Robert's concern about the permission check before lock TOCTOU
window (thanks to 0002 that modifies about 70 call sites and 0003 that ensures
that we should trap new ones if any).

Thoughts?

[1]: https://postgr.es/m/d721011cd3ec3aedd57b193ef10cf541f50df325.camel%40j-davis.com

Regards,

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

  68. Re: Avoid orphaned objects dependencies, take 3

    Robert Haas <robertmhaas@gmail.com> — 2026-05-13T20:20:21Z

    On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> 0003: Add Assert guard to detect permission check before lock regressions
>
> Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
> is called on a referenced object before a lock is held on it, which would widen
> the TOCTOU window between the permission check and the dependency recording.

I really like the idea of having some kind of cross-check system that
can detect future (or current) coding mistakes. But what I wonder
about this mechanism is: should we instead be insisting that we take a
lock and check permissions on every dependency? Is it an error to
record a dependency on an object without any sort of permissions
check?

Also, I think the mechanism might not be entirely safe. ProcessUtility
can result in executing user-defined functions which could
theoretically run other DDL and then it seems like this code would get
confused.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  69. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-05-18T12:14:34Z

    Hi,

On Wed, May 13, 2026 at 04:20:21PM -0400, Robert Haas wrote:
> On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > 0003: Add Assert guard to detect permission check before lock regressions
> >
> > Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
> > is called on a referenced object before a lock is held on it, which would widen
> > the TOCTOU window between the permission check and the dependency recording.
> 
> I really like the idea of having some kind of cross-check system that
> can detect future (or current) coding mistakes.

Thanks for the feedback! BTW, it detected a new one due to 4793fc41f82, so v21
attached does fix it to make the CI green.

> But what I wonder
> about this mechanism is: should we instead be insisting that we take a
> lock and check permissions on every dependency? Is it an error to
> record a dependency on an object without any sort of permissions
> check?

I'm not sure. For example, currently, without execute privilege on myfunc(), one
could create a view like:

CREATE VIEW v1 AS SELECT myfunc(a) FROM t1;

so that the dependency is recorded.

The execution permission is checked when the view is queried. I don't think this
example is a bug, so that I'm doubtful about insisting that we take a lock and
check permissions on every dependency.

> Also, I think the mechanism might not be entirely safe. ProcessUtility
> can result in executing user-defined functions which could
> theoretically run other DDL and then it seems like this code would get
> confused.

The assert fires when an aclcheck was tracked and the lock is not held. Since
the locks are transaction-scoped, any lock acquired during the statement persists,
so the assert passes regardless of nesting. So that looks not possible to get false
positives (assert fires) due to user-defined functions running other DDL.

The concern would be false negatives (missed detection) due to the reset wiping
the outer DDL's entries.

I believe, that's theoretically possible only if:

- The outer DDL has a P1 bug (aclcheck without lock on object X)
- A user-defined function happens to run DDL that also aclchecks and locks the same
object X
- The user-defined function DDL's lock satisfies the assert for the outer DDL

But then, the bug would be caught in normal testing without the user-defined
function being present.

So I'm not sure how this code could get confused. Do you have an example of what
you have in mind?

Regards,

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

  70. Re: Avoid orphaned objects dependencies, take 3

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-05-26T18:00:11Z

    On 18/05/2026 15:14, Bertrand Drouvot wrote:
> On Wed, May 13, 2026 at 04:20:21PM -0400, Robert Haas wrote:
>> On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
>> <bertranddrouvot.pg@gmail.com> wrote:
>>> 0003: Add Assert guard to detect permission check before lock regressions
>>>
>>> Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
>>> is called on a referenced object before a lock is held on it, which would widen
>>> the TOCTOU window between the permission check and the dependency recording.
>>
>> I really like the idea of having some kind of cross-check system that
>> can detect future (or current) coding mistakes.
> 
> Thanks for the feedback! BTW, it detected a new one due to 4793fc41f82, so v21
> attached does fix it to make the CI green.

I had a closer look at patch 0001. It doesn't fix all the problems, we 
definitely need patch 0002/0003 too, but it's a good step forward and I 
think it makes sense to commit it independently. So I'm focusing on that 
now.

I noticed we are already doing essentially the same thing for shared 
dependencies, in shdepLockAndCheckObject(). I see that you even copied 
the function comment from shdepLockAndCheckObject() to 
LockNotPinnedObject(), but I didn't see it being otherwise mentioned in 
this thread. In any case, that's a good argument for doing the same for 
non-shared dependencies that we already do for shared dependencies.

> +	if (object->classId == RelationRelationId)
> +	{
> +		/* skip shared relations as they are pinned */
> +		if (IsSharedRelation(object->objectId))
> +			return;
> +
> +		/*
> +		 * We must be in one of the two following cases that would already
> +		 * prevent the relation to be dropped: 1. The relation is already
> +		 * locked (could be an existing relation or a relation that we are
> +		 * creating). 2. The relation is protected indirectly (i.e an index
> +		 * protected by a lock on its table, a table protected by a lock on a
> +		 * function that depends of the table...). To avoid any risks, acquire
> +		 * a lock if there is none. That may add unnecessary lock for 2. but
> +		 * that's worth it.
> +		 */
> +		if (!CheckRelationOidLockedByMe(object->objectId, AccessShareLock, true))
> +			LockRelationOid(object->objectId, AccessShareLock);
> +		return;
> +	}

Hmm, shouldn't that re-check that the relation exists, after acquiring 
the lock, like the non-relation codepath does? Otherwise it's a little 
pointless to acquire the lock.

I did a bunch of little refactorings and ended up with the attached. 
Notable changes:

- I renamed and moved LockNotPinnedObject() into 
dependencyLockAndCheckObject(), for consistency with 
shdepLockAndCheckObject().

- Removed ObjectByIdExist(), inlined it into the caller. The argument to 
use SnapshotSelf or not seemed a bit too special to be generally useful

- Changed the error message and code to rhyme with the existing "role 
<OID> was concurrently dropped" errors. I didn't add the OID to the 
error message however, because that makes the testing hard. It'd be nice 
to have a general masking mechanism for OIDs and XIDs in error messages 
in tests, but now is not the time for that.

- Added a test for a dependency on a role too, to cover the existing 
shdepLockAndCheckObject() function. It's currently disabled because it 
prints the OID, though.


- Heikki

  71. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-05-26T20:18:50Z

    Hi,

On Tue, May 26, 2026 at 09:00:11PM +0300, Heikki Linnakangas wrote:
> On 18/05/2026 15:14, Bertrand Drouvot wrote:
> > On Wed, May 13, 2026 at 04:20:21PM -0400, Robert Haas wrote:
> > > On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
> > > <bertranddrouvot.pg@gmail.com> wrote:
> > > > 0003: Add Assert guard to detect permission check before lock regressions
> > > > 
> > > > Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
> > > > is called on a referenced object before a lock is held on it, which would widen
> > > > the TOCTOU window between the permission check and the dependency recording.
> > > 
> > > I really like the idea of having some kind of cross-check system that
> > > can detect future (or current) coding mistakes.
> > 
> > Thanks for the feedback! BTW, it detected a new one due to 4793fc41f82, so v21
> > attached does fix it to make the CI green.
> 
> I had a closer look at patch 0001.

Thanks!

> It doesn't fix all the problems, we
> definitely need patch 0002/0003 too, but it's a good step forward and I
> think it makes sense to commit it independently. 

Yeah, this will prevent orphaned objects which is a good step forward.

> So I'm focusing on that
> now.

Thanks! I'll resume working on something similar to 0002 and 0003 once 0001 gets
in.

> I noticed we are already doing essentially the same thing for shared
> dependencies, in shdepLockAndCheckObject(). I see that you even copied the
> function comment from shdepLockAndCheckObject() to LockNotPinnedObject(),
> but I didn't see it being otherwise mentioned in this thread. In any case,
> that's a good argument for doing the same for non-shared dependencies that
> we already do for shared dependencies.

Right.

> > +	if (object->classId == RelationRelationId)
> > +	{
> > +		/* skip shared relations as they are pinned */
> > +		if (IsSharedRelation(object->objectId))
> > +			return;
> > +
> > +		/*
> > +		 * We must be in one of the two following cases that would already
> > +		 * prevent the relation to be dropped: 1. The relation is already
> > +		 * locked (could be an existing relation or a relation that we are
> > +		 * creating). 2. The relation is protected indirectly (i.e an index
> > +		 * protected by a lock on its table, a table protected by a lock on a
> > +		 * function that depends of the table...). To avoid any risks, acquire
> > +		 * a lock if there is none. That may add unnecessary lock for 2. but
> > +		 * that's worth it.
> > +		 */
> > +		if (!CheckRelationOidLockedByMe(object->objectId, AccessShareLock, true))
> > +			LockRelationOid(object->objectId, AccessShareLock);
> > +		return;
> > +	}
> 
> Hmm, shouldn't that re-check that the relation exists, after acquiring the
> lock, like the non-relation codepath does? Otherwise it's a little pointless
> to acquire the lock.

Good catch! Your change:

+               if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true))
+                       return;
+               LockRelationOid(objectId, AccessShareLock);
+
+               if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId)))
+                       return;
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                errmsg("dependent relation was concurrently dropped")));

Looks good to me.

> I did a bunch of little refactorings and ended up with the attached. Notable
> changes:
> 
> - I renamed and moved LockNotPinnedObject() into
> dependencyLockAndCheckObject(), for consistency with
> shdepLockAndCheckObject().

Makes sense to me.

> - Removed ObjectByIdExist(), inlined it into the caller. The argument to use
> SnapshotSelf or not seemed a bit too special to be generally useful

Yeah, better to have this logic inlined as it will probably not be useful outside
of it.

> - Changed the error message and code to rhyme with the existing "role <OID>
> was concurrently dropped" errors. I didn't add the OID to the error message
> however, because that makes the testing hard.

Agreed that's better.

> - Added a test for a dependency on a role too, to cover the existing
> shdepLockAndCheckObject() function. It's currently disabled because it
> prints the OID, though.

Nit: It also adds:

+# function - role
+permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit"

That is not disabled and would already pass without the changes added in 0001.

So I wonder if we could just start by adding this new test (and the XXX one) in
a dedicated patch and then add 0001 that would focus on orphaned stuff only.

A few comments:

1/

+#include "catalog/index.h"

That doesn't look needed.

2/

It looks like pgindent "complains" on pg_depend.c for dependencyLockAndCheckObject().

Regards,

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

  72. Re: Avoid orphaned objects dependencies, take 3

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-05-27T15:53:35Z

    On 26/05/2026 23:18, Bertrand Drouvot wrote:
> On Tue, May 26, 2026 at 09:00:11PM +0300, Heikki Linnakangas wrote:
>> - Added a test for a dependency on a role too, to cover the existing
>> shdepLockAndCheckObject() function. It's currently disabled because it
>> prints the OID, though.
> 
> Nit: It also adds:
> 
> +# function - role
> +permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit"
> 
> That is not disabled and would already pass without the changes added in 0001.
> 
> So I wonder if we could just start by adding this new test (and the XXX one) in
> a dedicated patch and then add 0001 that would focus on orphaned stuff only.

True, we could do that. It doesn't really seem to me any better or worse 
than just including it in the same commit though.

> A few comments:
> 
> 1/
> 
> +#include "catalog/index.h"
> 
> That doesn't look needed.
> 
> 2/
> 
> It looks like pgindent "complains" on pg_depend.c for dependencyLockAndCheckObject().

Fixed these, and did some more copy-editing on the comments. With that, 
committed and backpatched. Version 14 needed a little more work to 
backport because it didn't have IsPinnedObject(). And I noticed that 
recordDependencyFor() was being called with InvalidOid on ALTER TABLE 
commands on tables with dropped columns. That was bogus but harmless 
before this patch; I fixed that too.

Thanks!

- Heikki

  73. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-01T09:21:39Z

    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

  74. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-03T18:08:13Z

    On Mon, 2026-06-01 at 09:21 +0000, Bertrand Drouvot wrote:
> 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.

RangeVarGetRelidExtended() coordinates three things:

  - name lookup
  - lock
  - ACL check

whereas recheckAclAndLock() only coordinates the latter two. That means
there can still be some strange failures, like:

  -- Session 1
  BEGIN;
  DROP SCHEMA s2;

  -- Session 2
  SET search_path=s2, s1;
  CREATE FUNCTION f() RETURNS INT LANGUAGE plpgsql AS
    $$ BEGIN RETURN 42; END; $$;

  -- Session 1
  COMMIT;

  -- Session 2
  ERROR:  referenced schema was concurrently dropped

even though no schema was actually referenced in the query, and a retry
of the transaction successfully creates the function in s1. Is that
expected?

Regards,
	Jeff Davis

  75. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-04T17:03:22Z

    On Mon, 2026-06-01 at 09:21 +0000, Bertrand Drouvot wrote:
> 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.

This could use some better high-level comments in the code. Something
like:

"DDL performs ACL checks on referenced objects before acquiring a lock
on them. The lock is acquired much later, when recording dependencies.
Track the ACL checks, so that we can re-check them after acquiring the
lock. XXX: consider refactoring so that we perform the name lookup,
acquire the lock, and check ACLs all in unison, like
RangeVarGetRelidExtended()."

Assuming I understand correctly.

Regards,
	Jeff Davis

  76. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-04T17:17:57Z

    On Wed, 2026-06-03 at 11:08 -0700, Jeff Davis wrote:
> RangeVarGetRelidExtended() coordinates three things:
> 
>   - name lookup
>   - lock
>   - ACL check
> 
> whereas recheckAclAndLock() only coordinates the latter two.

Given that we don't do another name lookup, the object Oid doesn't
change, and it's not obvious why we need a loop in this path.

A sequence like:

Earlier during DDL processing:
  0. Name lookup and ACL check (and track ACLs)

When recording dependencies:
  1. Lock object
  2. Check that it still exists, error if not
  3. recheck tracked ACLs, error if failure

could work too, right?

I see why you might want to do the checks while not holding the lock,
but it doesn't seem like a requirement (if the user doesn't have
permissions it should fail quickly and release). In any case, it's
worth a comment explaining why the loop is there.

Regards,
	Jeff Davis

  77. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-05T13:09:37Z

    Hi,

On Thu, Jun 04, 2026 at 10:03:22AM -0700, Jeff Davis wrote:
> On Mon, 2026-06-01 at 09:21 +0000, Bertrand Drouvot wrote:
> > 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.
> 
> This could use some better high-level comments in the code. Something
> like:
> 
> "DDL performs ACL checks on referenced objects before acquiring a lock
> on them. The lock is acquired much later, when recording dependencies.
> Track the ACL checks, so that we can re-check them after acquiring the
> lock.

Agreed, I just re-worded a bit to add some nuance in v24 attached, as:

"
may perform ACL checks on referenced objects without first holding a lock on
them. In that case, the lock is acquired much later, when recording the
dependencies. Track the ACL checks, so that we can re-check them after acquiring
the lock while recording dependencies."

> XXX: consider refactoring so that we perform the name lookup,
> acquire the lock, and check ACLs all in unison, like
> RangeVarGetRelidExtended()."

I like the XXX, as I agree that what you mentioned in [1] is an area of improvement
but not something directly linked to the TOCTOU issue that this patch is addressing.

FWIW, with the scenario you described in [1]:

- before 2fbb21170e9, the function would be linked to a non existing schema
- with 2fbb21170e9, it produces "ERROR:  referenced schema was concurrently dropped"

the current patch just "keep" the post 2fbb21170e9 behavior.

[1]: https://postgr.es/m/5315d15a42109297259d1a3264ad09e363eb98df.camel%40j-davis.com

Regards,

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

  78. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-05T13:12:34Z

    Hi,

On Thu, Jun 04, 2026 at 10:17:57AM -0700, Jeff Davis wrote:
> Given that we don't do another name lookup, the object Oid doesn't
> change, and it's not obvious why we need a loop in this path.
> 
> A sequence like:
> 
> Earlier during DDL processing:
>   0. Name lookup and ACL check (and track ACLs)
> 
> When recording dependencies:
>   1. Lock object
>   2. Check that it still exists, error if not
>   3. recheck tracked ACLs, error if failure
> 
> could work too, right?

Yeah, that's a good point, thanks!

> I see why you might want to do the checks while not holding the lock,
> but it doesn't seem like a requirement (if the user doesn't have
> permissions it should fail quickly and release).

I agree that we can get rid of the loop and that's what v24 shared up-thread [1]
is now implementing. The downside is that one could take a lock on an object
he has no privileges on but then, as you said, it will be released shortly after.

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

Regards,

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

  79. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-08T21:55:19Z

    On Fri, 2026-06-05 at 13:09 +0000, Bertrand Drouvot wrote:
> the current patch

  /*
   * Look up a tracked aclcheck entry for the given object.
   * Returns true if found, filling in roleId, mode, and inval_count.
   * Searches from the end to find the most recent check (the one with
the
   * freshest inval_count).

What if two tracked ACL checks had different modes? Don't you need to
do both checks or somehow combine the modes in the tracking entry?

(I'm not sure if this is a practical problem or not.)

Regards,
	Jeff Davis

  80. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-09T06:47:06Z

    Hi,

On Mon, Jun 08, 2026 at 02:55:19PM -0700, Jeff Davis wrote:
> On Fri, 2026-06-05 at 13:09 +0000, Bertrand Drouvot wrote:
> > the current patch
> 
>   /*
>    * Look up a tracked aclcheck entry for the given object.
>    * Returns true if found, filling in roleId, mode, and inval_count.
>    * Searches from the end to find the most recent check (the one with
> the
>    * freshest inval_count).
> 
> What if two tracked ACL checks had different modes? Don't you need to
> do both checks or somehow combine the modes in the tracking entry?
> 
> (I'm not sure if this is a practical problem or not.)

Humm, good point. I was focusing on a "full" role revoke but maybe what you
describe could be a practical problem (in case of privilege revoke from the
role).

In order to be on the safe side of things, the attached now iterates through all
matching entries (and not only the last one). It's still a flat array as its
linear scan is O(n) in the number of tracked entries, but that's fine since a
typical DDL statement tracks only a few objects.

Regards,

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

  81. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-09T18:04:20Z

    Hi,

On Tue, Jun 09, 2026 at 06:47:06AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Jun 08, 2026 at 02:55:19PM -0700, Jeff Davis wrote:
> > On Fri, 2026-06-05 at 13:09 +0000, Bertrand Drouvot wrote:
> > > the current patch
> > 
> >   /*
> >    * Look up a tracked aclcheck entry for the given object.
> >    * Returns true if found, filling in roleId, mode, and inval_count.
> >    * Searches from the end to find the most recent check (the one with
> > the
> >    * freshest inval_count).
> > 
> > What if two tracked ACL checks had different modes? Don't you need to
> > do both checks or somehow combine the modes in the tracking entry?
> > 
> > (I'm not sure if this is a practical problem or not.)
> 
> Humm, good point. I was focusing on a "full" role revoke but maybe what you
> describe could be a practical problem (in case of privilege revoke from the
> role).
> 
> In order to be on the safe side of things, the attached now iterates through all
> matching entries (and not only the last one). It's still a flat array as its
> linear scan is O(n) in the number of tracked entries, but that's fine since a
> typical DDL statement tracks only a few objects.

I just realized that the cfbot is complaining, let me fix it and come back with
an updated patch.

Regards,

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

  82. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-09T21:44:10Z

    Hi,

On Tue, 2026-06-09 at 06:47 +0000, Bertrand Drouvot wrote:
> In order to be on the safe side of things, the attached now iterates
> through all
> matching entries (and not only the last one).

I don't think:

  if (context == PROCESS_UTILITY_TOPLEVEL)

is quite right. With SPI, I bet you can end up in a situation where the
branch fires twice (causing an early reset) or not at all (because the
only call is PROCESS_UTLITY_QUERY).

In any case, I don't think mixing the tracking entries between entirely
different DDL commands is a good idea. If you execute DDL inside of SPI
in its own subtransaction, and it inserts a tracking entry to recheck
something, and then you roll back the subxact, you don't want the
tracking entry to then cause the outer transaction to fail. I didn't
test this, so if there's something preventing this kind of problem, let
me know.

We probably need to track where we are in the stack of ProcessUtility()
calls and keep the tracking entries separate, and always remove entries
from that level on return (or rollback).

Regards,
	Jeff Davis

  83. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-10T12:31:04Z

    Hi,

On Tue, Jun 09, 2026 at 02:44:10PM -0700, Jeff Davis wrote:
> Hi,
> 
> On Tue, 2026-06-09 at 06:47 +0000, Bertrand Drouvot wrote:
> > In order to be on the safe side of things, the attached now iterates
> > through all
> > matching entries (and not only the last one).
> 
> I don't think:
> 
>   if (context == PROCESS_UTILITY_TOPLEVEL)
> 
> is quite right.
> 
> In any case, I don't think mixing the tracking entries between entirely
> different DDL commands is a good idea. If you execute DDL inside of SPI
> in its own subtransaction, and it inserts a tracking entry to recheck
> something, and then you roll back the subxact, you don't want the
> tracking entry to then cause the outer transaction to fail. I didn't
> test this, so if there's something preventing this kind of problem, let
> me know.
> 
> We probably need to track where we are in the stack of ProcessUtility()
> calls and keep the tracking entries separate, and always remove entries
> from that level on return (or rollback).

I think you are right. I changed this in the attached so that aclcheck_tracked_count
is saved on entry and restored on return of each ProcessUtility call, so that each
nesting level's entries are kept separate.

Also, the reset is now done whenever tracking is initialized (means at the
outermost ProcessUtility call that starts tracking).

I think that addresses your concerns and it also fixes the issues reported
by the cfbot (that I mentioned up-thread).

I also added more tests related to nested calls.

Regards,

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

  84. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-10T15:16:56Z

    Hi,

On Wed, Jun 10, 2026 at 12:31:04PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Jun 09, 2026 at 02:44:10PM -0700, Jeff Davis wrote:
> > Hi,
> > 
> > On Tue, 2026-06-09 at 06:47 +0000, Bertrand Drouvot wrote:
> > > In order to be on the safe side of things, the attached now iterates
> > > through all
> > > matching entries (and not only the last one).
> > 
> > I don't think:
> > 
> >   if (context == PROCESS_UTILITY_TOPLEVEL)
> > 
> > is quite right.
> > 
> > In any case, I don't think mixing the tracking entries between entirely
> > different DDL commands is a good idea. If you execute DDL inside of SPI
> > in its own subtransaction, and it inserts a tracking entry to recheck
> > something, and then you roll back the subxact, you don't want the
> > tracking entry to then cause the outer transaction to fail. I didn't
> > test this, so if there's something preventing this kind of problem, let
> > me know.
> > 
> > We probably need to track where we are in the stack of ProcessUtility()
> > calls and keep the tracking entries separate, and always remove entries
> > from that level on return (or rollback).
> 
> I think you are right. I changed this in the attached so that aclcheck_tracked_count
> is saved on entry and restored on return of each ProcessUtility call, so that each
> nesting level's entries are kept separate.
> 
> Also, the reset is now done whenever tracking is initialized (means at the
> outermost ProcessUtility call that starts tracking).
> 
> I think that addresses your concerns and it also fixes the issues reported
> by the cfbot (that I mentioned up-thread).
> 
> I also added more tests related to nested calls.

PFA a new version of v26, it adds a new test as compared to the v26 previously
shared.

Regards,

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

  85. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-16T01:19:19Z

    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.

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?

Regards,
	Jeff Davis

  86. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-16T10:09:48Z

    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

  87. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-16T19:14:12Z

    On Tue, 2026-06-16 at 10:09 +0000, Bertrand Drouvot wrote:
> 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.

IIUC, this is necessary for correctness. If an ACL failure doesn't
cause a transaction abort, then there's a danger that we cause the
transaction to fail that should have succeeded.

So the ACL tracking needs to be precise: we can't track an ACL check
unless a failure always causes transaction abort; and we must track an
ACL check if it would cause a transaction abort. Right?

Regards,
	Jeff Davis

  88. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-17T05:44:47Z

    Hi,

On Tue, Jun 16, 2026 at 12:14:12PM -0700, Jeff Davis wrote:
> On Tue, 2026-06-16 at 10:09 +0000, Bertrand Drouvot wrote:
> > 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.
> 
> IIUC, this is necessary for correctness. If an ACL failure doesn't
> cause a transaction abort, then there's a danger that we cause the
> transaction to fail that should have succeeded.

Exactly, because we'd recheck an "harmless" failed ACL check and then produce
an error.

> So the ACL tracking needs to be precise: we can't track an ACL check
> unless a failure always causes transaction abort; and we must track an
> ACL check if it would cause a transaction abort. Right?

I would say: we just need to track (and recheck) ACL checks that succeeded.

I think that there is no reason to recheck (and so to record) a failed ACL as what
we are dealing with here is the TOCTOU window. Re-checking a failed ACL check would
handle cases when a GRANT has been given during the TOCTOU window which is not
useful (for our protection goal) compared to re-checking a REVOKE during the
TOCTOU window (as the latter would record a dependency on an object we don't have
permission on).

Doing so, as proposed in 0002, allows us to fix the "re-check a harmless failed
ACL bug" (demonstrated by the added test) and still protect us for REVOKE during
the TOCTOU window.

Thoughts?

Regards,

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

  89. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-18T23:21:33Z

    On Wed, 2026-06-17 at 05:44 +0000, Bertrand Drouvot wrote:
> > IIUC, this is necessary for correctness. If an ACL failure doesn't
> > cause a transaction abort, then there's a danger that we cause the
> > transaction to fail that should have succeeded.
> 
> Exactly, because we'd recheck an "harmless" failed ACL check and then
> produce
> an error.
> 
> > So the ACL tracking needs to be precise: we can't track an ACL
> > check
> > unless a failure always causes transaction abort; and we must track
> > an
> > ACL check if it would cause a transaction abort. Right?
> 
> I would say: we just need to track (and recheck) ACL checks that
> succeeded.

IIUC, we cannot have false positives (tracking ACL checks that wouldn't
have caused an abort) nor can we have false negatives (missing an ACL
check that could cause an abort).

It's hard for me to convince myself that we got all the cases right;
and if we have, that they won't be broken in the future.

For instance, I just realized that something else I'm working on is
related:

https://www.postgresql.org/message-id/5c629d2455946ad2fde3c184f64ea2c323ef2133.camel@j-davis.com

It does an ACL check inside a subtransaction, where the parent
transaction is a DDL statement. It happens to be a DROP statement, so
it's not recording new dependencies, so I don't think it breaks your
tracking mechanism, but it's too close for comfort.

We could keep the transaction ID in the tracking record, and ignore
entries from an aborted subxact. But it's getting fairly complex and
delicate.

Thoughts?

Regards,
	Jeff Davis

  90. Re: Avoid orphaned objects dependencies, take 3

    Jeff Davis <pgsql@j-davis.com> — 2026-06-19T02:13:38Z

    On Thu, 2026-06-18 at 16:21 -0700, Jeff Davis wrote:
> IIUC, we cannot have false positives (tracking ACL checks that
> wouldn't
> have caused an abort) nor can we have false negatives (missing an ACL
> check that could cause an abort).

Idea: what if we check for changes in ACLs on the object, rather than
whether it passes the check or not?

Then, if track an ACL check that wouldn't actually cause a failure,
then it still might be acceptable to throw an error if the ACL changes.
Still some details to sort out, so this is just an idea.

Regards,
	Jeff Davis

  91. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-19T13:28:21Z

    Hi,

On Thu, Jun 18, 2026 at 04:21:33PM -0700, Jeff Davis wrote:
> On Wed, 2026-06-17 at 05:44 +0000, Bertrand Drouvot wrote:
> > > IIUC, this is necessary for correctness. If an ACL failure doesn't
> > > cause a transaction abort, then there's a danger that we cause the
> > > transaction to fail that should have succeeded.
> > 
> > Exactly, because we'd recheck an "harmless" failed ACL check and then
> > produce
> > an error.
> > 
> > > So the ACL tracking needs to be precise: we can't track an ACL
> > > check
> > > unless a failure always causes transaction abort; and we must track
> > > an
> > > ACL check if it would cause a transaction abort. Right?
> > 
> > I would say: we just need to track (and recheck) ACL checks that
> > succeeded.
> 
> IIUC, we cannot have false positives (tracking ACL checks that wouldn't
> have caused an abort) nor can we have false negatives (missing an ACL
> check that could cause an abort).

Right.

> It's hard for me to convince myself that we got all the cases right;
> and if we have, that they won't be broken in the future.
> 
> For instance, I just realized that something else I'm working on is
> related:
> 
> https://www.postgresql.org/message-id/5c629d2455946ad2fde3c184f64ea2c323ef2133.camel@j-davis.com
> 
> It does an ACL check inside a subtransaction, where the parent
> transaction is a DDL statement. It happens to be a DROP statement, so
> it's not recording new dependencies, so I don't think it breaks your
> tracking mechanism, but it's too close for comfort.

I see, I don't think we have this pattern currently but yeah we may have it
in the future (and our current tracking mechanism would probably fail in such a
case).

> We could keep the transaction ID in the tracking record, and ignore
> entries from an aborted subxact. But it's getting fairly complex and
> delicate.

Agreed. We could use RegisterSubXactCallback to save/restore the entry count on
subtransaction abort, but as you say it adds complexity.

Regards,

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

  92. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-19T13:36:45Z

    Hi,

On Thu, Jun 18, 2026 at 07:13:38PM -0700, Jeff Davis wrote:
> On Thu, 2026-06-18 at 16:21 -0700, Jeff Davis wrote:
> > IIUC, we cannot have false positives (tracking ACL checks that
> > wouldn't
> > have caused an abort) nor can we have false negatives (missing an ACL
> > check that could cause an abort).
> 
> Idea: what if we check for changes in ACLs on the object, rather than
> whether it passes the check or not?
> 
> Then, if track an ACL check that wouldn't actually cause a failure,
> then it still might be acceptable to throw an error if the ACL changes.
> Still some details to sort out, so this is just an idea.

Yeah, I think I do prefer this idea. As you say, that could cause an error even
if the ACL change does not REVOKE anything on this object (say the ACL change is
a GRANT), but that should be rare in practice and probably much simpler to reason
about that way.

But I don't think tracking ACL changes would be enough though. I think we would
also need to track ROLE changes.

So what about?

- Save a copy of the object's ACL and compare at recheck time: If not the same,
then error out.
- Save the ROLE membership and compare at recheck time. If not the same, then
error out.

That way we cover both parts: the object's ACL and the ROLE membership.

That's just a high level idea, I can move forward and try to implement it.

Thoughts?

Regards,

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

  93. Re: Avoid orphaned objects dependencies, take 3

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-06-25T11:10:58Z

    Hi,

On Fri, Jun 19, 2026 at 01:28:21PM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Thu, Jun 18, 2026 at 04:21:33PM -0700, Jeff Davis wrote:
> 
> > We could keep the transaction ID in the tracking record, and ignore
> > entries from an aborted subxact. But it's getting fairly complex and
> > delicate.

As discussed off-list, let's move forward with this proposal (as the save a copy
of the object's ACL + ROLE membership mentioned in [1] would be more complex and
fragile).

So PFA the new version doing so. It's the same patch series as the previous one
except that it adds:

0004: Discard ACL tracking entries from aborted subtransactions

It stores SubTransactionId in each AclCheckEntry at recording time.
On subtransaction abort, AtEOSubXact_AclTrack() discards entries whose subxid
is >= the aborting subtransaction's ID.

The code is pretty simple but I suspect that adding a test would probably need to
create a brand new module (say test_acltrack) with C code forcing
recordDependencyOn() and BeginInternalSubTransaction()) and I'm not sure it's
worth it. Indeed, I think the test would need to do things similar to what you
shared in [2].

The patch is split to ease the review but should probably be merged as a single
patch.

[1]: https://postgr.es/m/ajVF7YWf%2Bpxs4cOf%40bdtpg
[2]: https://postgr.es/m/5c629d2455946ad2fde3c184f64ea2c323ef2133.camel@j-davis.com

Regards,

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