Re: [HACKERS] psql - add special variable to reflect the last querystatus - Mailing list pgsql-hackers

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.