Code review for magic-block patch. Remove separate header file pgmagic.h,

as this seems only likely to create headaches for module developers.  Put
the macro in the pre-existing fmgr.h file instead.  Avoid being too cute
about how many fields we can cram into a word, and avoid trying to fetch
from a library we've already unlinked.
Along the way, it occurred to me that the magic block really ought to be
'const' so it can be stored in the program text area.  Do the same for
the existing data blocks for PG_FUNCTION_INFO_V1 functions.

doc/src/sgml/xfunc.sgml

-<!-- $PostgreSQL: pgsql/doc/src/sgml/xfunc.sgml,v 1.113 2006/05/30 14:09:32 momjian Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/xfunc.sgml,v 1.114 2006/05/30 21:21:29 tgl Exp $ -->
 
  <sect1 id="xfunc">
   <title>User-Defined Functions</title>
@@ -1148,13 +1148,6 @@ CREATE FUNCTION square_root(double precision) RETURNS double precision
     that fails as well, the load will fail.
    </para>
 
-   <para>
-    After the module has been found, PostgreSQL looks for its magic block.
-    This block contains information about the environment a module was
-    compiled in. The server uses this to verify the module was compiled
-    under the same assumptions and environment as the backend.
-   </para>
-
    <para>
     The user ID the <productname>PostgreSQL</productname> server runs
     as must be able to traverse the path to the file you intend to
@@ -1960,31 +1953,36 @@ concat_text(PG_FUNCTION_ARGS)
 
       <listitem>
        <para>
-        To ensure your module is not loaded into an incompatible backend, it
-        is recommended to include a magic block. To do this you must include
-        the header <filename>pgmagic.h</filename> and declare the block as
-        follows:
+        Symbol names defined within object files must not conflict
+        with each other or with symbols defined in the
+        <productname>PostgreSQL</productname> server executable.  You
+        will have to rename your functions or variables if you get
+        error messages to this effect.
        </para>
+      </listitem>
 
-<programlisting>
-#include "pgmagic.h"
+      <listitem>
+       <para>
+        To ensure your module is not loaded into an incompatible server, it
+        is recommended to include a <quote>magic block</>.  This allows
+        the server to detect obvious incompatibilities, such as a module
+        compiled for a different major version of
+        <productname>PostgreSQL</productname>.  It is likely that magic
+        blocks will be required in future releases.  To include a magic
+        block, write this in one (and only one) of your module source files,
+        after having included the header <filename>fmgr.h</>:
+       </para>
 
+<programlisting>
+#ifdef PG_MODULE_MAGIC
 PG_MODULE_MAGIC;
+#endif
 </programlisting>
 
        <para>
-        If the module consists of multiple source files, this only needs to
-        be done in one of them.
-       </para>
-      </listitem>
-
-      <listitem>
-       <para>
-        Symbol names defined within object files must not conflict
-        with each other or with symbols defined in the
-        <productname>PostgreSQL</productname> server executable.  You
-        will have to rename your functions or variables if you get
-        error messages to this effect.
+        The <literal>#ifdef</> test can be omitted if your code doesn't
+        need to compile against pre-8.2 <productname>PostgreSQL</productname>
+        releases.
        </para>
       </listitem>
 

src/backend/utils/fmgr/dfmgr.c

@@ -8,19 +8,18 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/dfmgr.c,v 1.83 2006/05/30 14:09:32 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/dfmgr.c,v 1.84 2006/05/30 21:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
 
-#include <errno.h>
 #include <sys/stat.h>
 
 #include "dynloader.h"
 #include "miscadmin.h"
 #include "utils/dynamic_loader.h"
-#include "pgmagic.h"
+
 
 /*
  * List of dynamically loaded files (kept in malloc'd memory).
@@ -61,7 +60,8 @@ static char *expand_dynamic_library_name(const char *name);
 static char *substitute_libpath_macro(const char *name);
 
 /* Magic structure that module needs to match to be accepted */
-static Pg_magic_struct magic_data = PG_MODULE_MAGIC_DATA;
+static const Pg_magic_struct magic_data = PG_MODULE_MAGIC_DATA;
+
 
 /*
  * Load the specified dynamic-link library file, and look for a function
@@ -82,6 +82,7 @@ load_external_function(char *filename, char *funcname,
 {
 	DynamicFileList *file_scanner;
 	PGFunction	retval;
+	PGModuleMagicFunction magic_func;
 	char	   *load_error;
 	struct stat stat_buf;
 	char	   *fullname;
@@ -119,7 +120,6 @@ load_external_function(char *filename, char *funcname,
 
 	if (file_scanner == NULL)
 	{
-		PGModuleMagicFunction magic_func;
 		/*
 		 * File not loaded yet.
 		 */
@@ -150,44 +150,53 @@ load_external_function(char *filename, char *funcname,
 							fullname, load_error)));
 		}
 
-		/* Check the magic function to determine compatability */
-		magic_func = pg_dlsym( file_scanner->handle, PG_MAGIC_FUNCTION_NAME_STRING );
-		if( magic_func )
+		/* Check the magic function to determine compatibility */
+		magic_func = (PGModuleMagicFunction)
+			pg_dlsym(file_scanner->handle, PG_MAGIC_FUNCTION_NAME_STRING);
+		if (magic_func)
 		{
-			Pg_magic_struct *module_magic_data = magic_func();
-			if( module_magic_data->len != magic_data.len ||
-			    memcmp( module_magic_data, &magic_data, magic_data.len ) != 0 )
+			const Pg_magic_struct *magic_data_ptr = (*magic_func) ();
+
+			if (magic_data_ptr->len != magic_data.len ||
+			    memcmp(magic_data_ptr, &magic_data, magic_data.len) != 0)
 			{
-				pg_dlclose( file_scanner->handle );
-				
-				if( module_magic_data->len != magic_data.len )
+				/* copy data block before unlinking library */
+				Pg_magic_struct module_magic_data = *magic_data_ptr;
+
+				/* try to unlink library */
+				pg_dlclose(file_scanner->handle);
+				free((char *) file_scanner);
+
+				/*
+				 * Report suitable error.  It's probably not worth writing
+				 * a separate error message for each field; only the most
+				 * common case of wrong major version gets its own message.
+				 */
+				if (module_magic_data.version != magic_data.version)
 					ereport(ERROR,
-						(errmsg("incompatible library \"%s\": Magic block length mismatch",
-								fullname)));
-				if( module_magic_data->version != magic_data.version )
-					ereport(ERROR,
-						(errmsg("incompatible library \"%s\": Version mismatch",
-								fullname),
-						 errdetail("Expected %d.%d, got %d.%d", 
-						 	magic_data.version/100, magic_data.version % 100,
-						 	module_magic_data->version/100, module_magic_data->version % 100)));
-						 	
-				if( module_magic_data->magic != magic_data.magic )
-					ereport(ERROR,
-						(errmsg("incompatible library \"%s\": Magic constant mismatch",
-								fullname),
-					 errdetail("Expected 0x%08X, got 0x%08X", 
-						magic_data.magic, magic_data.magic)));
-				/* Should never get here */
-				ereport(ERROR,(errmsg("incompatible library \"%s\": Reason unknown",
+							(errmsg("incompatible library \"%s\": version mismatch",
+									fullname),
+							 errdetail("Server is version %d.%d, library is version %d.%d.",
+									   magic_data.version/100,
+									   magic_data.version % 100,
+									   module_magic_data.version/100,
+									   module_magic_data.version % 100)));
+				ereport(ERROR,
+						(errmsg("incompatible library \"%s\": magic block mismatch",
 								fullname)));
 			}
 		}
 		else
-		/* Currently we do not penalize modules for not having a
-		   magic block, it would break every external module in
-		   existance. At some point though... */
-			ereport(LOG, (errmsg("external library \"%s\" did not have magic block", fullname )));
+		{
+			/*
+			 * Currently we do not reject modules for not having a
+			 * magic block, it would break every external module in
+			 * existence. At some point though, this will become an ERROR.
+			 */
+			ereport(LOG,
+					(errmsg("library \"%s\" does not have a magic block",
+							fullname)));
+		}
 		
 		/* OK to link it into list */
 		if (file_list == NULL)

src/backend/utils/fmgr/fmgr.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/fmgr.c,v 1.100 2006/04/04 19:35:36 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/fmgr/fmgr.c,v 1.101 2006/05/30 21:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -66,7 +66,7 @@ typedef struct
 	TransactionId fn_xmin;		/* for checking up-to-dateness */
 	CommandId	fn_cmin;
 	PGFunction	user_fn;		/* the function's address */
-	Pg_finfo_record *inforec;	/* address of its info record */
+	const Pg_finfo_record *inforec;		/* address of its info record */
 } CFuncHashTabEntry;
 
 static HTAB *CFuncHash = NULL;
@@ -78,7 +78,7 @@ static void fmgr_info_C_lang(Oid functionId, FmgrInfo *finfo, HeapTuple procedur
 static void fmgr_info_other_lang(Oid functionId, FmgrInfo *finfo, HeapTuple procedureTuple);
 static CFuncHashTabEntry *lookup_C_func(HeapTuple procedureTuple);
 static void record_C_func(HeapTuple procedureTuple,
-			  PGFunction user_fn, Pg_finfo_record *inforec);
+			  PGFunction user_fn, const Pg_finfo_record *inforec);
 static Datum fmgr_oldstyle(PG_FUNCTION_ARGS);
 static Datum fmgr_security_definer(PG_FUNCTION_ARGS);
 
@@ -276,7 +276,7 @@ fmgr_info_C_lang(Oid functionId, FmgrInfo *finfo, HeapTuple procedureTuple)
 	Form_pg_proc procedureStruct = (Form_pg_proc) GETSTRUCT(procedureTuple);
 	CFuncHashTabEntry *hashentry;
 	PGFunction	user_fn;
-	Pg_finfo_record *inforec;
+	const Pg_finfo_record *inforec;
 	Oldstyle_fnextra *fnextra;
 	bool		isnull;
 	int			i;
@@ -405,12 +405,12 @@ fmgr_info_other_lang(Oid functionId, FmgrInfo *finfo, HeapTuple procedureTuple)
  * can validate the information record for a function not yet entered into
  * pg_proc.
  */
-Pg_finfo_record *
+const Pg_finfo_record *
 fetch_finfo_record(void *filehandle, char *funcname)
 {
 	char	   *infofuncname;
 	PGFInfoFunction infofunc;
-	Pg_finfo_record *inforec;
+	const Pg_finfo_record *inforec;
 	static Pg_finfo_record default_inforec = {0};
 
 	/* Compute name of info func */
@@ -493,7 +493,7 @@ lookup_C_func(HeapTuple procedureTuple)
  */
 static void
 record_C_func(HeapTuple procedureTuple,
-			  PGFunction user_fn, Pg_finfo_record *inforec)
+			  PGFunction user_fn, const Pg_finfo_record *inforec)
 {
 	Oid			fn_oid = HeapTupleGetOid(procedureTuple);
 	CFuncHashTabEntry *entry;

src/include/fmgr.h

@@ -11,7 +11,7 @@
  * Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/fmgr.h,v 1.43 2006/04/04 19:35:37 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/fmgr.h,v 1.44 2006/05/30 21:21:30 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -293,7 +293,7 @@ typedef struct
 } Pg_finfo_record;
 
 /* Expected signature of an info function */
-typedef Pg_finfo_record *(*PGFInfoFunction) (void);
+typedef const Pg_finfo_record *(*PGFInfoFunction) (void);
 
 /*
  *	Macro to build an info function associated with the given function name.
@@ -301,16 +301,75 @@ typedef Pg_finfo_record *(*PGFInfoFunction) (void);
  *	doesn't hurt to add DLLIMPORT in case they don't.
  */
 #define PG_FUNCTION_INFO_V1(funcname) \
-extern DLLIMPORT Pg_finfo_record * CppConcat(pg_finfo_,funcname) (void); \
-Pg_finfo_record * \
+extern DLLIMPORT const Pg_finfo_record * CppConcat(pg_finfo_,funcname)(void); \
+const Pg_finfo_record * \
 CppConcat(pg_finfo_,funcname) (void) \
 { \
-	static Pg_finfo_record my_finfo = { 1 }; \
+	static const Pg_finfo_record my_finfo = { 1 }; \
 	return &my_finfo; \
 } \
 extern int no_such_variable
 
 
+/*-------------------------------------------------------------------------
+ *		Support for verifying backend compatibility of loaded modules
+ *
+ * If a loaded module includes the macro call
+ *		PG_MODULE_MAGIC;
+ * (put this in only one source file), then we can check for obvious
+ * incompatibility, such as being compiled for a different major PostgreSQL
+ * version.
+ *
+ * To compile with versions of PostgreSQL that do not support this,
+ * you may put an #ifdef/#endif test around it.
+ * 
+ * The specific items included in the magic block are intended to be ones that
+ * are custom-configurable and especially likely to break dynamically loaded
+ * modules if they were compiled with other values.  Also, the length field
+ * can be used to detect definition changes.
+ *-------------------------------------------------------------------------
+ */
+
+/* Definition of the magic block structure */
+typedef struct
+{
+	int		len;				/* sizeof(this struct) */
+	int		version;			/* PostgreSQL major version */
+	int		funcmaxargs;		/* FUNC_MAX_ARGS */
+	int		indexmaxkeys;		/* INDEX_MAX_KEYS */
+	int		namedatalen;		/* NAMEDATALEN */
+} Pg_magic_struct;
+
+/* The actual data block contents */
+#define PG_MODULE_MAGIC_DATA \
+{ \
+	sizeof(Pg_magic_struct), \
+	PG_VERSION_NUM / 100, \
+	FUNC_MAX_ARGS, \
+	INDEX_MAX_KEYS, \
+	NAMEDATALEN \
+}
+
+/*
+ * Declare the module magic function.  It needs to be a function as the dlsym
+ * in the backend is only guaranteed to work on functions, not data
+ */
+typedef const Pg_magic_struct *(*PGModuleMagicFunction) (void);
+
+#define PG_MAGIC_FUNCTION_NAME Pg_magic_func
+#define PG_MAGIC_FUNCTION_NAME_STRING "Pg_magic_func"
+
+#define PG_MODULE_MAGIC \
+extern DLLIMPORT const Pg_magic_struct *PG_MAGIC_FUNCTION_NAME(void); \
+const Pg_magic_struct * \
+PG_MAGIC_FUNCTION_NAME(void) \
+{ \
+	static const Pg_magic_struct Pg_magic_data = PG_MODULE_MAGIC_DATA; \
+	return &Pg_magic_data; \
+} \
+extern int no_such_variable
+
+
 /*-------------------------------------------------------------------------
  *		Support routines and macros for callers of fmgr-compatible functions
  *-------------------------------------------------------------------------
@@ -414,7 +473,7 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val);
 /*
  * Routines in fmgr.c
  */
-extern Pg_finfo_record *fetch_finfo_record(void *filehandle, char *funcname);
+extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, char *funcname);
 extern void clear_external_function_hash(void *filehandle);
 extern Oid	fmgr_internal_function(const char *proname);
 extern Oid	get_fn_expr_rettype(FmgrInfo *flinfo);

src/include/pgmagic.h

-/*-------------------------------------------------------------------------
- *
- * pgmagic.h
- *	Defines a magic block that can mark a module in a way so show that
- *      it is compatible with the server it is being loaded into.
- *
- * This file is intended to be included into modules that wish to load
- * themselves into the backend. All they need to do is include this header
- * into one of the source files and include the line:
- *
- * PG_MODULE_MAGIC;
- *
- * The trailing semi-colon is optional. To work with versions of PostgreSQL
- * that do not support this, you may put an #ifdef/endif block around it.
- *
- * Note, there is space available, particularly in the bitfield part. If it
- * turns out that a change has happened within a major release that would
- * require all modules to be recompiled, just setting one unused bit there
- * will do the trick.
- *
- * Originally written by Martijn van Oosterhout <kleptog@svana.org>
- *
- * $PostgreSQL: pgsql/src/include/pgmagic.h,v 1.1 2006/05/30 14:09:32 momjian Exp $
- *
- *-------------------------------------------------------------------------
- */
- 
-#ifndef PGMAGIC_H
-#define PGMAGIC_H
-
-#include "c.h"
-
-/* The main structure in which the magic is stored. the length field is used
- * to detect major changes */
-
-typedef struct {
-  int len;
-  int version;
-  int magic;
-} Pg_magic_struct;
-
-/* Declare the module magic function. It needs to be a function as the dlsym
- * in the backend is only guarenteed to work on functions, not data */
-
-typedef Pg_magic_struct *(*PGModuleMagicFunction) (void);
-
-#define PG_MAGIC_FUNCTION_NAME Pg_magic_func
-#define PG_MAGIC_FUNCTION_NAME_STRING "Pg_magic_func"
-
-#define PG_MODULE_MAGIC     \
-extern DLLIMPORT Pg_magic_struct *PG_MAGIC_FUNCTION_NAME(void);       \
-Pg_magic_struct *                                            \
-PG_MAGIC_FUNCTION_NAME(void) \
-{ \
-  static Pg_magic_struct Pg_magic_data = PG_MODULE_MAGIC_DATA; \
-  return &Pg_magic_data; \
-}
-
-    /* Common user adjustable constants */
-#define PG_MODULE_MAGIC_CONST \
-   ((INDEX_MAX_KEYS <<  0) +                                    \
-    (FUNC_MAX_ARGS  <<  8) +                                    \
-    (NAMEDATALEN    << 16))
-
-/* Finally, the actual data block */
-#define PG_MODULE_MAGIC_DATA                                    \
-{                                                               \
-  sizeof(Pg_magic_struct),                                      \
-  PG_VERSION_NUM / 100,       /* Major version of postgres */   \
-  PG_MODULE_MAGIC_CONST,   /* Constants users can configure */  \
-}
-
-#endif  /* PGMAGIC_H */

src/test/regress/regress.c

 /*
- * $PostgreSQL: pgsql/src/test/regress/regress.c,v 1.66 2006/05/30 14:09:32 momjian Exp $
+ * $PostgreSQL: pgsql/src/test/regress/regress.c,v 1.67 2006/05/30 21:21:30 tgl Exp $
  */
 
 #include "postgres.h"
@@ -9,7 +9,6 @@
 #include "utils/geo_decls.h"	/* includes <math.h> */
 #include "executor/executor.h"	/* For GetAttributeByName */
 #include "commands/sequence.h"	/* for nextval() */
-#include "pgmagic.h"
 
 #define P_MAXDIG 12
 #define LDELIM			'('
@@ -28,7 +27,11 @@ extern int	oldstyle_length(int n, text *t);
 extern Datum int44in(PG_FUNCTION_ARGS);
 extern Datum int44out(PG_FUNCTION_ARGS);
 
+#ifdef PG_MODULE_MAGIC
 PG_MODULE_MAGIC;
+#endif
+
+
 /*
  * Distance from a point to a path
  */