Re: patch: garbage error strings in libpq

Tom Lane wrote:
>
> (1) The fact that gettext works at all seems to me to be sufficient
> empirical evidence that it's a working solution.

Tom, you have GOT to be joking. I agree with some of the things you say
(see below), but here you're just not making sense. Consider:

1. We're only talking about a very specific property of gettext(), namely
restoration of errno in calls that are not ordered w.r.t. a use of errno.
This is only relevant in a very limited number of all calls to gettext(),
so the mere fact that gettext() works doesn't prove anything relevant.

2. In those limited few cases, and for this specific property, we already
know that something in this general area is *not* working, so you'd have
to find, fix and test that before you could even make this argument.

3. Given that a call to gettext() crosses not just object-file but actual
library boundaries, what do you think the chances are--regardless of
language garantees and compiler aggressiveness--that we'd see the compiler
interleaving instructions from gettext() with a use of errno on the other
side of that boundary? Does the assumption that gettext() works at all,
therefore, make a reliable indication that there is no problem?

4. In the case of libpq_gettext(), we're not crossing library boundaries.
We're not even crossing object-file boundaries in some cases. That makes
the chances of instructions being scheduled across a call much
greater--and the question about sequence points much more relevant.

5. From what I understand, the latest versions of gcc are actually
beginning to schedule instructions across function call boundaries. That
will undoubtedly increase in the future. There is even a new feature that
can, as far as I can see, lead to instruction scheduling across source
file boundaries.

> (2) Whether there are
> sequence points in the function call or not, there most certainly are
> sequence points inside each called function. The spec allows the
> functions involved to be called in an unspecified order, but it doesn't
> allow the compiler to interleave the execution of several functions.

That would answer the big question here, but where does it say that? I
saw Neil's point that the sequence points before function calls apply for
the nested calls as well as the outer one, but there is no ordering
between those "nested-call" sequence points. It's all easy when you have
a total ordering, but we're in a partial ordering here.

So the missing piece of the puzzle is still that same question. Obviously
the compiler isn't allowed to interleave function executions (or at least
not let you find out about it) if there is a sequence point _between_
them. There is in sequential calls, for example, because there is a
sequence point before the function is entered. But what happens if there
isn't a separating sequence point, like here?

> (3) Interpretation or not, the approach that Jeroen proposes is
> unmaintainable; people will not remember to use such a kluge everywhere
> they'd need to. I'd much rather fix it in one place and do whatever we
> have to do to keep the compiler from breaking that one place.

That means you haven't seen the approach I suggested the day before
yesterday, where I also explained that I felt it best to fix the incumbent
bugs first before refactoring the result. You're still talking about my
initial quick-fix patch, which IMHO could have gone in already while we
were arguing over a long-term solution. That patch was ugly solely in the
sense that the original broken code was ugly; if you want to use words
like "kluge" then please direct them there. The copy-paste style of
writing loops didn't help either.

As I suggested on Wednesday, maybe we can wrap the combination of
printfPQExpBuffer() and libpq_gettext() into a single function that takes
a PGconn *, a format string, and varargs. This makes the calls a lot
shorter and simpler, it moots the question of sequence points because
errno would be the first thing to be evaluated, and en passant we'll fix a
few cases where we forgot to call libpq_gettext(). We'd have to have a
similar wrapper for snprintf(), but then I think all cases in libpq are
covered.

Jeroen