Pavel Stehule <pavel.stehule@gmail.com> writes:
> [ gset_13.diff ]
I looked at this a bit. I think you need to reconsider when and how the
\gset state gets cleaned up. Doing it inside StoreQueryResult is not
very good because that only gets reached upon successful execution.
Consider for example
select 1/0 \gset x
You'll get an ERROR from this, and a reasonable user would suppose that
that was that and the \gset is no longer in effect. But guess what,
it's still lurking under the surface, waiting to capture his next command.
This is also causing you unnecessary complication in
ExecQueryUsingCursor, which has to work around the fact that
StoreQueryResult destroys state.
I think it would be better to remove that responsibility from
StoreQueryResult and instead put the gset-list cleanup somewhere at the
end of query processing. Didn't really look into where would be the
best place, but it should be someplace that control passes through no
matter what came back from the server.
BTW, is StoreQueryResult in itself (that is, the switch on
PQresultStatus) actually doing anything useful? It appears to me that
the error cases are handled elsewhere, such that control never gets to
it unless the PQresultStatus is an expected value. If that were not the
case, printouts as laconic as "bad response\n" would certainly not be
acceptable --- people would want to see the underlying error message.
Also, I'm not sure I like the PSQL_CMD_NOSEND business, ie, trashing
the query buffer if anything can be found wrong with the \gset itself.
If I've done
big long multiline query here\gset x y
I'd expect that the error only discards the \gset and not the query.
An error in some other sort of backslash command in that situation
wouldn't discard the query buffer. For instance try this:
regression=# select 3+2
regression-# \goofus
Invalid command \goofus. Try \? for help.
regression-# ;?column?
---------- 5
(1 row)
regression=#
So AFAICS, PSQL_CMD_NOSEND just represents extra code that's making
things worse not better.
One more gripe is that the parsing logic for \gset is pretty
unintelligible. You've got a "state" variable with only two values,
whose names seem to boil down to whether a comma is expected or not.
And then you've got a separate "process_comma" flag, which means
... well, I'm not sure, but possibly the very same thing. For sure it's
not clear whether all four possible combinations of those two variables
are valid and what they'd mean. This could use another round of
thinking and rewriting. Or at least better comments.
regards, tom lane
