Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins

[Corey Huinker]

>
> I'm not sure whether there's a way to fix this that doesn't break other
> cases.  We could retrieve the pg_database.datcollate string from the
> remote, but that doesn't necessarily match up with any collation name
> we know about locally.  One pretty common failure mode would be that
> the datcollate string isn't a canonical spelling (eg, "en_US.UTF-8"
> where the name we know about is "en_US.utf8").  In general, datcollate
> is handled through other code paths than collation names, so it might
> easily be that it doesn't match anything in the remote's pg_collation
> catalog either :-(.
>

This is where we got stuck as well (+David who did a lot of digging on this
issue). Hence submitting the discovery without our half-baked patch.

We had difficulty finding the place in the code were LC_COLLATE gets
recombobulated into a recognized collation.

Re: [HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins

[Corey Huinker]

>
>
> The collation column is empty here, which means that collation for
> str* columns is default collation i.e. C. This isn't true, since the
> default ncollation on the foreign server is different from the default
> collation of local database. AFAIU, import foreign schema should have
> set appropriate collation of the foreign table.
>

That's what we saw. The query inside IMPORT FOREIGN SCHEMA assumes a NULL
collation means default, without asking the server what that default is. I
was thinking that we could change the select
inside postgresImportForeignSchema and replace
collname,
with something borrowed from information_schema.sql like

coalesce(collname, (SELECT encoding FROM pg_catalog.pg_database
WHERE datname = pg_catalog.current_database()))

which itself isn't right because encoding names don't match up perfectly
with collation names.

[HACKERS] Varying results when using merge joins over postgres_fdw vs hash joins

[Corey Huinker]

We are having an issue with a query that will return no results when the
query does a merge join with a foreign table, but (correctly) returns
results when using a hash join.

Here is the situation on the "remote" database (9.5):


# \d+ table_with_en_us_utf8_encoding
   Table "public.table_with_en_us_utf8_encoding"
 Column |  Type  | Modifiers | Storage  | Stats target |
Description
++---+--+--+-
 id | bigint |   | plain|  |
 str1   | character varying(255) |   | extended |  |
 str2   | character varying(255) |   | extended |  |
 str3   | character varying(255) |   | extended |  |
 str4   | character varying(3)   |   | extended |  |

analytics=# select encoding, datcollate, datctype from pg_database where
datname = current_database();
 encoding | datcollate  |  datctype
--+-+-
6 | en_US.UTF-8 | en_US.UTF-8




And here's what we do on the local side (9.6):

# select encoding, datcollate, datctype from pg_database where datname =
current_database();
 encoding | datcollate | datctype
--++--
6 | C  | C

# import foreign schema public limit to (table_with_en_us_utf8_encoding)
from server primary_replica into public;

# \d+ table_with_en_us_utf8_encoding
  Foreign table
"public.table_with_en_us_utf8_encoding"
 Column |  Type  | Collation | Nullable | Default | FDW
options  | Storage  | Stats target | Description
++---+--+-+--+--+--+-
 id | bigint |   |  | |
(column_name 'id')   | plain|  |
 str1   | character varying(255) |   |  | |
(column_name 'str1') | extended |  |
 str2   | character varying(255) |   |  | |
(column_name 'str2') | extended |  |
 str3   | character varying(255) |   |  | |
(column_name 'str3') | extended |  |
 str4   | character varying(3)   |   |  | |
(column_name 'str4') | extended |  |
Server: primary_replica
FDW options: (schema_name 'public', table_name
'table_with_en_us_utf8_encoding')

# create temporary table tmp_on_c_collated_foreign_server (str2 text);

# insert into tmp_on_c_collated_foreign_server (str2) values ('576228972');
# insert into tmp_on_c_collated_foreign_server (str2) values ('576228972');
# insert into tmp_on_c_collated_foreign_server (str2) values ('576228972');

--
-- query with merge join, returns zero rows
--
# explain (analyze, verbose) select e.str1, e.str2, e.str3 from
tmp_on_c_collated_foreign_server c left join table_with_en_us_utf8_encoding
e  on c.str2 = e.str2 where e.str4='2' ;

 QUERY PLAN

 Merge Join  (cost=18041.88..22322.92 rows=229221 width=1548) (actual
time=102.849..102.849 *rows=0* loops=1)
   Output: e.str1, e.str2, e.str3
   Merge Cond: ((e.str2)::text = c.str2)
   ->  Foreign Scan on public.table_with_en_us_utf8_encoding e
 (cost=17947.50..18705.95 rows=33709 width=93) (actual
time=102.815..102.815 rows=1 loops=1)
 Output: e.id, e.str1, e.str2, e.str3, e.str4
 Remote SQL: *SELECT str1, str2, str3 FROM
public.table_with_en_us_utf8_encoding WHERE ((str4 = '2'::text)) ORDER BY
str2 ASC NULLS LAST*
   ->  Sort  (cost=94.38..97.78 rows=1360 width=32) (actual
time=0.028..0.029 rows=7 loops=1)
 Output: c.str2
 Sort Key: c.str2
 Sort Method: quicksort  Memory: 25kB
 ->  Seq Scan on pg_temp_3.tmp_on_c_collated_foreign_server c
 (cost=0.00..23.60 rows=1360 width=32) (actual time=0.010..0.011 rows=7
loops=1)
   Output: c.str2
 Planning time: 4.285 ms
 Execution time: 104.458 ms
(14 rows)


--
-- query with hash join, returns rows

--

-- the default for the foreign server is to use remote estimates, so we
turn that off...

# alter foreign table table_with_en_us_utf8_encoding OPTIONS (ADD
use_remote_estimate 'false');
ALTER FOREIGN TABLE

-- and then run the same query again

# explain (analyze, verbose) select e.str1, e.str2, e.str3 from
tmp_on_c_collated_foreign_server c left join table_with_en_us_utf8_encoding
e  on c.str2 = e.str2 where e.str4='2' ;

QUERY PLAN
--
 Hash Join  (cost=110.68..139.45 rows=7 width=1548) (actual
time=154.280..154.286 *rows=7* loops=1)
   Output: e.str1, e.str2, e.str3
   Hash Cond: 

Re: [HACKERS] psql - add ability to test whether a variable exists

[Corey Huinker]

On Sat, Aug 26, 2017 at 2:09 PM, Pavel Stehule 
wrote:

>
>
> 2017-08-26 19:55 GMT+02:00 Fabien COELHO :
>
>>
>> Any colon prefixed syntax can be made to work because it is enough for
>>> the lexer to detect and handle... so
>>>
>>>  :{defined varname}
>>>
>>> may be an option, although I do not like the space much because it adds
>>> some fuzzyness in the lexer which has to process it. It is probably doable,
>>> though. I like having a "?" because there is a question. Other
>>> suggestions somehow in line with your proposal could be
>>>  :{?varname}
>>>  :{varname?}
>>> what do you think?
>>>
>>
>> Here is a version with the :{?varname} syntax.
>
>
> It looks much better for me.
>
> Regards
>
> Pavel
>

+1. Glad to have this feature. Any of the proposed syntaxes look good to
me, with a slight preference for {?var}.

Re: [HACKERS] CTE inlining

[Corey Huinker]

>
> I get that people with gigantic PostgreSQL installations with
> stringent performance requirements sometimes need to do odd things to
> squeeze out the last few percentage points of performance.  As the
> people (well, at least the people close to the ground) at these
> organizations are fully aware, performance optimizations are extremely
> volatile with respect to new versions of software, whether it's
> PostgreSQL, Oracle, the Linux kernel, or what have you.  They expect
> this, and they have processes in place to handle it.  If they don't,
> it's pilot error.
>

Well put. People on the ground in those situations go to great lengths to
freeze the query plan as-is. For them, an upgrade is something that is done
after months of planning. They might be surprised by the dropping of this
optimization fence, but the surprise won't be in production, and they've
got just as good of chance of being pleasantly surprised.


> > 2. Add a decorator for WITH (e.g. "WITH MATERIALIZED x (...") to add an
> > explicit optimization fence. This will for the first time add official
> > support for a query hint in the syntax which is a quite big precedent.
>
> Yep.  It's one we should think very carefully before we introduce.
>

There's a tiny, oblique precedence for this with Oracle's WITH [FUNCTION |
PROCEDURE] syntax. In both cases, the user is able to create an ephemeral
object that can be referenced later in the query. Good idea or bad, it's a
sign that others have been fine with that conceptual path.

Personally, I'm fine with WITH MATERIALIZED, but I'm also ok with just not
promising the fence. I think there is value in letting users break up a
complex query into understandable WITH-chunks, and that value shouldn't
prevent good performance. The fence will probably still be there anyway in
the case of INSERT/UPDATE RETURNING and cases where a CTE is referenced
more than once in the query that follows.

Re: [HACKERS] CTE inlining

[Corey Huinker]

On Mon, May 1, 2017 at 10:26 AM, Andreas Karlsson  wrote:

> What about WITH MATERIALIZED, borrowing from the MySQL terminology
> "materialized subquery"?


+1, you beat me to it.

Re: [HACKERS] Undefined psql variables

[Corey Huinker]

>
>
> > I suggest to reuse pgbench expression engine, developed by Haas Robert:-)
>
> Not a bad idea (though I'm sure there are other reasonable options, too).
>
>
I don't want to stand in the way of any progress in getting expressions
into \if and some subspecies of \set. But, assuming we don't get it into
v10, our documentations currently says this about expressions:

Expressions that do not properly evaluate to true or false will generate a
warning and be treated as false.


We should probably amend that to say something about the potential future
directions of expressions, perhaps this:

Expressions that do not properly evaluate to true or false will generate a
warning and be treated as false, though that behavior is subject to change
in future versions of psql.


That should guard us against people getting too attached to that behavior
in the interim.

With that said, I'm starting to like the idea of not boxing ourselves into
one type of expression. Maybe the first token could be the expression
context with the expression to follow

The expression type we have now
\if true

"defined" is a context (or "mode") that expects one token that might be a
psql varname
\if defined varname

"sql" is a context that treats the rest of the line as a SQL statement to
the current connection, and looks at the first column of first row for
"truth"
\if sql SELECT EXISTS(SELECT null FROM item WHERE manufacturer = 'Frobozz')

"pgbench" could invoke the pgbench expression engine.
\if pgbench 

Anything else is treated as an external expression evaluator. If an
expression has an unknown context "foo", check the ENV vars for EXPR_FOO,
and pipe the remaining expression tokens to $PSQL_EXPR_FOO if it exists,
and read the output of that for psql-boolean truth.  I think having a
context/mode token could allow us to have lots of pluggable expression
types with minimal effort to psql itself.

"python" invokes a python interpreter (if PSQL_EXPR_PYTHON is defined,
fails otherwise)
\if python print(:'varname' == 'Approved' or :'othervar' == 'Special')
which would echo
print('Approved' == 'Approved' or 'Regular' == 'Special')
to python, which would give the response "True", which is true

likewise with "bash" (assuming PSQL_EXPR_BASH=bash)
\if bash expr :'varname' = 'Approved'
would echo
expr 'Approved' = 'Approved'
to bash, which would return 1, which would be true.

So we'd get all that, with only having to internally code for an external
launcher, naked booleans, pgbench, defined, and I suppose we should have a
negation


\if not  


Which I guess would allow

\if not not not  


For consistency, we might want to change the default context to require an
explicit "bool", so

\if bool true

but if we want to do that, we should change it very soon.

tl;dr:
My proposal is:
* do the bare minimum of expression testing in psql (simple scalar truth,
variable definition, negation)
* do platform independent client-only-expressions in pgbench mode
* allow inline \gset-ish expressions with sql-mode
* and allow for platform/install dependent expressions via PSQL_EXPR_* env
vars.

Re: [HACKERS] delta relations in AFTER triggers

[Corey Huinker]

>
> Great.  Thanks.  I wonder if there is some way we can automatically
> include code fragments in the documentation without keeping them in
> sync manually.
>
>
In whatever extra docs you add, could you include an example of an INSERT
ON CONFLICT, and potentially a CTE query that does two operations on the
same table. I'm not clear on what to expect when a statement does a mix of
INSERT, UPDATE, and DELETE? Will there be multiple firings of the trigger
in a single statement, or will the before/after sets be mashed together
regardless of which part of the query generated it?

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

On Tue, Apr 11, 2017 at 2:56 AM, Fabien COELHO  wrote:

>
> Hello Pavel,
>
> I think so some local expression evaluation could be - but it should not be
>> placed in \if statement
>>
>
> Why?
>
> \expr issupported :VERSION_NUM >= 1
>>
>
> Hmmm. Although I do not buy this, it could work as a replacement for \set
> which it seems cannot be upgraded because some people may rely on it to
> just store whatever comes after it in a variable.
>

I have no strong opinion on how expressive expressions should be, but
having a separate \expr (or \setexpr, etc) gives us a green field to
develop them.

Re: [HACKERS] Undefined psql variables

[Corey Huinker]

On Sun, Apr 2, 2017 at 4:57 PM, Fabien COELHO  wrote:

>
> I'm inclined to suggest that we should require all extensions beyond the
>> boolean-literal case to be set up as a keyword followed by appropriate
>> argument(s); that seems like it's enough to prevent syntax conflicts from
>> future additions.  So you could imagine
>>
>> \if defined varname
>> \if sql boolean expression to send to server
>> \if compare value operator value
>>
>
> I'm still thinking:-)
>
> Independently of the my aethetical complaint against having a pretty
> unusual keyword prefix syntax, how would you envision a \set assignment
> variant? Would \if have a different expression syntax somehow?


Any further thoughts?

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

>
> I assume we want to keep that pre-existing behavior of \set in
> psql, that is, making a copy of that string and associating a
> name to it, whereas I guess pgbench computes a value from it and
> stores that value.
>

Maybe a \set-variant called \expr?

Re: [HACKERS] delta relations in AFTER triggers

[Corey Huinker]

>
> > That, and I suspect that people will start using this infrastructure
> > for some very cool projects.
>
> Yeah, I was excited to see this committed. It practically begs to be
> used for plpgsql TABLE valued variables backed by tuplestores.
>


(also very excited about this!)

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

On Sun, Apr 2, 2017 at 12:29 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> I'm anxious to help with these patches, but they seem a bit of a moving
>> target. Happy to jump in and review as soon as we've settled on what
>> should
>> be done.
>>
>
> The "v3" I sent basically adds both client & server version numbers in
> client-side variables, basically same code as suggested by Pavel for the
> server version, and some documentation.
>

patch applies via patch -p1

Works as advertised.
# \echo SERVER_VERSION_NAME
SERVER_VERSION_NAME
# \echo :SERVER_VERSION_NAME
10.0
# \echo :SERVER_VERSION_NUM
10
# \echo :VERSION_NUM
10

The new documentation is clear, and accurately reflects current name style.

Looking at #define STRINGIFY(), I got curious where else STRINGIFY was used:

$ git grep STRINGIFY
src/bin/psql/startup.c:#define STRINGIFY2(symbol) #symbol
src/bin/psql/startup.c:#define STRINGIFY(symbol) STRINGIFY2(symbol)
src/bin/psql/startup.c: SetVariable(pset.vars, "VERSION_NUM",
STRINGIFY(PG_VERSION_NUM));
src/tools/msvc/Solution.pm:s{PG_VERSION_STR "[^"]+"}{__STRINGIFY(x)
#x\n#define __STRINGIFY2(z) __STRINGIFY(z)\n#define PG_VERSION_STR
"PostgreSQL $self->{strver}$extraver, compiled by Visual C++ build "
__STRINGIFY2(_MSC_VER) ", $bits-bit"};

Without digging too deep, it seems like the redefinition wouldn't be
harmful, but it might make sense to not use the name STRINGIFY() if only to
avoid confusion with Solution.pm.



> The questions are:
>
>  - which version should be provided (10.0 10 ...)
>

A fixed length string without decimals seems best for the multitude of
tools that will want to manipulate that data.



>  - how should they be named?
>
>In v3 there is VERSION{,_NAME,_NUM} for client and
>SERVER_VERSION_{NUM,NAME} or SVERSION_NUM suggested
>by Pavel for server.
>

SERVER_VERSION_* is good.
VERSION_* is ok. Would CLIENT_VERSION_* or PSQL_VERSION_* be better?


>  - how desirable/useful is it to have this in 10?
>

Extensions and extension-ish packages will love the _NUM vars. The sooner
the better.

There's a lesser need for the _NAME vars.

Re: [HACKERS] asynchronous execution

[Corey Huinker]

>
>
> I'll continue working on this from this point aiming to the next
> commit fest.
>
>
This probably will not surprise you given the many commits in the past 2
weeks, but the patches no longer apply to master:

 $ git apply
~/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch
/home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:27:
trailing whitespace.
FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, NULL, 3);
/home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:39:
trailing whitespace.
#include "utils/resowner_private.h"
/home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:47:
trailing whitespace.
ResourceOwner   resowner;   /* Resource owner */
/home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:48:
trailing whitespace.

/home/ubuntu/async/0001-Allow-wait-event-set-to-be-registered-to-resource-ow.patch:57:
trailing whitespace.
WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, NULL,
3);
error: patch failed: src/backend/libpq/pqcomm.c:201
error: src/backend/libpq/pqcomm.c: patch does not apply
error: patch failed: src/backend/storage/ipc/latch.c:61
error: src/backend/storage/ipc/latch.c: patch does not apply
error: patch failed: src/backend/storage/lmgr/condition_variable.c:66
error: src/backend/storage/lmgr/condition_variable.c: patch does not apply
error: patch failed: src/backend/utils/resowner/resowner.c:124
error: src/backend/utils/resowner/resowner.c: patch does not apply
error: patch failed: src/include/storage/latch.h:101
error: src/include/storage/latch.h: patch does not apply
error: patch failed: src/include/utils/resowner_private.h:18
error: src/include/utils/resowner_private.h: patch does not apply

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

On Sun, Apr 2, 2017 at 11:16 AM, Pavel Stehule 
wrote:

>
>
> 2017-04-02 13:13 GMT+02:00 Fabien COELHO :
>
>>
>> Hello Pavel,
>>
>>   \echo :VERSION
   PostgreSQL 10devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit

 Probably some :VERSION_NUM would make some sense. See attached PoC
 patch.
 Would it make sense?

>>>
>>> Maybe better name for you CLIENT_VERSION_NUM
>>>
>>
>> If it was starting from nothing I would tend to agree with you, but there
>> is already an existing :VERSION variable, so it seemed logical to keep on
>> and create variants with the same prefix.
>
>
> you have true - so VERSION_NUM should be client side version
>
>
>>
>>
>> Can be SERVER_VERSION_NUM taken from connection info?
>>>
>>
>> Probably it could. It seems a little less straightforward than defining a
>> client-side string at compile time. The information is displayed when the
>> connection is established, so the information is there somewhere.
>>
>
> It is not too hard
>
> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
> index 94a3cfce90..d1ae81646f 100644
> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -3320,16 +3320,21 @@ checkWin32Codepage(void)
>  void
>  SyncVariables(void)
>  {
> +   charbuffer[100];
> +
> /* get stuff from connection */
> pset.encoding = PQclientEncoding(pset.db);
> pset.popt.topt.encoding = pset.encoding;
> pset.sversion = PQserverVersion(pset.db);
>
> +   snprintf(buffer, 100, "%d", pset.sversion);
> +
> SetVariable(pset.vars, "DBNAME", PQdb(pset.db));
> SetVariable(pset.vars, "USER", PQuser(pset.db));
> SetVariable(pset.vars, "HOST", PQhost(pset.db));
> SetVariable(pset.vars, "PORT", PQport(pset.db));
> SetVariable(pset.vars, "ENCODING", pg_encoding_to_char(pset.encod
> ing));
> +   SetVariable(pset.vars, "SVERSION_NUM", buffer);
>
> /* send stuff to it, too */
> PQsetErrorVerbosity(pset.db, pset.verbosity);
>
> Regards
>
> Pavel
>
>
>>
>>  psql (10devel, server 9.6.2)
>>
>> --
>> Fabien.
>>
>
>

I'm anxious to help with these patches, but they seem a bit of a moving
target. Happy to jump in and review as soon as we've settled on what should
be done.

Re: [HACKERS] Undefined psql variables

[Corey Huinker]

On Mon, Jan 23, 2017 at 12:53 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> > I was giving some thought to how psql handles undefined variables.
> > I would like an option where either psql can provide an alternate value
> > when an undefined variable is referenced, or a way to detect that a
> > specific variable is undefined and replace it with a defined variable.
>
> This seems pretty bizarre.  What's the use case?  Why would it not
> be better to build the behavior out of other spare parts, along the
> lines of COALESCE or perhaps
>
>   \if not defined(x)
>   \set x y
>   \fi
>
> Obviously the \if stuff is things we don't have yet either, but
> it seems less likely to have surprising side-effects.
>
> regards, tom lane
>

In light of the backticks variable expansion thread, I'm reviving this
thread in the hopes that a defined()-ish psql function can make it into v10.
It's something that cannot be solved with a query and \gset, so adding it
to psql boolean expressions is the only option I can see.

Re: [HACKERS] Variable substitution in psql backtick expansion

[Corey Huinker]

On Thu, Mar 30, 2017 at 1:33 PM, Tom Lane  wrote:

> single-quoted according to Unix shell conventions.  (So the
> processing would be a bit different from what it is for the
> same notation in SQL contexts.)
>

+1
Having been bit by format '%L' prepending an 'E' to any string that happens
to have a backslash in it, I'm in favor of this difference.

Any reason we wouldn't do :"VARIABLE" as well? People might expect it given
its use elsewhere, and it would make possible things like

SELECT '$HOME/lamentable application name dir/bin/myprog' as myprog \gset
`:"myprog" arg1 arg2`


both for expanding $HOME and keeping the lamentable dir path as one arg.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

New Patch v29: Now with less coverage!
(same as v28 minus the psql-on-error-stop.sql and associated changes)
Fabien raises some good points about if/then being a tremendous tool for
enhancing other existing regression tests.


On Wed, Mar 29, 2017 at 2:16 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:

>
> Hello Tom,
>
> If someone were to put together a TAP test suite that covered all that
>> and made for a meaningful improvement in psql's altogether-miserable
>> code coverage report[1], I would think that that would be a useful
>> expenditure of buildfarm time.
>>
>
> Ok, this is an interesting point.
>
> What I'm objecting to is paying the overhead for such a suite in order to
>> test just this one thing.
>>
>
> Well, it should start somewhere. Once something is running it is easier to
> add more tests.
>
> think that that passes the bang-for-buck test; or in other words, this
>> isn't the place I would start if I were creating a TAP suite for psql.
>>
>
> Sure, I would not have started with that either.
>
> Note that from this patch point of view, it is somehow logical to start
> testing a given feature when this very feature is being developed...
>
> The summary is that we agree that psql test coverage is abysmal, but you
> do not want to bootstrap a better test infrastructure for this particular
> and rather special new feature. Ok.
>
> Maybe Corey can submit another patch with the exit 3 test removed.
>
> --
> Fabien.
>
From 9caa338fa085bc1c0ee16f22c0c1c8d3c8fc1697 Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huin...@moat.com>
Date: Wed, 29 Mar 2017 15:30:22 -0400
Subject: [PATCH] psql if v29

---
 doc/src/sgml/ref/psql-ref.sgml |   90 ++-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1365 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 +++
 src/bin/psql/conditional.h |   62 ++
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql.out |  110 +++
 src/test/regress/sql/psql.sql  |  109 +++
 15 files changed, 1641 insertions(+), 290 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+The expression argument
+of \if or \elif
+is subject to variable interpolation and backquote expansion, just
+like any other backslash command argument.  After that it is evaluated
+like the value of an on/off option variable.  So a valid value
+is any unambiguous case-insensitive match for one of:
+true, false, 1,
+0, on, off,
+yes, no.  For example,
+t, T, and tR
+will all be considered to be true.
+
+
+Expressions that do not properly evaluate to true or false will
+generate a warning and be treated as false.
+   

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> 0001+0002 patch primarily for ease of review. will be following with a
> single v28 patch shortly.
>


0001-psql-if-v28.patch
Description: Binary data

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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 27, 2017 at 3:25 PM, Fabien COELHO  wrote:

>
> And here you go
>>
>
> Patch applies cleany, make check ok. Looks pretty good.
>
> A minor detail I have just noticed, sorry: now that options are discarded
> by functions, some string variable declarations should be moved back inside
> the active branch. You moved them out because you where sharing the
> variables between the active & inactive branches, but this is no longer
> necessary, and the project practice seems to declare variables just where
> they are needed. That would be pattern in d, encoding in encoding, fname in
> f and g and include and out and s, prefix in gset, opt in help, opt* in lo
> and pset and set, arg* in prompt, env* in setenv... and maybe a few others.
>
> --
> Fabien.
>

done:
encoding f g gset help include lo out prompt pset s set setenv sf sv t T
timing unset watch x z ! ?

weird cases where they're both still needed:
d write

0001+0002 patch primarily for ease of review. will be following with a
single v28 patch shortly.


0001-psql-if-v27.patch
Description: Binary data


0002-fix-var-scoping.patch
Description: Binary data

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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 27, 2017 at 10:34 AM, Fabien COELHO  wrote:

>
> Hello,
>
> I think that you could use another pattern where you init the
>>> PQExpBufferData structure instead of create it, so that only the string
>>> is
>>> malloced.
>>>
>>
>> In v26, I have the functions return PQExpBuffer. The two calling functions
>> then free it, which should solve any leak.
>>
>
> Yep, it works as well.
>
> Here's an addendum that does that. I can combine them in v27, but figured
>> this was quicker.
>>
>
> It works.
>
> However having just one full patch with a number would help so that I can
> say "ready to committer" or not on something.
>
> --
> Fabien.
>

And here you go (sorry for the delay, had errands to run).


0001-psql-if-v27.patch
Description: Binary data

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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> I think that you could use another pattern where you init the
> PQExpBufferData structure instead of create it, so that only the string is
> malloced.


In v26, I have the functions return PQExpBuffer. The two calling functions
then free it, which should solve any leak.


>
>
> I think that you should use appendPQExpBufferChar and Str instead of
>>> relying on the format variant which is probably expensive. Something
>>> like:
>>>
>>>   if (num_options > 0)
>>> append...Char(buf, ' ');
>>>   append...Str(buf, ...);
>>>
>>
>> All flavors of appendPQExpBuffer*() I can find have a const *char format
>> string, so no way to append a naked string. If you know differently, I'm
>> listening. Not fixed.
>>
>
> These prototypes are from "pqexpbuffer.h", and do not seem to rely on a
> format:
>
>
Here's an addendum that does that. I can combine them in v27, but figured
this was quicker.
From 2fa083eb0115278f817a2d1d0439a47951a9c48b Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huin...@moat.com>
Date: Mon, 27 Mar 2017 10:07:36 -0400
Subject: [PATCH 2/2] simpler append

---
 src/bin/psql/command.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index aa9dad4..a6168df 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -218,10 +218,9 @@ gather_boolean_expression(PsqlScanState scan_state, bool expansion, bool warn)
 	while ((value = psql_scan_slash_option(scan_state, slash_opt, NULL, false)))
 	{
 		/* add a space in between subsequent tokens */
-		if (num_options == 0)
-			appendPQExpBuffer(exp_buf, "%s", value);
-		else
-			appendPQExpBuffer(exp_buf, " %s", value);
+		if (num_options > 0)
+			appendPQExpBufferChar(exp_buf, ' ');
+		appendPQExpBufferStr(exp_buf, value);
 		num_options++;
 	}
 
-- 
2.7.4


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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Patches do not apply cleanly.
> Part 1 gets:
>  error: patch failed: src/test/regress/parallel_schedule:89
>  error: src/test/regress/parallel_schedule: patch does not apply
>
> There is still the useless file, ok it is removed by part2. Could have
> been just one patch...
>

parallel_schedule failed because I hadn't rebased recently enough.

git format-patch did us no favors there. New patch is redone as one commit.

ISTM that PQExpBuffer is partially a memory leak. Something should need to
> be freed?
>

I copied that pattern from somewhere else, so yeah, I duplicated whatever
leak was there. Fixed.


> I think that you should use appendPQExpBufferChar and Str instead of
> relying on the format variant which is probably expensive. Something like:
>
>   if (num_options > 0)
> append...Char(buf, ' ');
>   append...Str(buf, ...);
>

All flavors of appendPQExpBuffer*() I can find have a const *char format
string, so no way to append a naked string. If you know differently, I'm
listening. Not fixed.


>
> is_true_boolean_expression: "return (success) ? tf : false;"
> Is this simply: "return success && tf;"?
>

Neat. Done.


>
> Some functions have opt1, opt2, but some start at opt0. This does not look
> too consistent, although the inconsistency may be preexisting from your
> patch. Basically, there is a need for some more restructuring in
> "command.c".


It is pre-existing. Maybe this patch will inspire someone else to make the
other more consistent.

v26 attached
From fc0c466b0331839efe722d089a8ead521e8a827e Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huin...@moat.com>
Date: Sun, 26 Mar 2017 17:30:51 -0400
Subject: [PATCH] psql if v26

---
 doc/src/sgml/ref/psql-ref.sgml |   90 +-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1383 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 ++
 src/bin/psql/conditional.h |   62 +
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql.out |  110 ++
 .../regress/expected/psql_if_on_error_stop.out |   14 +
 src/test/regress/parallel_schedule |2 +-
 src/test/regress/serial_schedule   |1 +
 src/test/regress/sql/psql.sql  |  109 ++
 src/test/regress/sql/psql_if_on_error_stop.sql |   17 +
 19 files changed, 1699 insertions(+), 284 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h
 create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out
 create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored.  Conditional commands are checked only for valid nesting.
+
+
+

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

v25

- PQExpBuffer on gather_boolean_expression()
- convenience/clarity functions: ignore_slash_option(),
ignore_2_slash_options(), ignore_slash_line()
- remove inaccurate test of variable expansion in a false block
- added kitchen-sink test of parsing slash commands in a false block
- removed spurious file that shouldn't have been in v24
- removed any potential free(NULL) calls *that I introduced*, others remain
from master branch

NOT done:
- grouping all branching commands into one function - can be done in a
later patch for clarity
- combining _ef / _ev or _sf / _sv - can be done in a later patch for
clarity


On Fri, Mar 24, 2017 at 4:33 PM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO <coe...@cri.ensmp.fr>
> wrote:
>
>>
>> Hello Corey,
>>
>> I wished for the same thing, happy to use one if it is made known to me.
>>> I pulled that pattern from somewhere else in the code, and given that the
>>> max number of args for a command is around 4, I'm not too worried about
>>> scaling.
>>>
>>
>> If there are expressions one day like pgbench, the number of arguments
>> becomes arbitrary. Have you looked at PQExpBuffer?
>
>
> I will look into it.
>
>
>>
>>>> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
>>>> make sense to create a function instead of keeping the initial
>>>> copy-paste?
>>>>
>>>
>>> Yes, and a few things like that, but I wanted this patch to keep as much
>>> code as-is as possible.
>>>
>>
>> If you put the generic function at the same place, one would be more or
>> less kept and the other would be just removed?
>
>
>> "git diff --patience -w" gives a rather convenient output for looking at
>> the patch.
>
>
> Good to know about that option.
>
> As for a function for digested ignored slash options, it seems like I can
> disregard the true/false value of the "semicolon" parameter. Is that
> correct?
>
>
>> I would suggest to put together all if-related backslash command, so that
>>>> the stack management is all in one function instead of 4.
>>>>
>>>
>>> I recognize the urge to group them together, but would there be any
>>> actual
>>> code sharing between them? Wouldn't I be either re-checking the string
>>> "cmd" again, or otherwise setting an enum that I immediately re-check
>>> inside the all_branching_commands() function?
>>>
>>
>> I do not see that as a significant issue, especially compared to the
>> benefit of having the automaton transition management in a single place.
>
>
> I'm still struggling to see how this would add any clarity to the code
> beyond what I can achieve by clustering the exec_command_(if/elif/else/endif)
> near one another.
>
>
>
>


0001.if_endif.v25.patch
Description: Binary data

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

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 24, 2017 at 4:10 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> I wished for the same thing, happy to use one if it is made known to me.
>> I pulled that pattern from somewhere else in the code, and given that the
>> max number of args for a command is around 4, I'm not too worried about
>> scaling.
>>
>
> If there are expressions one day like pgbench, the number of arguments
> becomes arbitrary. Have you looked at PQExpBuffer?


I will look into it.


>
>>> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
>>> make sense to create a function instead of keeping the initial
>>> copy-paste?
>>>
>>
>> Yes, and a few things like that, but I wanted this patch to keep as much
>> code as-is as possible.
>>
>
> If you put the generic function at the same place, one would be more or
> less kept and the other would be just removed?


> "git diff --patience -w" gives a rather convenient output for looking at
> the patch.


Good to know about that option.

As for a function for digested ignored slash options, it seems like I can
disregard the true/false value of the "semicolon" parameter. Is that
correct?


> I would suggest to put together all if-related backslash command, so that
>>> the stack management is all in one function instead of 4.
>>>
>>
>> I recognize the urge to group them together, but would there be any actual
>> code sharing between them? Wouldn't I be either re-checking the string
>> "cmd" again, or otherwise setting an enum that I immediately re-check
>> inside the all_branching_commands() function?
>>
>
> I do not see that as a significant issue, especially compared to the
> benefit of having the automaton transition management in a single place.


I'm still struggling to see how this would add any clarity to the code
beyond what I can achieve by clustering the
exec_command_(if/elif/else/endif) near one another.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> A few comments about the patch.
>
> Patch applies. "make check" ok.
>
> As already pointed out, there is one useless file in the patch.
>
> Although currently there is only one expected argument for boolean
> expressions, the n² concatenation algorithm in gather_boolean_expression is
> not very elegant. Is there some string buffer data structure which could be
> used instead?
>

I wished for the same thing, happy to use one if it is made known to me.
I pulled that pattern from somewhere else in the code, and given that the
max number of args for a command is around 4, I'm not too worried about
scaling.


>
> ISTM that ignore_boolean_expression may call free on a NULL pointer if the
> expression is empty?
>

True. The psql code is actually littered with a lot of un-checked free(p)
calls, so I started to wonder if maybe we had a wrapper on free() that
checked for NULL. I'll fix this one just to be consistent.


>
> Generally I find the per-command functions rather an improvement.
>

I did too. I tried to split this patch up into two parts, one that broke
out the functions, and one that added if-then, and found that the first
patch was just as unwieldily without the if-then stuff as with.


>
> However there is an impact on testing because of all these changes. ISTM
> that test cases should reflect this situation and test that \cd, \edit, ...
> are indeed ignored properly and taking account there expected args...
>

I think one grand

\if false
\a
\c some_connect_string
...
\z some_table_name
\endif

should do the trick, but it wouldn't detect memory leaks.


>
> In "exec_command_connect" an argument is changed from "-reuse-previous" to
> "-reuse-previous=", not sure why.
>

It shouldn't have been. Good catch. Most commands were able to be migrated
with simple changes (status => *status, strcmp() if-block becomes
active-if-block, etc), but that one was slightly different.


>
> There seems to be pattern repetition for _ev _ef and _sf _sv. Would it
> make sense to create a function instead of keeping the initial copy-paste?
>

Yes, and a few things like that, but I wanted this patch to keep as much
code as-is as possible.


>
> I think that some functions could be used for some repeated cases such as
> "discard one arg", "discard one or two arg", "discard whole line", for the
> various inactive branches, so as to factor out code.
>

I'd be in favor of that as well


>
> I would suggest to put together all if-related backslash command, so that
> the stack management is all in one function instead of 4.
>

I recognize the urge to group them together, but would there be any actual
code sharing between them? Wouldn't I be either re-checking the string
"cmd" again, or otherwise setting an enum that I immediately re-check
inside the all_branching_commands() function?


>
> For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not
> sure why.


An oversight. Good catch.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 19, 2017 at 1:18 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> v24 highlights:
>>
>
> The v24 patch is twice larger that the previous submission. Sigh.
>
> If I'm reading headers correctly, it seems that it adds an
> "expected/psql-on-error-stop.out" file without a corresponding test
> source in "sql/". Is this file to be simply ignored, or is a source missing?


Ignore it. I created the new .sql/.out pair, realized that the file naming
convention was underscores not dashes, changed them and evidently forgot
that I had already added a dashed one to git.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 19, 2017 at 4:23 PM, Fabien COELHO  wrote:

>
> Hello Tom,
>
> I'm not entirely convinced that function-per-command is an improvement
>> though. [...]
>>
>
> I don't have a definite opinion on that core question yet, since I've not
>> read this version of the patch.  Anybody else want to give an opinion?
>>
>
> My 0.02€:
>
> I've already provided my view...
>
> Personnally I like good functions. Maybe a per-command-family set of
> functions could improve the code readability, but (1) I'm not sure this is
> achieved by this patch (eg the if-related state management is now
> dispatched in 4 functions) and (2) I'm not sure that this approach helps
> much with respect to trying to factor out backslash-command-related
> active-or-not argument management.
>
> However I have not looked at the patch in detail. I'm planing to do so
> later this week.


I offered to split the patch into two steps (1. break each "family" into
it's own function and 2. Do what's needed for \if-\endif) but got no
response. I can still do that if people think it's worthwhile.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 17, 2017 at 2:18 PM, Corey Huinker <corey.huin...@gmail.com>
wrote:

>
>> > \set x 'arg1 arg2'
>>
>> > \if false
>> > \cmd_that_takes_exactly_two_args :x
>> > \endif
>>
>> Yeah, throwing errors for bad arguments would also need to be suppressed.
>>
>> regards, tom lane
>>
>
> Ok, barring other feedback, I'm going to take my marching orders as "make
> each slash command active-aware". To keep that sane, I'm probably going to
> break out each slash command family into it's own static function.
>

...and here it is.

v24 highlights:

- finally using git format-patch
- all conditional slash commands broken out into their own functions
(exec_command_$NAME) , each one tests if it's in an active branch, and if
it's not it consumes the same number of parameters, but discards them.
comments for each slash-command family were left as-is, may need to be
bulked up.
- TAP tests discarded in favor of one ON_EROR_STOP test for invalid \elif
placement
- documentation recommending ON_ERROR_STOP removed, because invalid
expressions no longer throw off if-endif balance
- documentation updated to reflex that contextually-correct-but-invalid
boolean expressions are treated as false
- psql_get_variable has a passthrough void pointer now, but I ended up not
needing it. Instead, all slash commands in false blocks either fetch
OT_NO_EVAL or OT_WHOLE_LINE options. If I'm missing something, let me know.
From da8306acab0f0304d62f966c5217139bacfe722e Mon Sep 17 00:00:00 2001
From: Corey Huinker <corey.huin...@moat.com>
Date: Sat, 18 Mar 2017 12:47:25 -0400
Subject: [PATCH] Add \if...\endif blocks

---
 doc/src/sgml/ref/psql-ref.sgml |   90 +-
 src/bin/psql/.gitignore|2 +
 src/bin/psql/Makefile  |4 +-
 src/bin/psql/command.c | 1513 
 src/bin/psql/common.c  |4 +-
 src/bin/psql/conditional.c |  103 ++
 src/bin/psql/conditional.h |   62 +
 src/bin/psql/copy.c|4 +-
 src/bin/psql/help.c|7 +
 src/bin/psql/mainloop.c|   54 +-
 src/bin/psql/prompt.c  |6 +-
 src/bin/psql/prompt.h  |3 +-
 src/bin/psql/startup.c |8 +-
 src/test/regress/expected/psql-on-error-stop.out   |  506 +++
 src/test/regress/expected/psql.out |  100 ++
 .../regress/expected/psql_if_on_error_stop.out |   14 +
 src/test/regress/parallel_schedule |2 +-
 src/test/regress/serial_schedule   |1 +
 src/test/regress/sql/psql.sql  |  100 ++
 src/test/regress/sql/psql_if_on_error_stop.sql |   17 +
 20 files changed, 2316 insertions(+), 284 deletions(-)
 create mode 100644 src/bin/psql/conditional.c
 create mode 100644 src/bin/psql/conditional.h
 create mode 100644 src/test/regress/expected/psql-on-error-stop.out
 create mode 100644 src/test/regress/expected/psql_if_on_error_stop.out
 create mode 100644 src/test/regress/sql/psql_if_on_error_stop.sql

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..18f8bfe 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,93 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \elif has succeeded,
+later matching \elif commands are not evaluated but
+are treated as false.  Lines following an \else are
+processed only if no earlier matching \if
+or \elif succeeded.
+
+
+Lines being skipped are parsed normally to identify queries and
+backslash commands, but queries are not sent to the server, and
+backslash commands other than conditionals
+(\if, \elif,
+\else, \endif) are
+ignored

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> > \set x 'arg1 arg2'
>
> > \if false
> > \cmd_that_takes_exactly_two_args :x
> > \endif
>
> Yeah, throwing errors for bad arguments would also need to be suppressed.
>
> regards, tom lane
>

Ok, barring other feedback, I'm going to take my marching orders as "make
each slash command active-aware". To keep that sane, I'm probably going to
break out each slash command family into it's own static function.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> In the end, I suspect that teaching all the backslash commands to do
> nothing after absorbing their arguments is likely to be the least messy
> way to tackle this, even if it makes for a rather bulky patch.
>
>
Perhaps, but just glancing at \connect makes me think that for some
commands (present or future) the number of args might depend on the value
of the first arg, and variable expansion-or-not, backtick execution-or-not
could alter the number of apparent args on the line, like this:

\set x 'arg1 arg2'

\if false
\cmd_that_takes_exactly_two_args :x
\endif

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 17, 2017 at 11:42 AM, Tom Lane  wrote:

> Fabien COELHO  writes:
> >> I also fear that there are corner cases where the behavior would still
> >> be inconsistent.  Consider
> >>
> >> \if ...
> >> \set foo `echo \endif should not appear here`
>
> > In this instance, ISTM that there is no problem. On "\if true", set is
> > executed, all is well. On "\if false", the whole line would be skipped
> > because the if-related commands are only expected on their own line, all
> > is well again. No problem.
>
> AFAICS, you misunderstood the example completely, or else you're proposing
> syntax restrictions that are even more bizarre and unintelligible than
> I thought before.  We cannot have a situation where the syntax rules for
> backslash commands inside an \if are fundamentally different from what
> they are elsewhere; that's just going to lead to confusion and bug
> reports.
>
> regards, tom lane
>

I think Fabien was arguing that inside a false block there would be no
syntax rules beyond "is the first non-space character on this line a '\'
and if so is it followed with a if/elif/else/endif?". If the answer is no,
skip the line. To me that seems somewhat similar to Tom's suggestion that a
false branch just keeps consuming text until it encounters a \conditional
or EOF.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> command.c:38:25: fatal error: conditional.h: No such file or directory
>  #include "conditional.h"
>

Odd, it's listed as a new file in git status. Anyway, my point of posting
the WIP patch was to give people a reference point and spark discussion
about the next step, and it succeeded at that.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

Attached is the latest work. Not everything is done yet. I post it because
the next step is likely to be "tedious" as Tom put it, and if there's a way
out of it, I want to avoid it.

What is done:
- all changes here built off the v22 patch
- any function which had scan_state and cond_stack passed in now only has
scan_state, and cond_stack is extracted from the cb_passthrough pointer.
- ConditonalStack is now only explictly passed to get_prompt ... which
doesn't have scan state
- Conditional commands no longer reset scan state, nor do they clear the
query buffer
- boolean expressions consume all options, but only evaluate variables and
backticks in situations where those would be active
- invalid boolean arguments are treated as false
- contextually wrong \else, \endif, \elif are still errors

What is not done:
- TAP tests are not converted to regular regression test(s)
- skipped slash commands still consume the rest of the line

That last part is big, to quote Tom:

* More generally, I do not think that the approach of having exec_command
simply fall out immediately when in a false branch is going to work,
because it ignores the fact that different backslash commands have
different argument parsing rules.  Some will eat the rest of the line and
some won't.  I'm afraid that it might be necessary to remove that code
block and add a test to every single backslash command that decides
whether to actually perform its action after it's consumed its arguments.
That would be tedious :-(.  But as it stands, backslash commands will get
parsed differently (ie with potentially-different ending points) depending
on whether they're in a live branch or not, and that seems just way too
error-prone to be allowed to stand.


If that's what needs to be done, does it make sense to first commit a
pre-patch that encapsulates each command family ( \c and \connect are a
family,  all \d* commands are one family) into its own static function? It
would make the follow-up patch to if-endif cleaner and easier to review.


On Thu, Mar 16, 2017 at 5:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> > Ok, I've got some time now and I'm starting to dig into this. I'd like to
> > restate what I *think* my feedback is, in case I missed or misunderstood
> > something.
> > ...
> > 3. Change command scans to scan the whole boolean expression, not just
> > OT_NORMAL.
> > There's a couple ways to go about this. My gut reaction is to create a
> new
> > scan type OT_BOOL_EXPR, which for the time being is the same as
> > OT_WHOLE_LINE, but could one day be something different.
>
> OT_WHOLE_LINE is not what you want because that results in verbatim
> copying, without variable expansion or anything.  My vote would be to
> repeatedly do OT_NORMAL until you get a NULL, thereby consuming as
> many regular arguments as the backslash command has.  (After which,
> if it wasn't exactly one argument, complain, for the moment.  But this
> leaves the door open for something like "\if :foo = :bar".)  Note that
> this implies that "\if some-expression \someothercommand" will be allowed,
> but I think that's fine, as I see no reason to allow backslashes in
> whatever if-expression syntax we invent later.  OT_WHOLE_LINE is a bit of
> a bastard child and I'd just as soon not define it as being the lexing
> behavior of any new commands.
>
> > 5. Allow contextually-correct invalid boolean expressions to map to
> false.
>
> > Out-of-context \endif, \else, and \elif commands remain as errors to be
> > ignored, invalid expressions in an \if or legallyl-placed \elif are just
> > treated as false.
>
> WFM.
>
> regards, tom lane
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 2a9c412..7743fb0 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2064,6 +2064,102 @@ hello 10
 
 
   
+\if expression
+\elif expression
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks.
+A conditional block must begin with an \if and end
+with an \endif.  In between there may be any number
+of \elif clauses, which may optionally be followed
+by a single \else clause.  Ordinary queries and
+other types of backslash commands may (and usually do) appear between
+the commands forming a conditional block.
+
+
+The \if and \elif commands read
+the rest of the line and evaluate it as a boolean expression.  If the
+expression is true then processing continues
+normally; otherwise, lines are skipped until a
+matching \elif, \else,
+or \endif is reached.  Once
+an \if or \el

Re: [HACKERS] asynchronous execution

[Corey Huinker]

On Thu, Mar 16, 2017 at 4:17 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> > I reworked the test such that all of the foreign tables inherit from the
> > same parent table, and if you query that you do get async execution. But
> It
> > doesn't work when just stringing together those foreign tables with UNION
> > ALLs.
>
> > I don't know how to proceed with this review if that was a goal of the
> > patch.
>
> Whether it was a goal or not, I'd say there is something either broken
> or incorrectly implemented if you don't see that.  The planner (and
> therefore also the executor) generally treats inheritance the same as
> simple UNION ALL.  If that's not the case here, I'd want to know why.
>
> regards, tom lane
>

Updated commitfest entry to "Returned With Feedback".

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Mar 13, 2017 at 5:21 PM, Tom Lane  wrote:

> "Daniel Verite"  writes:
> > Tom Lane wrote:
> >> when we see \if is that we do nothing but absorb text
> >> until we see the matching \endif.  At that point we could bitch and
> throw
> >> everything away if, say, there's \elif after \else, or anything else you
> >> want to regard as a "compile time error".  Otherwise we start execution,
> >> and from there on it probably has to behave as we've been discussing.
> >> But this'd be pretty unfriendly from an interactive standpoint, and I'm
> >> not really convinced that it makes for significantly better error
> >> reporting.
>
> > On the whole, isn't that a reasonable model to follow for psql?
>
> One thing that occurs to me after more thought is that with such a model,
> we could not have different lexing rules for live vs not-live branches,
> since we would not have made those decisions before scanning the input.
> This seems problematic.  Even if you discount the question of whether
> variable expansion is allowed to change command-boundary decisions, we'd
> still not want backtick execution to happen everywhere in the block, ISTM.
>
> Maybe we could fix things so that backtick execution happens later, but
> it would be a pretty significant and invasive change to backslash command
> execution, I'm afraid.
>
> regards, tom lane
>

Ok, I've got some time now and I'm starting to dig into this. I'd like to
restate what I *think* my feedback is, in case I missed or misunderstood
something.

1. Convert perl tests to a single regular regression test.

2. Have MainLoop() pass the cond_stack to the lexer via
psql_scan_set_passthrough(scan_state, (void *) cond_stack);

3. Change command scans to scan the whole boolean expression, not just
OT_NORMAL.

There's a couple ways to go about this. My gut reaction is to create a new
scan type OT_BOOL_EXPR, which for the time being is the same as
OT_WHOLE_LINE, but could one day be something different.

4. Change variable expansion and backtick execution in false branches to
match new policy.

I've inferred that current preference would be for no expansion and no
execution.

5. Allow contextually-correct invalid boolean expressions to map to false.

Out-of-context \endif, \else, and \elif commands remain as errors to be
ignored, invalid expressions in an \if or legallyl-placed \elif are just
treated as false.

Did I miss anything?

Re: [HACKERS] asynchronous execution

[Corey Huinker]

On Mon, Mar 13, 2017 at 9:28 PM, Amit Langote <langote_amit...@lab.ntt.co.jp
> wrote:

> On 2017/03/14 10:08, Corey Huinker wrote:
> >> I don't think the plan itself will change as a result of applying this
> >> patch. You might however be able to observe some performance
> improvement.
> >
> > I could see no performance improvement, even with 16 separate queries
> > combined with UNION ALL. Query performance was always with +/- 10% of a
> 9.6
> > instance given the same script. I must be missing something.
>
> Hmm, maybe I'm missing something too.
>
> Anyway, here is an older message on this thread from Horiguchi-san where
> he shared some of the test cases that this patch improves performance for:
>
> https://www.postgresql.org/message-id/20161018.103051.
> 30820907.horiguchi.kyotaro%40lab.ntt.co.jp
>
> From that message:
>
> 
> I measured performance and had the following result.
>
> t0  - SELECT sum(a) FROM ;
> pl  - SELECT sum(a) FROM <4 local children>;
> pf0 - SELECT sum(a) FROM <4 foreign children on single connection>;
> pf1 - SELECT sum(a) FROM <4 foreign children on dedicate connections>;
>
> The result is written as "time (std dev )"
>
> sync
>   t0: 3820.33 (  1.88)
>   pl: 1608.59 ( 12.06)
>  pf0: 7928.29 ( 46.58)
>  pf1: 8023.16 ( 26.43)
>
> async
>   t0: 3806.31 (  4.49)0.4% faster (should be error)
>   pl: 1629.17 (  0.29)1.3% slower
>  pf0: 6447.07 ( 25.19)   18.7% faster
>  pf1: 1876.80 ( 47.13)   76.6% faster
> 
>
> IIUC, pf0 and pf1 is the same test case (all 4 foreign tables target the
> same server) measured with different implementations of the patch.
>
> Thanks,
> Amit
>

I reworked the test such that all of the foreign tables inherit from the
same parent table, and if you query that you do get async execution. But It
doesn't work when just stringing together those foreign tables with UNION
ALLs.

I don't know how to proceed with this review if that was a goal of the
patch.

Re: [HACKERS] asynchronous execution

[Corey Huinker]

>
> I don't think the plan itself will change as a result of applying this
> patch. You might however be able to observe some performance improvement.
>
> Thanks,
> Amit
>

I could see no performance improvement, even with 16 separate queries
combined with UNION ALL. Query performance was always with +/- 10% of a 9.6
instance given the same script. I must be missing something.

Re: [HACKERS] asynchronous execution

[Corey Huinker]

On Mon, Mar 13, 2017 at 1:06 AM, Corey Huinker <corey.huin...@gmail.com>
wrote:

>
>> I think it will, because Append itself has been made async-capable by one
>> of the patches and UNION ALL uses Append.  But as mentioned above, only
>> the postgres_fdw foreign tables will be able to utilize this for now.
>>
>>
> Ok, I'll re-run my test from a few weeks back and see if anything has
> changed.
>


I'm not able to discern any difference in plan between a 9.6 instance and
this patch.

The basic outline of my test is:

EXPLAIN ANALYZE
SELECT c1, c2, ..., cN FROM tab1 WHERE date = '1 day ago'
UNION ALL
SELECT c1, c2, ..., cN FROM tab2 WHERE date = '2 days ago'
UNION ALL
SELECT c1, c2, ..., cN FROM tab3 WHERE date = '3 days ago'
UNION ALL
SELECT c1, c2, ..., cN FROM tab4 WHERE date = '4 days ago'


I've tried this test where tab1 through tab4 all are the same postgres_fdw
foreign table.
I've tried this test where tab1 through tab4 all are different foreign
tables pointing to the same remote table sharing a the same server
definition.
I've tried this test where tab1 through tab4 all are different foreign
tables pointing each with it's own foreign server definition, all of which
happen to point to the same remote cluster.

Are there some postgresql.conf settings I should set to get a decent test?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Barring objection I'll push this so that Corey can rebase over it.
>
> regards, tom lane
>

Seems straightforward, and I appreciate you doing it for me!

Re: [HACKERS] asynchronous execution

[Corey Huinker]

>
>
> I think it will, because Append itself has been made async-capable by one
> of the patches and UNION ALL uses Append.  But as mentioned above, only
> the postgres_fdw foreign tables will be able to utilize this for now.
>
>
Ok, I'll re-run my test from a few weeks back and see if anything has
changed.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> (1) document that \if-related commands MUST be on their own
> line (i.e. like cpp #if directives?).
>

I have no opinion on whether \if-related comments must be on their own
line, though I coded as if that were the case.

I want to point out that the goal down the road is to allow rudimentary
expressions beyond just 'will this string cast to boolean true'.

For example, in the earlier thread "Undefined psql variables", I proposed a
slash command that would test if a named psql var were defined, and if not
then assign it a value.

Tom suggested leveraging if-then infrastructure like this

 \if not defined(x)
  \set x y
 \fi

Which would be great. I ask that whatever we decide in terms of how much
more input we read to digest the expression allow for constructs like the
one above.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Mar 12, 2017 at 1:52 AM, David G. Johnston <
david.g.johns...@gmail.com> wrote:
>
> Oddly, Corey was using you as support for this position...though without
an actual quote:
>
> """

Reading this, I started to wonder "so how did I get that impression?" and I
found this from Feb 9:

IMO, an erroneous backslash command should have no effect, period.
"It failed but we'll treat it as if it were valid" is a rathole
I don't want to descend into.  It's particularly bad in interactive
mode, because the most natural thing to do is correct your spelling
and issue the command again --- but if psql already decided to do
something on the strength of the mistaken command, that doesn't work,
and you'll have to do something or other to unwind the unwanted
control state before you can get back to what you meant to do.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Mar 11, 2017 at 4:17 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> > [ 0001.if_endif.v21.diff ]
>
> Starting to poke at this... the proposal to add prove checks for psql
> just to see whether \if respects ON_ERROR_STOP seems like an incredibly
> expensive way to test a rather minor point.  On my machine, "make check"
> in bin/psql goes from zero time to close to 8 seconds.  I'm not really
> on board with adding that kind of time to every buildfarm run for the
> foreseeable future just for this.
>
> Couldn't we get close to the same coverage by adding a single-purpose
> test script to the main regression tests?  Along the lines of
>
>  \set ON_ERROR_STOP 1
>  \if invalid
>  \echo should not get here
>  \endif
>  \echo should not get here either
>
> You could imagine just dropping that at the end of psql.sql, but I
> think probably a separate script is worth the trouble.
>
> regards, tom lane
>


I think I can manage that. Just to be clear, you're asking me to replace
the perl script with one new sql script? If so, there's probably a few
non-on-stop tests in there that might be worth preserving in regression
form.

Re: [HACKERS] asynchronous execution

[Corey Huinker]

On Thu, Feb 23, 2017 at 6:59 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> 9e43e87


Patch fails on current master, but correctly applies to 9e43e87. Thanks for
including the commit id.

Regression tests pass.

As with my last attempt at reviewing this patch, I'm confused about what
kind of queries can take advantage of this patch. Is it only cases where a
local table has multiple inherited foreign table children? Will it work
with queries where two foreign tables are referenced and combined with a
UNION ALL?

Re: [HACKERS] contrib modules and relkind check

[Corey Huinker]

On Tue, Feb 14, 2017 at 1:30 AM, Michael Paquier 
wrote:

> Hm... It may be a good idea to be consistent on the whole system and
> refer to "partitioned table" as a table without storage and used as an
> entry point for partitions. The docs use this term in CREATE TABLE,
> and we would finish with messages like "not a table or a partitioned
> table". Extra thoughts are welcome here, the current inconsistencies
> would be confusing for users.
>

Updated CF status to "Waiting on Author". Waiting, mostly for the
regression tests suggested.

Michael, do you want to add yourself as a reviewer?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Mar 3, 2017 at 1:25 AM, Fabien COELHO  wrote:

>
>
> For endif, I really exagerated, "switch { defaut: " is too much, please
>> accept my apology. Maybe just do the pop & error reporting?
>>
>
It seemed like overkill, but I decided to roll with it.


>
> Or maybe be more explicit:
>
>   switch (current state)
>   case NONE:
>  error no matching if;
>   case ELSE_FALSE:
>   case ELSE_TRUE:
>   case ...:
>  pop;
>  Assert(success);


the pop() function tests for an empty stack, so this switch is
double-testing, but it's also no big deal, so here you go...
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> For me, it is only slightly better: I think that for helping understanding
> and maintenance, the automaton state transitions should be all clear and
> loud in just one place, so I would really like to see a single common
> structure:
>
>   if (is "if") switch on all states;
>   else if (is "elif") switch on all states;
>   else if (is "else") switch on all states;
>   else if (is "endif") switch on all states;
>
> And minimal necessary error handling around that.
>
>
v20: attempt at implementing the switch-on-all-states style.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Mar 2, 2017 at 1:23 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> That is accurate. The only positive it has is that the user only
>> experiences one error, and it's the first error that was encountered if
>> reading top-to-bottom, left to right. It is an issue of which we
>> prioritize
>> - user experience or simpler code.
>>
>
> Hmmm. The last simpler structure I suggested, which is basically the one
> used in your code before the update, does check for the structure error
> first. The only drawback is that the condition is only evaluated when
> needed, which is an issue we can cope with, IMO.


Tom was pretty adamant that invalid commands are not executed. So in a case
like this, with ON_ERROR_STOP off:

\if false
\echo 'a'
\elif true
\echo 'b'
\elif invalid
\echo 'c'
\endif

Both 'b' and 'c' should print, because "\elif invalid" should not execute.
The code I had before was simpler, but it missed that.


>
> Now if you want to require committer opinion on this one, fine with me.
>>>
>>
>> Rather than speculate on what a committer thinks of this edge case (and
>> making a patch for each possible theory), I'd rather just ask them what
>> their priorities are and which user experience they favor.
>>
>
> ISTM that the consistent message by Robert & Tom was to provide simpler
> code even if the user experience is somehow degraded, as they required that
> user-friendly features were removed (eg trying to be nicer about structural
> syntax errors, barking in interactive mode so that the user always knows
> the current status, providing a detailed status indicator in the prompt...).
>

Ok, so here's one idea I tossed around, maybe this will strike the right
balance for you.

If I create a function like this:

static boolean
is_valid_else_context(IfState if_state, const char *cmd)
{
/* check for invalid \else / \elif contexts */

switch (if_state)

{
case IFSTATE_NONE:
/* not in an \if block */
psql_error("\\%s: no matching \\if\n", cmd);
return false;
break;
case IFSTATE_ELSE_TRUE:
case IFSTATE_ELSE_FALSE:
psql_error("\\%s: cannot occur after \\else\n", cmd);
return false;
break;
default:
break;
}
return true;
}



Then the elif block looks something like this:

else if (strcmp(cmd, "elif") == 0)
{
ifState if_state = conditional_stack_peek(cstack);

if (is_valid_else_context(if_state, "elif"))
{
/*
 * valid \elif context, check for valid expression
 */
bool elif_true = false;
success = read_boolean_expression(scan_state, "\\elif ",
_true);
if (success)
{
/*
 * got a valid boolean, what to do with it depends on
current
 * state
 */
switch (if_state)
{
case IFSTATE_IGNORED:
/*
 * inactive branch, do nothing.
 * either if-endif already had a true block,
 * or whole parent block is false.
 */
break;
case IFSTATE_TRUE:
/*
 * just finished true section of this if-endif,
 * must ignore the rest until \endif
 */
conditional_stack_poke(cstack, IFSTATE_IGNORED);
break;
case IFSTATE_FALSE:
/*
 * have not yet found a true block in this if-endif,
 * this might be the first.
 */
if (elif_true)
conditional_stack_poke(cstack, IFSTATE_TRUE);
break;
default:
/* error cases all previously ruled out */
break;
}
}
}
else
success = false;
psql_scan_reset(scan_state);
}


This is functionally the same as my latest patch, but the ugliness of
switching twice on if_state is hidden.

As an added benefit, the "else"-handling code gets pretty simple because it
can leverage that same function.

Does that handle your objections?

p.s.  do we try to avoid constructs likeif (success = my_function(var1,
var2))   ?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 12:23 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> on elif
>>  if misplaced elif
>> misplaced elif error
>>  else
>> eval expression
>>   => possible eval error
>> set new status if eval fine
>>
>
> Currently it is really:
>
>   switch (state) {
>   case NONE:
>   case ELSE_TRUE:
>   case ELSE_FALSE:
>  success = false;
>  show some error
>   default:
>   }
>   if (success) {
> success = evaluate_expression(...);
> if (success) {
>switch (state) {
>case ...:
>default:
>}
> }
>   }
>
> Which I do not find so neat. The previous one with nested switch-if-switch
> looked as bad.


That is accurate. The only positive it has is that the user only
experiences one error, and it's the first error that was encountered if
reading top-to-bottom, left to right. It is an issue of which we prioritize
- user experience or simpler code.

Now if you want to require committer opinion on this one, fine with me.


Rather than speculate on what a committer thinks of this edge case (and
making a patch for each possible theory), I'd rather just ask them what
their priorities are and which user experience they favor.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Mar 1, 2017 at 3:07 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> It doesn't strike me as much cleaner, but it's no worse, either.
>>
>
> Hmmm.
>
> The "if (x) { x = ... ; if (x) {" does not help much to improve
> readability and understandability...
>
> My 0.02€ about v19:
>
> If there are two errors, I do not care which one is shown, both will have
> to be fixed anyway in the end... So I would suggest to choose the simplest
> possible implementation:
>
>   on elif:
> always eval expression
>   => possible eval error
> switch
>   => including detecting misplaced elif errors
>
> If the second error must absolutely be shown in all cases, then add a
> second misplaced elif detection in the eval expression failure branch:
>
>   on elif
> always eval
> if (eval failed)
>   also checked for misplaced (hey user, you have 2 errors in fact...)
>   bye bye...
> // else eval was fine
> switch
>   including misplaced elif detection
>
> If the committer is angry at these simple approach, then revert to the
> strange looking and hard to understand switch-if-switch solution (~ v18, or
> some simplified? v19), but I do not think the be weak benefit is worth the
> code complexity.
>
> --
> Fabien.


Just to make things easy for the committer, the existing code only shows
the user one error:

on elif
  if misplaced elif
 misplaced elif error
  else
 eval expression
   => possible eval error
 set new status if eval fine


The issue at hand being the benefit to the user vs code complexity.

So, shall we send this off to the committers and let them decide?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
>>
>> Alternatively if the structure must really be kept, then deal with errors
>> in a first switch, read value *after* switch and deal with other errors
>> there, then start a second switch, and adjust the documentation accordingly?
>>
>>   switch
>> errors
>>   read
>>   if
>> errors
>>   // no error
>>   switch
>>
>
>
it's now something more like

switch
  error-conditions
if no-errors
  read
  if was a boolean
  switch last-state

It doesn't strike me as much cleaner, but it's no worse, either.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..9d245a9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,8 +3780,9 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
-or ! if the session is disconnected from the
+but @ if the session is in a false conditional
+block, or ^ if in single-line mode, or
+! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
 depends on why psql expects more input:
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f 

Re: [HACKERS] some dblink refactoring

[Corey Huinker]

On Tue, Feb 28, 2017 at 10:09 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> Here is a patch to refactor some macro hell in dblink.
>
> This patch was discussed in the background sessions thread as a
> prerequisite for some work there, but I figure I'll make a separate
> thread for it to give everyone interested in dblink a chance to respond
> separate from the other thread.
>

+1

Any chance we can make get_connect_string() a core function or at least
externally accessible?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sun, Feb 26, 2017 at 2:47 AM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> About v18: Patch applies, make check ok, psql tap tests ok.
>
>
> ISTM that contrary to the documentation "\elif something" is not evaluated
> in all cases, and the resulting code is harder to understand with a nested
> switch and condition structure:
>
>   switch
>   default
> read
> if
>switch
>
> I wish (this is a personal taste) it could avoid switch-nesting on the
> very same value. It should also conform to the documentation.
>

I wasn't too happy with it, but I figured it would spark discussion. I
succeeded.


>
> If there is no compelling reason for the switch-nesting, I would suggest
> to move the read_boolean_expression before the swich, to deal with error
> immediately there, and then to have just one switch.
>

I thought about doing it that way. However, in the case of:


\set x invalid

\if true

\else
\elif :x

\endif


The error has already "happened" at line 4, char 5, and it doesn't matter
what expression follows, you will get an error.  But because
read_boolean_expression() can emit errors, you would see the error saying
"invalid" isn't a valid boolean expression, and then see another error
saying that the \elif was out of place. If we suppress
read_boolean_expression()'s error reporting, then we have to re-create that
error message ourselves, or be fine with the error message on invalid
\elifs being inconsistent with invalid \ifs.

Similar to your suggestion below, we could encapsulate the first switch
into a function valid_elif_context(ConditionalStack), which might make the
code cleaner, but would make it harder to see that all swtich-cases are
covered between the two. That might be a tradeoff we want to make.


>
> Alternatively if the structure must really be kept, then deal with errors
> in a first switch, read value *after* switch and deal with other errors
> there, then start a second switch, and adjust the documentation accordingly?
>
>   switch
> errors
>   read
>   if
> errors
>   // no error
>   switch
>


How would the documentation have to change?

Also, the %R documentation has single line marker '^' before not executed
> '@', but it is the reverse in the code.


Noted and fixed in the next patch, good catch.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Typo "unterminted".
>

Fixed.


> The \if within the \if false branch is not tallied properly? Am I missing
> something?
>

Nope, you found a bug. FIxed. Test-case added.


> I changed the paragraph to
>>
>
>Lines within false branches are parsed normally, however, any
>> completed
>>queries are not sent to the server, and any completed commands
>> other
>>than conditionals (\if,
>> \elif,
>>\else, \endif) are ignored.
>>
>
> I'm not sure about the ", however, " commas, but I'm sure that I do not
> know English punctuation rules:-)
>

Re-worded it again for shorter sentences. Re-mentioned that conditionals
are still checked for proper nesting.

* Changed comments to reflect that \if always evalutes  even in a
false branch
* Changed \elif to first check if the command is in a proper \if block
before evaluating the expression. The invalid boolean message would mask
the bigger problem.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..27824d9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,83 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally for query/command
+completion, but any queries are not sent to the server, and any
+commands other than conditionals (\if,
+\elif, \else,
+\endif) are ignored. Conditional commands are
+still checked for valid nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,7 +3780,8 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
+but ^ if in single-line mode, or
+@ if the session is in a false conditional block,
 or ! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 %R is replaced by a character that
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> I'm not sure that '@' is the best choice, but this is just one char.


> I noticed that it takes precedence over '!'. Why not. ISTM that orthogonal
> features are not shown independently, but this is a preexisting state, and
> it allows to have a shorter prompt, so why not.
>

My reasoning was this: if you're in a false block, and you're not connected
to a db, the \c isn't going to work for you until you get out of the false
block, so right now being in a false block is a bigger problem than not
being connected to a db. I have no strong opinion about what should happen
here, so I welcome suggestions for what tops what.


>
> Anyway, the '%R' documentation needs to be updated.


Done.


> It could be nice to keep test cases that show what may happen?
>

Restored. It looks weird now, but it fixes the unterminated quoted string.


> The various simplifications required result in the feature being more
> error prone for the user. Maybe the documentation could add some kind of
> warning about that?


I changed the paragraph to
Lines within false branches are parsed normally, however, any
completed
queries are not sent to the server, and any completed commands other
than conditionals (\if, \elif,
\else, \endif) are ignored.

There's no mention that psql variables AREN'T expanded, so the user has
every expectation that they are.


>
> Add space after comma when calling send_query.
>

Done.


>
> I'm not sure why you removed the comments before \if in the doc example.
> Maybe keep a one liner?
>

Didn't mean to, restored.


> Why not reuse the pop loop trick to "destroy" the stack?


Forgot about that, restored.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..625c9a8 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,81 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+Lines within false branches are parsed normally, however, any completed
+queries are not sent to the server, and any completed commands other
+than conditionals (\if, \elif,
+\else, \endif) are ignored.
+
+
+  
 
   
 \ir or \include_relative 
filename
@@ -3703,7 +3778,8 @@ testdb= INSERT INTO my_table VALUES 
(:'content');
 
 
 In prompt 1 normally =,
-but ^ if in single-line mode,
+but ^ if in single-line mode, or
+@ if the session is in a false conditional block,
 or ! if the session is disconnected from the
 database (which can happen if \connect fails).
 In prompt 2 

Re: [HACKERS] Idea on how to simplify comparing two sets

[Corey Huinker]

>
> FWIW I've found myself needing the precursor to this this (give me the
> full diff) at least a couple times, and it's quite a PITA on anything but a
> trivial relation.
>
> It's also not possible to make this easier via an SRF because you don't
> know in advance what the result set looks like. So the best I've ever come
> up with is a file that can be included in psql that depends on having set
> two psql variables to the names of relations that can be queried (and if
> you need more than a relation you need to create a temp view).
>
> I've wondered about the possibility of allowing PLs the ability to
> dynamically define their return type based on their arguments. That would
> allow for an SRF to handle this case, and would be significantly more
> flexible than trying to do that using pseudotypes.


My experiences are similar. At the moment, I'm resigned to using relying on
pgtap:

-- set environment/path to point to "control"
create temporary table test_1_wants as ;

-- set environment/path to point to "experiment"
create temporary table test_1_has as ;

select results_eq( 'table test_1_has', 'table test_1_wants', 'test 1');

I've had to do it with temp tables any time the environment is different
between control/experiment, which is the case when you're developing a
drop-in replacement for an SRF or view or whatever.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Files "conditional.h" and "conditional.c" are missing from the patch.
>
> Also, is there a particular reason why tap test have been removed?


That would be because I diffed against my last commit, not the master
branch, sigh.

v16 is everything v15 promised to be.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..6c15c01 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -38,6 +38,7 @@
 #include "fe_utils/string_utils.h"
 
 #include "common.h"
+#include "conditional.h"
 #include "copy.h"
 #include "crosstabview.h"
 #include "describe.h"
@@ -62,6 +63,7 @@ typedef enum EditableObjectType
 /* functions for use in this file */
 static backslashResult exec_command(const char *cmd,
 PsqlScanState scan_state,
+ConditionalStack cstack,
 PQExpBuffer query_buf);
 static bool do_edit(const char *filename_arg, PQExpBuffer query_buf,
int lineno, bool *edited);
@@ -109,6 +111,7 @@ static void checkWin32Codepage(void);
 
 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 6:15 PM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
>> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
>> arguing that variable expansion shouldn't be able to insert, say, an \else
>> in a non-active branch?  Maybe, but if it can insert an \else in an active
>> branch, then why not non-active too?  Seems a bit inconsistent.
>>
>
> The major reason was avoiding situations like what Daniel showed: where
> value of a variable that is meaningless/undefined in the current
> false-block context gets expanded anyway, and thus code inside a false
> block has effects outside of that block. Granted, his example was
> contrived. I'm open to removing that feature and seeing what breaks in the
> test cases.
>


Welcome to v15, highlights:
- all conditional data structure management moved to conditional.h and
conditional.c
- conditional state lives in mainloop.c and is passed to
HandleSlashCommands, exec_command and get_prompt as needed
- no more pset.active_branch, uses conditional_active(conditional_stack)
instead
- PsqlScanState no longer has branching state
- Implements the %R '@' prompt on false branches.
- Variable expansion is never suppressed even in false blocks, regression
test edited to reflect this.
- ConditionalStack could morph into PsqlFileState without too much work.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..dac8e37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_h

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 5:59 PM, Tom Lane  wrote:

> Ah, I see why *that* wants to know about it ... I think.  I suppose you're
> arguing that variable expansion shouldn't be able to insert, say, an \else
> in a non-active branch?  Maybe, but if it can insert an \else in an active
> branch, then why not non-active too?  Seems a bit inconsistent.
>

The major reason was avoiding situations like what Daniel showed: where
value of a variable that is meaningless/undefined in the current
false-block context gets expanded anyway, and thus code inside a false
block has effects outside of that block. Granted, his example was
contrived. I'm open to removing that feature and seeing what breaks in the
test cases.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 5:11 PM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> Dunno, that sounds a lot like an "if the only tool I have is a hammer,
>> then this must be a nail" argument.
>
>
> More of a "don't rock the boat more than absolutely necessary", but
> knowing that adding another global struct might be welcomed is good to know.
>
>

After some research, GetVariable is called by psql_get_variable, which is
one of the callback functions passed to psql_scan_create(). So passing a
state variable around probably isn't going to work and PsqlFileState now
looks like the best option.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Dunno, that sounds a lot like an "if the only tool I have is a hammer,
> then this must be a nail" argument.


More of a "don't rock the boat more than absolutely necessary", but knowing
that adding another global struct might be welcomed is good to know.


> reasonable interpretation of what it's for.  Inventing a PsqlFileState or
> similar struct might be a good idea to help pull some of that cruft out of
> pset and get it back to having a reasonably clearly defined purpose of
> holding "current settings".
>

+1

command was ignored" warning messages).  I'm failing to follow why
>
GetVariable would need to care.
>

It took me a second to find the post, written by Daniel Verite on Jan 26,
quoting:

> Revised patch

A comment about control flow and variables:
in branches that are not taken, variables are expanded
nonetheless, in a way that can be surprising.
Case in point:

\set var 'ab''cd'
-- select :var;
\if false
  select :var ;
\else
  select 1;
\endif

The 2nd reference to :var has a quoting hazard, but since it's within
an "\if false" branch, at a glance it seems like this script might work.
In fact it barfs with:
  psql:script.sql:0: found EOF before closing \endif(s)

AFAICS what happens is that :var gets injected and starts a
runaway string, so that as far as the parser is concerned
the \else ..\endif block slips into the untaken branch, as a part of
that unfinished string.



So that was the reasoning behind requiring GetVariable to know whether or
not the statement was being ignored.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Wed, Feb 22, 2017 at 4:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> >> but if you think that it should be somewhere else, please advise Corey
> >> about where to put it.
>
> > Just a reminder that I'm standing by for advice.
>
> Sorry, I'd lost track of this thread.
>

Judging by the volume of the 50-or-so active threads on this list, I
figured you were busy.


> > The issue at hand is whether the if-state should be a part of the
> > PsqlScanState, or if it should be a separate state variable owned by
> > MainLoop() and passed to HandleSlashCommands(), ... or some other
> solution.
>
> My reaction to putting it in PsqlScanState is pretty much "over my dead
> body".  That's just trashing any pretense of an arms-length separation
> between psql and the lexer proper.  We only recently got done sweating
> blood to create that separation, why would we toss it away already?
>

Good to know that history.


>
> If you're concerned about the notational overhead of passing two arguments
> rather than one, my druthers would be to invent a new struct type, perhaps
> named along the lines of PsqlFileState or PsqlInputState, and pass that
> around.  One of its fields would be a PsqlScanState pointer, the rest
> would be for if-state and whatever else we think we need in per-input-file
> state.
>

I wasn't, my reviewer was. I thought about the super-state structure like
you described, and decided I was happy with two state params.


> However, that way is doubling down on the assumption that the if-state is
> exactly one-to-one with input file levels, isn't it?  We might be better
> off to just live with the separate arguments to preserve some flexibility
> in that regard.  The v12 patch doesn't look that awful in terms of what
> it's adding to argument lists.
>

The rationale for tying if-state to file levels is not so much of anything
if-then related, but rather of the mess we'd create for whatever poor soul
decided to undertake \while loops down the road, and the difficulties
they'd have trying to unwind/rewind jump points in file(s)...keeping it
inside one file makes things simpler for the coding and the coder.


> One thing I'm wondering is why the "active_branch" bool is in "pset"
> and not in the conditional stack.  That seems, at best, pretty grotty.
> _psqlSettings is meant for reasonably persistent state.
>

With the if-stack moved to MainLoop(), nearly all the active_branch checks
could be against a variable that lives in MainLoop(), with two big
exceptions: GetVariable() needs to know when NOT to expand a variable
because it's in a false-block, and get_prompt will need to know when it's
in a false block for printing the '@' prompt hint or equivalent, and pset
is the only global around I know of to do that. I can move nearly all the
is-this-branch-active checks to structures inside of MainLoop(), and that
does strike me as cleaner, but there will still have to be that gross bit
where we update pset.active_branch so that the prompt and GetVariable() are
clued in.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>  but if you think that it should be somewhere else, please advise Corey
> about where to put it.


Just a reminder that I'm standing by for advice.

The issue at hand is whether the if-state should be a part of the
PsqlScanState, or if it should be a separate state variable owned by
MainLoop() and passed to HandleSlashCommands(), ... or some other solution.

Re: [HACKERS] COPY IN/BOTH vs. extended query mode

[Corey Huinker]

On Sun, Feb 19, 2017 at 9:04 AM, Robert Haas  wrote:

If you tried to write an SQL-callable function that internally started
> and ended a copy from the client, then I think you would run into this
> problem, and probably some others.
>
>
That's it. I had a PoC patch submitted that allowed someone to do this

insert into some_table(id, total_sum) select id, sum(a_numeric_metric) from
copy_srf('/a/file/name') group by 1

or

insert into some_table(id, total_sum) select id, sum(a_numeric_metric) from
copy_srf('/a/program/name arg1 arg2',true) group by 1


and those worked just fine, however, attempts to use the STDIN

insert into some_table(id, total_sum) select id, sum(a_numeric_metric) from
copy_srf(null) group by 1

failed, because as it was explained to me, the order of such events would
be:

1. start query
2. send result set format to client
3. start copy which implies that query result set is done
4. finish copy
5. emit query results to client, but the defining result format is gone,
thus error.

I'm just putting this here for future reference in case there is a protocol
change in the works.

Re: [HACKERS] COPY IN/BOTH vs. extended query mode

[Corey Huinker]

On Mon, Feb 13, 2017 at 4:29 PM, Craig Ringer 
wrote:

>
>
> On 14 Feb. 2017 06:15, "Robert Haas"  wrote:
>
> On Mon, Jan 23, 2017 at 9:12 PM, Robert Haas 
> wrote:
> > According to the documentation for COPY IN mode, "If the COPY command
> > was issued via an extended-query message, the backend will now discard
> > frontend messages until a Sync message is received, then it will issue
> > ReadyForQuery and return to normal processing."  I added a similar
> > note to the documentation for COPY BOTH mode in
> > 91fa8532f4053468acc08534a6aac516ccde47b7, and the documentation
> > accurately describes the behavior of the server.  However, this seems
> > to make fully correct error handling for clients using libpq almost
> > impossible, because PQsendQueryGuts() sends
> > Parse-Bind-Describe-Execute-Sync in one shot without regard to whether
> > the command that was just sent invoked COPY mode (cf. the note in
> > CopyGetData about why we ignore Flush and Sync in that function).
> >
> > So imagine that the client uses libpq to send (via the extended query
> > protocol) a COPY IN command (or some hypothetical command that starts
> > COPY BOTH mode to begin).  If the server throws an error before the
> > Sync message is consumed, it will bounce back to PostgresMain which
> > will set doing_extended_query_message = true after which it will
> > consume messages, find the Sync, reset that flag, and send
> > ReadyForQuery.  On the other hand, if the server enters CopyBoth mode,
> > consumes the Sync message in CopyGetData (or a similar function), and
> > *then* throws an ERROR, the server will wait for a second Sync message
> > from the client before issuing ReadyForQuery.  There is no sensible
> > way of coping with this problem in libpq, because there is no way for
> > the client to know which part of the server code consumed the Sync
> > message that it already sent.  In short, from the client's point of
> > view, if it enters COPY IN or COPY BOTH mode via the extend query
> > protocol, and an error occurs on the server, the server MAY OR MAY NOT
> > expect a further Sync message before issuing ReadyForQuery, and the
> > client has no way of knowing -- except maybe waiting for a while to
> > see what happens.
> >
> > It does not appear to me that there is any good solution to this
> > problem.  Fixing it on the server side would require a wire protocol
> > change - e.g. one kind of Sync message that is used in a
> > Parse-Bind-Describe-Execute-Sync sequence that only terminates
> > non-COPY commands and another kind that is used to signal the end even
> > of COPY.  Fixing it on the client side would require all clients to
> > know prior to initiating an extended-query-protocol sequence whether
> > or not the command was going to initiate COPY, which is an awful API
> > even if didn't constitute an impossible-to-contemplate backward
> > compatibility break.  Perhaps we will have to be content to document
> > the fact that this part of the protocol is depressingly broken...
> >
> > ...unless of course somebody can see something that I'm missing here
> > and the situation isn't as bad as it currently appears to me to be.
>
> Anybody have any thoughts on this?
>
>
> I've been thinking on it a bit, but don't really have anything that can be
> done without a protocol version bump.
>
> We can't really disallow extended query protocol COPY, too much is likely
> to break. And we can't fix it without a protocol change.
>
> A warning in the docs for COPY would be appropriate, noting that clients
> should use the simple query protocol to issue COPY. It's kind of mixing
> layers, since many users won't see the protocol level or have any idea if
> their client driver uses ext or simple query, but we can at least advise
> libpq users.
>
> Also in the protocol docs, noting that clirnfa sending COPY should prefer
> the simple query protocol due to error recovery issues with COPY and
> extended query protocol.
>


Forgive my ignorance, but is this issue related to the Catch-22 I had with
"COPY as a set returning function", wherein a function that invokes
BeginCopyFrom() basically starts a result set, but then ends it to do the
BeginCopyFrom() having NULL (meaning STDIN) as the file, so that when the
results from the copy come back the 'T' record that was going to preface
the 'D' records emitted by the function is now gone?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Tue, Feb 14, 2017 at 4:44 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Corey Huinker <corey.huin...@gmail.com> writes:
> > So moving the conditional stack back into PsqlScanState has some side
> > effects: conditional.[ch] have to move to the fe_utils/ dirs, and now
> > pgbench, which does not use conditionals, would have to link to them. Is
> > that a small price to pay for modularity and easier-to-find code? Or
> should
> > I just tuck it back into psqlscan_int.[ch]?
>
> Pardon me for coming in late, but what in the world has this to do with
> the lexer's state at all?  IOW, I don't think I like either of what you're
> suggesting ...
>
> regards, tom lane
>

Patch v12 has them separated, if that was more to your liking. The stack
state lived in MainLoop() and was passed into HandleSlashCommands with an
extra state variable.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> @in't gonna execute it?
>>
>
> Hmmm... This is too much of an Americanism, IMHO.


The @ looks like a handwritten 'a'.  @in't gonna => ain't gonna => will
not. It's a bad joke, made as a way of saying that I also could not think
of a good mnemonic for '@' or ','.


> I'm here all week, try the veal.
>>
>
> Sorry, syntax error, you have lost me. Some googling suggests a reference
> to post WW2 "lounge entertainers", probably in the USA. I also do not
> understand why this would mean "yes".


It's a thing lounge entertainers said after they told a bad joke.


>   . for z (waiting for the end of the sentence, i.e. endif)
>

+1 ... if we end up displaying the not-true-and-not-evaluated 'z' state.


>   & for t (no real mnemonic)
>
> For two states:
>   * for being executed (beware, it is ***important***)
>

It does lend importance, but that's also the line continuation marker for
"comment". Would that be a problem?


>   / for not (under the hood, and it is opposed to *)
>

+1, I was going to suggest '/' for a false state, with two possible
metaphors to justify it
  1. the slash in a "no" sign ("no smoking", ghostbusters, etc)
  2. the leading char of a c/java/javascript comment (what is written here
is just words, not code)

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 13, 2017 at 3:40 PM, Fabien COELHO  wrote:

>
> Hello Robert,
>
> [...] I think we should try to make this REALLY simple.  We don't really
>> want to have everybody have to change their PROMPT1 and PROMPT2 strings for
>> this one feature.
>>
>
> Ok. I think that we agree that the stack was too much details.
>
> How about just introducing a new value for %R?
>>
>
> Yes. That is indeed one of the idea being discussed.
>
> [...] , or @ if commands are currently being ignored because of the result
>> of an \if test.
>>
>
,-or-@ has one advantage over t/f/z: we cannot infer the 'z' state purely
from pset.active_state, and the if-stack itself is sequestered in
scan_state, which is not visible to the get_prompt() function.

I suppose if somebody wanted it, a separate slash command that does a
verbose printing of the current if-stack would be nice, but mostly just to
explain to people how the if-stack works.


> If I can find some simple mnemonic for "," vs "@" for being executed vs
> ignored, I could live with that, but nothing obvious comes to my mind.
>

@in't gonna execute it?

I'm here all week, try the veal.

To sum up your points: just update %R (ok), keep it simple/short (ok... but
> how simple [2 vs 3 states] and short [1 or 2 chars]), and no real need to
> be too nice with the user beyond the vital (ok, that significantly
> simplifies things).


I'd be fine with either of these on aesthetic grounds. On technical
grounds, 'z' is harder to show.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 13, 2017 at 11:29 AM, Robert Haas  wrote:

> possible states here.  Just when you've figured out what tfzffft


I agree with what you've said, but wanted to point out that any condition
that follows a 'z' would itself be 'z'. Not that tfz is much better.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 13, 2017 at 3:04 PM, Fabien COELHO  wrote:

>
> Hello Corey,
>
> That effort was creating as many headaches as it solved. Rolled back v12
>> except for doc paragraph, added more descriptive tap test names. Enjoy.
>>
>
> The TAP tests are missing from the patch, not sure why...
>

I went back to master and re-applied v11, something must have gotten lost
in translation.


> The stack cleanup is nicer with pop calls.
>

Yeah, dunno why I didn't do that earlier.


> Two debatable suggestions about the example:
>  - put the on_error_stop as the very first thing in the example?
>  - add a comment explaining what the SELECT ... \gset does?


Both are reasonable and easy. Added.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..7e59192 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,81 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+-- stop the script if the condition variables are invalid boolean expressions
+\set ON_ERROR_STOP on
+-- check for the existence of two separate records in the database and store
+-- the results in separate psql variables
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..390bccd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> I wasn't too happy with the extra param, either. Having moved the guts to
>> conditional.c, I'm happy with that change and can move the stack head back
>> to scan_state with a clear conscience.
>>
>
>
That effort was creating as many headaches as it solved. Rolled back v12
except for doc paragraph, added more descriptive tap test names. Enjoy.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..e86b66b 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,79 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+-- set ON_ERROR_STOP in case the variables are not valid boolean expressions
+\set ON_ERROR_STOP on
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..390bccd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool
+read_boolean_expression(PsqlScanState scan_state, char *action,
+   bool *result)
+{
+   char*value = psql_scan_slash_option(scan_state,
+   
OT_NORMAL, NULL, false);
+   

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 13, 2017 at 11:26 AM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> ISTM that it is kind of a regression, because logically this is about the
>> scan state so it should be in the corresponding structure, and having two
>> structures to pass the scan state is not an improvement...
>
>
> I wasn't too happy with the extra param, either. Having moved the guts to
> conditional.c, I'm happy with that change and can move the stack head back
> to scan_state with a clear conscience.
>

So moving the conditional stack back into PsqlScanState has some side
effects: conditional.[ch] have to move to the fe_utils/ dirs, and now
pgbench, which does not use conditionals, would have to link to them. Is
that a small price to pay for modularity and easier-to-find code? Or should
I just tuck it back into psqlscan_int.[ch]?

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> ISTM that it is kind of a regression, because logically this is about the
> scan state so it should be in the corresponding structure, and having two
> structures to pass the scan state is not an improvement...


I wasn't too happy with the extra param, either. Having moved the guts to
conditional.c, I'm happy with that change and can move the stack head back
to scan_state with a clear conscience.


> I would suggest to also apply the advice to the example shown, including a
> comment about why the variable is set on.
>

+1

>
> Also, the last element of the tap tests should be distinct: I suggest to
> use 'if syntax error' and 'elif syntax error' in place of 'syntax error'
> for the two first tests.


+1, shouldn't take too long.

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Maybe this can be a discussed in a follow-up patch and Corey should
> proceed to finalize the if patch?


In the event that we can leave prompting to a later patch, here are the v12
highlights:
- created conditional.h and conditional.c which contain the functions with
stack-ish push/pop/peek/poke names
- now all non-test, non-doc changes are in src/bin/psql
- moved conditional stack out of scan_state, stack state maintained by
mainloop.c/startup.c, passed to HandleSlashCommands
- documentation encourages the user to employ ON_ERROR_STOP when using
conditionals
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..bdef6e9 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,77 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+Expressions that do not properly evaluate to true or false will
+generate an error and cause the \if or
+\elif command to fail.  Because that behavior may
+change branching context in undesirable ways (executing code which
+was intended to be skipped, causing \elif,
+\else, and \endif commands to
+pair with the wrong \if, etc), it is
+recommended that scripts which use conditionals also set
+ON_ERROR_STOP.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..1492b66 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -21,7 +21,7 @@ REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref
 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
 LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils -lpq
 
-OBJS=  command.o common.o help.o input.o stringutils.o mainloop.o copy.o \
+OBJS=  command.o common.o conditional.o help.o input.o stringutils.o 
mainloop.o copy.o \
startup.o prompt.o variables.o large_obj.o describe.o \
crosstabview.o tab-complete.o \
sql_help.o psqlscanslash.o \
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..fded2bc 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -38,6 +38,7 @@
 #include "fe_utils/string_utils.h"
 
 #include "common.h"
+#include "conditional.h"
 #include "copy.h"
 #include "crosstabview.h"
 #include "describe.h"
@@ -62,6 +63,7 @@ typedef enum EditableObjectType
 /* functions for use in this file */
 static backslashResult exec_command(const char *cmd,
 PsqlScanState scan_state,
+ConditionalStack cstack,
 PQExpBuffer 

Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Feb 11, 2017 at 5:57 PM, Greg Stark  wrote:

> On 10 February 2017 at 21:36, Fabien COELHO  wrote:
> >> command   prompt is now
> >> ---   ---
> >> \echo bob ''   = initial state, no branch going on at all
> >> \if yes   't' = inside a true branch
> >> \if no'tf' = false inside a true
> >> \endif't' = back to just the true branch
> >> \if yes   'tt'
> >> \if yes   'ttt'
> >> \if yes   '...ttt' = only show the last 3, but let it be known that
> >> there's at least one more'
> >> \else '...ttz' = past the point of a true bit of this branch
> >
> >
> > I like the "tfz" idea. I'm not sure whether the up to 6 characters is a
> > good, though.
>
>
> I haven't been following this thread but just skimming through it for
> the first time I thought this was more baroque than I was expecting. I
> was expecting something like a { for each level of nested if you're in
> so you can see how many deep you are. I didn't expect to see anything
> more complex than that.
>

So you'd just want to know nesting depth, with no indicator of true/false?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Feb 11, 2017 at 3:48 PM, Fabien COELHO  wrote:

>
> Just realized that '?' means "unknown transactional status" in %x. That
>> might cause confusion if a person had a prompt of %x%R. Is that enough
>> reason to pick a different cue?
>>
>
> Argh.
>
> "\?\.?[tfz]" seems distinctive enough. Note that %R uses "'=-*^!$( and %x
> uses *!?, which means that they already share 2 characters, so adding ?
> does not seem like a big issue if it was not one before.
>
> Otherwise, maybe "&" or "%", but it is less about a condition.


Fair enough, it shouldn't be too confusing then.

The get_prompt() function can see the global pset, obviously, but can't see
the scan_state, where the if-stack currently resides. I could give up on
the notion of a per-file if-stack and just have one in pset, but that might
make life difficult for whomever is brave enough to tackle \while loops. Or
I could give pset a pointer to the current if-stack inside the scan_state,
or I could have pset hold a stack of stacks. Unsure which way would be
best.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Feb 11, 2017 at 2:43 AM, Fabien COELHO  wrote:

>
> Ok, so that's not just PROMPT_READY, that's every prompt...which might be
>> ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
>> level always being '.'?
>>
>
> Yep. The idea is to keep it short, but to still have something to say
> "there are more levels" in the stack, hence the one dot. Basically I just
> compressed your 4 level proposal, and added a separator to deal with the
> preceding stuff and suggest the conditional.
>
> --
> Fabien.
>

Just realized that '?' means "unknown transactional status" in %x. That
might cause confusion if a person had a prompt of %x%R. Is that enough
reason to pick a different cue?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>   calvin=> \if true
>
  calvin?t=> SELECT 1 +
>   calvin?t->   2;
> 3
>   calvin?t=> \if true
>   calvin?t=>   \echo hello
> hello
>   calvin?t=> \endif
>   calvin?t=> \else
>   calvin?z=>   \echo ignored
>   calvin?t=> \endif
>   calvin=>
>

Ok, so that's not just PROMPT_READY, that's every prompt...which might be
ok. ? is a great optional cue, and you're thinking on 2 levels deep, 2nd
level always being '.'? I'll give that a shot.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Shouldn't there be some documentation changes to reflect the behavior on
> errors? A precise paragraph about that would be welcome, IMHO.
>

Oddly enough, the documentation I wrote hadn't addressed invalid booleans,
only the error messages did that.

The new behavior certainly warrants a mention, and I'll add that.


> Given that there is no more barking, then having some prompt indication
> that the code is inside a conditional branch becomes more important, so
> ISTM that there should be some plan to add it.


Yeah, prompting just got more important. I see a few ways to go about this:

1. Add a new prompt type, either %T for true (heh, pun) or %Y for
branching. It would print a string of chained 't' (branch is true), 'f'
(branch is false), 'z' (branch already had its true section). The depth
traversal would have a limit, say 3 levels deep, and if the tree goes more
than that deep, then '...' would be printed in the stead of any deeper
values. So the prompt would change through a  session like:

command   prompt is now
---   ---
\echo bob ''   = initial state, no branch going on at all
\if yes   't' = inside a true branch
\if no'tf' = false inside a true
\endif't' = back to just the true branch
\if yes   'tt'
\if yes   'ttt'
\if yes   '...ttt' = only show the last 3, but let it be known that
there's at least one more'
\else '...ttz' = past the point of a true bit of this branch

2. The printing of #1 could be integrated into %R only in PROMPT_READY
cases, either prepended or appended to the !/=/^, possibly separated by a :
3. Like #2, but prepended/appended in all circumstances
4. Keep %T (or %Y), and reflect the state of pset.active_branch within %R,
a single t/f/z
5. Like #4, but also printing the if-stack depth if > 1

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Feb 9, 2017 at 4:43 PM, Erik Rijkers <e...@xs4all.nl> wrote:

> On 2017-02-09 22:15, Tom Lane wrote:
>
>> Corey Huinker <corey.huin...@gmail.com> writes:
>>
>
> The feature now  ( at patch v10) lets you break off with Ctrl-C anywhere.
> I like it now much more.
>
> The main thing I still dislike somewhat about the patch is the verbose
> output. To be honest I would prefer to just remove /all/ the interactive
> output.
>
> I would vote to just make it remain silent if there is no error.   (and if
> there is an error, issue a message and exit)
>
> thanks,
>
> Erik Rijkers
>

Changes in this patch:
- invalid boolean expression on \if or \elif is treated as if the script
had a bad \command, so it either stops the script (ON_ERROR_STOP, script
mode), or just gives the ParseVariableBool error and continues.

- All interactive "barks" removed except for
"command ignored. use \endif or Ctrl-C to exit current branch" when the
user types a non-branching \command in a false branch
"query ignored. use \endif or Ctrl-C to exit current branch" when the
user types a non-branching \command in a false branch
"\if: escaped" when a user does press Ctrl-C and they escape a branch.

- remaining error messages are tersed:
 \elif: cannot occur after \else
 \elif: no matching \if
 \else: cannot occur after \else
 \else: no matching \if
 \endif: no matching \if
 found EOF before closing \endif(s)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..390bccd 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
  

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Thu, Feb 9, 2017 at 3:13 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > I (still) think this is a bad design.  Even if you've got all the
> > messages just right as things stand today, some new feature that comes
> > along in the future can change things so that they're not right any
> > more, and nobody's going to relish maintaining this.
>
> FWIW, I tend to agree that this is way overboard in terms of the amount of
> complexity going into the messages.  Also, I do not like what seems to
> be happening here:
>
> >> $ psql test < test2.sql -v ON_ERROR_STOP=0
> >> unrecognized value "error" for "\if ": boolean expected
> >> new \if is invalid, ignoring commands until next \endif
>
> IMO, an erroneous backslash command should have no effect, period.
> "It failed but we'll treat it as if it were valid" is a rathole
> I don't want to descend into.  It's particularly bad in interactive
> mode, because the most natural thing to do is correct your spelling
> and issue the command again --- but if psql already decided to do
> something on the strength of the mistaken command, that doesn't work,
> and you'll have to do something or other to unwind the unwanted
> control state before you can get back to what you meant to do.
>
> regards, tom lane
>

One way around this is to make the small change: commands with invalid
expressions are ignored in interactive mode.

Another way around it would be to ignore branching commands in interactive
mode altogether and give a message like "branching commands not supported
in interactive mode". That'd get rid of a lot of complexity right there. I
for one wouldn't miss it. The only use I saw for it was debugging a script,
and in that case the user can be their own branching via selective
copy/paste.

Do either of those sound appealing?

Re: [HACKERS] contrib modules and relkind check

[Corey Huinker]

On Mon, Feb 6, 2017 at 4:01 AM, Amit Langote 
wrote:

> On 2017/01/24 15:35, Amit Langote wrote:
> > On 2017/01/24 15:11, Michael Paquier wrote:
> >> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
> >>  wrote:
> >>> Some contrib functions fail to fail sooner when relations of
> unsupported
> >>> relkinds are passed, resulting in error message like one below:
> >>>
> >>> create table foo (a int);
> >>> create view foov as select * from foo;
> >>> select pg_visibility('foov', 0);
> >>> ERROR:  could not open file "base/13123/16488": No such file or
> directory
> >>>
> >>> Attached patch fixes that for all such functions I could find in
> contrib.
> >>>
> >>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple
> of
> >>> places (in pageinspect and pgstattuple).
> >>
> >> I have spent some time looking at your patch, and did not find any
> >> issues with it, nor did I notice code paths that were not treated or
> >> any other contrib modules sufferring from the same deficiencies that
> >> you may have missed. Nice work.
> >
> > Thanks for the review, Michael!
>
> Added to the next CF, just so someone can decide to pick it up later.
>
> https://commitfest.postgresql.org/13/988/
>
> Thanks,
> Amit
>

Is this still needing a reviewer? If so, here it goes:

Patch applies.

make check-pgstattuple-recurse, check-pg_visibility-recurse,
check-pageinspect-recurse
all pass.

Code is quite clear. It does raise two questions:

1. should have these tests named in core functions, like maybe:

relation_has_visibility(Relation)
relation_has_storage(Relation)
and/or corresponding void functions check_relation_has_visibility() and
check_relation_has_storage() which would raise the appropriate error
message when the boolean test fails.
Then again, this might be overkill since we don't add new relkinds very
often.

2. Is it better stylistically to have an AND-ed list of != tests, or a
negated list of OR-ed equality tests, and should the one style be changed
to conform to the other?

No new regression tests. I think we should create a dummy partitioned table
for each contrib and show that it fails.

Re: [HACKERS] Documentation improvements for partitioning

[Corey Huinker]

On Fri, Feb 3, 2017 at 4:15 AM, Amit Langote 
wrote:

> Here are some patches to improve the documentation about partitioned
> tables:
>
> 0001: Adds some details about partition_bound_spec to the CREATE TABLE
> page, especially:
>
>  - a note about inclusivity of range partition bounds,
>  - a note about the UNBOUNDED literal in case of range partitioning,
>  - a note about the NULL literal in case of list partitioning,
>
> I wonder if the above "note" should be added under the Notes section or
> are they fine to be added as part of the description of PARTITION OF
> clause. Also:
>
>  - in syntax synopsis, it appears now that any expression is OK to be used
>for individual bound datum, but it's not true.  Only literals are
>allowed.  So fixed that.
>  - added an example showing how to create partitions of a range
>partitioned table with multiple columns in the partition key
>  - added PARTITION BY and PARTITION OF (FOR VALUES) as PostgreSQL
>extensions in the compatibility section
>
>
> 0002: Adds details about partitioned tables to the DDL chapter (ddl.sgml)
>
>  - a new section named "Partitioned Tables" right next to the
>Inheritance and Partitioning sections is created.
>  - examples are added to the existing Partitioning section using the new
>partitioned tables.  Old text about implementing table partitioning
>using inheritance is kept, sort of as a still supported older
>alternative.
>
> 0003: Add partitioning keywords to keywords.sgml
>
> This is all I have for now.  Any feedback is greatly appreciated.  Adding
> this to the next CF.
>
> Thanks,
> Amit
>

Patch applies.

Overall this looks really good. It goes a long way towards stating some of
the things I had to learn through experimentation.

I had to read a really long way into the patch before finding a blurb that
I felt wasn't completely clear:

+
+ 
+  INSERT statements with ON CONFLICT
+  clause are currently not allowed on partitioned tables, that is,
+  cause error when specified.
+ 


Here's some other tries at saying the same thing, none of which are
completely satisfying:

...ON CONFLICT clause are currently not allowed on partitioned tables and
will cause an error?
...ON CONFLICT clause are currently not allowed on partitioned tables and
will instead cause an error?
...ON CONFLICT clause will currently cause an error if used on a
partitioned table?

As far as additional issues to cover, this bit:

+ 
+  
+   One cannot drop a NOT NULL constraint on a
+   partition's column, if the constraint is present in the parent
table.
+  
+ 

Maybe we should add something about how one would go about dropping a NOT
NULL constraint (parent first then partitions?)

In reviewing this patch, do all our target formats make word spacing
irrelevant? i.e. is there any point in looking at the number of spaces
after a period, etc?

A final note, because I'm really familiar with partitioning on Postgres and
other databases, documentation which is clear to me might not be to someone
less familiar with partitioning. Maybe we want another reviewer for that?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> It seems that ON_ERROR_STOP is mostly ignored by design when in
> interactive mode, probably because it is nicer not to disconnect the user
> who is actually typing things on a terminal.
>
> """
>   ON_ERROR_STOP
>
> By default, command processing continues after an error. When this
> variable is set to on, processing will instead stop immediately. In
> interactive mode, psql will return to the command prompt; otherwise, psql
> will exit, returning error code 3 to distinguish this case from fatal error
> conditions, which are reported using error code 1.
> """
>

This was my previous understanding of ON_ERROR_STOP. Somewhere in the
course of developing this patch I lost that. Glad to have it back.

The only changes I made were to invalid booleans on if/elif, and the final
branch balancing check won't set status to EXIT_USER unless it's
non-interactive and ON_ERROR_STOP = on.

> \if true
new \if is true, executing commands
> \endif
exited \if, executing commands
> \if false
new \if is false, ignoring commands until next \elif, \else, or \endif
> \endif
exited \if, executing commands
> \if error
unrecognized value "error" for "\if ": boolean expected
new \if is invalid, ignoring commands until next \endif
> \echo foo
inside inactive branch, command ignored.
> ^C
escaped \if, executing commands
> \echo foo
foo
> \endif
encountered un-matched \endif
>
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..63ddf0a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read 

Re: [HACKERS] asynchronous execution

[Corey Huinker]

On Fri, Feb 3, 2017 at 5:04 AM, Antonin Houska  wrote:

> Kyotaro HORIGUCHI  wrote:
>
> > I noticed that this patch is conflicting with 665d1fa (Logical
> > replication) so I rebased this. Only executor/Makefile
> > conflicted.
>
> I was lucky enough to see an infinite loop when using this patch, which I
> fixed by this change:
>
> diff --git a/src/backend/executor/execAsync.c
> b/src/backend/executor/execAsync.c
> new file mode 100644
> index 588ba18..9b87fbd
> *** a/src/backend/executor/execAsync.c
> --- b/src/backend/executor/execAsync.c
> *** ExecAsyncEventWait(EState *estate, long
> *** 364,369 
> --- 364,370 
>
> if ((w->events & WL_LATCH_SET) != 0)
> {
> +   ResetLatch(MyLatch);
> process_latch_set = true;
> continue;
> }



Hi, I've been testing this patch because seemed like it would help a use
case of mine, but can't tell if it's currently working for cases other than
a local parent table that has many child partitions which happen to be
foreign tables. Is it? I was hoping to use it for a case like:

select x, sum(y) from one_remote_table
union all
select x, sum(y) from another_remote_table
union all
select x, sum(y) from a_third_remote_table


but while aggregates do appear to be pushed down, it seems that the remote
tables are being queried in sequence. Am I doing something wrong?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 6, 2017 at 3:43 PM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> I did some more tests. I found a subtlety that I missed before: when
>> running under ON_ERROR_STOP, messages are not fully consistent:
>>
>>  sh> cat test.sql
>>  \set ON_ERROR_STOP on
>>  \if error
>>\echo NO
>>  \endif
>>  \echo NO
>>
>>  sh> ./psql < test.sql
>>  SET
>>  # ok
>>  unrecognized value "error" for "\if ": boolean expected
>>  # ok
>
>
> That's straight from ParseVariableBool, and we can keep that or suppress
> it. Whatever we do, we should do it with the notion that more complex
> expressions will eventually be allowed, but they'll still have to resolve
> to something that's a text boolean.
>
>
>>
>>  new \if is invalid, ignoring commands until next \endif
>>  # hmmm... but it does not, it is really stopping immediately...
>
>  found EOF before closing \endif(s)
>>  # no, it has just stopped before EOF because of the error...
>>
>
> Yeah, chattiness caught up to us here. Both of these messages can be
> suppressed, I think.
>
>
>>
>> Also I'm not quite sure why psql decided that it is in interactive mode
>> above, its stdin is a file, but why not.
>>
>> The issue is made more explicit with -f:
>>
>>  sh> ./psql -f test.sql
>>  SET
>>  psql:test.sql:2: unrecognized value "error" for "\if ": boolean
>> expected
>>  psql:test.sql:2: new \if is invalid, ignoring commands until next \endif
>>  psql:test.sql:2: found EOF before closing \endif(s)
>>
>> It stopped on line 2, which is expected, but it was not on EOF.
>>
>> I think that the message when stopping should be ", stopping as required
>> by ON_ERROR_STOP" or something like that instead of ", ignoring...", and
>> the EOF message should not be printed at all in this case.
>
>
> I agree, and will look into making that happen. Thanks for the test case.
>
>

I suppressed the endif-balance checking in cases where we're in an
already-failed situation.
In cases where there was a boolean parsing failure, and ON_ERROR_STOP is
on, the error message no longer speak of a future which the session does
not have. I could left the ParseVariableBool() message as the only one, but
wasn't sure that that was enough of an error message on its own.
Added the test case to the existing tap tests. Incidentally, the tap tests
aren't presently fooled into thinking they're interactive.

$ cat test2.sql
\if error
\echo NO
\endif
\echo NOPE
$ psql test < test2.sql -v ON_ERROR_STOP=0
unrecognized value "error" for "\if ": boolean expected
new \if is invalid, ignoring commands until next \endif
NOPE
$ psql test < test2.sql -v ON_ERROR_STOP=1
unrecognized value "error" for "\if ": boolean expected
new \if is invalid.
$ psql test -f test2.sql -v ON_ERROR_STOP=0
psql:test2.sql:1: unrecognized value "error" for "\if ": boolean
expected
psql:test2.sql:1: new \if is invalid, ignoring commands until next \endif
NOPE
$ psql test -f test2.sql -v ON_ERROR_STOP=1
psql:test2.sql:1: unrecognized value "error" for "\if ": boolean
expected
psql:test2.sql:1: new \if is invalid.


Revised cumulative patch attached for those playing along at home.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+  

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> I did some more tests. I found a subtlety that I missed before: when
> running under ON_ERROR_STOP, messages are not fully consistent:
>
>  sh> cat test.sql
>  \set ON_ERROR_STOP on
>  \if error
>\echo NO
>  \endif
>  \echo NO
>
>  sh> ./psql < test.sql
>  SET
>  # ok
>  unrecognized value "error" for "\if ": boolean expected
>  # ok


That's straight from ParseVariableBool, and we can keep that or suppress
it. Whatever we do, we should do it with the notion that more complex
expressions will eventually be allowed, but they'll still have to resolve
to something that's a text boolean.


>
>  new \if is invalid, ignoring commands until next \endif
>  # hmmm... but it does not, it is really stopping immediately...

 found EOF before closing \endif(s)
>  # no, it has just stopped before EOF because of the error...
>

Yeah, chattiness caught up to us here. Both of these messages can be
suppressed, I think.


>
> Also I'm not quite sure why psql decided that it is in interactive mode
> above, its stdin is a file, but why not.
>
> The issue is made more explicit with -f:
>
>  sh> ./psql -f test.sql
>  SET
>  psql:test.sql:2: unrecognized value "error" for "\if ": boolean
> expected
>  psql:test.sql:2: new \if is invalid, ignoring commands until next \endif
>  psql:test.sql:2: found EOF before closing \endif(s)
>
> It stopped on line 2, which is expected, but it was not on EOF.
>
> I think that the message when stopping should be ", stopping as required
> by ON_ERROR_STOP" or something like that instead of ", ignoring...", and
> the EOF message should not be printed at all in this case.


I agree, and will look into making that happen. Thanks for the test case.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 6, 2017 at 2:32 PM, Fabien COELHO  wrote:

>
> Do you think the TAP tests would benefit from having the input described
>> in a q(...) block rather than a string?
>>
>
> My 0.02€: Not really, so I would not bother. It breaks perl indentation
> and logic for a limited benefit. Maybe add comments if you feel that a test
> case is unclear.
>
> --
> Fabien.


Consolidated Fabien's TAP test additions with v7, in case anyone else wants
to be reviewing.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..4a3e471 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState scan_state)
return result;
 }
 
+/*
+ * Read and interpret argument as a boolean expression.
+ * Return true if a boolean value was successfully parsed.
+ */
+static bool
+read_boolean_expression(PsqlScanState scan_state, char *action,
+   bool *result)
+{
+   char*value = psql_scan_slash_option(scan_state,
+   
OT_NORMAL, NULL, false);
+   boolsuccess = ParseVariableBool(value, action, result);
+   free(value);
+   return success;
+}
+
+/*
+ * Return true if the command given is a branching command.
+ */
+static bool
+is_branching_command(const char *cmd)
+{
+   return (strcmp(cmd, "if") == 0 || strcmp(cmd, "elif") == 0
+   || strcmp(cmd, "else") == 0 || strcmp(cmd, "endif") == 
0);
+}
 
 /*
  * Subroutine to actually try to execute a backslash 

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Mon, Feb 6, 2017 at 11:21 AM, Corey Huinker <corey.huin...@gmail.com>
wrote:

>
>> Find attached a small patch to improve tap tests, which also checks that
>>> psql really exited by checking that nothing is printed afterwards.
>>>
>>
>>
> Do you think the TAP tests would benefit from having the input described
> in a q(...) block rather than a string?
>
> q(\if false
> \echo a
> \elif invalid
> \echo b
> \endif
> \echo c
> )
>
>
> It's a lot more lines, obviously, but it might make what is being tested
> clearer.
>
>
It occurred to me that the part of this patch most important to casual
users would be the printed messages at various states. I've enumerated
those below, along with the circumstances under which the user would see
them.

The following messages are for interactive and script users. They are also
errors which respect ON_ERROR_STOP.
---

\if statement which had an invalid boolean expression:

new \if is invalid, ignoring commands until next \endif


\elif was in a proper \if block, and not after the true block, but boolean
expression was invalid:

\elif is invalid, ignoring commands until next \endif


\elif statement after an \else

encountered \elif after \else


\elif statement outside of an \if block [*]

encountered un-matched \elif


\else outside of an \if

encountered un-matched \else


\else after an \else

encountered \else after \else


\endif statement outside of an \if block

encountered un-matched \endif


Input file ends with unresolved \if blocks

found EOF before closing \endif(s)


The following are interactive-only non-error informational messages.
-

\if statement which parsed to true:

new \if is true, executing commands


\if statement which parsed to false:

new \if is false, ignoring commands until next \elif, \else, or \endif

\if statement while already in a false/invalid block:

new \if is inside ignored block, ignoring commands until next \endif


\elif statement immediately after the true \if or \elif

\elif is after true block, ignoring commands until next \endif


\elif statement within a false block or subsequent elif after the first
ignored elif

\elif is inside ignored block, ignoring commands until next \endif


\elif was evaluated, was true

\elif is true, executing commands


\elif was evaluated, was false

\elif is false, ignoring commands until next \elif, \else, or \endif


\else statement in an ignored block or after the true block was found:

\else after true condition or in ignored block, ignoring commands until
next \endif


\else statement and all previous blocks were false

\else after all previous conditions false, executing commands


\endif statement ending only \if on the stack

exited \if, executing commands


\endif statement where last block was false but parent block is also false:

exited \\if to false parent branch, ignoring commands until next \endif


\endif statement where last block was true and parent is true

exited \\if to true parent branch, continuing executing commands


\endif statement where last block was false but parent is true

exited \\if to true parent branch, resuming executing commands


Script is currently in a false (or invalid) branch, and user entered a
command other than if/elif/endif:

inside inactive branch, command ignored.


Script currently in a false branch, and user entered a query:

inside inactive branch, query ignored. use \endif to exit current branch.


User in an \if branch and pressed ^C, with no more branches remaining:

escaped \\if, executing commands


User in an \if branch and pressed ^C, but parent branch was false:

escaped \\if to false parent branch, ignoring commands until next \endif


User in a true \if branch and pressed ^C, parent branch true

escaped \\if to true parent branch, continuing executing commands


User in a false \if branch and pressed ^C, parent branch true

escaped \if to true parent branch, resuming executing commands



Notes:


The text for ignored commands vs ignored queries is different.

The text for all the Ctrl-C messages re-uses the \endif messages, but are
"escaped" instead of "exited".

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
>
> Find attached a small patch to improve tap tests, which also checks that
>> psql really exited by checking that nothing is printed afterwards.
>>
>
>
Do you think the TAP tests would benefit from having the input described in
a q(...) block rather than a string?

q(\if false
\echo a
\elif invalid
\echo b
\endif
\echo c
)


It's a lot more lines, obviously, but it might make what is being tested
clearer.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Hmmm. ISTM that control-c must at least reset the stack, otherwise their
> is not easy way to get out. What can be left to another patch is doing a
> control-C for contents and then another one for the stack when there is no
> content.
>

And so it shall be.


> - put Fabien's tap test in place verbatim
>>
>
> Hmmm. That was really just a POC... I would suggest some more tests, eg:
>
>   # elif error
>   "\\if false\n\\elif error\n\\endif\n"
>
>   # ignore commands on error (stdout must be empty)
>   "\\if error\n\\echo NO\n\\else\n\\echo NO\n\\endif\n"
>

Those are already in the regression (around line 2763 of
expected/psql.out), are you saying we should have them in TAP as well?
Should we only do TAP tests?

Anyway, here's the Ctrl-C behavior:

# \if true
new \if is true, executing commands
# \echo msg
msg
# ^C
escaped \if, executing commands
# \if false
new \if is false, ignoring commands until next \elif, \else, or \endif
# \echo msg
inside inactive branch, command ignored.
# ^C
escaped \if, executing commands
# \echo msg
msg
# \endif
encountered un-matched \endif
#


Ctrl-C exits do the same before/after state checks that \endif does, the
lone difference being that it "escaped" the \if rather than "exited" the
\if. Thanks to Daniel for pointing out where it should be handled, because
I wasn't going to figure that out on my own.

v7's only major difference from v6 is the Ctrl-C branch escaping.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..4a3e471 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
/* eat any remaining arguments after a valid command */
/* note we suppress evaluation of backticks here */
@@ -194,6 +195,30 @@ read_connect_arg(PsqlScanState 

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Sat, Feb 4, 2017 at 11:53 AM, Corey Huinker <corey.huin...@gmail.com>
wrote:

> The check I was suggesting on whether Ctrl+C has been pressed
>> on an empty line seems harder to implement, because get_interactive()
>> just calls readline() or fgets(), which block to return when a whole
>> line is ready. AFAICS psql can't know what was the edit-in-progress
>> when these functions are interrupted by a signal instead of
>> returning normally.
>> But I don't think this check is essential, it could be left to another
>> patch.
>>
>
> Glad I wasn't missing something obvious.
> I suppose we could base the behavior on whether there's at least one full
> line already buffered.
> However, I agree that it can be left to another patch.
>

v6 patch. highlights:
- error messages are now a bit more terse, following suggestions
- help text is more terse and Conditionals section was moved below Input
Output
- leverage IFSTATE_NONE a bit to fold some not-in-a-branch cases into
existing switch statements, giving flatter, slightly cleaner code and that
addresses expected cases before exceptional ones
- comments highlight which messages are printed in both interactive and
script mode.
- put Fabien's tap test in place verbatim
- No mention of Ctrl-C or PROMPT. Those can be addressed in separate
patches.

There's probably some more consensus building to do over the interactive
messages and comments, and if interactive-ish tests are possible with TAP,
we should add those too.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index ae58708..c0ba4c4 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2035,6 +2035,67 @@ hello 10
 
   
 
+  
+\if expr
+\elif expr
+\else
+\endif
+
+
+This group of commands implements nestable conditional blocks, like
+this:
+
+
+SELECT
+EXISTS(SELECT 1 FROM customer WHERE customer_id = 123) as is_customer,
+EXISTS(SELECT 1 FROM employee WHERE employee_id = 456) as is_employee
+\gset
+\if :is_customer
+SELECT * FROM customer WHERE customer_id = 123;
+\elif :is_employee
+\echo 'is not a customer but is an employee'
+SELECT * FROM employee WHERE employee_id = 456;
+\else
+\if yes
+\echo 'not a customer or employee'
+\else
+\echo 'this should never print'
+\endif
+\endif
+
+
+Conditional blocks must begin with a \if and end
+with an \endif, and the pairs must be found in
+the same source file. If an EOF is reached on the main file or an
+\include-ed file before all local
+\if-\endif are matched, then
+psql will raise an error.
+
+
+A conditional block can have any number of 
+\elif clauses, which may optionally be followed by a
+single \else clause.
+
+
+The \if and \elif commands
+read the rest of the line and evaluate it as a boolean expression.
+Currently, expressions are limited to a single unquoted string
+which are evaluated like other on/off options, so the valid values
+are any unambiguous case insensitive matches for one of:
+true, false, 
1,
+0, on, off,
+yes, no.  So 
+t, T, and tR
+will all match true.
+
+
+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 proper nesting.
+
+
+  
 
   
 \ir or \include_relative 
filename
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index c53733f..7a418c6 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -61,8 +61,16 @@ uninstall:
 
 clean distclean:
rm -f psql$(X) $(OBJS) lex.backup
+   rm -rf tmp_check
 
 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
rm -f sql_help.h sql_help.c psqlscanslash.c
+
+
+check:
+   $(prove_check)
+
+installcheck:
+   $(prove_installcheck)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index f17f610..9058897 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -49,6 +49,7 @@
 #include "psqlscanslash.h"
 #include "settings.h"
 #include "variables.h"
+#include "fe_utils/psqlscan_int.h"
 
 /*
  * Editable database object types.
@@ -132,7 +133,7 @@ HandleSlashCmds(PsqlScanState scan_state,
status = PSQL_CMD_ERROR;
}
 
-   if (status != PSQL_CMD_ERROR)
+   if (status != PSQL_CMD_ERROR && pset.active_branch)
{
 

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> The check I was suggesting on whether Ctrl+C has been pressed
> on an empty line seems harder to implement, because get_interactive()
> just calls readline() or fgets(), which block to return when a whole
> line is ready. AFAICS psql can't know what was the edit-in-progress
> when these functions are interrupted by a signal instead of
> returning normally.
> But I don't think this check is essential, it could be left to another
> patch.
>

Glad I wasn't missing something obvious.
I suppose we could base the behavior on whether there's at least one full
line already buffered.
However, I agree that it can be left to another patch.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> I noticed that the "barking" is conditional to "success". ISTM that it
> should always "bark" in interactive mode, whether success or not.
>

"success" in those cases means "the expression was a valid boolean", and
non-success cases (should) result in an error being printed regardless of
interactive mode. If you see otherwise, let me know.


>
> While testing it and seeing the code, I agree that it is too
> verbose/redundant. At least remove "active/inactive, ".


Have done so, new patch pending "how-do-I-know-when-input-is-empty" in Ctrl
C.


> - Help text. New block in help text called "Conditionals"
>>
>
> Maybe it could be moved to "Input/Output" renamed as "Input/Output
> Control", or maybe the "Conditionals" section could be moved next to it, it
> seems more logical than after large objects.
>

I put it near the bottom, figuring someone would have a better idea of
where to put it. You did.



> I think that the descriptions are too long. The interactive user can be
> trusted to know what "if/elif/else/endif" mean, or to refer to the full
> documentation otherwise. The point is just to provide a syntax and function
> reminder, not a substitute for the doc. Thus I would suggest shorter
> one-line messages like:
>
>  \if begin a new conditional block
>  \elif   else if in the current conditional block
>  \else else in current conditional block
>  \endifend current conditional block
>

+1



>
>>
> Hmmm. Perl is perl. Attached an attempt at improving that, which is
> probably debatable, but at least it is easy to add further tests without
> massive copy-pasting.


+1 that's a good start.


>
>
> - regression tests now have comments to explain purpose
>>
>
> Ok.
>
> Small details about the code:
>
>   +   if (!pset.active_branch && !is_branching_command(cmd) )
>
> Not sure why there is a space before the last closing parenthesis.


+1

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Feb 3, 2017 at 7:42 PM, Jim Nasby  wrote:

> Since the current consensus is to be very verbose about \if, this is
> obviously a non-issue. Maybe worth adding a 'I' case to %R, but no big deal
> if that doesn't happen.


I think we left the door open to a separate patch for a prompt change.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Feb 3, 2017 at 3:49 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Fri, Feb 3, 2017 at 11:08 AM, Corey Huinker <corey.huin...@gmail.com>
> wrote:
> > I could bulk up the error message on if/elif like such:
> >
> > if: true, executing commands.
> > if: false, ignoring commands until next \else, \elif, or \endif.
> > if: error, ignoring all commands until next \endif
> > else: true, executing commands
> > else: false, ignoring commands until next \endif
> > else: error, ignoring commands until next \endif
> > endif: now executing commands
> > endif: ignoring commands until next [\else, [\elif [, \endif]]
> >
> > Basically, I'd tailor the message to as closely reflect what is possible
> for
> > the user at this moment.
>
> I think that this is kinda hairy.  When I see "endif: now executing
> commands", my reaction is "oh crap, which commands are you
> executing?".  What you really mean is that future command are expected
> to be executed unless things change (for example, due to another \if
> in the meantime), but somebody might have a different interpretation
> of these messages.
>
> I think that the messages you are proposing for "if" and "else" are
> reasonable, but for "endif" I would just say "endif: exiting if" or
> something like that.  If the user doesn't know to what state they are
> returning, c'est la vie.
>

That might be what we end up doing. I'm willing to see how unwieldy it gets
before rolling back to "endif: peace out".

The state logic has stuff to do anyway, so for the moment I've added
psql_error() messages at each endpoint. My current (unsubmitted) work has:

if you were in a true branch and leave it  (i.e yes->yes)

+   psql_error("exited \\if to true
parent branch, \n"
+   "continuing
executing commands\n");

if you were in a false branch beneath a true branch and leave it  (no->yes)

+   psql_error("exited \\if to true
parent branch, \n"
+   "resuming executing
commands\n");

And if you were in a branch that was a child of a false branch (no->no):

+   psql_error("exited \\if to false parent
branch, \n"
+   "ignoring commands until
next \\endif\n");

And the (yes->no) is an impossibility, so no message there.

I'm not too concerned about what wording we finally go with, and as the
coder I realize I'm too close to know the wording that will be most helpful
to an outsider, so I'm very much trusting others to guide me.

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Feb 3, 2017 at 12:20 PM, Daniel Verite 
wrote:

> If we add the functionality that Ctrl+C also exits from branches,
> we could do it like the shell does Ctrl+D for logout, that is it
> logs out only if the input buffer is empty, otherwise it does
> the other functionality bound to this key (normally Delete).
> So if you're in the middle of an edit, the first Ctrl+C will
> cancel the edit and a second one will go back from the /if
>

That does seem to be the consensus desired behavior. I'm just not sure
where to handle that. The var "cancel_pressed" shows up in a lot of places.
Advice?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

>
> Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll
>> start looking into it, as I'm not presently aware of what it is used for.
>>
>
> Not me.
>
> Wikipedia, which holds all the knowledge in the universe, says: "In many
> command-line interface environments, control-C is used to abort the current
> task and regain user control."
>

Well played (again). That one ranks up there with "and don't call me
Shirley." I meant in the specific psql-context, does it do anything other
than (attempt to) terminate sent-but-not-received SQL queries?

Re: \if, \elseif, \else, \endif (was Re: [HACKERS] PSQL commands: \quit_if, \quit_unless)

[Corey Huinker]

On Fri, Feb 3, 2017 at 7:24 AM, Fabien COELHO  wrote:

>
> Hello Erik,
>
> Still, it would be an improvement to be able to break out of an inactive
>> \if-branch with Ctrl-C.
>>
>
> Yes.
>
> '\endif' is too long to type, /and/ you have to know it.
>>
>
> Yep. \if is shorter but has been rejected. Ctrl-C should be the way out.


I could bulk up the error message on if/elif like such:

if: true, executing commands.
if: false, ignoring commands until next \else, \elif, or \endif.
if: error, ignoring all commands until next \endif
else: true, executing commands
else: false, ignoring commands until next \endif
else: error, ignoring commands until next \endif
endif: now executing commands
endif: ignoring commands until next [\else, [\elif [, \endif]]


Basically, I'd tailor the message to as closely reflect what is possible
for the user at this moment.


Can anyone think of a reason why Ctrl-C would be a bad idea? If not I'll
start looking into it, as I'm not presently aware of what it is used for.


2. Inside an \if block \q should be given precedence and cause a direct
>> exit of psql (or at the very least exit the if block(s)), as in regular SQL
>> statements (compare: 'select * from t \q' which will immediately exit psql
>> -- this is good. )
>>
>
> One use case if to be able to write "\if ... \q \endif" in scripts. If \q
> is always executed, then the use case is blown. I cannot think of any
> language that would execute anything in a false branch. So Ctrl-C or Ctrl-D
> is the way out, and \if control must really have precedence over its
> contents.
>
> 3. I think the 'barking' is OK because interactive use is certainly not
>> the first use-case.
>> But nonetheless it could be made a bit more terse without losing its
>> function.
>>
>
> [...] It really is a bit too wordy, [...]
>>
>
> Maybe.
>
> (or alternatively, just  mention 'if: active' or 'elif: inactive', etc.,
>> which has the advantage of being shorter)
>>
>
> This last version is too terse I think. The point is that the user
> understands whether their commands are going to be executed or ignored, and
> "active/inactive" is not very clear.
>
> 5. A real bug, I think:
>> #\if asdasd
>> unrecognized value "asdasd" for "\if ": boolean expected
>> # \q;
>> inside inactive branch, command ignored.
>> #
>>
>> That 'unrecognized value' message is fair enough but it is
>> counterintuitive that after an erroneous opening \if-expression, the
>> if-modus should be entered into. ( and now I have to type \endif again... )
>>
>
> Hmmm.
>
> It should tell that it is in an unclosed if and that it is currently
> ignoring commands, so the "barking" is missing.
>

It does need a bespoke bark.

Also, I do not think that implementing a different behavior for interactive
> is a good idea, because then typing directly and reading a file would
> result in different behaviors, which would not help debugging.
>

+1


>
> So, as Tom suggested (I think), the feature is not designed for
> interactive use, so it does not need to be optimized for that purpose,
> although it should be sane enough.


+1