Re: [PATCHES] ScanDirections
James William Pye wrote:
In any case, some parts of the patch merely makes some code make use of the
sdir.h macros instead of depending on -1/0/1--heapam in particular. At least
those portions should be applied, no?
I've applied the attached patch to CVS HEAD that just changes the code
to use the sdir.h macros when appropriate. Thanks for the patch.
-Neil
Index: src/backend/access/heap/heapam.c
===
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/backend/access/heap/heapam.c,v
retrieving revision 1.206
diff -c -r1.206 heapam.c
*** src/backend/access/heap/heapam.c 11 Jan 2006 08:43:11 - 1.206
--- src/backend/access/heap/heapam.c 21 Feb 2006 22:28:35 -
***
*** 172,178
* tuple as indicated by dir; return the next tuple in scan-rs_ctup,
* or set scan-rs_ctup.t_data = NULL if no more tuples.
*
! * dir == 0 means re-fetch the tuple indicated by scan-rs_ctup.
*
* Note: the reason nkeys/key are passed separately, even though they are
* kept in the scan descriptor, is that the caller may not want us to check
--- 172,179
* tuple as indicated by dir; return the next tuple in scan-rs_ctup,
* or set scan-rs_ctup.t_data = NULL if no more tuples.
*
! * dir == NoMovementScanDirection means re-fetch the tuple indicated
! * by scan-rs_ctup.
*
* Note: the reason nkeys/key are passed separately, even though they are
* kept in the scan descriptor, is that the caller may not want us to check
***
*** 189,200
*/
static void
heapgettup(HeapScanDesc scan,
! int dir,
int nkeys,
ScanKey key)
{
HeapTuple tuple = (scan-rs_ctup);
Snapshot snapshot = scan-rs_snapshot;
BlockNumber page;
Page dp;
int lines;
--- 190,202
*/
static void
heapgettup(HeapScanDesc scan,
! ScanDirection dir,
int nkeys,
ScanKey key)
{
HeapTuple tuple = (scan-rs_ctup);
Snapshot snapshot = scan-rs_snapshot;
+ bool backward = ScanDirectionIsBackward(dir);
BlockNumber page;
Page dp;
int lines;
***
*** 205,215
/*
* calculate next starting lineoff, given scan direction
*/
! if (dir 0)
{
- /*
- * forward scan direction
- */
if (!scan-rs_inited)
{
/*
--- 207,214
/*
* calculate next starting lineoff, given scan direction
*/
! if (ScanDirectionIsForward(dir))
{
if (!scan-rs_inited)
{
/*
***
*** 242,252
linesleft = lines - lineoff + 1;
}
! else if (dir 0)
{
- /*
- * reverse scan direction
- */
if (!scan-rs_inited)
{
/*
--- 241,248
linesleft = lines - lineoff + 1;
}
! else if (backward)
{
if (!scan-rs_inited)
{
/*
***
*** 352,358
* otherwise move to the next item on the page
*/
--linesleft;
! if (dir 0)
{
--lpp; /* move back in this page's ItemId array */
--lineoff;
--- 348,354
* otherwise move to the next item on the page
*/
--linesleft;
! if (backward)
{
--lpp; /* move back in this page's ItemId array */
--lineoff;
***
*** 373,379
/*
* return NULL if we've exhausted all the pages
*/
! if ((dir 0) ? (page == 0) : (page + 1 = scan-rs_nblocks))
{
if (BufferIsValid(scan-rs_cbuf))
ReleaseBuffer(scan-rs_cbuf);
--- 369,375
/*
* return NULL if we've exhausted all the pages
*/
! if (backward ? (page == 0) : (page + 1 = scan-rs_nblocks))
{
if (BufferIsValid(scan-rs_cbuf))
ReleaseBuffer(scan-rs_cbuf);
***
*** 384,390
return;
}
! page = (dir 0) ? (page - 1) : (page + 1);
heapgetpage(scan, page);
--- 380,386
return;
}
! page = backward ? (page - 1) : (page + 1);
heapgetpage(scan, page);
***
*** 393,399
dp = (Page) BufferGetPage(scan-rs_cbuf);
lines = PageGetMaxOffsetNumber((Page) dp);
linesleft = lines;
! if (dir 0)
{
lineoff = lines;
lpp = PageGetItemId(dp, lines);
--- 389,395
dp = (Page) BufferGetPage(scan-rs_cbuf);
lines = PageGetMaxOffsetNumber((Page) dp);
linesleft = lines;
! if (backward)
{
lineoff = lines;
lpp = PageGetItemId(dp, lines);
***
*** 421,431
*/
static void
heapgettup_pagemode(HeapScanDesc scan,
! int dir,
int nkeys,
ScanKey key)
{
HeapTuple tuple = (scan-rs_ctup);
BlockNumber page;
Page dp;
int lines;
--- 417,428
*/
static void
heapgettup_pagemode(HeapScanDesc scan,
! ScanDirection dir,
int nkeys,
ScanKey key)
{
HeapTuple tuple = (scan-rs_ctup);
+ bool backward = ScanDirectionIsBackward(dir);
BlockNumber page;
Page dp;
int lines;
***
*** 437,447
/*
* calculate next starting
Re: [PATCHES] ScanDirections
On Sun, 2006-02-19 at 17:47 -0700, James William Pye wrote: Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or whatever enum makes it. Why? -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] ScanDirections
On Sun, Feb 19, 2006 at 08:02:09PM -0500, Neil Conway wrote: Why? *Very* minor. one byte instead of four. Yes, not really worth the time, but I was poking around at code anyways and decided to write up a little patch. -- Regards, James William Pye ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] ScanDirections
James William Pye wrote: On Sun, Feb 19, 2006 at 08:02:09PM -0500, Neil Conway wrote: Why? *Very* minor. one byte instead of four. Is one byte any faster anyway? I would be expecting that with 64bit PC's a 64bit item is as fast, if not faster than a 32bit, or 8 bit. Yes, not really worth the time, but I was poking around at code anyways and decided to write up a little patch. ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] ScanDirections
On Sun, 2006-02-19 at 18:12 -0700, James William Pye wrote: *Very* minor. one byte instead of four. Arguably, enumerations are more readable than #defines and easier to debug. Why do we care about saving 3 bytes for ScanDirection? -Neil ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] ScanDirections
On Sun, Feb 19, 2006 at 08:21:31PM -0500, Neil Conway wrote: Arguably, enumerations are more readable than #defines and easier to debug. Agreed. However, I didn't think it would impede much here as I figured gdb would display 'f' or 'b' or 'n', dunno for sure tho as I didn't try. Why do we care about saving 3 bytes for ScanDirection? It's a drop in the bucket--or maybe water vapor in the vacinity, so I doubt I could find a stunning reason. -- Regards, James William Pye ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] ScanDirections
James William Pye [EMAIL PROTECTED] writes: Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or whatever enum makes it. Why is this a good idea? It strikes me as likely to break things, without really buying anything. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] ScanDirections
On Sun, Feb 19, 2006 at 09:04:09PM -0500, Tom Lane wrote: James William Pye [EMAIL PROTECTED] writes: Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or whatever enum makes it. Why is this a good idea? A more appropriately sized variable for the data that it will be holding? Certainly not a stunning improvement considering that it's only three bytes. It strikes me as likely to break things, without really buying anything. I tried to be careful and track everything down. However, I imagine I could have easily missed something, so I understand that concern. In any case, some parts of the patch merely makes some code make use of the sdir.h macros instead of depending on -1/0/1--heapam in particular. At least those portions should be applied, no? -- Regards, James William Pye ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] ScanDirections
James William Pye [EMAIL PROTECTED] writes: In any case, some parts of the patch merely makes some code make use of the sdir.h macros instead of depending on -1/0/1--heapam in particular. At least those portions should be applied, no? I can't see any great advantage there either ... regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] ScanDirections
On Sun, 2006-02-19 at 19:46 -0700, James William Pye wrote: In any case, some parts of the patch merely makes some code make use of the sdir.h macros instead of depending on -1/0/1--heapam in particular. At least those portions should be applied, no? I agree: using the symbolic names for the ScanDirection alternatives is more readable than using magic numbers, and costs us nothing. -Neil ---(end of broadcast)--- TIP 6: explain analyze is your friend
