Re: Spaces before newlines in view definitions in 9.3 - Mailing list pgsql-bugs

Joe Abbate <jma@freedomcircle.com> writes:
> View definition:
>  SELECT t1.c1,
>     t1.c2
>    FROM t1;

> It may not be immediately obvious but there is a space after the
> "t1.c1," and before the first newline.  In 9.2 and previous releases,
> the view definition is:

>  SELECT t1.c1, t1.c2
>    FROM t1;

Yeah, this was changed by commit 2f582f76b1945929ff07116cd4639747ce9bb8a1,
which added newlines to the output without making any attempt to suppress
the spaces that had been printed before.

> The problem is that the string comes back, e.g., from pg_get_viewdef()
> with those extra spaces before the newlines, e.g.,

> " SELECT t1.c1, \n    t1.c3 * 2 AS mc3\n   FROM t1;

> and YAML has a way displaying a text string nicely so that it can be
> recovered when it's read back, but it *doesn't* work if there are
> invisible characters such as tabs or spaces before a newline because
> obviously one can't tell how many or of what kind they are.

Hm.  I am not sure whether to consider that a significant complaint or
not, because in reality the ruleutils code has been pretty sloppy about
trailing whitespace ever since it grew any "pretty printing" capability
at all.  Looking for trailing whitespace in the ruleutils regression test,
I find it in "UNION ALL" and "INSERT ... VALUES" constructs, which were
that way long before that patch, and there are probably more cases that
don't happen to get exercised by the regression tests.  That patch
certainly made things worse, but it's not like pre-9.3 output was
YAML-safe.

Having said that, this seems relatively easy to fix by adjusting
appendContextKeyword to delete any trailing spaces that are immediately
before the newline it inserts.  AFAICS that's the only place in ruleutils
that inserts newlines that might follow a space.  get_target_list also
needs to do that when transposing a newline-started field value into the
main buffer; but that complexity is offset because it no longer need
consider the possibility of spaces before said newline.  I've tested the
attached patch and it seems to do what's wanted (note: I didn't bother
including the regression test output changes in the patch, as they're
bulky and boring).

I think that this is a reasonable thing to fix, but I'm not sure
if we ought to back-patch it into 9.3 or not.  The pretty-printing
change evidently broke your use-case but I'm thinking you weren't
pushing it very hard.

Comments?

            regards, tom lane

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 915fb7a..139939d 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** get_target_list(List *targetList, depars
*** 4479,4520 ****
          /* Consider line-wrapping if enabled */
          if (PRETTY_INDENT(context) && context->wrapColumn >= 0)
          {
!             int            leading_nl_pos = -1;
!             char       *trailing_nl;
!             int            pos;

!             /* Does the new field start with whitespace plus a new line? */
!             for (pos = 0; pos < targetbuf.len; pos++)
              {
!                 if (targetbuf.data[pos] == '\n')
!                 {
!                     leading_nl_pos = pos;
!                     break;
!                 }
!                 if (targetbuf.data[pos] != ' ')
!                     break;
              }
-
-             /* Locate the start of the current line in the output buffer */
-             trailing_nl = strrchr(buf->data, '\n');
-             if (trailing_nl == NULL)
-                 trailing_nl = buf->data;
              else
!                 trailing_nl++;

!             /*
!              * If the field we're adding is the first in the list, or it
!              * already has a leading newline, don't add anything. Otherwise,
!              * add a newline, plus some indentation, if either the new field
!              * would cause an overflow or the last field used more than one
!              * line.
!              */
!             if (colno > 1 &&
!                 leading_nl_pos == -1 &&
!                 ((strlen(trailing_nl) + strlen(targetbuf.data) > context->wrapColumn) ||
!                  last_was_multiline))
!                 appendContextKeyword(context, "", -PRETTYINDENT_STD,
!                                      PRETTYINDENT_STD, PRETTYINDENT_VAR);

              /* Remember this field's multiline status for next iteration */
              last_was_multiline =
--- 4479,4521 ----
          /* Consider line-wrapping if enabled */
          if (PRETTY_INDENT(context) && context->wrapColumn >= 0)
          {
!             int            leading_nl_pos;

!             /* Does the new field start with a new line? */
!             if (targetbuf.len > 0 && targetbuf.data[0] == '\n')
!                 leading_nl_pos = 0;
!             else
!                 leading_nl_pos = -1;
!
!             /* If so, we shouldn't add anything */
!             if (leading_nl_pos >= 0)
              {
!                 /* instead, remove any trailing spaces currently in buf */
!                 while (buf->len > 0 && buf->data[buf->len - 1] == ' ')
!                     buf->data[--(buf->len)] = '\0';
              }
              else
!             {
!                 char       *trailing_nl;

!                 /* Locate the start of the current line in the output buffer */
!                 trailing_nl = strrchr(buf->data, '\n');
!                 if (trailing_nl == NULL)
!                     trailing_nl = buf->data;
!                 else
!                     trailing_nl++;
!
!                 /*
!                  * Add a newline, plus some indentation, if the new field is
!                  * not the first and either the new field would cause an
!                  * overflow or the last field used more than one line.
!                  */
!                 if (colno > 1 &&
!                     ((strlen(trailing_nl) + strlen(targetbuf.data) > context->wrapColumn) ||
!                      last_was_multiline))
!                     appendContextKeyword(context, "", -PRETTYINDENT_STD,
!                                          PRETTYINDENT_STD, PRETTYINDENT_VAR);
!             }

              /* Remember this field's multiline status for next iteration */
              last_was_multiline =
*************** static void
*** 6236,6256 ****
  appendContextKeyword(deparse_context *context, const char *str,
                       int indentBefore, int indentAfter, int indentPlus)
  {
      if (PRETTY_INDENT(context))
      {
          context->indentLevel += indentBefore;

!         appendStringInfoChar(context->buf, '\n');
!         appendStringInfoSpaces(context->buf,
                                 Max(context->indentLevel, 0) + indentPlus);
!         appendStringInfoString(context->buf, str);

          context->indentLevel += indentAfter;
          if (context->indentLevel < 0)
              context->indentLevel = 0;
      }
      else
!         appendStringInfoString(context->buf, str);
  }

  /*
--- 6237,6264 ----
  appendContextKeyword(deparse_context *context, const char *str,
                       int indentBefore, int indentAfter, int indentPlus)
  {
+     StringInfo    buf = context->buf;
+
      if (PRETTY_INDENT(context))
      {
          context->indentLevel += indentBefore;

!         /* remove any trailing spaces currently in the buffer ... */
!         while (buf->len > 0 && buf->data[buf->len - 1] == ' ')
!             buf->data[--(buf->len)] = '\0';
!         /* ... then add a newline and some spaces */
!         appendStringInfoChar(buf, '\n');
!         appendStringInfoSpaces(buf,
                                 Max(context->indentLevel, 0) + indentPlus);
!
!         appendStringInfoString(buf, str);

          context->indentLevel += indentAfter;
          if (context->indentLevel < 0)
              context->indentLevel = 0;
      }
      else
!         appendStringInfoString(buf, str);
  }

  /*