Hello,
>> You changed the query strings to use "\n" instead of " ". I would not have
>> changed that, because it departs from the style around, and I do not think
>> it improves readability at the C code level.
>
> This was the style that was introduced by
> daa9fe8a5264a3f192efa5ddee8fb011ad9da365.
Yep. This does not imply that it is better, or worst. Currently it is not
consistent within the file, and there are only few instances, so maybe it
could be discussed anyway.
After giving it some thought, I'd say that at least I'd like the query to
be easy to read when dumped. This is not incompatible with some embedded
eol, on the contrary, but ISTM that it could keep some indentation as
well, which would be kind-of a middle ground. For readability, I'd also
consider turning keywords to upper case. Maybe it could look like:
"SELECT\n"
" somefield,\n"
" someotherfiled,\n"
...
"FROM some_table\n"
"JOIN ... ON ...\n" ...
All this is highly debatable, so ignore if you feel like it.
>> Would it make sense to accumulate in the other direction, older to newer,
>> so that new attributes are added at the end of the select?
>
> I think that could make sense, but the current style was introduced by
> daa9fe8a5264a3f192efa5ddee8fb011ad9da365. Should we revise that?
It seems to me more logical to do it while you're at it, but you are the
one writing the patches:-)
>> Should array_to_string be pg_catalog.array_to_string? All other calls seem
>> to have an explicit schema.
>
> It's not handled fully consistently in pg_dump. But my understanding is that
> this is no longer necessary because pg_dump explicitly sets the search path
> before running.
Dunno. It does not look consistent with a mix because the wary reviewer
think that there may be a potential bug:-) ISTM that explicit is better
than implicit in this context: not relying on search path would allow to
test the query independently, and anyway what you see is what you get.
Otherwise: v2 patch applies cleanly, compiles, global make check ok,
pg_dump tap ok.
"progargnames" is added in both branches of an if, which looks awkward.
I'd suggest maybe to add it once unconditionnaly.
I cannot test easily on older versions.
--
Fabien.