Обсуждение: Can a background worker exist without shared memory access for EXEC_BACKEND cases?

Hi,

I tried to implement a static background(bg) worker without shared
memory access (BGWORKER_SHMEM_ACCESS), it worked fine on Linux machine
where EXEC_BACKEND is not defined(thanks to the fork() implementation
which does great job to get the global state from the
postmaster(parent) to bg worker(child)).

However, the problem arised, when I switched to EXEC_BACKEND mode, it
seems it doesn't. I digged a bit and the following is my analysis: for
EXEC_BACKEND cases, (say on Windows platforms where fork() doesn't
exist) the way postmaster creates a background worker is entirely
different. It is done through SubPostmasterMain and the global state
from the postmaster is shared with the background worker via shared
memory. MyLatch variable also gets created in shared mode. And having
no shared memory access for the worker for EXEC_BACKEND cases(in
StartBackgroundWorker, the shared memory segments get detached), the
worker fails to receive all the global state from the postmaster.
Looks like the background worker needs to have the
BGWORKER_SHMEM_ACCESS flag while registering for EXEC_BACKEND cases.

Please feel free to correct me if I miss anything here.

If the above analysis looks fine, then please find a patch that adds
some info in bgworker.sgml and bgworker.h.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Jul 31, 2020 at 11:13 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> memory. MyLatch variable also gets created in shared mode. And having
> no shared memory access for the worker for EXEC_BACKEND cases(in
> StartBackgroundWorker, the shared memory segments get detached), the
> worker fails to receive all the global state from the postmaster.

What exactly do you mean by "all the global state"?

It's certainly true that if you declare some random static variable
and initialize it in the postmaster, and you don't take any special
precautions to propagate that into workers, then on an EXEC_BACKEND
build, it won't be set in the workers. That's why, for example, most
of the *ShmemInit() functions are written like this:

        TwoPhaseState = ShmemInitStruct("Prepared Transaction Table",

 TwoPhaseShmemSize(),
                                                                        &found);
        if (!IsUnderPostmaster)
...initialize the data structure...
        else
                Assert(found);

The assignment to TwoPhaseState is unconditional, because in an
EXEC_BACKEND build that's going to be done in every process, and
otherwise the variable won't be set. But the initialization of the
shared data structure happens conditionally, because that needs to be
done only once.

See also the BackendParameters stuff, which arranges to pass down a
bunch of things to exec'd backends.

I am not necessarily opposed to trying to clarify the documentation
and/or comments here, but "global state" is a fuzzy term that doesn't
really mean anything to me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thank you Robert for the comments.

On Mon, Aug 3, 2020 at 9:19 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Jul 31, 2020 at 11:13 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > memory. MyLatch variable also gets created in shared mode. And having
> > no shared memory access for the worker for EXEC_BACKEND cases(in
> > StartBackgroundWorker, the shared memory segments get detached), the
> > worker fails to receive all the global state from the postmaster.
>
> What exactly do you mean by "all the global state"?
>

My intention was exactly to refer to the variables specified in
BackendParameters struct.

>
> It's certainly true that if you declare some random static variable
> and initialize it in the postmaster, and you don't take any special
> precautions to propagate that into workers, then on an EXEC_BACKEND
> build, it won't be set in the workers. That's why, for example, most
> of the *ShmemInit() functions are written like this:
>
>         TwoPhaseState = ShmemInitStruct("Prepared Transaction Table",
>
>  TwoPhaseShmemSize(),
>                                                                         &found);
>         if (!IsUnderPostmaster)
> ...initialize the data structure...
>         else
>                 Assert(found);
>
> The assignment to TwoPhaseState is unconditional, because in an
> EXEC_BACKEND build that's going to be done in every process, and
> otherwise the variable won't be set. But the initialization of the
> shared data structure happens conditionally, because that needs to be
> done only once.
>
> See also the BackendParameters stuff, which arranges to pass down a
> bunch of things to exec'd backends.
>

I could get these points earlier in my initial analysis. In fact, I
could figure out the flow on Windows, how these parameters are shared
using a shared file(CreateFileMapping(), MapViewOfFile()), and the
shared file name being passed as an argv[2] to the child process, and
the way child process uses this file name to read the backend
parameters in read_backend_variables().

>
> I am not necessarily opposed to trying to clarify the documentation
> and/or comments here, but "global state" is a fuzzy term that doesn't
> really mean anything to me.
>

How about having "backend parameters from the postmaster....." as is
being referred to in the internal_forkexec() function comments? I
rephrased the comments adding "backend parameters.." and removing
"global state". Please find the v2 patch attached.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Aug 4, 2020 at 7:27 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> I could get these points earlier in my initial analysis. In fact, I
> could figure out the flow on Windows, how these parameters are shared
> using a shared file(CreateFileMapping(), MapViewOfFile()), and the
> shared file name being passed as an argv[2] to the child process, and
> the way child process uses this file name to read the backend
> parameters in read_backend_variables().

Doesn't that happen even if the background worker isn't declared to
use BGWORKER_SHMEM_ACCESS? See StartBackgroundWorker(): IIUC, we start
with shared memory access, then afterwards detach.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Aug 4, 2020 at 10:20 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Aug 4, 2020 at 7:27 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > I could get these points earlier in my initial analysis. In fact, I
> > could figure out the flow on Windows, how these parameters are shared
> > using a shared file(CreateFileMapping(), MapViewOfFile()), and the
> > shared file name being passed as an argv[2] to the child process, and
> > the way child process uses this file name to read the backend
> > parameters in read_backend_variables().
>
> Doesn't that happen even if the background worker isn't declared to
> use BGWORKER_SHMEM_ACCESS? See StartBackgroundWorker(): IIUC, we start
> with shared memory access, then afterwards detach.
>

Yes, the bg worker starts with shared memory access even with no
BGWORKER_SHMEM_ACCESS and later it gets detached in
StartBackgroundWorker() with PGSharedMemoryDetach().

if ((worker->bgw_flags & BGWORKER_SHMEM_ACCESS) == 0)
{
        dsm_detach_all();
        PGSharedMemoryDetach();
 }

In EXEC_BACKEND cases, right after PGSharedMemoryDetach(), the bg
worker will no longer be able to access the backend parameters, see
below(I tried this on my Ubuntu machine with a bgworker with no
BGWORKER_SHMEM_ACCESS flag and defined EXEC_BACKEND macro in
pg_config_manual.h) :

(gdb) p *MyLatch
Cannot access memory at address 0x7fd60424a6b4
(gdb) p *ShmemVariableCache
Cannot access memory at address 0x7fd58427bf80
(gdb) p ProcStructLock
$10 = (slock_t *) 0x7fd60429bd00 <error: Cannot access memory at
address 0x7fd60429bd00>
(gdb) p *AuxiliaryProcs
Cannot access memory at address 0x7fd60424cc60
(gdb) p *ProcGlobal
Cannot access memory at address 0x7fd604232880

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Aug 5, 2020 at 7:24 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> In EXEC_BACKEND cases, right after PGSharedMemoryDetach(), the bg
> worker will no longer be able to access the backend parameters, see
> below(I tried this on my Ubuntu machine with a bgworker with no
> BGWORKER_SHMEM_ACCESS flag and defined EXEC_BACKEND macro in
> pg_config_manual.h) :
>
> (gdb) p *MyLatch
> Cannot access memory at address 0x7fd60424a6b4
> (gdb) p *ShmemVariableCache
> Cannot access memory at address 0x7fd58427bf80
> (gdb) p ProcStructLock
> $10 = (slock_t *) 0x7fd60429bd00 <error: Cannot access memory at
> address 0x7fd60429bd00>
> (gdb) p *AuxiliaryProcs
> Cannot access memory at address 0x7fd60424cc60
> (gdb) p *ProcGlobal
> Cannot access memory at address 0x7fd604232880

Well all of those are pointers into the main shared memory segment,
which is expected to be inaccessible after it is detached. Hopefully
nobody should be surprised that if you don't specify
BGWORKER_SHMEM_ACCESS, you can't access data stored in shared memory.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 5, 2020 at 5:16 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Aug 5, 2020 at 7:24 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > In EXEC_BACKEND cases, right after PGSharedMemoryDetach(), the bg
> > worker will no longer be able to access the backend parameters, see
> > below(I tried this on my Ubuntu machine with a bgworker with no
> > BGWORKER_SHMEM_ACCESS flag and defined EXEC_BACKEND macro in
> > pg_config_manual.h) :
> >
> > (gdb) p *MyLatch
> > Cannot access memory at address 0x7fd60424a6b4
> > (gdb) p *ShmemVariableCache
> > Cannot access memory at address 0x7fd58427bf80
> > (gdb) p ProcStructLock
> > $10 = (slock_t *) 0x7fd60429bd00 <error: Cannot access memory at
> > address 0x7fd60429bd00>
> > (gdb) p *AuxiliaryProcs
> > Cannot access memory at address 0x7fd60424cc60
> > (gdb) p *ProcGlobal
> > Cannot access memory at address 0x7fd604232880
>
> Well all of those are pointers into the main shared memory segment,
> which is expected to be inaccessible after it is detached. Hopefully
> nobody should be surprised that if you don't specify
> BGWORKER_SHMEM_ACCESS, you can't access data stored in shared memory.
>

Right.

Will the proposed patch(v2) having some info in bgworker.sgml and
bgworker.h be ever useful to the users in some way?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Aug 5, 2020 at 9:02 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Will the proposed patch(v2) having some info in bgworker.sgml and
> bgworker.h be ever useful to the users in some way?

Well, it says things that aren't true, so, no, it's not useful. Your
patch claims that "the worker fails to receive the backend parameters
from the postmaster," but that's not the case. SubPostmasterMain()
first calls read_backend_variables() which calls
restore_backend_variables(); then later it calls
StartBackgroundWorker() which does PGSharedMemoryDetach(). So the
values of the backend variables *are* available in the worker
processes. Your debugger output also shows this: if
restore_backend_variables() weren't running in the child processes,
those variables would all be NULL, but you show them all at different
addresses in the 0x7fd... range, which is presumably where the shared
memory segment was mapped.

The reason you can't print out the results of dereferencing the
variables with * is because the memory to which the variables point is
no longer mapped in the process, not because the variables haven't
been initialized. If you looked at a variable that wasn't a pointer to
shared memory, but rather, say, an integer, like max_safe_fds or
MyCancelKey, I think you'd find that the value was preserved just
fine. I think you're confusing the pointer with the data to which it
points.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company