On Wed, Sep 16, 2009 at 6:43 PM, Emmanuel Cecchet <manu@asterdata.com> wrote:
> Here is a new version of the patch with an updated doc and psql.
Thanks, that's great.
I don't think the way the doc changes are formatted is consistent with
what we've done elsewhere. I think that breaking the options out as a
separate block could be OK (because otherwise they have to be
duplicated between COPY TO and COPY FROM) but it should be done more
like the way that the SELECT page is done. Also, you haven't
documented the syntax 100% correctly: the boolean options work just
like the boolean explain options - they take an optional argument
which if omitted defaults to true, but you can also specify 0, 1,
true, false, on, off. See defGetBoolean. So those should be
specified as:
BINARY [boolean]
OIDS [boolean]
CSV [boolean]
CSV_HEADER [boolean]
See how we did it in sql-explain.html.
> I changed the name of the CSV options to prefix them with csv_ to avoid
> confusion with any future options. I also had to change the grammar to allow
> '*' as a parameter (needed for cvs_force_quote).
You seem to have introduced a LARGE number of unnecessary whitespace
changes here which are not going to fly. You need to go through and
revert all of those. It's hard to tell what you've really changed
here, but also every whitespace change that gets committed is a
potential merge conflict for someone else; plus pgindent will
eventually change it back, thus creating another potential merge
conflict for someone else.
I am not 100% sold on renaming all of the CSV-specific options to add
"csv_". I would like to get an opinion from someone else on whether
that is a good idea or not. I am fairly certain it is NOT a good idea
to support BOTH the old and new option names, as you've done here. If
you're going to rename them, you should update gram.y and change the
makeDefElem() calls within the copy_opt_list productions to emit the
new names.
> When we decide to drop the old syntax (in 8.6?), we will be able to clean a
> lot especially in psql.
Considering that we are still carrying syntax that was deprecated in
7.3, I don't think it's likely that we'll phase out the present syntax
anywhere nearly that quickly. But it's reasonable to ask whether we
should think about removing support for the pre-7.3 syntax altogether
for 8.5. It doesn't seem to cost us much to keep that support around,
but then again it's been deprecated for seven major releases, so it
might be about time.
...Robert
