Обсуждение: Fwd: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS

Moving the report from security to -hackers on Noah's advice. Since
the function(s) involved in the crash are not present in any of the
released versions, it is not considered a security issue.

I can confirm that this is reproducible on the latest commit on
master, 3c0bcdbc66. Below is the original analysis, followed by Noah's
analysis.

To be able to reproduce it, please note that perl support is required;
 hence `./configure --with-perl`.

The note about 'security concerns around on_plperl_init parameter',
below, refers to now-fixed issue, at commit 13d8388151.

Best regards,
Gurjeet
http://Gurje.et

Forwarded Conversation
Subject: Unprivileged user can induce crash by using an SUSET param in PGOPTIONS
------------------------

From: Gurjeet Singh <gurjeet@singh.im>
Date: Mon, Jul 4, 2022 at 10:24 AM
To: Postgres Security <security@postgresql.org>
Cc: Bossart, Nathan <bossartn@amazon.com>


While poking at plperl's GUC in an internal discussion, I was able to
induce a crash (or an assertion failure in assert-enabled builds) as
an unprivileged user.

My investigation so far has revealed that the code path for the
following condition has never been tested, and because of this, when a
user tries to override an SUSET param via PGOPTIONS, Postgres tries to
perform a table lookup during process initialization. Because there's
no transaction in progress, and because this table is not in the
primed caches, we end up with code trying to dereference an
uninitialized  CurrentResourceOwner.

The condition:
User specifies PGOPTIONS"-c custom.param"
"custom.param" is used by an extension which is specified in
session_preload_libraries
The extension uses DefineCustom*Variable("custom.param", PGC_SUSET)
inside set_config_option()
  record->context == PGC_SUSET
  context == PGC_BACKEND
  calls pg_parameter_aclcheck()  -> eventually leads to
assertion-failure (or crash, when assertions are disabled)

See below for 1. How to reproduce, 2. Assertion failure stack, and 3.
Crash stack

When the user does not specify PGOPTIONS, the code in
define_custom_variable() returns prematurely, after a failed bsearch()
lookup, and hence avoids this bug.

I think similar crash can be  induced when the custom parameter is of
kind PGC_SU_BACKEND, because the code to handle that also invokes
pg_parameter_aclcheck(). Also, I believe the same condition would
arise if the extension is specified local_preload_libraries.

I haven't  been  able to think of an attack vector using this bug, but
it can be used to cause a denial-of-service by an authenticated user.
I'm sending this report to security list, instead of -hackers, out of
abundance of caution; please feel free to move it to -hackers, if it's
not considered a security concern.

I discovered this bug a couple of days ago, just before leaving on a
trip. But because of shortage of time over the weekend, I haven't been
able to dig deeper into it. Since I don't think I'll be able to spend
any significant time on it for at least another couple of days, I'm
sending this report without a patch or a proposed fix.

CC: Nathan, whose security concerns around on_plperl_init parameter
lead to this discovery.

[1]: How to reproduce

$ psql -c 'create user test'
CREATE ROLE

$ psql -c "alter system set session_preload_libraries='plperl'"
ALTER SYSTEM

$ # restart server

$ psql -c 'show session_preload_libraries'
 session_preload_libraries
---------------------------
 plperl
(1 row)

$ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
psql: error: connection to server on socket "/tmp/.s.psql.5432"
failed: server closed the connection unexpectedly
    ┆   This probably means the server terminated abnormally
        before or while processing the request.


[2]:  Assertion failure stack

LOG:  database system is ready to accept connections
TRAP: FailedAssertion("IsTransactionState()", File:
"../../../../../../POSTGRES/src/backend/utils/cache/catcache.c", Line:
1209, P
ID: 199868)
postgres: test postgres [local]
startup(ExceptionalCondition+0xd0)[0x55e503a4e6c9]
postgres: test postgres [local] startup(+0x7e069b)[0x55e503a2a69b]
postgres: test postgres [local] startup(SearchCatCache1+0x3a)[0x55e503a2a56b]
postgres: test postgres [local] startup(SearchSysCache1+0xc1)[0x55e503a46fe4]
postgres: test postgres [local]
startup(pg_parameter_aclmask+0x6f)[0x55e50345f098]
postgres: test postgres [local]
startup(pg_parameter_aclcheck+0x2d)[0x55e50346039c]
postgres: test postgres [local] startup(set_config_option+0x450)[0x55e503a70727]
postgres: test postgres [local] startup(+0x829ce8)[0x55e503a73ce8]
postgres: test postgres [local]
startup(DefineCustomStringVariable+0xa4)[0x55e503a74306]
/home/vagrant/dev/POSTGRES_builds/add_tz_param/db/lib/postgresql/plperl.so(_PG_init+0xd7)[0x7fed3d845425]
postgres: test postgres [local] startup(+0x80cc50)[0x55e503a56c50]
postgres: test postgres [local] startup(load_file+0x43)[0x55e503a566d9]
postgres: test postgres [local] startup(+0x81ba89)[0x55e503a65a89]
postgres: test postgres [local]
startup(process_session_preload_libraries+0x23)[0x55e503a65bc6]
postgres: test postgres [local] startup(PostgresMain+0x23b)[0x55e50388a52a]
postgres: test postgres [local] startup(+0x564c5d)[0x55e5037aec5d]
postgres: test postgres [local] startup(+0x564542)[0x55e5037ae542]
postgres: test postgres [local] startup(+0x560777)[0x55e5037aa777]
postgres: test postgres [local] startup(PostmasterMain+0x1374)[0x55e5037a9f10]
postgres: test postgres [local] startup(+0x451550)[0x55e50369b550]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7fed46dac083]
postgres: test postgres [local] startup(_start+0x2e)[0x55e503317eae]
LOG:  server process (PID 199868) was terminated by signal 6: Aborted
LOG:  terminating any other active server processes

[3]: Crash stack

(gdb) bt
#0  0x0000560937b35206 in ResourceArrayEnlarge (resarr=0x80)
    at ../../../../../../POSTGRES/src/backend/utils/resowner/resowner.c:222
#1  0x0000560937b36693 in ResourceOwnerEnlargeRelationRefs (owner=0x0)
    at ../../../../../../POSTGRES/src/backend/utils/resowner/resowner.c:1106
#2  0x0000560937ae32ca in RelationIncrementReferenceCount (rel=0x7fb697a0b860)
    at ../../../../../../POSTGRES/src/backend/utils/cache/relcache.c:2128
#3  0x0000560937ae322b in RelationIdGetRelation (relationId=6243)
    at ../../../../../../POSTGRES/src/backend/utils/cache/relcache.c:2074
#4  0x00005609374758a3 in relation_open (relationId=6243, lockmode=1)
    at ../../../../../../POSTGRES/src/backend/access/common/relation.c:59
#5  0x00005609375181ca in table_open (relationId=6243, lockmode=1)
    at ../../../../../../POSTGRES/src/backend/access/table/table.c:43
#6  0x0000560937ad41dc in SearchCatCacheMiss (cache=0x560938b60500,
nkeys=1, hashValue=658344123, hashIndex=3,
    v1=94597605943240, v2=0, v3=0, v4=0) at
../../../../../../POSTGRES/src/backend/utils/cache/catcache.c:1353
#7  0x0000560937ad40cc in SearchCatCacheInternal
(cache=0x560938b60500, nkeys=1, v1=94597605943240, v2=0, v3=0, v4=0)
    at ../../../../../../POSTGRES/src/backend/utils/cache/catcache.c:1295
#8  0x0000560937ad3de7 in SearchCatCache1 (cache=0x560938b60500,
v1=94597605943240)
    at ../../../../../../POSTGRES/src/backend/utils/cache/catcache.c:1163
#9  0x0000560937aedba7 in SearchSysCache1 (cacheId=41, key1=94597605943240)
    at ../../../../../../POSTGRES/src/backend/utils/cache/syscache.c:1180
#10 0x00005609375658b0 in pg_parameter_aclmask (name=0x560938b26670
"plperl.on_plperl_init", roleid=16384, mask=4096,
    how=ACLMASK_ANY) at
../../../../../POSTGRES/src/backend/catalog/aclchk.c:4234
#11 0x0000560937566b82 in pg_parameter_aclcheck (name=0x560938b26670
"plperl.on_plperl_init", roleid=16384, mode=4096)
    at ../../../../../POSTGRES/src/backend/catalog/aclchk.c:5048
#12 0x0000560937b14fbc in set_config_option (name=0x560938b26670
"plperl.on_plperl_init", value=0x560938ba13a0 "",
    context=PGC_BACKEND, source=PGC_S_CLIENT, action=GUC_ACTION_SET,
changeVal=true, elevel=19, is_reload=false)
    at ../../../../../../POSTGRES/src/backend/utils/misc/guc.c:7735
#13 0x0000560937b18408 in define_custom_variable (variable=0x560938b265c0)
    at ../../../../../../POSTGRES/src/backend/utils/misc/guc.c:9361
#14 0x0000560937b189fa in DefineCustomStringVariable
(name=0x7fb697963114 "plperl.on_plperl_init",
    short_desc=0x7fb6979630d0 "Perl initialization code to execute
once when plperl is first used.", long_desc=0x0,
    valueAddr=0x7fb697968730 <plperl_on_plperl_init>, bootValue=0x0,
context=PGC_SUSET, flags=0, check_hook=0x0,
    assign_hook=0x0, show_hook=0x0) at
../../../../../../POSTGRES/src/backend/utils/misc/guc.c:9589
#15 0x00007fb69795234d in _PG_init () at
../../../../../POSTGRES/src/pl/plperl/plperl.c:443
#16 0x0000560937afcc8c in internal_load_library (
--Type <RET> for more, q to quit, c to continue without paging--
    libname=0x560938b2e188
"/home/vagrant/dev/POSTGRES_builds/add_tz_param/db/lib/postgresql/plperl.so")
    at ../../../../../../POSTGRES/src/backend/utils/fmgr/dfmgr.c:289
#17 0x0000560937afc729 in load_file (filename=0x560938b2e748 "plperl",
restricted=false)
    at ../../../../../../POSTGRES/src/backend/utils/fmgr/dfmgr.c:156
#18 0x0000560937b0ab02 in load_libraries (libraries=0x560938b1b5c0
"plperl", gucname=0x560937cd3706 "session_preload_libraries",
    restricted=false) at
../../../../../../POSTGRES/src/backend/utils/init/miscinit.c:1668
#19 0x0000560937b0ac3f in process_session_preload_libraries ()
    at ../../../../../../POSTGRES/src/backend/utils/init/miscinit.c:1699
#20 0x0000560937945908 in PostgresMain (dbname=0x560938af9ca8
"postgres", username=0x560938b2b0d8 "test")
    at ../../../../../POSTGRES/src/backend/tcop/postgres.c:4170
#21 0x00005609378827ce in BackendRun (port=0x560938b240e0) at
../../../../../POSTGRES/src/backend/postmaster/postmaster.c:4504
#22 0x00005609378820b3 in BackendStartup (port=0x560938b240e0)
    at ../../../../../POSTGRES/src/backend/postmaster/postmaster.c:4232
#23 0x000056093787e5e3 in ServerLoop () at
../../../../../POSTGRES/src/backend/postmaster/postmaster.c:1806
#24 0x000056093787dd86 in PostmasterMain (argc=3, argv=0x560938af7be0)
    at ../../../../../POSTGRES/src/backend/postmaster/postmaster.c:1478
#25 0x000056093778078f in main (argc=3, argv=0x560938af7be0) at
../../../../../POSTGRES/src/backend/main/main.c:202
(gdb)

Best regards,
Gurjeet
http://Gurje.et


----------
From: Noah Misch <noah@leadboat.com>
Date: Tue, Jul 5, 2022 at 10:50 AM
To: Gurjeet Singh <gurjeet@singh.im>
Cc: Postgres Security <security@postgresql.org>, Bossart, Nathan
<bossartn@amazon.com>


On Mon, Jul 04, 2022 at 10:24:13AM -0700, Gurjeet Singh wrote:
>   calls pg_parameter_aclcheck()  -> eventually leads to
> assertion-failure (or crash, when assertions are disabled)

> I'm sending this report to security list, instead of -hackers, out of
> abundance of caution; please feel free to move it to -hackers, if it's
> not considered a security concern.

Thanks for the report.  v14 doesn't have pg_parameter_aclcheck().  If this is
specific to unreleased and beta versions, do use -hackers.

On Wed, Jul 20, 2022 at 07:31:47PM -0700, Gurjeet Singh wrote:
> Moving the report from security to -hackers on Noah's advice. Since
> the function(s) involved in the crash are not present in any of the
> released versions, it is not considered a security issue.
>
> I can confirm that this is reproducible on the latest commit on
> master, 3c0bcdbc66. Below is the original analysis, followed by Noah's
> analysis.
>
> To be able to reproduce it, please note that perl support is required;
>  hence `./configure --with-perl`.
>
> The note about 'security concerns around on_plperl_init parameter',
> below, refers to now-fixed issue, at commit 13d8388151.

This ACL lookup still happens when pre-loading libraries at session
startup with custom GUCs, as this checks if the GUC can be changed by
the user connecting or not.  I am adding an open item to track that.
--
Michael

Gurjeet Singh <gurjeet@singh.im> writes:
> While poking at plperl's GUC in an internal discussion, I was able to
> induce a crash (or an assertion failure in assert-enabled builds) as
> an unprivileged user.
> My investigation so far has revealed that the code path for the
> following condition has never been tested, and because of this, when a
> user tries to override an SUSET param via PGOPTIONS, Postgres tries to
> perform a table lookup during process initialization. Because there's
> no transaction in progress, and because this table is not in the
> primed caches, we end up with code trying to dereference an
> uninitialized  CurrentResourceOwner.

Right.  So there are basically two things we could do about this:

1. set_config_option could decline to call pg_parameter_aclcheck
if not IsTransactionState(), instead failing the assignment.
This isn't a great answer because it would result in disallowing
GUC assignments that users might expect to work.

2. We could move process_session_preload_libraries() to someplace
where a transaction is in progress -- probably, relocate it to
inside InitPostgres().

I'm inclined to think that #2 is a better long-term solution,
because it'd allow you to session-preload libraries that expect
to be able to do database access during _PG_init.  (Right now
that'd fail with the same sort of symptoms seen here.)  But
there's no denying that this might have surprising side-effects
for extensions that weren't expecting such a change.

It could also be reasonable to do both #1 and #2, with the idea
that #1 might save us from crashing if there are any other
code paths where we can reach that pg_parameter_aclcheck call
outside a transaction.

Thoughts?

            regards, tom lane

On Thu, Jul 21, 2022 at 05:44:11PM -0400, Tom Lane wrote:
> Right.  So there are basically two things we could do about this:
> 
> 1. set_config_option could decline to call pg_parameter_aclcheck
> if not IsTransactionState(), instead failing the assignment.
> This isn't a great answer because it would result in disallowing
> GUC assignments that users might expect to work.
> 
> 2. We could move process_session_preload_libraries() to someplace
> where a transaction is in progress -- probably, relocate it to
> inside InitPostgres().
> 
> I'm inclined to think that #2 is a better long-term solution,
> because it'd allow you to session-preload libraries that expect
> to be able to do database access during _PG_init.  (Right now
> that'd fail with the same sort of symptoms seen here.)  But
> there's no denying that this might have surprising side-effects
> for extensions that weren't expecting such a change.
> 
> It could also be reasonable to do both #1 and #2, with the idea
> that #1 might save us from crashing if there are any other
> code paths where we can reach that pg_parameter_aclcheck call
> outside a transaction.
> 
> Thoughts?

I wrote up a small patch along the same lines as #2 before seeing this
message.  It simply ensures that process_session_preload_libraries() is
called within a transaction.  I don't have a strong opinion about doing it
this way versus moving this call somewhere else as you proposed, but I'd
agree that #2 is a better long-term solution than #1.  AFAICT
shared_preload_libraries, even with EXEC_BACKEND, should not have the same
problem.

I'm not sure whether we should be worried about libraries that are already
creating transactions in their _PG_init() functions.  Off the top of my
head, I don't recall seeing anything like that.  Even if it does impact
some extensions, it doesn't seem like it'd be too much trouble to fix.

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8ba1c170f0..fd471d74a3 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4115,8 +4115,15 @@ PostgresMain(const char *dbname, const char *username)
     /*
      * process any libraries that should be preloaded at backend start (this
      * likewise can't be done until GUC settings are complete)
+     *
+     * If the user provided a setting at session startup for a custom GUC
+     * defined by one of these libraries, we might need syscache access when
+     * evaluating whether she has permission to set it, so do this step within
+     * a transaction.
      */
+    StartTransactionCommand();
     process_session_preload_libraries();
+    CommitTransactionCommand();
 
     /*
      * Send this backend's cancellation info to the frontend.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> +    StartTransactionCommand();
>      process_session_preload_libraries();
> +    CommitTransactionCommand();

Yeah, that way would avoid any questions about changing the order of
operations, but it seems like a mighty expensive solution: it's
adding a transaction to each backend start on the off chance that
(a) session_preload_libraries/local_preload_libraries is nonempty and
(b) the loaded libraries are going to do anything where it'd matter.
So that's why I thought of moving the call inside a pre-existing
transaction.

If we had to back-patch this into any released versions, I'd agree with
taking the performance hit in order to reduce the chance of side-effects.
But I think as long as we only have to do it in v15, it's not too late to
possibly cause some compatibility issues for extensions.

            regards, tom lane

On Thu, Jul 21, 2022 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Gurjeet Singh <gurjeet@singh.im> writes:
> > While poking at plperl's GUC in an internal discussion, I was able to
> > induce a crash (or an assertion failure in assert-enabled builds) as
> > an unprivileged user.
> > My investigation so far has revealed that the code path for the
> > following condition has never been tested, and because of this, when a
> > user tries to override an SUSET param via PGOPTIONS, Postgres tries to
> > perform a table lookup during process initialization. Because there's
> > no transaction in progress, and because this table is not in the
> > primed caches, we end up with code trying to dereference an
> > uninitialized  CurrentResourceOwner.
>
> Right.  So there are basically two things we could do about this:
>
> 1. set_config_option could decline to call pg_parameter_aclcheck
> if not IsTransactionState(), instead failing the assignment.
> This isn't a great answer because it would result in disallowing
> GUC assignments that users might expect to work.
>
> 2. We could move process_session_preload_libraries() to someplace
> where a transaction is in progress -- probably, relocate it to
> inside InitPostgres().
>
> I'm inclined to think that #2 is a better long-term solution,
> because it'd allow you to session-preload libraries that expect
> to be able to do database access during _PG_init.  (Right now
> that'd fail with the same sort of symptoms seen here.)  But
> there's no denying that this might have surprising side-effects
> for extensions that weren't expecting such a change.
>
> It could also be reasonable to do both #1 and #2, with the idea
> that #1 might save us from crashing if there are any other
> code paths where we can reach that pg_parameter_aclcheck call
> outside a transaction.
>
> Thoughts?

I had debated just wrapping the process_session_preload_libraries()
call with a transaction, like Nathan's patch posted ealier on this
thread does. But I hesitated because of the sensitivity around the
order of operations/call during process initialization.

I like the idea of performing library initialization in
InitPostgres(), as it performs the first transaction of the
connection, and because of the libraries' ability to gin up new GUC
variables that might need special handling, and also if it allows them
to do database access.

I think anywhere after the 'PostAuthDelay' check in InitPostgres()
would be a good place to perform process_session_preload_libraries().
I'm inclined to invoke it as late as possible, before we commit the
transaction.

As for making set_config_option() throw an error if not in
transaction, I'm not a big fan of checks  that break the flow, and of
unrelated code showing up when reading a function. For a casual
reader, such a check for transaction would make for a jarring
experience; "why are we checking for active transaction in  the guts
of guc.c?", they might think. If anything, such an error should be
thrown from or below pg_parameter_aclcheck().

But I am not sure if it should be exposed as an error. A user
encountering that error is not at fault. Hence I believe an assertion
check is more suitable for catching code that invokes
set_config_option() outside a transaction.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 21, 2022 at 3:29 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 05:44:11PM -0400, Tom Lane wrote:
> > Right.  So there are basically two things we could do about this:
> >
> > 1. set_config_option could decline to call pg_parameter_aclcheck
> > if not IsTransactionState(), instead failing the assignment.
> > This isn't a great answer because it would result in disallowing
> > GUC assignments that users might expect to work.
> >
> > 2. We could move process_session_preload_libraries() to someplace
> > where a transaction is in progress -- probably, relocate it to
> > inside InitPostgres().
> >
> > I'm inclined to think that #2 is a better long-term solution,
> > because it'd allow you to session-preload libraries that expect
> > to be able to do database access during _PG_init.  (Right now
> > that'd fail with the same sort of symptoms seen here.)  But
> > there's no denying that this might have surprising side-effects
> > for extensions that weren't expecting such a change.
> >
> > It could also be reasonable to do both #1 and #2, with the idea
> > that #1 might save us from crashing if there are any other
> > code paths where we can reach that pg_parameter_aclcheck call
> > outside a transaction.
> >
> > Thoughts?
>
> I wrote up a small patch along the same lines as #2 before seeing this
> message.  It simply ensures that process_session_preload_libraries() is
> called within a transaction.  I don't have a strong opinion about doing it
> this way versus moving this call somewhere else as you proposed, but I'd
> agree that #2 is a better long-term solution than #1.  AFAICT
> shared_preload_libraries, even with EXEC_BACKEND, should not have the same
> problem.
>
> I'm not sure whether we should be worried about libraries that are already
> creating transactions in their _PG_init() functions.  Off the top of my
> head, I don't recall seeing anything like that.  Even if it does impact
> some extensions, it doesn't seem like it'd be too much trouble to fix.
>
> diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
> index 8ba1c170f0..fd471d74a3 100644
> --- a/src/backend/tcop/postgres.c
> +++ b/src/backend/tcop/postgres.c
> @@ -4115,8 +4115,15 @@ PostgresMain(const char *dbname, const char *username)
>      /*
>       * process any libraries that should be preloaded at backend start (this
>       * likewise can't be done until GUC settings are complete)
> +     *
> +     * If the user provided a setting at session startup for a custom GUC
> +     * defined by one of these libraries, we might need syscache access when
> +     * evaluating whether she has permission to set it, so do this step within
> +     * a transaction.
>       */
> +    StartTransactionCommand();
>      process_session_preload_libraries();
> +    CommitTransactionCommand();
>
>      /*
>       * Send this backend's cancellation info to the frontend.

(none of the following is your patch's fault)

I don't think that is a good call-site for
process_session_preload_libraries(), because a library being loaded
can declare its own GUCs, hence I believe this should be called at
least before the call to BeginReportingGUCOptions().

If an extension creates a GUC with GUC_REPORT flag, it is violating
expectations. But since the DefineCustomXVariable() stack does not
prevent the callers from doing so, we must still honor the protocol
followed for all params with GUC_REPORT. And hence the

I think it'd be a good idea to ban the callers of
DefineCustomXVariable() from declaring their variable GUC_REPORT, to
ensure that only core code can define such variables.

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 21, 2022 at 07:30:20PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> +    StartTransactionCommand();
>>      process_session_preload_libraries();
>> +    CommitTransactionCommand();
> 
> Yeah, that way would avoid any questions about changing the order of
> operations, but it seems like a mighty expensive solution: it's
> adding a transaction to each backend start on the off chance that
> (a) session_preload_libraries/local_preload_libraries is nonempty and
> (b) the loaded libraries are going to do anything where it'd matter.
> So that's why I thought of moving the call inside a pre-existing
> transaction.
> 
> If we had to back-patch this into any released versions, I'd agree with
> taking the performance hit in order to reduce the chance of side-effects.
> But I think as long as we only have to do it in v15, it's not too late to
> possibly cause some compatibility issues for extensions.

Yeah, fair point.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jul 21, 2022 at 4:35 PM Gurjeet Singh <gurjeet@singh.im> wrote:
> I like the idea of performing library initialization in
> InitPostgres(), as it performs the first transaction of the
> connection, and because of the libraries' ability to gin up new GUC
> variables that might need special handling, and also if it allows them
> to do database access.
>
> I think anywhere after the 'PostAuthDelay' check in InitPostgres()
> would be a good place to perform process_session_preload_libraries().
> I'm inclined to invoke it as late as possible, before we commit the
> transaction.
>
> As for making set_config_option() throw an error if not in
> transaction, I'm not a big fan of checks  that break the flow, and of
> unrelated code showing up when reading a function. For a casual
> reader, such a check for transaction would make for a jarring
> experience; "why are we checking for active transaction in  the guts
> of guc.c?", they might think. If anything, such an error should be
> thrown from or below pg_parameter_aclcheck().
>
> But I am not sure if it should be exposed as an error. A user
> encountering that error is not at fault. Hence I believe an assertion
> check is more suitable for catching code that invokes
> set_config_option() outside a transaction.

Please see attached the patch that implements the above proposal.

The process_session_preload_libraries() call has been moved to the end
of InitPostgres(), just before we report the backend to
PgBackendStatus and commit the first transaction.

One notable side effect of this change is that
process_session_preload_libraries() is now called _before_ we
SetProcessingMode(NormalProcessing). Which means any database access
performed by _PG_init() of an extension will be doing it in
InitProcessing mode. I'm not sure if that's problematic.

The patch also adds an assertion in pg_parameter_aclcheck() to ensure
that there's a transaction is in progress before it's called.

The patch now lets the user connect, throws a warning, and does not crash.

$ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
WARNING:  permission denied to set parameter "plperl.on_plperl_init"
Expanded display is used automatically.
psql (15beta1)
Type "help" for help.

postgres@B:694512=>

Best regards,
Gurjeet
http://Gurje.et

On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote:
> One notable side effect of this change is that
> process_session_preload_libraries() is now called _before_ we
> SetProcessingMode(NormalProcessing). Which means any database access
> performed by _PG_init() of an extension will be doing it in
> InitProcessing mode. I'm not sure if that's problematic.

I cannot see a reason why on top of my mind.  The restrictions of
InitProcessing apply to two code paths of bgworkers connecting to a
database, and normal processing is used as a barrier to prevent the
creation of some objects.

> The patch also adds an assertion in pg_parameter_aclcheck() to ensure
> that there's a transaction is in progress before it's called.

+       /* It's pointless to call this function, unless we're in a transaction. */
+       Assert(IsTransactionState());

This can involve extension code, I think that this should be at least
an elog(ERROR) so as we have higher chances of knowing if something
still goes wrong in the wild.

> The patch now lets the user connect, throws a warning, and does not crash.
>
> $ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
> WARNING:  permission denied to set parameter "plperl.on_plperl_init"
> Expanded display is used automatically.
> psql (15beta1)
> Type "help" for help.

I am wondering whether we'd better have a test on this one with a
non-superuser.  Except for a few tests in the unsafe section,
session_preload_libraries has a limited amount of coverage.

+       /*
+        * process any libraries that should be preloaded at backend start (this
+        * can't be done until GUC settings are complete). Note that these libraries
+        * can declare new GUC variables.
+        */
+       process_session_preload_libraries();
There is no point in doing that during bootstrap anyway, no?
--
Michael

At Fri, 22 Jul 2022 10:00:34 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote:
> > One notable side effect of this change is that
> > process_session_preload_libraries() is now called _before_ we
> > SetProcessingMode(NormalProcessing). Which means any database access
> > performed by _PG_init() of an extension will be doing it in
> > InitProcessing mode. I'm not sure if that's problematic.
> 
> I cannot see a reason why on top of my mind.  The restrictions of
> InitProcessing apply to two code paths of bgworkers connecting to a
> database, and normal processing is used as a barrier to prevent the
> creation of some objects.
> 
> > The patch also adds an assertion in pg_parameter_aclcheck() to ensure
> > that there's a transaction is in progress before it's called.
> 
> +       /* It's pointless to call this function, unless we're in a transaction. */
> +       Assert(IsTransactionState());
> 
> This can involve extension code, I think that this should be at least
> an elog(ERROR) so as we have higher chances of knowing if something
> still goes wrong in the wild.

pg_parameter_aclmask involves the same assertion, so the same
backtrace can be obtained without it.  I think it is no bad of users
so I'm not sure ERROR is appropriate even if we were to add something
there.

> > The patch now lets the user connect, throws a warning, and does not crash.
> > 
> > $ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test
> > WARNING:  permission denied to set parameter "plperl.on_plperl_init"
> > Expanded display is used automatically.
> > psql (15beta1)
> > Type "help" for help.
> 
> I am wondering whether we'd better have a test on this one with a
> non-superuser.  Except for a few tests in the unsafe section,
> session_preload_libraries has a limited amount of coverage.

+1

> +       /*
> +        * process any libraries that should be preloaded at backend start (this
> +        * can't be done until GUC settings are complete). Note that these libraries
> +        * can declare new GUC variables.
> +        */
> +       process_session_preload_libraries();
> There is no point in doing that during bootstrap anyway, no?

This patch makes process_session_preload_libraries called in
autovacuum worker/launcher and background worker in addition to client
backends.  It seems to me we also need to prevent that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 22 Jul 2022 10:00:34 +0900, Michael Paquier <michael@paquier.xyz> wrote in
>> On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote:
>>> The patch also adds an assertion in pg_parameter_aclcheck() to ensure
>>> that there's a transaction is in progress before it's called.

>> This can involve extension code, I think that this should be at least
>> an elog(ERROR) so as we have higher chances of knowing if something
>> still goes wrong in the wild.

That assert strikes me as having been inserted with the advice of a
dartboard.  Why pg_parameter_aclcheck, and not any other aclchk.c
functions?  Why in the callers at all, rather than somewhere down
inside the syscache code?  And why isn't the existing Assert that
you started the thread with plenty sufficient for that already?

> This patch makes process_session_preload_libraries called in
> autovacuum worker/launcher and background worker in addition to client
> backends.  It seems to me we also need to prevent that.

Yeah.  I think the definition of session/local_preload_libraries
is that it loads libraries into *interactive* sessions.  The
existing coding seems already buggy in that regard, because it
will load such libraries into walsenders as well; isn't that
a POLA violation?

So I propose the attached.  I tested this to the extent of checking
that all our contrib modules can be loaded via session_preload_libraries.
That doesn't prove a whole lot about what outside extensions might do,
but it's some comfort.

            regards, tom lane

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8ba1c170f0..67098aa204 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4112,12 +4112,6 @@ PostgresMain(const char *dbname, const char *username)
     if (am_walsender)
         InitWalSender();

-    /*
-     * process any libraries that should be preloaded at backend start (this
-     * likewise can't be done until GUC settings are complete)
-     */
-    process_session_preload_libraries();
-
     /*
      * Send this backend's cancellation info to the frontend.
      */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index a5c208a20a..47605b9d59 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -1108,6 +1108,19 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
     /* Initialize this backend's session state. */
     InitializeSession();

+    /*
+     * If this is an interactive session, load any libraries that should be
+     * preloaded at backend start.  Since those are determined by GUCs, this
+     * can't happen until GUC settings are complete, but we want it to happen
+     * during the initial transaction in case anything that requires database
+     * access needs to be done.
+     */
+    if (!bootstrap &&
+        !IsAutoVacuumWorkerProcess() &&
+        !IsBackgroundWorker &&
+        !am_walsender)
+        process_session_preload_libraries();
+
     /* report this backend in the PgBackendStatus array */
     if (!bootstrap)
         pgstat_bestart();

On Fri, Jul 22, 2022 at 02:56:22PM -0400, Tom Lane wrote:
> +    /*
> +     * If this is an interactive session, load any libraries that should be
> +     * preloaded at backend start.  Since those are determined by GUCs, this
> +     * can't happen until GUC settings are complete, but we want it to happen
> +     * during the initial transaction in case anything that requires database
> +     * access needs to be done.
> +     */
> +    if (!bootstrap &&
> +        !IsAutoVacuumWorkerProcess() &&
> +        !IsBackgroundWorker &&
> +        !am_walsender)
> +        process_session_preload_libraries();

I worry that this will be easily missed when adding new types of
non-interactive sessions, but I can't claim to have a better idea.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Jul 22, 2022 at 02:56:22PM -0400, Tom Lane wrote:
>> +    if (!bootstrap &&
>> +        !IsAutoVacuumWorkerProcess() &&
>> +        !IsBackgroundWorker &&
>> +        !am_walsender)
>> +        process_session_preload_libraries();

> I worry that this will be easily missed when adding new types of
> non-interactive sessions, but I can't claim to have a better idea.

Yeah, that bothered me too.  A variant that I'd considered is to
create a local variable "bool interactive" and set it properly
in each of the arms of the if-chain dealing with authentication
(starting about postinit.c:800).  While that approach would cover
most of the tests shown above, it would not have exposed the issue
of needing to check am_walsender, so I'm not very convinced that
it'd be any better.

Another idea is to add a "bool interactive" parameter to InitPostgres,
thereby shoving the issue out to the call sites.  Still wouldn't
expose the am_walsender angle, but conceivably it'd be more
future-proof anyway?

            regards, tom lane

On Fri, Jul 22, 2022 at 06:44:04PM -0400, Tom Lane wrote:
> Another idea is to add a "bool interactive" parameter to InitPostgres,
> thereby shoving the issue out to the call sites.  Still wouldn't
> expose the am_walsender angle, but conceivably it'd be more
> future-proof anyway?

I hesitated to suggest this exactly because of the WAL sender problem, but
it does seem slightly more future-proof, so +1 for this approach.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Jul 22, 2022 at 06:44:04PM -0400, Tom Lane wrote:
>> Another idea is to add a "bool interactive" parameter to InitPostgres,
>> thereby shoving the issue out to the call sites.  Still wouldn't
>> expose the am_walsender angle, but conceivably it'd be more
>> future-proof anyway?

> I hesitated to suggest this exactly because of the WAL sender problem, but
> it does seem slightly more future-proof, so +1 for this approach.

So about like this then.  (I spent some effort on cleaning up the
disjointed-to-nonexistent presentation of InitPostgres' parameters.)

            regards, tom lane

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 088556ab54..58752368e7 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -354,7 +354,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
     if (pg_link_canary_is_frontend())
         elog(ERROR, "backend is incorrectly linked to frontend functions");

-    InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
+    InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);

     /* Initialize stuff for bootstrap-file processing */
     for (i = 0; i < MAXATTR; i++)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2e146aac93..70a9176c54 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -475,7 +475,7 @@ AutoVacLauncherMain(int argc, char *argv[])
     /* Early initialization */
     BaseInit();

-    InitPostgres(NULL, InvalidOid, NULL, InvalidOid, NULL, false);
+    InitPostgres(NULL, InvalidOid, NULL, InvalidOid, false, false, NULL);

     SetProcessingMode(NormalProcessing);

@@ -1694,12 +1694,13 @@ AutoVacWorkerMain(int argc, char *argv[])
         pgstat_report_autovac(dbid);

         /*
-         * Connect to the selected database
+         * Connect to the selected database, specifying no particular user
          *
          * Note: if we have selected a just-deleted database (due to using
          * stale stats info), we'll fail and exit here.
          */
-        InitPostgres(NULL, dbid, NULL, InvalidOid, dbname, false);
+        InitPostgres(NULL, dbid, NULL, InvalidOid, false, false,
+                     dbname);
         SetProcessingMode(NormalProcessing);
         set_ps_display(dbname);
         ereport(DEBUG1,
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 1c25457526..e541b16bdb 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -5654,7 +5654,11 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                  errmsg("database connection requirement not indicated during registration")));

-    InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, (flags & BGWORKER_BYPASS_ALLOWCONN) != 0);
+    InitPostgres(dbname, InvalidOid,    /* database to connect to */
+                 username, InvalidOid,    /* role to connect as */
+                 false,            /* never honor session_preload_libraries */
+                 (flags & BGWORKER_BYPASS_ALLOWCONN) != 0,    /* ignore datallowconn? */
+                 NULL);            /* no out_dbname */

     /* it had better not gotten out of "init" mode yet */
     if (!IsInitProcessingMode())
@@ -5677,7 +5681,11 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                  errmsg("database connection requirement not indicated during registration")));

-    InitPostgres(NULL, dboid, NULL, useroid, NULL, (flags & BGWORKER_BYPASS_ALLOWCONN) != 0);
+    InitPostgres(NULL, dboid,    /* database to connect to */
+                 NULL, useroid, /* role to connect as */
+                 false,            /* never honor session_preload_libraries */
+                 (flags & BGWORKER_BYPASS_ALLOWCONN) != 0,    /* ignore datallowconn? */
+                 NULL);            /* no out_dbname */

     /* it had better not gotten out of "init" mode yet */
     if (!IsInitProcessingMode())
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8ba1c170f0..3772329759 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4076,7 +4076,11 @@ PostgresMain(const char *dbname, const char *username)
      * it inside InitPostgres() instead.  In particular, anything that
      * involves database access should be there, not here.
      */
-    InitPostgres(dbname, InvalidOid, username, InvalidOid, NULL, false);
+    InitPostgres(dbname, InvalidOid,    /* database to connect to */
+                 username, InvalidOid,    /* role to connect as */
+                 !am_walsender, /* honor session_preload_libraries? */
+                 false,            /* don't ignore datallowconn */
+                 NULL);            /* no out_dbname */

     /*
      * If the PostmasterContext is still around, recycle the space; we don't
@@ -4112,12 +4116,6 @@ PostgresMain(const char *dbname, const char *username)
     if (am_walsender)
         InitWalSender();

-    /*
-     * process any libraries that should be preloaded at backend start (this
-     * likewise can't be done until GUC settings are complete)
-     */
-    process_session_preload_libraries();
-
     /*
      * Send this backend's cancellation info to the frontend.
      */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index a5c208a20a..de797c5933 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -622,29 +622,45 @@ BaseInit(void)
  * InitPostgres
  *        Initialize POSTGRES.
  *
+ * Arguments:
+ *    in_dbname, dboid: specify database to connect to, as described below
+ *    username, useroid: specify role to connect as, as described below
+ *    load_session_libraries: TRUE to honor session_preload_libraries
+ *    override_allow_connections: TRUE to connect despite !datallowconn
+ *    out_dbname: optional output parameter, see below; pass NULL if not used
+ *
  * The database can be specified by name, using the in_dbname parameter, or by
- * OID, using the dboid parameter.  In the latter case, the actual database
+ * OID, using the dboid parameter.  Specify NULL or InvalidOid respectively
+ * for the unused parameter.  If dboid is provided, the actual database
  * name can be returned to the caller in out_dbname.  If out_dbname isn't
  * NULL, it must point to a buffer of size NAMEDATALEN.
  *
- * Similarly, the username can be passed by name, using the username parameter,
+ * Similarly, the role can be passed by name, using the username parameter,
  * or by OID using the useroid parameter.
  *
- * In bootstrap mode no parameters are used.  The autovacuum launcher process
- * doesn't use any parameters either, because it only goes far enough to be
- * able to read pg_database; it doesn't connect to any particular database.
- * In walsender mode only username is used.
+ * In bootstrap mode the database and username parameters are NULL/InvalidOid.
+ * The autovacuum launcher process doesn't specify these parameters either,
+ * because it only goes far enough to be able to read pg_database; it doesn't
+ * connect to any particular database.  An autovacuum worker specifies a
+ * database but not a username; conversely, a physical walsender specifies
+ * username but not database.
+ *
+ * By convention, load_session_libraries should be passed as true in
+ * "interactive" sessions, false in background processes such as autovacuum.
  *
- * As of PostgreSQL 8.2, we expect InitProcess() was already called, so we
- * already have a PGPROC struct ... but it's not completely filled in yet.
+ * We expect that InitProcess() was already called, so we already have a
+ * PGPROC struct ... but it's not completely filled in yet.
  *
  * Note:
  *        Be very careful with the order of calls in the InitPostgres function.
  * --------------------------------
  */
 void
-InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-             Oid useroid, char *out_dbname, bool override_allow_connections)
+InitPostgres(const char *in_dbname, Oid dboid,
+             const char *username, Oid useroid,
+             bool load_session_libraries,
+             bool override_allow_connections,
+             char *out_dbname)
 {
     bool        bootstrap = IsBootstrapProcessingMode();
     bool        am_superuser;
@@ -1108,6 +1124,16 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
     /* Initialize this backend's session state. */
     InitializeSession();

+    /*
+     * If this is an interactive session, load any libraries that should be
+     * preloaded at backend start.  Since those are determined by GUCs, this
+     * can't happen until GUC settings are complete, but we want it to happen
+     * during the initial transaction in case anything that requires database
+     * access needs to be done.
+     */
+    if (load_session_libraries)
+        process_session_preload_libraries();
+
     /* report this backend in the PgBackendStatus array */
     if (!bootstrap)
         pgstat_bestart();
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index ea9a56d395..067b729d5a 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -449,8 +449,11 @@ extern PGDLLIMPORT AuxProcType MyAuxProcType;
 /* in utils/init/postinit.c */
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
-extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username,
-                         Oid useroid, char *out_dbname, bool override_allow_connections);
+extern void InitPostgres(const char *in_dbname, Oid dboid,
+                         const char *username, Oid useroid,
+                         bool load_session_libraries,
+                         bool override_allow_connections,
+                         char *out_dbname);
 extern void BaseInit(void);

 /* in utils/init/miscinit.c */

On Sat, Jul 23, 2022 at 01:23:24PM -0400, Tom Lane wrote:
> -    /*
> -     * process any libraries that should be preloaded at backend start (this
> -     * likewise can't be done until GUC settings are complete)
> -     */
> -    process_session_preload_libraries();

This patch essentially moveѕ the call to
process_session_preload_libraries() to earlier in PostgresMain().  The
discussion upthread seems to indicate that this is okay.  I did notice that
the log_disconnections handler won't be set up yet, so failures due to
session_preload_libraries won't be logged the same way as before.  Also,
the call to pgstat_report_connect() won't happen.  Neither of these strikes
me as particularly bad, but it seemed worth noting.

> + *    load_session_libraries: TRUE to honor session_preload_libraries

nitpick: Should we call out local_preload_libraries here, too?

> +    if (load_session_libraries)
> +        process_session_preload_libraries();

I noticed that a couple of places check whether whereToSendOutput is set to
DestRemote to determine if this is an interactive session.  Maybe something
like

    if (whereToSendOutput == DestRemote && !am_walsender)

would be a reasonably future-proof way to avoid the need for a new
InitPostgres() argument.

Otherwise, the patch looks good to me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> I noticed that a couple of places check whether whereToSendOutput is set to
> DestRemote to determine if this is an interactive session.

IIRC, that would end in not loading the preload libraries in a standalone
backend.  Perhaps that's what we want, but I'd supposed not.  Discuss.

            regards, tom lane

On Sun, Jul 24, 2022 at 11:49:23PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I noticed that a couple of places check whether whereToSendOutput is set to
>> DestRemote to determine if this is an interactive session.
> 
> IIRC, that would end in not loading the preload libraries in a standalone
> backend.  Perhaps that's what we want, but I'd supposed not.  Discuss.

Ah, I see.  There was a recent change to make sure shared_preload_libraries
are loaded in single-user mode (6c31ac0), but those are for load at "server
start" instead of "connection start."  However, AFAICT
session_preload_libraries is loaded in single-user mode today, and
single-user mode is arguably a connection, so my instinct is that we should
continue to process it in single-user mode.  I suppose we might be able to
add more hacks to load it in single-user mode without a new argument, but
at that point, we're probably not too far from your original proposal.
Given all this, I think I'm inclined for the new argument.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> Given all this, I think I'm inclined for the new argument.

Pushed like that then (after a bit more fooling with the comments).

I haven't done anything about a test case.  We can't rely on plperl
getting built, and even if we could, it doesn't have any TAP-style
tests so it'd be hard to get it to test this scenario.  However,
I do see that we're not testing session_preload_libraries anywhere,
which seems bad.  I wonder if it'd be a good idea to convert
auto_explain's TAP test to load auto_explain via session_preload_libraries
instead of shared_preload_libraries, and then pass in the settings for
each test via PGOPTIONS instead of constantly rewriting postgresql.conf.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> I wonder if it'd be a good idea to convert
> auto_explain's TAP test to load auto_explain via session_preload_libraries
> instead of shared_preload_libraries, and then pass in the settings for
> each test via PGOPTIONS instead of constantly rewriting postgresql.conf.

That whole config-file rewriting did feel a bit icky when I added more
tests recently, but I completely forgot about PGOPTIONS and -c.
Something like the attached is indeed much nicer.

- ilmari

From c9d6850e89ff95df1034c13ca9c10aaf1cf0f02f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 25 Jul 2022 16:23:09 +0100
Subject: [PATCH] Use PGOPTIONS and session_preload_libraries in auto_explain
 tests

Instead of constantly rewriting postgresql.conf.  Also change the from
shared_preload_libraries to session_preload_libraries to givesome
coverage of extensions providing custom GUCs.
---
 contrib/auto_explain/t/001_auto_explain.pl | 24 +++-------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index 1d952fb54d..b86761919b 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -16,38 +16,20 @@ sub query_log
     my ($node, $sql, $params) = @_;
     $params ||= {};
 
-    if (keys %$params)
-    {
-        for my $key (keys %$params)
-        {
-            $node->append_conf('postgresql.conf', "$key = $params->{$key}\n");
-        }
-        $node->reload;
-    }
+    local $ENV{PGOPTIONS} = join " ", map { "-c $_=$params->{$_}" } keys %$params;
 
     my $log    = $node->logfile();
     my $offset = -s $log;
 
     $node->safe_psql("postgres", $sql);
 
-    my $log_contents = slurp_file($log, $offset);
-
-    if (keys %$params)
-    {
-        for my $key (keys %$params)
-        {
-            $node->adjust_conf('postgresql.conf', $key, undef);
-        }
-        $node->reload;
-    }
-
-    return $log_contents;
+    return slurp_file($log, $offset);
 }
 
 my $node = PostgreSQL::Test::Cluster->new('main');
 $node->init;
 $node->append_conf('postgresql.conf',
-    "shared_preload_libraries = 'auto_explain'");
+    "session_preload_libraries = 'auto_explain'");
 $node->append_conf('postgresql.conf', "auto_explain.log_min_duration = 0");
 $node->append_conf('postgresql.conf', "auto_explain.log_analyze = on");
 $node->start;
-- 
2.30.2

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I wonder if it'd be a good idea to convert
>> auto_explain's TAP test to load auto_explain via session_preload_libraries
>> instead of shared_preload_libraries, and then pass in the settings for
>> each test via PGOPTIONS instead of constantly rewriting postgresql.conf.

> That whole config-file rewriting did feel a bit icky when I added more
> tests recently, but I completely forgot about PGOPTIONS and -c.
> Something like the attached is indeed much nicer.

Thanks!  I added a test to verify the permissions-checking issue
and pushed it.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> Tom Lane <tgl@sss.pgh.pa.us> writes:
>>> I wonder if it'd be a good idea to convert
>>> auto_explain's TAP test to load auto_explain via session_preload_libraries
>>> instead of shared_preload_libraries, and then pass in the settings for
>>> each test via PGOPTIONS instead of constantly rewriting postgresql.conf.
>
>> That whole config-file rewriting did feel a bit icky when I added more
>> tests recently, but I completely forgot about PGOPTIONS and -c.
>> Something like the attached is indeed much nicer.
>
> Thanks!  I added a test to verify the permissions-checking issue
> and pushed it.

Thanks!  Just one minor nitpick: setting an %ENV entry to `undef`
doesn't unset the environment variable, it sets it to the empty string.
To unset a variable it needs to be deleted from %ENV, i.e. `delete
$ENV{PGUSER};`.  Alternatively, wrap the relevant tests in a block and
use `local`, like in the `query_log` function.

>             regards, tom lane

- ilmari

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Thanks!  Just one minor nitpick: setting an %ENV entry to `undef`
> doesn't unset the environment variable, it sets it to the empty string.
> To unset a variable it needs to be deleted from %ENV, i.e. `delete
> $ENV{PGUSER};`.

Ah.  Still, libpq doesn't distinguish, so the test works anyway.
Not sure if it's worth changing.

            regards, tom lane

I wrote:
> =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
>> Thanks!  Just one minor nitpick: setting an %ENV entry to `undef`
>> doesn't unset the environment variable, it sets it to the empty string.
>> To unset a variable it needs to be deleted from %ENV, i.e. `delete
>> $ENV{PGUSER};`.

> Ah.  Still, libpq doesn't distinguish, so the test works anyway.
> Not sure if it's worth changing.

Meh ... I had to un-break the test for Windows, so did this while
at it, using the local-in-block method.  Thanks for the suggestion.

            regards, tom lane