Обсуждение: ScanDirections

Small patch that makes ScanDirection a char(f|b|n) instead of a int or long or
whatever enum makes it. I tried to track down all the areas that depend on it
being specifically -1, 0, or 1. heapam appeared to be the biggest/only one.
(At least `gmake check` passed.)
--
Regards, James William Pye

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

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

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.

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

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

James William Pye <pgsql@jwp.name> 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

On Sun, Feb 19, 2006 at 09:04:09PM -0500, Tom Lane wrote:
> James William Pye <pgsql@jwp.name> 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

James William Pye <pgsql@jwp.name> 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

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

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 -0000    1.206
--- src/backend/access/heap/heapam.c    21 Feb 2006 22:28:35 -0000
***************
*** 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 lineindex, given scan direction
       */
!     if (dir > 0)
      {
-         /*
-          * forward scan direction
-          */
          if (!scan->rs_inited)
          {
              /*
--- 434,441 ----
      /*
       * calculate next starting lineindex, given scan direction
       */
!     if (ScanDirectionIsForward(dir))
      {
          if (!scan->rs_inited)
          {
              /*
***************
*** 471,481 ****

          linesleft = lines - lineindex;
      }
!     else if (dir < 0)
      {
-         /*
-          * reverse scan direction
-          */
          if (!scan->rs_inited)
          {
              /*
--- 465,472 ----

          linesleft = lines - lineindex;
      }
!     else if (backward)
      {
          if (!scan->rs_inited)
          {
              /*
***************
*** 584,597 ****
               * otherwise move to the next item on the page
               */
              --linesleft;
!             if (dir < 0)
!             {
                  --lineindex;
-             }
              else
-             {
                  ++lineindex;
-             }
          }

          /*
--- 575,584 ----
               * otherwise move to the next item on the page
               */
              --linesleft;
!             if (backward)
                  --lineindex;
              else
                  ++lineindex;
          }

          /*
***************
*** 602,608 ****
          /*
           * 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);
--- 589,595 ----
          /*
           * 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);
***************
*** 613,626 ****
              return;
          }

!         page = (dir < 0) ? (page - 1) : (page + 1);
!
          heapgetpage(scan, page);

          dp = (Page) BufferGetPage(scan->rs_cbuf);
          lines = scan->rs_ntuples;
          linesleft = lines;
!         if (dir < 0)
              lineindex = lines - 1;
          else
              lineindex = 0;
--- 600,612 ----
              return;
          }

!         page = backward ? (page - 1) : (page + 1);
          heapgetpage(scan, page);

          dp = (Page) BufferGetPage(scan->rs_cbuf);
          lines = scan->rs_ntuples;
          linesleft = lines;
!         if (backward)
              lineindex = lines - 1;
          else
              lineindex = 0;
***************
*** 1008,1022 ****

      HEAPDEBUG_1;                /* heap_getnext( info ) */

-     /*
-      * Note: we depend here on the -1/0/1 encoding of ScanDirection.
-      */
      if (scan->rs_pageatatime)
!         heapgettup_pagemode(scan, (int) direction,
                              scan->rs_nkeys, scan->rs_key);
      else
!         heapgettup(scan, (int) direction,
!                    scan->rs_nkeys, scan->rs_key);

      if (scan->rs_ctup.t_data == NULL)
      {
--- 994,1004 ----

      HEAPDEBUG_1;                /* heap_getnext( info ) */

      if (scan->rs_pageatatime)
!         heapgettup_pagemode(scan, direction,
                              scan->rs_nkeys, scan->rs_key);
      else
!         heapgettup(scan, direction, scan->rs_nkeys, scan->rs_key);

      if (scan->rs_ctup.t_data == NULL)
      {
***************
*** 2745,2757 ****
          {
              scan->rs_cindex = scan->rs_mindex;
              heapgettup_pagemode(scan,
!                                 0,            /* "no movement" */
                                  0,            /* needn't recheck scan keys */
                                  NULL);
          }
          else
              heapgettup(scan,
!                        0,                    /* "no movement" */
                         0,                    /* needn't recheck scan keys */
                         NULL);
      }
--- 2727,2739 ----
          {
              scan->rs_cindex = scan->rs_mindex;
              heapgettup_pagemode(scan,
!                                 NoMovementScanDirection,
                                  0,            /* needn't recheck scan keys */
                                  NULL);
          }
          else
              heapgettup(scan,
!                        NoMovementScanDirection,
                         0,                    /* needn't recheck scan keys */
                         NULL);
      }
Index: src/backend/executor/execMain.c
===================================================================
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/backend/executor/execMain.c,v
retrieving revision 1.266
diff -c -r1.266 execMain.c
*** src/backend/executor/execMain.c    19 Feb 2006 00:04:26 -0000    1.266
--- src/backend/executor/execMain.c    21 Feb 2006 22:29:16 -0000
***************
*** 218,224 ****
      /*
       * run plan
       */
!     if (direction == NoMovementScanDirection)
          result = NULL;
      else
          result = ExecutePlan(estate,
--- 218,224 ----
      /*
       * run plan
       */
!     if (ScanDirectionIsNoMovement(direction))
          result = NULL;
      else
          result = ExecutePlan(estate,
Index: src/backend/tcop/pquery.c
===================================================================
RCS file: /Users/neilc/postgres/cvs_root/pgsql/src/backend/tcop/pquery.c,v
retrieving revision 1.98
diff -c -r1.98 pquery.c
*** src/backend/tcop/pquery.c    22 Nov 2005 18:17:21 -0000    1.98
--- src/backend/tcop/pquery.c    21 Feb 2006 22:30:56 -0000
***************
*** 795,801 ****
              nprocessed = queryDesc->estate->es_processed;
          }

!         if (direction != NoMovementScanDirection)
          {
              long        oldPos;

--- 795,801 ----
              nprocessed = queryDesc->estate->es_processed;
          }

!         if (!ScanDirectionIsNoMovement(direction))
          {
              long        oldPos;

***************
*** 837,843 ****
              nprocessed = queryDesc->estate->es_processed;
          }

!         if (direction != NoMovementScanDirection)
          {
              if (nprocessed > 0 && portal->atEnd)
              {
--- 837,843 ----
              nprocessed = queryDesc->estate->es_processed;
          }

!         if (!ScanDirectionIsNoMovement(direction))
          {
              if (nprocessed > 0 && portal->atEnd)
              {
***************
*** 890,902 ****

      (*dest->rStartup) (dest, CMD_SELECT, portal->tupDesc);

!     if (direction == NoMovementScanDirection)
      {
          /* do nothing except start/stop the destination */
      }
      else
      {
!         bool        forward = (direction == ForwardScanDirection);

          for (;;)
          {
--- 890,902 ----

      (*dest->rStartup) (dest, CMD_SELECT, portal->tupDesc);

!     if (ScanDirectionIsNoMovement(direction))
      {
          /* do nothing except start/stop the destination */
      }
      else
      {
!         bool        forward = ScanDirectionIsForward(direction);

          for (;;)
          {