Re: Spaces before newlines in view definitions in 9.3

Hello Tom,

On 08/11/13 17:24, Tom Lane wrote:
> Joe Abbate <jma(at)freedomcircle(dot)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.

Agreed it's not a significant complaint, just an extra annoyance --made
more visible because for Pyrseas 0.7 we added nicer view text formatting
rather than outputting what pg_get_viewdef gave us, no matter how ugly
and long the resulting quoted string looked like.

Our tests generally compare SQL texts in a "forgiving" manner,
particularly when it comes to whitespace. However, the problem was
introduced in 9.3 and broke some of our view tests (and analogous
matview tests derived from them). It's not a big deal if you don't
backpatch to 9.3, but it would help, since some of our tests now read:

if ("postgres version" < 90300):
fmt = "(%s%s %s%s )"
else:
fmt = "( %s\n %s\n %s\n %s\n)"

That would probably require yet another branch for 9.4 and later.

Best regards,

Joe