Subject: Re: [HACKERS] get_relation_stats_hook()
From: Simon Riggs <simon@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@commandprompt.com>,
	List pgsql-patches <pgsql-patches@postgresql.org>
In-Reply-To: <17759.1214499056@sss.pgh.pa.us>
References: <1214490037.3845.136.camel@ebony.site>
	<16415.1214493494@sss.pgh.pa.us> <1214496170.3845.196.camel@ebony.site>
	<17759.1214499056@sss.pgh.pa.us>
Content-Type: multipart/mixed; boundary="=-r50m2sXE/vO/x9eF6iRy"
Date: Mon, 30 Jun 2008 21:34:16 +0100
Message-Id: <1214858056.3845.514.camel@ebony.site>
Mime-Version: 1.0


--=-r50m2sXE/vO/x9eF6iRy
Content-Type: text/plain
Content-Transfer-Encoding: 7bit


On Thu, 2008-06-26 at 12:50 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndquadrant.com> writes:
> >> Surely you didn't mean ALL calls.  Please be more specific about what
> >> you're proposing.
> 
> > The statistics relation STATRELATT is accessed in a few places in the
> > planner. Since it is in the syscache it is accessed directly from there.
> > I would like to add hooks so that stats data can come from somewhere
> > else other than the syscache for tables, just as we can already do with
> > get_relation_stats_hook().
> 
> Well, defining the hooks as replacing STATRELATT lookups seems the wrong
> level of abstraction.  What if you want to insert stats that can't be
> represented by the current pg_statistic definition?  Even if there's no
> functional limitation, cons'ing up a dummy pg_statistic tuple seems the
> hard way to do it --- we didn't define get_relation_info_hook as working
> by supplying made-up catalog rows.  Furthermore, many of the interesting
> cases that someone might want to hook into are code paths that don't
> even try to look up a pg_statistic row because they know there won't be
> one, such as two out of the three cases in examine_variable().
> 
> I think you need to move up a level, and perhaps refactor some of the
> existing code to make it easier to inject made-up stats.

I've looked at ways of refactoring this, but the hooks provided here
complement the existing functionality fairly well. 

We could add per column info to RelOptInfo, though toasted stats data is
always going to be difficult to handle.

There are other ways of doing this
1. replace selfuncs directly via the catalog
2. put in a hook for each selfunc at top level
3. decide an abstract API we would like to support 

1 is possible now, 2 can still be done whether or not this patch is
accepted and I will likely submit a patch for that also, though 3 is too
open ended to pursue right now, IMHO.

Also, this patch can be backpatched more easily, to allow helping
current issues.

-- 
 Simon Riggs           www.2ndQuadrant.com
 PostgreSQL Training, Services and Support

--=-r50m2sXE/vO/x9eF6iRy
Content-Disposition: attachment; filename=plan_hooks.v2.patch
Content-Type: text/x-patch; name=plan_hooks.v2.patch; charset=UTF-8
Content-Transfer-Encoding: 7bit

Index: src/backend/optimizer/util/plancat.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/optimizer/util/plancat.c,v
retrieving revision 1.146
diff -c -r1.146 plancat.c
*** src/backend/optimizer/util/plancat.c	12 May 2008 00:00:49 -0000	1.146
--- src/backend/optimizer/util/plancat.c	30 Jun 2008 19:52:27 -0000
***************
*** 27,32 ****
--- 27,33 ----
  #include "nodes/makefuncs.h"
  #include "optimizer/clauses.h"
  #include "optimizer/plancat.h"
+ #include "optimizer/planhook.h"
  #include "optimizer/predtest.h"
  #include "optimizer/prep.h"
  #include "parser/parse_expr.h"
***************
*** 45,52 ****
  /* GUC parameter */
  bool		constraint_exclusion = false;
  
! /* Hook for plugins to get control in get_relation_info() */
! get_relation_info_hook_type get_relation_info_hook = NULL;
  
  
  static List *get_relation_constraints(PlannerInfo *root,
--- 46,55 ----
  /* GUC parameter */
  bool		constraint_exclusion = false;
  
! /* Hook for plugins to get planner info, defined in planhook.h */
! get_relation_info_hook_type 		get_relation_info_hook = NULL;
! get_relation_stats_hook_type 		get_relation_stats_hook = NULL;
! get_relation_avg_width_hook_type 	get_relation_avg_width_hook = NULL;
  
  
  static List *get_relation_constraints(PlannerInfo *root,
Index: src/backend/utils/adt/selfuncs.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/utils/adt/selfuncs.c,v
retrieving revision 1.249
diff -c -r1.249 selfuncs.c
*** src/backend/utils/adt/selfuncs.c	12 May 2008 00:00:51 -0000	1.249
--- src/backend/utils/adt/selfuncs.c	30 Jun 2008 19:52:52 -0000
***************
*** 87,92 ****
--- 87,93 ----
  #include "optimizer/pathnode.h"
  #include "optimizer/paths.h"
  #include "optimizer/plancat.h"
+ #include "optimizer/planhook.h"
  #include "optimizer/predtest.h"
  #include "optimizer/restrictinfo.h"
  #include "optimizer/var.h"
***************
*** 3749,3755 ****
  		}
  		else if (rte->rtekind == RTE_RELATION)
  		{
! 			vardata->statsTuple = SearchSysCache(STATRELATT,
  												 ObjectIdGetDatum(rte->relid),
  												 Int16GetDatum(var->varattno),
  												 0, 0);
--- 3750,3761 ----
  		}
  		else if (rte->rtekind == RTE_RELATION)
  		{
! 			if (get_relation_stats_hook)
! 				vardata->statsTuple = (*get_relation_stats_hook) 
! 												(ObjectIdGetDatum(rte->relid),
! 												 Int16GetDatum(var->varattno));
! 			else
! 				vardata->statsTuple = SearchSysCache(STATRELATT,
  												 ObjectIdGetDatum(rte->relid),
  												 Int16GetDatum(var->varattno),
  												 0, 0);
***************
*** 3869,3878 ****
  							index->indpred == NIL)
  							vardata->isunique = true;
  						/* Has it got stats? */
! 						vardata->statsTuple = SearchSysCache(STATRELATT,
! 										   ObjectIdGetDatum(index->indexoid),
! 													  Int16GetDatum(pos + 1),
! 															 0, 0);
  						if (vardata->statsTuple)
  							break;
  					}
--- 3875,3889 ----
  							index->indpred == NIL)
  							vardata->isunique = true;
  						/* Has it got stats? */
! 						if (get_relation_stats_hook)
! 							vardata->statsTuple = (*get_relation_stats_hook) 
! 														(ObjectIdGetDatum(index->indexoid),
! 														 Int16GetDatum(pos + 1));
! 						else
! 							vardata->statsTuple = SearchSysCache(STATRELATT,
! 														 ObjectIdGetDatum(index->indexoid),
! 														 Int16GetDatum(pos + 1),
! 														 0, 0);
  						if (vardata->statsTuple)
  							break;
  					}
***************
*** 5507,5516 ****
  		colnum = 1;
  	}
  
! 	tuple = SearchSysCache(STATRELATT,
! 						   ObjectIdGetDatum(relid),
! 						   Int16GetDatum(colnum),
! 						   0, 0);
  
  	if (HeapTupleIsValid(tuple))
  	{
--- 5518,5532 ----
  		colnum = 1;
  	}
  
! 	if (get_relation_stats_hook)
! 		tuple = (*get_relation_stats_hook) (
! 							ObjectIdGetDatum(relid),
! 							Int16GetDatum(colnum));
! 	else
! 		tuple = SearchSysCache(STATRELATT,
! 							ObjectIdGetDatum(relid),
! 							Int16GetDatum(colnum),
! 							0, 0);
  
  	if (HeapTupleIsValid(tuple))
  	{
Index: src/backend/utils/cache/lsyscache.c
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/backend/utils/cache/lsyscache.c,v
retrieving revision 1.157
diff -c -r1.157 lsyscache.c
*** src/backend/utils/cache/lsyscache.c	13 Apr 2008 20:51:21 -0000	1.157
--- src/backend/utils/cache/lsyscache.c	30 Jun 2008 19:54:46 -0000
***************
*** 27,32 ****
--- 27,33 ----
  #include "catalog/pg_proc.h"
  #include "catalog/pg_statistic.h"
  #include "catalog/pg_type.h"
+ #include "optimizer/planhook.h"
  #include "miscadmin.h"
  #include "nodes/makefuncs.h"
  #include "utils/array.h"
***************
*** 2457,2462 ****
--- 2458,2468 ----
  {
  	HeapTuple	tp;
  
+ 	if (get_relation_avg_width_hook)
+ 		return (*get_relation_avg_width_hook) (
+ 						ObjectIdGetDatum(relid),
+ 						Int16GetDatum(attnum));
+ 
  	tp = SearchSysCache(STATRELATT,
  						ObjectIdGetDatum(relid),
  						Int16GetDatum(attnum),
Index: src/include/optimizer/plancat.h
===================================================================
RCS file: /home/sriggs/pg/REPOSITORY/pgsql/src/include/optimizer/plancat.h,v
retrieving revision 1.49
diff -c -r1.49 plancat.h
*** src/include/optimizer/plancat.h	1 Apr 2008 00:48:33 -0000	1.49
--- src/include/optimizer/plancat.h	30 Jun 2008 13:41:48 -0000
***************
*** 17,30 ****
  #include "nodes/relation.h"
  #include "utils/rel.h"
  
- /* Hook for plugins to get control in get_relation_info() */
- typedef void (*get_relation_info_hook_type) (PlannerInfo *root,
- 														 Oid relationObjectId,
- 														 bool inhparent,
- 														 RelOptInfo *rel);
- extern PGDLLIMPORT get_relation_info_hook_type get_relation_info_hook;
- 
- 
  extern void get_relation_info(PlannerInfo *root, Oid relationObjectId,
  				  bool inhparent, RelOptInfo *rel);
  
--- 17,22 ----

--=-r50m2sXE/vO/x9eF6iRy
Content-Disposition: attachment; filename=planhook.h
Content-Type: text/x-chdr; name=planhook.h; charset=UTF-8
Content-Transfer-Encoding: 7bit

/*-------------------------------------------------------------------------
 *
 * planhook.h
 *	  prototypes for plan hooks
 *
 *
 * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * $PostgreSQL: pgsql/src/include/optimizer/planhook.h,v 1.48 2008/03/15 20:46:31 tgl Exp $
 *
 *-------------------------------------------------------------------------
 */
#ifndef PLANHOOK_H
#define PLANHOOK_H

#include "access/htup.h"
#include "nodes/relation.h"
#include "utils/rel.h"

/* Hook for plugins to get control in get_relation_info() */
typedef void (*get_relation_info_hook_type) (PlannerInfo *root,
														 Oid relationObjectId,
														 bool inhparent,
														 RelOptInfo *rel);
extern PGDLLIMPORT get_relation_info_hook_type get_relation_info_hook;

/*
 * The following hooks are called per-attribute in later stages of the
 * planner because this detailed information is not stored in the RelOptInfo 
 * data structure. The APIs emulate stats data access from pg_statistic,
 * rather than attempting a more abstract form.
 */
typedef HeapTuple (*get_relation_stats_hook_type) (Datum relid, Datum attnum);
extern PGDLLIMPORT get_relation_stats_hook_type get_relation_stats_hook;

typedef int32 (*get_relation_avg_width_hook_type) (Datum relid, Datum attnum);
extern PGDLLIMPORT get_relation_avg_width_hook_type get_relation_avg_width_hook;

#endif   /* PLANHOOK_H */

--=-r50m2sXE/vO/x9eF6iRy--