Horton

Thread

Commits

  1. Update some tests in 013_crash_restart.pl.

  2. Avoid freeing objects during json aggregate finalization

  3. SQL/JSON constructors

  4. Force NO_LOCALE / UTF8 for test_oat_hooks tests

  5. Temporarily disable installcheck for test_oat_hooks module

  6. Fix new Object Access hooks test

  7. Add String object access hooks

  8. Add a test module for Object Access hooks

  1. New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-18T03:21:56Z

    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.

(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.)

  2. Re: New Object Access Type hooks

    Joshua Brindle <joshua.brindle@crunchydata.com> — 2022-03-18T14:16:02Z

    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.

> (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.)
>
>
>
> [1] https://www.postgresql.org/message-id/flat/664799.1647456444%40sss.pgh.pa.us#c9721c2da88d59684ac7ac5fc36f09c1

>

  3. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-18T15:15:47Z

    
> 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

  4. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-18T22:04:11Z

    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

  5. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-21T15:41:04Z

    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

  6. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-21T19:57:34Z

    
> 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.

  7. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-21T19:58:32Z

    
> 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

  8. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-21T20:30:57Z

    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

  9. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-21T23:08:48Z

    
> 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.

  10. Re: New Object Access Type hooks

    Thomas Munro <thomas.munro@gmail.com> — 2022-03-22T05:03:31Z

    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...

  11. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T13:15:15Z

    
> 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

  12. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T13:25:18Z

    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

  13. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T14:41:05Z

    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

  14. Re: New Object Access Type hooks

    Julien Rouhaud <rjuju123@gmail.com> — 2022-03-22T15:14:44Z

    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&dt=2022-03-22%2014%3A40%3A10&stg=misc-check.

  15. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T15:26:45Z

    
> 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&dt=2022-03-22%2014%3A40%3A10&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

  16. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T15:48:45Z

    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&dt=2022-03-22%2014%3A40%3A10&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

  17. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T16:02:18Z

    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

  18. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T16:09:20Z

    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

  19. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T16:11:09Z

    
> 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

  20. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T16:15:39Z

    
> On Mar 22, 2022, at 9:11 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> Give me a couple minutes....

  21. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T16:26:02Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> 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....

Maybe better to suppress the audit messages if in a parallel worker?

			regards, tom lane

  22. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T16:33:49Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/22/22 11:26, Mark Dilger wrote:
>> culicidae is complaining:
>> 2022-03-22 14:53:27.198 UTC [2167008][not initialized][:0] FATAL:  test_oat_hooks must be loaded via shared_preload_libraries

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

After checking culicidae's config, I've duplicated this failure
by building with EXEC_BACKEND defined.  So I'd opine that there
is something broken about the method test_oat_hooks uses to
decide if it was loaded via shared_preload_libraries or not.
(Note that the failures appear to be coming out of auxiliary
processes such as the checkpointer.)

As a quick-n-dirty fix to avoid reverting the entire test module,
perhaps just delete this error check for now.

			regards, tom lane

  23. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T16:48:24Z

    
> On Mar 22, 2022, at 9:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 3/22/22 11:26, Mark Dilger wrote:
>>> culicidae is complaining:
>>> 2022-03-22 14:53:27.198 UTC [2167008][not initialized][:0] FATAL:  test_oat_hooks must be loaded via shared_preload_libraries
> 
>> That seems quite weird. I'm not sure how it's getting loaded at all if
>> not via shared_preload_libraries
> 
> After checking culicidae's config, I've duplicated this failure
> by building with EXEC_BACKEND defined.  So I'd opine that there
> is something broken about the method test_oat_hooks uses to
> decide if it was loaded via shared_preload_libraries or not.
> (Note that the failures appear to be coming out of auxiliary
> processes such as the checkpointer.)
> 
> As a quick-n-dirty fix to avoid reverting the entire test module,
> perhaps just delete this error check for now.

Ok, done as you suggest:

  24. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T16:58:19Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Mar 22, 2022, at 9:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As a quick-n-dirty fix to avoid reverting the entire test module,
>> perhaps just delete this error check for now.

> Ok, done as you suggest:

I only suggested removing the error check in _PG_init, not
changing the way the test works.

			regards, tom lane

  25. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T17:04:15Z

    On 3/22/22 12:58, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Mar 22, 2022, at 9:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> As a quick-n-dirty fix to avoid reverting the entire test module,
>>> perhaps just delete this error check for now.
>> Ok, done as you suggest:
> I only suggested removing the error check in _PG_init, not
> changing the way the test works.
>
> 			



Mark and I discussed this offline, and decided there was no requirement
for the module to be preloaded. Do you have a different opinion?


cheers


andrew


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

  26. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T17:04:55Z

    
> On Mar 22, 2022, at 9:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
>> Ok, done as you suggest:
> 
> I only suggested removing the error check in _PG_init, not
> changing the way the test works.

I should have been more explicit and said, "done as y'all suggest".  The "To" line of that email was to you and Andrew.

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

  27. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T17:08:45Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/22/22 12:58, Tom Lane wrote:
>> I only suggested removing the error check in _PG_init, not
>> changing the way the test works.

> Mark and I discussed this offline, and decided there was no requirement
> for the module to be preloaded. Do you have a different opinion?

No, I was actually about to make the same point: it seems to me there
are arguable use-cases for loading it shared, loading it per-session
(perhaps via ALTER USER SET or ALTER DATABASE SET to target particular
users/DBs), or even manually LOADing it.  So the module code should
not be prejudging how it's used.

On reflection, I withdraw my complaint about changing the way the
test script loads the module.  Getting rid of the need for a custom
.conf file simplifies the test module, and that seems good.
So I'm on board with Mark's patch now.

			regards, tom lane

  28. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T17:47:27Z

    On 3/22/22 13:08, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 3/22/22 12:58, Tom Lane wrote:
>>> I only suggested removing the error check in _PG_init, not
>>> changing the way the test works.
>> Mark and I discussed this offline, and decided there was no requirement
>> for the module to be preloaded. Do you have a different opinion?
> No, I was actually about to make the same point: it seems to me there
> are arguable use-cases for loading it shared, loading it per-session
> (perhaps via ALTER USER SET or ALTER DATABASE SET to target particular
> users/DBs), or even manually LOADing it.  So the module code should
> not be prejudging how it's used.
>
> On reflection, I withdraw my complaint about changing the way the
> test script loads the module.  Getting rid of the need for a custom
> .conf file simplifies the test module, and that seems good.
> So I'm on board with Mark's patch now.
>
> 			



OK, I have pushed that.


cheers


andrew


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

  29. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T18:01:59Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> OK, I have pushed that.

It seems like you could remove the NO_INSTALLCHECK restriction
too.  You already removed the comment defending it, and it
seems to work fine as an installcheck now if I remove that
locally.

Other nitpicks:

* the IsParallelWorker test could use a comment

* I notice a typo "permisisons" in test_oat_hooks.sql

			regards, tom lane

  30. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T20:34:52Z

    On 3/22/22 14:01, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> OK, I have pushed that.
> It seems like you could remove the NO_INSTALLCHECK restriction
> too.  You already removed the comment defending it, and it
> seems to work fine as an installcheck now if I remove that
> locally.
>
> Other nitpicks:
>
> * the IsParallelWorker test could use a comment
>
> * I notice a typo "permisisons" in test_oat_hooks.sql
>
> 			



Fixed


cheers


andrew


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

  31. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T22:18:45Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> Fixed

Now that we got past the hard failures, we can see that the test
falls over with (some?) non-default encodings, as for instance
here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-22%2020%3A23%3A13

I can replicate that by running the test under LANG=en_US.iso885915.
What I think is happening is:

(1) Rather unwisely, the relevant InvokeNamespaceSearchHook calls
appear in recomputeNamespacePath.  That means that their timing
depends heavily on accidents of caching.

(2) If we decide that we need an encoding conversion to talk to
the client, there'll be a lookup for the conversion function
early during session startup.  That will cause the namespace
search path to get computed then, before the test module has been
loaded and certainly before the audit GUC has been turned on.

(3) At the point where the test expects some audit notices
to come out, nothing happens because the search path is
already validated.

I'm inclined to think that (1) is a seriously bad idea,
not only because of this instability, but because

(a) the namespace cache logic is unlikely to cause the search-path
cache to get invalidated when something happens that might cause an
OAT hook to wish to change its decision, and

(b) this placement means that the hook is invoked during cache loading
operations that are likely to be super-sensitive to any additional
catalog accesses a hook might wish to do.  (I await the results of the
CLOBBER_CACHE_ALWAYS animals with trepidation.)

Now, if our attitude to the OAT hooks is that we are going to
sprinkle some at random and whether they are useful is someone
else's problem, then maybe these are not interesting concerns.

			regards, tom lane

  32. Re: New Object Access Type hooks

    Andres Freund <andres@anarazel.de> — 2022-03-22T22:20:11Z

    Hi,

On 2022-03-22 13:47:27 -0400, Andrew Dunstan wrote:
> OK, I have pushed that.

Seems like it might actually be good to test that object access hooks work
well in a parallel worker. How about going the other way and explicitly setting
force_parallel_mode = disabled for parts of the test and to enabled for
others?

- Andres

  33. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-03-22T22:37:37Z

    
> On Mar 22, 2022, at 3:20 PM, Andres Freund <andres@anarazel.de> wrote:
> 
> Seems like it might actually be good to test that object access hooks work
> well in a parallel worker. How about going the other way and explicitly setting
> force_parallel_mode = disabled for parts of the test and to enabled for
> others?

Wouldn't we get differing numbers of NOTICE messages depending on how many parallel workers there are?  Or would you propose setting the number of workers to a small, fixed value?

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

  34. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T22:41:45Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Mar 22, 2022, at 3:20 PM, Andres Freund <andres@anarazel.de> wrote:
>> Seems like it might actually be good to test that object access hooks work
>> well in a parallel worker. How about going the other way and explicitly setting
>> force_parallel_mode = disabled for parts of the test and to enabled for
>> others?

> Wouldn't we get differing numbers of NOTICE messages depending on how
> many parallel workers there are?  Or would you propose setting the
> number of workers to a small, fixed value?

The value would have to be "1", else you are going to have issues
with notices from different workers being interleaved differently
from run to run.  You might have that anyway, due to interleaving
of leader and worker messages.

			regards, tom lane

  35. Re: New Object Access Type hooks

    Andres Freund <andres@anarazel.de> — 2022-03-22T23:08:51Z

    Hi,

On 2022-03-22 18:41:45 -0400, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
> >> On Mar 22, 2022, at 3:20 PM, Andres Freund <andres@anarazel.de> wrote:
> >> Seems like it might actually be good to test that object access hooks work
> >> well in a parallel worker. How about going the other way and explicitly setting
> >> force_parallel_mode = disabled for parts of the test and to enabled for
> >> others?
> 
> > Wouldn't we get differing numbers of NOTICE messages depending on how
> > many parallel workers there are?  Or would you propose setting the
> > number of workers to a small, fixed value?

Yes.


> The value would have to be "1", else you are going to have issues
> with notices from different workers being interleaved differently
> from run to run.

Yea. Possible one could work around those with some effort (using multiple
notification channels maybe), but there seems little to glean from multiple
workers that a single worker wouldn't show.


> You might have that anyway, due to interleaving of leader and worker
> messages.

That part could perhaps be addressed by setting parallel_leader_participation
= 0.

Greetings,

Andres Freund

  36. Re: New Object Access Type hooks

    Justin Pryzby <pryzby@telsasoft.com> — 2022-03-22T23:13:11Z

    If I'm not wrong, this is still causing issues at least on cfbot/windows, even
since f0206d99.

https://cirrus-ci.com/task/5266352712712192
https://cirrus-ci.com/task/5061218867085312
https://cirrus-ci.com/task/5663822005403648
https://cirrus-ci.com/task/5744257246953472

https://cirrus-ci.com/task/5744257246953472
[22:26:50.939] test test_oat_hooks               ... FAILED      173 ms

https://api.cirrus-ci.com/v1/artifact/task/5744257246953472/log/src/test/modules/test_oat_hooks/regression.diffs
diff -w -U3 c:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out c:/cirrus/src/test/modules/test_oat_hooks/results/test_oat_hooks.out
--- c:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out	2022-03-22 22:13:23.386895200 +0000
+++ c:/cirrus/src/test/modules/test_oat_hooks/results/test_oat_hooks.out	2022-03-22 22:26:51.104419600 +0000
@@ -15,12 +15,6 @@
 NOTICE:  in process utility: superuser finished CreateRoleStmt
 CREATE TABLE regress_test_table (t text);
 NOTICE:  in process utility: superuser attempting CreateStmt
-NOTICE:  in object access: superuser attempting namespace search (subId=0) [no report on violation, allowed]
-LINE 1: CREATE TABLE regress_test_table (t text);
-                     ^
-NOTICE:  in object access: superuser finished namespace search (subId=0) [no report on violation, allowed]
-LINE 1: CREATE TABLE regress_test_table (t text);
-                     ^
 NOTICE:  in object access: superuser attempting create (subId=0) [explicit]

  37. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-22T23:36:50Z

    Justin Pryzby <pryzby@telsasoft.com> writes:
> If I'm not wrong, this is still causing issues at least on cfbot/windows, even
> since f0206d99.

That's probably a variant of the encoding dependency I described
upthread.

			regards, tom lane

  38. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-22T23:51:20Z

    On 3/22/22 18:18, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> Fixed
> Now that we got past the hard failures, we can see that the test
> falls over with (some?) non-default encodings, as for instance
> here:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-22%2020%3A23%3A13
>
> I can replicate that by running the test under LANG=en_US.iso885915.
> What I think is happening is:
>
> (1) Rather unwisely, the relevant InvokeNamespaceSearchHook calls
> appear in recomputeNamespacePath.  That means that their timing
> depends heavily on accidents of caching.
>
> (2) If we decide that we need an encoding conversion to talk to
> the client, there'll be a lookup for the conversion function
> early during session startup.  That will cause the namespace
> search path to get computed then, before the test module has been
> loaded and certainly before the audit GUC has been turned on.
>
> (3) At the point where the test expects some audit notices
> to come out, nothing happens because the search path is
> already validated.
>
> I'm inclined to think that (1) is a seriously bad idea,
> not only because of this instability, but because
>
> (a) the namespace cache logic is unlikely to cause the search-path
> cache to get invalidated when something happens that might cause an
> OAT hook to wish to change its decision, and
>
> (b) this placement means that the hook is invoked during cache loading
> operations that are likely to be super-sensitive to any additional
> catalog accesses a hook might wish to do.  (I await the results of the
> CLOBBER_CACHE_ALWAYS animals with trepidation.)
>
> Now, if our attitude to the OAT hooks is that we are going to
> sprinkle some at random and whether they are useful is someone
> else's problem, then maybe these are not interesting concerns.


So this was a pre-existing problem that the test has exposed? I don't
think we can just say "you deal with it", and if I understand you right
you don't think that either.

I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
temporarily.


cheers


andrew

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

  39. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-23T00:07:02Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/22/22 18:18, Tom Lane wrote:
>> Now, if our attitude to the OAT hooks is that we are going to
>> sprinkle some at random and whether they are useful is someone
>> else's problem, then maybe these are not interesting concerns.

> So this was a pre-existing problem that the test has exposed? I don't
> think we can just say "you deal with it", and if I understand you right
> you don't think that either.

Yeah, my point exactly: the placement of those hooks needs to be rethought.
I'm guessing what we ought to do is let the cached namespace OID list
get built without interference, and then allow the OAT hook to filter
it when it's read.

> I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
> temporarily.

I was able to reproduce it under "make check" as long as I had
LANG set to one of the troublesome values, so I'm not real sure
that that'll be enough.

			regards, tom lane

  40. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-23T00:11:01Z

    On 3/22/22 20:07, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 3/22/22 18:18, Tom Lane wrote:
>>> Now, if our attitude to the OAT hooks is that we are going to
>>> sprinkle some at random and whether they are useful is someone
>>> else's problem, then maybe these are not interesting concerns.
>> So this was a pre-existing problem that the test has exposed? I don't
>> think we can just say "you deal with it", and if I understand you right
>> you don't think that either.
> Yeah, my point exactly: the placement of those hooks needs to be rethought.
> I'm guessing what we ought to do is let the cached namespace OID list
> get built without interference, and then allow the OAT hook to filter
> it when it's read.
>
>> I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
>> temporarily.
> I was able to reproduce it under "make check" as long as I had
> LANG set to one of the troublesome values, so I'm not real sure
> that that'll be enough.
>
> 			


The buildfarm only runs installcheck under different locales/encodings.


cheers


andrew

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

  41. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-23T15:19:26Z

    On 3/22/22 20:11, Andrew Dunstan wrote:
> On 3/22/22 20:07, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 3/22/22 18:18, Tom Lane wrote:
>>>> Now, if our attitude to the OAT hooks is that we are going to
>>>> sprinkle some at random and whether they are useful is someone
>>>> else's problem, then maybe these are not interesting concerns.
>>> So this was a pre-existing problem that the test has exposed? I don't
>>> think we can just say "you deal with it", and if I understand you right
>>> you don't think that either.
>> Yeah, my point exactly: the placement of those hooks needs to be rethought.
>> I'm guessing what we ought to do is let the cached namespace OID list
>> get built without interference, and then allow the OAT hook to filter
>> it when it's read.
>>
>>> I could make the buildfarm quiet again by resetting NO_INSTALLCHECK
>>> temporarily.
>> I was able to reproduce it under "make check" as long as I had
>> LANG set to one of the troublesome values, so I'm not real sure
>> that that'll be enough.
>>
>> 			
>
> The buildfarm only runs installcheck under different locales/encodings.


But you're right about the non-installcheck cases. fairywren had that
issue. I have committed a (tested) fix for those too to force
NO_LOCALE/UTF8.


cheers


andrew


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

  42. Re: New Object Access Type hooks

    Andres Freund <andres@anarazel.de> — 2022-03-24T20:59:58Z

    Hi,

I just rebased the meson tree (atop 75b1521dae1) and the test_oat_hooks test
fail on windows with meson. They don't with our "homegrown" buildsystem, but
just because it won't run them.

https://cirrus-ci.com/build/6101947223638016
https://cirrus-ci.com/task/5869668815601664?logs=check_world#L67
https://api.cirrus-ci.com/v1/artifact/task/5869668815601664/log/build/testrun/test_oat_hooks/pg_regress/regression.diffs

diff -w -U3 C:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out C:/cirrus/build/testrun/test_oat_hooks/pg_regress/results/test_oat_hooks.out
--- C:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out	2022-03-24 18:56:39.592048000 +0000
+++ C:/cirrus/build/testrun/test_oat_hooks/pg_regress/results/test_oat_hooks.out	2022-03-24 19:03:33.910466700 +0000
@@ -15,12 +15,6 @@
 NOTICE:  in process utility: superuser finished CreateRoleStmt
 CREATE TABLE regress_test_table (t text);
 NOTICE:  in process utility: superuser attempting CreateStmt
-NOTICE:  in object access: superuser attempting namespace search (subId=0) [no report on violation, allowed]
-LINE 1: CREATE TABLE regress_test_table (t text);
-                     ^
-NOTICE:  in object access: superuser finished namespace search (subId=0) [no report on violation, allowed]
-LINE 1: CREATE TABLE regress_test_table (t text);
-                     ^
 NOTICE:  in object access: superuser attempting create (subId=0) [explicit]
 NOTICE:  in object access: superuser finished create (subId=0) [explicit]
 NOTICE:  in object access: superuser attempting create (subId=0) [explicit]


I don't think this is meson specific...

Greetings,

Andres Freund

  43. Re: New Object Access Type hooks

    Andrew Dunstan <andrew@dunslane.net> — 2022-03-24T21:31:59Z

    On 3/24/22 16:59, Andres Freund wrote:
> Hi,
>
> I just rebased the meson tree (atop 75b1521dae1) and the test_oat_hooks test
> fail on windows with meson. They don't with our "homegrown" buildsystem, but
> just because it won't run them.
>
> https://cirrus-ci.com/build/6101947223638016
> https://cirrus-ci.com/task/5869668815601664?logs=check_world#L67
> https://api.cirrus-ci.com/v1/artifact/task/5869668815601664/log/build/testrun/test_oat_hooks/pg_regress/regression.diffs
>
> diff -w -U3 C:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out C:/cirrus/build/testrun/test_oat_hooks/pg_regress/results/test_oat_hooks.out
> --- C:/cirrus/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out	2022-03-24 18:56:39.592048000 +0000
> +++ C:/cirrus/build/testrun/test_oat_hooks/pg_regress/results/test_oat_hooks.out	2022-03-24 19:03:33.910466700 +0000
> @@ -15,12 +15,6 @@
>  NOTICE:  in process utility: superuser finished CreateRoleStmt
>  CREATE TABLE regress_test_table (t text);
>  NOTICE:  in process utility: superuser attempting CreateStmt
> -NOTICE:  in object access: superuser attempting namespace search (subId=0) [no report on violation, allowed]
> -LINE 1: CREATE TABLE regress_test_table (t text);
> -                     ^
> -NOTICE:  in object access: superuser finished namespace search (subId=0) [no report on violation, allowed]
> -LINE 1: CREATE TABLE regress_test_table (t text);
> -                     ^
>  NOTICE:  in object access: superuser attempting create (subId=0) [explicit]
>  NOTICE:  in object access: superuser finished create (subId=0) [explicit]
>  NOTICE:  in object access: superuser attempting create (subId=0) [explicit]
>
>
> I don't think this is meson specific...


Even if you use NO_LOCALE=1/ENCODING=UTF8 as the Makefile now does?


cheers


andrew

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

  44. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-24T21:44:31Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 3/24/22 16:59, Andres Freund wrote:
>> I just rebased the meson tree (atop 75b1521dae1) and the test_oat_hooks test
>> fail on windows with meson.

> Even if you use NO_LOCALE=1/ENCODING=UTF8 as the Makefile now does?

Note that that's basically a workaround for buggy placement of the
OAT hooks, as per previous discussion.  I hope that we fix that bug
pretty soon, so it shouldn't really be a factor for the meson conversion.

			regards, tom lane

  45. Re: New Object Access Type hooks

    Andres Freund <andres@anarazel.de> — 2022-03-24T21:58:22Z

    Hi,

On 2022-03-24 17:44:31 -0400, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 3/24/22 16:59, Andres Freund wrote:
> >> I just rebased the meson tree (atop 75b1521dae1) and the test_oat_hooks test
> >> fail on windows with meson.
> 
> > Even if you use NO_LOCALE=1/ENCODING=UTF8 as the Makefile now does?

I didn't do that, no. I guess I should have re-checked that in the makefile /
reread the commit message in more detail - although it really doesn't provide
a lot of that. That's, uh, some shoveling problems under the carpet.

I'll do that for now then.


> Note that that's basically a workaround for buggy placement of the
> OAT hooks, as per previous discussion.  I hope that we fix that bug
> pretty soon, so it shouldn't really be a factor for the meson conversion.

Yea. I found it's a lot easier to rebase the meson tree frequently rather than
do it in larger batches, so I just do so every few days...

Greetings,

Andres Freund

  46. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T16:16:39Z

    
> 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?  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...

I just got a crash in this test again.  Are you still interested?  I still have the logs.  No core file appears to have been generated.

The test failure is

not ok 5 - psql query died successfully after SIGQUIT

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

  47. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T17:03:10Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> I just got a crash in this test again.  Are you still interested?  I still have the logs.  No core file appears to have been generated.
> The test failure is
> not ok 5 - psql query died successfully after SIGQUIT

Hmm ... I can see one problem with that test:

ok( pump_until(
		$killme,
		$psql_timeout,
		\$killme_stderr,
		qr/WARNING:  terminating connection because of crash of another server process|server closed the connection unexpectedly|connection to server was lost/m
	),
	"psql query died successfully after SIGQUIT");

It's been a little while since that message looked like that.
Nowadays you get

WARNING:  terminating connection because of unexpected SIGQUIT signal
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

Usually the test would succeed anyway because of matching the
second or third regex alternative, but I wonder if there is
some other spelling of libpq's complaint that shows up
occasionally.  It'd be nice if we could see the contents of
$killme_stderr upon failure.

			regards, tom lane

  48. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T17:41:17Z

    I wrote:
> Usually the test would succeed anyway because of matching the
> second or third regex alternative, but I wonder if there is
> some other spelling of libpq's complaint that shows up
> occasionally.  It'd be nice if we could see the contents of
> $killme_stderr upon failure.

OK, now I'm confused, because pump_until is very clearly
*trying* to report exactly that:

        if (not $proc->pumpable())
        {
            diag("pump_until: process terminated unexpectedly when searching for \"$until\" with stream: \"$$stream\"");
            return 0;
        }

and if I intentionally break the regex then I do see this
output when running the test by hand:

# Running: pg_ctl kill QUIT 1922645
ok 4 - killed process with SIGQUIT
# pump_until: process terminated unexpectedly when searching for "(?^m:WARNING:  terminating connection because of crash of another server process|server closed the connection foounexpectedly|connection to server was lost)" with stream: "psql:<stdin>:9: WARNING:  terminating connection because of unexpected SIGQUIT signal
# psql:<stdin>:9: server closed the connection unexpectedly
#       This probably means the server terminated abnormally
#       before or while processing the request.
# "
not ok 5 - psql query died successfully after SIGQUIT

Is our CI setup failing to capture stderr from TAP tests??

			regards, tom lane

  49. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T17:44:25Z

    
> On Apr 4, 2022, at 10:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Is our CI setup failing to capture stderr from TAP tests??

I'm looking into the way our TAP test infrastructure assigns port numbers to nodes, and whether that is reasonable during parallel test runs with nodes stopping and starting again.  On casual inspection, that doesn't seem ok, because the Cluster.pm logic to make sure nodes get unique ports doesn't seem to be thinking about other parallel tests running.  It will notice if another node is already bound to the port, but if another node has been killed and not yet restarted, won't things go off the rails?

I'm writing a parallel test just for this.  Will get back to you.

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

  50. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T17:51:13Z

    I wrote:
> Is our CI setup failing to capture stderr from TAP tests??

Oh, I'm barking up the wrong tree.  This test must have been run
against HEAD between 6da65a3f9 (23 Feb) and 2beb4acff (31 Mar), when
pump_until indeed didn't print this essential information :-(

If you just got this failure, could you look in the log to
see if there's a pump_until report?

			regards, tom lane

  51. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T17:56:11Z

    
> On Apr 4, 2022, at 10:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Oh, I'm barking up the wrong tree.  This test must have been run
> against HEAD between 6da65a3f9 (23 Feb) and 2beb4acff (31 Mar), when
> pump_until indeed didn't print this essential information :-(
> 
> If you just got this failure, could you look in the log to
> see if there's a pump_until report?

I was running `make -j12 check-world` against my local patched version of master:

commit 80399fa5f208c4acd4ec194c47e534ba8dd3ae7c (HEAD -> 0001)
Author: Mark Dilger <mark.dilger@enterprisedb.com>
Date:   Mon Mar 28 13:35:11 2022 -0700

    Allow grant and revoke of privileges on parameters
    
    Add new SET and ALTER SYSTEM privileges for configuration parameters
    (GUCs), and a new catalog, pg_parameter_acl, for tracking grants of
    these privileges.
    
    The privilege to SET a parameter marked USERSET is inherent in that
    parameter's marking and cannot be revoked.  This saves cycles when
    performing SET operations, as looking up privileges in the catalog
    can be skipped.  If we find that administrators need to revoke SET
    privilege on a particular variable from public, that variable can be
    redefined in future releases as SUSET with a default grant of SET to
    PUBLIC issued.

commit 4eb9798879680dcc0e3ebb301cf6f925dfa69422 (origin/master, origin/HEAD, master)
Author: Andrew Dunstan <andrew@dunslane.net>
Date:   Mon Apr 4 10:12:30 2022 -0400

    Avoid freeing objects during json aggregate finalization
    
    Commit f4fb45d15c tried to free memory during aggregate finalization.
    This cause issues, particularly when used as a window function, so stop
    doing that.
    
    Per complaint by Jaime Casanova and diagnosis by Andres Freund
    
    Discussion: https://postgr.es/m/YkfeMNYRCGhySKyg@ahch-to


The test logs are attached.

  52. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T18:07:06Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> The test logs are attached.

Server log looks as-expected:

2022-04-04 09:11:51.087 PDT [2084] 013_crash_restart.pl LOG:  statement: SELECT pg_sleep(3600);
2022-04-04 09:11:51.094 PDT [2083] 013_crash_restart.pl WARNING:  terminating connection because of unexpected SIGQUIT signal
2022-04-04 09:11:51.095 PDT [2070] LOG:  server process (PID 2083) exited with exit code 2
2022-04-04 09:11:51.095 PDT [2070] DETAIL:  Failed process was running: INSERT INTO alive VALUES($$in-progress-before-sigquit$$) RETURNING status;
2022-04-04 09:11:51.095 PDT [2070] LOG:  terminating any other active server processes

I was hoping to see regress_log_013_crash_restart, though.

			regards, tom lane

  53. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T18:09:45Z

    
> On Apr 4, 2022, at 11:07 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I was hoping to see regress_log_013_crash_restart, though.

  54. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T18:32:40Z

    
> On Apr 4, 2022, at 10:44 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> I'm writing a parallel test just for this.  Will get back to you.

Ok, that experiment didn't accomplish anything, beyond refreshing my memory regarding Cluster.pm preferring sockets over ports.

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

  55. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T19:01:15Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:

> # Running: pg_ctl kill QUIT 2083
> ok 4 - killed process with SIGQUIT
> # pump_until: process terminated unexpectedly when searching for "(?^m:WARNING:  terminating connection because of crash of another server process|server closed the connection unexpectedly|connection to server was lost)" with stream: "psql:<stdin>:9: WARNING:  terminating connection because of unexpected SIGQUIT signal
> # psql:<stdin>:9: could not send data to server: Socket is not connected
> # "
> not ok 5 - psql query died successfully after SIGQUIT

And there we have it: the test wasn't updated for the new backend message
spelling, and we're seeing a different frontend behavior.  Evidently the
backend is dying before we're able to send the "SELECT 1;" to it.

I'm not quite sure whether it's a libpq bug that it doesn't produce the
"connection to server was lost" message here, but in any case I suspect
that we shouldn't be checking for the second and third regex alternatives.
The "terminating connection" warning absolutely should get through, and
if it doesn't we want to know about it.  So my proposal for a fix is
to change the regex to be just "WARNING:  terminating connection because
of unexpected SIGQUIT signal".

			regards, tom lane

  56. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T19:05:10Z

    I wrote:
> The "terminating connection" warning absolutely should get through,

... oh, no, that's not guaranteed at all, since it's sent from quickdie().
So scratch that.  Maybe we'd better add "could not send data to server"
to the regex?

			regards, tom lane

  57. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T20:28:17Z

    
> On Apr 4, 2022, at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I wrote:
>> The "terminating connection" warning absolutely should get through,
> 
> ... oh, no, that's not guaranteed at all, since it's sent from quickdie().
> So scratch that.  Maybe we'd better add "could not send data to server"
> to the regex?

If it fails in pqsecure_raw_write(), you get either "server closed the connection unexpectedly" or "could not send data to server".  Do we need to support pgtls_write() or pg_GSS_write(), which have different error messages?  Can anybody run the tests with TLS or GSS enabled?  I assume the test framework prevents this, but I didn't check too closely....

Is it possible that pgFlush will call pqSendSome which calls pqReadData before trying to write anything, and get back a "could not receive data from server" from pqsecure_raw_read()?

It's a bit hard to prove to myself which paths might be followed through this code.  Thoughts?

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

  58. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-04T20:47:04Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Apr 4, 2022, at 12:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So scratch that.  Maybe we'd better add "could not send data to server"
>> to the regex?

> If it fails in pqsecure_raw_write(), you get either "server closed the connection unexpectedly" or "could not send data to server".  Do we need to support pgtls_write() or pg_GSS_write(), which have different error messages?

Don't see why, since this test sets up a new cluster in which neither
is enabled.

> Is it possible that pgFlush will call pqSendSome which calls pqReadData before trying to write anything, and get back a "could not receive data from server" from pqsecure_raw_read()?

Yeah, it's plausible to get a failure on either the write or read side
depending on timing.

Perhaps libpq should be trying harder to make those cases look alike, but
this test is about server behavior not libpq behavior, so I'm inclined
to just make it lax.

			regards, tom lane

  59. Re: New Object Access Type hooks

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-04T20:50:40Z

    
> On Apr 4, 2022, at 1:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Yeah, it's plausible to get a failure on either the write or read side
> depending on timing.
> 
> Perhaps libpq should be trying harder to make those cases look alike, but
> this test is about server behavior not libpq behavior, so I'm inclined
> to just make it lax.

+1.

I've gotten this test failure only a few times in perhaps the last six months, so if we narrow the opportunity for test failure without closing it entirely, we're just making the test failures that much harder to diagnose.

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

  60. Re: New Object Access Type hooks

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-05T02:10:50Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Apr 4, 2022, at 1:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps libpq should be trying harder to make those cases look alike, but
>> this test is about server behavior not libpq behavior, so I'm inclined
>> to just make it lax.

> +1.

> I've gotten this test failure only a few times in perhaps the last six months, so if we narrow the opportunity for test failure without closing it entirely, we're just making the test failures that much harder to diagnose.

Done that way.

			regards, tom lane

  61. Re: New Object Access Type hooks

    Michael Paquier <michael@paquier.xyz> — 2022-04-18T06:50:11Z

    On Thu, Mar 24, 2022 at 05:44:31PM -0400, Tom Lane wrote:
> Note that that's basically a workaround for buggy placement of the
> OAT hooks, as per previous discussion.  I hope that we fix that bug
> pretty soon, so it shouldn't really be a factor for the meson conversion.

So, this issue is still listed as an open item.  What should we do?
From what I get, the caching issues with the namespace lookup hook are
not new to v15, they just get exposed by the new test module
test_oat_hooks/.  FWIW, I would vote against moving around hook calls
in back branches as that could cause compatibility problems in
existing code relying on them, but it surely is unstable to keep these
when recomputing the search_path.

A removal from recomputeNamespacePath() implies an addition at the end
of fetch_search_path() and fetch_search_path_array().  Perhaps an
extra one in RangeVarGetCreationNamespace()?  The question is how much
of these we want, for example the search hook would be called now even
when doing relation-specific checks like RelationIsVisible() and the
kind.
--
Michael

  62. Re: New Object Access Type hooks

    Michael Paquier <michael@paquier.xyz> — 2022-06-22T07:48:15Z

    On Mon, Apr 18, 2022 at 03:50:11PM +0900, Michael Paquier wrote:
> A removal from recomputeNamespacePath() implies an addition at the end
> of fetch_search_path() and fetch_search_path_array().  Perhaps an
> extra one in RangeVarGetCreationNamespace()?  The question is how much
> of these we want, for example the search hook would be called now even
> when doing relation-specific checks like RelationIsVisible() and the
> kind.

I have been playing with this issue, and if we want to minimize the
number of times the list of namespaces in activeSearchPath gets
checked through the search hook, it looks like this is going to
require an additional cached list of namespace OIDs filtered through
InvokeNamespaceSearchHook().  However, it is unclear to me how we can
guarantee that any of the code paths forcing a recomputation of
activeSearchPath are not used for a caching phase, so it looks rather
easy to mess up things and finish with a code path using an unfiltered
activeSearchPath.  The set of *IsVisible() routines should be fine, at
least.

At the end, I am not sure that it is a wise time to redesign this
area close to beta2, so I would vote for leaving this issue aside for
now.  Another thing that we could do is to tweak the tests and silence
the part around OAT_NAMESPACE_SEARCH, which would increase the coverage
with installcheck, removing the need for ENCODING and NO_LOCALE.
--
Michael