Thread: Path to enable a module to change the stack_base_ptr

Path to enable a module to change the stack_base_ptr

From
Thomas Hallgren
Date:
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);


 /*****************************************************************************

Re: Path to enable a module to change the stack_base_ptr

From
Tom Lane
Date:
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

Re: Path to enable a module to change the stack_base_ptr

From
Thomas Hallgren
Date:
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



Re: Path to enable a module to change the stack_base_ptr

From
Bruce Momjian
Date:
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

Re: Path to enable a module to change the stack_base_ptr

From
Tom Lane
Date:
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

Re: Path to enable a module to change the stack_base_ptr

From
Bruce Momjian
Date:
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

Re: Path to enable a module to change the stack_base_ptr

From
Thomas Hallgren
Date:
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