Re: [HACKERS] Re: [COMMITTERS] Using contrib modules in check (Re: pgsql: Fix BRIN to use SnapshotAny during summarization)

2015-08-13 Thread Alvaro Herrera
Alvaro Herrera wrote:
 Tom Lane wrote:

  (If I were tasked with fixing it, I'd be tempted to rewrite it to do
  all the work in one call and return a tuplestore; the alternative
  seems to be to try to keep the index open across multiple calls,
  which would be a mess.)
 
 Here's a patch doing that.

Pushed, thanks.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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


[HACKERS] Re: [COMMITTERS] Using contrib modules in check (Re: pgsql: Fix BRIN to use SnapshotAny during summarization)

2015-08-12 Thread Alvaro Herrera
Tom Lane wrote:

 However, we did learn something valuable from the fact that all the
 -DCLOBBER_CACHE_ALWAYS critters failed on it: per my earlier message,
 brin_page_items() is unsafe against a relcache flush on the index.
 I'll put that on the 9.5 open items list.
 
 (If I were tasked with fixing it, I'd be tempted to rewrite it to do
 all the work in one call and return a tuplestore; the alternative
 seems to be to try to keep the index open across multiple calls,
 which would be a mess.)

Here's a patch doing that.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services
commit 41eba17795e9cf9b0c23e05def5d33b0dbdd4807
Author: Alvaro Herrera alvhe...@alvh.no-ip.org
AuthorDate: Wed Aug 12 15:41:15 2015 -0300
CommitDate: Wed Aug 12 15:41:15 2015 -0300

fix brin_page_items to use a tuplestore

diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 7adcfa8..f695b18 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -37,18 +37,6 @@ typedef struct brin_column_state
 	FmgrInfo	outputFn[FLEXIBLE_ARRAY_MEMBER];
 } brin_column_state;
 
-typedef struct brin_page_state
-{
-	BrinDesc   *bdesc;
-	Page		page;
-	OffsetNumber offset;
-	bool		unusedItem;
-	bool		done;
-	AttrNumber	attno;
-	BrinMemTuple *dtup;
-	brin_column_state *columns[FLEXIBLE_ARRAY_MEMBER];
-} brin_page_state;
-
 
 static Page verify_brin_page(bytea *raw_page, uint16 type,
  const char *strtype);
@@ -119,89 +107,90 @@ verify_brin_page(bytea *raw_page, uint16 type, const char *strtype)
 Datum
 brin_page_items(PG_FUNCTION_ARGS)
 {
-	brin_page_state *state;
-	FuncCallContext *fctx;
+	bytea	   *raw_page = PG_GETARG_BYTEA_P(0);
+	Oid			indexRelid = PG_GETARG_OID(1);
+	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo-resultinfo;
+	TupleDesc	tupdesc;
+	MemoryContext oldcontext;
+	Tuplestorestate *tupstore;
+	Relation	indexRel;
+	Page		page;
+	OffsetNumber offset;
+	AttrNumber	attno;
+	bool		unusedItem;
+	BrinDesc   *bdesc;
+	BrinMemTuple *dtup;
+	brin_column_state **columns;
 
 	if (!superuser())
 		ereport(ERROR,
 (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  (errmsg(must be superuser to use raw page functions;
 
-	if (SRF_IS_FIRSTCALL())
+	/* check to see if caller supports us returning a tuplestore */
+	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg(set-valued function called in context that cannot accept a set)));
+	if (!(rsinfo-allowedModes  SFRM_Materialize) ||
+		rsinfo-expectedDesc == NULL)
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg(materialize mode required, but it is not allowed in this context)));
+
+	/* Build a tuple descriptor for our result type */
+	if (get_call_result_type(fcinfo, NULL, tupdesc) != TYPEFUNC_COMPOSITE)
+		elog(ERROR, return type must be a row type);
+
+	/* Build tuplestore to hold the result rows */
+	oldcontext = MemoryContextSwitchTo(rsinfo-econtext-ecxt_per_query_memory);
+
+	tupstore = tuplestore_begin_heap(true, false, work_mem);
+	rsinfo-returnMode = SFRM_Materialize;
+	rsinfo-setResult = tupstore;
+	rsinfo-setDesc = tupdesc;
+
+	MemoryContextSwitchTo(oldcontext);
+
+	indexRel = index_open(indexRelid, AccessShareLock);
+
+	/* minimally verify the page we got */
+	page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, regular);
+
+	bdesc = brin_build_desc(indexRel);
+	offset = FirstOffsetNumber;
+	unusedItem = false;
+	dtup = NULL;
+
+	/*
+	 * Initialize output functions for all indexed datatypes; simplifies
+	 * calling them later.
+	 */
+	columns = palloc(sizeof(brin_column_state *) * RelationGetDescr(indexRel)-natts);
+	for (attno = 1; attno = bdesc-bd_tupdesc-natts; attno++)
 	{
-		bytea	   *raw_page = PG_GETARG_BYTEA_P(0);
-		Oid			indexRelid = PG_GETARG_OID(1);
-		Page		page;
-		TupleDesc	tupdesc;
-		MemoryContext mctx;
-		Relation	indexRel;
-		AttrNumber	attno;
+		Oid			output;
+		bool		isVarlena;
+		BrinOpcInfo *opcinfo;
+		int			i;
+		brin_column_state *column;
 
-		/* minimally verify the page we got */
-		page = verify_brin_page(raw_page, BRIN_PAGETYPE_REGULAR, regular);
+		opcinfo = bdesc-bd_info[attno - 1];
+		column = palloc(offsetof(brin_column_state, outputFn) +
+		sizeof(FmgrInfo) * opcinfo-oi_nstored);
 
-		/* create a function context for cross-call persistence */
-		fctx = SRF_FIRSTCALL_INIT();
-
-		/* switch to memory context appropriate for multiple function calls */
-		mctx = MemoryContextSwitchTo(fctx-multi_call_memory_ctx);
-
-		/* Build a tuple descriptor for our result type */
-		if (get_call_result_type(fcinfo, NULL, tupdesc) != TYPEFUNC_COMPOSITE)
-			elog(ERROR, return type must be a row type);
-
-		indexRel = index_open(indexRelid, AccessShareLock);
-
-		state = palloc(offsetof(brin_page_state, columns) +
-			  sizeof(brin_column_state) * RelationGetDescr(indexRel)-natts);
-
-		state-bdesc =