Hi,

there's (at least) one point I do not understand in bufpage.h.
Could one of the "older" hackers explain it to me, please.

Is od_pagesize in any way more or less opaque than pd_lower, pd_upper,
pd_special, etc?  If it is, why?

If it's not, should I post a patch that puts pagesize directly into
PageHeaderData?

Servus
 Manfred

Thu, 20 Jun 2002 17:47:59 +0200
Manfred Koizar <mkoi-pg@aon.at>
Page OpaqueData
Manfred Koizar <mkoi-pg@aon.at> writes:
> Is od_pagesize in any way more or less opaque than pd_lower, pd_upper,
> pd_special, etc?  If it is, why?

I surmise that there was once some idea of supporting multiple page
sizes simultaneously, but it's not real clear why the macros
PageGetPageSize/PageSetPageSize wouldn't be a sufficient abstraction
layer; the extra level of struct naming for pd_opaque has no obvious
usefulness.  In any case I doubt that dealing with multiple page sizes
would be worth the trouble it would be to support.

> If it's not, should I post a patch that puts pagesize directly into
> PageHeaderData?

If you're so inclined.  Given that pd_opaque is hidden in those macros,
there wouldn't be much of any gain in readability either, so I haven't
worried about changing the declaration.

			regards, tom lane



Mon, 24 Jun 2002 12:53:42 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Page OpaqueData 
On Mon, 24 Jun 2002 12:53:42 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>the extra level of struct naming for pd_opaque has no obvious
>usefulness.
>
>> [...] should I post a patch that puts pagesize directly into
>> PageHeaderData?
>
>If you're so inclined.  Given that pd_opaque is hidden in those macros,
>there wouldn't be much of any gain in readability either, so I haven't
>worried about changing the declaration.

Thanks for the clarification.  Here is the patch.  Not much gain, but at
least it saves the next junior hacker from scratching his head ...

Cordialmente
 Manfredinho :-)

PS: Please do not apply before "Page access" patch from 2002-06-20.

diff -ru ../base/src/backend/access/hash/hashutil.c src/backend/access/hash/hashutil.c
--- ../base/src/backend/access/hash/hashutil.c	2002-05-21 11:54:11.000000000 +0200
+++ src/backend/access/hash/hashutil.c	2002-06-21 16:43:24.000000000 +0200
@@ -131,13 +131,13 @@
 	HashPageOpaque opaque;
 
 	Assert(page);
-	Assert(((PageHeader) (page))->pd_lower >= (sizeof(PageHeaderData) - sizeof(ItemIdData)));
+	Assert(((PageHeader) (page))->pd_lower >= SizeOfPageHeaderData);
 #if 1
 	Assert(((PageHeader) (page))->pd_upper <=
 		   (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData))));
 	Assert(((PageHeader) (page))->pd_special ==
 		   (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData))));
-	Assert(((PageHeader) (page))->pd_opaque.od_pagesize == BLCKSZ);
+	Assert(PageGetPageSize(page) == BLCKSZ);
 #endif
 	if (flags)
 	{
diff -ru ../base/src/include/storage/bufpage.h src/include/storage/bufpage.h
--- ../base/src/include/storage/bufpage.h	2002-06-20 12:22:21.000000000 +0200
+++ src/include/storage/bufpage.h	2002-06-21 16:38:17.000000000 +0200
@@ -65,8 +65,7 @@
  * byte-offset position, tuples can be physically shuffled on a page
  * whenever the need arises.
  *
- * AM-generic per-page information is kept in the pd_opaque field of
- * the PageHeaderData.	(Currently, only the page size is kept here.)
+ * AM-generic per-page information is kept in PageHeaderData.
  *
  * AM-specific per-page data (if any) is kept in the area marked "special
  * space"; each AM has an "opaque" structure defined somewhere that is
@@ -92,25 +91,18 @@
 
 
 /*
+ * disk page organization
  * space management information generic to any page
  *
- *		od_pagesize		- size in bytes.
- *						  Minimum possible page size is perhaps 64B to fit
- *						  page header, opaque space and a minimal tuple;
- *						  of course, in reality you want it much bigger.
- *						  On the high end, we can only support pages up
- *						  to 32KB because lp_off/lp_len are 15 bits.
- */
-typedef struct OpaqueData
-{
-	uint16		od_pagesize;
-} OpaqueData;
-
-typedef OpaqueData *Opaque;
-
-
-/*
- * disk page organization
+ *		pd_lower   	- offset to start of free space.
+ *		pd_upper   	- offset to end of free space.
+ *		pd_special 	- offset to start of special space.
+ *		pd_pagesize	- size in bytes.
+ *					  Minimum possible page size is perhaps 64B to fit
+ *					  page header, opaque space and a minimal tuple;
+ *					  of course, in reality you want it much bigger.
+ *					  On the high end, we can only support pages up
+ *					  to 32KB because lp_off/lp_len are 15 bits.
  */
 typedef struct PageHeaderData
 {
@@ -124,7 +116,7 @@
 	LocationIndex pd_lower;		/* offset to start of free space */
 	LocationIndex pd_upper;		/* offset to end of free space */
 	LocationIndex pd_special;	/* offset to start of special space */
-	OpaqueData	pd_opaque;		/* AM-generic information */
+	uint16		pd_pagesize;
 	ItemIdData	pd_linp[1];		/* beginning of line pointer array */
 } PageHeaderData;
 
@@ -216,14 +208,14 @@
  * however, it can be called on a page for which there is no buffer.
  */
 #define PageGetPageSize(page) \
-	((Size) ((PageHeader) (page))->pd_opaque.od_pagesize)
+	((Size) ((PageHeader) (page))->pd_pagesize)
 
 /*
  * PageSetPageSize
  *		Sets the page size of a page.
  */
 #define PageSetPageSize(page, size) \
-	(((PageHeader) (page))->pd_opaque.od_pagesize = (size))
+	(((PageHeader) (page))->pd_pagesize = (size))
 
 /* ----------------
  *		page special data macros

Servus
 Manfred



Wed, 26 Jun 2002 16:38:47 +0200
Manfred Koizar <mkoi-pg@aon.at>
Re: [HACKERS] Page OpaqueData 

Patch applied.  Thanks.

---------------------------------------------------------------------------


 Manfred Koizar wrote:
> On Mon, 24 Jun 2002 12:53:42 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >the extra level of struct naming for pd_opaque has no obvious
> >usefulness.
> >
> >> [...] should I post a patch that puts pagesize directly into
> >> PageHeaderData?
> >
> >If you're so inclined.  Given that pd_opaque is hidden in those macros,
> >there wouldn't be much of any gain in readability either, so I haven't
> >worried about changing the declaration.
> 
> Thanks for the clarification.  Here is the patch.  Not much gain, but at
> least it saves the next junior hacker from scratching his head ...
> 
> Cordialmente
>  Manfredinho :-)
> 
> PS: Please do not apply before "Page access" patch from 2002-06-20.
> 
> diff -ru ../base/src/backend/access/hash/hashutil.c src/backend/access/hash/hashutil.c
> --- ../base/src/backend/access/hash/hashutil.c	2002-05-21 11:54:11.000000000 +0200
> +++ src/backend/access/hash/hashutil.c	2002-06-21 16:43:24.000000000 +0200
> @@ -131,13 +131,13 @@
>  	HashPageOpaque opaque;
>  
>  	Assert(page);
> -	Assert(((PageHeader) (page))->pd_lower >= (sizeof(PageHeaderData) - sizeof(ItemIdData)));
> +	Assert(((PageHeader) (page))->pd_lower >= SizeOfPageHeaderData);
>  #if 1
>  	Assert(((PageHeader) (page))->pd_upper <=
>  		   (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData))));
>  	Assert(((PageHeader) (page))->pd_special ==
>  		   (BLCKSZ - MAXALIGN(sizeof(HashPageOpaqueData))));
> -	Assert(((PageHeader) (page))->pd_opaque.od_pagesize == BLCKSZ);
> +	Assert(PageGetPageSize(page) == BLCKSZ);
>  #endif
>  	if (flags)
>  	{
> diff -ru ../base/src/include/storage/bufpage.h src/include/storage/bufpage.h
> --- ../base/src/include/storage/bufpage.h	2002-06-20 12:22:21.000000000 +0200
> +++ src/include/storage/bufpage.h	2002-06-21 16:38:17.000000000 +0200
> @@ -65,8 +65,7 @@
>   * byte-offset position, tuples can be physically shuffled on a page
>   * whenever the need arises.
>   *
> - * AM-generic per-page information is kept in the pd_opaque field of
> - * the PageHeaderData.	(Currently, only the page size is kept here.)
> + * AM-generic per-page information is kept in PageHeaderData.
>   *
>   * AM-specific per-page data (if any) is kept in the area marked "special
>   * space"; each AM has an "opaque" structure defined somewhere that is
> @@ -92,25 +91,18 @@
>  
>  
>  /*
> + * disk page organization
>   * space management information generic to any page
>   *
> - *		od_pagesize		- size in bytes.
> - *						  Minimum possible page size is perhaps 64B to fit
> - *						  page header, opaque space and a minimal tuple;
> - *						  of course, in reality you want it much bigger.
> - *						  On the high end, we can only support pages up
> - *						  to 32KB because lp_off/lp_len are 15 bits.
> - */
> -typedef struct OpaqueData
> -{
> -	uint16		od_pagesize;
> -} OpaqueData;
> -
> -typedef OpaqueData *Opaque;
> -
> -
> -/*
> - * disk page organization
> + *		pd_lower   	- offset to start of free space.
> + *		pd_upper   	- offset to end of free space.
> + *		pd_special 	- offset to start of special space.
> + *		pd_pagesize	- size in bytes.
> + *					  Minimum possible page size is perhaps 64B to fit
> + *					  page header, opaque space and a minimal tuple;
> + *					  of course, in reality you want it much bigger.
> + *					  On the high end, we can only support pages up
> + *					  to 32KB because lp_off/lp_len are 15 bits.
>   */
>  typedef struct PageHeaderData
>  {
> @@ -124,7 +116,7 @@
>  	LocationIndex pd_lower;		/* offset to start of free space */
>  	LocationIndex pd_upper;		/* offset to end of free space */
>  	LocationIndex pd_special;	/* offset to start of special space */
> -	OpaqueData	pd_opaque;		/* AM-generic information */
> +	uint16		pd_pagesize;
>  	ItemIdData	pd_linp[1];		/* beginning of line pointer array */
>  } PageHeaderData;
>  
> @@ -216,14 +208,14 @@
>   * however, it can be called on a page for which there is no buffer.
>   */
>  #define PageGetPageSize(page) \
> -	((Size) ((PageHeader) (page))->pd_opaque.od_pagesize)
> +	((Size) ((PageHeader) (page))->pd_pagesize)
>  
>  /*
>   * PageSetPageSize
>   *		Sets the page size of a page.
>   */
>  #define PageSetPageSize(page, size) \
> -	(((PageHeader) (page))->pd_opaque.od_pagesize = (size))
> +	(((PageHeader) (page))->pd_pagesize = (size))
>  
>  /* ----------------
>   *		page special data macros
> 
> Servus
>  Manfred
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
> http://www.postgresql.org/users-lounge/docs/faq.html
> 
> 
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026



Tue, 2 Jul 2002 02:18:43 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] Page OpaqueData