Re: Invalid YAML output from EXPLAIN

Testing 9.0 beta, I found that EXPLAINing certain queries in YAML
format will produce invalid YAML, for example:

explain (format yaml) select * from foo where str_val = 'a: b';

The problem in this case is that a colon followed by whitespace is not
allowed in an unquoted plain YAML string because a parser would
interpret it as the start of a map.

So the current code in escape_yaml() is inadequate for producing valid
YAML. I think it would have to also consider at least the following
characters as special "-" ":" "[" "]" "{" "}" "," "\"" "'"
"|" "*" "&". Technically, it would also need to trap empty strings,
and strings with leading or trailing whitespace.

Making escape_yaml() completely bulletproof with this approach would
be quite difficult, and (IMO) not worth the effort, especially given
that an important requirement is that the output be machine readable,
and in my experience YAML parsers are often far from perfect.

I would therefore argue for simply calling escape_json() to produce
double quoted output for all string values, and only have numeric
values unquoted. This is not really any less human readable, and is
far more machine readable.

Patch attached.

- Dean

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

Dean Rasheed wrote:
...
> So the current code in escape_yaml() is inadequate for producing valid
> YAML. I think it would have to also consider at least the following
> characters as special "-" ":" "[" "]" "{" "}" "," "\"" "'"
> "|" "*" "&". Technically, it would also need to trap empty strings,
> and strings with leading or trailing whitespace.
>
> Making escape_yaml() completely bulletproof with this approach would
> be quite difficult, and (IMO) not worth the effort
...

Doesn't seem like a lot of effort to me. You've already laid out most of
the exceptions above, although they require a few tweaks.
The rules should be:

Requires quoting only if the first character:
& * ! | > ' " % @ ` #

Same as above, but no quoting if the second character is "safe":
- ? :

Always requires quoting:
":<space>" "<space>#" aka ': ' ' #'

Always requires quoting:
, [ ] { }

Always require quoting:
(leading space) (trailing space) (empty string)

See:
http://yaml.org/spec/1.2/spec.html section 5.3 and 7.3.3

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201006070943
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkwM+wAACgkQvJuQZxSWSsgWZACcCgb0rDvA6ZVhHId/q568gBGo
sjgAoLY7HbkI7sRpO45vi0jSRJ2Fiytk
=v7T/
-----END PGP SIGNATURE-----

"Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
> The rules should be:

> Requires quoting only if the first character:
> & * ! | > ' " % @ ` #

> Same as above, but no quoting if the second character is "safe":
> - ? :

> Always requires quoting:
> ":<space>" "<space>#" aka ': ' ' #'

> Always requires quoting:
> , [ ] { }

> Always require quoting:
> (leading space) (trailing space) (empty string)

Egad ... this is supposed to be an easily machine-generatable format?

If it's really as broken as the above suggests, I think we should
rip it out while we still can.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

Tom Lane wrote:
...
> Egad ... this is supposed to be an easily machine-generatable format?
>
> If it's really as broken as the above suggests, I think we should
> rip it out while we still can.

Heh ... not like you to shrink from a challenge. ;)

I don't think the above would be particularly hard to implement myself,
but if it becomes a really big deal, we can certainly punt by simply
quoting anything containing an indicator (the special characters above).
It will still be 100% valid YAML, just with some excess quoting for the
very rare case when a value contains one of the special characters.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201006071035
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkwNA+4ACgkQvJuQZxSWSshSswCg81kd3FdYnQup1eLWGesm+vm+
VO8AoL1Fwil/vXfRdRHx4A4zZUTDbZuT
=oPDv
-----END PGP SIGNATURE-----

On Mon, Jun 7, 2010 at 10:37 AM, Greg Sabino Mullane <greg(at)turnstep(dot)com> wrote:
> Tom Lane wrote:
> I don't think the above would be particularly hard to implement myself,
> but if it becomes a really big deal, we can certainly punt by simply
> quoting anything containing an indicator (the special characters above).
> It will still be 100% valid YAML, just with some excess quoting for the
> very rare case when a value contains one of the special characters.

Since you're the main advocate of this feature, I think you should
implement it rather than leaving it to Tom or I.

The reason why I was initially skeptical of adding a YAML output
format is that JSON is a subset of YAML. Therefore, the JSON output
format ought to be perfectly sufficient for anyone using a YAML
parser. If it's not, that's because their YAML processor is broken,
and they should get a new one, or because the YAML spec is defective.
The YAML format got voted in by consensus because people thought that
it would also make a nice alternative to the text format for human
readable output. I don't believe that (it uses way too much vertical
space) but even if you accept the argument, the more we make the YAML
format look like the JSON format, the less water that argument holds.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas wrote:
> On Mon, Jun 7, 2010 at 10:37 AM, Greg Sabino Mullane <greg(at)turnstep(dot)com> wrote:
>
>> Tom Lane wrote:
>> I don't think the above would be particularly hard to implement myself,
>> but if it becomes a really big deal, we can certainly punt by simply
>> quoting anything containing an indicator (the special characters above).
>> It will still be 100% valid YAML, just with some excess quoting for the
>> very rare case when a value contains one of the special characters.
>>
>
> Since you're the main advocate of this feature, I think you should
> implement it rather than leaving it to Tom or I.
>

Or anyone else :-)

> The reason why I was initially skeptical of adding a YAML output
> format is that JSON is a subset of YAML. Therefore, the JSON output
> format ought to be perfectly sufficient for anyone using a YAML
> parser.
>

There is some debate on this point, IIRC.

cheers

andrew

"Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
> I don't think the above would be particularly hard to implement myself,
> but if it becomes a really big deal, we can certainly punt by simply
> quoting anything containing an indicator (the special characters above).

I would go with that. The quoting rules you proposed previously seem
way too complicated --- meaning potentially buggy, and even if they're
not buggy, the behavior would seem unpredictable to most users.

regards, tom lane

On 7 June 2010 15:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
>> I don't think the above would be particularly hard to implement myself,
>> but if it becomes a really big deal, we can certainly punt by simply
>> quoting anything containing an indicator (the special characters above).
>
> I would go with that.  The quoting rules you proposed previously seem
> way too complicated --- meaning potentially buggy, and even if they're
> not buggy, the behavior would seem unpredictable to most users.
>

Well actually it's not just everything containing a special character,
it's also anything with leading or trailing whitespace, and empty
strings (not sure that can ever happen in practice).

It's because of the potential for bugs in this area, that I'd propose
just quoting everything (except numeric values) as in my original
patch.

Regards,
Dean

* Tom Lane:

> Egad ... this is supposed to be an easily machine-generatable format?

Perhaps you could surround all strings with "" in the generator, and
escape all potentially special characters (which seems to include some
whitespace even in quoted strings, unfortunately)?

It has been claimed before that YAML is a superset of JSON, so why
can't the YAML folks use the existing JSON output instead?

--
Florian Weimer <fweimer(at)bfk(dot)de>
BFK edv-consulting GmbH http://www.bfk.de/
Kriegsstraße 100 tel: +49-721-96201-1
D-76133 Karlsruhe fax: +49-721-96201-99

Florian Weimer wrote:
> It has been claimed before that YAML is a superset of JSON, so why
> can't the YAML folks use the existing JSON output instead?
>

Because JSON just crosses the line where it feels like there's so much
markup that people expect a tool is necessary to read it, which has
always been the issue with XML too--bad human readability. I was on the
fence about YAML until I used it for a client issue over the weekend. I
was able to hack together a quick tool to work on the issue that parsed
enough YAML *without using an external library* well enough for my
purposes in an hour, one that was still far more robust than a similar
hack trying to read plain old text format for EXPLAIN. And the client
was able to follow what was going on as I passed YAML output back and
forth with them. Just having every field labeled clearly cut off all
the usual "which of these is the startup cost again?" questions I'm used
to getting.

The complaints about YAML taking up too much vertical space are
understandable, but completely opposite of what I care about. I can
e-mail a customer a YAML plan and it will survive to the other side and
even in a reply back to me. Whereas any non-trivial text format one is
guaranteed to utterly destroyed by line wrapping along the way.

I think this thread could use a fresh example to remind anyone who
hasn't played with the curent YAML format what it looks like. Here's
one from a query against the Dell Store 2 database:

EXPLAIN SELECT * FROM customers WHERE customerid>1000 ORDER BY zip;
QUERY PLAN
----------
Sort (cost=4449.30..4496.80 rows=19000 width=268)
Sort Key: zip
-> Seq Scan on customers (cost=0.00..726.00 rows=19000 width=268)
Filter: (customerid > 1000)

EXPLAIN (FORMAT YAML) SELECT * FROM customers WHERE customerid>1000
ORDER BY zip;
QUERY PLAN
-------------------------------------
- Plan: +
Node Type: Sort +
Startup Cost: 4449.30 +
Total Cost: 4496.80 +
Plan Rows: 19000 +
Plan Width: 268 +
Sort Key: +
- zip +
Plans: +
- Node Type: Seq Scan +
Parent Relationship: Outer +
Relation Name: customers +
Alias: customers +
Startup Cost: 0.00 +
Total Cost: 726.00 +
Plan Rows: 19000 +
Plan Width: 268 +
Filter: (customerid > 1000)

--
Greg Smith 2ndQuadrant US Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.us

Greg Smith <greg(at)2ndquadrant(dot)com> writes:
> The complaints about YAML taking up too much vertical space are
> understandable, but completely opposite of what I care about. I can
> e-mail a customer a YAML plan and it will survive to the other side and
> even in a reply back to me. Whereas any non-trivial text format one is
> guaranteed to utterly destroyed by line wrapping along the way.

> I think this thread could use a fresh example to remind anyone who
> hasn't played with the curent YAML format what it looks like.

So? This doesn't look amazingly unlike the current JSON output,
and to the extent that we have to add more quoting to it, it's
going to look even more like the JSON output.

Given the lack of any field separators other than newlines, I'm also
finding myself extremely doubtful about the claim that it survives
line-wrapping mutilations well. For instance this bit:

- Node Type: Seq Scan
Parent Relationship: Outer

doesn't appear to have anything but whitespace to distinguish it from

- Node Type: Seq Scan Parent
Relationship: Outer

regards, tom lane

> It's because of the potential for bugs in this area, that I'd propose
> just quoting everything (except numeric values) as in my original
> patch.

I don't see a problem with this.

I supported YAML output because I find it easier to read and copy&paste
than the other outputs. This is still the case even with quoting. And
it's not exactly a hugely intrusive patch.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

Tom Lane wrote:
> This doesn't look amazingly unlike the current JSON output,
> and to the extent that we have to add more quoting to it, it's
> going to look even more like the JSON output.
>

I don't know about that; here's the JSON one:

EXPLAIN (FORMAT JSON) SELECT * FROM customers WHERE customerid>1000
ORDER BY zip;
QUERY PLAN
-------------------------------------------
[ +
{ +
"Plan": { +
"Node Type": "Sort", +
"Startup Cost": 4449.30, +
"Total Cost": 4496.80, +
"Plan Rows": 19000, +
"Plan Width": 268, +
"Sort Key": ["zip"], +
"Plans": [ +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Outer",+
"Relation Name": "customers", +
"Alias": "customers", +
"Startup Cost": 0.00, +
"Total Cost": 726.00, +
"Plan Rows": 19000, +
"Plan Width": 268, +
"Filter": "(customerid > 1000)"+
} +
] +
} +
} +
]

From the perspective of how that's less useful as a human form of
output, it's longer, wider, and has redundant punctuation that gets in
the way.

I think that YAML quoting will need to respect one of the special cases
to keep from ruining its readability: "Requires quoting only if the
first character" for " will make its current format look terrible if
that rule is applied to the whole line instead. That sounds like a
necessary special case to include: don't quote any quote characters
that appear unless they're the first character on the line. Everything
else could switch back to really aggressive quoting in every spot and
that wouldn't hurt the readability of the format very much IMHO.

> Given the lack of any field separators other than newlines, I'm also
> finding myself extremely doubtful about the claim that it survives
> line-wrapping mutilations well.

All I was claiming there is that the output is dramatically less wide
than the standard text format of the same plan, and therefore far less
likely to get nailed by a mail client that wraps at normal line widths.
Agreed that once wrapping does occur, it has serious problems too.

Here are the stats for this plan, leaving off the QUERY PLAN header from
each:

TEXT: 4 vertical, 69 horizontal
YAML: 18 vertical, 36 horizontal
JSON: 25 vertical, 43 horizontal
XML[1]: 27 vertical, 60 horizontal

Quote the TEXT line with "> " or get a plan with one more line of
intendation, and you're likely to get wrapped badly at the 72 character
line limit some clients use. Quite a bit more headroom before the YAML
format will wrap like that; JSON is in the middle.

I now see plenty of use for YAML when exchanging plans over e-mail, and
it's a bonus that should survive that format to be parseable on the
other side. JSON and XML are certainly the preferred way to feed plans
into analysis tools. unambiguously.

[1] Might as well make this a complete example:

<explain xmlns="http://www.postgresql.org/2009/explain"> +
<Query> +
<Plan> +
<Node-Type>Sort</Node-Type> +
<Startup-Cost>4449.30</Startup-Cost> +
<Total-Cost>4496.80</Total-Cost> +
<Plan-Rows>19000</Plan-Rows> +
<Plan-Width>268</Plan-Width> +
<Sort-Key> +
<Item>zip</Item> +
</Sort-Key> +
<Plans> +
<Plan> +
<Node-Type>Seq Scan</Node-Type> +
<Parent-Relationship>Outer</Parent-Relationship>+
<Relation-Name>customers</Relation-Name> +
<Alias>customers</Alias> +
<Startup-Cost>0.00</Startup-Cost> +
<Total-Cost>726.00</Total-Cost> +
<Plan-Rows>19000</Plan-Rows> +
<Plan-Width>268</Plan-Width> +
<Filter>(customerid &gt; 1000)</Filter> +
</Plan> +
</Plans> +
</Plan> +
</Query> +
</explain>

--
Greg Smith 2ndQuadrant US Baltimore, MD
PostgreSQL Training, Services and Support
greg(at)2ndQuadrant(dot)com www.2ndQuadrant.us

* Greg Smith:

> Florian Weimer wrote:
>> It has been claimed before that YAML is a superset of JSON, so why
>> can't the YAML folks use the existing JSON output instead?
>>
>
> Because JSON just crosses the line where it feels like there's so much
> markup that people expect a tool is necessary to read it, which has
> always been the issue with XML too--bad human readability.

But YAML is not human-readable. There are human-readable subsets of
it, but the general serializers do not produce them, and specific
serializers are difficult to get right (as we've seen).

> EXPLAIN (FORMAT YAML) SELECT * FROM customers WHERE customerid>1000
> ORDER BY zip;
> QUERY PLAN
> -------------------------------------
> - Plan: +
> Node Type: Sort +
> Startup Cost: 4449.30 +
> Total Cost: 4496.80 +
> Plan Rows: 19000 +
> Plan Width: 268 +
> Sort Key: +
> - zip +
> Plans: +
> - Node Type: Seq Scan +
> Parent Relationship: Outer +
> Relation Name: customers +
> Alias: customers +
> Startup Cost: 0.00 +
> Total Cost: 726.00 +
> Plan Rows: 19000 +
> Plan Width: 268 +
> Filter: (customerid > 1000)

What does your parser do with this (equivalent but shorter) YAML
output?

- Plan: !!map
&0 Node Type: Sort
&1 Startup Cost: 4449.30
&2 Total Cost: 4496.80
&3 Plan Rows: &5 19000
&4 Plan Width: &6 268
Sort Key: ["zip"]
Plans: !!seq
- *0: Seq Scan
Parent Relationship: Outer
Relation Name: &7 customers
Alias: *7
*1: 0.00
*2: 726.00
*3: *5
*4: *6
Filter: (customerid > 1000)

Looking at the spec, it's rather difficult to come up with a readable
subset which can parsed easily and is general in the sense that it can
express empty strings, strings with embedded newlines, and so on.
YAML's rules for dealing with whitespace are fairly complex, but are
probably needed to get a more compact notation than JSON.

--
Florian Weimer <fweimer(at)bfk(dot)de>
BFK edv-consulting GmbH http://www.bfk.de/
Kriegsstraße 100 tel: +49-721-96201-1
D-76133 Karlsruhe fax: +49-721-96201-99

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> But YAML is not human-readable. There are human-readable subsets of
> it, but the general serializers do not produce them, and specific
> serializers are difficult to get right (as we've seen).

No, it *is* human readable. Indeed, that's one of the things that
differentiates it from JSON: readability is the main goal, whereas
JSON's goals are different. The readablity necessarily makes
the parsing rules more complex, but that's the implicit tradeoff.
(Did you miss the part where the other Greg is sending explain
plans via email?)

> What does your parser do with this (equivalent but shorter)
> YAML output?
>
> - Plan: !!map
> &0 Node Type: Sort
> &1 Startup Cost: 4449.30
> &2 Total Cost: 4496.80
> &3 Plan Rows: &5 19000
> &4 Plan Width: &6 268
> Sort Key: ["zip"]
> Plans: !!seq
> - *0: Seq Scan
> Parent Relationship: Outer
> Relation Name: &7 customers
> Alias: *7
> *1: 0.00
> *2: 726.00
> *3: *5
> *4: *6
> Filter: (customerid > 1000)

But we're not using alias nodes (nor would we ever want to), so I'm not
sure what the point of your contrived example is. That's shorter, but
certainly not easier to read by human /or/ machine.

> Looking at the spec, it's rather difficult to come up with a readable
> subset which can parsed easily and is general in the sense that it can
> express empty strings, strings with embedded newlines, and so on.
> YAML's rules for dealing with whitespace are fairly complex, but are
> probably needed to get a more compact notation than JSON.

I'll state that both embedded newlines and column names and values with
funny characters like '*' and '|' are rare events, and the great majority
of things you'll see in an explain plan are plain ol' ASCII, in which
YAML produces a very good representation. But you are right that we need
to make sure we are handling the whitespace correctly.

When I get some free time, I'll make a patch to implement as much of
the spec as we sanely can. As I said before, I don't think we need to
strive for putting everything we possibly can into "plain scalar"
objects, as we can cover 99% of the cases easy enough and fall back to
'when in doubt, quote' for the rest.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
End Point Corporation http://www.endpoint.com/
PGP Key: 0x14964AC8 201006080931
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkwOR2gACgkQvJuQZxSWSshkVwCgzqunUkawnBRGwOV8msQPudN8
UmkAoM1wz+wFCEz34CMJ7VH+S7T3mc43
=8OjG
-----END PGP SIGNATURE-----

On Tue, Jun 8, 2010 at 9:37 AM, Greg Sabino Mullane <greg(at)turnstep(dot)com> wrote:
> When I get some free time, I'll make a patch to implement as much of
> the spec as we sanely can.

Saying that you'll fix it but not on any particular timetable is
basically equivalent to saying that you're not willing to fix it at
all. We are trying to get a release out the door. I'm not trying to
be rude, but it's frustrating to me when people object to having their
code ripped out but also won't commit to getting it fixed in a timely
fashion.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Mon, Jun 7, 2010 at 4:14 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> Testing 9.0 beta, I found that EXPLAINing certain queries in YAML
> format will produce invalid YAML, for example:
>
> explain (format yaml) select * from foo where str_val = 'a: b';
>
> The problem in this case is that a colon followed by whitespace is not
> allowed in an unquoted plain YAML string because a parser would
> interpret it as the start of a map.
>
> So the current code in escape_yaml() is inadequate for producing valid
> YAML. I think it would have to also consider at least the following
> characters as special "-"  ":"  "["  "]"  "{"  "}"  ","  "\""  "'"
> "|"  "*"  "&". Technically, it would also need to trap empty strings,
> and strings with leading or trailing whitespace.
>
> Making escape_yaml() completely bulletproof with this approach would
> be quite difficult, and (IMO) not worth the effort, especially given
> that an important requirement is that the output be machine readable,
> and in my experience YAML parsers are often far from perfect.
>
> I would therefore argue for simply calling escape_json() to produce
> double quoted output for all string values, and only have numeric
> values unquoted. This is not really any less human readable, and is
> far more machine readable.
>
> Patch attached.

I've committed a patch which I think will address this issue without
uglifying the output quite so much. Also, I didn't like the idea of
not applying escaping to both the keys and values, even though we
think we'll never have a key that requires escaping. With this
approach, that change isn't needed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Tue, Jun 8, 2010 at 10:47 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jun 7, 2010 at 4:14 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> Testing 9.0 beta, I found that EXPLAINing certain queries in YAML
>> format will produce invalid YAML, for example:
>>
>> explain (format yaml) select * from foo where str_val = 'a: b';
>>
>> The problem in this case is that a colon followed by whitespace is not
>> allowed in an unquoted plain YAML string because a parser would
>> interpret it as the start of a map.
>>
>> So the current code in escape_yaml() is inadequate for producing valid
>> YAML. I think it would have to also consider at least the following
>> characters as special "-"  ":"  "["  "]"  "{"  "}"  ","  "\""  "'"
>> "|"  "*"  "&". Technically, it would also need to trap empty strings,
>> and strings with leading or trailing whitespace.
>>
>> Making escape_yaml() completely bulletproof with this approach would
>> be quite difficult, and (IMO) not worth the effort, especially given
>> that an important requirement is that the output be machine readable,
>> and in my experience YAML parsers are often far from perfect.
>>
>> I would therefore argue for simply calling escape_json() to produce
>> double quoted output for all string values, and only have numeric
>> values unquoted. This is not really any less human readable, and is
>> far more machine readable.
>>
>> Patch attached.
>
> I've committed a patch which I think will address this issue without
> uglifying the output quite so much.  Also, I didn't like the idea of
> not applying escaping to both the keys and values, even though we
> think we'll never have a key that requires escaping.  With this
> approach, that change isn't needed.

Er, I should also say, thanks for the report, and please test. I am
definitely not an expert on YAML.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Er, I should also say, thanks for the report, and please test.  I am
> definitely not an expert on YAML.
>

I'm not an expert on YAML either, but I don't think this works (at
least it breaks against the online YAML parser here:
http://yaml-online-parser.appspot.com/). If the string starts with a
".", then it tries to treat it as a floating point number and baulks
if the rest of the string isn't a valid number.

Actually that gives me another argument for quoting *all* string
values: if a string value happens to be a valid number and we output
it unquoted, then parsers will treat is a number, forcing the user to
cast it to a string in their code. This is likely to lead to bugs in
user code, where things initially appear to work, then unexpectedly
start failing in pathalogical cases.

Personally, I'd also not try to escape keys at all. If in the future
we add a new key, then we'd be forced to choose between quoting it, or
more likely choosing a different key that doesn't require quoting,
which is what most people do in my (admittedly limited) experience.

In short, despite everything that's been said on this thread, I still
prefer my patch - but then I suppose I would say that :-)

Cheers,
Dean

On 9 June 2010 07:58, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> I prefer my patch - but then I suppose I would say that :-)
>

Seriously though, I can think of a number of good arguments in favour
of the "quote all string values unconditionally" approach:

- It's the simplest, least obtrusive fix to our code.
- It's the least likely to have bugs in our code.
- It's the least likely to lead to bugs in client code.
- It gives clean, consistent output (IMO).
- It's the easiest to machine parse (marginally).
- Strings are always strings.
- It's still perfectly human readable.
- Re arguments about mailers chopping it up, it's easier to piece
together again if you know all long lines end with a double quote.

I love Florian's example, because it illustrates that there are all
sorts of YAML output that we *could* produce that would be technically
equivalent. However, I think this is a case of the simplest solution
being the best.

Regards,
Dean

On Wed, Jun 9, 2010 at 2:58 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Er, I should also say, thanks for the report, and please test.  I am
>> definitely not an expert on YAML.
>>
>
> I'm not an expert on YAML either, but I don't think this works (at
> least it breaks against the online YAML parser here:
> http://yaml-online-parser.appspot.com/). If the string starts with a
> ".", then it tries to treat it as a floating point number and baulks
> if the rest of the string isn't a valid number.

Really? I enter:

- foo
- bar
- .baz

And it produces this JSON:

[
"foo",
"bar",
".baz"
]

That looks OK to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 12:07, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 2:58 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> Er, I should also say, thanks for the report, and please test.  I am
>>> definitely not an expert on YAML.
>>>
>>
>> I'm not an expert on YAML either, but I don't think this works (at
>> least it breaks against the online YAML parser here:
>> http://yaml-online-parser.appspot.com/). If the string starts with a
>> ".", then it tries to treat it as a floating point number and baulks
>> if the rest of the string isn't a valid number.
>
> Really?  I enter:
>
> - foo
> - bar
> - .baz
>
> And it produces this JSON:
>
> [
>  "foo",
>  "bar",
>  ".baz"
> ]
>
> That looks OK to me.
>

Ah, OK I didn't test those cases properly before composing my email.
It's actually only a "." on its own that it can't parse.

- just: write some
- yaml:
- .

ERROR:

invalid literal for float(): .

I'm not sure if that's valid YAML or not.

My comment about numbers still applies though. The following are
different values:

- just: write some
- yaml:
- 123
- "123"

[
{
"just": "write some"
},
{
"yaml": [
123,
"123"
]
}
]

Regards,
Dean

On Wed, Jun 9, 2010 at 7:23 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 9 June 2010 12:07, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jun 9, 2010 at 2:58 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> Er, I should also say, thanks for the report, and please test.  I am
>>>> definitely not an expert on YAML.
>>>>
>>>
>>> I'm not an expert on YAML either, but I don't think this works (at
>>> least it breaks against the online YAML parser here:
>>> http://yaml-online-parser.appspot.com/). If the string starts with a
>>> ".", then it tries to treat it as a floating point number and baulks
>>> if the rest of the string isn't a valid number.
>>
>> Really?  I enter:
>>
>> - foo
>> - bar
>> - .baz
>>
>> And it produces this JSON:
>>
>> [
>>  "foo",
>>  "bar",
>>  ".baz"
>> ]
>>
>> That looks OK to me.
>>
>
> Ah, OK I didn't test those cases properly before composing my email.
> It's actually only a "." on its own that it can't parse.

Well, at first blush, that looks like it might be a bug in the parser.
I don't see anything in the spec to indicate that that case should be
treated specially.

> My comment about numbers still applies though. The following are
> different values:
>
> - just: write some
> - yaml:
>  - 123
>  - "123"

Well, you can't have abc mean the same thing as "abc" but then
complain that 123 isn't equivalent to "123"...

This format is really a pain to work with.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 12:32, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 7:23 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 12:07, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jun 9, 2010 at 2:58 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>>> Er, I should also say, thanks for the report, and please test.  I am
>>>>> definitely not an expert on YAML.
>>>>>
>>>>
>>>> I'm not an expert on YAML either, but I don't think this works (at
>>>> least it breaks against the online YAML parser here:
>>>> http://yaml-online-parser.appspot.com/). If the string starts with a
>>>> ".", then it tries to treat it as a floating point number and baulks
>>>> if the rest of the string isn't a valid number.
>>>
>>> Really?  I enter:
>>>
>>> - foo
>>> - bar
>>> - .baz
>>>
>>> And it produces this JSON:
>>>
>>> [
>>>  "foo",
>>>  "bar",
>>>  ".baz"
>>> ]
>>>
>>> That looks OK to me.
>>>
>>
>> Ah, OK I didn't test those cases properly before composing my email.
>> It's actually only a "." on its own that it can't parse.
>
> Well, at first blush, that looks like it might be a bug in the parser.
>  I don't see anything in the spec to indicate that that case should be
> treated specially.
>

Yeah, I think it *is* valid, and JYaml parses it OK.
Some people will no doubt say "if your parser can't handle it, get a
better parser", but I'd rather not make it more difficult than it
needs to be.

>> My comment about numbers still applies though. The following are
>> different values:
>>
>> - just: write some
>> - yaml:
>>  - 123
>>  - "123"
>
> Well, you can't have abc mean the same thing as "abc" but then
> complain that 123 isn't equivalent to "123"...
>

Yeah. I know that JYaml parses 123 to a java.lang.Integer, and "123"
to a java.lang.String.

> This format is really a pain to work with.
>

Agreed :-(

- Dean

On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> please test.

Well your patch definitely fixes my original bug, and AFAICT always
produces valid YAML output now. I've only found one case where a
particular parser has difficulty parsing the output, and you'd have to
write a pretty perverse query to hit that case.

So that just leaves this sort of thing:

explain (format yaml) select * from foo as "123";
QUERY PLAN
-------------------------
- Plan: +
Node Type: Seq Scan+
Relation Name: foo +
Alias: 123 +
Startup Cost: 0.00 +
Total Cost: 23.10 +
Plan Rows: 1310 +
Plan Width: 32
(1 row)

Does anyone care that Alias will sometimes be a string, and sometimes a number?

ITSM that, since postgresql knows that it's a string, it ought to
output something that parsers can unambiguously treat as a string too.

But this is also a pretty obscure case that probably only someone
deliberately trying to be awkward would do (which is me, with my
tester hat on :-)).

Regards,
Dean

On Wed, Jun 9, 2010 at 8:46 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> please test.
>
> Well your patch definitely fixes my original bug, and AFAICT always
> produces valid YAML output now. I've only found one case where a
> particular parser has difficulty parsing the output, and you'd have to
> write a pretty perverse query to hit that case.

Excellent.

> So that just leaves this sort of thing:
>
> explain (format yaml) select * from foo as "123";
>       QUERY PLAN
> -------------------------
>  - Plan:                +
>     Node Type: Seq Scan+
>     Relation Name: foo +
>     Alias: 123         +
>     Startup Cost: 0.00 +
>     Total Cost: 23.10  +
>     Plan Rows: 1310    +
>     Plan Width: 32
> (1 row)
>
> Does anyone care that Alias will sometimes be a string, and sometimes a number?
>
> ITSM that, since postgresql knows that it's a string, it ought to
> output something that parsers can unambiguously treat as a string too.
>
> But this is also a pretty obscure case that probably only someone
> deliberately trying to be awkward would do (which is me, with my
> tester hat on :-)).

I guess we could do this by (a) conditionalizing the YAML case in
ExplainProperty() in the same way that the JSON case is currently
conditionalized, and (b) changing the first if statement in
escape_yaml() to set needs_quoting = true unless the first character
is alphabetic or an underscore.

By the way, can I ask why you're not just using the JSON format for
this? I mean, I'm glad you are, because it exposed a bug that we got
fixed before release, but it seems a little masochistic...!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 14:14, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 8:46 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 03:48, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> please test.
>>
>> Well your patch definitely fixes my original bug, and AFAICT always
>> produces valid YAML output now. I've only found one case where a
>> particular parser has difficulty parsing the output, and you'd have to
>> write a pretty perverse query to hit that case.
>
> Excellent.
>
>> So that just leaves this sort of thing:
>>
>> explain (format yaml) select * from foo as "123";
>>       QUERY PLAN
>> -------------------------
>>  - Plan:                +
>>     Node Type: Seq Scan+
>>     Relation Name: foo +
>>     Alias: 123         +
>>     Startup Cost: 0.00 +
>>     Total Cost: 23.10  +
>>     Plan Rows: 1310    +
>>     Plan Width: 32
>> (1 row)
>>
>> Does anyone care that Alias will sometimes be a string, and sometimes a number?
>>
>> ITSM that, since postgresql knows that it's a string, it ought to
>> output something that parsers can unambiguously treat as a string too.
>>
>> But this is also a pretty obscure case that probably only someone
>> deliberately trying to be awkward would do (which is me, with my
>> tester hat on :-)).
>
> I guess we could do this by (a) conditionalizing the YAML case in
> ExplainProperty() in the same way that the JSON case is currently
> conditionalized, and (b) changing the first if statement in
> escape_yaml() to set needs_quoting = true unless the first character
> is alphabetic or an underscore.
>

Yes, I think that would do it.

> By the way, can I ask why you're not just using the JSON format for
> this?  I mean, I'm glad you are, because it exposed a bug that we got
> fixed before release, but it seems a little masochistic...!
>

Actually I doubt that I will use this feature at all! I only use
EXPLAIN from psql, and usually I'm the only one who needs to read it,
so the TEXT format will remain my preferred option.

I was just doing some random beta testing, working through the list of
cool new features.

Dean

On Wed, Jun 9, 2010 at 9:35 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> Does anyone care that Alias will sometimes be a string, and sometimes a number?
>> I guess we could do this by (a) conditionalizing the YAML case in
>> ExplainProperty() in the same way that the JSON case is currently
>> conditionalized, and (b) changing the first if statement in
>> escape_yaml() to set needs_quoting = true unless the first character
>> is alphabetic or an underscore.
> Yes, I think that would do it.

After further review, it appears to me that this change is pretty much
required, because otherwise a string like 0xa won't be quoted. I
might think it's OK for "123" to turn into 123, but I'm not going to
be so happy about "0xa" turning into 10. Please test the attached
patch.

>> By the way, can I ask why you're not just using the JSON format for
>> this?  I mean, I'm glad you are, because it exposed a bug that we got
>> fixed before release, but it seems a little masochistic...!
>
> Actually I doubt that I will use this feature at all! I only use
> EXPLAIN from psql, and usually I'm the only one who needs to read it,
> so the TEXT format will remain my preferred option.
>
> I was just doing some random beta testing, working through the list of
> cool new features.

Quick, somebody give this man a cigar!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 9, 2010 at 9:35 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>> Does anyone care that Alias will sometimes be a string, and sometimes a number?

> After further review, it appears to me that this change is pretty much
> required, because otherwise a string like 0xa won't be quoted. I
> might think it's OK for "123" to turn into 123, but I'm not going to
> be so happy about "0xa" turning into 10. Please test the attached
> patch.

I still agree with Dean's original proposal: always quote the values of
strings.

regards, tom lane

On Wed, Jun 9, 2010 at 11:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jun 9, 2010 at 9:35 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>> Does anyone care that Alias will sometimes be a string, and sometimes a number?
>
>> After further review, it appears to me that this change is pretty much
>> required, because otherwise a string like 0xa won't be quoted.  I
>> might think it's OK for "123" to turn into 123, but I'm not going to
>> be so happy about "0xa" turning into 10.  Please test the attached
>> patch.
>
> I still agree with Dean's original proposal: always quote the values of
> strings.

I'd still rather rip the format out entirely than do that. Dean's
proposal was based on the idea that it would be safe to quote only the
values and not the keys, which is not something I care to bank on.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 9, 2010 at 11:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I still agree with Dean's original proposal: always quote the values of
>> strings.

> I'd still rather rip the format out entirely than do that.

I'd be on board with that too ;-)

> Dean's
> proposal was based on the idea that it would be safe to quote only the
> values and not the keys, which is not something I care to bank on.

Why not? Surely we can restrict EXPLAIN's set of key names to be safe.

regards, tom lane

On 9 June 2010 16:05, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 11:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Jun 9, 2010 at 9:35 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>>>>> Does anyone care that Alias will sometimes be a string, and sometimes a number?
>>
>>> After further review, it appears to me that this change is pretty much
>>> required, because otherwise a string like 0xa won't be quoted.  I
>>> might think it's OK for "123" to turn into 123, but I'm not going to
>>> be so happy about "0xa" turning into 10.  Please test the attached
>>> patch.
>>
>> I still agree with Dean's original proposal: always quote the values of
>> strings.
>
> I'd still rather rip the format out entirely than do that.  Dean's
> proposal was based on the idea that it would be safe to quote only the
> values and not the keys, which is not something I care to bank on.
>

Why? There are only a small number of keys, and we're completely in
control of them. You'd very quickly notice if one of them needed
quoting, which is not the case with values.

Dean

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:

> So that just leaves this sort of thing:
>
> explain (format yaml) select * from foo as "123";

What about other quoted identifiers? If I recall correctly, a
quoted identifier can contain *any* characters (although a quote
must be represented as two adjacent quotes).

test=# create schema "0";
CREATE SCHEMA
test=# create table "0"."123" ("456" int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"123_pkey" for table "123"
CREATE TABLE
test=# explain (format yaml) select * from "0"."123" where "456" =
'789';
QUERY PLAN
-----------------------------------
- Plan: +
Node Type: Index Scan +
Scan Direction: Forward +
Index Name: "\"123_pkey\"" +
Relation Name: 123 +
Alias: 123 +
Startup Cost: 0.00 +
Total Cost: 8.27 +
Plan Rows: 1 +
Plan Width: 4 +
Index Cond: "(\"456\" = 789)"
(1 row)

Also, the trailing spaces in this format (stripped from this email
to avoid odd spacing) are more than a little annoying.

-Kevin

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

Robert Haas wrote:
>> When I get some free time, I'll make a patch to implement as
>> much of the spec as we sanely can.

> Saying that you'll fix it but not on any particular timetable is
> basically equivalent to saying that you're not willing to fix it at
> all.

It's not equivalent at all. If I wasn't willing to fix it all,
I'd say so.

> We are trying to get a release out the door. I'm not trying to
> be rude, but it's frustrating to me when people object to having their
> code ripped out but also won't commit to getting it fixed in a timely
> fashion.

You might not be trying, but you are coming across as quite rude. The
bug was only reported Monday morning, and you are yelling at me
on a Tuesday night for not being willing to drop everything I'm doing
and fix it right now? Yes, we're heading towards 9.0 and yes, I'd
sure hate to see YAML ripped out (especially now that it's been
listed near and far as one of our new features), but I've got bills
to pay and writing a patch is a volunteer effort for me.

Since you seem so keen on telling other people what they should be
doing, here's some of your own medicine: why not focus on something
other than YAML, which myself and many other people can write, and
work more on the 9.0 open issues that your energy and expertise
would be more suited for?

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 201006091156
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkwPuawACgkQvJuQZxSWSshqSwCgyUoNhi8r/ug/joERXJfJF4mu
3h4AoOtLUHWcN3udePN1Ne2jc+gBa/uS
=OtxW
-----END PGP SIGNATURE-----

On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jun 9, 2010 at 11:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I still agree with Dean's original proposal: always quote the values of
>>> strings.
>
>> I'd still rather rip the format out entirely than do that.
>
> I'd be on board with that too ;-)
>
>> Dean's
>> proposal was based on the idea that it would be safe to quote only the
>> values and not the keys, which is not something I care to bank on.
>
> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.

It seems to me that it would be easy for a future patch to break this
by accident.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Why not? Surely we can restrict EXPLAIN's set of key names to be safe.

> It seems to me that it would be easy for a future patch to break this
> by accident.

Really? What likely key names would be in need of quoting? I can't
imagine accepting a field name that contains punctuation or leading
or trailing whitespace, for example.

regards, tom lane

On 9 June 2010 17:25, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>
>> It seems to me that it would be easy for a future patch to break this
>> by accident.
>
> Really?  What likely key names would be in need of quoting?  I can't
> imagine accepting a field name that contains punctuation or leading
> or trailing whitespace, for example.
>

And any of those things would break the XML output too.
I can't imagine that going unnoticed.

Dean

On Wed, Jun 9, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>
>> It seems to me that it would be easy for a future patch to break this
>> by accident.
>
> Really?  What likely key names would be in need of quoting?  I can't
> imagine accepting a field name that contains punctuation or leading
> or trailing whitespace, for example.

It seemed to me, in particular, that someone might use a # symbol,
like "# of Iterations".

Maybe I'm being paranoid.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 17:52, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>>
>>> It seems to me that it would be easy for a future patch to break this
>>> by accident.
>>
>> Really?  What likely key names would be in need of quoting?  I can't
>> imagine accepting a field name that contains punctuation or leading
>> or trailing whitespace, for example.
>
> It seemed to me, in particular, that someone might use a # symbol,
> like "# of Iterations".
>

Then the resulting XML tagname would be invalid too
I think they would soon realise/be told that it was a bad idea.

Dean

On Wed, Jun 9, 2010 at 12:57 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 9 June 2010 17:52, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jun 9, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>>>
>>>> It seems to me that it would be easy for a future patch to break this
>>>> by accident.
>>>
>>> Really?  What likely key names would be in need of quoting?  I can't
>>> imagine accepting a field name that contains punctuation or leading
>>> or trailing whitespace, for example.
>>
>> It seemed to me, in particular, that someone might use a # symbol,
>> like "# of Iterations".
>>
>
> Then the resulting XML tagname would be invalid too
> I think they would soon realise/be told that it was a bad idea.

Hmm, you're right. Maybe we should go with your approach, then.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, Jun 9, 2010 at 11:57 AM, Greg Sabino Mullane <greg(at)turnstep(dot)com> wrote:
> The bug was only reported Monday morning, and you are yelling at me
> on a Tuesday night for not being willing to drop everything I'm doing
> and fix it right now?

I am not saying and have not said that you needed to drop everything
you were doing and fix it right now. Had you said, "I will get a
patch for this out this week", that would have been fine with me.
What I did and do object to is that your commitment to fix it was
completely open-ended. From reading your email, there's no way for
someone to know whether you'll get to this in two days or a month, and
a month, at least in my opinion, is too long, maybe at any time but
certainly at this point in the release cycle.

> Since you seem so keen on telling other people what they should be
> doing, here's some of your own medicine: why not focus on something
> other than YAML, which myself and many other people can write, and
> work more on the 9.0 open issues that your energy and expertise
> would be more suited for?

I think this comment is a little snide, but it deserves a serious
response. I spent most of yesterday afternoon and evening working on
every open item that I had a clue about, and another two hours this
morning. Most of the remaining items are either things that I am not
qualified to fix or for which there are currently patches out for
comment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, Jun 9, 2010 at 12:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 12:57 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 17:52, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jun 9, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>>>>
>>>>> It seems to me that it would be easy for a future patch to break this
>>>>> by accident.
>>>>
>>>> Really?  What likely key names would be in need of quoting?  I can't
>>>> imagine accepting a field name that contains punctuation or leading
>>>> or trailing whitespace, for example.
>>>
>>> It seemed to me, in particular, that someone might use a # symbol,
>>> like "# of Iterations".
>>>
>>
>> Then the resulting XML tagname would be invalid too
>> I think they would soon realise/be told that it was a bad idea.
>
> Hmm, you're right.  Maybe we should go with your approach, then.

After thinking about this further, I think I'd still like to take one
more crack at fixing this without quoting absolutely everything. I
argued against this feature, but we decided to take it, and it seems
that one of the major arguments that is being put forward is that it
will be more readable than JSON, because it will have less
punctuation. While the idea of optimizing a machine-readable format
for human-readability doesn't typically carry much water around here,
it's really the only use case for having this particular feature at
all, so, if we're not going to rip it out, ISTM we ought to respect
what it's there for. I would be more than willing to agree that if
one more attempt isn't sufficient to fix the problem then we'll either
quote everything, or rip the whole thing out.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, Jun 9, 2010 at 12:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 12:57 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 17:52, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jun 9, 2010 at 12:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>>> On Wed, Jun 9, 2010 at 11:14 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>>> Why not?  Surely we can restrict EXPLAIN's set of key names to be safe.
>>>>
>>>>> It seems to me that it would be easy for a future patch to break this
>>>>> by accident.
>>>>
>>>> Really?  What likely key names would be in need of quoting?  I can't
>>>> imagine accepting a field name that contains punctuation or leading
>>>> or trailing whitespace, for example.
>>>
>>> It seemed to me, in particular, that someone might use a # symbol,
>>> like "# of Iterations".
>>>
>>
>> Then the resulting XML tagname would be invalid too
>> I think they would soon realise/be told that it was a bad idea.
>
> Hmm, you're right.  Maybe we should go with your approach, then.

After thinking about this further, I think I'd still like to take one
more crack at fixing this without quoting absolutely everything. I
argued against this feature, but we decided to take it, and it seems
that one of the major arguments that is being put forward is that it
will be more readable than JSON, because it will have less
punctuation. While the idea of optimizing a machine-readable format
for human-readability doesn't typically carry much water around here,
it's really the only use case for having this particular feature at
all, so, if we're not going to rip it out, ISTM we ought to respect
what it's there for. I would be more than willing to agree that if
one more attempt isn't sufficient to fix the problem then we'll either
quote everything, or rip the whole thing out.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 19:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> After thinking about this further, I think I'd still like to take one
> more crack at fixing this without quoting absolutely everything.  I
> argued against this feature, but we decided to take it, and it seems
> that one of the major arguments that is being put forward is that it
> will be more readable than JSON, because it will have less
> punctuation.

Hmm. Well it's quite subjective, but IMO it's already more readable
than JSON regardless of whether or not values are quoted, simply
because it doesn't have [ ] and { } for lists and maps, which for JSON
adds significantly to the number of lines in longer plans.

Compare

QUERY PLAN
----------------------------------------
- Plan: +
Node Type: Nested Loop +
Join Type: Inner +
Startup Cost: 0.00 +
Total Cost: 57313.15 +
Plan Rows: 4579600 +
Plan Width: 16 +
Plans: +
- Node Type: Seq Scan +
Parent Relationship: Outer +
Relation Name: foo +
Alias: f1 +
Startup Cost: 0.00 +
Total Cost: 31.40 +
Plan Rows: 2140 +
Plan Width: 8 +
- Node Type: Materialize +
Parent Relationship: Inner +
Startup Cost: 0.00 +
Total Cost: 42.10 +
Plan Rows: 2140 +
Plan Width: 8 +
Plans: +
- Node Type: Seq Scan +
Parent Relationship: Outer+
Relation Name: foo +
Alias: f2 +
Startup Cost: 0.00 +
Total Cost: 31.40 +
Plan Rows: 2140 +
Plan Width: 8

with
QUERY PLAN
-----------------------------------------------
[ +
{ +
"Plan": { +
"Node Type": "Nested Loop", +
"Join Type": "Inner", +
"Startup Cost": 0.00, +
"Total Cost": 57313.15, +
"Plan Rows": 4579600, +
"Plan Width": 16, +
"Plans": [ +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Outer", +
"Relation Name": "foo", +
"Alias": "f1", +
"Startup Cost": 0.00, +
"Total Cost": 31.40, +
"Plan Rows": 2140, +
"Plan Width": 8 +
}, +
{ +
"Node Type": "Materialize", +
"Parent Relationship": "Inner", +
"Startup Cost": 0.00, +
"Total Cost": 42.10, +
"Plan Rows": 2140, +
"Plan Width": 8, +
"Plans": [ +
{ +
"Node Type": "Seq Scan", +
"Parent Relationship": "Outer",+
"Relation Name": "foo", +
"Alias": "f2", +
"Startup Cost": 0.00, +
"Total Cost": 31.40, +
"Plan Rows": 2140, +
"Plan Width": 8 +
} +
] +
} +
] +
} +
} +
]

For me, the presence or absence of quotes around the values would make
little difference to the readability, compared to all those brackets.

> While the idea of optimizing a machine-readable format
> for human-readability doesn't typically carry much water around here,
> it's really the only use case for having this particular feature at
> all, so, if we're not going to rip it out, ISTM we ought to respect
> what it's there for.  I would be more than willing to agree that if
> one more attempt isn't sufficient to fix the problem then we'll either
> quote everything, or rip the whole thing out.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise Postgres Company
>

Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> Hmm. Well it's quite subjective, but IMO it's already more readable
> than JSON regardless of whether or not values are quoted, simply
> because it doesn't have [ ] and { } for lists and maps, which for JSON
> adds significantly to the number of lines in longer plans.

Yeah. Also, I think it would be fair to not quote values that are known
constants (for example, Node Type: Seq Scan) and are chosen to not need
quoting. It's just the things that are variables that worry me.

regards, tom lane

On Wed, Jun 9, 2010 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> Hmm. Well it's quite subjective, but IMO it's already more readable
>> than JSON regardless of whether or not values are quoted, simply
>> because it doesn't have [ ] and { } for lists and maps, which for JSON
>> adds significantly to the number of lines in longer plans.
>
> Yeah.  Also, I think it would be fair to not quote values that are known
> constants (for example, Node Type: Seq Scan) and are chosen to not need
> quoting.  It's just the things that are variables that worry me.

Passing down information about which things are known constants seems
more complicated to me than just getting the quoting rules right in
the first place. If you look at the patch I proposed, you'll see that
it's really quite simple and only a slight tightening of what I
committed already.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On 9 June 2010 20:56, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>>> Hmm. Well it's quite subjective, but IMO it's already more readable
>>> than JSON regardless of whether or not values are quoted, simply
>>> because it doesn't have [ ] and { } for lists and maps, which for JSON
>>> adds significantly to the number of lines in longer plans.
>>
>> Yeah.  Also, I think it would be fair to not quote values that are known
>> constants (for example, Node Type: Seq Scan) and are chosen to not need
>> quoting.  It's just the things that are variables that worry me.
>
> Passing down information about which things are known constants seems
> more complicated to me than just getting the quoting rules right in
> the first place.  If you look at the patch I proposed, you'll see that
> it's really quite simple and only a slight tightening of what I
> committed already.
>

Reading the YAML spec, I've just spotted yet another case that'll
break what you're proposing: if you don't quote "true" and "false",
the parser will think they're booleans rather than strings.

This is really why I'm opposed to this approach. There are just so
many gotchas that it's impossible to be 100% sure that you've
accounted for them all.

Regards,
Dean

On Wed, Jun 9, 2010 at 4:47 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> On 9 June 2010 20:56, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Jun 9, 2010 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>>>> Hmm. Well it's quite subjective, but IMO it's already more readable
>>>> than JSON regardless of whether or not values are quoted, simply
>>>> because it doesn't have [ ] and { } for lists and maps, which for JSON
>>>> adds significantly to the number of lines in longer plans.
>>>
>>> Yeah.  Also, I think it would be fair to not quote values that are known
>>> constants (for example, Node Type: Seq Scan) and are chosen to not need
>>> quoting.  It's just the things that are variables that worry me.
>>
>> Passing down information about which things are known constants seems
>> more complicated to me than just getting the quoting rules right in
>> the first place.  If you look at the patch I proposed, you'll see that
>> it's really quite simple and only a slight tightening of what I
>> committed already.
>>
>
> Reading the YAML spec, I've just spotted yet another case that'll
> break what you're proposing: if you don't quote "true" and "false",
> the parser will think they're booleans rather than strings.
>
> This is really why I'm opposed to this approach. There are just so
> many gotchas that it's impossible to be 100% sure that you've
> accounted for them all.

OK, I give up.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

On Wed, Jun 9, 2010 at 4:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 9, 2010 at 4:47 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> On 9 June 2010 20:56, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Wed, Jun 9, 2010 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>>>>> Hmm. Well it's quite subjective, but IMO it's already more readable
>>>>> than JSON regardless of whether or not values are quoted, simply
>>>>> because it doesn't have [ ] and { } for lists and maps, which for JSON
>>>>> adds significantly to the number of lines in longer plans.
>>>>
>>>> Yeah.  Also, I think it would be fair to not quote values that are known
>>>> constants (for example, Node Type: Seq Scan) and are chosen to not need
>>>> quoting.  It's just the things that are variables that worry me.
>>>
>>> Passing down information about which things are known constants seems
>>> more complicated to me than just getting the quoting rules right in
>>> the first place.  If you look at the patch I proposed, you'll see that
>>> it's really quite simple and only a slight tightening of what I
>>> committed already.
>>>
>>
>> Reading the YAML spec, I've just spotted yet another case that'll
>> break what you're proposing: if you don't quote "true" and "false",
>> the parser will think they're booleans rather than strings.
>>
>> This is really why I'm opposed to this approach. There are just so
>> many gotchas that it's impossible to be 100% sure that you've
>> accounted for them all.
>
> OK, I give up.

I have committed your patch, with some changes to the comments.

Thanks for bearing with me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas wrote:
> On Wed, Jun 9, 2010 at 4:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Wed, Jun 9, 2010 at 4:47 PM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> >> On 9 June 2010 20:56, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >>> On Wed, Jun 9, 2010 at 3:50 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>>> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> >>>>> Hmm. Well it's quite subjective, but IMO it's already more readable
> >>>>> than JSON regardless of whether or not values are quoted, simply
> >>>>> because it doesn't have [ ] and { } for lists and maps, which for JSON
> >>>>> adds significantly to the number of lines in longer plans.
> >>>>
> >>>> Yeah. ?Also, I think it would be fair to not quote values that are known
> >>>> constants (for example, Node Type: Seq Scan) and are chosen to not need
> >>>> quoting. ?It's just the things that are variables that worry me.
> >>>
> >>> Passing down information about which things are known constants seems
> >>> more complicated to me than just getting the quoting rules right in
> >>> the first place. ?If you look at the patch I proposed, you'll see that
> >>> it's really quite simple and only a slight tightening of what I
> >>> committed already.
> >>>
> >>
> >> Reading the YAML spec, I've just spotted yet another case that'll
> >> break what you're proposing: if you don't quote "true" and "false",
> >> the parser will think they're booleans rather than strings.
> >>
> >> This is really why I'm opposed to this approach. There are just so
> >> many gotchas that it's impossible to be 100% sure that you've
> >> accounted for them all.
> >
> > OK, I give up.
>
> I have committed your patch, with some changes to the comments.
>
> Thanks for bearing with me.

So, is there still value to a YAML format vs. JSON? They look similar
to me in this simple case:

test=> EXPLAIN (FORMAT JSON) SELECT * FROM pg_class;
QUERY PLAN
------------------------------------
[ +
{ +
"Plan": { +
"Node Type": "Seq Scan", +
"Relation Name": "pg_class",+
"Alias": "pg_class", +
"Startup Cost": 0.00, +
"Total Cost": 9.53, +
"Plan Rows": 253, +
"Plan Width": 190 +
} +
} +
]
(1 row)

test=> EXPLAIN (FORMAT YAML) SELECT * FROM pg_class;
QUERY PLAN
-------------------------------
- Plan: +
Node Type: "Seq Scan" +
Relation Name: "pg_class"+
Alias: "pg_class" +
Startup Cost: 0.00 +
Total Cost: 9.53 +
Plan Rows: 253 +
Plan Width: 190
(1 row)

Is unquoted identifiers the only value for YAML?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ None of us is going to be here forever. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> So, is there still value to a YAML format vs. JSON? They look similar
> to me in this simple case:

Well, removing the various braces and brackets reduces the line count
significantly. Not convinced it's really worth much though.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > So, is there still value to a YAML format vs. JSON? They look similar
> > to me in this simple case:
>
> Well, removing the various braces and brackets reduces the line count
> significantly. Not convinced it's really worth much though.

Ah, I see that now. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ None of us is going to be here forever. +