Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands:\quit_if, \quit_unless) - Mailing list pgsql-hackers

Hello Corey,

> And here it is

About the patch v3:

## DOCUMENTATION

I'm wondering what pg would do on "EXISTS(SELECT 1 FROM customer)" if 
there are many employees. EXPLAIN suggests a seq_scan, which seems bad. 
With "(SELECT COUNT(*) FROM pgbench_accounts) <> 0" pg seems to generate 
an index only scan on a large table, so maybe it is a better way to do it. 
It seems strange that there is no better way to do that in a relational 
tool, the relational model being an extension of set theory... and there 
is no easy way (?) to check whether a set is empty.

"""If an EOF is reached on the main file or an 
<command>\include</command>-ed file before all 
<command>\if</command>-<command>\endif</command> are matched, then psql 
will raise an error."""

In sentence above: "before all" -> "before all local"? "then" -> ""?

"other options booleans" -> "other booleans of options"? or "options' 
booleans" maybe, but that is for people?

"unabigous" -> "unambiguous"

I think that the three paragraph explanation about non evaluation could be 
factor into one, maybe something like: """Lines within false branches are 
not evaluated in any way: queries are not sent to the server, non 
conditional commands are not evaluated but bluntly ignored, nested if 
expressions in such branches are also not evaluated but are tallied to 
check for nesting."""

I would suggest to merge elif/else constraints by describing what is 
expected rather than what is not expected. """An \if command may contain 
any number of \elif clauses and may end with one \else clause""".


## CODE

In "read_boolean_expression":
 + if (value)

"if (value != NULL)" is usually used, I think.
 + if (value == NULL) +   return false; /* not set -> assume "off" */

This is dead code, because value has been checked to be non NULL a few 
lines above.
 + (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0) + (pg_strncasecmp(value, "off", (len > 2 ? len : 2)) == 0)

Hmmm, not easy to parse. Maybe it deserves a comment?
"check at least two chars to distinguish on & off"

",action" -> ", action" (space after commas).

The "result" is not set on errors, but maybe it should be set to false 
anyway and explicitely, instead of relying on some prior initialization?
Or document that the result is not set on errors.

if command:
  if (is active) {    success = ...    if (success) {      ...    }  }  if (success) {    ...  }

The second test on success may not rely on the value set above. That looks 
very strange. ISTM that the state should be pushed whether parsing 
succeeded or not. Moreover, it results in:
  \if ERROR     \echo X  \else     \echo Y  \endif

having both X & Y printed and error reported on else and endif. I think 
that an expression error should just put the stuff in ignore state.


On "else" when in state ignored, ISTM that it should remain in state 
ignore, not switch to else-false.


Comment about "IFSTATE_FALSE" talks about the state being true, maybe a 
copy-paste error?

In comments: "... variables the branch" -> "variables if the branch"

The "if_endifs_balanced" variable is not very useful. Maybe just the test 
and error reporting in the right place:
 if (... && !psqlscan_branch_empty(scan_state))   psql_error("found EOF before closing \\endif(s)\n");


 +  #endif   /* MAINLOOP_H */

 - /*  * Main processing loop for reading lines of input  *     and sending them to the backend.

Do not add/remove empty lines in places unrelated to the patch?


History saving: I'm wondering whether all read line should be put into 
history, whether executed or not.

Is it possible to make some of the added functions static? If so, do it.

I checked that it does stop on errors with -v ON_ERROR_STOP=1. However I 
would be more at ease if this was tested somewhere.

-- 
Fabien.