Re: psql - add special variable to reflect the last query status

Hello Tom,

> * I think the ERROR_CODE variable should instead be named SQLSTATE.
> That is what the SQL standard calls this string, and it's also what
> just about all our documentation calls it; see PG_DIAG_SQLSTATE
> in libpq, or the SQLSTATE 'xxxxx' construct in pl/pgsql, or the
> sqlstate attribute of an exception object in plpython, etc etc.

I choose ERROR_CODE because it matched the ERROR boolean. But is may be a
misnomer if the status is that all is well. I'm okay with SQLSTATE.

> * I'm not exactly convinced that there's a use-case for STATUS that's
> not covered as well or better by ERROR. Client code that looks at
> PQresStatus for anything beyond error/not-error is usually doing that
> because it's library code that doesn't know what kind of query it's
> working on. It seems like a stretch that a psql script would not know
> that. Also, PQresultStatus memorializes some legacy distinctions, like
> "fatal" vs "nonfatal" error, that I think we'd be better off not
> exposing in psql scripting.

Ok.

> * It might be better if SQLSTATE and ERROR_MESSAGE were left
> unchanged by a non-error query.

Hmmm. I'm not sure. If so, ERROR_MESSAGE should be LAST_ERROR_MESSAGE
maybe? Then what about LAST_ERROR_SQLSTATE to go with it, and let SQLSTATE
& ERROR_MESSAGE reflect the last command, and ERROR is just the boolean to
test if it occured?

> That would reduce the need to copy them into other variables just
> because you needed to do something else before printing them. It'd save
> a few cycles too.

Well, my suggestion would mean that they would be copied when an error
occurs, but only when it occurs, which would not be often.

If you would like them, I'm not sure how these variable should be
initialized. Undefined? Empty?

> * Speaking of which, has anyone tried to quantify the performance
> impact of this patch? It might well be negligible, but I do not
> think we should assume that without evidence.

I think it should be negligible compared to network connections, aborting
an ongoing transaction, reading the script...

But I do not know libpq internals so I may be quite naive.

> * I wonder why you didn't merge this processing into ProcessResult,
> instead of inventing an extra function (whose call site seems rather
> poorly chosen anyhow --- what's the justification for not counting this
> overhead as part of the query runtime?). You could probably eliminate
> the refactoring you did, since it wouldn't be necessary to recompute
> AcceptResult's result that way.

Hmmm. I assume that you are unhappy about ResultIsSuccess.

The refactoring is because the function is used twice. I choose to do that
because the functionality is clear and could be made as a function which
improved readability. Ok, PQresultStatus is thus called twice, I assumed
that it is just reading a field in a struct... it could be returned to the
caller with an additional pointer to avoid that.

The SendResult & ProcessResult functions are already quite heavy to my
taste, I did not want to add significantly to that.

The ProcessResult switch does not test all states cleanly, it is really
about checking about copy, and not so clear, so I do not think that it
would mix well to add the variable stuff in the middle of that.

SendQuery is also pretty complex, including gotos everywhere.

So I did want to add to these two functions beyond the minimum. Now, I can
also inline everything coldly in ProcessResult, no problem.

--
Fabien.