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