At low rates, this can lead to pgbench taking significantly longer to
terminate than the user might expect. Repair.
Fabien Coelho, reviewed by Aleksander Alekseev, Álvaro Herrera, and me.
Struct QL_HELP used to be defined as static in the sql_help.h header
file, which is included in sql_help.c and help.c, thus creating two
separate instances of the struct. This causes a warning from GCC 6,
because the struct is not used in sql_help.c.
Instead, declare the struct as extern in the header file and define it
in sql_help.c. This also allows making a bunch of functions static
because they are no longer needed outside of sql_help.c.
Reviewed-by: Thomas Munro <thomas.munro@enterprisedb.com>
We've not found a use for this so far, and the current need, which
is to convert the visibility map to a new format, does not suit the
existing design anyway. So just rip it out.
Author: Masahiko Sawada, slightly revised by me.
Discussion: 20160215211313.GB31273@momjian.us
A thinko in a96761391 caused pg_ctl to get it exactly backwards when
deciding whether to report problems to the Windows eventlog or to stderr.
Per bug #14001 from Manuel Mathar, who also identified the fix.
Like the previous patch, back-patch to all supported branches.
As written, this would accept e.g. 123e9 as a function name. Aside
from being mildly astonishing, that would come back to haunt us if
we ever try to add float constants to the expression syntax. Insist
that function names start with letters (or at least non-digits).
In passing reset yyline as well as yycol when starting a new expression.
This variable is useless since it's used nowhere, but if we're going
to have it we should have it act sanely.
Add four new SQL accessible functions: pg_control_system(),
pg_control_checkpoint(), pg_control_recovery(), and pg_control_init()
which expose a subset of the control file data.
Along the way move the code to read and validate the control file to
src/common, where it can be shared by the new backend functions
and the original pg_controldata frontend program.
Patch by me, significant input, testing, and review by Michael Paquier.
The existing code confuses the byte length of the string (which is
relevant when passing it to pg_strncasecmp) with the character length
of the string (which is relevant when it is used with the SQL substring
function). Separate those two concepts.
Report and patch by Kyotaro Horiguchi, reviewed by Thomas Munro and
reviewed and further revised by me.
This makes the psql() method much more capable: it captures both stdout
and stderr; it now returns the psql exit code rather than stdout; a
timeout can now be specified, as can ON_ERROR_STOP behavior; it gained a
new "on_error_die" (defaulting to off) parameter to raise an exception
if there's any problem. Finally, additional parameters to psql can be
passed if there's need for further tweaking.
For convenience, a new safe_psql() method retains much of the old
behavior of psql(), except that it uses on_error_die on, so that
problems like syntax errors in SQL commands can be detected more easily.
Many existing TAP test files now use safe_psql, which is what is really
wanted. A couple of ->psql() calls are now added in the commit_ts
tests, which verify that the right thing is happening on certain errors.
Some ->command_fails() calls in recovery tests that were verifying that
psql failed also became ->psql() calls now.
Author: Craig Ringer. Some tweaks by Álvaro Herrera
Reviewed-By: Michaël Paquier
Fabien Coelho, reviewed mostly by Michael Paquier and me, but also by
Heikki Linnakangas, BeomYong Lee, Kyotaro Horiguchi, Oleksander
Shulgin, and Álvaro Herrera.
Some code in the RewindTest test suite is more generally useful than
just for that suite, so put it where other test suites can reach it.
Some postgresql.conf parameters change their default values when a
cluster is initialized with 'allows_streaming' than the previous
behavior; most notably, autovacuum is no longer turned off.
(Also, we no longer call pg_ctl promote with -w, but that flag doesn't
actually do anything in promote so there's no behavior change.)
Author: Michael Paquier
Architecture reference material specifies this order, and s_lock.h
inline assembly agrees. The former order failed to provide mutual
exclusion to lwlock.c and perhaps to other clients. The two xlc
buildfarm members, hornet and mandrill, have failed sixteen times with
duplicate key errors involving pg_class_oid_index or pg_type_oid_index.
Back-patch to 9.5, where commit b64d92f1a5
introduced atomics.
Reviewed by Andres Freund and Tom Lane.
It was using %u to read a string that was earlier produced by snprintf with %d
into a signed integer variable. This seems to work in practice but is
incorrect.
found by cppcheck
Suppress creation of the pg_upgrade delete script when the new data
directory is inside the old data directory.
Reported-by: IRC
Backpatch-through: 9.3, where delete script tests were added
In commit a5c43b88 the behavior of command line pg_config was
inadvertantly changed to include the config name when specific
configs are requested, similar to when none are requested and
all are emitted. This breaks scripts that expect to use
pg_config for e.g. PGXS. Revert the behavior to the previous.
Move and refactor the underlying code for the pg_config client
application to src/common in support of sharing it with a new
system information SRF called pg_config() which makes the same
information available via SQL. Additionally wrap the SRF with a
new system view, as called pg_config.
Patch by me with extensive input and review by Michael Paquier
and additional review by Alvaro Herrera.
The original code wasn't careful to test the file descriptor returned by
PQsocket() for an invalid socket. If an invalid socket did turn up,
that would amount to calling FD_ISSET with fd = -1, whereby undefined
behavior can be invoked.
To fix, test file descriptor for validity and stop further processing if
that fails.
Problem noticed by Coverity.
There is an existing FD_ISSET callsite that does check for invalid
sockets beforehand, but the error message reported by it was
strerror(errno); in testing the aforementioned change, that turns out to
result in "bad socket: Success" which isn't terribly helpful. Instead
use PQerrorMessage() in both places which is more likely to contain an
useful error message.
Backpatch-through: 9.1.
NextXID has been rendered in the form of a pg_lsn even though it
really is not. This can cause confusion, so change the format from
%u/%u to %u:%u, per discussion on hackers.
Complaint by me, patch by me and Bruce, reviewed by Michael Paquier
and Alvaro. Applied to HEAD only.
Author: Joe Conway, Bruce Momjian
Reviewed-by: Michael Paquier, Alvaro Herrera
Backpatch-through: master
There is no reason to have the per-thread logfile file pointer as a
separate parameter in various functions: it's much simpler to put it in
the per-thread state struct instead, which is already being passed to
all functions that need the log file anyway. Change the callsites in
which it was used as a boolean to test whether logging is active, so
that they use the use_log global variable instead.
No backpatch, even though this exists since commit a887c486d5 of March
2010, because this is just for cleanliness' sake and the surrounding
code has been modified a lot recently anyway.
Many automated test suites call pg_ctl. Buildfarm members axolotl,
hornet, mandrill, shearwater, sungazer and tern have failed when server
shutdown took longer than the pg_ctl default 60s timeout. This addition
permits slow hosts to easily raise the timeout without us editing a
--timeout argument into every test suite pg_ctl call. Back-patch to 9.1
(all supported versions) for the sake of automated testing.
Reviewed by Tom Lane.
This is a quick hack, due to be reverted when its purpose has been served,
to try to gather information about why some of the buildfarm critters
regularly fail with "postmaster does not shut down" complaints. Maybe they
are just really overloaded, but maybe something else is going on. Hence,
instrument pg_ctl to print the current time when it starts waiting for
postmaster shutdown and when it gives up, and add a lot of logging of the
current time in the server's checkpoint and shutdown code paths.
No attempt has been made to make this pretty. I'm not even totally sure
if it will build on Windows, but we'll soon find out.
If a view is split into CREATE TABLE + CREATE RULE to break a circular
dependency, then any triggers on the view must be dumped/reloaded after
the CREATE RULE; else the backend may reject the CREATE TRIGGER because
it's the wrong type of trigger for a plain table. This works all right
in plain dump/restore because of pg_dump's sorting heuristic that places
triggers after rules. However, when using parallel restore, the ordering
must be enforced by a dependency --- and we didn't have one.
Fixing this is a mere matter of adding an addObjectDependency() call,
except that we need to be able to find all the triggers belonging to the
view relation, and there was no easy way to do that. Add fields to
pg_dump's TableInfo struct to remember where the associated TriggerInfo
struct(s) are.
Per bug report from Dennis Kögel. The failure can be exhibited at least
as far back as 9.1, so back-patch to all supported branches.
This field was included in the original definition of the printQueryOpt
struct in commit a45195a191, but it was not used anywhere in that
commit, nor since then. Spotted by Dickson S. Guedes.
Provide per-script statistical info (count of transactions executed
under that script, average latency for the whole script) after a
multi-script run, adding an intermediate level of detail to existing
global stats and per-command stats.
Author: Fabien Coelho
Reviewer: Michaël Paquier, Álvaro Herrera
Dividing INT_MIN by -1 or taking INT_MIN modulo -1 can sometimes
cause floating-point exceptions or otherwise misbehave.
Fabien Coelho and Michael Paquier
Add
- ALTER SYSTEM SET/RESET ... -> GUC variables
- ALTER TABLE ... SET WITH -> OIDS
- ALTER DATABASE/FUNCTION/ROLE/USER ... SET/RESET -> GUC variables
- ALTER DATABASE/FUNCTION/ROLE/USER ... SET ... -> FROM CURRENT/TO
- ALTER DATABASE/FUNCTION/ROLE/USER ... SET ... TO/= -> possible values
Author: Fujii Masao
Reviewed-by: Michael Paquier, Masahiko Sawada
This doesn't add any functionality but just shuffles things around so
that it can be reused and improved later.
Author: Fabien Coelho
Reviewed-by: Michael Paquier, Álvaro Herrera
listForeignTables' invocation of processSQLNamePattern did not match up
with the other ones that handle potentially-schema-qualified names; it
failed to make use of pg_table_is_visible() and also passed the name
arguments in the wrong order. Bug seems to have been aboriginal in commit
0d692a0dc9. It accidentally sort of worked as long as you didn't
inquire too closely into the behavior, although the silliness was later
exposed by inconsistencies in the test queries added by 59efda3e50
(which I probably should have questioned at the time, but didn't).
Per bug #13899 from Reece Hart. Patch by Reece Hart and Tom Lane.
Back-patch to all affected branches.
Previously, it was possible to specify one or several custom scripts to
run, or only one of the builtin scripts. With this patch it is also
possible to specify to run the builtin scripts multiple times, using the
new -b option. Also, unify the code for both cases; this eases future
pgbench improvements.
Author: Fabien Coelho
Review: Michaël Paquier, Álvaro Herrera
pgindent for recent commits; also change some variables from int to
boolean, which is how they are really used.
Mostly submitted by Fabien Coelho; this is in preparation to commit
further patches to the file.
Add
- ALTER FOREIGN DATA WRAPPER -> RENAME TO
- ALTER SERVER -> RENAME TO
- ALTER SERVER ... VERSION ... -> OPTIONS
- CREATE FOREIGN DATA WRAPPER -> OPTIONS
- CREATE SERVER -> OPTIONS
- CREATE|ALTER USER MAPPING -> OPTIONS
From: Andreas Karlsson <andreas@proxel.se>
The original code was adding double quotes to an already-quoted
identifier, leading to nonsensical results. Remove the quoting call.
I introduced the broken code in 7eca575d1c of 9.5 era, so backpatch to
9.5.
Report and patch by Elvis Pranskevichus
Reviewed by Michael Paquier
Aggregate nodes now have two new modes: a "partial" mode where they
output the unfinalized transition state, and a "finalize" mode where
they accept unfinalized transition states rather than individual
values as input.
These new modes are not used anywhere yet, but they will be necessary
for parallel aggregation. The infrastructure also figures to be
useful for cases where we want to aggregate local data and remote
data via the FDW interface, and want to bring back partial aggregates
from the remote side that can then be combined with locally generated
partial aggregates to produce the final value. It may also be useful
even when neither FDWs nor parallelism are in play, as explained in
the comments in nodeAgg.c.
David Rowley and Simon Riggs, reviewed by KaiGai Kohei, Heikki
Linnakangas, Haribabu Kommi, and me.
This code has been there for a long time, but it's never really been
needed. Cygwin has its own utility for registering, unregistering,
stopping and starting Windows services, and that's what's used in the
Cygwin postgres packages. So now pg_ctl for Cygwin looks like it is for
any Unix platform.
Michael Paquier and me
Coverity quite reasonably complained that this check for fout==NULL
occurred after we'd already dereferenced fout. However, the check
is just dead code since there is no code path by which CreateArchive
can return a null pointer. Errors such as can't-open-that-file are
reported down inside CreateArchive, and control doesn't return.
So let's silence the warning by removing the dead code, rather than
continuing to pretend it does something.
Coverity didn't complain about this before 5b5fea2a1, so back-patch
to 9.5 like that patch.
pg_dump's original approach to handling extension member objects was to
run around and clear (or set) their dump flags rather late in its data
collection process. Unfortunately, quite a lot of code expects those flags
to be valid before that; which was an entirely reasonable expectation
before we added extensions. In particular, this explains Karsten Hilbert's
recent report of pg_upgrade failing on a database in which an extension
has been installed into the pg_catalog schema. Its objects are initially
marked as not-to-be-dumped on the strength of their schema, and later we
change them to must-dump because we're doing a binary upgrade of their
extension; but we've already skipped essential tasks like making associated
DO_SHELL_TYPE objects.
To fix, collect extension membership data first, and incorporate it in the
initial setting of the dump flags, so that those are once again correct
from the get-go. This has the undesirable side effect of slightly
lengthening the time taken before pg_dump acquires table locks, but testing
suggests that the increase in that window is not very much.
Along the way, get rid of ugly special-case logic for deciding whether
to dump procedural languages, FDWs, and foreign servers; dump decisions
for those are now correct up-front, too.
In 9.3 and up, this also fixes erroneous logic about when to dump event
triggers (basically, they were *always* dumped before). In 9.5 and up,
transform objects had that problem too.
Since this problem came in with extensions, back-patch to all supported
versions.
Rather than passing around DumpOptions and RestoreOptions as separate
arguments, add fields to struct Archive to carry pointers to these objects,
and access them through those fields when needed. There already was a
RestoreOptions pointer in Archive, though for no obvious reason it was part
of the "private" struct rather than out where pg_dump.c could see it.
Doing this allows reversion of quite a lot of parameter-addition changes
made in commit 0eea8047bf, which is a good thing IMO because this will
reduce the code delta between 9.4 and 9.5, probably easing a few future
back-patch efforts. Moreover, the previous commit only added a DumpOptions
argument to functions that had to have it at the time, which means we could
anticipate still more code churn (and more back-patch hazard) as the
requirement spread further. I'd hit exactly that problem in my upcoming
patch to fix extension membership marking, which is what motivated me to
do this.
The completion of CREATE INDEX CONCURRENTLY was lacking in several ways
compared to a plain CREATE INDEX command:
- CREATE INDEX <name> ON completes table names, but didn't with
CONCURRENTLY.
- CREATE INDEX completes ON and existing index names, but with
CONCURRENTLY it only completed ON.
- CREATE INDEX <name> completes ON, but didn't with CONCURRENTLY.
These are now all fixed.
The previous code supported a syntax like CREATE INDEX name
CONCURRENTLY, which never existed. Mistake introduced in commit
37ec19a15c. Remove the addition of
CONCURRENTLY at that point.
Turns out the only reason initdb -X worked is that pg_mkdir_p won't
whine if you point it at something that's a symlink to a directory.
Otherwise, the attempt to create pg_xlog/ just like all the other
subdirectories would have failed. Let's be a little more explicit
about what's happening. Oversight in my patch for bug #13853
(mea culpa for not testing -X ...)
When we're creating subdirectories of PGDATA during initdb, we know darn
well that the parent directory exists (or should exist) and that the new
subdirectory doesn't (or shouldn't). There is therefore no need to use
anything more complicated than mkdir(). Using pg_mkdir_p() just opens us
up to unexpected failure modes, such as the one exhibited in bug #13853
from Nuri Boardman. It's not very clear why pg_mkdir_p() went wrong there,
but it is clear that we didn't need to be trying to create parent
directories in the first place. We're not even saving any code, as proven
by the fact that this patch nets out at minus five lines.
Since this is a response to a field bug report, back-patch to all branches.
pg_ctl is using isatty() to verify whether the process is running in a
terminal, and if not it sends its output to Windows' Event Log ... which
does the wrong thing when the output has been redirected to a pipe, as
reported in bug #13592.
To fix, make pg_ctl use the code we already have to detect service-ness:
in the master branch, move src/backend/port/win32/security.c to src/port
(with suitable tweaks so that it runs properly in backend and frontend
environments); pg_ctl already has access to pgport so it Just Works. In
older branches, that's likely to cause trouble, so instead duplicate the
required code in pg_ctl.c.
Author: Michael Paquier
Bug report and diagnosis: Egon Kocjan
Backpatch: all supported branches
Although these temp tables will get removed from template1 at the end of
the standalone-backend run, that's too late to keep them from getting
copied into the template0 and postgres databases, now that we use only a
single backend run for the whole sequence. While no real harm is done
by the extra copies (since they'd be deleted on first use of the temp
schema), it's still unsightly, and it would mean some wasted cycles for
every database creation for the life of the installation.
Oversight in commit c4a8812cf6. Noticed by Amit Langote.
The order of inclusion of .o files makes a difference in linker output;
not a functional difference, but still a bitwise difference, which annoys
some packagers who would like reproducible builds.
Report and patch by Christoph Berg
A pointless and confusing error message is shown to the user when
attempting to identify a 9.3 or older remote server with a 9.5/9.6
pg_receivexlog, because the return signature of IDENTIFY_SYSTEM was
changed in 9.4. There's no good reason for the warning message, so
shuffle code around to keep it quiet.
(pg_recvlogical is also affected by this commit, but since it obviously
cannot work with 9.3 that doesn't actually matter much.)
Backpatch to 9.5.
Reported by Marco Nenciarini, who also wrote the initial patch. Further
tweaked by Robert Haas and Fujii Masao; reviewed by Michael Paquier and
Craig Ringer.
This requires adding some more infrastructure to handle both case-sensitive
and case-insensitive matching, as well as the ability to match a prefix of
a previous word. So it ends up being about a wash line-count-wise, but
it's just as big a readability win here as in the SQL tab completion rules.
Michael Paquier, some adjustments by me
In the refactoring in commit d37b816dc9,
we mostly kept to the original design whereby only the last few words
on the line were matched to identify a completable pattern. However,
after commit d854118c8d, there's really
no reason to do it like that: where it's sensible, we can use patterns
that expect to match the entire input line. And mostly, it's sensible.
Matching the entire line greatly reduces the odds of a false match that
leads to offering irrelevant completions. Moreover (though I've not
tried to measure this), it should make tab completion faster since
many of the patterns will be discarded after a single integer comparison
that finds that the wrong number of words appear on the line.
There are certain identifiable places where we still need to use
TailMatches because the statement in question is allowed to appear
embedded in a larger statement. These are just a small minority of
the existing patterns, though, so the benefit of switching where
possible is large.
It's possible that this patch has removed some within-line matching
behaviors that are in fact desirable, but we can put those back when
we get complaints. Most of the removed behaviors are certainly silly.
Michael Paquier, with some further adjustments by me
Commit c7e27becd2 fixed this on the backend side, but we neglected
the fact that several code paths in pg_dump were printing reloptions
values that had not gotten massaged by ruleutils. Apply essentially the
same quoting logic in those places, too.
The variables newestCommitTs and oldestCommitTs sound as if they are
timestamps, but in fact they are the transaction Ids that correspond
to the newest and oldest timestamps rather than the actual timestamps.
Rename these variables to reflect that they are actually xids: to wit
newestCommitTsXid and oldestCommitTsXid respectively. Also modify
related code in a similar fashion, particularly the user facing output
emitted by pg_controldata and pg_resetxlog.
Complaint and patch by me, review by Tom Lane and Alvaro Herrera.
Backpatch to 9.5 where these variables were first introduced.
As of commit d5563d7df, psql -c no longer implies -X, but not all of
our regression testing scripts had gotten that memo.
To ensure consistency of results across different developers, make
sure that *all* invocations of psql in all scripts in our tree
use -X, even where this is not what previously happened.
Michael Paquier and Tom Lane
For some reason, we've been overlooking the fact that pg_receivexlog
and pg_recvlogical are using wrong translation domains all along,
so their output hasn't ever been translated. The right domain is
pg_basebackup, not their own executable names.
Noticed by Ioseph Kim, who's been working on the Korean translation.
Backpatch pg_receivexlog to 9.2 and pg_recvlogical to 9.4.
t/002_databases.pl was expecting to see a specific physical order of the
rows in pg_database. I broke that in HEAD with commit 01e386a325,
but I'd say it's a pretty fragile test methodology in any case, so fix
it in 9.5 as well.
Commit ed7b3b3811 purported to remove initdb's use of VACUUM FULL,
as had been agreed to in a pghackers discussion back in Dec 2014.
But it missed this one ...
This reverts most of commit 83dec5a71 in favor of having connectDatabase()
store the possibly-reusable password in a static variable, similar to the
coding we've had for a long time in pg_dump's version of that function.
To avoid possible problems with unwanted password reuse, make callers
specify whether it's reasonable to attempt to re-use the password.
This is a wash for cases where re-use isn't needed, but it is far simpler
for callers that do want that. Functionally there should be no difference.
Even though we're past RC1, it seems like a good idea to back-patch this
into 9.5, like the prior commit. Otherwise, if there are any third-party
users of connectDatabase(), they'll have to deal with an API change in
9.5 and then another one in 9.6.
Michael Paquier
When pg_dump prompts the user for a password, it remembers the password
for possible re-use by parallel worker processes. However, libpq might
have extracted the password from a connection string originally passed
as "dbname". Since we don't record the original form of dbname but
break it down to host/port/etc, the password gets lost. Fix that by
retrieving the actual password from the PGconn.
(It strikes me that this whole approach is rather broken, as it will also
lose other information such as options that might have been present in
the connection string. But we'll leave that problem for another day.)
In passing, get rid of rather silly use of malloc() for small fixed-size
arrays.
Back-patch to 9.3 where parallel pg_dump was introduced.
Report and fix by Zeus Kronion, adjusted a bit by Michael Paquier and me
Yesterday in commit d854118c8, I had a serious brain fade leading me to
underestimate the number of words that the tab-completion logic could
divide a line into. On input such as "(((((", each character will get
seen as a separate word, which means we do indeed sometimes need more
space for the words than for the original line. Fix that.
psql offered USING, WHERE, and SET in this context, but SET is not a valid
possibility here. Seems to have been a thinko in commit f5ab0a14ea
which added DELETE's USING option.
Up to now, the tab completion logic has only examined the last few words
of the current input line; "last few" being originally as few as four
words, but lately up to nine words. Furthermore, it only looked at what
libreadline considers the current line of input, which made it rather
myopic if you split your command across lines. This was tolerable,
sort of, so long as the match patterns were only designed to consider the
last few words of input; but with the recent addition of HeadMatches()
and Matches() matching rules, we really have to do better if we want
those to behave sanely.
Hence, change the code to break the entire line down into words, and to
include any previous lines in the command buffer along with the active
readline input buffer.
This will be a little bit slower than the previous coding, but some
measurements say that even a query of several thousand characters can be
parsed in a hundred or so microseconds on modern machines; so it's really
not going to be significant for interactive tab completion. To reduce
the cost some, I arranged to avoid the per-word malloc calls that used
to occur: all the words are now kept in one malloc'd buffer.
Replace tests like
else if (pg_strcasecmp(prev4_wd, "CREATE") == 0 &&
pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
(pg_strcasecmp(prev_wd, "BEFORE") == 0 ||
pg_strcasecmp(prev_wd, "AFTER") == 0))
with new notation like this:
else if (TailMatches4("CREATE", "TRIGGER", MatchAny, "BEFORE|AFTER"))
In addition, provide some macros COMPLETE_WITH_LISTn() to reduce the amount
of clutter needed to specify a small number of predetermined completion
alternatives.
This makes the code substantially more compact: tab-complete.c gets over a
thousand lines shorter in this patch, despite the addition of a couple of
hundred lines of infrastructure for the new notations. The new way of
specifying match rules seems a whole lot more readable and less
error-prone, too.
There's a lot more that could be done now to make matching faster and more
reliable; for example I suspect that most of the TailMatches() rules should
now be Matches() rules. That would allow them to be skipped after a single
integer comparison if there aren't the right number of words on the line,
and it would reduce the risk of unintended matches. But for now, (mostly)
refrain from reworking any match rules in favor of just converting what
we've got into the new notation.
Thomas Munro, reviewed by Michael Paquier, some adjustments by me
Previously the completion used the wrong word to match 'BY'. This was
introduced brokenly, in b2de2a. While at it, also add completion of
IN TABLESPACE ... OWNED BY and fix comments referencing nonexistent
syntax.
Reported-By: Michael Paquier
Author: Michael Paquier and Andres Freund
Discussion: CAB7nPqSHDdSwsJqX0d2XzjqOHr==HdWiubCi4L=Zs7YFTUne8w@mail.gmail.com
Backpatch: 9.4, like the commit introducing the bug
Per a recommendation from Tomas Vondra, it's more helpful to refer to
the value that determines how skewed a Gaussian or exponential
distribution is as a parameter rather than a threshold.
Since it's not quite too late to get this right in 9.5, where it was
introduced, back-patch this. Most of the patch changes only comments
and documentation, but a few pgbench messages are altered to match.
Fabien Coelho, reviewed by Michael Paquier and by me.
Previously, each subroutine in initdb fired up its own standalone backend
session. Over time we'd grown as many as fifteen of these sessions,
and the cumulative startup and shutdown work for them was getting pretty
noticeable. Combining things so that all these steps share a single
backend session cuts a good 10% off the total runtime of initdb, more
if you're not fsync'ing.
The main stumbling block to doing this before was that some of the sessions
were run with -j and some not. The improved definition of -j mode
implemented by my previous commit makes it possible to fix that by running
all the post-bootstrap steps with -j; we just have to use double instead of
single newlines to end command strings. (This is only absolutely necessary
around the VACUUM and CREATE DATABASE steps, since those can't be run in a
transaction block. But it seems best to make them all use double newlines
so that the commands remain separate for error-reporting purposes.)
A minor disadvantage is that since initdb can't tell how much of its
output the backend has executed, we can no longer have the per-step
progress reporting initdb used to print. But things are fast enough
nowadays that that's not really all that useful anyway.
In passing, add more const decoration to some of the static arrays in
initdb.c.
Turns out we must set rl_basic_word_break_characters *before* we call
rl_initialize() the first time, because it will quietly copy that value
elsewhere --- but only on the first call. (Love these undocumented
dependencies.) I broke this yesterday in commit 2ec477dc8108339d;
like that commit, back-patch to all active branches. Per report from
Pavel Stehule.
It emerges that libreadline doesn't notice terminal window size change
events unless they occur while collecting input. This is easy to stumble
over if you resize the window while using a pager to look at query output,
but it can be demonstrated without any pager involvement. The symptom is
that queries exceeding one line are misdisplayed during subsequent input
cycles, because libreadline has the wrong idea of the screen dimensions.
The safest, simplest way to fix this is to call rl_reset_screen_size()
just before calling readline(). That causes an extra ioctl(TIOCGWINSZ)
for every command; but since it only happens when reading from a tty, the
performance impact should be negligible. A more valid objection is that
this still leaves a tiny window during entry to readline() wherein delivery
of SIGWINCH will be missed; but the practical consequences of that are
probably negligible. In any case, there doesn't seem to be any good way to
avoid the race, since readline exposes no functions that seem safe to call
from a generic signal handler --- rl_reset_screen_size() certainly isn't.
It turns out that we also need an explicit rl_initialize() call, else
rl_reset_screen_size() dumps core when called before the first readline()
call.
rl_reset_screen_size() is not present in old versions of libreadline,
so we need a configure test for that. (rl_initialize() is present at
least back to readline 4.0, so we won't bother with a test for it.)
We would need a configure test anyway since libedit's emulation of
libreadline doesn't currently include such a function. Fortunately,
libedit seems not to have any corresponding bug.
Merlin Moncure, adjusted a bit by me
Commit d5563d7df9 drew complaints from Coverity, which quite
correctly complained that one copy of each -c or -f string was being
leaked. What's more, simple_action_list_append was allocating enough space
for still a third copy of each string as part of the SimpleActionListCell,
even though that coding method had been superseded by a separate strdup
operation. There were some other minor coding infelicities too. The
documentation needed more work as well, eg it forgot to explain that -c
causes psql not to accept any interactive input.
Complete "ALTER POLICY" with a policy name, as we do for DROP POLICY.
And, complete "ALTER POLICY polname ON" with a table name that has such
a policy, as we do for DROP POLICY, rather than with any table name
at all.
Masahiko Sawada
Commit 344cdff2c made failure to open the target of --output fatal.
For consistency, the --log-file switch should behave similarly.
Like the previous commit, back-patch to 9.5 but no further.
Daniel Verite
To support this, we must reconcile some historical anomalies in the
behavior of -c. In particular, as a backward-incompatibility, -c no
longer implies --no-psqlrc.
Pavel Stehule (code) and Catalin Iacob (documentation). Review by
Michael Paquier and myself. Proposed behavior per Tom Lane.
Noted by Tom Lane:
- PostgresNode had a BEGIN block which created files, contrary to
perlmod suggestions to do that only on INIT blocks.
- Assign ports randomly rather than starting from 90600.
Noted by Noah Misch:
- Change use of no-longer-set PGPORT environment variable to $node->port
- Don't start a server in pg_controldata test
- PostgresNode was reading the PID file incorrectly; test the right
thing, and chomp the line we read from the PID file.
- Remove an unused $devnull variable
- Use 'pg_ctl kill' instead of "kill" directly, for Windos portability.
- Make server log names more informative.
Author: Michael Paquier
Formerly, if "psql -o foo" failed to open the output file "foo", it would
print an error message but then carry on as though -o had not been
specified at all. This seems contrary to expectation: a program that
cannot open its output file normally fails altogether. Make psql do
exit(1) after reporting the error.
If "\o foo" failed to open "foo", it would print an error message but then
reset the output file to stdout, as if the argument had been omitted.
This is likewise pretty surprising behavior. Make it keep the previous
output state, instead.
psql keeps SIGPIPE interrupts disabled when it is writing to a pipe, either
a pipe specified by -o/\o or a transient pipe opened for purposes such as
using a pager on query output. The logic for this was too simple and could
sometimes re-enable SIGPIPE when a -o pipe was still active, thus possibly
leading to an unexpected psql crash later.
Fixing the last point required getting rid of the kluge in PrintQueryTuples
and ExecQueryUsingCursor whereby they'd transiently change the global
queryFout state, but that seems like good cleanup anyway.
Back-patch to 9.5 but not further; these are minor-enough issues that
changing the behavior in stable branches doesn't seem appropriate.
The formatting modes that depend on knowledge of the terminal window width
did not work right when printing a query result that's been fetched in
sections (as a result of FETCH_SIZE). ExecQueryUsingCursor() would force
use of the pager as soon as there's more than one result section, and then
print.c would see an output file pointer that's not stdout and incorrectly
conclude that the terminal window width isn't relevant.
This has been broken all along for non-expanded "wrapped" output format,
and as of 9.5 the issue affects expanded mode as well. The problem also
caused "\pset expanded auto" mode to invariably *not* switch to expanded
output in a segmented result, which seems to me to be exactly backwards.
To fix, we need to pass down an "is_pager" flag to inform the print.c
subroutines that some calling level has already replaced stdout with a
pager pipe, so they should (a) not do that again and (b) nonetheless honor
the window size. (Notably, this makes the first is_pager test in
print_aligned_text() not be dead code anymore.)
This patch is a bit invasive because there are so many existing calls of
printQuery()/printTable(), but fortunately all but a couple can just pass
"false" for the added parameter.
Back-patch to 9.5 but no further. Given the lack of field complaints,
it's not clear that we should change the behavior in stable branches.
Also, the API change for printQuery()/printTable() might possibly break
third-party code, again something we don't like to do in stable branches.
However, it's not quite too late to do this in 9.5, and with the larger
scope of the problem there, it seems worth doing.
The original code was a bit clunky; make it more amenable for further
reuse by creating a new Perl package PostgresNode, which is an
object-oriented representation of a single server, with some support
routines such as init, start, stop, psql. This serves as a better basis
on which to build further test code, and enables writing tests that use
more than one server without too much complication.
This commit modifies a lot of the existing test files, mostly to remove
explicit calls to system commands (pg_ctl) replacing them with method
calls of a PostgresNode object. The result is quite a bit more
straightforward.
Also move some initialization code to BEGIN and INIT blocks instead of
having it straight in as top-level code.
This commit also introduces package RecursiveCopy so that we can copy
whole directories without having to depend on packages that may not be
present on vanilla Perl 5.8 installations.
I also ran perltidy on the modified files, which changes some code sites
that are not otherwise touched by this patch. I tried to avoid this,
but it ended up being more trouble than it's worth.
Authors: Michael Paquier, Álvaro Herrera
Review: Noah Misch
Don't force the data width to extend all the way to the right margin if it
doesn't need to. This reverts the behavior in non-wrapping cases to be
what it was in 9.4. Also, make the logic that ensures the data line width
is at least equal to the record-header line width a little less obscure.
In passing, avoid possible calculation of log10(0). Probably that's
harmless, given the lack of field complaints, but it seems risky:
conversion of NaN to an integer isn't well defined.
Robert Haas
Peter Eisentraut
Tom Lane
Joe Conway
Alvaro Herrera
Noah Misch
Bruce Momjian
Magnus Hagander
Fujii Masao
Kevin Grittner
Simon Riggs
Andrew Dunstan
Andres Freund