Re: PQputCopyEnd doesn't adhere to its API contract

On Thu, Sep 4, 2014 at 6:38 PM, David Johnston
<david.g.johnston@gmail.com> wrote:
> The implied suggestion is that if I do find any other areas that look like
> they need fixing - even in the same file - I should separate them out into a
> separate patch.

Yes.

> Though I have seen various "while I was in there I also
> fixed such-and-such" commits previously so the line is at least somewhat
> fluid.

Yep, it's a judgement call.  As a general rule though, I think there's
often a pretty clear connection between the main topic of the commit
and the changes folded in.  Another way to think about this is that,
at least for non-committers (and frequently also for committers), any
patch someone writes is going to need an upvote from *at least* one
other person in order to go in.  If somebody can look at the patch and
easily determine that it solves some problem and doesn't make anything
worse, then they're likely to like it (and if they're a committer,
maybe commit it).  But if they see changes they like mixed in with
changes they don't like, then they're likely to either bounce it back,
or just say, hmm, this looks like it will take some time to deal with,
let me put that on my TODO list.  And of course sometimes it never
makes it off the TODO list.

Now, it's a fair point that this makes it hard to get large-scale
changes done.  But, to some extent, that's a good thing.  Prudence,
indeed, will dictate that source code or documentation long
established should not be changed for light or transient causes.  For
the rest, if you feel a large scale change is really needed, it's best
to start with a proposal: "I think we need to rehash the documentation
in section X because of reason Y."  You've made a few proposals to
rehash sections of the documentation on pgsql-hackers, but I haven't
actually seen clear and compelling justification for those reworkings.
Clearly, you like it better the new way, but the person who did it the
existing way likely prefers their version, and they've been around
longer than you.  :-)  By stating your objectives up-front, you can
see whether people agree with those objectives or not.  If they do,
you can hope to find the final patch criticized only on fine details
which are easily remedied; but if they don't, then some rethinking may
be needed.

>> This seems pointless.  Of course general documentation will be less
>> specific than documentation for specific functions.
>
> The existing wording was being verbose in order to be correct.  In a summary
> like this I'd trade being reasonably accurate and general for the precision
> that is included elsewhere.

This is an example of a goal where you might solicit people's general
thoughts before starting.  Maybe people will agree that removing
details in a certain place is useful, or maybe they won't, but it
needs discussion.

> One of the trade-offs I mentioned...its more style than anything but
> removing the parenthetical (if there is not error in the command) and
> writing it more directly seemed preferable in an overview such as this.
>
> Better:  The function will either throw an error or return a PGresult
> object[...]

Nope.  This is not C++, nor is it the backend.  It will not throw anything.

>> +  <para>
>> +   Second, the application should use the functions in this
>> +   section to receive data rows or transmit buffer loads.  Buffer loads
>> are
>> +   not guaranteed to be processed until the copy transfer is completed.
>> +  </para>
>>
>> The main change here vs. the existing text is that you're now using
>> the phase "buffer loads" to refer to what gets transmitted, and "data
>> rows" to refer to what gets received.  The existing text uses the term
>> "data rows" for both, which seems correct to me.  My first reaction on
>> reading your revised text was "wait, what's a buffer load?".
>
> So, my generalization policy working in reverse - since the transmit side
> does not have to be in complete rows implying that they are here is (albeit
> acceptably) inaccurate.

The existing wording doesn't say that each call to one of the
functions in question must contain only whole data rows of itself.  It
merely says that these are the functions you need to use to send data
rows, which is true.

>> -   At this point further SQL commands can be issued via
>> -   <function>PQexec</function>.  (It is not possible to execute other SQL
>> -   commands using the same connection while the <command>COPY</command>
>> -   operation is in progress.)
>>
>> Removing this text doesn't seem like a good idea.  It's a quite
>> helpful clarification.  The <note> you've added in its place doesn't
>> seem like a good substitute for it, and more generally, I think we
>> should avoid the overuse of constructs like <note>.  Emphasis needs to
>> be used minimally or it loses its meaning.
>
> Was trying to remove repetition here - happy to consider alternative way of
> doing so if the note is objectionable.

I guess it doesn't seem repetitive to me.

> Its great to be able to read the documentation like a book but it also wants
> to be useful for scanning and a total lack of emphasis makes that more
> difficult.  I'm trying to see if there is middle ground to be had.

Maybe, but if so, that's a general issue that should be discussed
aside from this patch.  You can't create a new policy without
discussion and start trying to enforce it on the documentation one bit
at a time.  That will drive everyone crazy.

> I appreciate the time you have taken on this and will look at my thoughts
> with the new understanding you have given me.
>
> Thank You!

Thanks for getting involved.  I apologize if I was brusque here or at
other times in the past (or future).  Unfortunately I've been insanely
busy and it doesn't bring out the best in me, but I really do value
your (and everyone's efforts) to move PostgreSQL forward.

Thanks,

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