Re: [HACKERS] Dynamic result sets from procedures

[Daniel Verite]

Peter Eisentraut wrote:

> > CREATE PROCEDURE test()
> > LANGUAGE plpgsql
> > AS $$
> >   RETURN QUERY  EXECUTE 'SELECT 1 AS col1, 2 AS col2';
> > END;
> > $$;
> > 
> > Or is that not possible or not desirable?
> 
> RETURN means the execution ends there, so how would you return multiple
> result sets?

RETURN alone yes, but RETURN QUERY continues the execution, appending
rows to the single result set of the function. In the case of a
procedure, I guess each RETURN QUERY could generate an independant
result set.

> But maybe you don't want to return all those results, so you'd need a
> way to designate which ones, e.g.,
> 
> AS $$
> SELECT set_config('something', 'value');
> SELECT * FROM interesting_table;  -- return only this one
> SELECT set_config('something', 'oldvalue');
> $$;

Yes, in that case, lacking PERFORM in SQL, nothing simple comes to
mind on how to return certain results and not others.
But if it was in an SQL function, it wouldn't return the rows of
"interesting_table" either. I think it would be justified to say to just
use plpgsql for that kind of sequence.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Dynamic result sets from procedures

[Daniel Verite]

Peter Eisentraut wrote:

> CREATE PROCEDURE pdrstest1()
> LANGUAGE SQL
> AS $$
> DECLARE c1 CURSOR WITH RETURN FOR SELECT * FROM cp_test2;
> DECLARE c2 CURSOR WITH RETURN FOR SELECT * FROM cp_test3;
> $$;
> 
> CALL pdrstest1();
> 
> and that returns those two result sets to the client.

If applied to plpgsql, to return a dynamic result, the following
does work:

CREATE PROCEDURE test()
LANGUAGE plpgsql
AS $$
DECLARE
 query text:= 'SELECT 1 AS col1, 2 AS col2';
BEGIN
 EXECUTE 'DECLARE c CURSOR WITH RETURN FOR ' || query;
END;
$$;

This method could be used, for instance, to build a pivot with dynamic
columns in a single client-server round-trip, which is not possible today
with the query-calling-functions interface.
More generally, I guess this should help in the whole class of situations
where the client needs polymorphic results, which is awesome.

But instead of having procedures not return anything,
couldn't they return whatever resultset(s) they want to
("no resultset" being just a particular case of "anything"),
so that we could leave out cursors and simply write:

CREATE PROCEDURE test()
LANGUAGE plpgsql
AS $$
  RETURN QUERY  EXECUTE 'SELECT 1 AS col1, 2 AS col2';
END;
$$;

Or is that not possible or not desirable?

Similarly, for the SQL language, I wonder if the above example
could be simplified to:

CREATE PROCEDURE pdrstest1()
LANGUAGE SQL
AS $$
 SELECT * FROM cp_test2;
 SELECT * FROM cp_test3;
$$;
by which the two result sets would go back to the client again
without declaring explicit cursors.
Currently, it does not error out and no result set is sent.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] psql: new help related to variables are not too readable

[Daniel Verite]

Tomas Vondra wrote:

> That's not what I meant. I've never felt a strong urge to avoid wrapping
> at 80 chars when translating strings - simply translate in the most
> meaningful way, if it gets longer than 80 chars and can't be easily
> shortened, just wrap. If there are 60 or 80 characters does not change
> this much - 80 chars may allow more unwrapped strings, of course, but
> it's a minor change for the translators. Or at least me, others may
> disagree, of course.

The difficulty varies across languages since some are more compact
than others, and the choice of wrapping or not is not
consistent across translations.

As an example, in the previous version, the first variable comes
out as:

en
  AUTOCOMMIT if set, successful SQL commands are automatically
committed

de
  AUTOCOMMIT wenn gesetzt werden alle erfolgreichen SQL-Befehle
 automatisch committet
es
  AUTOCOMMIT si está definida, órdenes SQL exitosas se comprometen
 automáticamente
fr
  AUTOCOMMIT si activé, les commandes SQL réussies sont
automatiquement validées

he
  AUTOCOMMITאם הופעל, פקודות SQL מחויבות באופן אוטומטי

it
  AUTOCOMMIT se impostato, i comandi SQL riusciti sono salvati
automaticamente

ko
  AUTOCOMMIT 지정 하면 SQL 명령이 성공하면 자동으로 커밋

pl
  AUTOCOMMIT gdy ustawiony, polecenia SQL zakończone powodzeniem są
automatycznie zatwierdzone

pt_BR
  AUTOCOMMIT se definido, comandos SQL bem sucedidos são
automaticamente efetivados

ru
  AUTOCOMMIT если установлен, успешные SQL-команды фиксируются
автоматически

sv
  AUTOCOMMIT om satt, efterföljande SQL-kommandon commit:as
automatiskt

zh_CN
  AUTOCOMMIT 如果被设置，成功的SQL命令将会被自动提交

The original line in english has exactly 80 characters.
When translated in other latin languages, it tends to be a bit
longer.

German and spanish translators have taken the trouble
to break the message into two lines of less than
80 chars with the second one nicely indented, also
aligned in the .po file:

msgid "  AUTOCOMMIT if set, successful SQL commands are automatically
committed\n"
msgstr ""
"  AUTOCOMMIT si está definida, órdenes SQL exitosas se
comprometen\n"
" automáticamente\n"


But other translations are not necessarily done this way, or
at least not consistently. If only for that, having more
characters in the description before screen wrap should help
a bit. In the above example, I think with the new presentation
only the polish version wouldn't fit in 80 chars without
wrapping.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Tom Lane wrote:

> Since we've blown past 80 columns on some of the other
> output, maybe that doesn't matter.  Or maybe we should shorten this
> variable name so it doesn't force reformatting of all this text.

The two-space left margin on the entire block does not add that
much to readability, IMV, so maybe we could reclaim these
two characters.

Another idea: since there are already several variables for which
the text + spacing + presentation don't fit anyway, 
we could forget about the tabular presentation and grow
vertically.

That would look like the following, for example, with a 3-space margin
for the description:

AUTOCOMMIT
   If set, successful SQL commands are automatically committed
COMP_KEYWORD_CASE
   Determines the case used to complete SQL key words
   [lower, upper, preserve-lower, preserve-upper]
DBNAME
   The currently connected database name
ECHO
   Controls what input is written to standard output
   [all, errors, none, queries]
ECHO_HIDDEN
   If set, display internal queries executed by backslash commands;
   if set to "noexec", just show without execution
ENCODING
   Current client character set encoding
FETCH_COUNT
   The number of result rows to fetch and display at a time
   (default: 0=unlimited)
etc..

To me that looks like a good trade-off: it eases the size constraints
for both the description and the name of the variable, at the cost
of consuming one more line per variable, but that's why the pager
is for.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Andres Freund wrote:

> > One option may be to leave that decision to the user by providing a
> > PQBatchAutoFlush(true|false) property, along with a PQBatchFlush()
> > function.
> 
> What'd be the difference between PQflush() and PQbatchFlush()?

I guess no difference, I was just not seeing that libpq already provides
this functionality...

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Andres Freund wrote:

-   if (pqFlush(conn) < 0)
-   goto sendFailed;
+   if (conn->batch_status == PQBATCH_MODE_OFF)
+   {
+   /*
+* Give the data a push.  In nonblock mode, don't complain if
we're unable
+* to send it all; PQgetResult() will do any additional
flushing needed.
+*/
+   if (pqFlush(conn) < 0)
+   goto sendFailed;
+   }

Seems to be responsible for roughly an 1.7x speedup in tps and equivalent
decrease in latency, based on the "progress" info.
I wonder how much of that corresponds to a decrease in the number of
packets versus the number of syscalls. Both matter, I guess.

But OTOH there are certainly batch workloads where it will be preferrable
for the first query to reach the server ASAP, rather than waiting to be
coalesced with the next ones.
libpq is not going to know what's best.
One option may be to leave that decision to the user by providing a
PQBatchAutoFlush(true|false) property, along with a PQBatchFlush()
function. Maybe we could even let the user set the size of the sending
buffer, so those who really want to squeeze performance may tune it
for their network and workload.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Craig Ringer wrote:

> The kernel will usually do some packet aggregation unless we use
> TCP_NODELAY (which we don't and shouldn't)

Not sure. As a point of comparison, Oracle has it as a tunable
parameter (TCP.NODELAY), and they changed its default from
No to Yes starting from their 10g R2.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Andres Freund wrote:

> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.

Here's an updated version of the patch I made during review,
adding \beginbatch and \endbatch to pgbench.
The performance improvement appears clearly
with a custom script of this kind:
  \beginbatch
 UPDATE pgbench_branches SET bbalance = bbalance + 1 WHERE bid = 0;
 ..above repeated 1000 times...
  \endbatch

versus the same with a BEGIN; END; pair instead of \beginbatch \endbatch

On localhost on my desktop I tend to see a 30% difference in favor
of the batch mode with that kind of test.
On slower networks there are much bigger differences.

The latest main patch (v10) must also be slightly updated for HEAD,
because of this:
error: patch failed: src/interfaces/libpq/exports.txt:171
v11 attached without any other change.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index c3bd4f9..366f278 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -4656,6 +4656,526 @@ int PQflush(PGconn *conn);
 
  
 
+ 
+  Batch mode and query pipelining
+
+  
+   libpq
+   batch mode
+  
+
+  
+   libpq
+   pipelining
+  
+
+  
+   libpq supports queueing up queries into
+   a pipeline to be executed as a batch on the server. Batching queries allows
+   applications to avoid a client/server round-trip after each query to get
+   the results before issuing the next query.
+  
+
+  
+   An example of batch use may be found in the source distribution in
+   src/test/modules/test_libpq/testlibpqbatch.c.
+  
+
+  
+   When to use batching
+
+   
+Much like asynchronous query mode, there is no performance disadvantage to
+using batching and pipelining. It increases client application complexity
+and extra caution is required to prevent client/server deadlocks but
+can sometimes offer considerable performance improvements.
+   
+
+   
+Batching is most useful when the server is distant, i.e. network latency
+(ping time) is high, and when many small operations are 
being performed in
+rapid sequence. There is usually less benefit in using batches when each
+query takes many multiples of the client/server round-trip time to execute.
+A 100-statement operation run on a server 300ms round-trip-time away would 
take
+30 seconds in network latency alone without batching; with batching it may 
spend
+as little as 0.3s waiting for results from the server.
+   
+
+   
+Use batches when your application does lots of small
+INSERT, UPDATE and
+DELETE operations that can't easily be transformed into
+operations on sets or into a
+COPY operation.
+   
+
+   
+Batching is less useful when information from one operation is required by 
the
+client before it knows enough to send the next operation. The client must
+introduce a synchronisation point and wait for a full client/server
+round-trip to get the results it needs. However, it's often possible to
+adjust the client design to exchange the required information server-side.
+Read-modify-write cycles are especially good candidates; for example:
+
+ BEGIN;
+ SELECT x FROM mytable WHERE id = 42 FOR UPDATE;
+ -- result: x=2
+ -- client adds 1 to x:
+ UPDATE mytable SET x = 3 WHERE id = 42;
+ COMMIT;
+
+could be much more efficiently done with:
+
+ UPDATE mytable SET x = x + 1 WHERE id = 42;
+
+   
+
+   
+
+ The batch API was introduced in PostgreSQL 10.0, but clients using 
PostgresSQL 10.0 version of libpq can
+ use batches on server versions 8.4 and newer. Batching works on any server
+ that supports the v3 extended query protocol.
+
+   
+
+  
+
+  
+   Using batch mode
+
+   
+To issue batches the application must switch
+a connection into batch mode. Enter batch mode with PQenterBatchMode(conn)
 or test
+whether batch mode is active with PQbatchStatus(conn). 
In batch mode only asynchronous operations are permitted, and
+COPY is not recommended as it most likely will trigger 
failure in batch processing. 
+Using any synchronous command execution functions such as 
PQfn,
+PQexec or one of its sibling functions are error 
conditions.
+Functions allowed in batch mode are described in . 
+   
+
+   
+The client uses libpq's asynchronous query functions to dispatch work,
+marking the end of each batch with PQbatchSyncQueue.
+And to get results, it uses PQgetResult and
+PQbatchProcessQueue. It may eventually exit
+batch mode with PQexitBatchMode once all results are
+processed.
+   
+
+   
+
+ It is best to use batch mode with libpq in
+ non-blocking mode. If used 
in
+ blocking mode it is possible for a client/server deadlock to occur. The
+ client will block trying 

Re: [HACKERS] Disallowing multiple queries per PQexec()

[Daniel Verite]

Andres Freund wrote:

> Since it's an application writer's choice whether to use it,
> it seems to make not that much sense to have a
> serverside guc - it can't really be sensible set.

The application writers who are concerned by this wouldn't
know that they have a choice. If there were informed, 
supposedly they would grok the SQL syntax to begin with,
understanding the necessity and workings of proper quoting, and
the problem would not exist.

What is proposed AFAIU is an optional policy to be set on already
developed client apps, not a setting that is meant to be played with
by them.

An analogy I can find in existing GUCs, and that incidentally is
actually relevant to me as an app writer, is lo_compat_privileges
It's SUSET, it's not GUC_REPORT. Either it's on and the app
is not subject to permission checking for large objects,
or it's off and it is subject to them.
It's something that is relevant at deployment time, and not really besides
that, and it's the DBA's problem to set the policy depending on the app
requirements and the security requirements, rather than the app's problem
to adjust to whatever value there is in there.
As an example of app requirement, if the app has to let a user create a
large object and a different user to delete it, this GUC must be on,
otherwise such a scenario is not allowed, as unlinking is not a grantable
privilege, just like drop table.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Disallowing multiple queries per PQexec()

[Daniel Verite]

Fabien COELHO wrote:

> I'm not fully convinced by this feature: using multiple queries is a 
> useful trick to reduce network-related latency by combining several 
> queries in one packet. Devs and even ORMs could use this trick.

It's proposed as an option. For apps that intentionally put multiple
commands in single PQexec calls, or for apps for which the deployer doesn't
know or care whether they do that, the setting should be kept to its default
value that let such calls pass, as they pass today.

In my understanding of the proposal, there is no implication that
intentionally using such multiple commands is bad, or that
it should be obsoleted in the future.

It's only bad in the specific case when this possibility is not used
normally by the app, but it's available to an attacker to make an
attack both easier to mount and more potent than a single-query injection.
This reasoning is also based on the assumption that the app has
bugs concerning quoting of parameters, but it's a longstanding class of
bugs that plague tons of low-quality code in production, and it shows no
sign of going away.

> Many SQL injection attacks focus on retrieving sensitive data, 
> in which case "' UNION SELECT ... --" would still work nicely. Should we 
> allow to forbid UNION? And/or disallow comments as well, which are 
> unlikely to be used from app code, and would make this harder? If pg is to 
> provide SQL injection guards by removing some features on some 
> connections, maybe it could be more complete about it.

It looks more like the "training wheel" patch that
was discussed  in https://commitfest.postgresql.org/13/948/
rather than something that should be in this patch.
This is a different direction because allowing or disallowing
compound statements is a clear-cut thing, whereas making
a list of SQL constructs to cripple to mitigate bugs is more like opening
a Pandora box.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Disallowing multiple queries per PQexec()

[Daniel Verite]

Tom Lane wrote:

> Bearing in mind that I'm not really for this at all...

It's a band-aid, but certainly there are cases
where a DBA confronted to a badly written website
would just want to be able to:
  ALTER USER webuser SET allow_multiple_queries TO off;

> But if an attacker is able to inject a SET command,
> he's already found a way around it.  So there's no real
> point in locking down the GUC to prevent that.

I can think of the following case, where given the SQL-injectable
   select id from users where email='$email';
an attacker would submit this string in $email:
  foo' AND set_config('allow_multiple_queries', 'on', false)='on
which opens the rest of the session for a second injection, this
time in the form of several colon-separated commands that
would do the actual damage.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Disallowing multiple queries per PQexec()

[Daniel Verite]

Surafel Temesgen wrote:

> I modified the patch as such and added to commitfest 2017-07.

A couple comments:

+   {"disallow_multiple_queries", PGC_POSTMASTER,
CLIENT_CONN_OTHER,
+   gettext_noop("Disallow multiple queries per query
string."),
+   NULL
+   },

PGC_POSTMASTER implies that it's an instance-wide setting.
Is is intentional? I can understand that it's more secure for this not to
be changeable in an existing session, but it's also much less usable if you
can't set it per-database and per-user.
Maybe it should be PGC_SUSET ?


+   if ((strcmp(commandTagHead, "BEGIN") != 0) ||
(strcmp(commandTagTail, "COMMIT") != 0) )
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
errmsg("cannot execute multiple commands unless it is a transaction
block")));

Shouldn't ROLLBACK be considered too as ending a transaction block?
Also, can it use a more specific code than ERRCODE_SYNTAX_ERROR?
It feels more like a rule violation than a syntax error.



Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] export import bytea from psql

[Daniel Verite]

Pavel Stehule wrote:

> It is similar to my first or second proposal - rejected by Tom :(

Doesn't it differ? ISTM that in the patches/discussion related to:
https://commitfest.postgresql.org/11/787/
it was proposed to change \set in one way or another,
and also in the point #1 of this present thread:

> > 1. using psql variables - we just need to write commands \setfrom
> > \setfrombf - this should be very simple implementation. The value can be
> > used more times. On second hand - the loaded variable can hold lot of
> > memory (and it can be invisible for user).


My comment is: let's not change \set or even create a variant
of it just for importing verbatim file contents, in text or binary,
because interpolating psql variables differently in the query itself
is all we need.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] export import bytea from psql

[Daniel Verite]

Pavel Stehule wrote:

> 1. using psql variables - we just need to write commands \setfrom
> \setfrombf - this should be very simple implementation. The value can be
> used more times. On second hand - the loaded variable can hold lot of
> memory (and it can be invisible for user).

This could be simplified by using the variable only for the filename,
and then injecting the contents of the file into the PQexec'd query
as the interpolation of the variable.
We already have:
 :var for verbatim injection
 :'var' for injection quoted-as-text
 :"var" for injection quoted-as-identifier

What if we add new forms of dereferencing, for instance
(not necessarily with this exact syntax):
 : for injecting the quoted-as-text contents of the file pointed
  to by var.
 :{var} same thing but with file contents quoted as binary
 (PQescapeByteaConn)

then we could write:

\set img '/path/to/image.png'
insert into image(binary) values(:{img});

We could also go further  in that direction. More new interpolation
syntax could express that a variable is to be passed as a
parameter rather than injected (assuming a parametrized query),
and whether the value is directly the contents or it's a filename
pointing to the contents, and whether its format is binary or text,
and even support an optional oid or typename coupled to
the variable.
That would be a lot of new syntax for interpolation but no new
backslash command and no change to \set itself.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PG 10 release notes

[Daniel Verite]

Fabien COELHO wrote:

>Fix psql \p to always print what would be executed by \g or \w (Daniel
>Vérité)
> 
>Previously \p didn't properly print the reverted-to command after a
>buffer contents reset. CLARIFY?
> 
> The fix is linked to the change introduced by Tom when 
> refactoring/cleaning up in e984ef586 (\if) which change psql's \p 
> behavior.

That's my recollection as well. The "Previously" does not refer to 9.6,
but to that commit.

> I'm not sure how this should appear in the release notes. Maybe not at 
> all, associated to the feature in which the behavioral change was 
> introduced...

There is small change of behavior coming as a by-product of the
introduction of  /if.../endif blocks.

When doing in 9.x:

select '1st buffer' \g
followed by \e
and editing with select '2nd buffer' (without ending the query)
and then back in psql doing '\r' and '\p', the result is
select '2nd buffer'

The same with v10 leads instead to
select '1st buffer'

I'm not sure whether it's above the level of detail worth being mentioned
in the release notes.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Fabien COELHO wrote:

> Pavel also suggested some support for TEXT, although I would like to see a 
> use case. That could be another extension to the engine.

SQLSTATE is text.

Also when issuing "psql -v someoption=value -f script", it's reasonable
to want to compare :someoptionvar to 'somevalue' in the script.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Fabien COELHO wrote:

> Now it could be decided that \set in psql stays simplistic because it is 
> not needed as much as it is with pgbench. Fine with me.

It's not just that. It's that currently, if we do in psql:

\set d sqrt(1 + random(1000) * 17)

then we get that:

\echo :d
sqrt(1+random(1000)*17)

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.

Certainly if we want the same sort of evaluator in pgbench and psql
we'd better share the code between them, but I don't think it will be
exposed by the same backslash commands in both programs,
if only for that backward compatibility concern.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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)

[Daniel Verite]

  Hi,

In interactive mode, the warning in untaken branches is misleading
when \endif is on the same line as the commands that
are skipped. For instance:

  postgres=# \if false \echo NOK \endif
  \echo command ignored; use \endif or Ctrl-C to exit current \if block
  postgres=# 

From the point of view of the user, the execution flow has exited
the branch already when this warning is displayed.
Of course issuing the recommended \endif at this point doesn't work:

  postgres=# \endif
  \endif: no matching \if

Maybe that part of the message: 
"use \endif or Ctrl-C to exit current \if block"
should be displayed only when coming back at the prompt,
and if the flow is still in an untaken branch at this point?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Tom Lane wrote:

> I really dislike this syntax proposal.  It would get us into having
> to count nested brackets, and distinguish quoted from unquoted brackets,
> and so on ... and for what?  It's not really better than
> 
>   \if sql select 1 from pg_catalog.pg_extension where extname='pgcrypto'
> 
> (ie, "\if sql ...text to send to server...").

That's fine by me. The advantage of enclosing the query is to leave
open the possibility of accepting additional contents after the query,
like options (as \copy does), or other expression terms to combine
with the query's result. But we can live without that.

> If you're worried about shaving keystrokes, make the "select" be implicit.
> That would be particularly convenient for the common case where you're
> just trying to evaluate a boolean expression with no table reference.

These expressions look more natural without the SELECT keyword,
besides the size, but OTOH "\if sql 1 from table where expr"
looks awkward. Given an implicit select, I would prefer
"\if exists (select 1 from table where expr)" but now it's not shorter.

An advantage of prepending the SELECT automatically, is that it
would prevent people from abusing this syntax by putting
update/insert/delete or even DDL in there, imagining that this would
be a success/failure test for these operations.
Having these fail to execute in the first place, when called by \if,
seems like a sane failure mode that we would gain incidentally.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Fabien COELHO wrote:

> My 0.02 € about server-side expressions: ISTM that there is nothing 
> obvious/easy to do to include these:
> 
>   - how would it work, both with \set ... and \if ...?

The idea is that the need to have two command (a SELECT .. \gset
followed by an \if) and a temporary variable in-between would be
lifted by implementing a close equivalent in one command.
It would behave essentially the same as the two commands.

I don't see that \set must have to be involved in that improvement,
although it could be indirectly, depending on what exactly we
implement.

\set differs in that it already exists in released versions,
so we have the backward compatibility to consider.
With \if we are not bound by that, but what \if will be at feature
freeze needs to be as convenient as we can make it in this release,
and not block progress in v11 and later, as these future improvements
will have to be backward-compatible against v10.


>   - should it be just simple expressions or may it allow complex
> queries?

Let's imagine that psql would support a syntax like this:
  \if [select current_setting('server_version_num')::int < 11]
or
  \if [select 1 from pg_catalog.pg_extension where extname='pgcrypto']

where by convention [ and ] enclose an SQL query that's assumed to
return a single-row, single-column bool-ish value, and in which
psql variables would be expanded, like they are now in
backtick expressions.
Queries can be as complex as necessary, they just have to fit in one line.

>   - how would error detection and handling work from a script?

The same as SELECT..\gset followed by \if, when the SELECT fails.

>   - should it have some kind of continuation, as expressions are
> likely to be longer than a constant?

No, to me that falls into the issue of continuation of backslash
commands in general, which is discussed separately.

>   - how would they interact with possible client-side expressions?

In no way at all,in the sense that, either you choose to use an SQL
evaluator, or a client-side evaluator (if it exists), or a backtick
expression.
They are mutually exclusive for a single \if invocation.

Client-side evaluation would have to be called with a syntax
that is unambiguously different. For example it could be
\if (:SQLSTATE = '23505')
  \echo A record with the unique key :key_id already exists
  rollback
\endif

AFAIK we don't have :SQLSTATE yet, but we should :)

Maybe the parentheses are not required, or we could require a different set
of brackets to enclose an expression to evaluate internally, like {}, or
whatever provided it's not ambiguous.

> (on this point, I think that client-side is NOT needed for "psql".
>  It makes sense for "pgbench" in a benchmarking context where the
>  client must interact with the server in some special meaningful
>  way, but for simple scripting the performance requirement and
>  logic is not the same, so server-side could be enough).

Client-side evaluation is useful for the example above, where
you expect that you might be in a failed transaction, or even
not connected, and you need to do quite simple tests.
We can do that already with backtick expansion and a shell evaluation
command, but it's a bit heavy/inelegant and creates a dependency on
external commands that is detrimental to portability.
I agree that we don't need a full-featured built-in evaluator, because
the cases where it's needed will be rare enough that it's reasonable
to have to defer to an external evaluator in those cases.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Suggested fix for \p and \r in psql

[Daniel Verite]

Tom Lane wrote:

> If we do phrase it like that, I think that the most natural behavior
> for \r is the way I have it in HEAD --- you'd expect it to clear
> the query buffer, but you would not expect it to forget the most
> recent command.

Okay, leaving out \r as it is and changing only \p works for me.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Suggested fix for \p and \r in psql

[Daniel Verite]

Tom Lane wrote:

> > 1. \p ignores the "previous buffer". Example:
> 
> Yeah, I did that intentionally, thinking that the old behavior was
> confusing.  We can certainly discuss it though.  I'd tend to agree
> with your point that \p and \w should print the same thing, but
> maybe neither of them should look at the previous_buf.
> 
> > 2. \r keeps the "previous buffer". I think it should clear it.
> 
> I don't really agree with this.  The fact that it used to clear both
> buffers was an implementation accident that probably nobody had even
> understood clearly.  ISTM that loses functionality because you can't
> do \g anymore.

I don't have a strong opinion on \r. As a user I tend to use Ctrl+C
rather than \r for the edit in progress.
The documentation over-simplifies things as if there
was only one query buffer, instead of two of them.

  \r or \reset
  Resets (clears) the query buffer. 

From just that reading, the behavior doesn't seem right when
we "clear the query buffer", and then print it, and it doesn't
come out as empty.

About \p alone, if it doesn't output what \g is about to run, I
think that's a problem because ISTM that it's the main use
case of \p

Following the chain of consistency, the starting point seems to be
that \g sends the previous query if the edit-in-progress input
buffer is empty, instead of saying, empty buffer = empty query,
so nothing to send.
Presumably the usage of issuing  \g alone to repeat the last
query is well established, so we can't change it and need
to adjust the other commands to be as less surprising
as possible with our two buffers.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Variable substitution in psql backtick expansion

[Daniel Verite]

Fabien COELHO wrote:

> Note that this is already available indirectly, as show in the 
> documentation.
> 
>   SELECT some-boolean-expression AS okay \gset
>   \if :okay
> \echo boolean expression was true
>   \else
> \echo boolean expression was false
>   \endif

Yes, the question was whether we leave it as that for v10,
or if it's worth a last-minute improvement for usability,
assuming it's doable, similarly to what $subject does to backtick
expansion for external evaluation.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

[HACKERS] Suggested fix for \p and \r in psql

[Daniel Verite]

  Hi,

I've noticed two issues with the query buffer post-commit e984ef5
(Support \if ... \elif ... \else ... \endif in psql scripting):

1. \p ignores the "previous buffer". Example:

postgres=# select 1;
 ?column? 
--
1
(1 row)

postgres=# \p
Query buffer is empty.

That doesn't match the pre-commit behavior, and is not
consistent with \e or \w


2. \r keeps the "previous buffer". I think it should clear it. Example:

postgres=# select 1;
 ?column? 
--
1
(1 row)

postgres=# select 2 \r
Query buffer reset (cleared).
postgres=# \w /tmp/buffer
postgres=# \! cat /tmp/buffer
select 1;

I suggest the attached fix, with a few new regression tests.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 94a3cfc..6554268 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -106,14 +106,14 @@ static backslashResult exec_command_lo(PsqlScanState scan_state, bool active_bra
 const char *cmd);
 static backslashResult exec_command_out(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_print(PsqlScanState scan_state, bool active_branch,
-   PQExpBuffer query_buf);
+   PQExpBuffer query_buf, PQExpBuffer previous_buf);
 static backslashResult exec_command_password(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_prompt(PsqlScanState scan_state, bool active_branch,
 	const char *cmd);
 static backslashResult exec_command_pset(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_quit(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_reset(PsqlScanState scan_state, bool active_branch,
-   PQExpBuffer query_buf);
+   PQExpBuffer query_buf, PQExpBuffer previous_buf);
 static backslashResult exec_command_s(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_set(PsqlScanState scan_state, bool active_branch);
 static backslashResult exec_command_setenv(PsqlScanState scan_state, bool active_branch,
@@ -192,8 +192,8 @@ static void checkWin32Codepage(void);
  * execution of the backslash command (for example, \r clears it).
  *
  * previous_buf contains the query most recently sent to the server
- * (empty if none yet).  This should not be modified here, but some
- * commands copy its content into query_buf.
+ * (empty if none yet).  This should not be modified here (except by
+ * \r), but some commands copy its content into query_buf.
  *
  * query_buf and previous_buf will be NULL when executing a "-c"
  * command-line option.
@@ -362,7 +362,8 @@ exec_command(const char *cmd,
 	else if (strcmp(cmd, "o") == 0 || strcmp(cmd, "out") == 0)
 		status = exec_command_out(scan_state, active_branch);
 	else if (strcmp(cmd, "p") == 0 || strcmp(cmd, "print") == 0)
-		status = exec_command_print(scan_state, active_branch, query_buf);
+		status = exec_command_print(scan_state, active_branch,
+	query_buf, previous_buf);
 	else if (strcmp(cmd, "password") == 0)
 		status = exec_command_password(scan_state, active_branch);
 	else if (strcmp(cmd, "prompt") == 0)
@@ -372,7 +373,8 @@ exec_command(const char *cmd,
 	else if (strcmp(cmd, "q") == 0 || strcmp(cmd, "quit") == 0)
 		status = exec_command_quit(scan_state, active_branch);
 	else if (strcmp(cmd, "r") == 0 || strcmp(cmd, "reset") == 0)
-		status = exec_command_reset(scan_state, active_branch, query_buf);
+		status = exec_command_reset(scan_state, active_branch,
+	query_buf, previous_buf);
 	else if (strcmp(cmd, "s") == 0)
 		status = exec_command_s(scan_state, active_branch);
 	else if (strcmp(cmd, "set") == 0)
@@ -1827,12 +1829,15 @@ exec_command_out(PsqlScanState scan_state, bool active_branch)
  */
 static backslashResult
 exec_command_print(PsqlScanState scan_state, bool active_branch,
-   PQExpBuffer query_buf)
+   PQExpBuffer query_buf, PQExpBuffer previous_buf)
 {
 	if (active_branch)
 	{
 		if (query_buf && query_buf->len > 0)
 			puts(query_buf->data);
+		/* Applies to previous query if current buffer is empty */
+		else if (previous_buf && previous_buf->len > 0)
+			puts(previous_buf->data);
 		else if (!pset.quiet)
 			puts(_("Query buffer is empty."));
 		fflush(stdout);
@@ -2056,10 +2061,11 @@ exec_command_quit(PsqlScanState scan_state, bool active_branch)
  */
 static backslashResult
 exec_command_reset(PsqlScanState scan_state, bool active_branch,
-   PQExpBuffer query_buf)
+   PQExpBuffer query_buf, PQExpBuffer previous_buf)
 {
 	if (active_branch)
 	{
+		resetPQExpBuffer(previous_buf);
 		resetPQExpBuffer(query_buf);
 		psql_scan_reset(scan_state);
 		if (!pset.quiet)
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8aa914f..f1c516a 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2932,3 +2932,30 @@ 

Re: [HACKERS] Variable substitution in psql backtick expansion

[Daniel Verite]

Tom Lane wrote:

> Thoughts?

ISTM that expr is too painful to use to be seen as the
idiomatic way of achieving comparison in psql.

Among its disadvantages, it won't work on windows, and its
interface is hard to work with due to the necessary
quoting of half its operators, and the mandatory spacing
between arguments.

Also the quoting rules and command line syntax
depend on the underlying shell.
Isn't it going to be tricky to produce code that works
across different families of shells, like bourne and csh?

I think that users would rather have the option to just put
an SQL expression behind \if. That implies a working connection
to evaluate, which expr doesn't, but that's no
different from the other backslash commands that read
the database.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> Hmm, With batch mode, after sending COPY command to server(and server
> started processing the query and goes into COPY state) , client does not
> immediately read the result , but it keeps sending other queries to the
> server. By this time, server already encountered the error
> scenario(Receiving different message during COPY state) and sent error
> messages

IOW, the test intentionally violates the protocol and then all goes wonky
because of that.
That's why I was wondering upthread what's it's supposed to test.
I mean, regression tests are meant to warn against a desirable behavior
being unknowingly changed by new code into an undesirable behavior.
Here we have the undesirable behavior to start with.
What kind of regression could we fear from that?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Michael Paquier wrote:

> # Running: testlibpqbatch dbname=postgres 1 copyfailure
> dispatching SELECT query failed: cannot queue commands during COPY
> 
> COPYBUF: 5
> 
> Error status and message got from server due to COPY command failure
> are : PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during
> COPY from stdin
> CONTEXT:  COPY batch_demo, line 1

Same result here.

BTW the doc says: 
  "In batch mode only asynchronous operations are permitted, and COPY is
   not recommended as it most likely will trigger failure in batch
processing"
Yet it seems that the test assumes that it should work nonetheless.
I don't quite understand what we expect of this test, given what's
documented. Or what am I missing?

While looking at the regress log, I noticed multiple spurious
test_singlerowmode tests among the others. Should be fixed with:

--- a/src/test/modules/test_libpq/testlibpqbatch.c
+++ b/src/test/modules/test_libpq/testlibpqbatch.c
@@ -1578,6 +1578,7 @@ main(int argc, char **argv)
run_batch_abort = 0;
run_timings = 0;
run_copyfailure = 0;
+   run_singlerowmode = 0;
if (strcmp(argv[3], "disallowed_in_batch") == 0)
run_disallowed_in_batch = 1;
else if (strcmp(argv[3], "simple_batch") == 0)

There's also the fact that this test doesn't care whether it fails 
(compare with all the "goto fail" of the other tests).

And it happens that PQsetSingleRowMode() fails after the call to
PQbatchQueueProcess() that instantiates a PGresult
of status PGRES_BATCH_END to indicate the end of the batch,

To me this shows how this way of setting the single row mode is not ideal.
In order to avoid the failure, the loop should know in advance 
what kind of result it's going to dequeue before calling PQgetResult(),
which doesn't feel right.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> Am going to include the test which you shared in the test patch. Please let
> me know if you want to cover anymore specific cases to gain confidence.

One bit of functionality that does not work in batch mode and is left
as is by the patch is the PQfn() fast path interface and the large object
functions that use it.

Currently, calling lo_* functions inside a batch will fail with a message
that depends on whether the internal lo_initialize() has been successfully
called before.

If it hasn't, PQerrorMessage() will be:
   "Synchronous command execution functions are not allowed in batch mode"
which is fine, but it comes by happenstance from lo_initialize()
calling PQexec() to fetch the function OIDs from pg_catalog.pg_proc.

OTOH, if lo_initialize() has succeeded before, a call to a large
object function will fail with a different message:
  "connection in wrong state"
which is emitted by PQfn() based on conn->asyncStatus != PGASYNC_IDLE

Having an unified error message would be more user friendly.

Concerning the doc, when saying in 33.5.2:
 "In batch mode only asynchronous operations are permitted"
the server-side functions are indeed ruled out, since PQfn() is
synchronous, but maybe we should be a bit more explicit
about that?

Chapter 34.3 (Large Objects / Client Interfaces) could also
mention the incompatibility with batch mode.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> Please let me know if you think this is not enough but wanted to update
> section 33.6 also?

Yes, if the right place to call PQsetSingleRowMode() is immediately
after PQbatchQueueProcess(), I think we need to update
"33.6. Retrieving Query Results Row-By-Row"
with that information, otherwise what it says is just wrong
when in batch mode.

Also, in 33.5, I suggest to not use the "currently executing
query" as a synonym for the "query currently being processed"
to avoid any confusion between what the backend is executing
and a prior query whose results are being collected by the client
at the same moment.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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)

[Daniel Verite]

Tom Lane wrote:

> OT_WHOLE_LINE is not what you want because that results in verbatim
> copying, without variable expansion or anything

But if we want to implement "\if defined :foo" in the future
isn't it just what we need?

Also we could leave open the option to accept an SQL expression
here. I expect people will need SQL as the evaluator in a lot of cases.
So far we need to do that:

  SELECT sql_expr ... AS varname \gset
  \if :varname
  ...
  \endif

Surely users will wonder right away why they can't write it like this
instead:

  \if (sql_expr)
  ...
  \endif

There's a precedent with \copy accepting a query inside parentheses,
using OT_WHOLE_LINE.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> So, attached the alternative fix for this issue.
> Please share me your thoughts.

I assume you prefer the alternative fix because it's simpler.

> I would also like to hear Craig's opinion on it before applying this fix
> to the original patch, just to make sure am not missing anything here.

+1

The main question is whether the predicates enforced
by PQsetSingleRowMode() apply in batch mode in all cases
when it's legit to call that function. Two predicates
that may be problematic are:
if (conn->asyncStatus != PGASYNC_BUSY)
return 0;
and
if (conn->result)
return 0;

The general case with batch mode is that, from the doc:
"The client interleaves result processing with sending batch queries"
Note that I've not even tested that here, I've tested
batching a bunch of queries in a first step and getting the results
in a second step.
I am not confident that the above predicates will be true
in all cases. Also your alternative fix assumes that we add
a user-visible exception to PQsetSingleRowMode in batch mode,
whereby it must not be called as currently documented:
  "call PQsetSingleRowMode immediately after a successful call of 
   PQsendQuery (or a sibling function)"
My gut feeling is that it's not the right direction, I prefer making
the single-row a per-query attribute internally and keep
PQsetSingleRowMode's contract unchanged from the
user's perspective.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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)

[Daniel Verite]

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.

This is basically what bash does. In an if/else/fi block
in an interactive session, the second prompt is displayed at every new
line and nothing gets executed until it recognizes the end of the
block and it's valid as a whole. Otherwise, nothing of the block
gets executed. That doesn't strike me as unfriendly.

When non-interactive, in addition to the block not being executed,
the fact that it fails implies that the execution of the current script
is ended, independently of the errexit setting.
If errexit is set, the interpreter terminates. If it was
an included script and errexit is not set, the execution resumes
after the point of the inclusion.

On the whole, isn't that a reasonable model to follow for psql?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> >  while (QbatchQueueProcess(conn)) {
> >r = PQsetSingleRowMode(conn);
> >if (r!=1) {
> >   fprintf(stderr, "PQsetSingleRowMode() failed");
> >}
> >..

> Thanks for investigating the problem, and could you kindly explain what
> "next iteration" you mean here? Because I don't see any problem in
> following sequence of calls - PQbatchQueueProcess(),PQsetSingleRowMode()
> , PQgetResult()

I mean the next iteration of the above while statement. Referring
to the doc, that would be the "next batch entry":

  " To get the result of the first batch entry the client must call
   PQbatchQueueProcess. It must then call PQgetResult and handle the
   results until PQgetResult returns null (or would return null if
   called). The result from the next batch entry may then be retrieved
   using PQbatchQueueProcess and the cycle repeated"

Attached is a bare-bones testcase showing the problem.
As it is, it works, retrieving results for three "SELECT 1"
in the same batch.  Now in order to use the single-row
fetch mode, consider adding this:

r = PQsetSingleRowMode(conn);
if (r!=1) {
  fprintf(stderr, "PQsetSingleRowMode() failed for i=%d\n", i);
}

When inserted after the call to PQbatchQueueProcess,
which is what I understand you're saying works for you,
it fails for me when starting to get the results of the 2nd query
and after. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


test-singlerow-batch.c
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

  Hi,

I notice that PQsetSingleRowMode() doesn't work when getting batch results.  

The function is documented as:
" int PQsetSingleRowMode(PGconn *conn);

  This function can only be called immediately after PQsendQuery or one
  of its sibling functions, before any other operation on the connection
  such as PQconsumeInput or PQgetResult"

But PQbatchQueueProcess() unconditionally clears conn->singleRowMode,
so whatever it was when sending the query gets lost, and besides
other queries might have been submitted in the meantime.

Also if trying to set that mode when fetching like this

 while (QbatchQueueProcess(conn)) {
   r = PQsetSingleRowMode(conn);
   if (r!=1) {
  fprintf(stderr, "PQsetSingleRowMode() failed");
   }
   .. 

it might work the first time, but on the next iterations, conn->asyncStatus
might be PGASYNC_READY, which is a failure condition for
PQsetSingleRowMode(), so that won't do.

ISTM that the simplest fix would be that when in batch mode,
PQsetSingleRowMode() should register that the last submitted query
does request that mode, and when later QbatchQueueProcess dequeues
the batch entry for the corresponding query, this flag would be popped off
and set as the current mode.

Please find attached the suggested fix, against the v5 of the patch.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 3c0be46..8ddf63d 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1425,7 +1425,7 @@ PQmakePipelinedCommand(PGconn *conn)
 	}
 	entry->next = NULL;
 	entry->query = NULL;
-
+	entry->singleRowMode = false;
 	return entry;
 }
 
@@ -1783,16 +1783,28 @@ PQsetSingleRowMode(PGconn *conn)
 	/*
 	 * Only allow setting the flag when we have launched a query and not yet
 	 * received any results.
+	 * In batch mode, store the flag in the queue for applying it later.
 	 */
 	if (!conn)
 		return 0;
-	if (conn->asyncStatus != PGASYNC_BUSY)
-		return 0;
 	if (conn->queryclass != PGQUERY_SIMPLE &&
 		conn->queryclass != PGQUERY_EXTENDED)
 		return 0;
 	if (conn->result)
 		return 0;
+	if (conn->batch_status == PQBATCH_MODE_OFF)
+	{
+		if (conn->asyncStatus != PGASYNC_BUSY)
+			return 0;
+	}
+	else
+	{
+		/* apply to the last submitted query in the batch, or fail */
+		if (conn->cmd_queue_tail != NULL)
+			conn->cmd_queue_tail->singleRowMode = true;
+		else
+			return 0;
+	}
 
 	/* OK, set flag */
 	conn->singleRowMode = true;
@@ -2120,9 +2132,8 @@ PQbatchQueueProcess(PGconn *conn)
 	 * Initialize async result-accumulation state */
 	pqClearAsyncResult(conn);
 
-	/* reset single-row processing mode */
-	conn->singleRowMode = false;
-
+	/* Set single-row processing mode */
+	conn->singleRowMode = next_query->singleRowMode;
 
 	conn->last_query = next_query->query;
 	next_query->query = NULL;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 33f212f..af4f753 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -315,6 +315,7 @@ typedef struct pgCommandQueueEntry
 {
 	PGQueryClass queryclass;	/* Query type; PGQUERY_SYNC for sync msg */
 	char	   *query;			/* SQL command, or NULL if unknown */
+	bool	   singleRowMode;   /* apply single row mode to this query */
 	struct pgCommandQueueEntry *next;
 }	PGcommandQueueEntry;
 

-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

>if (PQbatchStatus(st->con) != PQBATCH_MODE_ON)
> 
>But, it is better to use if (PQbatchStatus(st->con) ==
> PQBATCH_MODE_OFF) for this verification. Reason is there is one more state
> in batch mode - PQBATCH_MODE_ABORTED. So, if the batch mode is in aborted
> status, this check will still assume that the connection is not in batch
> mode.
> In a same way, "if(PQbatchStatus(st->con) == PQBATCH_MODE_ON)" check is
> better to be modified as "PQbatchStatus(st->con) != PQBATCH_MODE_OFF".

Agreed, these two tests need to be changed to account for the
PQBATCH_MODE_ABORTED case. Fixed in new patch.

> 2.  @@ -2207,7 +2227,47 @@ doCustom(TState *thread, CState *st, StatsData
> *agg)
> + if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
> + {
> + commandFailed(st, "already in batch mode");
> + break;
> + }
> 
> This is not required as below PQbatchBegin() internally does this
> verification.
> 
> + PQbatchBegin(st->con);

The point of this test is to specifically forbid a sequence like this
\beginbatch
...(no \endbatch)
\beginbatch
...
and according to the doc for PQbatchBegin, it looks like the return
code won't tell us:
   "Causes a connection to enter batch mode if it is currently idle or
   already in batch mode.
int PQbatchBegin(PGconn *conn);
   Returns 1 for success"

My understanding is that "already in batch mode" is not an error.

   "Returns 0 and has no effect if the connection is not currently
   idle, i.e. it has a result ready, is waiting for more input from
   the server, etc. This function does not actually send anything to
   the server, it just changes the libpq connection state"

> 3. It is better to check the return value of PQbatchBegin() rather than
> assuming success. E.g: PQbatchBegin() will return false if the connection
> is in copy in/out/both states.

Given point #2 above, I think both tests are needed:
   if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
{
   commandFailed(st, "already in batch mode");
   break;
}
if (PQbatchBegin(st->con) == 0)
{
   commandFailed(st, "failed to start a batch");
   break;
}

> > 3. In relation to #2, PQsendQuery() is not forbidden in batch mode
> > although I don't think it can work with it, as it's based on the old
> > protocol.
> >
> It is actually forbidden and you can see the error message "cannot
> PQsendQuery in batch mode, use PQsendQueryParams" when you use
> PQsendQuery().

Sorry for blaming the innocent piece of code, looking closer
it's pgbench that does not display the message.
With the simple query protocol, sendCommand() does essentially:

  r = PQsendQuery(st->con, sql);

... other stuff...

  if (r == 0)
  {
if (debug)
  fprintf(stderr, "client %d could not send %s\n",
  st->id, command->argv[0]);
st->ecnt++;
return false;
  }

So only in debug mode does it inform the user that it failed, and
even then, it does not display PQerrorMessage(st->con).

In non-debug mode it's opaque to the user. If it always fail, it appears
to loop forever. The caller has this comment that is relevant to the problem:

if (!sendCommand(st, command))
  {
/*
 * Failed. Stay in CSTATE_START_COMMAND state, to
 * retry. ??? What the point or retrying? Should
 * rather abort?
 */
return;
  }

I think it doesn't bother anyone up to now because the immediate
failure modes of PQsendQuery() are resource allocation or protocol
failures that tend to never happen.

Anyway currently \beginbatch avoids the problem by checking
whether querymode == QUERY_SIMPLE, to fail gracefully
instead of letting the endless loop happen.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f6cb5d4..f93673e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -269,7 +269,8 @@ typedef enum
 *
 * CSTATE_START_COMMAND starts the execution of a command.  On a SQL
 * command, the command is sent to the server, and we move to
-* CSTATE_WAIT_RESULT state.  On a \sleep meta-command, the timer is 
set,
+* CSTATE_WAIT_RESULT state unless in batch mode.
+* On a \sleep meta-command, the timer is set,
 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 * meta-commands are executed immediately.
 *
@@ -1882,11 +1883,24 @@ sendCommand(CState *st, Command *command)
if (commands[j]->type != SQL_COMMAND)
continue;
preparedStatementName(name, st->use_file, j);
-   res = PQprepare(st->con, name,
- commands[j]->argv[0], 
commands[j]->argc - 1, NULL);
-   if 

Re: [HACKERS] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Vaishnavi Prabakaran wrote:

> Yes, I have created a new patch entry into the commitfest 2017-03 and
> attached the latest patch with this e-mail.

Please find attached a companion patch implementing the batch API in
pgbench, exposed as \beginbatch and \endbatch meta-commands
(without documentation).

The idea for now is to make it easier to exercise the API and test
how batching performs. I guess I'll submit the patch separately in
a future CF, depending on when/if the libpq patch goes in.

While developing this, I noted a few things with 0001-v4:

1. lack of initialization for count in PQbatchQueueCount.
Trivial fix:

--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1874,7 +1874,7 @@ PQisBusy(PGconn *conn)
 int
 PQbatchQueueCount(PGconn *conn)
 {
-   int count;
+   int count = 0;
PGcommandQueueEntry *entry;

2. misleading error message in PQexecStart. It gets called by a few other
functions than PQexec, such as PQprepare. As I understand it, the intent
here is to forbid the synchronous functions in batch mode, so this error
message should not single out PQexec.

@@ -1932,6 +2425,13 @@ PQexecStart(PGconn *conn)
if (!conn)
return false;
 
+   if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status !=
PQBATCH_MODE_OFF)
+   {
+   printfPQExpBuffer(>errorMessage,
+ libpq_gettext("cannot
PQexec in batch mode\n"));
+   return false;
+   }
+

3. In relation to #2, PQsendQuery() is not forbidden in batch mode
although I don't think it can work with it, as it's based on the old
protocol. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f6cb5d4..9b2fce8 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -269,7 +269,8 @@ typedef enum
 *
 * CSTATE_START_COMMAND starts the execution of a command.  On a SQL
 * command, the command is sent to the server, and we move to
-* CSTATE_WAIT_RESULT state.  On a \sleep meta-command, the timer is 
set,
+* CSTATE_WAIT_RESULT state unless in batch mode.
+* On a \sleep meta-command, the timer is set,
 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 * meta-commands are executed immediately.
 *
@@ -1882,11 +1883,24 @@ sendCommand(CState *st, Command *command)
if (commands[j]->type != SQL_COMMAND)
continue;
preparedStatementName(name, st->use_file, j);
-   res = PQprepare(st->con, name,
- commands[j]->argv[0], 
commands[j]->argc - 1, NULL);
-   if (PQresultStatus(res) != PGRES_COMMAND_OK)
-   fprintf(stderr, "%s", 
PQerrorMessage(st->con));
-   PQclear(res);
+   if (PQbatchStatus(st->con) != PQBATCH_MODE_ON)
+   {
+   res = PQprepare(st->con, name,
+   
commands[j]->argv[0], commands[j]->argc - 1, NULL);
+   if (PQresultStatus(res) != 
PGRES_COMMAND_OK)
+   fprintf(stderr, "%s", 
PQerrorMessage(st->con));
+   PQclear(res);
+   }
+   else
+   {
+   /*
+* In batch mode, we use asynchronous 
functions. If a server-side
+* error occurs, it will be processed 
later among the other results.
+*/
+   if (!PQsendPrepare(st->con, name,
+  
commands[j]->argv[0], commands[j]->argc - 1, NULL))
+   fprintf(stderr, "%s", 
PQerrorMessage(st->con));
+   }
}
st->prepared[st->use_file] = true;
}
@@ -2165,7 +2179,13 @@ doCustom(TState *thread, CState *st, StatsData *agg)
return;
}
else
-   st->state = CSTATE_WAIT_RESULT;
+   {
+   /* Wait for results, unless in 
batch mode */
+

Re: [HACKERS] One-shot expanded output in psql using \gx

[Daniel Verite]

Christoph Berg wrote:

> Both fixed, thanks for the review! Version 3 attached.

It looks good to me.

Stephen Frost is also reviewer on the patch, so I'm moving the
status back to "Needs review" at
https://commitfest.postgresql.org/13/973/
and let him proceed.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] One-shot expanded output in psql using \gx

[Daniel Verite]

Christoph Berg wrote:

> The new version tests \g and \gx with a new query, and
> re-running it on the last query buffer.

Thanks, here's a review:

The patch compiles and works as expected.

The code follows the same pattern as other one-shot command
modifiers, setting a flag in the global "pset" struct
in exec_command() and resetting it at the end of SendQuery().

- make installcheck-world: ok

- sgml doc: ok

- help text: ok

- includes regression tests: ok

- tab-completion: works but the list in tab-complete.c:backslash_commands[]
is sorted alphabetically so "\\gx" should come after "\\gset"

- another nitpick: in PrintQueryTuples() it assigns a bool:
+   /* one-shot expanded output requested via \gx */
+   if (pset.g_expanded)
+   my_popt.topt.expanded = true;
+ 

"expanded" is defined as a tri-valued short int (print.h:98):
  typedef struct printTableOpt
  {
//
unsigned short int expanded;/* expanded/vertical output (if supported
by
 * output format); 0=no, 1=yes, 2=auto */
Although there is still code that puts true/false in this variable
(probably because it was a bool before?), I assume that for new
code, assigning 0/1/2 should be preferred.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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)

[Daniel Verite]

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.

Are we sold on the idea that conditionals should be implemented
by meta-commands, rather than for example terminal symbols of
a new grammar on top of the existing?

To recall the context, psql variables are really macros that may
contain meta-commands, and when they do they're essentially
injected and executed at the point of interpolation. That's more
or less what started this thread: demo'ing how we could exit
conditionally by injecting '\q' or nothing into a variable, and
saying that even if doable it was pretty weird, and it would be
better to have real conditional structures instead.

But when conditional structures are implemented as
meta-commands, there's the problem that this structure
can be generated on the fly too, which in a way is no less weird.
While I think that the introduction of conditionals in
psql is great, I'm getting doubtful about that part.
Are there popular script languages or preprocessors
that accept variables/macros instead of symbols to structure
the flow of instructions? I can't think of any myself.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

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

[Daniel Verite]

Corey Huinker wrote:

[about Ctrl-C]

> 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?

Probably you don't need to care about cancel_pressed, and
the /if stack could be unwound at the point the SIGINT
handler longjumps to, in mainloop.c:

/* got here with longjmp */

/* reset parsing state */
psql_scan_finish(scan_state);
psql_scan_reset(scan_state);
resetPQExpBuffer(query_buf);
resetPQExpBuffer(history_buf);
count_eof = 0;
slashCmdStatus = PSQL_CMD_UNKNOWN;
prompt_status = PROMPT_READY;
pset.stmt_lineno = 1;
cancel_pressed = false;

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.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

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

[Daniel Verite]

Corey Huinker wrote:

> I meant in the specific psql-context, does it do anything other
> than (attempt to) terminate sent-but-not-received SQL queries?

It cancels the current edit in the query buffer, including the
case when it spans multiple lines.
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

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Daniel Verite]

Peter Eisentraut wrote:

> You really have (at least) three options here:
> 
> 1. Rename nothing
> 2. Rename directory only
> 3. Rename everything

I vote for 1) as I believe the renaming will create more confusion
than it's worth, not even considering the renaming of functions
and views.

What if we look at the change from the pessimistic angle?
An example of confusion that the change would create:
a lot of users currently choose pg_wal for the destination
directory of their archive command. Less-informed users
that set up archiving and/or log shipping in PG10 based on
advice online from previous versions will be fairly
confused about the missing pg_xlog, and the fact that the
pg_wal directory they're supposed to create already exists.

Also googling for pg_wal, I'm finding food for thought like this
IBM technote:
http://www-01.ibm.com/support/docview.wss?uid=isg3T1015637
which recommends to 
"Remove all files under /var/lib/pgsql/9.0/data/pg_wal/"
and also calls that directory the "write-ahead log directory"
which is quite confusing because apparently it's the destination of
their archive command.

This brings the question: what about the people who will delete
their pg_wal (ex-pg_xlog) when they face a disk-full condition and
they mix up in their mind the archive directory and the WAL directory?
It's hard to guess how many could make that mistake but I don't see
why it would be less probable than confusing pg_xlog and pg_log.
At least the contents of the latter directories look totally
different, contrary to the wal directory versus wal archive.

Also, what about the users who are helped currently by
the fact that "pg_xlog" is associated at the top of google results
to good articles that explain what it is, why it fills up and what
to do about it? By burying the name "pg_xlog" we're also loosing that
connection, and in the worst case making worse the problem we
wanted to help with.

There's also the disruption in existing backup scripts that directly
reference pg_xlog. Obviously these scripts are critical, and there's
something weird in breaking that intentionally. The motivation of the
breakage is likely to be felt as frivolous and unfair to those people who
are adversely impacted by the change, even if the part of not
reading the release notes is their fault.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> I updated the documentation as well.  I think this is committable if
> there are not objections.

That works for me. I tested and read it and did not find anything
unexpected or worth objecting.
"\unset var" acting as "\set var off" makes sense considering
that its opposite "\set var" is now an accepted
synonym of "\set var on" for the boolean built-ins.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

I wrote:

> This would allow the hook to distinguish between initialization and
> unsetting, which in turn will allow it to deny the \unset in the
> cases when it doesn't make any sense conceptually (like AUTOCOMMIT).

I notice that in the commited patch, you added the ability
for DeleteVariable() to reject the deletion if the hook
disagrees. 
+   {
+   /* Allow deletion only if hook is okay with
NULL value */
+   if (!(*current->assign_hook) (NULL))
+   return false;   /* message
printed by hook */

But this can't happen in practice because as mentioned just upthread
the hook called with NULL doesn't know if the variable is getting unset
or initialized, so rejecting on NULL is not an option.

Attached is a proposed patch to add initial values to
SetVariableAssignHook() to solve this problem, and an example for
\unset AUTOCOMMIT being denied by the hook.

As a side effect, we see all buit-in variables when issuing \set
rather than just a few.

Does it make sense?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 0574b5b..631b3f6 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -166,14 +166,6 @@ main(int argc, char *argv[])
 
SetVariable(pset.vars, "VERSION", PG_VERSION_STR);
 
-   /* Default values for variables */
-   SetVariableBool(pset.vars, "AUTOCOMMIT");
-   SetVariable(pset.vars, "VERBOSITY", "default");
-   SetVariable(pset.vars, "SHOW_CONTEXT", "errors");
-   SetVariable(pset.vars, "PROMPT1", DEFAULT_PROMPT1);
-   SetVariable(pset.vars, "PROMPT2", DEFAULT_PROMPT2);
-   SetVariable(pset.vars, "PROMPT3", DEFAULT_PROMPT3);
-
parse_psql_options(argc, argv, );
 
/*
@@ -785,6 +777,11 @@ showVersion(void)
 static bool
 autocommit_hook(const char *newval)
 {
+   if (newval == NULL)
+   {
+   psql_error("built-in variable %s cannot be unset.\n", 
"AUTOCOMMIT");
+   return false;
+   }
return ParseVariableBool(newval, "AUTOCOMMIT", );
 }
 
@@ -1002,20 +999,20 @@ EstablishVariableSpace(void)
 {
pset.vars = CreateVariableSpace();
 
-   SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook);
-   SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
-   SetVariableAssignHook(pset.vars, "QUIET", quiet_hook);
-   SetVariableAssignHook(pset.vars, "SINGLELINE", singleline_hook);
-   SetVariableAssignHook(pset.vars, "SINGLESTEP", singlestep_hook);
-   SetVariableAssignHook(pset.vars, "FETCH_COUNT", fetch_count_hook);
-   SetVariableAssignHook(pset.vars, "ECHO", echo_hook);
-   SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook);
-   SetVariableAssignHook(pset.vars, "ON_ERROR_ROLLBACK", 
on_error_rollback_hook);
-   SetVariableAssignHook(pset.vars, "COMP_KEYWORD_CASE", 
comp_keyword_case_hook);
-   SetVariableAssignHook(pset.vars, "HISTCONTROL", histcontrol_hook);
-   SetVariableAssignHook(pset.vars, "PROMPT1", prompt1_hook);
-   SetVariableAssignHook(pset.vars, "PROMPT2", prompt2_hook);
-   SetVariableAssignHook(pset.vars, "PROMPT3", prompt3_hook);
-   SetVariableAssignHook(pset.vars, "VERBOSITY", verbosity_hook);
-   SetVariableAssignHook(pset.vars, "SHOW_CONTEXT", show_context_hook);
+   SetVariableAssignHook(pset.vars, "AUTOCOMMIT", autocommit_hook, "on");
+   SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook, 
"off");
+   SetVariableAssignHook(pset.vars, "QUIET", quiet_hook, "off");
+   SetVariableAssignHook(pset.vars, "SINGLELINE", singleline_hook, "off");
+   SetVariableAssignHook(pset.vars, "SINGLESTEP", singlestep_hook, "off");
+   SetVariableAssignHook(pset.vars, "FETCH_COUNT", fetch_count_hook, "-1");
+   SetVariableAssignHook(pset.vars, "ECHO", echo_hook, "none");
+   SetVariableAssignHook(pset.vars, "ECHO_HIDDEN", echo_hidden_hook, 
"off");
+   SetVariableAssignHook(pset.vars, "ON_ERROR_ROLLBACK", 
on_error_rollback_hook, "off");
+   SetVariableAssignHook(pset.vars, "COMP_KEYWORD_CASE", 
comp_keyword_case_hook, "preserve-upper");
+   SetVariableAssignHook(pset.vars, "HISTCONTROL", histcontrol_hook, 
"none");
+   SetVariableAssignHook(pset.vars, "PROMPT1", prompt1_hook, 
DEFAULT_PROMPT1);
+   SetVariableAssignHook(pset.vars, "PROMPT2", prompt2_hook, 
DEFAULT_PROMPT2);
+   SetVariableAssignHook(pset.vars, "PROMPT3", prompt3_hook, 
DEFAULT_PROMPT3);
+   SetVariableAssignHook(pset.vars, "VERBOSITY", verbosity_hook, 
"default");
+   SetVariableAssignHook(pset.vars, "SHOW_CONTEXT", show_context_hook, 
"errors");
 }
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 91e4ae8..2e5c3e0 100644
--- a/src/bin/psql/variables.c
+++ 

Re: [HACKERS] One-shot expanded output in psql using \G

[Daniel Verite]

Stephen Frost wrote:

> That's not how '\dx' works, as I pointed out, so I don't see having the
> second character being 'x' to imply "\x mode" makes sense.

\gx means "like \g but output with expanded display"
It turns out that it's semantically close to "\g with \x" so
I refered to it like that as a shortcut upthread, my fault.
It was never meant to establish a precedent that combining
two letters would mean "do the first one-letter command and the
second as a sub-command" which indeed woud be inconsistent with
the existing \ef, \sf, \sv, \d[*] and so on.

> I can't recall ever using the other formatting toggles (aligned, HTML,
> and tuples only) before in interactive sessions, except (rarely) with
> \o.

\a is handy to read sizeable chunks of text in fields that 
contain newlines, and personally I need it on a regular basis
in interactive sessions. It depends on the kind of data you have to work
with.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> Moreover, the committed patch is inconsistent in that it forbids
> only one of the above.  Why is it okay to treat unset as "off",
> but not okay to treat the default empty-string value as "on"?

Treating unset (NULL in the value) as "off" comes from the fact 
that except AUTOCOMMIT, our built-in variables are not initialized
with a default value. For instance we call this in EstablishVariableSpace();
  SetVariableAssignHook(pset.vars, "ON_ERROR_STOP", on_error_stop_hook);
then on_error_stop_hook is called with NULL as the value
then ParseVariableBool(NULL, "ON_ERROR_STOP", _error_stop)
is what initializes pset.on_error_stop to false.

The same happens if/when the variable is unset. Again the hook is called
with NULL, and it sets back the pset.* variable in a hardcoded default state,
which is false for all booleans.

Incidentally I want to suggest to change that, so that all variables
should be initialized with a non-NULL value right from the start, and the
value would possibly come to NULL only if it's unset.
This would allow the hook to distinguish between initialization and
unsetting, which in turn will allow it to deny the \unset in the
cases when it doesn't make any sense conceptually (like AUTOCOMMIT).

Forcing values for our built-in variables would also avoid the following:

=# \echo :ON_ERROR_STOP
:ON_ERROR_STOP

Even if the variable ON_ERROR_STOP does exist in the VariableSpace
and has a hook and there's an initialized corresponding pset.on_error_stop,
from the user's viewpoint, it's as if the variable doesn't exist
until they intialize it explicitly.
I suggest that it doesn't make much sense and it would be better
to have that instead right from the start:

=# \echo :ON_ERROR_STOP
off

> One possible compromise that would address your concern about display
> is to modify the hook API some more so that variable hooks could actually
> substitute new values.  Then for example the bool-variable hooks could
> effectively replace "\set AUTOCOMMIT" by "\set AUTOCOMMIT on" and
> "\unset AUTOCOMMIT" by "\set AUTOCOMMIT off", ensuring that interrogation
> of their values always produces sane results.

Agreed, that looks like a good compromise.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> Evidently, this test script is relying on the preceding behavior that
> setting a bool variable to an empty string was equivalent to setting
> it to "true".  If it's just that script I would be okay with saying
> "well, it's a bug in that script" ... but I'm a bit worried that this
> may be the tip of the iceberg, ie maybe a lot of people have done
> things like this.  Should we reconsider the decision to reject empty
> strings in ParseVariableBool?

Sigh. It was considered upthread, I'm requoting the relevant bit:


 if (pg_strncasecmp(value, "true", len) == 0)
  return true;
  It happens that "" as a value yields the same result than "true" for this
  test so it passes, but it seems rather unintentional.

  The resulting problem from the POV of the user is that we can do that,
  for instance:

test=> \set AUTOCOMMIT

  without error message or feedback, and that leaves us without much
  clue about autocommit being enabled:

test=> \echo :AUTOCOMMIT

test=> 

  So I've changed ParseVariableBool() in v4 to reject empty string as an
  invalid boolean (but not NULL since the startup logic requires NULL
  meaning false in the early initialization of these variables).

  "make check" seems OK with that, I hope it doesn't cause any regression
  elsewhere.


So it does cause regressions. I don't know if we should reaccept
empty strings immediately to fix that, but in the long run, I think
that the above situation with the empty :AUTOCOMMIT is not really
sustainable: when we extend what we do with variables
(/if /endif and so on), it will become even more of a problem.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> Also, if you want to argue that allowing it to change intra-
> transaction is too confusing, why would we only forbid this direction
> of change and not both directions?

The thread "Surprising behaviour of \set AUTOCOMMIT ON" at:
https://www.postgresql.org/message-id/CAH2L28sTP-9dio3X1AaZRyWb0-ANAx6BDBi37TGmvw1hBiu0oA%40mail.gmail.com
seemed to converge towards the conclusion implemented in the autocommit_hook
proposed in the patch.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

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

[Daniel Verite]

Corey Huinker wrote:

> >   \if ERROR
> >  \echo X
> >   \else
> >  \echo Y
> >   \endif
> >
> > having both X & Y printed and error reported on else and endif. I think
> > that an expression error should just put the stuff in ignore state.
> >
> 
> Not just false, but ignore the whole if-endif? interesting. I hadn't
> thought of that. Can do.

If we use the Unix shell as a model, in POSIX "test" and  "if"
are such that an evaluation error (exit status>1) leads to the same
flow than when evaluating to false (exit status=1).

References I can find:

test:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

if:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04_07

BTW, in "set -e" mode, it also says that a failure to evaluate an "if"
 expression does not lead to the script stopping:

  The -e setting shall be ignored when executing the compound list
  following the while, until, if, or elif reserved word, a pipeline
  beginning with the ! reserved word, or any command of an AND-OR list
  other than the last


So psql is not following that model with ON_ERROR_STOP if it exits
with an error when unable to evaluate an "if" expression.
I'm not implying that we should necessarily adopt the shell behavior,
but as these choices have certainly been made in POSIX for good
reasons, we should make sure to think twice about why they don't
apply to psql.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] One-shot expanded output in psql using \G

[Daniel Verite]

Christoph Berg wrote:

> But do we really want to choose
> something different just because MySQL is using it?

That's not what I meant. If mysql wasn't using \G
I'd still suggest the name \gx because:

- it means the functionality of \g combined with \x so
semantically it makes sense.

- there is no precedent in psql that the upper-case version
of a meta-command as a variant of the lower-case version:
\C has nothing to do with \c, and \H nothing with \h, and
\T and \t are equally disconnected

- there hasn't been much use up to now of uppercase
meta-commands, C,T and H are the only ones I see in \?
\d[something] is crowded  with lots of "something", whereas \D is not
used at all. The pattern seems to be that uppercase is the exception.

FWIW I don't share the feeling that \G is easier to remember or type
than \gx.

> \G will be much easier to explain to existing users (both people
> coming from MySQL to PostgreSQL, and PostgreSQL users doing a detour
> into foreign territory), and it would be one difference less to have
> to care about when typing on the CLIs.

That's a good argument, but if it's pitted against psql's
consistency with itself, I'd expect the latter to win.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] One-shot expanded output in psql using \G

[Daniel Verite]

Christoph Berg wrote:

> A workaround is to submit queries using "\x\g\x", but that's ugly,
> clutters the output with toggle messages, and will forget that "\x
> auto" was set.
> 
> Mysql's CLI client is using \G for this purpose, and adding the very
> same functionality to psql fits nicely into the set of existing
> backslash commands: \g sends the query buffer, \G will do exactly the
> same as \g (including parameters), but forces expanded output just for
> this query.

+1 for the functionality but should we choose to ignore the comparison
to mysql, I'd suggest \gx for the name.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

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

[Daniel Verite]

Corey Huinker wrote:

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

This contrasts with line 2: -- select :var
where the reference to :var is inoffensive.

To avoid that kind of trouble, would it make sense not to expand
variables in untaken branches?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

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

[Daniel Verite]

Corey Huinker wrote:

> >
> > 1: unrecognized value "whatever" for "\if"; assuming "on"
> >
> > I do not think that the script should continue with such an assumption.
> >
> 
> I agree, and this means we can't use ParseVariableBool() as-is

The patch at https://commitfest.postgresql.org/12/799/
in the ongoing CF already changes ParseVariableBool()
to not assume that unrecognizable values should be set to
"on".

There's also the fact that ParseVariableBool() in HEAD assumes
that an empty value is valid and true, which I think leads to this
inconsistency in the current patch:

\set empty
\if :empty
select 1 as result \gset
\else
select 2 as result \gset
\endif
\echo 'result is' :result

produces: result is 1 (so an empty string evaluates to true)

Yet this sequence:

\if
select 1 as result \gset
\else
select 2 as result \gset
\endif
\echo 'result is' :result

produces: result is 2 (so an empty \if evaluates to false)

The equivalence between empty value and true in
ParseVariableBool() is also suppressed in the above-mentioned
patch.

ISTM that it's important that eventually ParseVariableBool()
and \if agree on what evaluates to true and false (and the
more straightforward way to achieve that is by \if calling
directly ParseVariableBool), but that it's not productive that we
discuss /if issues relatively to the behavior of ParseVariableBool()
in HEAD at the moment, as it's likely to change.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

  I just wrote:

> - add enum-style suggestions on invalid input for \pset x, \pset pager,
>  and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
>  HISTCONTROL, VERBOSITY, SHOW_CONTEXT, \x, \pager

There's no such thing as \pager, I meant to write:

  \pset x, \pset pager,
  and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
  HISTCONTROL, VERBOSITY, SHOW_CONTEXT


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> I took a quick look through this.  It seems to be going in generally
> the right direction, but here's a couple of thoughts:

Here's an update with these changes:

per Tom's suggestions upthread:

- change ParseVariableBool() signature to return validity as bool.

- remove ParseCheckVariableNum() in favor of using tightened up
  ParseVariableNum() and GetVariableNum().

- updated header comments in variables.h

other changes:

- autocommit_hook rejects transitions from OFF to ON when inside a
transaction, per suggestion of Rahila Syed (which was the original
motivation for the set of changes of this patch).

- slight doc update for HISTCONTROL (values outside of enum not longer
allowed)

- add enum-style suggestions on invalid input for \pset x, \pset pager,
  and \set of ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
  HISTCONTROL, VERBOSITY, SHOW_CONTEXT, \x, \pager

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 9915731..042d277 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -3213,9 +3213,8 @@ bar
  list. If set to a value of ignoredups, lines
  matching the previous history line are not entered. A value of
  ignoreboth combines the two options. If
- unset, or if set to none (or any other value
- than those above), all lines read in interactive mode are
- saved on the history list.
+ unset, or if set to none, all lines read
+ in interactive mode are saved on the history list.
 
 
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 4139b77..091a138 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -253,26 +253,31 @@ exec_command(const char *cmd,
opt1 = read_connect_arg(scan_state);
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
-   reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
-   TRI_YES : TRI_NO;
-
-   free(opt1);
-   opt1 = read_connect_arg(scan_state);
+   bool on_off;
+   success = ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, _off);
+   if (success)
+   {
+   reuse_previous = on_off ? TRI_YES : TRI_NO;
+   free(opt1);
+   opt1 = read_connect_arg(scan_state);
+   }
}
else
reuse_previous = TRI_DEFAULT;
 
-   opt2 = read_connect_arg(scan_state);
-   opt3 = read_connect_arg(scan_state);
-   opt4 = read_connect_arg(scan_state);
+   if (success)/* do not attempt to connect if 
reuse_previous argument was invalid */
+   {
+   opt2 = read_connect_arg(scan_state);
+   opt3 = read_connect_arg(scan_state);
+   opt4 = read_connect_arg(scan_state);
 
-   success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+   success = do_connect(reuse_previous, opt1, opt2, opt3, 
opt4);
 
+   free(opt2);
+   free(opt3);
+   free(opt4);
+   }
free(opt1);
-   free(opt2);
-   free(opt3);
-   free(opt4);
}
 
/* \cd */
@@ -1548,7 +1553,7 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   success = ParseVariableBool(opt, "\\timing", 
);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2665,16 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   {
+   bool on_off;
+   if (ParseVariableBool(value, NULL, _off))
+   popt->topt.expanded = on_off ? 1 : 0;
+   else
+   {
+   PsqlVarEnumError(param, value, "on, off, auto");
+   return false;
+   }
+   }
else
  

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> However, it only really works if psql never overwrites the values
> after startup, whereas I believe all of these are overwritten by
> a \c command.

Yes, there are reset to reflect the properties of the new connection.

> So maybe it's more user-friendly to make these variables fully
> reserved, even at the risk of breaking existing scripts.  But
> I don't think it's exactly an open-and-shut question.

You mean if we make that fail:  \set ENCODING UTF8
it's going to make that fail too:
  SELECT something AS "ENCODING"[,...]  \gset
and I agree it's not obvious that this trade-off has to be
made. Personally I'm fine with the status quo and will
not add that hook into the patch unless pressed to.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Ashutosh Sharma wrote:

> postgres=# \echo :ENCODING
> UTF8
> postgres=# \set ENCODING xyz
> postgres=# \echo :ENCODING
> xyz
> 
> I think currently we are not even showing what are the different valid
> encoding names to the end users like we show it for other built-in
> variables
> VERBOSITY, ECHO etc. I mean if i run '\set VERBOSITY' followed by tab
> command it does show me the valid values for VERBOSITY but not for
> ENCODING.

Setting ENCODING has no effect, like DBNAME, USER, HOST and PORT.
In a way, it's a read-only variable that's here to inform the user,
not as a means to change the encoding (\encoding does that and
has proper support for tab completion)

What we could do as of this patch is emit an error when we try
to change ENCODING, with a hook returning false and
a proper error message hinting to \encoding.

I'm working on adding such messages to other variables.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> "Daniel Verite" <dan...@manitou-mail.org> writes:
> > [ psql-var-hooks-v6.patch ]
> 
> I took a quick look through this.  It seems to be going in generally
> the right direction, but here's a couple of thoughts:

Thanks for looking into this!
 
> I'm inclined to think that the best choice is for the function result
> to be the ok/not ok flag, and pass the variable-to-be-modified as an
> output parameter.  That fits better with the notion that the variable
> is not to be modified on failure.

Agreed, if never clobbering the variable, having the valid/invalid state
returned by ParseVariableBool() allows for simpler code. I'm changing it
that way.

> Also, why aren't you using ParseVariableBool's existing ability to report
> errors?

Its' because there are two cases:
- either only a boolean can be given to the command or variable,
in which we let ParseVariableBool() tell:
   unrecognized value "bogus" for "command": boolean expected

- or other parameters besides boolean are acceptable, in which case we
can't say "boolean expected", because in fact a boolean is no more or
less expected than the other valid values.

We could shave code by reducing ParseVariableBool()'s error message to:
  unrecognized value "bogus" for "name"
clearing the distinction between [only booleans are expected]
and [booleans or enum are expected].
Then almost all callers that have their own message could rely
on ParseVariableBool() instead, as they did previously.

Do we care about the "boolean expected" part of the error message?

>  else if (value)
> ! {
> ! boolvalid;
> ! boolnewval = ParseVariableBool(value, NULL, );
> ! if (valid)
> ! popt->topt.expanded = newval;
> ! else
> ! {
> ! psql_error("unrecognized value \"%s\" for \"%s\"\n",
> !value, param);
> ! return false;
> ! }
> ! }
> is really the hard way and you could have just done
> 
> - popt->topt.expanded = ParseVariableBool(value, param);
> + success = ParseVariableBool(value, param,
> >topt.expanded);

I get the idea, except that in this example, the compiler is
unhappy because popt->topt.expanded is not a bool, and an
explicit cast here would be kludgy.

For the error printing part, it would go away with the above 
suggestion


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] sequence data type

[Daniel Verite]

Peter Eisentraut wrote:

> So in order to correctly answer the question, is the sequence about to
> run out, you need to look not only at
> the sequence but also any columns it is associated with.  check_postgres
> figures this out, but it's complicated and slow, and not easy to do
> manually.

It strikes me that this is a compelling argument for setting a sensible
MAXVALUE when creating a sequence for a SERIAL, rather than binding
the sequence to a datatype.

In existing releases the SERIAL code sets the maxvalue to 2^63 even
though it knows that the column is limited to 2^31.
It looks like setting it to 2^31 would be enough for the sake of
monitoring.

More generally, what is of interest for the monitoring is how close 
the sequence's last_value is from its max_value, independently of an
underlying type.

2^{15,31,63} are specific cases of particular interest, but there's
no reason to only check for these limits when you can do it
for every possible limit.

For instance if a user has a business need to limit an ID to 1 billion,
they should alter the sequence to a MAXVALUE of 1 billion, and be
interested in how close they are from that, not from 2^31. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] sequence data type

[Daniel Verite]

Michael Paquier wrote:

> Hm. Is symmetry an important properly for sequences? It seems to me
> that if we map with the data types we had better use the MIN values
> instead for consistency. So the concept of this patch is rather weird
> and would introduce an exception with the rest of the system just for
> sequences.

Besides there's a related compatibility break in that,
if a sequence is created in an existing release like this:

CREATE SEQUENCE s MINVALUE -9223372036854775808;

And then it's dumped/reloaded on a backend that has
the patch applied, it fails with:

 MINVALUE (-9223372036854775808) is too large for sequence data type bigint

This value (-2^63) is legal in current releases, although it happens
to be off-by-1 compared to the default minvalue for a sequence going
downwards. Arguably it's the default that is weird.

I've started the thread at [1] to discuss whether it makes sense 
in the first place that our CREATE SEQUENCE takes -(2^63)+1 as the
default minvalue rather than -2^63, independantly of this patch.

I think the current patch transforms this oddity into an actual
issue  by making it impossible to reach the real minimum of
a sequence with regard to the type tied to it (-2^15 for smallint,
-2^31 for int, -2^63 for bigint), whereas in HEAD you can still
adjust minvalue to fix the off-by-1 against the bigint range, if you
happen to care about it, the only problem being that you first
need to figure this out by yourself.


[1]
https://www.postgresql.org/message-id/4865a75e-f490-4e9b-b8e7-3d78694c3...@manitou-mail.org


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] sequence data type

[Daniel Verite]

Peter Eisentraut wrote:

> This could probably be sorted out somehow, but I don't want
> to be too lax now and cause problems for later features.  There is a
> similar case, namely changing the return type of a function, which we
> also prohibit.

Consider the case of a table with a SERIAL column which later
has to become a BIGINT due to growth.
Currently a user would just alter the column's type and does
need to do anything with the sequence.

With the patch, it becomes a problem because

- ALTER SEQUENCE seqname MAXVALUE new_value
will fail because new_value is beyond the range of INT4.

- ALTER SEQUENCE seqname TYPE BIGINT
does not exist (yet?)

- DROP SEQUENCE seqname  (with the idea of recreating the
sequence immediately after) will be rejected because the table
depends on the sequence.

What should a user do to upgrade the SERIAL column?

BTW, I notice that a direct UPDATE of pg_sequence happens
to work (now that we have pg_sequence thanks to your other
recent contributions on sequences), but I guess it falls under the
rule mentioned in 
https://www.postgresql.org/docs/devel/static/catalogs.html

"You can drop and recreate the tables, add columns, insert and update values,
and severely mess up your system that way. Normally, one should not change
the system catalogs by hand, there are normally SQL commands to do that"

Previously, UPDATE seqname SET property=value was rejected with
a specific error "cannot change sequence".

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Alvaro Herrera wrote:

> With that, pushed and I hope this is closed for good.

Great!
I understand the fix couldn't be backpatched because
we are not allowed to change the StringInfo struct in
existing releases. But it occurs to me that the new "long_ok"
flag might not be necessary after all now that it's settled that
the length remains an int32.
The flag is used to enforce a rule that the string can't normally grow
past 1GB, and can reach 2GB only as an exception that we choose to
exercise for COPY starting with v10.
But that 1GB rule is never mentioned in stringinfo.[ch] AFAICS.
I know that 1GB is the varlena size and is a limit for various things,
but I don't know to what extent StringInfo is concerned by that.

Is it the case that users of StringInfo in existing releases
and extensions are counting on its allocator to fail and abort
if the buffer grows over the varlena range?

If it's true, then we're stuck with the current situation
for existing releases.
OTOH if this seems like a nonlegit expectation, couldn't we say that
the size limit is 2^31 for all, and suppress the flag introduced by
the fix?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

[HACKERS] Off-by-one oddity in minval for decreasing sequences

[Daniel Verite]

 Hi,

When testing the patch at https://commitfest.postgresql.org/12/768/
("sequence data type" by Peter E.), I notice that there's a preexisting
oddity in the fact that sequences created with a negative increment
in current releases initialize the minval to -(2^63)+1 instead of -2^63,
the actual lowest value for a bigint.

postgres=# CREATE SEQUENCE s INCREMENT BY -1;
CREATE SEQUENCE

postgres=# SELECT seqmin,seqmin+pow(2::numeric,63)
   FROM pg_sequence where seqrelid='s'::regclass;
seqmin|  ?column?  
--+
 -9223372036854775807 | 1.

But it's still possible to set it to -2^63 manually either by
altering the sequence or by specifying it explicitly at CREATE time
with CREATE SEQUENCE s MINVALUE -9223372036854775808
so it's inconsistent with the potential argument that we couldn't
store this value for some reason.

postgres=# ALTER SEQUENCE s minvalue -9223372036854775808;
ALTER SEQUENCE
postgres=# select seqmin,seqmin+pow(2::numeric,63)
   from pg_sequence where seqrelid='s'::regclass;
seqmin|  ?column?  
--+
 -9223372036854775808 | 0.


The defaults comes from these definitions, in include/pg_config_manual.h

/*
 * Set the upper and lower bounds of sequence values.
 */
#define SEQ_MAXVALUEPG_INT64_MAX
#define SEQ_MINVALUE(-SEQ_MAXVALUE)

with no comment as to why SEQ_MINVALUE is not PG_INT64_MIN.

When using other types than bigint, Peter's patch fixes the inconsistency
but also makes it worse by ISTM applying the rule that the lowest value
is forbidden for int2 and int4 in addition to int8.

I'd like to suggest that we don't do that starting with HEAD, by
setting seqmin to the real minimum of the supported range, because
wasting that particular value seems silly and a hazard if
someone wants to use a sequence to store any integer
as opposed to just calling nextval().

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Rahila Syed wrote:

> Kindly consider following comments,

Sorry for taking so long to post an update.

> There should not be an option to the caller to not follow the behaviour of
> setting valid to either true/false.

OK, fixed.

> In following examples, incorrect error message is begin displayed.
> “ON_ERROR_ROLLBACK” is an enum and also
> accepts value 'interactive' .  The error message says boolean expected.

Indeed. Fixed for all callers of ParseVariableBool() than can accept 
non-boolean arguments too.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..46bcf19 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,25 +254,30 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, ) ?
TRI_YES : TRI_NO;
-
-   free(opt1);
-   opt1 = read_connect_arg(scan_state);
+   if (success)
+   {
+   free(opt1);
+   opt1 = read_connect_arg(scan_state);
+   }
}
else
reuse_previous = TRI_DEFAULT;
 
-   opt2 = read_connect_arg(scan_state);
-   opt3 = read_connect_arg(scan_state);
-   opt4 = read_connect_arg(scan_state);
+   if (success)/* do not attempt to connect if 
reuse_previous argument was invalid */
+   {
+   opt2 = read_connect_arg(scan_state);
+   opt3 = read_connect_arg(scan_state);
+   opt4 = read_connect_arg(scan_state);
 
-   success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+   success = do_connect(reuse_previous, opt1, opt2, opt3, 
opt4);
 
+   free(opt2);
+   free(opt3);
+   free(opt4);
+   }
free(opt1);
-   free(opt2);
-   free(opt3);
-   free(opt4);
}
 
/* \cd */
@@ -1548,7 +1553,11 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   {
+   boolnewval = ParseVariableBool(opt, "\\timing", 
);
+   if (success)
+   pset.timing = newval;
+   }
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2669,18 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   {
+   boolvalid;
+   boolnewval = ParseVariableBool(value, NULL, );
+   if (valid)
+   popt->topt.expanded = newval;
+   else
+   {
+   psql_error("unrecognized value \"%s\" for 
\"%s\"\n",
+  value, param);
+   return false;
+   }
+   }
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2689,14 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
-   popt->topt.numericLocale = ParseVariableBool(value, 
param);
+   {
+   boolvalid;
+   boolnewval = ParseVariableBool(value, param, 
);
+   if (valid)
+   popt->topt.numericLocale = newval;
+   else
+   return false;
+   }
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2751,18 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)

Re: [HACKERS] [GENERAL] Select works only when connected from login postgres

[Daniel Verite]

Tom Lane wrote:

> BTW, I realized while testing this that there's still one gap in our
> understanding of what went wrong for you: cases like "SELECT 'hello'"
> should not have tried to use the pager, because that would've produced
> less than a screenful of data

At some point emacs was mentioned as the terminal:

>> And I guess I did that intentionally, my .bashrc has
>>
>>   # I use emacs shells, I got a "pager" already:
>>   export PAGER=''

The M-x shell mode of emacs has a so-called "dumb" terminal
emulation (that's the value of $TERM) where the notion of a "page"
doesn't quite apply.

For instance, when using emacs 24.3 with my default pager on an
Ubuntu desktop, this is what I get:

test=> select 1;
WARNING: terminal is not fully functional
-  (press RETURN)
 ?column? 
--
1
(1 row)

I suspect that psql is unable to determine the screen size
of the "dumb" terminal, and that it's the fault of the terminal
rather than psql.
The warning is displayed by "less" AFAICS.

There are other psql features like tab-completion that don't work
in this mode because emacs interpret keystrokes first for
itself, in effect mixing emacs functionalities with these of the
application run in the terminal. It's awesome sometimes
and irritating at other times depending on what you expect :)

OTOH it has also a M-x term command/mode that provides a
more sophisticated screen emulation into which paging seems
to work exactly like in a normal terminal and the emacs key bindings
are turned off. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Alvaro Herrera wrote:

> I have now pushed this to 9.5, 9.6 and master.  It could be backpatched
> to 9.4 with ease (just a small change in heap_form_tuple); anything
> further back would require much more effort.
> 
> I used a 32-bit limit using sizeof(int32).  I tested and all the
> mentioned cases seem to work sanely; if you can spare some more time to
> test what was committed, I'd appreciate it.

My tests are OK too but I see an issue with the code in
enlargeStringInfo(), regarding integer overflow.
The bit of comment that says:

  Note we are assuming here that limit <= INT_MAX/2, else the above
  loop could overflow.

is obsolete, it's now INT_MAX instead of INT_MAX/2.

There's a related problem here:
newlen = 2 * str->maxlen;
while (needed > newlen)
newlen = 2 * newlen;
str->maxlen is an int going up to INT_MAX so [2 * str->maxlen] now
*will* overflow when [str->maxlen > INT_MAX/2].
Eventually it somehow works because of this:
if (newlen > limit)
newlen = limit;
but newlen is wonky (when resulting from int overflow)
before being brought back to limit.

PFA a minimal fix.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index b618b37..b01afbe 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -313,14 +313,13 @@ enlargeStringInfo(StringInfo str, int needed)
 	 * for efficiency, double the buffer size each time it overflows.
 	 * Actually, we might need to more than double it if 'needed' is big...
 	 */
-	newlen = 2 * str->maxlen;
+	newlen = 2 * (Size)str->maxlen;		/* avoid integer overflow */
 	while (needed > newlen)
 		newlen = 2 * newlen;
 
 	/*
-	 * Clamp to the limit in case we went past it.  Note we are assuming here
-	 * that limit <= INT_MAX/2, else the above loop could overflow.  We will
-	 * still have newlen >= needed.
+	 * Clamp to the limit in case we went past it. We will still have
+	 * newlen >= needed.
 	 */
 	if (newlen > limit)
 		newlen = limit;

-- 
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] pgbench - allow backslash continuations in \set expressions

[Daniel Verite]

Fabien COELHO wrote:

>  - if not, are possible corner case backward incompatibilities introduced
>by such feature ok?

In psql, if backslash followed by [CR]LF is interpreted as a
continuation symbol, commands like these seem problematic
on Windows since backslash is the directory separator:

\cd \
\cd c:\somepath\

Shell invocations also come to mind:
\! dir \


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Alvaro Herrera wrote:

> But I realized that doing it this way is simple enough;
> and furthermore, in any platforms where int is 8 bytes (ILP64), this
> would automatically allow for 63-bit-sized stringinfos

On such platforms there's the next problem that we can't
send COPY rows through the wire protocol when they're larger
than 2GB.

Based on the tests with previous iterations of the patch that used
int64 for the StringInfo length, the COPY backend code does not
gracefully fail in that case.

What happened when trying (linux x86_64) with a 2GB-4GB row
is that the size seems to overflow and is sent as a 32-bit integer
with the MSB set, which is confusing for the client side (at least
libpq cannot deal with it).

If we consider what would happen with the latest patch on a platform
with sizeof(int)=8 and a \copy invocation like this:

\copy (select big,big,big,big,big,big,big,big,.. FROM
(select lpad('', 1024*1024*200) as big) s) TO /dev/null

if we put enough copies of "big" in the select-list to grow over 2GB,
and then over 4GB.

On a platform with sizeof(int)=4 this should normally fail over 2GB with
"Cannot enlarge string buffer containing $X bytes by $Y more bytes"

I don't have an ILP64 environment myself to test, but I suspect
it would malfunction instead of cleanly erroring out on such
platforms.

One advantage of hardcoding the StringInfo limit to 2GB independantly
of sizeof(int) is to basically avoid the problem.

Also, without this limit, we can "COPY FROM/TO file" really huge rows, 4GB
and beyond, like I tried successfully during the tests mentioned upthread
(again with len as int64 on x86_64).
So such COPYs would succeed or fail depending on whether they deal with
a file or a network connection.
Do we want this difference in behavior?
(keeping in mind that they will both fail anyway if any individual field
in the row is larger than 1GB)


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Alvaro Herrera wrote:

> I propose to rename allow_long to huge_ok.  "Huge" is the terminology
> used by palloc anyway.  I'd keep makeLongStringInfo() and
> initLongStringInfo() though as interface, because using Huge instead of
> Long there looks strange.  Not wedded to that, though (particularly as
> it's a bit inconsistent).

"Long" makes sense to me as qualifying a limit greater than
MaxAllocSize but lower (or equal) than MaxAllocHugeSize.

In memutils.h we have these definitions:

#define MaxAllocSize   ((Size) 0x3fff)   /* 1 gigabyte - 1 */
#define MaxAllocHugeSize   ((Size) -1 >> 1) /* SIZE_MAX / 2 */

And in enlargeStringInfo() the patch adds this:
/*
 * Maximum size for strings with allow_long=true.
 * It must not exceed INT_MAX, as the StringInfo routines
 * expect offsets into the buffer to fit into an int.
 */
const int max_alloc_long = 0x7fff;

On a 32-bit arch, we can expect max_alloc_long == MaxAllocHugeSize,
but on a 64-bit arch, it will be much smaller with MaxAllocHugeSize
at (2^63)-1.

IOW, the patch only doubles the maximum size of StringInfo
whereas we could say that it should multiply it by 2^32 to pretend to
the "huge" qualifier.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

   I wrote:

> So I went through the psql commands which don't fail on parse errors
> for booleans
> [...]

Here's a v5 patch implementing the suggestions mentioned upthread:
all meta-commands calling ParseVariableBool() now fail
when the boolean argument can't be parsed successfully.

Also includes a minor change to SetVariableAssignHook() that now
returns the result of the hook it calls after installing it.
It doesn't make any difference in psql behavior since callers
of SetVariableAssignHook() ignore its return value, but it's
more consistent with SetVariable().

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..356467b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,25 +254,30 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, ) ?
TRI_YES : TRI_NO;
-
-   free(opt1);
-   opt1 = read_connect_arg(scan_state);
+   if (success)
+   {
+   free(opt1);
+   opt1 = read_connect_arg(scan_state);
+   }
}
else
reuse_previous = TRI_DEFAULT;
 
-   opt2 = read_connect_arg(scan_state);
-   opt3 = read_connect_arg(scan_state);
-   opt4 = read_connect_arg(scan_state);
+   if (success)/* do not attempt to connect if 
reuse_previous argument was invalid */
+   {
+   opt2 = read_connect_arg(scan_state);
+   opt3 = read_connect_arg(scan_state);
+   opt4 = read_connect_arg(scan_state);
 
-   success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+   success = do_connect(reuse_previous, opt1, opt2, opt3, 
opt4);
 
+   free(opt2);
+   free(opt3);
+   free(opt4);
+   }
free(opt1);
-   free(opt2);
-   free(opt3);
-   free(opt4);
}
 
/* \cd */
@@ -1548,7 +1553,11 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   {
+   boolnewval = ParseVariableBool(opt, "\\timing", 
);
+   if (success)
+   pset.timing = newval;
+   }
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2669,14 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   {
+   boolvalid;
+   boolnewval = ParseVariableBool(value, param, 
);
+   if (valid)
+   popt->topt.expanded = newval;
+   else
+   return false;
+   }
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2668,8 +2684,16 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
/* locale-aware numeric output */
else if (strcmp(param, "numericlocale") == 0)
{
+
if (value)
-   popt->topt.numericLocale = ParseVariableBool(value, 
param);
+   {
+   boolvalid;
+   boolnewval = ParseVariableBool(value, param, 
);
+   if (valid)
+   popt->topt.numericLocale = newval;
+   else
+   return false;
+   }
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2748,14 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
-   popt->topt.tuples_only = ParseVariableBool(value, 
param);
+   {
+

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Stephen Frost wrote:

> That certainly doesn't feel right.  I'm thinking that if we're going to
> throw an error back to the user about a value being invalid then we
> shouldn't change the current value.
> 
> My initial thought was that perhaps we should pass the current value to
> ParseVariableBool() and let it return whatever the "right" answer is,
> however, your use of ParseVariableBool() for enums that happen to accept
> on/off means that won't really work.

That's not needed once ParseVariableBool() informs the caller about
the validity of the boolean expression, which is what the patch already
does.

For instance I just implemented it for \timing and the diff consists of just
that:

if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing",
NULL);
+   {
+   boolnewval = ParseVariableBool(opt, "\\timing",
);
+   if (success)
+   pset.timing = newval;
+   }
else
pset.timing = !pset.timing;

That makes \timing foobar being rejected as a bad command with a
proper error message and no change of state, which is just what we want.

> Perhaps the right answer is to flip this around a bit and treat booleans
> as a special case of enums and have a generic solution for enums.
> Consider something like:
> 
> ParseVariableEnum(valid_enums, str_value, name, curr_value);
> 
> 'valid_enums' would be a simple list of what the valid values are for a
> given variable and their corresponding value, str_value the string the
> user typed, name the name of the variable, and curr_value the current
> value of the variable.

Firstly I'd like to insist that such a refactoring is not necessary
for this patch and I feel like it would be out of place in it.

That being said, if we wanted this, I think it would be successful
only if we'd first change our internal variables pset.* from a struct 
of different types to a list of variables from some kind of common
abstract type and an abstraction layer to access them.
That would be an order of magnitude more sophisticated than what we
have.

Otherwise as I tried to explain in [1], I don't see how we could write
a ParseVariableEnum() that would return different types
and take variable inputs.
Or if we say that ParseVariableEnum should not return the value
but affect the variable directly, that would require refactoring
all call sites, and what's the benefit that would justify
such large changes?
Plus we have two different non-mergeable concepts of variables
that need this parser:
psql variables from VariableSpace stored as strings,
and C variables directly instantiated as native types.


Also, the argument that bools are just another type of enums
is legitimate in theory, but as in psql we accept any left-anchored
match of true/false/on/off/0/1, it means that the enumeration
of values is in fact:

0
1
t
tr
tru
true
f
fa
fal
fals
false
on
of
off

I don't see that it would help if the code treated the above like just a
vanilla list of values, comparable to the other qualifiers like "auto",
"expanded", "vertical", an so on, notwithstanding the fact
that they don't share the same types.

I think that the current code with ParseVariableBool() that only
handles booleans is better in terms of separation of concerns
and readability.

[1]
https://www.postgresql.org/message-id/fc879967-da93-43b6-aa5a-92f2d825e786@mm

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Stephen Frost wrote:

> Are you working to make those changes?  I'd rather we make the changes
> to this code once rather than push what you have now only to turn around
> and change it significantly again.

So I went through the psql commands which don't fail on parse errors
for booleans. I'd like to attract attention on \c, because I see
several options.

\c [-reuse-previous=BOOL] ...other args..

Current: if we can't parse BOOL, assume it's ON, and go on with reconnecting.

Option1: if we can't parse BOOL, stop here, don't reconnect, set
the command result as "failed", also ignoring the other arguments.

Option2: maybe we want to create a difference between interactive
and non-interactive use, as there's already one in handling
the failure to connect through \c.
The manpage says:
   If the connection attempt failed (wrong user name, access denied,
   etc.), the previous connection will only be kept if psql is in
   interactive mode. When executing a non-interactive script,
   processing will immediately stop with an error.

Proposal: if interactive, same as Option1.
If not interactive, -reuse-previous=bogus has the same outcome
as a failure to connect. Which I think means two subcases:
if it's through \i then kill the connection but don't quit psql
if it's through -f then quit psql.

Option3: leave this command unchanged, avoiding trouble.

\timing BOOL

Current: non-parseable BOOL interpreted as TRUE. Empty BOOL toggles the
state.

Proposal: on non-parseable BOOL, command failure and pset.timing is 
left unchanged.

\pset [x | expanded | vertical ] BOOL
\pset numericlocale BOOL
\pset [tuples_only | t] BOOL
\pset footer BOOL
\t BOOL (falls into pset_do("tuples_only", ...))
\pset pager BOOL

Current: non-parseable non-empty BOOL interpreted as TRUE. Empty BOOL
toggles the on/off state. In some cases, BOOL interpretation is attempted
only after specific built-in values have been ruled out first.

Proposal: non-parseable BOOL implies command failure and unchanged state.

About the empty string when a BOOL is expected. Only \c -reuse-previous
seems concerned:

#= \c -reuse-previous=
acts the same as
#= \c -reuse-previous=ON

Proposal: handle empty as when the value is bogus.

The other commands interpret this lack of value specifically to toggle
the state, so it's a non-issue for them.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Stephen Frost wrote:

> Are you working to make those changes?  I'd rather we make the changes
> to this code once rather than push what you have now only to turn around
> and change it significantly again.

If, as a maintainer, you prefer this together in one patch, I'm fine
with that. So I'll update it shortly with changes in \timing and
a few other callers of ParseVariableBool().

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Stephen Frost wrote:

> Just fyi, there seems to be some issues with this patch because setting
> my PROMPT1 and PROMPT2 variables result in rather odd behavior.

Good catch! The issue is that the patch broke the assumption
of prompt hooks that their argument points to a long-lived buffer.
The attached v4 fixes the bug by passing to hooks a pointer to the final
strdup'ed value in VariableSpace rather than temp space, as commented
in SetVariable().

Also I've changed something else in ParseVariableBool(). The code assumes
"false" when value==NULL, but when value is an empty string, the result
was true and considered valid, due to the following test being
positive when len==0 (both with HEAD or the v3 patch from this thread):

if (pg_strncasecmp(value, "true", len) == 0)
return true;
It happens that "" as a value yields the same result than "true" for this
test so it passes, but it seems rather unintentional.

The resulting problem from the POV of the user is that we can do that,
for instance:

   test=> \set AUTOCOMMIT

without error message or feedback, and that leaves us without much
clue about autocommit being enabled:

   test=> \echo :AUTOCOMMIT

   test=> 

So I've changed ParseVariableBool() in v4 to reject empty string as an
invalid boolean (but not NULL since the startup logic requires NULL
meaning false in the early initialization of these variables).

"make check" seems OK with that, I hope it doesn't cause any regression
elsewhere.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..61b2304 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,7 +254,7 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, NULL) ?
TRI_YES : TRI_NO;
 
free(opt1);
@@ -1548,7 +1548,7 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   pset.timing = ParseVariableBool(opt, "\\timing", NULL);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2660,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   popt->topt.expanded = ParseVariableBool(value, param, 
NULL);
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2669,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
-   popt->topt.numericLocale = ParseVariableBool(value, 
param);
+   popt->topt.numericLocale = ParseVariableBool(value, 
param, NULL);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2724,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
-   popt->topt.tuples_only = ParseVariableBool(value, 
param);
+   popt->topt.tuples_only = ParseVariableBool(value, 
param, NULL);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
@@ -2756,7 +2756,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
popt->topt.pager = 2;
else if (value)
{
-   if (ParseVariableBool(value, param))
+   if (ParseVariableBool(value, param, NULL))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
@@ -2778,7 +2778,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "footer") == 0)
{
if (value)
-   popt->topt.default_footer = ParseVariableBool(value, 
param);
+   popt->topt.default_footer = ParseVariableBool(value, 
param, NULL);
  

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Tom Lane wrote:

> Stephen Frost  writes:
> > In reviewing this patch, I also noticed that it's set up to assume a
> > 'true' result when a variable can't be parsed by ParseVariableBool.
> 
> I suppose that's meant to be backwards-compatible with the current
> behavior:
> 
> regression=# \timing foo
> unrecognized value "foo" for "\timing"; assuming "on"
> Timing is on.

Exactly. The scope of the patch is limited to the effect
of \set assignments to built-in variables.

> but I agree that if we're changing things in this area, that would
> be high on my list of things to change.  I think what we want going
> forward is to disallow setting "special" variables to invalid values,
> and that should hold both for regular variables that have special
> behaviors, and for the special-syntax cases like \timing.

+1

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] proposal: psql \setfileref

[Daniel Verite]

Corey Huinker wrote:

> I am not asking for this feature now, but do you see any barriers to later
> adding x/xq/xb/xbq equivalents for executing a shell command?

For text contents, we already have this (pasted right from the doc):

testdb=> \set content `cat my_file.txt`
testdb=> INSERT INTO my_table VALUES (:'content');

Maybe we might look at why it doesn't work for binary string and fix that?

I can think of three reasons:

- psql use C-style '\0' terminated string implying no nul byte in variables.
That could potentially be fixed by tweaking the data structures and
accessors.

- the backtick operator trims ending '\n' from the result of the command
(like the shell), but we could add a variant that doesn't have this behavior.

- we don't have "interpolate as binary", an operator that will
essentially apply PQescapeByteaConn rather than PQescapeStringConn.

For example, I'd suggest this syntax:

-- slurp a file into a variable, with no translation whatsoever
\set content ``cat my_binary_file``

-- interpolate as binary (backquotes instead of quotes)
INSERT INTO my_table VALUES(:`content`);

If we had something like this, would we still need filerefs? I feel like
the point of filerefs is mainly to work around lack of support for
binary in variables, but maybe supporting the latter directly would
be an easier change to accept.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

  Hi,

I'm attaching v3 of the patch with error reporting for
FETCH_COUNT as mentioned upthread, and rebased
on the most recent master.

> I was suggesting change on the lines of extending generic_boolean_hook to
> include enum(part in enum_hooks which calls ParseVariableBool) and
> integer parsing as well.

Well, generic_boolean_hook() is meant to change this, for instance:

  static void
  on_error_stop_hook(const char *newval)
  {
 pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
  }

into that:

  static bool
  on_error_stop_hook(const char *newval)
  {
 return generic_boolean_hook(newval, "ON_ERROR_STOP",
   _error_stop);
   }

with the goal that the assignment does not occur if "newval" is bogus.
The change is really minimal.

When we're dealing with enum-or-bool variables, such as for instance
ECHO_HIDDEN, the patch replaces this:

  static void
  echo_hidden_hook(const char *newval)
  {
if (newval == NULL)
  pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
else if (pg_strcasecmp(newval, "noexec") == 0)
  pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
  pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
else  /* ParseVariableBool printed msg if needed */
  pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
  }

with that:

  static bool
  echo_hidden_hook(const char *newval)
  {
if (newval == NULL)
  pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
else if (pg_strcasecmp(newval, "noexec") == 0)
  pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
else
{
  bool isvalid;
  bool val = ParseVariableBool(newval, "ECHO_HIDDEN", );
  if (!isvalid)
return false; /* ParseVariableBool printed msg */
  pset.echo_hidden = val ? PSQL_ECHO_HIDDEN_ON : PSQL_ECHO_HIDDEN_OFF;
}
return true;
  }

The goal being again to reject a bogus assignment, as opposed to replacing
it with any hardwired value.
Code-wise, we can't call generic_boolean_hook() here because we need
to assign a non-boolean specific value after having parsed the ON/OFF
user-supplied string.

More generally, it turns out that the majority of hooks are concerned
by this patch, as they parse user-supplied values, but there
are 4 distinct categories of variables:

1- purely ON/OFF vars:
   AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP

2- ON/OFF mixed with enum values:
  ECHO_HIDDEN, ON_ERROR_ROLLBACK

3- purely enum values:
  COMP_KEYWORD_CASE, HISTCONTROL, ECHO, VERBOSITY, SHOW_CONTEXT

4- numeric values:
  FETCH_COUNT

If you suggest that the patch should refactor the implementation
of hooks for case #2, only two hooks are concerned and they consist
of non-mergeable enum-specific code interleaved with generic code,
so I don't foresee any gain in fusioning. I have the same opinion about
merging any of #1, #2, #3, #4 together.
But feel free to post code implementing your idea if you disagree,
maybe I just don't figure out what you have in mind.
For case #3, these hooks clearly follow a common pattern, but I also
don't see any benefit in an opportunistic rewrite given the nature of
the functions.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..61b2304 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,7 +254,7 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, NULL) ?
TRI_YES : TRI_NO;
 
free(opt1);
@@ -1548,7 +1548,7 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   pset.timing = ParseVariableBool(opt, "\\timing", NULL);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2660,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   popt->topt.expanded = ParseVariableBool(value, param, 
NULL);
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2669,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
  

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Rahila Syed wrote:

> I have applied this patch on latest HEAD and have done basic testing which
> works fine.

Thanks for reviewing this patch!

> >if (current->assign_hook)
> >-   (*current->assign_hook) (current->value);
> >-   return true;
> >+   {
> >+   confirmed = (*current->assign_hook) (value);
> >+   }
> >+   if (confirmed)
> 
> Spurious brackets

OK.

> >static bool
> >+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
> >+{
> 
> Contrary to what name suggests this function does not seem to have other
> implementations as in a hook.
> Also this takes care of rejecting a syntactically wrong value only for
> boolean variable hooks like autocommit_hook,
> on_error_stop_hook. However, there are other variable hooks which call
> ParseVariableBool.
> For instance, echo_hidden_hook which is handled separately in the patch.
> Thus there is some duplication of code between generic_boolean_hook and
> echo_hidden_hook.
> Similarly for on_error_rollback_hook.

The purpose of generic_boolean_hook() is to handle the case of a
boolean variable that only accepts ON or OFF, and has its pset.varname
declared as bool. I thought of this case as "generic" because that's
the base case and several variables need no more than that.

ECHO_HIDDEN differs from the generic boolean case because it also
accepts "noexec" and pset.echo_hidden is an enum, not a boolean. When
considering refactoring echo_hidden_hook() to call
generic_boolean_hook() instead of ParseVariableBool() after 
having established that the value is not "noexec", I don't see
any advantage in clarity or code size, so I'm not in favor of that change.

The same applies to on_error_rollback_hook(), which has to deal
with a specific enum as well.

> >-static void
> >+static bool
> > fetch_count_hook(const char *newval)
> > {
> >pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
> >+   return true;
> > }
> 
> Shouldn't invalid numeric string assignment for numeric variables be
> handled too?

Agreed. Assignments like "\set FETCH_COUNT bogus" don't provoke any
user feedback currently, which is not ideal. I'll add this in a
v3 of the patch tomorrow.

> Instead of generic_boolean_hook cant we have something like follows which
> like generic_boolean_hook can be called from specific variable assignment
> hooks,
> 
> static bool
> ParseVariable(newval, VariableName, )
> {
> if (VariableName == ‘AUTOCOMMIT’ || ECHO_HIDDEN || other variable with
> hooks which call ParseVariableBool )
>   ON_ERROR_ROLLBACK>
> else if (VariableName == ‘FETCH_COUNT’)
> ParseVariableNum();
> }

It's not possible because pset.var corresponds to different fields from
struct _psqlSettings that have different types: bool, int and some
enum types.
Besides, I don't think it would go well with hooks. If we wanted one
big function that knows all about parsing all built-in variables, we
could just as well dispense with hooks, since their current purpose in
psql is to achieve this parsing, but in a decentralized way.
Or if we keep them, our various built-in variables would be
essentially tied to the same one-big-hook-that-does-all, but isn't
that an antipattern for hooks?


> >@@ -260,7 +276,7 @@ SetVariableAssignHook(VariableSpace space, const char
> *name, VariableAssignHook
> >current->assign_hook = hook;
> >current->next = NULL;
> >previous->next = current;
> >-   (*hook) (NULL);
> >+   (void)(*hook) (NULL);   /* ignore return value */
> 
> Sorry for my lack of understanding, can you explain why is above change
> needed?

"hook" is changed by the patch from [pointer to function returning
void], to [pointer to function returning bool]. The cast to void is
not essential, it just indicates that we deliberately want to
ignore the return value here. I expect some compilers might
complain under a high level of warnings without this cast, although
TBH if you ask me, I wouldn't know which compiler with which flags
exactly.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] [PATCH] Better logging of COPY queries if log_statement='all'

[Daniel Verite]

Aleksander Alekseev wrote:

> According to my colleagues it would be very nice to have this feature.
> For instance, if you are trying to optimize PostgreSQL for application
> that uses COPY and you don't have access to or something like this. 
> It could also be useful in some other cases.

Outside of the app, what can be already set up is an AFTER INSERT
FOR EACH ROW trigger that essentially does:

  raise LOG, '%', NEW;

The main drawback of this approach is that, for each line of data
emitted to the log, there's another STATEMENT: copy... line added.
But that might be not too bad for certain uses.

Ideally we should be able to access the new rowset as a whole through
a statement-level trigger. In that case, the data could be logged in
a one-shot operation by that trigger.
There's a related item in the TODO list:
  "Allow statement-level triggers to access modified rows"
and an old thread on -hackers here:
https://www.postgresql.org/message-id/20060522150647.ge24...@svana.org
that discussed this topic in relation to MSSQL having this functionality.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] proposal: psql \setfileref

[Daniel Verite]

Gilles Darold wrote:

>postgres=# \setfileref b /dev/random
>postgres=# insert into test (:b);
> 
> Process need to be killed using SIGTERM.

This can be fixed by setting sigint_interrupt_enabled to true
before operating on the file. Then ctrl-C would be able to cancel
the command.

See comment in common.c, above the declaration of 
sigint_interrupt_enabled:

/*
 []
 * SIGINT is supposed to abort all long-running psql operations, not only
 * database queries.  In most places, this is accomplished by checking
 * cancel_pressed during long-running loops.  However, that won't work when
 * blocked on user input (in readline() or fgets()).  In those places, we
 * set sigint_interrupt_enabled TRUE while blocked, instructing the signal
 * catcher to longjmp through sigint_interrupt_jmp.  We assume readline and
 * fgets are coded to handle possible interruption.  (XXX currently this does
 * not work on win32, so control-C is less useful there)
 */


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Craig Ringer wrote:

> I think it's mostly of interest to app authors and driver developers
> and that's what it's aimed at. pg_restore could benefit a lot too.

Wouldn't pgbench benefit from it?
It was mentioned some time ago [1], in relationship to the 
\into construct, how client-server latency was important enough to
justify the use of a "\;" separator between statements, to send them
as a group.

But with the libpq batch API, maybe this could be modernized
with meta-commands like this:
  \startbatch
  ...
  \endbatch
which would essentially call PQbeginBatchMode() and PQsendEndBatch().
Inside the batch section, collecting results would be interleaved
with sending queries.
Interdepencies between results and subsequent queries could
be handled or ignored, depending on how sophisticated we'd want
this.

This might also draw more users to the batch API, because
it would make it easier to check how exactly it affects
the performance of specific sequences of SQL statements to be
grouped in a batch.
For instance it would make sense for programmers to benchmark mock-ups
of their code with pgbench with/without batching, before embarking on
adapting it from blocking mode to asynchronous/non-blocking mode.


[1]
https://www.postgresql.org/message-id/alpine.DEB.2.20.1607140925400.1962%40sto

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Tomas Vondra wrote:

> A few minor comments regarding the patch:
> 
> 1) CopyStartSend seems pretty pointless - It only has one function call 
> in it, and is called on exactly one place (and all other places simply 
> call allowLongStringInfo directly). I'd get rid of this function and 
> replace the call in CopyOneRowTo(() with allowLongStringInfo().
> 
> 2) allowlong seems awkward, allowLong or allow_long would be better
> 
> 3) Why does resetStringInfo reset the allowLong flag? Are there any 
> cases when we want/need to forget the flag value? I don't think so, so 
> let's simply not reset it and get rid of the allowLongStringInfo() 
> calls. Maybe it'd be better to invent a new makeLongStringInfo() method 
> instead, which would make it clear that the flag value is permanent.
> 
> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log 
> message, but with '%d' and not '%ld' (as needed after changing the type 
> to Size).
>
> 5) The comment at allowLongStringInfo talks about allocLongStringInfo 
> (i.e. wrong function name).

Here's an updated patch. Compared to the previous version:

- removed CopyStartSend (per comment #1 in review)

- renamed flag to allow_long (comment #2)

- resetStringInfo no longer resets the flag (comment #3).

- allowLongStringInfo() is removed (comment #3 and #5),
makeLongStringInfo() and initLongStringInfo() are provided
instead, as alternatives to makeStringInfo() and initStringInfo().

- StringInfoData.len is back to int type, 2GB max.
(addresses comment #4 incidentally).
This is safer because  many routines peeking
into StringInfoData use int for offsets into the buffer,
for instance most of the stuff in backend/libpq/pqformat.c
Altough these routines are not supposed to be called on long
buffers, this assertion was not enforced in the code, and
with a 64-bit length effectively over 2GB, they would misbehave
pretty badly.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
diff --git a/src/backend/access/common/heaptuple.c 
b/src/backend/access/common/heaptuple.c
index 6d0f3f3..ed9cabd 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -741,7 +741,9 @@ heap_form_tuple(TupleDesc tupleDescriptor,
 * Allocate and zero the space needed.  Note that the tuple body and
 * HeapTupleData management structure are allocated in one chunk.
 */
-   tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + len);
+   tuple = MemoryContextAllocExtended(CurrentMemoryContext,
+  
HEAPTUPLESIZE + len,
+  
MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
tuple->t_data = td = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
 
/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 432b0ca..7419362 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -397,7 +397,7 @@ ReceiveCopyBegin(CopyState cstate)
pq_sendint(, format, 2);/* per-column 
formats */
pq_endmessage();
cstate->copy_dest = COPY_NEW_FE;
-   cstate->fe_msgbuf = makeStringInfo();
+   cstate->fe_msgbuf = makeLongStringInfo();
}
else if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 2)
{
@@ -1892,7 +1892,7 @@ CopyTo(CopyState cstate)
cstate->null_print_client = cstate->null_print; /* default */
 
/* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
-   cstate->fe_msgbuf = makeStringInfo();
+   cstate->fe_msgbuf = makeLongStringInfo();
 
/* Get info about the columns we need to process. */
cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs * 
sizeof(FmgrInfo));
@@ -2707,8 +2707,8 @@ BeginCopyFrom(ParseState *pstate,
cstate->cur_attval = NULL;
 
/* Set up variables to avoid per-attribute overhead. */
-   initStringInfo(>attribute_buf);
-   initStringInfo(>line_buf);
+   initLongStringInfo(>attribute_buf);
+   initLongStringInfo(>line_buf);
cstate->line_buf_converted = false;
cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
cstate->raw_buf_index = cstate->raw_buf_len = 0;
diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index 7382e08..0125047 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -37,6 +37,24 @@ makeStringInfo(void)
 }
 
 /*
+ * makeLongStringInfo
+ *
+ * Same as makeStringInfo for larger strings.
+ */
+StringInfo
+makeLongStringInfo(void)
+{
+   StringInfo  res;
+
+   res = (StringInfo) palloc(sizeof(StringInfoData));
+
+   initLongStringInfo(res);
+
+   return res;
+}
+
+
+/*
  * initStringInfo
  *
  * Initialize a StringInfoData struct (with previously undefined contents)
@@ -47,12 +65,25 @@ 

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

[Daniel Verite]

Tomas Vondra wrote:

> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log 
> message, but with '%d' and not '%ld' (as needed after changing the type 
> to Size).

This could be %zu for the Size type. It's supported by elog().

However, it occurs to me that if we don't claim the 2GB..4GB range for
the CopyData message, because its Int32 length is not explicitly
unsigned as mentioned upthread, then it should follow that we don't
need to change StringInfo.{len,maxlen} from int type to Size type.

We should just set the upper limit for StringInfo.maxlen to
(0x7fff-1) instead of MaxAllocHugeSize for stringinfos with the
allow_long flag set, and leave the len and maxlen fields to their
original, int type.

Does that make sense?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] [PATCH] get_home_path: use HOME

[Daniel Verite]

Tom Lane wrote:

> If we take this patch, what's to stop someone from complaining that we
> broke *their* badly-designed system that abuses the HOME variable?

POSIX warns against doing that, listing HOME in the variables that
should be left to their intended usage:
http://pubs.opengroup.org/onlinepubs/9699919799/


  If the variables in the following two sections are present in the
  environment during the execution of an application or utility, they
  shall be given the meaning described below
  [...]
  HOME
  The system shall initialize this variable at the time of login to
  be a pathname of the user's home directory. See .


psql is indirectly using $HOME already for readline and terminfo:

$ HOME=/tmp/home2 strace psql 2>tr ; grep home2 tr
...
stat("/tmp/home2/.terminfo", 0x7ff985bf4730) = -1 ENOENT (No such file or
directory)
stat("/tmp/home2/.inputrc", 0x7fff3f641d70) = -1 ENOENT (No such file or
directory)

Also when using Debian's psql, the wrapper looks for it own config file in
$HOME:
open("/tmp/home2/.postgresqlrc", O_RDONLY) = -1 ENOENT (No such file or
directory)
Being written in Perl, it could use getpwuid(), but it doesn't, like I
believe
the majority of programs that just want the home directory.

+1 on using HOME for being consistent with other pieces of code around
postgres, and for the easiness of locally overriding it when
troubleshooting problems with dot files.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Ashutosh Bapat wrote:

> You may want to add this to the November commitfest
> https://commitfest.postgresql.org/11/.

Done. It's at https://commitfest.postgresql.org/11/799/

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Improvements in psql hooks for variables

[Daniel Verite]

Rahila Syed wrote:


> I am beginning to review this patch. Initial comment. I got following
> compilation error when I applied the patch on latest sources and run make.

Sorry about that, I forgot to make clean, here's an updated patch.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..61b2304 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,7 +254,7 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, NULL) ?
TRI_YES : TRI_NO;
 
free(opt1);
@@ -1548,7 +1548,7 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   pset.timing = ParseVariableBool(opt, "\\timing", NULL);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2660,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   popt->topt.expanded = ParseVariableBool(value, param, 
NULL);
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2669,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
-   popt->topt.numericLocale = ParseVariableBool(value, 
param);
+   popt->topt.numericLocale = ParseVariableBool(value, 
param, NULL);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2724,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
-   popt->topt.tuples_only = ParseVariableBool(value, 
param);
+   popt->topt.tuples_only = ParseVariableBool(value, 
param, NULL);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
@@ -2756,7 +2756,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
popt->topt.pager = 2;
else if (value)
{
-   if (ParseVariableBool(value, param))
+   if (ParseVariableBool(value, param, NULL))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
@@ -2778,7 +2778,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "footer") == 0)
{
if (value)
-   popt->topt.default_footer = ParseVariableBool(value, 
param);
+   popt->topt.default_footer = ParseVariableBool(value, 
param, NULL);
else
popt->topt.default_footer = !popt->topt.default_footer;
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7ce05fb..9dcfc0a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,59 @@ showVersion(void)
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
-static void
+/*
+ * Hook to set an internal flag from a user-supplied string value.
+ * If the syntax is correct, affect *flag and return true.
+ * Otherwise, keep *flag untouched and return false.
+ */
+static bool
+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
+{
+   bool isvalid;
+   bool val = ParseVariableBool(newval, varname, );
+   if (isvalid)
+   *flag = val;
+   return isvalid;
+}
+
+static bool
 autocommit_hook(const char *newval)
 {
-   pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+   return generic_boolean_hook(newval, "AUTOCOMMIT", );
 }
 
-static void
+static bool
 on_error_stop_hook(const char *newval)
 {
-   pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+   return generic_boolean_hook(newval, "ON_ERROR_STOP", 
_error_stop);
 }
 
-static 

[HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

 Hi,

Following the discussion on forbidding an AUTOCOMMIT off->on
switch mid-transaction [1], attached is a patch that let the hooks
return a boolean indicating whether a change is allowed.

Using the hooks, bogus assignments to built-in variables can
be dealt with more strictly.

For example, pre-patch behavior:

  =# \set ECHO errors
  =# \set ECHO on
  unrecognized value "on" for "ECHO"; assuming "none"
  =# \echo :ECHO
  on

which has two problems:
- we have to assume a value, even though we can't know what the user meant.
- after assignment, the user-visible value of the variable diverges from its
internal counterpart (pset.echo in this case).


Post-patch:
  =# \set ECHO errors
  =# \set ECHO on
  unrecognized value "on" for "ECHO"
  \set: error while setting variable
  =# \echo :ECHO
  errors

Both the internal pset.* state and the user-visible value are kept unchanged
is the input value is incorrect.

Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
a switch when the conditions are not met.


Another user-visible effect of the patch is that, using a bogus value
for a built-in variable on the command-line becomes a fatal error
that prevents psql to continue.
This is not directly intended by the patch but is a consequence
of SetVariable() failing.

Example:
  $ ./psql -vECHO=bogus
  unrecognized value "bogus" for "ECHO"
  psql: could not set variable "ECHO"
  $ echo $?
  1

The built-in vars concerned by the change are:

booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP

non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
 HISTCONTROL, VERBOSITY, SHOW_CONTEXT

We could go further to close the gap between pset.* and the built-in
variables,
by changing how they're initialized and forbidding deletion as Tom
suggests in [2], but if there's negative feedback on the above changes,
I think we should hear it first.

[1]
https://www.postgresql.org/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
[2]
https://www.postgresql.org/message-id/4695.1473961140%40sss.pgh.pa.us


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7ce05fb..9dcfc0a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,59 @@ showVersion(void)
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
-static void
+/*
+ * Hook to set an internal flag from a user-supplied string value.
+ * If the syntax is correct, affect *flag and return true.
+ * Otherwise, keep *flag untouched and return false.
+ */
+static bool
+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
+{
+   bool isvalid;
+   bool val = ParseVariableBool(newval, varname, );
+   if (isvalid)
+   *flag = val;
+   return isvalid;
+}
+
+static bool
 autocommit_hook(const char *newval)
 {
-   pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+   return generic_boolean_hook(newval, "AUTOCOMMIT", );
 }
 
-static void
+static bool
 on_error_stop_hook(const char *newval)
 {
-   pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+   return generic_boolean_hook(newval, "ON_ERROR_STOP", 
_error_stop);
 }
 
-static void
+static bool
 quiet_hook(const char *newval)
 {
-   pset.quiet = ParseVariableBool(newval, "QUIET");
+   return generic_boolean_hook(newval, "QUIET", );
 }
 
-static void
+static bool
 singleline_hook(const char *newval)
 {
-   pset.singleline = ParseVariableBool(newval, "SINGLELINE");
+   return generic_boolean_hook(newval, "SINGLELINE", );
 }
 
-static void
+static bool
 singlestep_hook(const char *newval)
 {
-   pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
+   return generic_boolean_hook(newval, "SINGLESTEP", );
 }
 
-static void
+static bool
 fetch_count_hook(const char *newval)
 {
pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
+   return true;
 }
 
-static void
+static bool
 echo_hook(const char *newval)
 {
if (newval == NULL)
@@ -837,39 +853,52 @@ echo_hook(const char *newval)
pset.echo = PSQL_ECHO_NONE;
else
{
-   psql_error("unrecognized value \"%s\" for \"%s\"; assuming 
\"%s\"\n",
-  newval, "ECHO", "none");
-   pset.echo = PSQL_ECHO_NONE;
+   psql_error("unrecognized value \"%s\" for \"%s\"\n",
+  newval, "ECHO");
+   return false;
}
+   return true;
 }
 
-static void
+static bool
 echo_hidden_hook(const char *newval)
 {
if (newval == NULL)
pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
else if (pg_strcasecmp(newval, "noexec") == 0)
pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
-   else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
-   pset.echo_hidden = 

Re: [HACKERS] Surprising behaviour of \set AUTOCOMMIT ON

[Daniel Verite]

Tom Lane wrote:

> I think changing the hook API is a pretty reasonable thing to do here
> (though I'd make it its own patch and then add the autocommit change
> on top).  When was the last time you actually wanted to set VERBOSITY
> to "fubar"?

It looks easy to make the hooks return a bool, and when returning false,
cancel the assignment of the variable.
I volunteer to write that patch.

It would close the hazard that exists today of an internal psql flag
getting decorrelated from the corresponding variable, due to feeding
it with a wrong value, or in the case of autocommit, in the wrong
context.

Also with that, the behavior of ParseVariableBool() assuming "on"
when it's being fed with junk could be changed. Instead it could just
reject the assignment rather than emit a warning, and both
the internal flag and the variable would keep the values they had
at the point of the bogus assignment.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Surprising behaviour of \set AUTOCOMMIT ON

[Daniel Verite]

Rahila Syed wrote:

> However, including the check here will require returning the status
> of command from this hook. i.e if we throw error inside this
> hook we will need to return false indicating the value has not changed.

Looking at the other variables hooks, they already emit errors and can
deny the effect of a change corresponding to a new value, without
informing the caller. Why would autocommit be different?

For example echo_hook does this:

/* ...if the value is in (queries,errors,all,none) then
   assign pset.echo accordingly ... */
else
{
  psql_error("unrecognized value \"%s\" for \"%s\"; assuming \"%s\"\n",
   newval, "ECHO", "none");
  pset.echo = PSQL_ECHO_NONE;
}


If the user issues \set ECHO FOOBAR, then it produces the above error
message and makes the same internal change as if \set ECHO none
had been issued.

But, the value of the variable as seen by the user is still FOOBAR:

\set
[...other variables...]
ECHO = 'FOOBAR'

The proposed patch doesn't follow that behavior, as it denies
a new value for AUTOCOMMIT. You might argue that it's better,
but on the other side, there are two reasons against it:

- it's not consistent with the other uses of \set that affect psql,
such as ECHO, ECHO_HIDDEN,  ON_ERROR_ROLLBACK,
 COMP_KEYWORD_CASE... and even AUTOCOMMIT as
 \set AUTOCOMMIT FOOBAR is not denied, just reinterpreted.

- it doesn't address the case of another method than \set being used
 to set the variable, as in : SELECT 'on' as "AUTOCOMMIT" \gset
 whereas the hook would work in that case.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] PATCH: Batch/pipelining support for libpq

[Daniel Verite]

Craig Ringer wrote:

> Updated patch attached.

Please find attached a couple fixes for typos I've came across in
the doc part.

Also it appears that PQqueriesInBatch() doesn't work as documented.
It says:
 "Returns the number of queries still in the queue for this batch"
but in fact it's implemented as a boolean:

+/* PQqueriesInBatch
+ *   Return true if there are queries currently pending in batch mode
+ */+int
+PQqueriesInBatch(PGconn *conn)
+{
+   if (!PQisInBatchMode(conn))
+   return false;
+
+   return conn->cmd_queue_head != NULL;
+}

However, is this function really needed? It doesn't seem essential to
the API. You don't call it in the test program either.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 73c6c03..af4f922 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -4653,7 +4653,7 @@ int PQflush(PGconn *conn);
 
 could be much more efficiently done with:
 
- UPDATE mytable SET x = x + 1;
+ UPDATE mytable SET x = x + 1 WHERE id = 42;
 

 
@@ -4696,7 +4696,7 @@ int PQflush(PGconn *conn);
  non-blocking mode. If used 
in
  blocking mode it is possible for a client/server deadlock to occur. The
  client will block trying to send queries to the server, but the server 
will
- block trying to send results from queries it's already processed to the
+ block trying to send results from queries it has already processed to the
  client. This only occurs when the client sends enough queries to fill its
  output buffer and the server's receive buffer before switching to
  processing input from the server, but it's hard to predict exactly when
@@ -5015,7 +5015,7 @@ int PQgetNextQuery(PGconn *conn);
  
   
   Returns the number of queries still in the queue for this batch, not
-  including any query that's currently having results being processsed.
+  including any query that's currently having results being processed.
   This is the number of times PQgetNextQuery has to be
   called before the query queue is empty again.
 
@@ -5037,7 +5037,7 @@ int PQqueriesInBatch(PGconn *conn);
 
  
   
-   Returns 1 if the batch curently being received on a
+   Returns 1 if the batch currently being received on a
libpq connection in batch mode is
aborted, 0

-- 
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] Surprising behaviour of \set AUTOCOMMIT ON

[Daniel Verite]

Rushabh Lathia wrote:

> It might happen that SetVariable() can be called from other places in
> future,
> so its good to have all the set variable related checks inside
> SetVariable().

There's already another way to skip the \set check:
  select 'on' as "AUTOCOMMIT" \gset

But there's a function in startup.c which might be the ideal location
for the check, as it looks like the one and only place where the
autocommit flag actually changes:

static void
autocommit_hook(const char *newval)
{
 pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
}



Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Surprising behaviour of \set AUTOCOMMIT ON

[Daniel Verite]

Rahila Syed wrote:

> >Above error coming because in below code block change, you don't have
> check for
> >command (you should check opt0 for AUTOCOMMIT command)

> Corrected in the attached.

There are other values than ON: true/yes and theirs abbreviations,
the value 1, and anything that doesn't resolve to OFF is taken as ON.
ParseVariableBool() in commands.c already does the job of converting
these to bool, the new code should probably just call that function
instead of parsing the value itself.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Renaming of pg_xlog and pg_clog

[Daniel Verite]

Joshua D. Drake wrote:

> You log in, see that all the space and you find that you are using a
> ton of disk space. You look around for anything you can delete. You
> find a directory called pg_xlog, it says log, junior ignorant, don't
> want to be a sysadmin 101 says, "delete logs".

Yes, it happens. I don't deny the problem, but I'm wondering
about the wishful thinking we're possibly falling into here
concerning the solution.

Let's imagine that pg_xlog is named wal instead.
How does that help our user with the disk space problem?
Does that point to a path of resolution? I don't see it.
What do we think that user's next move will be?
After all, WAL means Write Ahead *Log*.

On the other hand, by decommissioning pg_xlog as a name,
that makes it obsolete in all presentations, tutorials, docs
that refer to that directory, and there are many of them.
There are years of confusion ahead with questions like
"where is that pg_xlog directory that I'm supposed to
monitor, or move into a different partition, etc...",
for a benefit that remains to be seen.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] Renaming of pg_xlog and pg_clog

[Daniel Verite]

Craig Ringer wrote:

> People won't see a README in amongst 5000 xlog segments while
> freaking out about the sever being down.

Maybe they're more likely to google "pg_xlog".
BTW, renaming it will not help with respect to that, as
there's a pretty good corpus on knowledge linked to that
particular keyword.

The first google results of "pg_xlog" are, for me:

- Solving pg_xlog out of disk space problem on Postgres 
- PostgreSQL: Documentation: 9.1: WAL Internals
- Why is my pg_xlog directory so huge? - PostgreSQL
- Database Soup: Don't delete pg_xlog
...

That's spot-on. For each person deleting stuff in pg_xlog and then
regretting it, how many look it up in the above results and avoid
making the mistake? Will they find comparable results if the
directory is "wal" ?

Aside from that, we might also question how much of the excuse
"pg_xlog looked like it was just deletable logs" is a lie made up
after the fact, because anybody wrecking a database is not against
deflecting a bit of the blame to the software, that's human.

The truth being that they took the gamble that postgres
will somehow recover from the deletion, at the risk of possibly
loosing the latest transactions. That doesn't necessarily look
so bad when current transactions are failing anyway for lack of disk
space, users are yelling at you, and you're frantically searching for
anything to delete in the FS to come back online quickly.
Personally I'm quite skeptical of the *name* of the directory
playing that much of a role in this scenario.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] sslmode=require fallback

[Daniel Verite]

Magnus Hagander wrote:

> > I don't understand why you want to change the default.  Is it for
> > performance?  Has it been measured?
> >
> >
> Yes. I've run into it multiple times, but I haven't specifically measured
> it. But I've had more than one situation where turning it off has
> completely removed a performance problem.

Here's a test case retrieving 133000 rows representing
100Mbytes of text, that shows a 4x slowdown with ssl.
ssl_renegotiation_limit is set to 0 and the cache is warmed up
by repeated executions.

Without SSL:

$ time psql -At "postgresql://localhost/mlists?sslmode=disable"\
  -c "select subject from mail" -o /dev/null
real0m1.359s
user0m0.544s
sys 0m0.084s

With SSL:
$ time psql -At "postgresql://localhost/mlists?sslmode=require"\ 
  -c "select subject from mail" -o /dev/null
real0m5.395s
user0m1.080s
sys 0m0.116s

The CPU is Intel(R) Xeon(R) CPU E31230 @ 3.20GHz, OS is Debian7
with kernel 3.2.0-4.

Personally I think that TLS for local networking is wrong as a default, and
it's unfortunate that distros like Debian/Ubuntu end up using that.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \crosstabview fixes

[Daniel Verite]

Tom Lane wrote:

> Pushed, thanks.
> BTW, I see we've been spelling your name with an insufficient number
> of accents in the commit logs and release notes.  Can't do much about
> the logs, but will fix the release notes.

I use myself the nonaccented version of my name in "From" headers, as
a concession to mailers that are not always rfc2047-friendly, so I can't
blame anyone for leaving out one or both of these accents :)
Thanks for taking care of this anyway!


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \crosstabview fixes

[Daniel Verite]

Tom Lane wrote:

> > "Daniel Verite" <dan...@manitou-mail.org> writes:
> >> To avoid the confusion between "2:4" and "2":"4" or 2:4,
> >> and the ambiguity with a possibly existing "2:4" column,
> >> maybe we should abandon this syntax and require the optional
> >> scolH to be on its own at the end of the command.
> 
> > That would be OK with me; it's certainly less of a hack than what's
> > there now.  (I went back and forth about how much effort to put into
> > dealing with the colon syntax; I think the version I have in my patch
> > would be all right, but it's not perfect.)
> 
> Here's a patch along those lines.  Any objections?

Checking http://www.postgresql.org/docs/devel/static/app-psql.html ,
I notice that the last example is still using the syntax for arguments
that has been deprecated by commit 6f0d6a507, as discussed in this
thread.
A fix to psql-ref.sgml is attached.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 4160665..df79a37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4173,7 +4173,7 @@ numerical order and columns with an independant, ascending numerical order.
 testdb= SELECT t1.first as "A", t2.first+100 AS "B", t1.first*(t2.first+100) as "AxB",
 testdb( row_number() over(order by t2.first) AS ord
 testdb( FROM my_table t1 CROSS JOIN my_table t2 ORDER BY 1 DESC
-testdb( \crosstabview A B:ord AxB
+testdb( \crosstabview "A" "B" "AxB" ord
  A | 101 | 102 | 103 | 104 
 ---+-+-+-+-
  4 | 404 | 408 | 412 | 416

-- 
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] Rename max_parallel_degree?

[Daniel Verite]

Robert Haas wrote:

> Of course, we could make this value 1-based rather than 0-based, as
> Peter Geoghegan suggested a while back.  But as I think I said at the
> time, I think that's more misleading than helpful.  The leader
> participates in the parallel plan, but typically does far less of the
> work beneath the Gather node than the other nodes involved in the
> query, often almost none.  In short, the leader is special.
> Pretending that it's just another process involved in the parallel
> group isn't doing anyone a favor.

FWIW, that's not how it looks from the outside (top or vmstat).
I'm ignorant about how parallel tasks are assigned in the planner,
but when trying various values for max_parallel_degree and running
simple aggregates on large tables on a single 4 core CPU doing
nothing else, I'm only ever seeing max_parallel_degree+1 processes
indiscriminately at work, often in the same state (R running or
D waiting for disk).

Also when looking at exec times, for a CPU-bound sample query, I get
for instance the results below, when increasing parallelism one step
at a time, on a 4-core CPU.
I've checked with EXPLAIN that the planner allocates each time
a number of workers that's exactly equal to max_parallel_degree.
("Workers Planned" under the Gather node).

 mp_degree | exec_time | speedup (against degree=0)
---+---+-
 0 | 10850.835 |1.00
 1 |  5833.208 |1.86
 2 |  3990.144 |2.72
 3 |  3165.606 |3.43
 4 |  3315.153 |3.27
 5 |  .931 |3.25
 6 |  3354.995 |3.23

If the leader didn't do much work here, how would degree=1 produce
such a speedup (1.86) ?

There's also the fact that allowing 4 workers does not help compared
to 3, even though there are 4 cores here. Again, doesn't it make sense
only if the leader is as important as the workers in terms of CPU
usage? 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \crosstabview fixes

[Daniel Verite]

Tom Lane wrote:

> I wrote:
> > My feeling is that what we should do is undo the change to use OT_SQLID,
> > and in indexOfColumn() perform a downcasing/dequoting conversion that
> > duplicates what OT_SQLID does in psqlscanslash.l.
> 
> Here's an updated patch that does it that way, and also adopts Christoph's
> documentation suggestion about "sortcolH".  Any further comments?

For my part, I'm fine with this. Thanks!

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] \crosstabview fixes

[Daniel Verite]

Christoph Berg wrote:

> If there's no way out, what about changing it the other way, i.e.
> breaking the case where the column is named by a number? That seems
> much less of a problem in practice.

I don't think it would be acceptable.
But there's still the option of keeping the dedicated parser.

crosstabview doc says:
"The usual SQL case folding and quoting rules apply to column names"

Tom objected to that, upthread:
> I noticed that the \crosstabview documentation asserts that column
> name arguments are handled per standard SQL semantics.  In point of
> fact, though, the patch expends a couple hundred lines to implement
> what is NOT standard SQL semantics: matching unquoted names
> case-insensitively is anything but that.

Indeed it differs, but that's not necessarily bad. If there's a FOO
column and it's refered to as \crosstabview foo... it will complain about
an ambiguity only if there's another column that's named foo, or Foo, or
any case variant. Quoting becomes mandatory only in that case,
or of course if the column referred to contains spaces or double
quotes. 

For example, both these invocations work:
current=# SELECT 'X' as "FOO", 'Y', 'z' \crosstabview foo 2
 FOO | Y 
-+---
 X   | z

current=# SELECT 'X' as "FOO", 'Y', 'z' \crosstabview FOO 2
 FOO | Y 
-+---
 X   | z

OTOH a detected ambiguity would be:
current=# SELECT 'X' as "FOO", 'Y' as "foo", 'z' \crosstabview FOO 2
Ambiguous column name: FOO

which is solved by quoting the argument:
current=# SELECT 'X' as "FOO", 'Y' as "foo", 'z' \crosstabview "FOO" 2
 FOO | Y 
-+---
 X   | z


Whereas using the generic column parser with Tom's patch, the only
accepted invocation out of the 4 examples above is the last one:

new=# SELECT 'X' as "FOO", 'Y', 'z' \crosstabview foo 2
\crosstabview: column name not found: "foo"

new # SELECT 'X' as "FOO", 'Y', 'z' \crosstabview FOO 2
\crosstabview: column name not found: "foo"

new=# SELECT 'X' as "FOO", 'Y' as "foo", 'z' \crosstabview FOO 2
\crosstabview: vertical and horizontal headers must be different columns

new=# SELECT 'X' as "FOO", 'Y' as "foo", 'z' \crosstabview "FOO" 2
 FOO | Y 
-+---
 X   | z

Personally I prefer the current behavior, but I can also understand
the appeal from a maintainer's point of view of getting rid of it in
favor of already existing, well-tested generic code.
In which case, what the dedicated parser does is a moot point.

However, because of the aforementioned problem of the interpretation
of column names as numbers, maybe the balance comes back to the
dedicated parser? In which case, there's the question of whether how
it handles case folding as shown above is OK, or whether it should
just downcase unquoted identifiers to strictly match SQL rules.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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