Обсуждение: broken comment justification logic in new pgindent

Поиск
Список
Период
Сортировка

broken comment justification logic in new pgindent

От
Tom Lane
Дата:
I'm noticing that the latest pgindent run has frequently rejustified
block comments to end in column 80 or 81, causing them to wrap in an
ugly way (at least in emacs).  I thought the agreement was to limit
lines to 79 chars max?

For one example see lines 475 ff in /src/backend/access/nbtree/nbtpage.c
--- the first lines of two successive paragraphs in the comment have
been made too long, which they were not before.

I'm not sure about this offhand, but I think that all the cases I've
seen have involved first lines of paragraphs inside block comments.
        regards, tom lane


Re: broken comment justification logic in new pgindent

От
Bruce Momjian
Дата:
Tom Lane wrote:
> I'm noticing that the latest pgindent run has frequently rejustified
> block comments to end in column 80 or 81, causing them to wrap in an
> ugly way (at least in emacs).  I thought the agreement was to limit
> lines to 79 chars max?
>
> For one example see lines 475 ff in /src/backend/access/nbtree/nbtpage.c
> --- the first lines of two successive paragraphs in the comment have
> been made too long, which they were not before.
>
> I'm not sure about this offhand, but I think that all the cases I've
> seen have involved first lines of paragraphs inside block comments.

Good point.  I see the maximum length changed in this commit:

    revision 1.70
    date: 2004/10/02 01:10:58;  author: momjian;  state: Exp;  lines: +1 -1
    Update length from 75 to 79.

We were discussing some pgindent issues at that time on hackers, but I
don't see any complaints about the length, so I am unsure why I modified
it, but perhaps I received a private communication asking why it wasn't
79.

Anyway, I have updated the script to be 78, and attached is a diff
against nbpage.c, but I have not applied a change to that file.

Would you like another pgindent run with the new value of 78?  Should be
run on CVS HEAD only or 8.0.X too?

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: nbtpage.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/nbtree/nbtpage.c,v
retrieving revision 1.89
diff -c -r1.89 nbtpage.c
*** nbtpage.c    6 Nov 2005 19:29:00 -0000    1.89
--- nbtpage.c    7 Nov 2005 22:53:41 -0000
***************
*** 205,223 ****
          if (metad->btm_root != P_NONE)
          {
              /*
!              * Metadata initialized by someone else.  In order to guarantee no
!              * deadlocks, we have to release the metadata page and start all
!              * over again.    (Is that really true? But it's hardly worth trying
!              * to optimize this case.)
               */
              _bt_relbuf(rel, metabuf);
              return _bt_getroot(rel, access);
          }

          /*
!          * Get, initialize, write, and leave a lock of the appropriate type on
!          * the new root page.  Since this is the first page in the tree, it's
!          * a leaf as well as the root.
           */
          rootbuf = _bt_getbuf(rel, P_NEW, BT_WRITE);
          rootblkno = BufferGetBlockNumber(rootbuf);
--- 205,223 ----
          if (metad->btm_root != P_NONE)
          {
              /*
!              * Metadata initialized by someone else.  In order to guarantee
!              * no deadlocks, we have to release the metadata page and start
!              * all over again.    (Is that really true? But it's hardly worth
!              * trying to optimize this case.)
               */
              _bt_relbuf(rel, metabuf);
              return _bt_getroot(rel, access);
          }

          /*
!          * Get, initialize, write, and leave a lock of the appropriate type
!          * on the new root page.  Since this is the first page in the tree,
!          * it's a leaf as well as the root.
           */
          rootbuf = _bt_getbuf(rel, P_NEW, BT_WRITE);
          rootblkno = BufferGetBlockNumber(rootbuf);
***************
*** 412,427 ****
      Page        page = BufferGetPage(buf);

      /*
!      * ReadBuffer verifies that every newly-read page passes PageHeaderIsValid,
!      * which means it either contains a reasonably sane page header or is
!      * all-zero.  We have to defend against the all-zero case, however.
       */
      if (PageIsNew(page))
          ereport(ERROR,
                  (errcode(ERRCODE_INDEX_CORRUPTED),
!                  errmsg("index \"%s\" contains unexpected zero page at block %u",
!                         RelationGetRelationName(rel),
!                         BufferGetBlockNumber(buf)),
                   errhint("Please REINDEX it.")));

      /*
--- 412,428 ----
      Page        page = BufferGetPage(buf);

      /*
!      * ReadBuffer verifies that every newly-read page passes
!      * PageHeaderIsValid, which means it either contains a reasonably sane
!      * page header or is all-zero.    We have to defend against the all-zero
!      * case, however.
       */
      if (PageIsNew(page))
          ereport(ERROR,
                  (errcode(ERRCODE_INDEX_CORRUPTED),
!             errmsg("index \"%s\" contains unexpected zero page at block %u",
!                    RelationGetRelationName(rel),
!                    BufferGetBlockNumber(buf)),
                   errhint("Please REINDEX it.")));

      /*
***************
*** 440,446 ****
  /*
   *    _bt_getbuf() -- Get a buffer by block number for read or write.
   *
!  *        blkno == P_NEW means to get an unallocated index page.  The page
   *        will be initialized before returning it.
   *
   *        When this routine returns, the appropriate lock is set on the
--- 441,447 ----
  /*
   *    _bt_getbuf() -- Get a buffer by block number for read or write.
   *
!  *        blkno == P_NEW means to get an unallocated index page.    The page
   *        will be initialized before returning it.
   *
   *        When this routine returns, the appropriate lock is set on the
***************
*** 480,495 ****
           * it, or even worse our own caller does, we could deadlock.  (The
           * own-caller scenario is actually not improbable. Consider an index
           * on a serial or timestamp column.  Nearly all splits will be at the
!          * rightmost page, so it's entirely likely that _bt_split will call us
!          * while holding a lock on the page most recently acquired from FSM.
!          * A VACUUM running concurrently with the previous split could well
!          * have placed that page back in FSM.)
           *
!          * To get around that, we ask for only a conditional lock on the reported
!          * page.  If we fail, then someone else is using the page, and we may
!          * reasonably assume it's not free.  (If we happen to be wrong, the
!          * worst consequence is the page will be lost to use till the next
!          * VACUUM, which is no big problem.)
           */
          for (;;)
          {
--- 481,496 ----
           * it, or even worse our own caller does, we could deadlock.  (The
           * own-caller scenario is actually not improbable. Consider an index
           * on a serial or timestamp column.  Nearly all splits will be at the
!          * rightmost page, so it's entirely likely that _bt_split will call
!          * us while holding a lock on the page most recently acquired from
!          * FSM. A VACUUM running concurrently with the previous split could
!          * well have placed that page back in FSM.)
           *
!          * To get around that, we ask for only a conditional lock on the
!          * reported page.  If we fail, then someone else is using the page,
!          * and we may reasonably assume it's not free.  (If we happen to be
!          * wrong, the worst consequence is the page will be lost to use till
!          * the next VACUUM, which is no big problem.)
           */
          for (;;)
          {
***************
*** 649,658 ****
      BTPageOpaque opaque;

      /*
!      * It's possible to find an all-zeroes page in an index --- for example, a
!      * backend might successfully extend the relation one page and then crash
!      * before it is able to make a WAL entry for adding the page. If we find a
!      * zeroed page then reclaim it.
       */
      if (PageIsNew(page))
          return true;
--- 650,659 ----
      BTPageOpaque opaque;

      /*
!      * It's possible to find an all-zeroes page in an index --- for example,
!      * a backend might successfully extend the relation one page and then
!      * crash before it is able to make a WAL entry for adding the page. If we
!      * find a zeroed page then reclaim it.
       */
      if (PageIsNew(page))
          return true;
***************
*** 782,789 ****
      BTPageOpaque opaque;

      /*
!      * We can never delete rightmost pages nor root pages.    While at it, check
!      * that page is not already deleted and is empty.
       */
      page = BufferGetPage(buf);
      opaque = (BTPageOpaque) PageGetSpecialPointer(page);
--- 783,790 ----
      BTPageOpaque opaque;

      /*
!      * We can never delete rightmost pages nor root pages.    While at it,
!      * check that page is not already deleted and is empty.
       */
      page = BufferGetPage(buf);
      opaque = (BTPageOpaque) PageGetSpecialPointer(page);
***************
*** 808,815 ****
       * We need to get an approximate pointer to the page's parent page. Use
       * the standard search mechanism to search for the page's high key; this
       * will give us a link to either the current parent or someplace to its
!      * left (if there are multiple equal high keys).  To avoid deadlocks, we'd
!      * better drop the target page lock first.
       */
      _bt_relbuf(rel, buf);
      /* we need a scan key to do our search, so build one */
--- 809,816 ----
       * We need to get an approximate pointer to the page's parent page. Use
       * the standard search mechanism to search for the page's high key; this
       * will give us a link to either the current parent or someplace to its
!      * left (if there are multiple equal high keys).  To avoid deadlocks,
!      * we'd better drop the target page lock first.
       */
      _bt_relbuf(rel, buf);
      /* we need a scan key to do our search, so build one */
***************
*** 843,851 ****
       * page.  The sibling that was current a moment ago could have split, so
       * we may have to move right.  This search could fail if either the
       * sibling or the target page was deleted by someone else meanwhile; if
!      * so, give up.  (Right now, that should never happen, since page deletion
!      * is only done in VACUUM and there shouldn't be multiple VACUUMs
!      * concurrently on the same table.)
       */
      if (leftsib != P_NONE)
      {
--- 844,852 ----
       * page.  The sibling that was current a moment ago could have split, so
       * we may have to move right.  This search could fail if either the
       * sibling or the target page was deleted by someone else meanwhile; if
!      * so, give up.  (Right now, that should never happen, since page
!      * deletion is only done in VACUUM and there shouldn't be multiple
!      * VACUUMs concurrently on the same table.)
       */
      if (leftsib != P_NONE)
      {
***************
*** 872,880 ****
          lbuf = InvalidBuffer;

      /*
!      * Next write-lock the target page itself.    It should be okay to take just
!      * a write lock not a superexclusive lock, since no scans would stop on an
!      * empty page.
       */
      buf = _bt_getbuf(rel, target, BT_WRITE);
      page = BufferGetPage(buf);
--- 873,881 ----
          lbuf = InvalidBuffer;

      /*
!      * Next write-lock the target page itself.    It should be okay to take
!      * just a write lock not a superexclusive lock, since no scans would stop
!      * on an empty page.
       */
      buf = _bt_getbuf(rel, target, BT_WRITE);
      page = BufferGetPage(buf);
***************
*** 904,911 ****
      rbuf = _bt_getbuf(rel, rightsib, BT_WRITE);

      /*
!      * Next find and write-lock the current parent of the target page. This is
!      * essentially the same as the corresponding step of splitting.
       */
      ItemPointerSet(&(stack->bts_btitem.bti_itup.t_tid),
                     target, P_HIKEY);
--- 905,912 ----
      rbuf = _bt_getbuf(rel, rightsib, BT_WRITE);

      /*
!      * Next find and write-lock the current parent of the target page. This
!      * is essentially the same as the corresponding step of splitting.
       */
      ItemPointerSet(&(stack->bts_btitem.bti_itup.t_tid),
                     target, P_HIKEY);
***************
*** 949,957 ****

      /*
       * If we are deleting the next-to-last page on the target's level, then
!      * the rightsib is a candidate to become the new fast root. (In theory, it
!      * might be possible to push the fast root even further down, but the odds
!      * of doing so are slim, and the locking considerations daunting.)
       *
       * We can safely acquire a lock on the metapage here --- see comments for
       * _bt_newroot().
--- 950,958 ----

      /*
       * If we are deleting the next-to-last page on the target's level, then
!      * the rightsib is a candidate to become the new fast root. (In theory,
!      * it might be possible to push the fast root even further down, but the
!      * odds of doing so are slim, and the locking considerations daunting.)
       *
       * We can safely acquire a lock on the metapage here --- see comments for
       * _bt_newroot().
***************
*** 992,999 ****
      /*
       * Update parent.  The normal case is a tad tricky because we want to
       * delete the target's downlink and the *following* key.  Easiest way is
!      * to copy the right sibling's downlink over the target downlink, and then
!      * delete the following item.
       */
      page = BufferGetPage(pbuf);
      opaque = (BTPageOpaque) PageGetSpecialPointer(page);
--- 993,1000 ----
      /*
       * Update parent.  The normal case is a tad tricky because we want to
       * delete the target's downlink and the *following* key.  Easiest way is
!      * to copy the right sibling's downlink over the target downlink, and
!      * then delete the following item.
       */
      page = BufferGetPage(pbuf);
      opaque = (BTPageOpaque) PageGetSpecialPointer(page);
***************
*** 1154,1162 ****

      /*
       * If parent became half dead, recurse to try to delete it. Otherwise, if
!      * right sibling is empty and is now the last child of the parent, recurse
!      * to try to delete it.  (These cases cannot apply at the same time,
!      * though the second case might itself recurse to the first.)
       */
      if (parent_half_dead)
      {
--- 1155,1163 ----

      /*
       * If parent became half dead, recurse to try to delete it. Otherwise, if
!      * right sibling is empty and is now the last child of the parent,
!      * recurse to try to delete it.  (These cases cannot apply at the same
!      * time, though the second case might itself recurse to the first.)
       */
      if (parent_half_dead)
      {

Re: broken comment justification logic in new pgindent

От
Tom Lane
Дата:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Good point.  I see the maximum length changed in this commit:
>     revision 1.70
>     date: 2004/10/02 01:10:58;  author: momjian;  state: Exp;  lines: +1 -1
>     Update length from 75 to 79.

> We were discussing some pgindent issues at that time on hackers, but I
> don't see any complaints about the length, so I am unsure why I modified
> it, but perhaps I received a private communication asking why it wasn't
> 79.

I remember that discussion, and I remember we agreed to update it to 79.
You're missing the point completely: the problem at hand is that
pgindent is failing to honor its width parameter in some situations.

> Anyway, I have updated the script to be 78,

This is just going to create a lot of gratutitous re-indents without
actually fixing the problem, because whatever bug is causing pgindent
to sometimes not honor the -l switch will still be there.
        regards, tom lane


Re: broken comment justification logic in new pgindent

От
Bruce Momjian
Дата:
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Good point.  I see the maximum length changed in this commit:
> >     revision 1.70
> >     date: 2004/10/02 01:10:58;  author: momjian;  state: Exp;  lines: +1 -1
> >     Update length from 75 to 79.
> 
> > We were discussing some pgindent issues at that time on hackers, but I
> > don't see any complaints about the length, so I am unsure why I modified
> > it, but perhaps I received a private communication asking why it wasn't
> > 79.
> 
> I remember that discussion, and I remember we agreed to update it to 79.
> You're missing the point completely: the problem at hand is that
> pgindent is failing to honor its width parameter in some situations.

My guess is that there is a one-off bug in there.

> > Anyway, I have updated the script to be 78,
> 
> This is just going to create a lot of gratutitous re-indents without
> actually fixing the problem, because whatever bug is causing pgindent
> to sometimes not honor the -l switch will still be there.

I have no idea how we are going to find the bug.  I am thinking we need
to just reduce the maximum length until the length is less than 80.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 


Re: broken comment justification logic in new pgindent

От
Tom Lane
Дата:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> My guess is that there is a one-off bug in there.

At least a two-off ... but I think it's more likely some sort of
wrong-state error, given the narrow places where it happens.  I have not
observed any non-comment code being mis-justified, for instance.
        regards, tom lane


Fixes for 8.1 run of pgindent

От
Bruce Momjian
Дата:
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > My guess is that there is a one-off bug in there.
> 
> At least a two-off ... but I think it's more likely some sort of
> wrong-state error, given the narrow places where it happens.  I have not
> observed any non-comment code being mis-justified, for instance.

OK, I have spent some unpleasant time tracking down the pgindent
problems from 8.1.  Some were my fault, and some the BSD indent code.

First, my fault was not updating the typedefs for both /bin and /lib ---
I did only /bin.  This was documented, but not clearly.  I have improved
the docs on this.

Second, Tom found that when we took the margin to 79, BSD indent had a
bug that the first line after a blank comment line could go to 80 or 81
columns.  I tracked down this bug and applied a fix to my version, the
patch in our CVS, and the indent tarball on our ftp server.

Third, I found that if more then 150 'else if' are used in the same
statement, the comments are shifted to start on column 100.  I have
found the cause for this (stack of 150 hardcoded) and fixed that too
everywhere. I have rerun pgindent on psql/tab-complete.c and committed it
to CVS for both branches.

I have tested the new pgindent on our existing CVS and the only changes
are for comments to fix the bad wrapping, and to fix the missing /lib
typedefs.

I think we should rerun pgindent on 8.1.X and HEAD to correct the
reported problems.  I am betting 90% of our patches either come from
CVS head or 8.1.X branches greater than 8.1.0.

I have on my TODO list to test GNU indent again during 8.2 to see how it
does.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
359-1001+  If your life is a hard drive,     |  13 Roberts Road +  Christ can be your backup.        |  Newtown Square,
Pennsylvania19073
 


Re: Fixes for 8.1 run of pgindent

От
Tom Lane
Дата:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think we should rerun pgindent on 8.1.X and HEAD to correct the
> reported problems.  I am betting 90% of our patches either come from
> CVS head or 8.1.X branches greater than 8.1.0.

Can you post a diff showing what would change exactly?

I'd like to hold off for at least a little bit on reindenting HEAD,
because I've got a fair size set of changes for nulls-in-arrays that
I'm still a day or two away from committing.  Reindenting right now
is likely to cause a rather painful merge problem because I updated a
lot of comments.  I'm not sure who else has major patches in progress
(anyone out there?), but I may not be the only one with a problem.
        regards, tom lane


Re: Fixes for 8.1 run of pgindent

От
Alvaro Herrera
Дата:
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I think we should rerun pgindent on 8.1.X and HEAD to correct the
> > reported problems.  I am betting 90% of our patches either come from
> > CVS head or 8.1.X branches greater than 8.1.0.
> 
> Can you post a diff showing what would change exactly?
> 
> I'd like to hold off for at least a little bit on reindenting HEAD,
> because I've got a fair size set of changes for nulls-in-arrays that
> I'm still a day or two away from committing.  Reindenting right now
> is likely to cause a rather painful merge problem because I updated a
> lot of comments.  I'm not sure who else has major patches in progress
> (anyone out there?), but I may not be the only one with a problem.

I have two moderate-size patches that I'm planning to submit shortly.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support