Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Fixing the problem envolves deciding what is the right behavior, and
> update the documentation and the implementation accordingly. Currently the
> documentation suggests that :ERROR is about SQL error (subject to
> interpretation), and the implementation is more or less consistent with
> that view, but not always, as pointed out by Daniel.
FWIW, I think we should adopt the rule that :ERROR reflects any error
condition, whether server-side or client-side. It's unlikely that
scripts would really not care about client-side errors. Moreover,
I do not think we can reliably tell the difference; there are always
going to be corner cases that are hard to classify. Example: if we
lose the server connection, is that a server-side error or client-side?
Or maybe neither, maybe the network went down.
Because of this concern, I find the idea of inventing separate
SQL_ERROR and CLIENT_ERROR variables to be a pretty terrible one.
I think we'd be creating a lot of make-work and hard choices for
ourselves, to do something that most script writers won't give a
fig about. If you've got subtle error-handling logic in mind,
you shouldn't be trying to implement your app in a psql script
in the first place, IMO anyway.
It's unclear to me whether to push ahead with Daniel's existing
patch or not. It doesn't look to me like it's making things
any worse from the error-consistency standpoint than they were
already, so I'd be inclined to consider error semantics cleanup
as something to be done separately/later.
BTW, it looks to me like the last patch is still not right as far
as when to close copystream --- won't it incorrectly close a
stream obtained from pset.copyStream? While you could fix that
with
- if (pset.gfname && copystream != NULL)
+ if (!pset.copyStream && pset.gfname && copystream != NULL)
I think that's getting overly complex and unmaintainable. I'd
be inclined to code things more like
FILE *copystream = NULL;
bool need_close = false;
...
need_close = openQueryOutputFile(...);
...
if (need_close)
// close copystream here
regards, tom lane
