Re: generic copy options

Robert Haas wrote:
> 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.
I looked at the way it is done in SELECT and there is a section per
clause (from clause, where clause, ...). So I am not sure how you want
to apply that here besides the copy parameters and the option clause.
> 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.
>
Ok, fixed.
>> 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.
>
Sorry, I overlooked a format in Eclipse that formatted the whole file
instead of the block I was working on. This should be fixed now.
> 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.
>
Agreed for the makeDefElem().
For changing the names, I think that names like 'header', 'escape' and
'quote' are too generic to not conflict with something that is not csv.
If you think of another format that could be added to copy, it is likely
to re-use the same variable names. The only thing that seems odd is that
if you use a CSV_* option, you still have to add CSV [on] to the option
list which seems kind of redundant.

>> 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.
>
While I understand the need for the server to still support the syntax,
is it necessary for newer version of psql to support the old syntax?

I am attaching the new version of the patch with the current
modifications addressing your comments.

Emmanuel

--
Emmanuel Cecchet
Aster Data Systems
Web: http://www.asterdata.com