Re: [HACKERS] row literal problem

2012-07-20 Thread Tom Lane
I wrote:
 I think the way to solve this is to do whatever it takes to get access
 to the subplan targetlist.  We could then do something a bit cleaner
 than what the named-rowtype code is currently doing: if there are
 resjunk columns in the subplan targetlist, use the tlist to create a
 JunkFilter, and then pass the tuples through that.  After that we can
 insist that the tuples don't have any extra columns.

Here's a draft patch for that.  It wasn't quite as ugly as I feared.
A lot of the apparent bulk of the patch is because I chose to split
ExecEvalVar into separate functions for the scalar and whole-row
cases, which seemed appropriate because they now get different
ExprState node types.

regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 0ea21ca5f91a704ccde2c765bd92d0c5af7ead8b..ae7987ee97482e1224912f3e96d5a90eaa1f10f7 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
***
*** 20,26 
   *		ExecProject		- form a new tuple by projecting the given tuple
   *
   *	 NOTES
!  *		The more heavily used ExecEvalExpr routines, such as ExecEvalVar(),
   *		are hotspots. Making these faster will speed up the entire system.
   *
   *		ExecProject() is used to make tuple projections.  Rather then
--- 20,26 
   *		ExecProject		- form a new tuple by projecting the given tuple
   *
   *	 NOTES
!  *		The more heavily used ExecEvalExpr routines, such as ExecEvalScalarVar,
   *		are hotspots. Making these faster will speed up the entire system.
   *
   *		ExecProject() is used to make tuple projections.  Rather then
*** static Datum ExecEvalAggref(AggrefExprSt
*** 68,80 
  static Datum ExecEvalWindowFunc(WindowFuncExprState *wfunc,
     ExprContext *econtext,
     bool *isNull, ExprDoneCond *isDone);
- static Datum ExecEvalVar(ExprState *exprstate, ExprContext *econtext,
- 			bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowVar(ExprState *exprstate, ExprContext *econtext,
  	bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowSlow(ExprState *exprstate, ExprContext *econtext,
  	 bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalConst(ExprState *exprstate, ExprContext *econtext,
  			  bool *isNull, ExprDoneCond *isDone);
--- 68,85 
  static Datum ExecEvalWindowFunc(WindowFuncExprState *wfunc,
     ExprContext *econtext,
     bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalScalarVar2(ExprState *exprstate, ExprContext *econtext,
!    bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowVar(WholeRowVarExprState *wrvstate,
! 	ExprContext *econtext,
  	bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowFast(WholeRowVarExprState *wrvstate,
! 	 ExprContext *econtext,
! 	 bool *isNull, ExprDoneCond *isDone);
! static Datum ExecEvalWholeRowSlow(WholeRowVarExprState *wrvstate,
! 	 ExprContext *econtext,
  	 bool *isNull, ExprDoneCond *isDone);
  static Datum ExecEvalConst(ExprState *exprstate, ExprContext *econtext,
  			  bool *isNull, ExprDoneCond *isDone);
*** ExecEvalWindowFunc(WindowFuncExprState *
*** 553,572 
  }
  
  /* 
!  *		ExecEvalVar
   *
!  *		Returns a Datum whose value is the value of a range
!  *		variable with respect to given expression context.
   *
!  * Note: ExecEvalVar is executed only the first time through in a given plan;
!  * it changes the ExprState's function pointer to pass control directly to
!  * ExecEvalScalarVar, ExecEvalWholeRowVar, or ExecEvalWholeRowSlow after
!  * making one-time checks.
   * 
   */
  static Datum
! ExecEvalVar(ExprState *exprstate, ExprContext *econtext,
! 			bool *isNull, ExprDoneCond *isDone)
  {
  	Var		   *variable = (Var *) exprstate-expr;
  	TupleTableSlot *slot;
--- 558,576 
  }
  
  /* 
!  *		ExecEvalScalarVar
   *
!  *		Returns a Datum whose value is the value of a scalar (not whole-row)
!  *		range variable with respect to given expression context.
   *
!  * Note: ExecEvalScalarVar is executed only the first time through in a given
!  * plan; it changes the ExprState's function pointer to pass control directly
!  * to ExecEvalScalarVar2 after making one-time checks.
   * 
   */
  static Datum
! ExecEvalScalarVar(ExprState *exprstate, ExprContext *econtext,
!   bool *isNull, ExprDoneCond *isDone)
  {
  	Var		   *variable = (Var *) exprstate-expr;
  	TupleTableSlot 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix mapping of PostgreSQL encodings to Python encodings.

2012-07-20 Thread Jan Urbański

On 18/07/12 17:17, Heikki Linnakangas wrote:

On 14.07.2012 17:50, Jan Urbański wrote:

If pg_do_encoding_conversion() throws an error, you don't get a chance
to call Py_DECREF() to release the string. Is that a problem?

If an error occurs in PLy_traceback(), after incrementing
recursion_depth, you don't get a chance to decrement it again. I'm not
sure if the Py* function calls can fail, but at least seemingly trivial
things like initStringInfo() can throw an out-of-memory error.


Of course you're right (on both accounts).

Here's a version with a bunch of PG_TRies thrown in.

Cheers,
Jan
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index c375ac0..9944f72
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** static char *get_source_line(const char
*** 28,33 
--- 28,41 
  
  
  /*
+  * Guard agains re-entrant calls to PLy_traceback, which can happen if
+  * traceback formatting functions raise Python errors.
+  */
+ #define TRACEBACK_RECURSION_LIMIT 2
+ static int	recursion_depth = 0;
+ 
+ 
+ /*
   * Emit a PG error or notice, together with any available info about
   * the current Python error, previously set by PLy_exception_set().
   * This should be used to propagate Python errors into PG.	If fmt is
*** static char *get_source_line(const char
*** 38,46 
  void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	char	   *xmsg;
! 	char	   *tbmsg;
! 	int			tb_depth;
  	StringInfoData emsg;
  	PyObject   *exc,
  			   *val,
--- 46,54 
  void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	char	   *xmsg = NULL;
! 	char	   *tbmsg = NULL;
! 	int			tb_depth = 0;
  	StringInfoData emsg;
  	PyObject   *exc,
  			   *val,
*** PLy_elog(int elevel, const char *fmt,...
*** 62,68 
  	}
  	PyErr_Restore(exc, val, tb);
  
! 	PLy_traceback(xmsg, tbmsg, tb_depth);
  
  	if (fmt)
  	{
--- 70,90 
  	}
  	PyErr_Restore(exc, val, tb);
  
! 	if (recursion_depth++ = TRACEBACK_RECURSION_LIMIT)
! 	{
! 		PG_TRY();
! 		{
! 			PLy_traceback(xmsg, tbmsg, tb_depth);
! 		}
! 		PG_CATCH();
! 		{
! 			recursion_depth--;
! 			PG_RE_THROW();
! 		}
! 		PG_END_TRY();
! 	}
! 
! 	recursion_depth--;
  
  	if (fmt)
  	{
diff --git a/src/pl/plpython/plpy_util.c b/src/pl/plpython/plpy_util.c
new file mode 100644
index 4aabafc..94c035e
*** a/src/pl/plpython/plpy_util.c
--- b/src/pl/plpython/plpy_util.c
*** PLy_free(void *ptr)
*** 61,126 
  PyObject *
  PLyUnicode_Bytes(PyObject *unicode)
  {
! 	PyObject   *rv;
! 	const char *serverenc;
  
! 	/*
! 	 * Map PostgreSQL encoding to a Python encoding name.
! 	 */
! 	switch (GetDatabaseEncoding())
  	{
! 		case PG_SQL_ASCII:
! 			/*
! 			 * Mapping SQL_ASCII to Python's 'ascii' is a bit bogus. Python's
! 			 * 'ascii' means true 7-bit only ASCII, while PostgreSQL's
! 			 * SQL_ASCII means that anything is allowed, and the system doesn't
! 			 * try to interpret the bytes in any way. But not sure what else
! 			 * to do, and we haven't heard any complaints...
! 			 */
! 			serverenc = ascii;
! 			break;
! 		case PG_WIN1250:
! 			serverenc = cp1250;
! 			break;
! 		case PG_WIN1251:
! 			serverenc = cp1251;
! 			break;
! 		case PG_WIN1252:
! 			serverenc = cp1252;
! 			break;
! 		case PG_WIN1253:
! 			serverenc = cp1253;
! 			break;
! 		case PG_WIN1254:
! 			serverenc = cp1254;
! 			break;
! 		case PG_WIN1255:
! 			serverenc = cp1255;
! 			break;
! 		case PG_WIN1256:
! 			serverenc = cp1256;
! 			break;
! 		case PG_WIN1257:
! 			serverenc = cp1257;
! 			break;
! 		case PG_WIN1258:
! 			serverenc = cp1258;
! 			break;
! 		case PG_WIN866:
! 			serverenc = cp866;
! 			break;
! 		case PG_WIN874:
! 			serverenc = cp874;
! 			break;
! 		default:
! 			/* Other encodings have the same name in Python. */
! 			serverenc = GetDatabaseEncodingName();
! 			break;
  	}
  
! 	rv = PyUnicode_AsEncodedString(unicode, serverenc, strict);
! 	if (rv == NULL)
! 		PLy_elog(ERROR, could not convert Python Unicode object to PostgreSQL server encoding);
  	return rv;
  }
  
--- 61,106 
  PyObject *
  PLyUnicode_Bytes(PyObject *unicode)
  {
! 	PyObject	*bytes, *rv;
! 	char		*utf8string, *encoded;
  
! 	/* first encode the Python unicode object with UTF-8 */
! 	bytes = PyUnicode_AsUTF8String(unicode);
! 	if (bytes == NULL)
! 		PLy_elog(ERROR, could not convert Python Unicode object to bytes);
! 
! 	utf8string = PyBytes_AsString(bytes);
! 	if (utf8string == NULL) {
! 		Py_DECREF(bytes);
! 		PLy_elog(ERROR, could not extract bytes from encoded string);
! 	}
! 
! 	/* then convert to server encoding */
! 	PG_TRY();
  	{
! 		encoded = (char *) pg_do_encoding_conversion(
! 			(unsigned char *) utf8string,
! 			strlen(utf8string),
! 			PG_UTF8,
! 			GetDatabaseEncoding());
! 	}
! 	PG_CATCH();
! 	{
! 		Py_DECREF(bytes);
! 		PG_RE_THROW();
  	}
+ 	PG_END_TRY();
  
! 	/* finally, build a bytes object in the server encoding */
! 	rv = PyBytes_FromStringAndSize(encoded, strlen(encoded));
! 
! 	/* 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Fix mapping of PostgreSQL encodings to Python encodings.

2012-07-20 Thread Jan Urbański

On 20/07/12 08:59, Jan Urbański wrote:

On 18/07/12 17:17, Heikki Linnakangas wrote:

On 14.07.2012 17:50, Jan Urbański wrote:

If pg_do_encoding_conversion() throws an error, you don't get a chance
to call Py_DECREF() to release the string. Is that a problem?

If an error occurs in PLy_traceback(), after incrementing
recursion_depth, you don't get a chance to decrement it again. I'm not
sure if the Py* function calls can fail, but at least seemingly trivial
things like initStringInfo() can throw an out-of-memory error.


Of course you're right (on both accounts).

Here's a version with a bunch of PG_TRies thrown in.


Silly me, playing tricks with postincrements before fully waking up.

Here's v3, with a correct inequality test for exceeding the traceback 
recursion test.


J
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index c375ac0..0ad8358
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** static char *get_source_line(const char
*** 28,33 
--- 28,41 
  
  
  /*
+  * Guard agains re-entrant calls to PLy_traceback, which can happen if
+  * traceback formatting functions raise Python errors.
+  */
+ #define TRACEBACK_RECURSION_LIMIT 2
+ static int	recursion_depth = 0;
+ 
+ 
+ /*
   * Emit a PG error or notice, together with any available info about
   * the current Python error, previously set by PLy_exception_set().
   * This should be used to propagate Python errors into PG.	If fmt is
*** static char *get_source_line(const char
*** 38,46 
  void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	char	   *xmsg;
! 	char	   *tbmsg;
! 	int			tb_depth;
  	StringInfoData emsg;
  	PyObject   *exc,
  			   *val,
--- 46,54 
  void
  PLy_elog(int elevel, const char *fmt,...)
  {
! 	char	   *xmsg = NULL;
! 	char	   *tbmsg = NULL;
! 	int			tb_depth = 0;
  	StringInfoData emsg;
  	PyObject   *exc,
  			   *val,
*** PLy_elog(int elevel, const char *fmt,...
*** 62,68 
  	}
  	PyErr_Restore(exc, val, tb);
  
! 	PLy_traceback(xmsg, tbmsg, tb_depth);
  
  	if (fmt)
  	{
--- 70,90 
  	}
  	PyErr_Restore(exc, val, tb);
  
! 	if (recursion_depth++  TRACEBACK_RECURSION_LIMIT)
! 	{
! 		PG_TRY();
! 		{
! 			PLy_traceback(xmsg, tbmsg, tb_depth);
! 		}
! 		PG_CATCH();
! 		{
! 			recursion_depth--;
! 			PG_RE_THROW();
! 		}
! 		PG_END_TRY();
! 	}
! 
! 	recursion_depth--;
  
  	if (fmt)
  	{
diff --git a/src/pl/plpython/plpy_util.c b/src/pl/plpython/plpy_util.c
new file mode 100644
index 4aabafc..94c035e
*** a/src/pl/plpython/plpy_util.c
--- b/src/pl/plpython/plpy_util.c
*** PLy_free(void *ptr)
*** 61,126 
  PyObject *
  PLyUnicode_Bytes(PyObject *unicode)
  {
! 	PyObject   *rv;
! 	const char *serverenc;
  
! 	/*
! 	 * Map PostgreSQL encoding to a Python encoding name.
! 	 */
! 	switch (GetDatabaseEncoding())
  	{
! 		case PG_SQL_ASCII:
! 			/*
! 			 * Mapping SQL_ASCII to Python's 'ascii' is a bit bogus. Python's
! 			 * 'ascii' means true 7-bit only ASCII, while PostgreSQL's
! 			 * SQL_ASCII means that anything is allowed, and the system doesn't
! 			 * try to interpret the bytes in any way. But not sure what else
! 			 * to do, and we haven't heard any complaints...
! 			 */
! 			serverenc = ascii;
! 			break;
! 		case PG_WIN1250:
! 			serverenc = cp1250;
! 			break;
! 		case PG_WIN1251:
! 			serverenc = cp1251;
! 			break;
! 		case PG_WIN1252:
! 			serverenc = cp1252;
! 			break;
! 		case PG_WIN1253:
! 			serverenc = cp1253;
! 			break;
! 		case PG_WIN1254:
! 			serverenc = cp1254;
! 			break;
! 		case PG_WIN1255:
! 			serverenc = cp1255;
! 			break;
! 		case PG_WIN1256:
! 			serverenc = cp1256;
! 			break;
! 		case PG_WIN1257:
! 			serverenc = cp1257;
! 			break;
! 		case PG_WIN1258:
! 			serverenc = cp1258;
! 			break;
! 		case PG_WIN866:
! 			serverenc = cp866;
! 			break;
! 		case PG_WIN874:
! 			serverenc = cp874;
! 			break;
! 		default:
! 			/* Other encodings have the same name in Python. */
! 			serverenc = GetDatabaseEncodingName();
! 			break;
  	}
  
! 	rv = PyUnicode_AsEncodedString(unicode, serverenc, strict);
! 	if (rv == NULL)
! 		PLy_elog(ERROR, could not convert Python Unicode object to PostgreSQL server encoding);
  	return rv;
  }
  
--- 61,106 
  PyObject *
  PLyUnicode_Bytes(PyObject *unicode)
  {
! 	PyObject	*bytes, *rv;
! 	char		*utf8string, *encoded;
  
! 	/* first encode the Python unicode object with UTF-8 */
! 	bytes = PyUnicode_AsUTF8String(unicode);
! 	if (bytes == NULL)
! 		PLy_elog(ERROR, could not convert Python Unicode object to bytes);
! 
! 	utf8string = PyBytes_AsString(bytes);
! 	if (utf8string == NULL) {
! 		Py_DECREF(bytes);
! 		PLy_elog(ERROR, could not extract bytes from encoded string);
! 	}
! 
! 	/* then convert to server encoding */
! 	PG_TRY();
  	{
! 		encoded = (char *) pg_do_encoding_conversion(
! 			(unsigned char *) utf8string,
! 			strlen(utf8string),
! 			PG_UTF8,
! 			GetDatabaseEncoding());
! 	}
! 	PG_CATCH();
! 	{
! 		

[HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Benedikt Grundmann
We yesterday encountered a program that in a degenerate case issued in
a single transaction a huge number of selects (in a single transaction
but each select in a separate call to PGExec) (huge = ~ 400,000).

That transaction would continue to eat memory up until a point where
calls to malloc (in aset.c) would fail and log for example:

,out of memory,Failed on request of size 11.

Both from that transaction and random others.  In additional observation
of interest to us was that while both VIRT and RES was growing VIRT was
always roughly double of RES.  Which seems to indicate that whatever
allocations were done not all of the memory allocated was actually
touched (server is a Centos6 box).

So I have two questions:

   - Is that expected expected behaviour?  The transaction was
 in READ_COMMITED mode, and my best guess is that this implies
 that some snapshot is taken before each subselect and all
 of them are only freed once the transaction is finished

   - Any recommendations on the use of overcommit?  We had it
 disabled on the assumption that with overcommit the OOM
 killer might kill a random process and that it is better
 to instead have a process that is actually allocating fail
 (and on the additional assumption that any memory allocated
 by postgres would actually be touched).

This is not a huge production issue for us as we can fix the
program to no longer issue huge numbers of selects.  But we
would like to understand.

Thank you very much,

Bene

PS:
The machine has huge amounts of memory and during normal operations
it looks like this:

-bash-4.1$ cat /proc/meminfo
MemTotal:   49413544 kB
MemFree: 1547604 kB
Buffers:5808 kB
Cached: 43777988 kB
SwapCached:0 kB
Active: 18794732 kB
Inactive:   27309980 kB
Active(anon):   13786796 kB
Inactive(anon):  1411296 kB
Active(file):5007936 kB
Inactive(file): 25898684 kB
Unevictable:   71928 kB
Mlocked:   55576 kB
SwapTotal:   2047992 kB
SwapFree:2047992 kB
Dirty: 12100 kB
Writeback: 79684 kB
AnonPages:   2393372 kB
Mapped: 12887392 kB
Shmem:  12871676 kB
Slab:1050468 kB
SReclaimable: 190068 kB
SUnreclaim:   860400 kB
KernelStack:4832 kB
PageTables:   450584 kB
NFS_Unstable:  23312 kB
Bounce:0 kB
WritebackTmp:  0 kB
CommitLimit:26754764 kB
Committed_AS:   17394312 kB
VmallocTotal:   34359738367 kB
VmallocUsed:  120472 kB
VmallocChunk:   34333956900 kB
HardwareCorrupted: 0 kB
AnonHugePages:   1599488 kB
HugePages_Total:   0
HugePages_Free:0
HugePages_Rsvd:0
HugePages_Surp:0
Hugepagesize:   2048 kB
DirectMap4k:5604 kB
DirectMap2M: 2078720 kB
DirectMap1G:48234496 kB

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Heikki Linnakangas

On 20.07.2012 10:19, Benedikt Grundmann wrote:

We yesterday encountered a program that in a degenerate case issued in
a single transaction a huge number of selects (in a single transaction
but each select in a separate call to PGExec) (huge = ~ 400,000).

That transaction would continue to eat memory up until a point where
calls to malloc (in aset.c) would fail and log for example:

,out of memory,Failed on request of size 11.

...

- Is that expected expected behaviour?  The transaction was
  in READ_COMMITED mode, and my best guess is that this implies
  that some snapshot is taken before each subselect and all
  of them are only freed once the transaction is finished


In more complicated scenarios, with e.g subtransactions, it's normal 
that there's some growth in memory usage like that. But with simple 
SELECTs, I don't think there should be.


Can you write a simple self-contained test script that reproduces this? 
That would make it easier to see where the memory is going.


PS, you should upgrade, the latest minor version in 8.4.x series is 
8.4.12. If possible, upgrading to a more recent major version would be a 
good idea too. I don't know if it will help with this problem, but it 
might..


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Andres Freund
Hi,

On Friday, July 20, 2012 09:19:31 AM Benedikt Grundmann wrote:
 We yesterday encountered a program that in a degenerate case issued in
 a single transaction a huge number of selects (in a single transaction
 but each select in a separate call to PGExec) (huge = ~ 400,000).
 
 That transaction would continue to eat memory up until a point where
 calls to malloc (in aset.c) would fail and log for example:
 
 ,out of memory,Failed on request of size 11.
Could you show us the different statements youre running in that transaction? 
Are you using any user defined functions, deferred foreign keys, or anything  
like that?

After youve got that out of memory message, the log should show a list of 
memory contexts with the amount of memory allocated in each. Could you include 
that in a mail?

Greetings,

Andres
-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pgsql_fdw in contrib

2012-07-20 Thread Shigeru HANADA
On Thu, Jul 19, 2012 at 6:06 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 Hanada-san,

 What about the status of your patch?

Sorry for absence.  I have been struggling with connection recovery
issue, but I haven't fixed it yet.  So I'd like to withdraw this patch
from this CF and mark it as returned with feedback because it is far
from commit at the moment.

Of course fixing naming issue too is in my TODO list.

 Even though the 1st commit-fest is getting closed soon,
 I'd like to pay efforts for reviewing to pull up the status of
 pgsql_fdw into ready for committer by beginning of the
 upcoming commit-fest.

Thanks again, I'll try harder this summer. ;-)

Regards,
-- 
Shigeru Hanada

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Benedikt Grundmann
First of all thanks to everyone who has replied so far.

On Fri, Jul 20, 2012 at 10:04 AM, Andres Freund and...@2ndquadrant.com wrote:

 Hi,

 On Friday, July 20, 2012 09:19:31 AM Benedikt Grundmann wrote:
  We yesterday encountered a program that in a degenerate case
  issued in a single transaction a huge number of selects (in a
  single transaction but each select in a separate call to PGExec)
  (huge = ~ 400,000).

  That transaction would continue to eat memory up until a point
  where calls to malloc (in aset.c) would fail and log for example:

  ,out of memory,Failed on request of size 11.
 Could you show us the different statements youre running in that transaction?

They all look like this:

DECLARE sqmlcursor51587 CURSOR FOR select
entry_time,source,bad_fields,isin,sedol,cusip,bloomberg,reuters,exchange_code,currency,description,bbg_instrument_type,instrument_type,specifics,definer,primary_exchange,is_primary_security,is_primary_listing,tags,bloomberg_id,status
from vw_instruments_v7 where jane_symbol = E'FOO BAR' and true and
effective_until = (select max(effective_until) from
vw_instruments_v7)

Sorry I imagine that the fact that this generates a cursor every time
is important
but it had honestly escaped my attention, because the library we use to query
the database uses CURSORs basically for every select, so that it can process
the data in batches (in this particular case that is conceptually unnecessary as
the query will only return one row, but the library does not know that).

 Are you using any user defined functions, deferred foreign keys, or anything
 like that?

No there are several versions of the above view and while the one
mentioned above contains calls to regexp_replace I can reproduce
the same behaviour with a different version of the view that just
renames columns of the underlying table.

 After youve got that out of memory message, the log should show
 a list of memory contexts with the amount of memory allocated in
 each. Could you include that in a mail?

We are using csv logging, which through me off for a moment because I couldn't
find it in there.  But indeed in the .log file I see memory contexts but
I don't know how to correlate them.

I assume they only get written when out of memory happens, so I have included
below the very first one.

TopMemoryContext: 268528136 total in 31 blocks; 37640 free (160
chunks); 268490496 used
  TopTransactionContext: 24576 total in 2 blocks; 14872 free (4
chunks); 9704 used
  Local Buffer Lookup Table: 2088960 total in 8 blocks; 234944 free
(25 chunks); 1854016 used
  Type information cache: 24576 total in 2 blocks; 11888 free (5
chunks); 12688 used
  Operator lookup cache: 24576 total in 2 blocks; 11888 free (5
chunks); 12688 used
  PL/PgSQL function context: 24576 total in 2 blocks; 14384 free (14
chunks); 10192 used
  CFuncHash: 8192 total in 1 blocks; 1680 free (0 chunks); 6512 used
  Rendezvous variable hash: 8192 total in 1 blocks; 1680 free (0
chunks); 6512 used
  PLpgSQL function cache: 24520 total in 2 blocks; 3744 free (0
chunks); 20776 used
  Operator class cache: 8192 total in 1 blocks; 1680 free (0 chunks); 6512 used
  MessageContext: 131072 total in 5 blocks; 54792 free (5 chunks); 76280 used
  smgr relation table: 24576 total in 2 blocks; 5696 free (4 chunks); 18880 used
  TransactionAbortContext: 32768 total in 1 blocks; 32736 free (0
chunks); 32 used
  Portal hash: 8192 total in 1 blocks; 1680 free (0 chunks); 6512 used
  PortalMemory: 8192 total in 1 blocks; 7888 free (1 chunks); 304 used
PortalHeapMemory: 1024 total in 1 blocks; 848 free (0 chunks); 176 used
  ExecutorState: 65600 total in 4 blocks; 33792 free (18 chunks); 31808 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
  Relcache by OID: 24576 total in 2 blocks; 11792 free (3 chunks); 12784 used
  CacheMemoryContext: 1342128 total in 21 blocks; 290016 free (11
chunks); 1052112 used
idx_raw_cfd_bear_commodity_position_records_position_date: 2048
total in 1 blocks; 752 free (0 chunks); 1296 used
CachedPlan: 3072 total in 2 blocks; 2008 free (2 chunks); 1064 used
CachedPlanSource: 3072 total in 2 blocks; 1656 free (0 chunks); 1416 used
SPI Plan: 1024 total in 1 blocks; 808 free (0 chunks); 216 used
CachedPlan: 3072 total in 2 blocks; 1984 free (1 chunks); 1088 used
CachedPlanSource: 3072 total in 2 blocks; 1696 free (0 chunks); 1376 used
SPI Plan: 1024 total in 1 blocks; 808 free (0 chunks); 216 used
CachedPlan: 1024 total in 1 blocks; 312 free (0 chunks); 712 used
CachedPlanSource: 3072 total in 2 blocks; 1584 free (0 chunks); 1488 used
SPI Plan: 1024 total in 1 blocks; 832 free (0 chunks); 192 used
CachedPlan: 1024 total in 1 blocks; 304 free (0 chunks); 720 used
CachedPlanSource: 3072 total in 2 blocks; 1960 free (3 chunks); 1112 used
SPI Plan: 1024 total in 1 blocks; 808 free (0 chunks); 216 used
CachedPlan: 1024 total in 1 blocks; 304 free (0 chunks); 720 used

Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Benedikt Grundmann
On Fri, Jul 20, 2012 at 9:10 AM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
 On 20.07.2012 10:19, Benedikt Grundmann wrote:

 We yesterday encountered a program that in a degenerate case issued in
 a single transaction a huge number of selects (in a single transaction
 but each select in a separate call to PGExec) (huge = ~ 400,000).

 That transaction would continue to eat memory up until a point where
 calls to malloc (in aset.c) would fail and log for example:

 ,out of memory,Failed on request of size 11.

 ...


 - Is that expected expected behaviour?  The transaction was
   in READ_COMMITED mode, and my best guess is that this implies
   that some snapshot is taken before each subselect and all
   of them are only freed once the transaction is finished


 In more complicated scenarios, with e.g subtransactions, it's normal that
 there's some growth in memory usage like that. But with simple SELECTs, I
 don't think there should be.

 Can you write a simple self-contained test script that reproduces this? That
 would make it easier to see where the memory is going.

Assuming that it isn't obvious now that I realized that we generate a cursor
every time, I will give it a shot otherwise.

 PS, you should upgrade, the latest minor version in 8.4.x series is 8.4.12.
 If possible, upgrading to a more recent major version would be a good idea
 too. I don't know if it will help with this problem, but it might..

We are in fact automatically doing an upgrade in testing to 9.1 every day
at the moment.  We plan to pull the trigger in production in a few weeks.

Thanks,

Bene

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Benedikt Grundmann
On Fri, Jul 20, 2012 at 10:46 AM, Benedikt Grundmann
bgrundm...@janestreet.com wrote:

 DECLARE sqmlcursor51587 CURSOR FOR select
 entry_time,source,bad_fields,isin,sedol,cusip,bloomberg,reuters,exchange_code,currency,description,bbg_instrument_type,instrument_type,specifics,definer,primary_exchange,is_primary_security,is_primary_listing,tags,bloomberg_id,status
 from vw_instruments_v7 where jane_symbol = E'FOO BAR' and true and
 effective_until = (select max(effective_until) from
 vw_instruments_v7)

 Sorry I imagine that the fact that this generates a cursor every time
 is important
 but it had honestly escaped my attention, because the library we use to query
 the database uses CURSORs basically for every select, so that it can process
 the data in batches (in this particular case that is conceptually unnecessary 
 as
 the query will only return one row, but the library does not know that).

Actually I believe this must be it.  I just went back and checked the library
and it does not CLOSE the cursors.  This is normally not a problem as most
transactions we have run one or two queries only...  I'll patch the library
to CLOSE the cursor when all the data has been delivered and test if the
error does not happen then.

I also noticed just know that all TopMemoryContext's after the first one
look significantly different.  They contain large PortalMemory sections.
Are those related to cursors?

 TransactionAbortContext: 32768 total in 1 blocks; 32736 free (0
chunks); 32 used
  Portal hash: 8380416 total in 10 blocks; 3345088 free (34 chunks);
5035328 used
  PortalMemory: 16769024 total in 11 blocks; 2737280 free (15 chunks);
14031744 used
PortalHeapMemory: 56320 total in 9 blocks; 4320 free (0 chunks); 52000 used
  ExecutorState: 57344 total in 3 blocks; 15248 free (3 chunks); 42096 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 8192 total in 1 blocks; 8160 free (0 chunks); 32 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
PortalHeapMemory: 56320 total in 9 blocks; 4320 free (0 chunks); 52000 used
  ExecutorState: 57344 total in 3 blocks; 15248 free (3 chunks); 42096 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 8192 total in 1 blocks; 8160 free (0 chunks); 32 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
ExprContext: 0 total in 0 blocks; 0 free (0 chunks); 0 used
...


Thanks everyone,

Bene

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Benedikt Grundmann
On Fri, Jul 20, 2012 at 10:56 AM, Benedikt Grundmann
bgrundm...@janestreet.com wrote:
 On Fri, Jul 20, 2012 at 10:46 AM, Benedikt Grundmann
 bgrundm...@janestreet.com wrote:

 Actually I believe this must be it.  I just went back and checked the library
 and it does not CLOSE the cursors.  This is normally not a problem as most
 transactions we have run one or two queries only...  I'll patch the library
 to CLOSE the cursor when all the data has been delivered and test if the
 error does not happen then.

Just to confirm that indeed fixed it.  Sorry for the noise.

Bene

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] postgres 8.4.9: running out of memory (malloc fails) when running a transaction that runs a LOT of selects

2012-07-20 Thread Andres Freund
Hi,

On Friday, July 20, 2012 11:56:09 AM Benedikt Grundmann wrote:
 I also noticed just know that all TopMemoryContext's after the first one
 look significantly different.  They contain large PortalMemory sections.
 Are those related to cursors?
Yes.

Andres
-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] SP-GiST for ranges based on 2d-mapping and quad-tree

2012-07-20 Thread Heikki Linnakangas

On 13.07.2012 02:00, Alexander Korotkov wrote:

Done. There are separate patch for get rid of TrickFunctionCall2 and
version of SP-GiST for ranges based on that patch.


Looking at the SP-GiST patch now..

It would be nice to have an introduction, perhaps as a file comment at 
the top of rangetypes_spgist.c, explaining how the quad tree works. I 
have a general idea of what a quad tree is, but it's not immediately 
obvious how it maps to SP-GiST. What is stored on a leaf node and an 
internal node? What is the 'prefix' (seems to be the centroid)? How are 
ranges mapped to 2D points? (the function comment of getQuadrant() is a 
good start for that last one)


In spg_range_quad_inner_consistent(), if in-hasPrefix == true, ISTM 
that in all cases where 'empty' is true, 'which' is set to 0, meaning 
that there can be no matches in any of the quadrants. In most of the 
case-branches, you explicitly check for 'empty', but even in the ones 
where you don't, I think you end up setting which=0 if empty==true. I'm 
not 100% sure about the RANGESTRAT_ADJACENT case, though. Am I missing 
something?


It would be nice to avoid the code duplication between the new 
bounds_adjacent() function, and the range_adjacent_internal(). Perhaps 
move bounds_adjacent() to rangetypes.c and use it in 
range_adjacent_internal() too?


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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] row literal problem

2012-07-20 Thread Merlin Moncure
On Fri, Jul 20, 2012 at 1:31 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 I wrote:
 I think the way to solve this is to do whatever it takes to get access
 to the subplan targetlist.  We could then do something a bit cleaner
 than what the named-rowtype code is currently doing: if there are
 resjunk columns in the subplan targetlist, use the tlist to create a
 JunkFilter, and then pass the tuples through that.  After that we can
 insist that the tuples don't have any extra columns.

 Here's a draft patch for that.  It wasn't quite as ugly as I feared.
 A lot of the apparent bulk of the patch is because I chose to split
 ExecEvalVar into separate functions for the scalar and whole-row
 cases, which seemed appropriate because they now get different
 ExprState node types.

Thanks for that!  Applying the patch and confirming the fix turned up
no issues. I did a perfunctory review and it all looks pretty good:
maybe ExecInitExpr could use a comment describing the
InvalidAttrNumber check though...it's somewhat common knowledge that
InvalidAttrNumber means row variables but it's also used to initialize
variables before loops scans and things like that.

merlin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pgbench -i order of vacuum

2012-07-20 Thread Amit Kapila
 From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Jeff Janes
 Sent: Friday, July 20, 2012 5:36 AM


 Is there a reason to vacuum the pgbench_* tables after the indexes on them
are built, rather than before?

 Since the indexes are on fresh tables, they can't have anything that needs
to be cleaned.

The command it executes is vacuum analyze .., so it will do analyze also
on table which means
it will collect stats corresponding to table and index. So if you do it
before creation of index pgbench might behave 
different.
In specific, from function do_analyze_rel(), it will not call
compute_index_stats() if you execute the command before
Creation of index.

With Regards,
Amit Kapila.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pgbench -i order of vacuum

2012-07-20 Thread Jeff Janes
On Fri, Jul 20, 2012 at 7:57 AM, Amit Kapila amit.kap...@huawei.com wrote:
 From: pgsql-hackers-ow...@postgresql.org
 [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Jeff Janes
 Sent: Friday, July 20, 2012 5:36 AM


 Is there a reason to vacuum the pgbench_* tables after the indexes on them
 are built, rather than before?

 Since the indexes are on fresh tables, they can't have anything that needs
 to be cleaned.

 The command it executes is vacuum analyze .., so it will do analyze also
 on table which means
 it will collect stats corresponding to table and index.

Are there stats collected on indexes?  I thought all stats were on
tables only, and the only reason to visit the index was to remove
all-visible-dead entries.

Cheers,

Jeff

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Event Triggers reduced, v1

2012-07-20 Thread Robert Haas
On Wed, Jul 18, 2012 at 10:22 AM, Robert Haas robertmh...@gmail.com wrote:
 The next step here is obviously to complete the work necessary to
 actually be able to fire these triggers, as opposed to just saying
 that we fire them.  I'll write up my thoughts on that topic in a
 separate email.  I don't think there's a ton of work left to be done
 there, but more than zero.

I have committed code to do this.  It's basically similar to what you
had before, but I reworked the event cache machinery heavily.  I think
that the new organization will be easier to extent to meet future
needs, and it also gets rid of a lot of boilerplate code whose job was
to translate between different representations.  I also committed the
PL/pgsql support code, but not the code for the other PLs.  It should
be easy to rebase that work and resubmit it as a separate patch,
though, or maybe one patch per PL would be better.

Obviously, there's quite a bit more work that could be done here --
passing more context, add more firing points, etc. -- but now we've at
least got the basics.

As previously threatened, I amended this code so that triggers fire
once per DDL command.  So internally generated command nodes that are
used as an argument-passing mechanism do not fire triggers, for
example.  I believe this is the right decision because I think, as I
mentioned in another email to Tom yesterday, that generating and
passing around command tags is a really bad practice that we should be
looking to eliminate, not institutionalize.  It posed a major obstacle
to my 9.2-era efforts to clean up the behavior of our DDL under
concurrency, for example.

I think, however, that it would be useful to have an event trigger
that is defined to fire every time a certain type of SQL object gets
created rather than every time a certain command gets executed.
That could complement, not replace, this mechanism.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Using pg_upgrade on log-shipping standby servers

2012-07-20 Thread Bruce Momjian
On Tue, Jul 17, 2012 at 06:02:40PM -0400, Bruce Momjian wrote:
 Second, the user files (large) are certainly identical, it is only the
 system tables (small) that _might_ be different, so rsync'ing just those
 would add the guarantee, but I know of no easy way to rsync just the
 system tables.

OK, new idea.  I said above I didn't know how to copy just the non-user
table files (which are not modified by pg_upgrade), but actually, if you
use link mode, the user files are the only files with a hard link count
of 2.  I could create a script that copied from the master to the slave
only those files with a link count of one.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] pgbench -i order of vacuum

2012-07-20 Thread Tom Lane
Jeff Janes jeff.ja...@gmail.com writes:
 On Fri, Jul 20, 2012 at 7:57 AM, Amit Kapila amit.kap...@huawei.com wrote:
 The command it executes is vacuum analyze .., so it will do analyze also
 on table which means
 it will collect stats corresponding to table and index.

 Are there stats collected on indexes?

Only for expression indexes, which there aren't any of in the standard
pgbench scenario.  I don't see a reason not to change the ordering
as you suggest.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Using pg_upgrade on log-shipping standby servers

2012-07-20 Thread Aidan Van Dyk
If you're wanting to automatically do some upgrades wouldn't an easier route be:

1) run pg_upgrade, up to the point where it actually start's
copying/linking in old cluster data files, and stop the new
postmaster.
2) Take a base backup style copy (tar, rsync, $FAVOURITE) of the new
cluster (small, since without data files)
3) Have pg_upgrade leave a log of exactly which old cluster data files
go where in the new cluster

That way, anybody, any script, etc who wants to make a new standby
from and old one only needs the pg_upgrade base backup (which should
be small, no data, just catalog stuff), and the log of which old files
to move where.

The only pre-condition is that the standby's old pg *APPLIED* WAL up
to the exact same point as the master's old pg.  In that case the
standby's old cluster data files should same enough (maybe hint bits
off?) to be used.

a.

On Fri, Jul 20, 2012 at 12:25 PM, Bruce Momjian br...@momjian.us wrote:
 On Tue, Jul 17, 2012 at 06:02:40PM -0400, Bruce Momjian wrote:
 Second, the user files (large) are certainly identical, it is only the
 system tables (small) that _might_ be different, so rsync'ing just those
 would add the guarantee, but I know of no easy way to rsync just the
 system tables.

 OK, new idea.  I said above I didn't know how to copy just the non-user
 table files (which are not modified by pg_upgrade), but actually, if you
 use link mode, the user files are the only files with a hard link count
 of 2.  I could create a script that copied from the master to the slave
 only those files with a link count of one.

 --
   Bruce Momjian  br...@momjian.ushttp://momjian.us
   EnterpriseDB http://enterprisedb.com

   + It's impossible for everything to be true. +

 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers




-- 
Aidan Van Dyk Create like a god,
ai...@highrise.ca   command like a king,
http://www.highrise.ca/   work like a slave.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Using pg_upgrade on log-shipping standby servers

2012-07-20 Thread Bruce Momjian
On Fri, Jul 20, 2012 at 12:39:12PM -0400, Aidan Van Dyk wrote:
 If you're wanting to automatically do some upgrades wouldn't an easier route 
 be:
 
 1) run pg_upgrade, up to the point where it actually start's
 copying/linking in old cluster data files, and stop the new
 postmaster.
 2) Take a base backup style copy (tar, rsync, $FAVOURITE) of the new
 cluster (small, since without data files)
 3) Have pg_upgrade leave a log of exactly which old cluster data files
 go where in the new cluster
 
 That way, anybody, any script, etc who wants to make a new standby
 from and old one only needs the pg_upgrade base backup (which should
 be small, no data, just catalog stuff), and the log of which old files
 to move where.
 
 The only pre-condition is that the standby's old pg *APPLIED* WAL up
 to the exact same point as the master's old pg.  In that case the
 standby's old cluster data files should same enough (maybe hint bits
 off?) to be used.

I am not sure what a base backup is buying us here --- base backup is
designed to create a backup while the server is running, and it is down
at that point.  I think what you are suggesting is to make a data dir
copy while just the schema is in place.  That is possible, but it opens
up all kinds of possible failure cases because pg_upgrade operations
have to be done in a specific order --- it feels very fragile.

I think the commands to run after pg_upgrade --link completes on both
primary and standby might be as easy as:

cd /u/pg/pgsql.old/data
find . -links 1 -exec cp {} /u/pgsql/data \;

Why would we want anything more complicated than this?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

2012-07-20 Thread Robert Haas
On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Kevin Grittner kevin.gritt...@wicourts.gov writes:
 Rather than trying to enforce this in the
 ALTER FUNCTION implementation, maybe we should just advise that if
 you're going to grant ownership of a C function to a role which
 might accidentally or maliciously allow it to be called with NULL
 input, the C function should return NULL in that case.

 Yeah, the just-code-defensively option is worth considering too.

After rereading this thread, I think I agree with Kevin as well.  In
order to have a problem, you have to (a) be superuser (i.e. be a
person who already has many ways of compromising the security and
integrity of the system), (b) decide to grant ownership of a C
function to a non-superuser, and (c) fail to verify that the function
is coded in a sufficiently defensive fashion to survive whatever that
user might do with it.  So it seems plausible to simply define this
problem as administrator error rather than a failure of security.

Having said that, I do believe that answer is to some extent a
cop-out.  It's practical to define things that way here because the
cost of checking for NULL inputs is pretty darn small.  But suppose
ALTER FUNCTION had a facility to change the type of a function
argument.  Would we then insist that any C function intended to be
used in this way must check that the type of each argument matches the
function's expectation?  Fortunately, we don't have to decide that
right now because ALTER FUNCTION doesn't allow that and probably never
will.  But it would certainly be awkward if we did someday allow that,
because now any C function that is intended to be used in this way has
to include this extra check or be labelled insecure.  Short of
allowing the C code to advertise the function's expectations so that
CREATE/ALTER FUNCTION can cross-check them, I don't see a way out of
that problem because on the flip side, the C code could - not to
put too fine a point on it - be relying on just about anything.  It
could assert that work_mem is less than 1GB (and the user could crash
it by increasing work_mem).  It could crash on any day except Tuesday
(we always do payroll here on Tuesdays, so why would you call it on
any other day?).  It could erase the entire database unless it's
marked immutable.  We would surely blame any of those failure modes on
the person who wrote the C code, and if we make the opposite decision
here, then I think we put ourselves in the awkward position of trying
to adjudicate what is and is not reasonably for third parties to do in
their C code.  I don't really want to go there, but I do think this
points to the need for extreme caution if we ever create any more
function properties that C coders might be tempted to rely on, or
allow any properties that C code may already be relying on to be
changed in new ways.

I'm going to mark this patch as rejected in the CF app, which is not
intended to foreclose debate on what to do about this, but just to
state that there seems to be no consensus to solve this problem in the
manner proposed.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] row literal problem

2012-07-20 Thread Tom Lane
Merlin Moncure mmonc...@gmail.com writes:
 On Fri, Jul 20, 2012 at 1:31 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 Here's a draft patch for that.  It wasn't quite as ugly as I feared.
 A lot of the apparent bulk of the patch is because I chose to split
 ExecEvalVar into separate functions for the scalar and whole-row
 cases, which seemed appropriate because they now get different
 ExprState node types.

 Thanks for that!  Applying the patch and confirming the fix turned up
 no issues. I did a perfunctory review and it all looks pretty good:
 maybe ExecInitExpr could use a comment describing the
 InvalidAttrNumber check though...it's somewhat common knowledge that
 InvalidAttrNumber means row variables but it's also used to initialize
 variables before loops scans and things like that.

Thanks for testing.  I added the suggested comment and made some other
cosmetic improvements, and have committed this.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Andrew Dunstan


On 07/19/2012 09:54 AM, Andrew Dunstan wrote:




Meanwhile, I would like to remove the prepared_transactions test from 
the main isolation schedule, and add a new Make target which runs that 
test explicitly. Is there any objection to that?






Here's the patch for that.

cheers

andrew


diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index 74baed5..d0af9d1 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -72,3 +72,13 @@ installcheck: all
 
 check: all
 	./pg_isolation_regress --temp-install=./tmp_check --inputdir=$(srcdir) --top-builddir=$(top_builddir) --schedule=$(srcdir)/isolation_schedule
+
+# versions of the check tests that include the prepared_transactions test
+# it only makes sense to run these if set up to use prepared transactions,
+# via TEMP_CONFIG for the check case, or via the postgresql.conf for the
+# installcheck case.
+installcheck_prepared_txns: all
+	./pg_isolation_regress --psqldir='$(PSQLDIR)' --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule_prepared_txns
+
+check_prepared_txns: all
+	./pg_isolation_regress --temp-install=./tmp_check --inputdir=$(srcdir) --top-builddir=$(top_builddir) --schedule=$(srcdir)/isolation_schedule_prepared_txns
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 669c0f2..2184975 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -9,7 +9,6 @@ test: ri-trigger
 test: partial-index
 test: two-ids
 test: multiple-row-versions
-test: prepared-transactions
 test: fk-contention
 test: fk-deadlock
 test: fk-deadlock2
diff --git a/src/test/isolation/isolation_schedule_prepared_txns b/src/test/isolation/isolation_schedule_prepared_txns
new file mode 100644
index 000..669c0f2
--- /dev/null
+++ b/src/test/isolation/isolation_schedule_prepared_txns
@@ -0,0 +1,16 @@
+test: simple-write-skew
+test: receipt-report
+test: temporal-range-integrity
+test: project-manager
+test: classroom-scheduling
+test: total-cash
+test: referential-integrity
+test: ri-trigger
+test: partial-index
+test: two-ids
+test: multiple-row-versions
+test: prepared-transactions
+test: fk-contention
+test: fk-deadlock
+test: fk-deadlock2
+test: eval-plan-qual

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Event Triggers reduced, v1

2012-07-20 Thread Thom Brown
On 20 July 2012 16:50, Robert Haas robertmh...@gmail.com wrote:
 On Wed, Jul 18, 2012 at 10:22 AM, Robert Haas robertmh...@gmail.com wrote:
 The next step here is obviously to complete the work necessary to
 actually be able to fire these triggers, as opposed to just saying
 that we fire them.  I'll write up my thoughts on that topic in a
 separate email.  I don't think there's a ton of work left to be done
 there, but more than zero.

 I have committed code to do this.  It's basically similar to what you
 had before, but I reworked the event cache machinery heavily.  I think
 that the new organization will be easier to extent to meet future
 needs, and it also gets rid of a lot of boilerplate code whose job was
 to translate between different representations.  I also committed the
 PL/pgsql support code, but not the code for the other PLs.  It should
 be easy to rebase that work and resubmit it as a separate patch,
 though, or maybe one patch per PL would be better.

 Obviously, there's quite a bit more work that could be done here --
 passing more context, add more firing points, etc. -- but now we've at
 least got the basics.

 As previously threatened, I amended this code so that triggers fire
 once per DDL command.  So internally generated command nodes that are
 used as an argument-passing mechanism do not fire triggers, for
 example.  I believe this is the right decision because I think, as I
 mentioned in another email to Tom yesterday, that generating and
 passing around command tags is a really bad practice that we should be
 looking to eliminate, not institutionalize.  It posed a major obstacle
 to my 9.2-era efforts to clean up the behavior of our DDL under
 concurrency, for example.

 I think, however, that it would be useful to have an event trigger
 that is defined to fire every time a certain type of SQL object gets
 created rather than every time a certain command gets executed.
 That could complement, not replace, this mechanism.

I've just run my own set of tests against these changes, which tests
every supported DDL command (with the exception of CREATE USER
MAPPING, ALTER USER MAPPING, DROP USER MAPPING, CREATE LANGUAGE
and DROP LANGUAGE), and many variants of each command, and
everything behaves exactly as expected. :)

-- 
Thom

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Alvaro Herrera

Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:
 
 On 07/19/2012 09:54 AM, Andrew Dunstan wrote:
 
 
 
  Meanwhile, I would like to remove the prepared_transactions test from 
  the main isolation schedule, and add a new Make target which runs that 
  test explicitly. Is there any objection to that?
 
 
 
 
 Here's the patch for that.

Looks reasonable.  I'd like to have an include directive in regress
files so that we don't have to repeat the whole set in the second file,
but please don't let that wishlist item to stop you from committing this
patch.

Are you planning to have one of your animals run the prepared xacts
schedule? :-)

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Alvaro Herrera

Excerpts from Noah Misch's message of mar jul 17 16:28:32 -0400 2012:
 
 On Tue, Jul 17, 2012 at 01:56:19PM -0400, Alvaro Herrera wrote:

  However, there's more work to do in isolation testing.  It'd be good to
  have it being routinely run in serializable isolation level, for
  example, not just in read committed.
 
 Except for the foreign key specs, isolation test specs request a specific
 isolation level when starting their transactions.  Running such specs under
 different default_transaction_isolation settings primarily confirms that
 BEGIN TRANSACTION ISOLATION LEVEL x is indistinguishable from BEGIN under
 default_transaction_isolation = x.  It might also discover transaction
 isolation sensitivity in the setup/cleanup steps, which often omit explicit
 transaction control.  I don't think such verification justifies regularly
 running thousands of tests.  The foreign key tests, however, would benefit
 from running under all three isolation levels.  Let's control it per-spec
 instead of repeating the entire suite.

Understood and agreed.  Maybe we could use a new directive in the spec
file format for this.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Andrew Dunstan


On 07/20/2012 01:37 PM, Alvaro Herrera wrote:

Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:

On 07/19/2012 09:54 AM, Andrew Dunstan wrote:



Meanwhile, I would like to remove the prepared_transactions test from
the main isolation schedule, and add a new Make target which runs that
test explicitly. Is there any objection to that?




Here's the patch for that.

Looks reasonable.  I'd like to have an include directive in regress
files so that we don't have to repeat the whole set in the second file,
but please don't let that wishlist item to stop you from committing this
patch.

Are you planning to have one of your animals run the prepared xacts
schedule? :-)




Possibly - I'm tossing up in my mind the best way to do that. (Hacking 
the script on a particular client would be the wrong way.)


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

2012-07-20 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, the just-code-defensively option is worth considering too.

 After rereading this thread, I think I agree with Kevin as well. ...
 Having said that, I do believe that answer is to some extent a
 cop-out.

I agree with that --- doing nothing at all doesn't seem like the best
option here.

 ... on the flip side, the C code could - not to
 put too fine a point on it - be relying on just about anything.

And with that too.  The STRICT option is a fairly obvious security
hazard, but who's to say there are not others?  I think it'd be easier
to make a case for forbidding a non-superuser from altering *any*
property of a C function.  I'd rather start from the point of allowing
only what is clearly safe than disallowing only what is clearly unsafe.

Taking a step or two back, I think that the real use-case we should
be considering here is allowing non-superusers to own (or at least
install) extensions that contain C functions.  We would probably want
the non-superuser to be able to drop the extension again, maybe
ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
not too darn much else.  Fooling with any of the contained objects
doesn't seem like something we want to permit, since it's likely that
something like a datatype is going to have dependencies on not just
specific objects' properties but their interrelationships.

One possible approach to that is to say that the nominal owner of such
an extension only owns the extension itself, and ownership of the
contained objects is held by, say, the bootstrap superuser.  There are
other ways too of course, but this way would bypass the problem of
figuring out how to restrict what an object's nominal owner can do
to it.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 Excerpts from Andrew Dunstan's message of vie jul 20 13:15:12 -0400 2012:
 Meanwhile, I would like to remove the prepared_transactions test from 
 the main isolation schedule, and add a new Make target which runs that 
 test explicitly. Is there any objection to that?

 Looks reasonable.  I'd like to have an include directive in regress
 files so that we don't have to repeat the whole set in the second file,
 but please don't let that wishlist item to stop you from committing this
 patch.

I'm not thrilled with replicating the test-list file either.  But it is
not necessary: look at the way the bigtest target is defined in the
main regression makefile.  You can just add some more test names on the
command line, to be done in addition to what's in the schedule file.
I think we should do it that way here too.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Event Triggers reduced, v1

2012-07-20 Thread Tom Lane
The Windows buildfarm members don't seem too happy with the latest
patch.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] isolation check takes a long time

2012-07-20 Thread Andrew Dunstan


On 07/20/2012 01:56 PM, Tom Lane wrote:

I'm not thrilled with replicating the test-list file either.  But it is
not necessary: look at the way the bigtest target is defined in the
main regression makefile.  You can just add some more test names on the
command line, to be done in addition to what's in the schedule file.
I think we should do it that way here too.





Oh, I wasn't aware of that. Will do.

cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

2012-07-20 Thread Robert Haas
On Fri, Jul 20, 2012 at 1:52 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Jun 12, 2012 at 5:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Yeah, the just-code-defensively option is worth considering too.

 After rereading this thread, I think I agree with Kevin as well. ...
 Having said that, I do believe that answer is to some extent a
 cop-out.

 I agree with that --- doing nothing at all doesn't seem like the best
 option here.

 ... on the flip side, the C code could - not to
 put too fine a point on it - be relying on just about anything.

 And with that too.  The STRICT option is a fairly obvious security
 hazard, but who's to say there are not others?  I think it'd be easier
 to make a case for forbidding a non-superuser from altering *any*
 property of a C function.  I'd rather start from the point of allowing
 only what is clearly safe than disallowing only what is clearly unsafe.

That seems like a fairly drastic overreaction.  Are you going to ban
renaming it or changing the owner, which are in completely different
code paths?  Yuck.  Even if you only ban it for the main ALTER
FUNCTION code path, it seems pretty draconian, because it looks to me
like nearly everything else that's there is perfectly safe.  I mean,
assuming the guy who wrote the C code didn't do anything completely
insane or malicious, setting GUCs or whatever should be perfectly OK.
Honestly, if you want to change something in the code, I'm not too
convinced that there's anything better than what Noah proposed
originally.

 Taking a step or two back, I think that the real use-case we should
 be considering here is allowing non-superusers to own (or at least
 install) extensions that contain C functions.  We would probably want
 the non-superuser to be able to drop the extension again, maybe
 ALTER EXTENSION SET SCHEMA, maybe ALTER EXTENSION OWNER ... and likely
 not too darn much else.  Fooling with any of the contained objects
 doesn't seem like something we want to permit, since it's likely that
 something like a datatype is going to have dependencies on not just
 specific objects' properties but their interrelationships.

Moreover, it breaks dump-and-restore.

 One possible approach to that is to say that the nominal owner of such
 an extension only owns the extension itself, and ownership of the
 contained objects is held by, say, the bootstrap superuser.  There are
 other ways too of course, but this way would bypass the problem of
 figuring out how to restrict what an object's nominal owner can do
 to it.

I don't particularly care for that solution; it seems like a kludge.
I've kind of wondered whether we ought to have checks in all the ALTER
routines that spit up if you try to ALTER an extension member from any
place other than an extension upgrade script...  but that still
wouldn't prevent the extension owner from dropping the members out of
the extension and then modifying them afterwards.  I'm not sure we
want to prevent that in general, but maybe there could be some
locked-down mode that has that effect.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

2012-07-20 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 I don't particularly care for that solution; it seems like a kludge.
 I've kind of wondered whether we ought to have checks in all the ALTER
 routines that spit up if you try to ALTER an extension member from any
 place other than an extension upgrade script...  but that still
 wouldn't prevent the extension owner from dropping the members out of
 the extension and then modifying them afterwards.  I'm not sure we
 want to prevent that in general, but maybe there could be some
 locked-down mode that has that effect.

Right, I wasn't too clear about that, but I meant that we'd have some
sort of locked-down state for an extension that would forbid fooling
with its contents.  For development purposes, or for anybody that knows
what they're doing, adding/subtracting/modifying member objects is
mighty handy.  But a non-superuser who's loaded an extension that
contains C functions ought not have those privileges for it.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Event Triggers reduced, v1

2012-07-20 Thread Robert Haas
On Fri, Jul 20, 2012 at 2:12 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 The Windows buildfarm members don't seem too happy with the latest
 patch.

I'm looking at this now, but am so far mystified.  Something's
obviously broken as regards how the trigger flags get set up, but if
that were broken in a trivial way it would be broken everywhere, and
it's not.  Will keep searching...

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Event Triggers reduced, v1

2012-07-20 Thread Robert Haas
On Fri, Jul 20, 2012 at 1:22 PM, Thom Brown t...@linux.com wrote:
 I've just run my own set of tests against these changes, which tests
 every supported DDL command (with the exception of CREATE USER
 MAPPING, ALTER USER MAPPING, DROP USER MAPPING, CREATE LANGUAGE
 and DROP LANGUAGE), and many variants of each command, and
 everything behaves exactly as expected. :)

Nice!

But all of those commands you just mentioned ought to work, too.

The documentation probably needs some updating on that point, come to
think of it.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CHECK NO INHERIT syntax

2012-07-20 Thread Alvaro Herrera
Excerpts from Peter Eisentraut's message of mié jul 18 17:49:37 -0400 2012:
 Sorry to raise this once again, but I still find this CHECK NO INHERIT
 syntax to a bit funny.  We are currently using something like
 
 CHECK NO INHERIT (foo  0)
 
 But we already have a different syntax for attaching attributes to
 constraints (NOT DEFERRABLE, NOT VALID,  etc.), so it would make more
 sense to have
 
 CHECK (foo  0) NO INHERIT

Okay, given the astounding acceptance of your proposal, the attached patch
fixes things in that way.  This only include changes to the core code;
I'll prepare documentation and regression tests tweaks while I wait for an
answer to the request below.

 There is also a hole in the current implementation.  Domain constraints
 silently allow NO INHERIT to be specified, even though other senseless
 attributes are rejected.

True.  I have added an error check at creation time.  Please suggest
improved wording for the message:

alvherre=# create domain positiveint2 as int check (value  0) no inherit;
ERROR:  CHECK constraints for domains cannot be NO INHERIT

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


check-no-inherit.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CHECK NO INHERIT syntax

2012-07-20 Thread Tom Lane
Alvaro Herrera alvhe...@commandprompt.com writes:
 True.  I have added an error check at creation time.  Please suggest
 improved wording for the message:

 alvherre=# create domain positiveint2 as int check (value  0) no inherit;
 ERROR:  CHECK constraints for domains cannot be NO INHERIT

I think CHECK constraints for domains cannot be marked NO INHERIT
would be fine.

  ConstraintElem:
 - CHECK opt_no_inherit '(' a_expr ')' 
 ConstraintAttributeSpec
 + CHECK '(' a_expr ')' opt_no_inherit 
 ConstraintAttributeSpec

This doesn't seem to me to meet the principle of least surprise.  Surely
NO INHERIT ought to be folded into ConstraintAttributeSpec so that it
acts like other constraint decorations, ie order isn't significant.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [COMMITTERS] pgsql: Remove prepared transactions from main isolation test schedule.

2012-07-20 Thread Tom Lane
Andrew Dunstan and...@dunslane.net writes:
 Remove prepared transactions from main isolation test schedule.

 There is no point in running this test when prepared transactions are 
 disabled,
 which is the default. New make targets that include the test are provided. 
 This
 will save some useless waste of cycles on buildfarm machines.

Having done that, shouldn't we remove prepared-transactions_1.out (the
expected file for the prepared-transactions-disabled case)?

Having a megabyte-sized test file for that case has always seemed pretty
wasteful to me.  Now that the test won't be run unless intentionally
selected, it seems like people who are using it would expect it to
actually test prepared transactions --- so having it pass when they're
disabled seems wrong.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [COMMITTERS] pgsql: Remove prepared transactions from main isolation test schedule.

2012-07-20 Thread Andrew Dunstan


On 07/20/2012 04:17 PM, Tom Lane wrote:

Andrew Dunstan and...@dunslane.net writes:

Remove prepared transactions from main isolation test schedule.
There is no point in running this test when prepared transactions are disabled,
which is the default. New make targets that include the test are provided. This
will save some useless waste of cycles on buildfarm machines.

Having done that, shouldn't we remove prepared-transactions_1.out (the
expected file for the prepared-transactions-disabled case)?

Having a megabyte-sized test file for that case has always seemed pretty
wasteful to me.  Now that the test won't be run unless intentionally
selected, it seems like people who are using it would expect it to
actually test prepared transactions --- so having it pass when they're
disabled seems wrong.






Good point. Will do.

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Restrict ALTER FUNCTION CALLED ON NULL INPUT (was Re: Not quite a security hole: CREATE LANGUAGE for non-superusers)

2012-07-20 Thread Robert Haas
On Fri, Jul 20, 2012 at 3:45 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 I don't particularly care for that solution; it seems like a kludge.
 I've kind of wondered whether we ought to have checks in all the ALTER
 routines that spit up if you try to ALTER an extension member from any
 place other than an extension upgrade script...  but that still
 wouldn't prevent the extension owner from dropping the members out of
 the extension and then modifying them afterwards.  I'm not sure we
 want to prevent that in general, but maybe there could be some
 locked-down mode that has that effect.

 Right, I wasn't too clear about that, but I meant that we'd have some
 sort of locked-down state for an extension that would forbid fooling
 with its contents.  For development purposes, or for anybody that knows
 what they're doing, adding/subtracting/modifying member objects is
 mighty handy.  But a non-superuser who's loaded an extension that
 contains C functions ought not have those privileges for it.

I could see having such a mode.  I'm not sure that it would eliminate
people's desire to manually give away functions, though.  In fact,
thinking about a couple of our customers, I'm pretty sure it wouldn't.
 Now whether it's a good idea is another question, but...

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Resetting libpq connections after an app error

2012-07-20 Thread Daniele Varrazzo
Hello,

apologize for bumping the question to -hackers but I got no answer
from -general. If there is a better ML to post it let me know.

In a libpq application, if there is an application error between
PQsendQuery and PQgetResult, is there a way to revert a connection
back to an usable state (i.e. from transaction status ACTIVE to IDLE)
without using the network in a blocking way? We are currently doing

while (NULL != (res = PQgetResult(conn-pgconn))) {
PQclear(res);
}

but this is blocking, and if the error had been caused by the network
down, we'll just get stuck in a poll() waiting for a timeout.

Thank you very much.

-- Daniele

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] CHECK NO INHERIT syntax

2012-07-20 Thread Alvaro Herrera
Excerpts from Tom Lane's message of vie jul 20 16:12:05 -0400 2012:
 
 Alvaro Herrera alvhe...@commandprompt.com writes:
  True.  I have added an error check at creation time.  Please suggest
  improved wording for the message:
 
  alvherre=# create domain positiveint2 as int check (value  0) no inherit;
  ERROR:  CHECK constraints for domains cannot be NO INHERIT
 
 I think CHECK constraints for domains cannot be marked NO INHERIT
 would be fine.

Thanks.

   ConstraintElem:
  -CHECK opt_no_inherit '(' a_expr ')' ConstraintAttributeSpec
  +CHECK '(' a_expr ')' opt_no_inherit ConstraintAttributeSpec
 
 This doesn't seem to me to meet the principle of least surprise.  Surely
 NO INHERIT ought to be folded into ConstraintAttributeSpec so that it
 acts like other constraint decorations, ie order isn't significant.

Oh, true; that's a bit more involved.  I verified it works correctly to
have a constraint marked NOT VALID NO INHERIT or the other way around.
I haven't checked whether the changes to ConstraintAttributeSpec have
side effects -- I think it's OK but I might be missing something.

Here's a (hopefully) complete patch.

-- 
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support


check-no-inherit-2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers