From: Tom Lane <tgl@sss.pgh.pa.us>
To: Scott Marlowe <scott.marlowe@gmail.com>
cc: pgsql-performance <pgsql-performance@postgresql.org>
Subject: Re: Small performance regression in 9.2 has a big impact
In-reply-to: <23971.1416956254@sss.pgh.pa.us>
References: 
 <CAOR=d=3j1U_q-zf8+jUx1hkx8ps+N8pm=EUTqyFdJ5ov=+fawg@mail.gmail.com>
 <21247.1416952515@sss.pgh.pa.us> <23971.1416956254@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
 message dated "Tue, 25 Nov 2014 17:57:34 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <24625.1416974273.0@sss.pgh.pa.us>
Date: Tue, 25 Nov 2014 22:59:30 -0500
Message-ID: <24669.1416974370@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-performance-owner@postgresql.org

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <24625.1416974273.1@sss.pgh.pa.us>

I wrote:
>> Hmm, I don't like the trend here.  For the repeat-1000x query, I get
>> these reported execution times:

>> 8.4	360 ms
>> 9.0	365 ms
>> 9.1	440 ms
>> 9.2	510 ms
>> 9.3	550 ms
>> 9.4	570 ms
>> head	570 ms

> I made a quick-hack patch to suppress redundant GetDefaultOpclass calls
> in typcache.c, and found that that brought HEAD's runtime down to 460ms.

I found some additional low-hanging fruit by comparing gprof call counts
in 8.4 and HEAD:

* OverrideSearchPathMatchesCurrent(), which is not there at all in 8.4
or 9.2, accounts for a depressingly large amount of palloc/pfree traffic.
The implementation was quick-n-dirty to begin with, but workloads
like this one call it often enough to make it a pain point.

* plpgsql's setup_param_list() contributes another large fraction of
added palloc/pfree traffic; this is evidently caused by the temporary
bitmapset needed for its bms_first_member() loop, which was not there
in 8.4 but is there in 9.2.

I've been able to bring HEAD's runtime down to about 415 ms with the
collection of more-or-less quick hacks attached.  None of them are
ready to commit but I thought I'd post them for the record.

After review of all this, I think the aspect of your example that is
causing performance issues is that there are a lot of non-inline-able
SQL-language function calls.  That's not a case that we've put much
thought into lately.  I doubt we are going to get all the way back to
where 8.4 was in the short term, because I can see that there is a
significant amount of new computation associated with collation
management during parsing (catcache lookups driven by get_typcollation,
assign_collations_walker, etc).  The long-term answer to that is to
improve the SQL-language function support so that it can cache the results
of parsing the function body; we have surely got enough plancache support
for that now, but no one's attempted to apply it in functions.c.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="quick-performance-hacks.patch";
 charset="us-ascii"
Content-ID: <24625.1416974273.2@sss.pgh.pa.us>
Content-Description: quick-performance-hacks.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/t=
ypcache.c
index 8c6c7fc..bf80563 100644
*** a/src/backend/utils/cache/typcache.c
--- b/src/backend/utils/cache/typcache.c
*************** static HTAB *TypeCacheHash =3D NULL;
*** 77,82 ****
--- 77,84 ----
  #define TCFLAGS_CHECKED_FIELD_PROPERTIES	0x0010
  #define TCFLAGS_HAVE_FIELD_EQUALITY			0x0020
  #define TCFLAGS_HAVE_FIELD_COMPARE			0x0040
+ #define TCFLAGS_CHECKED_BTREE_OPCLASS		0x0080
+ #define TCFLAGS_CHECKED_HASH_OPCLASS		0x0100
=20=20
  /* Private information to support comparisons of enum values */
  typedef struct
*************** lookup_type_cache(Oid type_id, int flags
*** 227,249 ****
  	{
  		Oid			opclass;
=20=20
! 		opclass =3D GetDefaultOpClass(type_id, BTREE_AM_OID);
! 		if (OidIsValid(opclass))
  		{
! 			typentry->btree_opf =3D get_opclass_family(opclass);
! 			typentry->btree_opintype =3D get_opclass_input_type(opclass);
  		}
  		/* If no btree opclass, we force lookup of the hash opclass */
  		if (typentry->btree_opf =3D=3D InvalidOid)
  		{
  			if (typentry->hash_opf =3D=3D InvalidOid)
  			{
! 				opclass =3D GetDefaultOpClass(type_id, HASH_AM_OID);
! 				if (OidIsValid(opclass))
  				{
! 					typentry->hash_opf =3D get_opclass_family(opclass);
! 					typentry->hash_opintype =3D get_opclass_input_type(opclass);
  				}
  			}
  		}
  		else
--- 229,259 ----
  	{
  		Oid			opclass;
=20=20
! 		if (!(typentry->flags & TCFLAGS_CHECKED_BTREE_OPCLASS))
  		{
! 			opclass =3D GetDefaultOpClass(type_id, BTREE_AM_OID);
! 			if (OidIsValid(opclass))
! 			{
! 				typentry->btree_opf =3D get_opclass_family(opclass);
! 				typentry->btree_opintype =3D get_opclass_input_type(opclass);
! 			}
! 			typentry->flags |=3D TCFLAGS_CHECKED_BTREE_OPCLASS;
  		}
  		/* If no btree opclass, we force lookup of the hash opclass */
  		if (typentry->btree_opf =3D=3D InvalidOid)
  		{
  			if (typentry->hash_opf =3D=3D InvalidOid)
  			{
! 				if (!(typentry->flags & TCFLAGS_CHECKED_HASH_OPCLASS))
  				{
! 					opclass =3D GetDefaultOpClass(type_id, HASH_AM_OID);
! 					if (OidIsValid(opclass))
! 					{
! 						typentry->hash_opf =3D get_opclass_family(opclass);
! 						typentry->hash_opintype =3D get_opclass_input_type(opclass);
! 					}
  				}
+ 				typentry->flags |=3D TCFLAGS_CHECKED_HASH_OPCLASS;
  			}
  		}
  		else
*************** lookup_type_cache(Oid type_id, int flags
*** 268,278 ****
  	{
  		Oid			opclass;
=20=20
! 		opclass =3D GetDefaultOpClass(type_id, HASH_AM_OID);
! 		if (OidIsValid(opclass))
  		{
! 			typentry->hash_opf =3D get_opclass_family(opclass);
! 			typentry->hash_opintype =3D get_opclass_input_type(opclass);
  		}
  	}
=20=20
--- 278,292 ----
  	{
  		Oid			opclass;
=20=20
! 		if (!(typentry->flags & TCFLAGS_CHECKED_HASH_OPCLASS))
  		{
! 			opclass =3D GetDefaultOpClass(type_id, HASH_AM_OID);
! 			if (OidIsValid(opclass))
! 			{
! 				typentry->hash_opf =3D get_opclass_family(opclass);
! 				typentry->hash_opintype =3D get_opclass_input_type(opclass);
! 			}
! 			typentry->flags |=3D TCFLAGS_CHECKED_HASH_OPCLASS;
  		}
  	}
=20=20
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespac=
e.c
index 911f015..ca93307 100644
*** a/src/backend/catalog/namespace.c
--- b/src/backend/catalog/namespace.c
*************** CopyOverrideSearchPath(OverrideSearchPat
*** 3145,3164 ****
  bool
  OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
  {
! 	/* Easiest way to do this is GetOverrideSearchPath() and compare */
! 	bool		result;
! 	OverrideSearchPath *cur;
=20=20
! 	cur =3D GetOverrideSearchPath(CurrentMemoryContext);
! 	if (path->addCatalog =3D=3D cur->addCatalog &&
! 		path->addTemp =3D=3D cur->addTemp &&
! 		equal(path->schemas, cur->schemas))
! 		result =3D true;
! 	else
! 		result =3D false;
! 	list_free(cur->schemas);
! 	pfree(cur);
! 	return result;
  }
=20=20
  /*
--- 3145,3178 ----
  bool
  OverrideSearchPathMatchesCurrent(OverrideSearchPath *path)
  {
! 	ListCell *lc, *lcc;
=20=20
! 	recomputeNamespacePath();
! 	lc =3D list_head(activeSearchPath);
! 	if (path->addTemp)
! 	{
! 		if (lc && lfirst_oid(lc) =3D=3D myTempNamespace)
! 			lc =3D lnext(lc);
! 		else
! 			return false;
! 	}
! 	if (path->addCatalog)
! 	{
! 		if (lc && lfirst_oid(lc) =3D=3D PG_CATALOG_NAMESPACE)
! 			lc =3D lnext(lc);
! 		else
! 			return false;
! 	}
! 	foreach(lcc, path->schemas)
! 	{
! 		if (lc && lfirst_oid(lc) =3D=3D lfirst_oid(lcc))
! 			lc =3D lnext(lc);
! 		else
! 			return false;
! 	}
! 	if (lc)
! 		return false;
! 	return true;
  }
=20=20
  /*
diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index f770608..a78ff48 100644
*** a/src/include/nodes/bitmapset.h
--- b/src/include/nodes/bitmapset.h
*************** extern Bitmapset *bms_join(Bitmapset *a,
*** 89,94 ****
--- 89,95 ----
=20=20
  /* support for iterating through the integer elements of a set: */
  extern int	bms_first_member(Bitmapset *a);
+ extern int	bms_next_member(const Bitmapset *a, int prevbit);
=20=20
  /* support for hashtables using Bitmapsets as keys: */
  extern uint32 bms_hash_value(const Bitmapset *a);
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index c927b78..fbaf377 100644
*** a/src/backend/nodes/bitmapset.c
--- b/src/backend/nodes/bitmapset.c
*************** bms_first_member(Bitmapset *a)
*** 841,846 ****
--- 841,899 ----
  }
=20=20
  /*
+  * bms_next_member - find next member of a set
+  *
+  * Returns smallest member greater than "prevbit", or -1 if there is none.
+  *
+  * This is intended as support for iterating through the members of a set.
+  * The typical pattern is
+  *
+  *			x =3D -1;
+  *			while ((x =3D bms_next_member(inputset, x)) >=3D 0)
+  *				process member x;
+  */
+ int
+ bms_next_member(const Bitmapset *a, int prevbit)
+ {
+ 	int			nwords;
+ 	int			wordnum;
+ 	bitmapword	mask;
+=20
+ 	if (a =3D=3D NULL)
+ 		return -1;
+ 	nwords =3D a->nwords;
+ 	prevbit++;
+ 	mask =3D (~(bitmapword) 0) << BITNUM(prevbit);
+ 	for (wordnum =3D WORDNUM(prevbit); wordnum < nwords; wordnum++)
+ 	{
+ 		bitmapword	w =3D a->words[wordnum];
+=20
+ 		/* ignore bits before prevbit */
+ 		w &=3D mask;
+=20
+ 		if (w !=3D 0)
+ 		{
+ 			int			result;
+=20
+ 			w =3D RIGHTMOST_ONE(w);
+=20
+ 			result =3D wordnum * BITS_PER_BITMAPWORD;
+ 			while ((w & 255) =3D=3D 0)
+ 			{
+ 				w >>=3D 8;
+ 				result +=3D 8;
+ 			}
+ 			result +=3D rightmost_one_pos[w & 255];
+ 			return result;
+ 		}
+=20
+ 		/* in subsequent words, consider all bits */
+ 		mask =3D (~(bitmapword) 0);
+ 	}
+ 	return -1;
+ }
+=20
+ /*
   * bms_hash_value - compute a hash key for a Bitmapset
   *
   * Note: we must ensure that any two bitmapsets that are bms_equal() will
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 11cb47b..d6d414f 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5263,5269 ****
  	 */
  	if (!bms_is_empty(expr->paramnos))
  	{
- 		Bitmapset  *tmpset;
  		int			dno;
=20=20
  		paramLI =3D (ParamListInfo)
--- 5263,5268 ----
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5276,5283 ****
  		paramLI->numParams =3D estate->ndatums;
=20=20
  		/* Instantiate values for "safe" parameters of the expression */
! 		tmpset =3D bms_copy(expr->paramnos);
! 		while ((dno =3D bms_first_member(tmpset)) >=3D 0)
  		{
  			PLpgSQL_datum *datum =3D estate->datums[dno];
=20=20
--- 5275,5282 ----
  		paramLI->numParams =3D estate->ndatums;
=20=20
  		/* Instantiate values for "safe" parameters of the expression */
! 		dno =3D -1;
! 		while ((dno =3D bms_next_member(expr->paramnos, dno)) >=3D 0)
  		{
  			PLpgSQL_datum *datum =3D estate->datums[dno];
=20=20
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5292,5298 ****
  				prm->ptype =3D var->datatype->typoid;
  			}
  		}
- 		bms_free(tmpset);
=20=20
  		/*
  		 * Set up link to active expr where the hook functions can find it.
--- 5291,5296 ----

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

------- =_aaaaaaaaaa0--