New Object Access Type hooks

On Thu, Mar 17, 2022 at 11:21 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

Hackers,

Over in [1], Joshua proposed a new set of Object Access Type hooks based on strings rather than Oids.

His patch was written to be applied atop my patch for granting privileges on gucs.

On review of his patch, I became uncomfortable with the complete lack of regression test coverage. To be fair, he did paste a bit of testing logic to the thread, but it appears to be based on pgaudit, and it is unclear how to include such a test in the core project, where pgaudit is not assumed to be installed.

First, I refactored his patch to work against HEAD and not depend on my GUCs patch. Find that as v1-0001. The refactoring exposed a bit of a problem. To call the new hook for SET and ALTER SYSTEM commands, I need to pass in the Oid of a catalog table. But since my GUC patch isn't applied yet, there isn't any such table (pg_setting_acl or whatnot) to pass. So I'm passing InvalidOid, but I don't know if that is right. In any event, if we want a new API like this, we should think a bit harder about whether it can be used to check operations where no table Oid is applicable.

Second, I added a new test directory, src/test/modules/test_oat_hooks, which includes a new loadable module with hook implementations and a regression test for testing the object access hooks. The main point of the test is to log which hooks get called in which order, and which hooks do or do not get called when other hooks allow or deny access. That information shows up in the expected output as NOTICE messages.

This second patch has gotten a little long, and I'd like another pair of eyes on this before spending a second day on the effort. Please note that this is a quick WIP patch in response to the patch Joshua posted earlier today. Sorry for sometimes missing function comments, etc. The goal, if this design seems acceptable, is to polish this, hopefully with Joshua's assistance, and get it committed *before* my GUCs patch, so that my patch can be rebased to use it. Otherwise, if this is rejected, I can continue on the GUC patch without this.

This is great, thank you for doing this. I didn't even realize the OAT
hooks had no regression tests.

It looks good to me, I reviewed both and tested the module. I wonder
if the slight abuse of subid is warranted with brand new hooks going
in but not enough to object, I just hope this doesn't rise to the too
large to merge this late level.

On Mar 18, 2022, at 7:16 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:

This is great, thank you for doing this. I didn't even realize the OAT
hooks had no regression tests.

It looks good to me, I reviewed both and tested the module. I wonder
if the slight abuse of subid is warranted with brand new hooks going
in but not enough to object, I just hope this doesn't rise to the too
large to merge this late level.

The majority of the patch is regression testing code, stuff which doesn't get installed. It's even marked as NO_INSTALLCHECK, so it won't get installed even as part of "make installcheck". That seems safe enough to me.

Not including tests of OAT seems worse, as it leaves us open to breaking the behavior without realizing we've done so. A refactoring of the core code might cause hooks to be called in a different order, something which isn't necessarily wrong, but should not be done unknowingly.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/18/22 11:15, Mark Dilger wrote:

On Mar 18, 2022, at 7:16 AM, Joshua Brindle <joshua.brindle@crunchydata.com> wrote:

This is great, thank you for doing this. I didn't even realize the OAT
hooks had no regression tests.

It looks good to me, I reviewed both and tested the module. I wonder
if the slight abuse of subid is warranted with brand new hooks going
in but not enough to object, I just hope this doesn't rise to the too
large to merge this late level.

The core code is extracted from a current CF patch, so I think in
principle it's OK.

I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.

The majority of the patch is regression testing code, stuff which doesn't get installed. It's even marked as NO_INSTALLCHECK, so it won't get installed even as part of "make installcheck". That seems safe enough to me.

Not including tests of OAT seems worse, as it leaves us open to breaking the behavior without realizing we've done so. A refactoring of the core code might cause hooks to be called in a different order, something which isn't necessarily wrong, but should not be done unknowingly.

Yes, and in any case we've added test code after feature freeze in the past.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/17/22 23:21, Mark Dilger wrote:

Hackers,

Over in [1], Joshua proposed a new set of Object Access Type hooks based on strings rather than Oids.

His patch was written to be applied atop my patch for granting privileges on gucs.

On review of his patch, I became uncomfortable with the complete lack of regression test coverage. To be fair, he did paste a bit of testing logic to the thread, but it appears to be based on pgaudit, and it is unclear how to include such a test in the core project, where pgaudit is not assumed to be installed.

First, I refactored his patch to work against HEAD and not depend on my GUCs patch. Find that as v1-0001. The refactoring exposed a bit of a problem. To call the new hook for SET and ALTER SYSTEM commands, I need to pass in the Oid of a catalog table. But since my GUC patch isn't applied yet, there isn't any such table (pg_setting_acl or whatnot) to pass. So I'm passing InvalidOid, but I don't know if that is right. In any event, if we want a new API like this, we should think a bit harder about whether it can be used to check operations where no table Oid is applicable.

My first inclination is to say it's probably ok. The immediately obvious
alternative would be to create yet another set of functions that don't
have classId parameters. That doesn't seem attractive.

Modulo that issue I think patch 1 is basically ok, but we should fix the
comments in objectaccess.c.  Rather than "It is [the] entrypoint ..." we
should have something like "Oid variant entrypoint ..." and "Name
variant entrypoint ...", and also fix the function names in the comments.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mar 18, 2022, at 3:04 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.

The .control file exists because the test defines a loadable module which defines the hooks. The test_oat_hooks.h file was extraneous, and has been removed in v2.

On Mar 21, 2022, at 8:41 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

My first inclination is to say it's probably ok. The immediately obvious
alternative would be to create yet another set of functions that don't
have classId parameters. That doesn't seem attractive.

Modulo that issue I think patch 1 is basically ok, but we should fix the
comments in objectaccess.c. Rather than "It is [the] entrypoint ..." we
should have something like "Oid variant entrypoint ..." and "Name
variant entrypoint ...", and also fix the function names in the comments.

Joshua,

Do you care to create a new version of this, perhaps based on the v2-0001 patch I just posted?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/21/22 15:57, Mark Dilger wrote:

On Mar 18, 2022, at 3:04 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

I haven't looked at it in detail, but regarding the test code I'm not
sure why there's a .control file, since this isn't a loadable extension,
not why there's a test_oat_hooks.h file.

The .control file exists because the test defines a loadable module which defines the hooks.

To the best of my knowledge .control files are only used by extensions,
not by other modules. They are only referenced in
src/backend/commands/extension.c in the backend code. For example,
auto_explain which is a loadable module but not en extension does not
have one, and I bet if you remove it you'll find this will work just fine.

cheers

andrew

--

Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mar 21, 2022, at 1:30 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

To the best of my knowledge .control files are only used by extensions,
not by other modules. They are only referenced in
src/backend/commands/extension.c in the backend code. For example,
auto_explain which is a loadable module but not en extension does not
have one, and I bet if you remove it you'll find this will work just fine.

Fixed, also with adjustments to Joshua's function comments.

On Fri, Mar 18, 2022 at 4:22 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

(FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when testing v1-0001. I'm not sure yet what that is about.)

Doesn't look like 0001 has anything to do with that... Are you on a
Mac? Did it look like this recent failure from CI?

https://cirrus-ci.com/task/4686108033286144
https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/regress_log_013_crash_restart
https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/013_crash_restart_primary.log

I have no idea what is going on there, but searching for discussion
brought me here...

On Mar 21, 2022, at 10:03 PM, Thomas Munro <thomas.munro@gmail.com> wrote:

On Fri, Mar 18, 2022 at 4:22 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

(FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when testing v1-0001. I'm not sure yet what that is about.)

Doesn't look like 0001 has anything to do with that... Are you on a
Mac?

Yes, macOS Catalina, currently 10.15.7.

Did it look like this recent failure from CI?

https://cirrus-ci.com/task/4686108033286144
https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/regress_log_013_crash_restart
https://api.cirrus-ci.com/v1/artifact/task/4686108033286144/log/src/test/recovery/tmp_check/log/013_crash_restart_primary.log

I no longer have the logs for comparison.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/22/22 01:03, Thomas Munro wrote:

On Fri, Mar 18, 2022 at 4:22 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:

(FYI, I got a test failure from src/test/recovery/t/013_crash_restart.pl when testing v1-0001. I'm not sure yet what that is about.)

Doesn't look like 0001 has anything to do with that... Are you on a
Mac? Did it look like this recent failure from CI?

Probably not connected. It's working fine for me on Ubuntu/

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 3/21/22 19:08, Mark Dilger wrote:

On Mar 21, 2022, at 1:30 PM, Andrew Dunstan <andrew@dunslane.net> wrote:

To the best of my knowledge .control files are only used by extensions,
not by other modules. They are only referenced in
src/backend/commands/extension.c in the backend code. For example,
auto_explain which is a loadable module but not en extension does not
have one, and I bet if you remove it you'll find this will work just fine.

Fixed, also with adjustments to Joshua's function comments.

Pushed with slight adjustments - the LOAD was unnecessary as was the
setting of client_min_messages - the latter would have made buildfarm
animals unhappy.

Now you need to re-submit your GUCs patch I think.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On Tue, Mar 22, 2022 at 10:41:05AM -0400, Andrew Dunstan wrote:

Pushed with slight adjustments - the LOAD was unnecessary as was the
setting of client_min_messages - the latter would have made buildfarm
animals unhappy.

For the record this just failed on my buildfarm animal:
https://brekka.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&amp;dt=2022-03-22%2014%3A40%3A10&amp;stg=misc-check.

On Mar 22, 2022, at 8:14 AM, Julien Rouhaud <rjuju123@gmail.com> wrote:

Hi,

On Tue, Mar 22, 2022 at 10:41:05AM -0400, Andrew Dunstan wrote:

Pushed with slight adjustments - the LOAD was unnecessary as was the
setting of client_min_messages - the latter would have made buildfarm
animals unhappy.

For the record this just failed on my buildfarm animal:
https://brekka.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&amp;dt=2022-03-22%2014%3A40%3A10&amp;stg=misc-check.

culicidae is complaining:

==~_~===-=-===~_~== pgsql.build/src/test/modules/test_oat_hooks/log/postmaster.log ==~_~===-=-===~_~==
2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG: starting PostgreSQL 15devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 11.2.0-18) 11.2.0, 64-bit
2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG: listening on Unix socket "/tmp/pg_regress-RiE7x8/.s.PGSQL.6280"
2022-03-22 14:53:27.198 UTC [2167008][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.202 UTC [2167006][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.203 UTC [2167009][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: checkpointer process (PID 2167006) exited with exit code 1
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: terminating any other active server processes
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: shutting down because restart_after_crash is off
2022-03-22 14:53:27.206 UTC [2166986][postmaster][:0] LOG: database system is shut down
==~_~===-=-===~_~== pgsql.build/src/test/modules/test_rls_hooks/log/initdb.log ==~_~===-=-===~_~==

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/22/22 11:26, Mark Dilger wrote:

On Mar 22, 2022, at 8:14 AM, Julien Rouhaud <rjuju123@gmail.com> wrote:

Hi,

On Tue, Mar 22, 2022 at 10:41:05AM -0400, Andrew Dunstan wrote:

Pushed with slight adjustments - the LOAD was unnecessary as was the
setting of client_min_messages - the latter would have made buildfarm
animals unhappy.

For the record this just failed on my buildfarm animal:
https://brekka.postgresql.org/cgi-bin/show_stage_log.pl?nm=lapwing&amp;dt=2022-03-22%2014%3A40%3A10&amp;stg=misc-check.

culicidae is complaining:

==~_~===-=-===~_~== pgsql.build/src/test/modules/test_oat_hooks/log/postmaster.log ==~_~===-=-===~_~==
2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG: starting PostgreSQL 15devel on x86_64-pc-linux-gnu, compiled by gcc (Debian 11.2.0-18) 11.2.0, 64-bit
2022-03-22 14:53:27.175 UTC [2166986][postmaster][:0] LOG: listening on Unix socket "/tmp/pg_regress-RiE7x8/.s.PGSQL.6280"
2022-03-22 14:53:27.198 UTC [2167008][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.202 UTC [2167006][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.203 UTC [2167009][not initialized][:0] FATAL: test_oat_hooks must be loaded via shared_preload_libraries
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: checkpointer process (PID 2167006) exited with exit code 1
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: terminating any other active server processes
2022-03-22 14:53:27.204 UTC [2166986][postmaster][:0] LOG: shutting down because restart_after_crash is off
2022-03-22 14:53:27.206 UTC [2166986][postmaster][:0] LOG: database system is shut down
==~_~===-=-===~_~== pgsql.build/src/test/modules/test_rls_hooks/log/initdb.log ==~_~===-=-===~_~==

That seems quite weird. I'm not sure how it's getting loaded at all if
not via shared_preload_libraries

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

That seems quite weird. I'm not sure how it's getting loaded at all if
not via shared_preload_libraries

Some other animals are showing this:

diff -U3 /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out
--- /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out	2022-03-22 11:57:40.224991011 -0400
+++ /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out	2022-03-22 11:59:59.998983366 -0400
@@ -48,6 +48,8 @@
 SELECT * FROM regress_test_table;
 NOTICE:  in executor check perms: superuser attempting execute
 NOTICE:  in executor check perms: superuser finished execute
+NOTICE:  in executor check perms: superuser attempting execute
+NOTICE:  in executor check perms: superuser finished execute
  t 
 ---
 (0 rows)
@@ -95,6 +97,8 @@
                       ^
 NOTICE:  in executor check perms: non-superuser attempting execute
 NOTICE:  in executor check perms: non-superuser finished execute
+NOTICE:  in executor check perms: non-superuser attempting execute
+NOTICE:  in executor check perms: non-superuser finished execute
  t 
 ---
 (0 rows)
@@ -168,6 +172,8 @@
                       ^
 NOTICE:  in executor check perms: superuser attempting execute
 NOTICE:  in executor check perms: superuser finished execute
+NOTICE:  in executor check perms: superuser attempting execute
+NOTICE:  in executor check perms: superuser finished execute
  t 
 ---
 (0 rows)

I can duplicate that by adding "force_parallel_mode = regress"
to test_oat_hooks.conf, so a fair bet is that the duplication
comes from executing the same hook in both leader and worker.

regards, tom lane

On 3/22/22 12:02, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

That seems quite weird. I'm not sure how it's getting loaded at all if
not via shared_preload_libraries

Some other animals are showing this:

diff -U3 /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out
--- /home/postgres/pgsql/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out	2022-03-22 11:57:40.224991011 -0400
+++ /home/postgres/pgsql/src/test/modules/test_oat_hooks/results/test_oat_hooks.out	2022-03-22 11:59:59.998983366 -0400
@@ -48,6 +48,8 @@
SELECT * FROM regress_test_table;
NOTICE:  in executor check perms: superuser attempting execute
NOTICE:  in executor check perms: superuser finished execute
+NOTICE:  in executor check perms: superuser attempting execute
+NOTICE:  in executor check perms: superuser finished execute
t 
---
(0 rows)
@@ -95,6 +97,8 @@
^
NOTICE:  in executor check perms: non-superuser attempting execute
NOTICE:  in executor check perms: non-superuser finished execute
+NOTICE:  in executor check perms: non-superuser attempting execute
+NOTICE:  in executor check perms: non-superuser finished execute
t 
---
(0 rows)
@@ -168,6 +172,8 @@
^
NOTICE:  in executor check perms: superuser attempting execute
NOTICE:  in executor check perms: superuser finished execute
+NOTICE:  in executor check perms: superuser attempting execute
+NOTICE:  in executor check perms: superuser finished execute
t 
---
(0 rows)

I can duplicate that by adding "force_parallel_mode = regress"
to test_oat_hooks.conf, so a fair bet is that the duplication
comes from executing the same hook in both leader and worker.

OK, thanks. My test didn't include that one setting :-(

If I can't com up with a very quick fix I'll revert it.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mar 22, 2022, at 9:09 AM, Andrew Dunstan <andrew@dunslane.net> wrote:

If I can't com up with a very quick fix I'll revert it.

The problem is coming from the REGRESS_exec_check_perms, which was included in the patch to demonstrate when the other hooks fired relative to the ExecutorCheckPerms_hook, but since it is causing problems, I can submit a patch with that removed. Give me a couple minutes....

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company