Thread: Path to enable a module to change the stack_base_ptr
Here is a patch that will enable a module to change the stack_base_ptr temporarilly during a call. A background discussion can be found here: http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg64586.html Regards, Thomas Hallgren Index: src/backend/tcop/postgres.c =================================================================== RCS file: /projects/cvsroot/pgsql/src/backend/tcop/postgres.c,v retrieving revision 1.463 diff -u -r1.463 postgres.c --- src/backend/tcop/postgres.c 26 Sep 2005 15:51:12 -0000 1.463 +++ src/backend/tcop/postgres.c 2 Oct 2005 07:16:14 -0000 @@ -2285,6 +2285,27 @@ } } +/* + * set_stack_base_ptr: Assign a new stack base pointer + * + * Modules that call on the backend code from a thread different then the + * main thread can use this function to temporarilly swap the stack base + * pointer during the call. + * + * NOTE! This does in no way mean that the backend code is thread-safe. It + * cannot run multiple threads simultaniously. The caller must ensure that + * only one thread at a time is calling, that the call is encapsulated in + * a PG_TRY/PG_CATCH, and that signal handlers are installed to cater for + * signals in other threads then main. + */ +char* +set_stack_base_ptr(char* new_base_ptr) +{ + char* old_base_ptr = stack_base_ptr; + stack_base_ptr = new_base_ptr; + return old_base_ptr; +} + /* GUC assign hook to update max_stack_depth_bytes from max_stack_depth */ bool assign_max_stack_depth(int newval, bool doit, GucSource source) Index: src/include/miscadmin.h =================================================================== RCS file: /projects/cvsroot/pgsql/src/include/miscadmin.h,v retrieving revision 1.179 diff -u -r1.179 miscadmin.h --- src/include/miscadmin.h 17 Aug 2005 22:14:34 -0000 1.179 +++ src/include/miscadmin.h 2 Oct 2005 07:16:15 -0000 @@ -216,6 +216,7 @@ /* in tcop/postgres.c */ extern void check_stack_depth(void); +extern char* set_stack_base_ptr(char* new_base_ptr); /*****************************************************************************
Thomas Hallgren <thhal@mailblocks.com> writes:
> Here is a patch that will enable a module to change the stack_base_ptr
> temporarilly during a call.
I'm not really in favor of this ... I think you are trying to make the
backend do something that will never work reliably.
If we were to try to support this, I'd prefer to just make
stack_base_ptr non static, rather than add overhead code.
regards, tom lane
Tom Lane wrote: >Thomas Hallgren <thhal@mailblocks.com> writes: > > >>Here is a patch that will enable a module to change the stack_base_ptr >>temporarilly during a call. >> >> > >I'm not really in favor of this ... I think you are trying to make the >backend do something that will never work reliably. > >If we were to try to support this, I'd prefer to just make >stack_base_ptr non static, rather than add overhead code. > > regards, tom lane > > Sure, that works for me. Regards, Thomas Hallgren
Thomas Hallgren wrote: > Tom Lane wrote: > > >Thomas Hallgren <thhal@mailblocks.com> writes: > > > > > >>Here is a patch that will enable a module to change the stack_base_ptr > >>temporarilly during a call. > >> > >> > > > >I'm not really in favor of this ... I think you are trying to make the > >backend do something that will never work reliably. > > > >If we were to try to support this, I'd prefer to just make > >stack_base_ptr non static, rather than add overhead code. > > > > regards, tom lane > > > > > Sure, that works for me. Do we want to make this change for 8.1? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Tom Lane wrote:
>>> I'm not really in favor of this ... I think you are trying to make the
>>> backend do something that will never work reliably.
> Do we want to make this change for 8.1?
I don't want to do it at all. The justification given is to allow the
backend to support multithreading introduced by an add-on library, which
is a hopeless cause. Removing "static" from that variable declaration
is surely a cheap enough change, but what about the next request, and
the one after that?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Tom Lane wrote: > >>> I'm not really in favor of this ... I think you are trying to make the > >>> backend do something that will never work reliably. > > > Do we want to make this change for 8.1? > > I don't want to do it at all. The justification given is to allow the > backend to support multithreading introduced by an add-on library, which > is a hopeless cause. Removing "static" from that variable declaration > is surely a cheap enough change, but what about the next request, and > the one after that? Well, I have not seen the next request yet, but it seems harmless for a useful extension to the database, namely PL/Java. I do believe this is one of the reasons PL/J took a different approach, though. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >>> Tom Lane wrote: >>> >>>> I'm not really in favor of this ... I think you are trying to make the >>>> backend do something that will never work reliably. >>>> > > >> Do we want to make this change for 8.1? >> > > I don't want to do it at all. The justification given is to allow the > backend to support multithreading introduced by an add-on library, which > is a hopeless cause. Removing "static" from that variable declaration > is surely a cheap enough change, but what about the next request, and > the one after that? > Tom, I don't request that the backend should support multiple threads simultaneously. It's one thread at a time. I can't think of a "next request" in this direction. I'm very aware that the backend is single-threaded and that you have no intention to change that. Neither do I. Having the stack_base public is actually useful for another purpose also. It can allow you to make assertions that check if an abitrary pointer is 'on stack' or not. The MemoryContextContains() could be made safer too by just returning false when the given pointer is between the stack_base and the current stack_pointer. Perhaps that could be added to the patch? Regards, Thomas Hallgren