Thread: Tuplestore should remember the memory context it's created in

With regards to this bug report:
http://archives.postgresql.org/pgsql-bugs/2009-12/msg00241.php

I think we should change tuplestore code so that callers of
tuplestore_put* don't need to switch to the correct memory context (and
resource owner, after this patch) before call. Instead,
tuplestore_begin_heap() should memorize the context and resource owner
used to create the tuplestore, and use that in tuplestore_put*
functions. AFAICS it is always a bug to be in a different memory context
in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
robust to not put the burden on the callers.

Patch against CVS HEAD to do that and fix the reported bug attached. Now
that the tuplestore_put* switches to the right memory context, we could
remove that from all the callers, but this patch only does it for pl_exec.c.

Thoughts?

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
? GNUmakefile
? config.log
? config.status
? gin-splay-1.patch
? gin-splay-2.patch
? gin-splay-3.patch
? md-1.c
? md-1.patch
? temp-file-resowner-2.patch
? contrib/fuzzystrmatch/.deps
? contrib/fuzzystrmatch/fuzzystrmatch.sql
? contrib/pg_standby/.deps
? contrib/pg_standby/pg_standby
? contrib/pgbench/fsynctest
? contrib/pgbench/fsynctest.c
? contrib/pgbench/fsynctestfile
? contrib/spi/.deps
? src/Makefile.global
? src/backend/aaa.patch
? src/backend/postgres
? src/backend/access/common/.deps
? src/backend/access/gin/.deps
? src/backend/access/gist/.deps
? src/backend/access/hash/.deps
? src/backend/access/heap/.deps
? src/backend/access/index/.deps
? src/backend/access/nbtree/.deps
? src/backend/access/transam/.deps
? src/backend/bootstrap/.deps
? src/backend/catalog/.deps
? src/backend/catalog/postgres.bki
? src/backend/catalog/postgres.description
? src/backend/catalog/postgres.shdescription
? src/backend/commands/.deps
? src/backend/executor/.deps
? src/backend/foreign/.deps
? src/backend/foreign/dummy/.deps
? src/backend/foreign/postgresql/.deps
? src/backend/lib/.deps
? src/backend/libpq/.deps
? src/backend/main/.deps
? src/backend/nodes/.deps
? src/backend/optimizer/geqo/.deps
? src/backend/optimizer/path/.deps
? src/backend/optimizer/plan/.deps
? src/backend/optimizer/prep/.deps
? src/backend/optimizer/util/.deps
? src/backend/parser/.deps
? src/backend/po/af.mo
? src/backend/po/cs.mo
? src/backend/po/de.mo
? src/backend/po/es.mo
? src/backend/po/fr.mo
? src/backend/po/hr.mo
? src/backend/po/hu.mo
? src/backend/po/it.mo
? src/backend/po/ja.mo
? src/backend/po/ko.mo
? src/backend/po/nb.mo
? src/backend/po/nl.mo
? src/backend/po/pl.mo
? src/backend/po/pt_BR.mo
? src/backend/po/ro.mo
? src/backend/po/ru.mo
? src/backend/po/sk.mo
? src/backend/po/sl.mo
? src/backend/po/sv.mo
? src/backend/po/tr.mo
? src/backend/po/zh_CN.mo
? src/backend/po/zh_TW.mo
? src/backend/port/.deps
? src/backend/postmaster/.deps
? src/backend/regex/.deps
? src/backend/rewrite/.deps
? src/backend/snowball/.deps
? src/backend/snowball/snowball_create.sql
? src/backend/storage/buffer/.deps
? src/backend/storage/file/.deps
? src/backend/storage/freespace/.deps
? src/backend/storage/ipc/.deps
? src/backend/storage/large_object/.deps
? src/backend/storage/lmgr/.deps
? src/backend/storage/page/.deps
? src/backend/storage/smgr/.deps
? src/backend/tcop/.deps
? src/backend/tsearch/.deps
? src/backend/utils/.deps
? src/backend/utils/probes.h
? src/backend/utils/adt/.deps
? src/backend/utils/cache/.deps
? src/backend/utils/error/.deps
? src/backend/utils/fmgr/.deps
? src/backend/utils/hash/.deps
? src/backend/utils/init/.deps
? src/backend/utils/mb/.deps
? src/backend/utils/mb/Unicode/BIG5.TXT
? src/backend/utils/mb/Unicode/CP950.TXT
? src/backend/utils/mb/conversion_procs/conversion_create.sql
? src/backend/utils/mb/conversion_procs/ascii_and_mic/.deps
? src/backend/utils/mb/conversion_procs/cyrillic_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc2004_sjis2004/.deps
? src/backend/utils/mb/conversion_procs/euc_cn_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc_jis_2004_and_shift_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/.deps
? src/backend/utils/mb/conversion_procs/euc_kr_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc_tw_and_big5/.deps
? src/backend/utils/mb/conversion_procs/latin2_and_win1250/.deps
? src/backend/utils/mb/conversion_procs/latin_and_mic/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_ascii/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_big5/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_cyrillic/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_cn/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_jp/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_kr/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_tw/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_gb18030/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_gbk/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_iso8859/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_johab/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_shift_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_sjis/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_sjis2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_uhc/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_win/.deps
? src/backend/utils/misc/.deps
? src/backend/utils/mmgr/.deps
? src/backend/utils/resowner/.deps
? src/backend/utils/sort/.deps
? src/backend/utils/time/.deps
? src/bin/initdb/.deps
? src/bin/initdb/initdb
? src/bin/initdb/po/cs.mo
? src/bin/initdb/po/de.mo
? src/bin/initdb/po/es.mo
? src/bin/initdb/po/fr.mo
? src/bin/initdb/po/it.mo
? src/bin/initdb/po/ja.mo
? src/bin/initdb/po/ko.mo
? src/bin/initdb/po/pl.mo
? src/bin/initdb/po/pt_BR.mo
? src/bin/initdb/po/ro.mo
? src/bin/initdb/po/ru.mo
? src/bin/initdb/po/sk.mo
? src/bin/initdb/po/sl.mo
? src/bin/initdb/po/sv.mo
? src/bin/initdb/po/ta.mo
? src/bin/initdb/po/tr.mo
? src/bin/initdb/po/zh_CN.mo
? src/bin/initdb/po/zh_TW.mo
? src/bin/pg_config/.deps
? src/bin/pg_config/pg_config
? src/bin/pg_config/po/cs.mo
? src/bin/pg_config/po/de.mo
? src/bin/pg_config/po/es.mo
? src/bin/pg_config/po/fr.mo
? src/bin/pg_config/po/it.mo
? src/bin/pg_config/po/ja.mo
? src/bin/pg_config/po/ko.mo
? src/bin/pg_config/po/nb.mo
? src/bin/pg_config/po/pl.mo
? src/bin/pg_config/po/pt_BR.mo
? src/bin/pg_config/po/ro.mo
? src/bin/pg_config/po/ru.mo
? src/bin/pg_config/po/sl.mo
? src/bin/pg_config/po/sv.mo
? src/bin/pg_config/po/ta.mo
? src/bin/pg_config/po/tr.mo
? src/bin/pg_config/po/zh_CN.mo
? src/bin/pg_config/po/zh_TW.mo
? src/bin/pg_controldata/.deps
? src/bin/pg_controldata/pg_controldata
? src/bin/pg_controldata/po/cs.mo
? src/bin/pg_controldata/po/de.mo
? src/bin/pg_controldata/po/es.mo
? src/bin/pg_controldata/po/fa.mo
? src/bin/pg_controldata/po/fr.mo
? src/bin/pg_controldata/po/hu.mo
? src/bin/pg_controldata/po/it.mo
? src/bin/pg_controldata/po/ja.mo
? src/bin/pg_controldata/po/ko.mo
? src/bin/pg_controldata/po/nb.mo
? src/bin/pg_controldata/po/pl.mo
? src/bin/pg_controldata/po/pt_BR.mo
? src/bin/pg_controldata/po/ro.mo
? src/bin/pg_controldata/po/ru.mo
? src/bin/pg_controldata/po/sk.mo
? src/bin/pg_controldata/po/sl.mo
? src/bin/pg_controldata/po/sv.mo
? src/bin/pg_controldata/po/ta.mo
? src/bin/pg_controldata/po/tr.mo
? src/bin/pg_controldata/po/zh_CN.mo
? src/bin/pg_controldata/po/zh_TW.mo
? src/bin/pg_ctl/.deps
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_ctl/po/cs.mo
? src/bin/pg_ctl/po/de.mo
? src/bin/pg_ctl/po/es.mo
? src/bin/pg_ctl/po/fr.mo
? src/bin/pg_ctl/po/it.mo
? src/bin/pg_ctl/po/ja.mo
? src/bin/pg_ctl/po/ko.mo
? src/bin/pg_ctl/po/pt_BR.mo
? src/bin/pg_ctl/po/ro.mo
? src/bin/pg_ctl/po/ru.mo
? src/bin/pg_ctl/po/sk.mo
? src/bin/pg_ctl/po/sl.mo
? src/bin/pg_ctl/po/sv.mo
? src/bin/pg_ctl/po/ta.mo
? src/bin/pg_ctl/po/tr.mo
? src/bin/pg_ctl/po/zh_CN.mo
? src/bin/pg_ctl/po/zh_TW.mo
? src/bin/pg_dump/.deps
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/po/cs.mo
? src/bin/pg_dump/po/de.mo
? src/bin/pg_dump/po/es.mo
? src/bin/pg_dump/po/fr.mo
? src/bin/pg_dump/po/it.mo
? src/bin/pg_dump/po/ja.mo
? src/bin/pg_dump/po/ko.mo
? src/bin/pg_dump/po/nb.mo
? src/bin/pg_dump/po/pt_BR.mo
? src/bin/pg_dump/po/ro.mo
? src/bin/pg_dump/po/ru.mo
? src/bin/pg_dump/po/sk.mo
? src/bin/pg_dump/po/sl.mo
? src/bin/pg_dump/po/sv.mo
? src/bin/pg_dump/po/tr.mo
? src/bin/pg_dump/po/zh_CN.mo
? src/bin/pg_dump/po/zh_TW.mo
? src/bin/pg_resetxlog/.deps
? src/bin/pg_resetxlog/pg_resetxlog
? src/bin/pg_resetxlog/po/cs.mo
? src/bin/pg_resetxlog/po/de.mo
? src/bin/pg_resetxlog/po/es.mo
? src/bin/pg_resetxlog/po/fr.mo
? src/bin/pg_resetxlog/po/hu.mo
? src/bin/pg_resetxlog/po/it.mo
? src/bin/pg_resetxlog/po/ja.mo
? src/bin/pg_resetxlog/po/ko.mo
? src/bin/pg_resetxlog/po/nb.mo
? src/bin/pg_resetxlog/po/pt_BR.mo
? src/bin/pg_resetxlog/po/ro.mo
? src/bin/pg_resetxlog/po/ru.mo
? src/bin/pg_resetxlog/po/sk.mo
? src/bin/pg_resetxlog/po/sl.mo
? src/bin/pg_resetxlog/po/sv.mo
? src/bin/pg_resetxlog/po/ta.mo
? src/bin/pg_resetxlog/po/tr.mo
? src/bin/pg_resetxlog/po/zh_CN.mo
? src/bin/pg_resetxlog/po/zh_TW.mo
? src/bin/psql/.deps
? src/bin/psql/psql
? src/bin/psql/po/cs.mo
? src/bin/psql/po/de.mo
? src/bin/psql/po/es.mo
? src/bin/psql/po/fa.mo
? src/bin/psql/po/fr.mo
? src/bin/psql/po/hu.mo
? src/bin/psql/po/it.mo
? src/bin/psql/po/ja.mo
? src/bin/psql/po/ko.mo
? src/bin/psql/po/nb.mo
? src/bin/psql/po/pt_BR.mo
? src/bin/psql/po/ro.mo
? src/bin/psql/po/ru.mo
? src/bin/psql/po/sk.mo
? src/bin/psql/po/sl.mo
? src/bin/psql/po/sv.mo
? src/bin/psql/po/tr.mo
? src/bin/psql/po/zh_CN.mo
? src/bin/psql/po/zh_TW.mo
? src/bin/scripts/.deps
? src/bin/scripts/clusterdb
? src/bin/scripts/createdb
? src/bin/scripts/createlang
? src/bin/scripts/createuser
? src/bin/scripts/dropdb
? src/bin/scripts/droplang
? src/bin/scripts/dropuser
? src/bin/scripts/reindexdb
? src/bin/scripts/vacuumdb
? src/bin/scripts/po/cs.mo
? src/bin/scripts/po/de.mo
? src/bin/scripts/po/es.mo
? src/bin/scripts/po/fr.mo
? src/bin/scripts/po/it.mo
? src/bin/scripts/po/ja.mo
? src/bin/scripts/po/ko.mo
? src/bin/scripts/po/pt_BR.mo
? src/bin/scripts/po/ro.mo
? src/bin/scripts/po/ru.mo
? src/bin/scripts/po/sk.mo
? src/bin/scripts/po/sl.mo
? src/bin/scripts/po/sv.mo
? src/bin/scripts/po/ta.mo
? src/bin/scripts/po/tr.mo
? src/bin/scripts/po/zh_CN.mo
? src/bin/scripts/po/zh_TW.mo
? src/include/pg_config.h
? src/include/stamp-h
? src/interfaces/ecpg/compatlib/.deps
? src/interfaces/ecpg/compatlib/exports.list
? src/interfaces/ecpg/compatlib/libecpg_compat.so.3.1
? src/interfaces/ecpg/compatlib/libecpg_compat.so.3.2
? src/interfaces/ecpg/ecpglib/.deps
? src/interfaces/ecpg/ecpglib/exports.list
? src/interfaces/ecpg/ecpglib/libecpg.so.6.1
? src/interfaces/ecpg/ecpglib/libecpg.so.6.2
? src/interfaces/ecpg/ecpglib/po/de.mo
? src/interfaces/ecpg/ecpglib/po/es.mo
? src/interfaces/ecpg/ecpglib/po/fr.mo
? src/interfaces/ecpg/ecpglib/po/it.mo
? src/interfaces/ecpg/ecpglib/po/ja.mo
? src/interfaces/ecpg/ecpglib/po/pt_BR.mo
? src/interfaces/ecpg/ecpglib/po/tr.mo
? src/interfaces/ecpg/include/ecpg_config.h
? src/interfaces/ecpg/include/stamp-h
? src/interfaces/ecpg/pgtypeslib/.deps
? src/interfaces/ecpg/pgtypeslib/exports.list
? src/interfaces/ecpg/pgtypeslib/libpgtypes.so.3.1
? src/interfaces/ecpg/pgtypeslib/libpgtypes.so.3.2
? src/interfaces/ecpg/preproc/.deps
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/ecpg/preproc/po/de.mo
? src/interfaces/ecpg/preproc/po/es.mo
? src/interfaces/ecpg/preproc/po/fr.mo
? src/interfaces/ecpg/preproc/po/it.mo
? src/interfaces/ecpg/preproc/po/ja.mo
? src/interfaces/ecpg/preproc/po/pt_BR.mo
? src/interfaces/ecpg/preproc/po/tr.mo
? src/interfaces/libpq/.deps
? src/interfaces/libpq/exports.list
? src/interfaces/libpq/libpq.so.5.2
? src/interfaces/libpq/libpq.so.5.3
? src/interfaces/libpq/po/af.mo
? src/interfaces/libpq/po/cs.mo
? src/interfaces/libpq/po/de.mo
? src/interfaces/libpq/po/es.mo
? src/interfaces/libpq/po/fr.mo
? src/interfaces/libpq/po/hr.mo
? src/interfaces/libpq/po/it.mo
? src/interfaces/libpq/po/ja.mo
? src/interfaces/libpq/po/ko.mo
? src/interfaces/libpq/po/nb.mo
? src/interfaces/libpq/po/pl.mo
? src/interfaces/libpq/po/pt_BR.mo
? src/interfaces/libpq/po/ru.mo
? src/interfaces/libpq/po/sk.mo
? src/interfaces/libpq/po/sl.mo
? src/interfaces/libpq/po/sv.mo
? src/interfaces/libpq/po/ta.mo
? src/interfaces/libpq/po/tr.mo
? src/interfaces/libpq/po/zh_CN.mo
? src/interfaces/libpq/po/zh_TW.mo
? src/pl/plperl/.deps
? src/pl/plperl/SPI.c
? src/pl/plperl/results
? src/pl/plperl/po/de.mo
? src/pl/plperl/po/es.mo
? src/pl/plperl/po/fr.mo
? src/pl/plperl/po/it.mo
? src/pl/plperl/po/ja.mo
? src/pl/plperl/po/pt_BR.mo
? src/pl/plperl/po/tr.mo
? src/pl/plpgsql/src/.deps
? src/pl/plpgsql/src/pl_scan.c
? src/pl/plpgsql/src/po/de.mo
? src/pl/plpgsql/src/po/es.mo
? src/pl/plpgsql/src/po/fr.mo
? src/pl/plpgsql/src/po/it.mo
? src/pl/plpgsql/src/po/ja.mo
? src/pl/plpgsql/src/po/ro.mo
? src/pl/plpgsql/src/po/tr.mo
? src/port/.deps
? src/port/pg_config_paths.h
? src/test/regress/.deps
? src/test/regress/log
? src/test/regress/pg_regress
? src/test/regress/results
? src/test/regress/testtablespace
? src/test/regress/tmp_check
? src/test/regress/expected/constraints.out
? src/test/regress/expected/copy.out
? src/test/regress/expected/create_function_1.out
? src/test/regress/expected/create_function_2.out
? src/test/regress/expected/largeobject.out
? src/test/regress/expected/largeobject_1.out
? src/test/regress/expected/misc.out
? src/test/regress/expected/tablespace.out
? src/test/regress/sql/constraints.sql
? src/test/regress/sql/copy.sql
? src/test/regress/sql/create_function_1.sql
? src/test/regress/sql/create_function_2.sql
? src/test/regress/sql/largeobject.sql
? src/test/regress/sql/misc.sql
? src/test/regress/sql/tablespace.sql
? src/timezone/.deps
? src/timezone/zic
Index: src/backend/utils/sort/tuplestore.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/sort/tuplestore.c,v
retrieving revision 1.48
diff -c -r1.48 tuplestore.c
*** src/backend/utils/sort/tuplestore.c    11 Jun 2009 14:49:06 -0000    1.48
--- src/backend/utils/sort/tuplestore.c    22 Dec 2009 11:39:11 -0000
***************
*** 58,63 ****
--- 58,64 ----
  #include "executor/executor.h"
  #include "storage/buffile.h"
  #include "utils/memutils.h"
+ #include "utils/resowner.h"
  #include "utils/tuplestore.h"


***************
*** 105,110 ****
--- 106,113 ----
      bool        truncated;        /* tuplestore_trim has removed tuples? */
      long        availMem;        /* remaining memory available, in bytes */
      BufFile    *myfile;            /* underlying file, or NULL if none */
+     MemoryContext context;        /* memory context for holding tuples */
+     ResourceOwner resowner;

      /*
       * These function pointers decouple the routines that must know what kind
***************
*** 246,251 ****
--- 249,256 ----
      state->truncated = false;
      state->availMem = maxKBytes * 1024L;
      state->myfile = NULL;
+     state->context = CurrentMemoryContext;
+     state->resowner = CurrentResourceOwner;

      state->memtupcount = 0;
      state->memtupsize = 1024;    /* initial guess */
***************
*** 533,538 ****
--- 538,546 ----
                          TupleTableSlot *slot)
  {
      MinimalTuple tuple;
+     MemoryContext oldcxt;
+
+     oldcxt = MemoryContextSwitchTo(state->context);

      /*
       * Form a MinimalTuple in working memory
***************
*** 541,546 ****
--- 549,556 ----
      USEMEM(state, GetMemoryChunkSpace(tuple));

      tuplestore_puttuple_common(state, (void *) tuple);
+
+     MemoryContextSwitchTo(oldcxt);
  }

  /*
***************
*** 550,561 ****
--- 560,575 ----
  void
  tuplestore_puttuple(Tuplestorestate *state, HeapTuple tuple)
  {
+     MemoryContext oldcxt = MemoryContextSwitchTo(state->context);
+
      /*
       * Copy the tuple.    (Must do this even in WRITEFILE case.)
       */
      tuple = COPYTUP(state, tuple);

      tuplestore_puttuple_common(state, (void *) tuple);
+
+     MemoryContextSwitchTo(oldcxt);
  }

  /*
***************
*** 568,577 ****
--- 582,596 ----
                       Datum *values, bool *isnull)
  {
      MinimalTuple tuple;
+     MemoryContext oldcxt;
+
+     oldcxt = MemoryContextSwitchTo(state->context);

      tuple = heap_form_minimal_tuple(tdesc, values, isnull);

      tuplestore_puttuple_common(state, (void *) tuple);
+
+     MemoryContextSwitchTo(oldcxt);
  }

  static void
***************
*** 579,584 ****
--- 598,604 ----
  {
      TSReadPointer *readptr;
      int            i;
+     ResourceOwner oldowner;

      switch (state->status)
      {
***************
*** 635,641 ****
--- 655,664 ----
               * the temp file(s) are created in suitable temp tablespaces.
               */
              PrepareTempTablespaces();
+             oldowner = CurrentResourceOwner;
+             CurrentResourceOwner = state->resowner;
              state->myfile = BufFileCreateTemp(state->interXact);
+             CurrentResourceOwner = oldowner;

              /*
               * Freeze the decision about whether trailing length words will be
Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.251
diff -c -r1.251 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c    9 Nov 2009 00:26:55 -0000    1.251
--- src/pl/plpgsql/src/pl_exec.c    22 Dec 2009 11:39:12 -0000
***************
*** 2172,2178 ****
  {
      TupleDesc    tupdesc;
      int            natts;
-     MemoryContext oldcxt;
      HeapTuple    tuple = NULL;
      bool        free_tuple = false;

--- 2172,2177 ----
***************
*** 2212,2221 ****
                                                  tupdesc->attrs[0]->atttypmod,
                                                      isNull);

-                     oldcxt = MemoryContextSwitchTo(estate->tuple_store_cxt);
                      tuplestore_putvalues(estate->tuple_store, tupdesc,
                                           &retval, &isNull);
-                     MemoryContextSwitchTo(oldcxt);
                  }
                  break;

--- 2211,2218 ----
***************
*** 2285,2294 ****
                                          tupdesc->attrs[0]->atttypmod,
                                          isNull);

-         oldcxt = MemoryContextSwitchTo(estate->tuple_store_cxt);
          tuplestore_putvalues(estate->tuple_store, tupdesc,
                               &retval, &isNull);
-         MemoryContextSwitchTo(oldcxt);

          exec_eval_cleanup(estate);
      }
--- 2282,2289 ----
***************
*** 2301,2309 ****

      if (HeapTupleIsValid(tuple))
      {
-         oldcxt = MemoryContextSwitchTo(estate->tuple_store_cxt);
          tuplestore_puttuple(estate->tuple_store, tuple);
-         MemoryContextSwitchTo(oldcxt);

          if (free_tuple)
              heap_freetuple(tuple);
--- 2296,2302 ----
***************
*** 2353,2366 ****

      while (true)
      {
-         MemoryContext old_cxt;
          int            i;

          SPI_cursor_fetch(portal, true, 50);
          if (SPI_processed == 0)
              break;

-         old_cxt = MemoryContextSwitchTo(estate->tuple_store_cxt);
          for (i = 0; i < SPI_processed; i++)
          {
              HeapTuple    tuple = SPI_tuptable->vals[i];
--- 2346,2357 ----
***************
*** 2372,2378 ****
                  heap_freetuple(tuple);
              processed++;
          }
-         MemoryContextSwitchTo(old_cxt);

          SPI_freetuptable(SPI_tuptable);
      }
--- 2363,2368 ----
***************
*** 2394,2399 ****
--- 2384,2390 ----
  {
      ReturnSetInfo *rsi = estate->rsi;
      MemoryContext oldcxt;
+     ResourceOwner oldowner;

      /*
       * Check caller can handle a set result in the way we want
***************
*** 2408,2416 ****
--- 2399,2410 ----
      estate->tuple_store_cxt = rsi->econtext->ecxt_per_query_memory;

      oldcxt = MemoryContextSwitchTo(estate->tuple_store_cxt);
+     oldowner = CurrentResourceOwner;
+     CurrentResourceOwner = estate->tuple_store_owner;
      estate->tuple_store =
          tuplestore_begin_heap(rsi->allowedModes & SFRM_Materialize_Random,
                                false, work_mem);
+     CurrentResourceOwner = oldowner;
      MemoryContextSwitchTo(oldcxt);

      estate->rettupdesc = rsi->expectedDesc;
***************
*** 2636,2641 ****
--- 2630,2636 ----

      estate->tuple_store = NULL;
      estate->tuple_store_cxt = NULL;
+     estate->tuple_store_owner = CurrentResourceOwner;
      estate->rsi = rsi;

      estate->found_varno = func->found_varno;
Index: src/pl/plpgsql/src/plpgsql.h
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/plpgsql.h,v
retrieving revision 1.125
diff -c -r1.125 plpgsql.h
*** src/pl/plpgsql/src/plpgsql.h    13 Nov 2009 22:43:42 -0000    1.125
--- src/pl/plpgsql/src/plpgsql.h    22 Dec 2009 11:39:12 -0000
***************
*** 701,706 ****
--- 701,707 ----

      Tuplestorestate *tuple_store;        /* SRFs accumulate results here */
      MemoryContext tuple_store_cxt;
+     ResourceOwner tuple_store_owner;
      ReturnSetInfo *rsi;

      int            found_varno;

On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
...
> AFAICS it is always a bug to be in a different memory context
> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
> robust to not put the burden on the callers.
> ...
> Patch against CVS HEAD to do that and fix the reported bug attached. Now
> that the tuplestore_put* switches to the right memory context, we could
> remove that from all the callers, but this patch only does it for pl_exec.c.
>
> Thoughts?

I thought there were comments specifically explaining why it was done
that way but I don't recall what they said. Perhaps it was a
performance concern since it's going to happen for every tuple put in
the tuplestore and usually you'll just be in the same memory context
anyways. It would certainly be a lot less confusing the way you
describe though.

-- 
greg

On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> With regards to this bug report:
> http://archives.postgresql.org/pgsql-bugs/2009-12/msg00241.php

Hm, do we have any build farm members running with work_mem set to the minimum?

-- 
greg

Greg Stark <gsstark@mit.edu> writes:
> On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> AFAICS it is always a bug to be in a different memory context
>> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
>> robust to not put the burden on the callers.

> I thought there were comments specifically explaining why it was done
> that way but I don't recall what they said.

I think it was just a performance optimization.  It's probably not
measurable though; even in the in-memory case there's at least a palloc
inside the put() function, no?
        regards, tom lane

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Patch against CVS HEAD to do that and fix the reported bug attached. Now
> that the tuplestore_put* switches to the right memory context, we could
> remove that from all the callers, but this patch only does it for pl_exec.c.

BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
is necessary/appropriate.  Under what circumstances would that be a good
idea?
        regards, tom lane

Tom Lane wrote:
> Greg Stark <gsstark@mit.edu> writes:
>> On Tue, Dec 22, 2009 at 11:45 AM, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>> AFAICS it is always a bug to be in a different memory context
>>> in tuplestore_put* than in tuplestore_begin_heap(), so it would be more
>>> robust to not put the burden on the callers.
> 
>> I thought there were comments specifically explaining why it was done
>> that way but I don't recall what they said.
> 
> I think it was just a performance optimization.  It's probably not
> measurable though; even in the in-memory case there's at least a palloc
> inside the put() function, no?

Yes. And many of the callers do the memory context switching dance anyway.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> I think it was just a performance optimization.  It's probably not
>> measurable though; even in the in-memory case there's at least a palloc
>> inside the put() function, no?

> Yes. And many of the callers do the memory context switching dance anyway.

Yeah, I was just noticing that.  We should go around and clean those up
if we apply this change.

Looking at the CVS history, I think the reason tuplestore doesn't do its
own memory context switch is that it was cloned from tuplesort, which
didn't either at the time.  But several years ago we changed tuplesort
to be safer about this (it actually keeps its own memory context now),
so it's just inconsistent that tuplestore still exposes the risk.

The ownership business is another story though.  tuplesort doesn't
make any attempt to defend itself against resource-owner changes.  If we
need this for tuplestore I bet we need it for tuplesort too; but do we?
        regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> Patch against CVS HEAD to do that and fix the reported bug attached. Now
>> that the tuplestore_put* switches to the right memory context, we could
>> remove that from all the callers, but this patch only does it for pl_exec.c.
> 
> BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
> is necessary/appropriate.  Under what circumstances would that be a good
> idea?

A PL/pgSQL normally runs in the whatever resource owner is current when
the function is called. When we allocate the tuplestore for return
tuples, it's associated with the current resource owner.

But if you have an exception-block, we start a new subtransaction and
switch to the subtransaction resource owner. If you have a RETURN
NEXT/QUERY in the block, the tuplestore (or the temporary file backing
it, to be precise) is initialized into the subtransaction resource
owner, which is released at subtransaction commit. The subtransaction
resource owner is not the right owner for the tuplestore holding return
tuples.

We already take care to use the right memory context for the tuplestore,
but now that temp files are associated with resource owners, we need to
use the right resource owner as well.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> BTW, I'm not convinced that the owner-switchery you added to pl_exec.c
>> is necessary/appropriate.  Under what circumstances would that be a good
>> idea?

> A PL/pgSQL normally runs in the whatever resource owner is current when
> the function is called. When we allocate the tuplestore for return
> tuples, it's associated with the current resource owner.

> But if you have an exception-block, we start a new subtransaction and
> switch to the subtransaction resource owner. If you have a RETURN
> NEXT/QUERY in the block, the tuplestore (or the temporary file backing
> it, to be precise) is initialized into the subtransaction resource
> owner, which is released at subtransaction commit.

Got it.  So doesn't tuplesort have the same issue?

The patch definitely requires more than zero comments.
        regards, tom lane

Tom Lane wrote:
> Got it.  So doesn't tuplesort have the same issue?

Tuplesort has the same general problem that the caller of puttuple needs
to be in the right resource owner. Which ought to be fixed, especially
since tuplesort doesn't require that for the memory context anymore.

But we don't use tuplesort to return tuples from functions, so it's not
broken in a user-visible way. Unless you can think of another scenario
like that.

> The patch definitely requires more than zero comments.

Sure.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> Got it.  So doesn't tuplesort have the same issue?

> Tuplesort has the same general problem that the caller of puttuple needs
> to be in the right resource owner. Which ought to be fixed, especially
> since tuplesort doesn't require that for the memory context anymore.

> But we don't use tuplesort to return tuples from functions, so it's not
> broken in a user-visible way. Unless you can think of another scenario
> like that.

(1) create a cursor whose plan involves a sort that will spill to disk
(2) enter subtransaction
(3) fetch from cursor (causing the sort to actually happen)
(4) leave subtransaction
(5) fetch some more from cursor

Too busy to develop a test case right now, but ISTM it ought to fail.
        regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> But we don't use tuplesort to return tuples from functions, so it's not
>> broken in a user-visible way. Unless you can think of another scenario
>> like that.
> 
> (1) create a cursor whose plan involves a sort that will spill to disk
> (2) enter subtransaction
> (3) fetch from cursor (causing the sort to actually happen)
> (4) leave subtransaction
> (5) fetch some more from cursor
> 
> Too busy to develop a test case right now, but ISTM it ought to fail.

That was exactly the case that we originally fixed, that caused this
PL/pgSQL issue. It works because cursors run within the portal
ResourceOwner.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com