Re: [HACKERS] Dynamic result sets from procedures
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
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
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
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
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
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
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
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 to
Re: [HACKERS] Disallowing multiple queries per PQexec()
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()
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()
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()
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
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
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
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
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
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)
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
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
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
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
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
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
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 @@ NOTICE:
Re: [HACKERS] Variable substitution in psql backtick expansion
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
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
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
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
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)
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
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)
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
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
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
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 (PQresultSt
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
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(&conn->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
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
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)
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)
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)
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
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
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
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, &options); /* @@ -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", &pset.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/vari
Re: [HACKERS] One-shot expanded output in psql using \G
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
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", &pset.on_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
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
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)
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
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
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)
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)
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
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
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, &on_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", &pset.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, &on_off)) + popt->topt.expanded = on_off ? 1 : 0; + else + { + PsqlVarEnumError(param, value, "on, off, auto"); + return false; + } + }
Re: [HACKERS] Improvements in psql hooks for variables
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
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
Tom Lane wrote: > "Daniel Verite" 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, &valid); > ! 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, > &popt->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
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
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
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" ?
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
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
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, &success) ? 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", &success); + 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, &valid); + 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, &valid); + 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) {
Re: [HACKERS] [GENERAL] Select works only when connected from login postgres
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" ?
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
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" ?
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" ?
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
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, &success) ? 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", &success); + 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, &valid); + 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, &valid); + 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
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", &success); + 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
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
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
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
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
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
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", &pset.on_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", &isvalid); 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") ==
Re: [HACKERS] Improvements in psql hooks for variables
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, &pset.var) > { > 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'
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
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
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" ?
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(&buf, format, 2);/* per-column formats */ pq_endmessage(&buf); 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(&cstate->attribute_buf); - initStringInfo(&cstate->line_buf); + initLongStringInfo(&cstate->attribute_buf); + initLongStringInfo(&cstate->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 undefine
Re: [HACKERS] pg_dump / copy bugs with "big lines" ?
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
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
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
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, &isvalid); + if (isvalid) + *flag = val; + return isvalid; +} + +static bool autocommit_hook(const char *newval) { - pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT"); + return generic_boolean_hook(newval, "AUTOCOMMIT", &pset.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", &ps
[HACKERS] Improvements in psql hooks for variables
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, &isvalid); + if (isvalid) + *flag = val; + return isvalid; +} + +static bool autocommit_hook(const char *newval) { - pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT"); + return generic_boolean_hook(newval, "AUTOCOMMIT", &pset.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", &pset.on_error_stop); } -static void +static bool quiet_hook(const char *newval) { - pset.quiet = ParseVariableBool(newval, "QUIET"); + return generic_boolean_hook(newval, "QUIET", &pset.quiet); } -static void +static bool singleline_hook(const char *newval) { - pset.singleline = ParseVariableBool(newval, "SINGLELINE"); + return generic_boolean_hook(newval, "SINGLELINE", &pset.singleline); } -static void +static bool singlestep_hook(const char *newval) { - pset.singlestep = ParseVariableBool(newval, "SINGLESTEP"); + return generic_boolean_hook(newval, "SINGLESTEP", &pset.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 (ParseV
Re: [HACKERS] Surprising behaviour of \set AUTOCOMMIT ON
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
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
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
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
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
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
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
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
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
Tom Lane wrote: > > "Daniel Verite" 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?
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
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
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