Обсуждение: why do shmem attach?
Exec-on-startup was removed by Bruce long time ago. Why we still attach to shmem after fork? Or shmem inheritance is not portable? Also, all this ShmemIndex stuff seems to be useless (except of backend PID lookup but it's for sure should be in separate hash table). And why separate shmem segment (!!!) is used for Slocks (ipc.c:CreateAndInitSLockMemory(), etc) - they use so small amount of memory! Just wondering... I'm going to use old shmem init code for WAL but would like to denote that shmem stuff need in cleanup. Vadim
[Charset koi8-r unsupported, filtering to ASCII...] > Exec-on-startup was removed by Bruce long time ago. > Why we still attach to shmem after fork? No idea. I know the shared memory stuff is not copy-on-write for forked children, so I am not sure why you would have to attach to it. > Or shmem inheritance is not portable? If it works on your machine with it removed, commit the change and I can test it here. I don't know of any portability problems with shared memory children. > Also, all this ShmemIndex stuff seems to be useless > (except of backend PID lookup but it's for sure > should be in separate hash table). > And why separate shmem segment (!!!) is used for > Slocks (ipc.c:CreateAndInitSLockMemory(), etc) - they > use so small amount of memory! No idea. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian wrote: > > > Or shmem inheritance is not portable? > > If it works on your machine with it removed, commit the change and I can > test it here. I don't know of any portability problems with shared > memory children. I wrote simple test program and it works under FreeBSD and Solaris (on Ultra). Currently I'm not able to do more. Actually, I worry does this work under MS Windows. Vadim
> -----Original Message----- > From: owner-pgsql-hackers@postgreSQL.org > [mailto:owner-pgsql-hackers@postgreSQL.org]On Behalf Of Vadim Mikheev > Sent: Monday, September 20, 1999 2:34 PM > To: Bruce Momjian > Cc: PostgreSQL Developers List > Subject: Re: [HACKERS] why do shmem attach? > > > Bruce Momjian wrote: > > > > > Or shmem inheritance is not portable? > > > > If it works on your machine with it removed, commit the change and I can > > test it here. I don't know of any portability problems with shared > > memory children. > > I wrote simple test program and it works under FreeBSD and Solaris > (on Ultra). Currently I'm not able to do more. Actually, I worry > does this work under MS Windows. > Where do we attach to shmem after fork() ? I couldn't find the place. Regards. Hiroshi Inoue Inoue@tpf.co.jp
Hiroshi Inoue wrote:
>
> Where do we attach to shmem after fork() ?
> I couldn't find the place.
Ops, sorry, you're right - postinit.c:InitCommunication():
if (!IsUnderPostmaster) /* postmaster already did this */ { PostgresIpcKey = key;
AttachSharedMemoryAndSemaphores(key); }
Though, AttachSharedMemoryAndSemaphores():
if (key == PrivateIPCKey) { CreateSharedMemoryAndSemaphores(key, 16); return; }
... and useless shmem attachment stuff follows after this ...
Cleanup is still required, but subj is closed, thanks -:)
Vadim
> Bruce Momjian wrote: > > > > > Or shmem inheritance is not portable? > > > > If it works on your machine with it removed, commit the change and I can > > test it here. I don't know of any portability problems with shared > > memory children. > > I wrote simple test program and it works under FreeBSD and Solaris > (on Ultra). Currently I'm not able to do more. Actually, I worry > does this work under MS Windows. This code was not added for MS Windows, so it is anyone's guess whether it needs it. Let's remove it and see. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> Hiroshi Inoue wrote:
> >
> > Where do we attach to shmem after fork() ?
> > I couldn't find the place.
>
> Ops, sorry, you're right - postinit.c:InitCommunication():
>
> if (!IsUnderPostmaster) /* postmaster already did this */
> {
> PostgresIpcKey = key;
> AttachSharedMemoryAndSemaphores(key);
> }
>
> Though, AttachSharedMemoryAndSemaphores():
>
> if (key == PrivateIPCKey)
> {
> CreateSharedMemoryAndSemaphores(key, 16);
> return;
> }
>
> ... and useless shmem attachment stuff follows after this ...
>
> Cleanup is still required, but subj is closed, thanks -:)
My guess is that this is something I missed when removing the exec().
-- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
Vadim Mikheev <vadim@krs.ru> writes:
> Though, AttachSharedMemoryAndSemaphores():
> if (key == PrivateIPCKey)
> {
> CreateSharedMemoryAndSemaphores(key, 16);
> return;
> }
> ... and useless shmem attachment stuff follows after this ...
That path is used for a standalone backend. Is that useless?
> Cleanup is still required, but subj is closed, thanks -:)
I don't think it's worth messing with either. It'd be nice for code
beautification purposes to (a) combine the three shared-mem segments
we currently have into one, and (b) rely on the postmaster's having
attached the segment, so that all backends will see it at the same
location in their address space, which would let us get rid of the
MAKE_OFFSET/MAKE_PTR cruft. But getting the full benefit would
require cleaning up a lot of code, and it just doesn't seem like
a high-priority task. I'm also a little worried that we'd be
sacrificing portability --- some day we might be glad that we can
move those segments around...
regards, tom lane
Vadim Mikheev <vadim@krs.ru> writes:
> Also, all this ShmemIndex stuff seems to be useless
> (except of backend PID lookup but it's for sure
> should be in separate hash table).
Have I got a deal for you ;-). I have uncommitted changes that add
a pointer (SHMEM_OFFSET that is) to each backend's PROC struct into
the per-backend info array that already existed in shmem.c. The
routines in shmem.c that searched for PROC structures are now in
sinval.c, and just do a simple scan of the ProcState array to find
the PROC structs. They should be a whole lot faster --- which is
good since these things run with spinlocks held...
These changes are intermixed with other things that are currently
triggering a lot of NOTICE: Buffer Leak messages in the regress tests,
so I don't want to commit until I've puzzled out the buffer refcount
issue. But I've got 'em and they seem to work fine.
> And why separate shmem segment (!!!) is used for
> Slocks (ipc.c:CreateAndInitSLockMemory(), etc) - they
> use so small amount of memory!
Historical reasons I suppose. shmem.c does assume that spinlocks
are already up and running when it is initialized, so combining
everything into one segment would require care. But it's surely
doable if someone wants to take the time. (I don't.)
regards, tom lane
Tom Lane wrote: > > Vadim Mikheev <vadim@krs.ru> writes: > > Also, all this ShmemIndex stuff seems to be useless > > (except of backend PID lookup but it's for sure > > should be in separate hash table). > > Have I got a deal for you ;-). I have uncommitted changes that add > a pointer (SHMEM_OFFSET that is) to each backend's PROC struct into > the per-backend info array that already existed in shmem.c. The > routines in shmem.c that searched for PROC structures are now in > sinval.c, and just do a simple scan of the ProcState array to find > the PROC structs. They should be a whole lot faster --- which is > good since these things run with spinlocks held... Nice. I have new member for PROC that should be searched sometime -:) Vadim
> I don't think it's worth messing with either. It'd be nice for code > beautification purposes to (a) combine the three shared-mem segments > we currently have into one, and (b) rely on the postmaster's having > attached the segment, so that all backends will see it at the same > location in their address space, which would let us get rid of the > MAKE_OFFSET/MAKE_PTR cruft. But getting the full benefit would > require cleaning up a lot of code, and it just doesn't seem like > a high-priority task. I'm also a little worried that we'd be > sacrificing portability --- some day we might be glad that we can > move those segments around... My opinion is that this code is complex enough without additional complexity. If something can be removed/cleaned, why not do it? It is usually very easy to do and doesn't take much time. The next person who has to mess with it will thank us. -- Bruce Momjian | http://www.op.net/~candle maillist@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Tom Lane wrote:
>
> Vadim Mikheev <vadim@krs.ru> writes:
> > Though, AttachSharedMemoryAndSemaphores():
> > if (key == PrivateIPCKey)
> > {
> > CreateSharedMemoryAndSemaphores(key, 16);
> > return;
> > }
> > ... and useless shmem attachment stuff follows after this ...
>
> That path is used for a standalone backend. Is that useless?
Isn't key equal to PrivateIPCKey for standalone backend?
>
> > Cleanup is still required, but subj is closed, thanks -:)
>
> I don't think it's worth messing with either. It'd be nice for code
> beautification purposes to (a) combine the three shared-mem segments
> we currently have into one, and (b) rely on the postmaster's having
I would try to use more than one segment for buffer pool if
max seg size is not enough for all buffers.
> attached the segment, so that all backends will see it at the same
> location in their address space, which would let us get rid of the
> MAKE_OFFSET/MAKE_PTR cruft. But getting the full benefit would
> require cleaning up a lot of code, and it just doesn't seem like
> a high-priority task. I'm also a little worried that we'd be
> sacrificing portability --- some day we might be glad that we can
> move those segments around...
We can't. MAKE_OFFSET/MAKE_PTR was used because of after
fork/exec/shmat backend' ShmemBase was different from
postmaster' one. But we can't move *BufferDescriptors
if some running backend already uses BufferDescriptors.
But I agreed - this is not high-priority task -:)
Vadim
Vadim Mikheev <vadim@krs.ru> writes:
>> I don't think it's worth messing with either. It'd be nice for code
>> beautification purposes to (a) combine the three shared-mem segments
>> we currently have into one, and (b) rely on the postmaster's having
> I would try to use more than one segment for buffer pool if
> max seg size is not enough for all buffers.
Ah, that would be a nice end-run around kernels with small SHMMAX,
wouldn't it?
>> I'm also a little worried that we'd be
>> sacrificing portability --- some day we might be glad that we can
>> move those segments around...
> We can't. MAKE_OFFSET/MAKE_PTR was used because of after
> fork/exec/shmat backend' ShmemBase was different from
> postmaster' one. But we can't move *BufferDescriptors
> if some running backend already uses BufferDescriptors.
Right, we can't relocate a segment within the address space of
an already-running backend. What I meant was that being able
to put it at different addresses in different backends might be
needed again someday, even though right now we don't need it.
> But I agreed - this is not high-priority task -:)
Yup. Plenty of high-priority ones, too...
regards, tom lane
Vadim Mikheev <vadim@krs.ru> writes:
> Tom Lane wrote:
>> Have I got a deal for you ;-). I have uncommitted changes that add
>> a pointer (SHMEM_OFFSET that is) to each backend's PROC struct into
>> the per-backend info array that already existed in shmem.c.
> Nice. I have new member for PROC that should be searched
> sometime -:)
OK, cool. Easy enough to add now. The reason I did this was that
I added to PROC the OID of the database the backend is attached to,
so that I could make a routine to tell whether any running backends
are connected to a given database. I couldn't quite stomach adding
yet another ShmemIndex-traverser to shmem.c, so...
(I'm sure you can see already where I'm going with that: DESTROY
DATABASE now refuses to destroy a database that has running backends.
I got burnt that way once too often. The interlock against
halfway-started backends was a tad tricky, but I think it works.)
regards, tom lane