psql - add special variable to reflect the last query status

2017-04-04 22:05 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

After some discussions about what could be useful since psql scripts now
accepts tests, this patch sets a few variables which can be used by psql
after a "front door" (i.e. actually typed by the user) query:

- RESULT_STATUS: the status of the query
- ERROR: whether the query failed
- ERROR_MESSAGE: ...
- ROW_COUNT: #rows affected

SELECT * FROM ;
\if :ERROR
\echo oops
\q
\endif

I'm not sure that the names are right. Maybe STATUS would be better than
RESULT_STATUS.

good ideas

Regards

Pavel

Hi

2017-04-04 23:01 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-04-04 22:05 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

After some discussions about what could be useful since psql scripts now
accepts tests, this patch sets a few variables which can be used by psql
after a "front door" (i.e. actually typed by the user) query:

- RESULT_STATUS: the status of the query
- ERROR: whether the query failed
- ERROR_MESSAGE: ...
- ROW_COUNT: #rows affected

SELECT * FROM ;
\if :ERROR
\echo oops
\q
\endif

I'm not sure that the names are right. Maybe STATUS would be better than
RESULT_STATUS.

I am sending review of this patch:

1. I agree so STATUS is better name, than RESULT status. Currently it
returns values with prefix PGRES (like PGRES_FATAL_ERROR, PGRES_TUPLES_OK).
Maybe we should to cut this prefix. FATAL_ERROR, TUPLES_OK looks better for
custom level. The PGRES prefix has not sense in psql.

2. I propose availability to read ERROR_CODE - sometimes it can be more
practical than parsing error possible translated message

3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense.
This behave is maybe too strict for psql and the processing needs more
nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for
DDL) and "" for ERROR_MESSAGE when there are not any error? It will be
consistent with already implemented LASTOID variable (and other state psql
variables). Using default values are not strict clean, but it can reduce
complexity of psql scripts.

4. all regress tests passed
5. there are not any problem with doc building

Regards

Pavel

Hello Pavel,

After some discussions about what could be useful since psql scripts now
accepts tests, this patch sets a few variables which can be used by psql
after a "front door" (i.e. actually typed by the user) query:

- RESULT_STATUS: the status of the query
- ERROR: whether the query failed
- ERROR_MESSAGE: ...
- ROW_COUNT: #rows affected

SELECT * FROM ;
\if :ERROR
\echo oops
\q
\endif

I'm not sure that the names are right. Maybe STATUS would be better than
RESULT_STATUS.

I am sending review of this patch:

1. I agree so STATUS is better name, than RESULT status.

Ok, looks simpler.

Currently it returns values with prefix PGRES (like PGRES_FATAL_ERROR,
PGRES_TUPLES_OK). Maybe we should to cut this prefix. FATAL_ERROR,
TUPLES_OK looks better for custom level. The PGRES prefix has not sense
in psql.

Indeed. I skipped "PGRES_".

2. I propose availability to read ERROR_CODE - sometimes it can be more
practical than parsing error possible translated message

Ok.

3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense.
This behave is maybe too strict for psql and the processing needs more
nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for
DDL) and "" for ERROR_MESSAGE when there are not any error? It will be
consistent with already implemented LASTOID variable (and other state psql
variables). Using default values are not strict clean, but it can reduce
complexity of psql scripts.

My intention was that it could be tested with the "is defined" syntax,
which is yet to be agreed upon and implemented, so maybe generating empty
string is a better option.

For ROW_COUNT, I think that it should be consistent with what PL/pgSQL
does, so it think that 0 should be the default.

4. all regress tests passed
5. there are not any problem with doc building

Please find attached a v2 which hopefully takes into account all your
points above.

Open question: should it gather more PQerrorResultField, or the two
selected one are enough? If more, which should be included?

--
Fabien.

2017-05-22 9:40 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Hello Pavel,

After some discussions about what could be useful since psql scripts now

accepts tests, this patch sets a few variables which can be used by psql
after a "front door" (i.e. actually typed by the user) query:

- RESULT_STATUS: the status of the query
- ERROR: whether the query failed
- ERROR_MESSAGE: ...
- ROW_COUNT: #rows affected

SELECT * FROM ;
\if :ERROR
\echo oops
\q
\endif

I'm not sure that the names are right. Maybe STATUS would be better than
RESULT_STATUS.

I am sending review of this patch:

1. I agree so STATUS is better name, than RESULT status.

Ok, looks simpler.

Currently it returns values with prefix PGRES (like PGRES_FATAL_ERROR,

PGRES_TUPLES_OK). Maybe we should to cut this prefix. FATAL_ERROR,
TUPLES_OK looks better for custom level. The PGRES prefix has not sense in
psql.

Indeed. I skipped "PGRES_".

2. I propose availability to read ERROR_CODE - sometimes it can be more

practical than parsing error possible translated message

Ok.

3. The fields ERROR_MESSAGE and ROW_COUNT are set only when it has sense.

This behave is maybe too strict for psql and the processing needs more
nesting \if command. What do you think about -1 or 0 for ROW_COUNT (for
DDL) and "" for ERROR_MESSAGE when there are not any error? It will be
consistent with already implemented LASTOID variable (and other state psql
variables). Using default values are not strict clean, but it can reduce
complexity of psql scripts.

My intention was that it could be tested with the "is defined" syntax,
which is yet to be agreed upon and implemented, so maybe generating empty
string is a better option.

For ROW_COUNT, I think that it should be consistent with what PL/pgSQL
does, so it think that 0 should be the default.

4. all regress tests passed

5. there are not any problem with doc building

Please find attached a v2 which hopefully takes into account all your
points above.

Open question: should it gather more PQerrorResultField, or the two
selected one are enough? If more, which should be included?

I don't think so it is necessary. No in this moment. ERROR_CODE and
ERROR_MESSAGE are fundamental - and if we add other, then we should to add
all. Has not sense to add only some.

Regards

Pavel

Please find attached a v2 which hopefully takes into account all your
points above.

Open question: should it gather more PQerrorResultField, or the two
selected one are enough? If more, which should be included?

I don't think so it is necessary. No in this moment. ERROR_CODE and
ERROR_MESSAGE are fundamental - and if we add other, then we should to add
all. Has not sense to add only some.

Ok. I'm fine with stopping at CODE & MESSAGE.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2017-05-22 21:33 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Please find attached a v2 which hopefully takes into account all your

points above.

Open question: should it gather more PQerrorResultField, or the two
selected one are enough? If more, which should be included?

I don't think so it is necessary. No in this moment. ERROR_CODE and
ERROR_MESSAGE are fundamental - and if we add other, then we should to add
all. Has not sense to add only some.

Ok. I'm fine with stopping at CODE & MESSAGE.

I have not any other comments. The implementation is trivial. I rerun all
tests and tests passed.

I'll mark this patch as ready for commiters.

Regards

Pavel

Hello Pavel,

I have not any other comments. The implementation is trivial. [...]

Indeed.

I'll mark this patch as ready for commiters.

Thanks for the review.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I have not any other comments. The implementation is trivial. I rerun all
tests and tests passed.

I'll mark this patch as ready for commiters.

Oops, I just noticed a stupid confusion on my part which got through, I
was setting "ERROR" as "success", inverting the expected boolean value.

Here is a fixed version.

--
Fabien.

2017-06-17 7:58 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

I have not any other comments. The implementation is trivial. I rerun all

tests and tests passed.

I'll mark this patch as ready for commiters.

Oops, I just noticed a stupid confusion on my part which got through, I
was setting "ERROR" as "success", inverting the expected boolean value.

Here is a fixed version.

I missed it too.

We can introduce macro SetVariableBool(vars, varname, bool) instead

SetVariable(pset.vars, "ERROR", "FALSE");

Regards

Pavel

Hi

2017-06-19 5:55 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2017-06-17 7:58 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

I have not any other comments. The implementation is trivial. I rerun all

tests and tests passed.

I'll mark this patch as ready for commiters.

Oops, I just noticed a stupid confusion on my part which got through, I
was setting "ERROR" as "success", inverting the expected boolean value.

Here is a fixed version.

I missed it too.

We can introduce macro SetVariableBool(vars, varname, bool) instead

SetVariable(pset.vars, "ERROR", "FALSE");

I checked source code, and it requires little bit more harder refactoring
because now we have SetVariableBool - what is unhappy name, because it
initialize variable to ON value. It is question what is better name?

I found more interesting issue - the code of SetResultVariables is
partially redundant with AcceptResult - maybe the switch there can be
shared.

Regards

Pavel

Hello Pavel,

We can introduce macro SetVariableBool(vars, varname, bool) instead

SetVariable(pset.vars, "ERROR", "FALSE");

I checked source code, and it requires little bit more harder refactoring
because now we have SetVariableBool - what is unhappy name, because it
initialize variable to ON value. It is question what is better name?

The boolean values (on/off 1/0 true/false...) accepted for pg settings is
probably convenient but also somehow fuzzy.

From a programming point of view, I like booleans to have either true or
false values, and nothing else.

I agree that the existing "SetVariableBool" function is a misnommer, it
should be "SetVariableOn" given what it does, and it is not what we need.

Here is a v4 which attempts to extend & reuse the function. People might
be surprised that TRUE is used where ON was used before, so I'm not sure.

I found more interesting issue - the code of SetResultVariables is
partially redundant with AcceptResult - maybe the switch there can be
shared.

I agree that there is some common structure, but ISTM that the
AcceptResult function is called in a variety of situation where variables
are not to be set (eg "internal" queries, not user provided queries), so I
thought it best to keep the two apart.

--
Fabien.

2017-06-27 17:30 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Hello Pavel,

We can introduce macro SetVariableBool(vars, varname, bool) instead

SetVariable(pset.vars, "ERROR", "FALSE");

I checked source code, and it requires little bit more harder refactoring
because now we have SetVariableBool - what is unhappy name, because it
initialize variable to ON value. It is question what is better name?

The boolean values (on/off 1/0 true/false...) accepted for pg settings is
probably convenient but also somehow fuzzy.

From a programming point of view, I like booleans to have either true or
false values, and nothing else.

I agree that the existing "SetVariableBool" function is a misnommer, it
should be "SetVariableOn" given what it does, and it is not what we need.

switching default setting from ON to TRUE requires wider discussion - in
this moment I like to have special function "SetVariableON".

Here is a v4 which attempts to extend & reuse the function. People might
be surprised that TRUE is used where ON was used before, so I'm not sure.

I found more interesting issue - the code of SetResultVariables is

partially redundant with AcceptResult - maybe the switch there can be
shared.

I agree that there is some common structure, but ISTM that the
AcceptResult function is called in a variety of situation where variables
are not to be set (eg "internal" queries, not user provided queries), so I
thought it best to keep the two apart.

I understand, but It is not nice, really - maybe only switch can be moved
to some inlining function like IsSuccess() - more .. with this function,
the SetResultVariables function will be more cleaner

Regards

Pavel

Hello Pavel,

I agree that the existing "SetVariableBool" function is a misnommer, it
should be "SetVariableOn" given what it does, and it is not what we
need.

switching default setting from ON to TRUE requires wider discussion -

Yep.

in this moment I like to have special function "SetVariableON".

I'm fine with this, but this make it a change totally unrelated to this
patch as it would not use the function... Moreover, this function would
not use an hypothetical "set var bool" function because of the debatable
on/off vs true/false change.

Also, a "set var bool" function would be called only twice, which is not
very beneficial for a oneliner, so I left it out.

I agree that there is some common structure, but ISTM that the
AcceptResult function is called in a variety of situation where variables
are not to be set (eg "internal" queries, not user provided queries), so I
thought it best to keep the two apart.

I understand, but It is not nice, really - maybe only switch can be moved
to some inlining function like IsSuccess() - more .. with this function,
the SetResultVariables function will be more cleaner

Indeed. Attached v5 does that.

--
Fabien.

2017-06-28 9:25 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Hello Pavel,

I agree that the existing "SetVariableBool" function is a misnommer, it

should be "SetVariableOn" given what it does, and it is not what we need.

switching default setting from ON to TRUE requires wider discussion -

Yep.

in this moment I like to have special function "SetVariableON".

I'm fine with this, but this make it a change totally unrelated to this
patch as it would not use the function... Moreover, this function would not
use an hypothetical "set var bool" function because of the debatable on/off
vs true/false change.

Also, a "set var bool" function would be called only twice, which is not
very beneficial for a oneliner, so I left it out.

I agree that there is some common structure, but ISTM that the

AcceptResult function is called in a variety of situation where variables
are not to be set (eg "internal" queries, not user provided queries), so
I
thought it best to keep the two apart.

I understand, but It is not nice, really - maybe only switch can be moved
to some inlining function like IsSuccess() - more .. with this function,
the SetResultVariables function will be more cleaner

Indeed. Attached v5 does that.

juju - something like this

+ if (success)
+ {
+ char   *ntuples = PQcmdTuples(results);
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ SetVariable(pset.vars, "ERROR", "FALSE");
+ }
+ else
+ {
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ SetVariable(pset.vars, "ERROR", "TRUE");
+ }
+}

It can be simplified

SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0);
SetVariable(pset.vars, "ERROR", success ? "FALSE" : "TRUE");

Regards

Pavel

Hello Pavel,

+ if (success)
+ {
+ char   *ntuples = PQcmdTuples(results);
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ SetVariable(pset.vars, "ERROR", "FALSE");
+ }
+ else
+ {
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ SetVariable(pset.vars, "ERROR", "TRUE");
+ }
+}

It can be simplified

SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0);

According to the documentation, PQcmdTuples returns "" in some cases and
ISTM we want "0" instead for consistency, so that it is always a number. I
rejected calling PQcmdTuples twice:

..., success && *PQcmdTuples(results) ? PQcmdTuples(results) : "0")

Thus it makes the "if (success)" necessary for ROW_COUNT, and then it
looked simpler to handle ERROR the same way.

Now if the semantics is changed to put as row count whatever comes out of
the function, even if not a count, then the code could indeed be
simplified as you suggest.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2017-06-28 10:04 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

Hello Pavel,

+ if (success)

+ {
+ char   *ntuples = PQcmdTuples(results);
+ SetVariable(pset.vars, "ROW_COUNT", *ntuples ? ntuples : "0");
+ SetVariable(pset.vars, "ERROR", "FALSE");
+ }
+ else
+ {
+ SetVariable(pset.vars, "ROW_COUNT", "0");
+ SetVariable(pset.vars, "ERROR", "TRUE");
+ }
+}

It can be simplified

SetVariable(pset.vars, "ROW_COUNT", success ? PQcmdTuples(results) : 0);

According to the documentation, PQcmdTuples returns "" in some cases and
ISTM we want "0" instead for consistency, so that it is always a number. I
rejected calling PQcmdTuples twice:

..., success && *PQcmdTuples(results) ? PQcmdTuples(results) : "0")

Thus it makes the "if (success)" necessary for ROW_COUNT, and then it
looked simpler to handle ERROR the same way.

Now if the semantics is changed to put as row count whatever comes out of
the function, even if not a count, then the code could indeed be simplified
as you suggest.

Understand

Pavel

A few thoughts about this patch:

* 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'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.

* It might be better if SQLSTATE and ERROR_MESSAGE were left
unchanged by a non-error query. 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.

* 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 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.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO <coelho@cri.ensmp.fr> writes:

* 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.

Uh ... what?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers