[Jim Nasby <Jim.Nasby@BlueTreble.com>]

A client was getting some hard to diagnose out of memory errors. What 
made this especially confusing was that there was no context reported at 
all, other than the (enormous) statement that triggered the error.

At first I thought the lack of context indicated a palloc had failed 
during ereport() (since we apparently just toss the previous error when 
that happens), but it turns out there's some error reporting in 
pg_stat_statements that's less than ideal. Attached patch fixes, though 
I'm not sure if %lld is portable or not.

I'll also argue that this is a bug and should be backpatched, but I'm 
not hell-bent on that.

At the same time I looked for other messages that don't explicitly 
reference pg_stat_statements; the only others are in 
pg_stat_statements_internal() complaining about being called in an 
inappropriate function context. Presumably at that point there's a 
reasonable error context stack so I didn't bother with them.

This still seems a bit fragile to me though. Anyone working in here has 
to notice that most every errmsg mentions pg_stat_statements and decide 
there's a good reason for that. ISTM it'd be better to push a new 
ErrorContextCallback onto the stack any time we enter the module. If 
folks think that's a good idea I'll pursue it as a separate patch.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 59b8a2e..06a0912 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1893,14 +1893,24 @@ qtext_load_file(Size *buffer_size)
 
 	/* Allocate buffer; beware that off_t might be wider than size_t */
 	if (stat.st_size <= MaxAllocSize)
-		buf = (char *) malloc(stat.st_size);
-	else
-		buf = NULL;
-	if (buf == NULL)
 	{
+		buf = (char *) malloc(stat.st_size);
+
+		if (buf == NULL)
+		{
+			ereport(LOG,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory attempting to pg_stat_statement file"),
+				 errdetail("file \"%s\": size %lld", PGSS_TEXT_FILE, stat.st_size)));
+			CloseTransientFile(fd);
+			return NULL;
+		}
+	} else {
 		ereport(LOG,
+			 	/* Is there a better code to use? IE: SQLSTATE 53000, 53400 or 54000 */
 				(errcode(ERRCODE_OUT_OF_MEMORY),
-				 errmsg("out of memory")));
+				 errmsg("pg_stat_statement file is too large to process"),
+				 errdetail("file \"%s\": size %lld", PGSS_TEXT_FILE, stat.st_size)));
 		CloseTransientFile(fd);
 		return NULL;
 	}


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

Attachments:

  [text/plain] patch.diff (1.2K, ../5601D354.5000703@BlueTreble.com/2-patch.diff)
  download | inline diff:
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 59b8a2e..06a0912 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1893,14 +1893,24 @@ qtext_load_file(Size *buffer_size)
 
 	/* Allocate buffer; beware that off_t might be wider than size_t */
 	if (stat.st_size <= MaxAllocSize)
-		buf = (char *) malloc(stat.st_size);
-	else
-		buf = NULL;
-	if (buf == NULL)
 	{
+		buf = (char *) malloc(stat.st_size);
+
+		if (buf == NULL)
+		{
+			ereport(LOG,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory attempting to pg_stat_statement file"),
+				 errdetail("file \"%s\": size %lld", PGSS_TEXT_FILE, stat.st_size)));
+			CloseTransientFile(fd);
+			return NULL;
+		}
+	} else {
 		ereport(LOG,
+			 	/* Is there a better code to use? IE: SQLSTATE 53000, 53400 or 54000 */
 				(errcode(ERRCODE_OUT_OF_MEMORY),
-				 errmsg("out of memory")));
+				 errmsg("pg_stat_statement file is too large to process"),
+				 errdetail("file \"%s\": size %lld", PGSS_TEXT_FILE, stat.st_size)));
 		CloseTransientFile(fd);
 		return NULL;
 	}