Noah Misch [Mon, 2 Feb 2015 15:00:45 +0000 (10:00 -0500)]
Cherry-pick security-relevant fixes from upstream imath library.
This covers alterations to buffer sizing and zeroing made between imath
1.3 and imath 1.20. Valgrind Memcheck identified the buffer overruns
and reliance on uninitialized data; their exploit potential is unknown.
Builds specifying --with-openssl are unaffected, because they use the
OpenSSL BIGNUM facility instead of imath. Back-patch to 9.0 (all
supported versions).
Security: CVE-2015-0243
Noah Misch [Mon, 2 Feb 2015 15:00:45 +0000 (10:00 -0500)]
Fix buffer overrun after incomplete read in pullf_read_max().
Most callers pass a stack buffer. The ensuing stack smash can crash the
server, and we have not ruled out the viability of attacks that lead to
privilege escalation. Back-patch to 9.0 (all supported versions).
Marko Tiikkaja
Security: CVE-2015-0243
Bruce Momjian [Mon, 2 Feb 2015 15:00:45 +0000 (10:00 -0500)]
port/snprintf(): fix overflow and do padding
Prevent port/snprintf() from overflowing its local fixed-size
buffer and pad to the desired number of digits with zeros, even
if the precision is beyond the ability of the native sprintf().
port/snprintf() is only used on systems that lack a native
snprintf().
Reported by Bruce Momjian. Patch by Tom Lane. Backpatch to all
supported versions.
Security: CVE-2015-0242
Bruce Momjian [Mon, 2 Feb 2015 15:00:45 +0000 (10:00 -0500)]
to_char(): prevent writing beyond the allocated buffer
Previously very long localized month and weekday strings could
overflow the allocated buffers, causing a server crash.
Reported and patch reviewed by Noah Misch. Backpatch to all
supported versions.
Security: CVE-2015-0241
Bruce Momjian [Mon, 2 Feb 2015 15:00:44 +0000 (10:00 -0500)]
to_char(): prevent accesses beyond the allocated buffer
Previously very long field masks for floats could access memory
beyond the existing buffer allocated to hold the result.
Reported by Andres Freund and Peter Geoghegan. Backpatch to all
supported versions.
Security: CVE-2015-0241
Tom Lane [Mon, 2 Feb 2015 05:18:54 +0000 (00:18 -0500)]
Doc: fix syntax description for psql's \setenv.
The variable name isn't optional --- looks like a copy-and-paste-o from
the \set command, where it is.
Dilip Kumar
Peter Eisentraut [Mon, 2 Feb 2015 04:08:39 +0000 (23:08 -0500)]
Translation updates
Source-Git-URL: git://git.postgresql.org/git/pgtranslation/messages.git
Source-Git-Hash:
2ba4cf334b8ed1d46593e3127ecc673eb96bc7a8
Peter Eisentraut [Mon, 2 Feb 2015 03:36:44 +0000 (22:36 -0500)]
doc: Improve claim about location of pg_service.conf
The previous wording claimed that the file was always in /etc, but of
course this varies with the installation layout. Write instead that it
can be found via `pg_config --sysconfdir`. Even though this is still
somewhat incorrect because it doesn't account of moved installations, it
at least conveys that the location depends on the installation.
Tom Lane [Sun, 1 Feb 2015 21:53:20 +0000 (16:53 -0500)]
Release notes for 9.4.1, 9.3.6, 9.2.10, 9.1.15, 9.0.19.
Tom Lane [Sat, 31 Jan 2015 23:35:21 +0000 (18:35 -0500)]
Fix documentation of psql's ECHO all mode.
"ECHO all" is ignored for interactive input, and has been for a very long
time, though possibly not for as long as the documentation has claimed the
opposite. Fix that, and also note that empty lines aren't echoed, which
while dubious is another longstanding behavior (it's embedded in our
regression test files for one thing). Per bug #12721 from Hans Ginzel.
In HEAD, also improve the code comments in this area, and suppress an
unnecessary fflush(stdout) when we're not echoing. That would likely
be safe to back-patch, but I'll not risk it mere hours before a release
wrap.
Tom Lane [Sat, 31 Jan 2015 03:45:44 +0000 (22:45 -0500)]
Update time zone data files to tzdata release 2015a.
DST law changes in Chile and Mexico (state of Quintana Roo).
Historical changes for Iceland.
Tom Lane [Fri, 30 Jan 2015 18:05:01 +0000 (13:05 -0500)]
Fix Coverity warning about contrib/pgcrypto's mdc_finish().
Coverity points out that mdc_finish returns a pointer to a local buffer
(which of course is gone as soon as the function returns), leaving open
a risk of misbehaviors possibly as bad as a stack overwrite.
In reality, the only possible call site is in process_data_packets()
which does not examine the returned pointer at all. So there's no
live bug, but nonetheless the code is confusing and risky. Refactor
to avoid the issue by letting process_data_packets() call mdc_finish()
directly instead of going through the pullf_read() API.
Although this is only cosmetic, it seems good to back-patch so that
the logic in pgp-decrypt.c stays in sync across all branches.
Marko Kreen
Tom Lane [Fri, 30 Jan 2015 17:30:43 +0000 (12:30 -0500)]
Fix assorted oversights in range selectivity estimation.
calc_rangesel() failed outright when comparing range variables to empty
constant ranges with < or >=, as a result of missing cases in a switch.
It also produced a bogus estimate for > comparison to an empty range.
On top of that, the >= and > cases were mislabeled throughout. For
nonempty constant ranges, they managed to produce the right answers
anyway as a result of counterbalancing typos.
Also, default_range_selectivity() omitted cases for elem <@ range,
range &< range, and range &> range, so that rather dubious defaults
were applied for these operators.
In passing, rearrange the code in rangesel() so that the elem <@ range
case is handled in a less opaque fashion.
Report and patch by Emre Hasegeli, some additional work by me
Kevin Grittner [Fri, 30 Jan 2015 15:01:36 +0000 (09:01 -0600)]
Allow pg_dump to use jobs and serializable transactions together.
Since 9.3, when the --jobs option was introduced, using it together
with the --serializable-deferrable option generated multiple
errors. We can get correct behavior by allowing the connection
which acquires the snapshot to use SERIALIZABLE, READ ONLY,
DEFERRABLE and pass that to the workers running the other
connections using REPEATABLE READ, READ ONLY. This is a bit of a
kluge since the SERIALIZABLE behavior is achieved by running some
of the participating connections at a different isolation level,
but it is a simple and safe change, suitable for back-patching.
This will be followed by a proposal for a more invasive fix with
some slight behavioral changes on just the master branch, based on
suggestions from Andres Freund, but the kluge will be applied to
master until something is agreed along those lines.
Back-patched to 9.3, where the --jobs option was added.
Based on report from Alexander Korotkov
Stephen Frost [Fri, 30 Jan 2015 02:59:51 +0000 (21:59 -0500)]
Fix BuildIndexValueDescription for expressions
In
804b6b6db4dcfc590a468e7be390738f9f7755fb we modified
BuildIndexValueDescription to pay attention to which columns are visible
to the user, but unfortunatley that commit neglected to consider indexes
which are built on expressions.
Handle error-reporting of violations of constraint indexes based on
expressions by not returning any detail when the user does not have
table-level SELECT rights.
Backpatch to 9.0, as the prior commit was.
Pointed out by Tom.
Tom Lane [Fri, 30 Jan 2015 01:18:40 +0000 (20:18 -0500)]
Handle unexpected query results, especially NULLs, safely in connectby().
connectby() didn't adequately check that the constructed SQL query returns
what it's expected to; in fact, since commit
08c33c426bfebb32 it wasn't
checking that at all. This could result in a null-pointer-dereference
crash if the constructed query returns only one column instead of the
expected two. Less excitingly, it could also result in surprising data
conversion failures if the constructed query returned values that were
not I/O-conversion-compatible with the types specified by the query
calling connectby().
In all branches, insist that the query return at least two columns;
this seems like a minimal sanity check that can't break any reasonable
use-cases.
In HEAD, insist that the constructed query return the types specified by
the outer query, including checking for typmod incompatibility, which the
code never did even before it got broken. This is to hide the fact that
the implementation does a conversion to text and back; someday we might
want to improve that.
In back branches, leave that alone, since adding a type check in a minor
release is more likely to break things than make people happy. Type
inconsistencies will continue to work so long as the actual type and
declared type are I/O representation compatible, and otherwise will fail
the same way they used to.
Also, in all branches, be on guard for NULL results from the constructed
query, which formerly would cause null-pointer dereference crashes.
We now print the row with the NULL but don't recurse down from it.
In passing, get rid of the rather pointless idea that
build_tuplestore_recursively() should return the same tuplestore that's
passed to it.
Michael Paquier, adjusted somewhat by me
Andres Freund [Thu, 29 Jan 2015 16:49:03 +0000 (17:49 +0100)]
Properly terminate the array returned by GetLockConflicts().
GetLockConflicts() has for a long time not properly terminated the
returned array. During normal processing the returned array is zero
initialized which, while not pretty, is sufficient to be recognized as
a invalid virtual transaction id. But the HotStandby case is more than
aesthetically broken: The allocated (and reused) array is neither
zeroed upon allocation, nor reinitialized, nor terminated.
Not having a terminating element means that the end of the array will
not be recognized and that recovery conflict handling will thus read
ahead into adjacent memory. Only terminating when hitting memory
content that looks like a invalid virtual transaction id. Luckily
this seems so far not have caused significant problems, besides making
recovery conflict more expensive.
Discussion:
20150127142713.GD29457@awork2.anarazel.de
Backpatch into all supported branches.
Heikki Linnakangas [Thu, 29 Jan 2015 17:35:55 +0000 (19:35 +0200)]
Fix bug where GIN scan keys were not initialized with gin_fuzzy_search_limit.
When gin_fuzzy_search_limit was used, we could jump out of startScan()
without calling startScanKey(). That was harmless in 9.3 and below, because
startScanKey()() didn't do anything interesting, but in 9.4 it initializes
information needed for skipping entries (aka GIN fast scans), and you
readily get a segfault if it's not done. Nevertheless, it was clearly wrong
all along, so backpatch all the way to 9.1 where the early return was
introduced.
(AFAICS startScanKey() did nothing useful in 9.3 and below, because the
fields it initialized were already initialized in ginFillScanKey(), but I
don't dare to change that in a minor release. ginFillScanKey() is always
called in gingetbitmap() even though there's a check there to see if the
scan keys have already been initialized, because they never are; ginrescan()
free's them.)
In the passing, remove unnecessary if-check from the second inner loop in
startScan(). We already check in the first loop that the condition is true
for all entries.
Reported by Olaf Gawenda, bug #12694, Backpatch to 9.1 and above, although
AFAICS it causes a live bug only in 9.4.
Stephen Frost [Wed, 28 Jan 2015 22:43:02 +0000 (17:43 -0500)]
Clean up range-table building in copy.c
Commit
804b6b6db4dcfc590a468e7be390738f9f7755fb added the build of a
range table in copy.c to initialize the EState es_range_table since it
can be needed in error paths. Unfortunately, that commit didn't
appreciate that some code paths might end up not initializing the rte
which is used to build the range table.
Fix that and clean up a couple others things along the way- build it
only once and don't explicitly set it on the !is_from path as it
doesn't make any sense there (cstate is palloc0'd, so this isn't an
issue from an initializing standpoint either).
The prior commit went back to 9.0, but this only goes back to 9.1 as
prior to that the range table build happens immediately after building
the RTE and therefore doesn't suffer from this issue.
Pointed out by Robert.
Stephen Frost [Mon, 12 Jan 2015 22:04:11 +0000 (17:04 -0500)]
Fix column-privilege leak in error-message paths
While building error messages to return to the user,
BuildIndexValueDescription, ExecBuildSlotValueDescription and
ri_ReportViolation would happily include the entire key or entire row in
the result returned to the user, even if the user didn't have access to
view all of the columns being included.
Instead, include only those columns which the user is providing or which
the user has select rights on. If the user does not have any rights
to view the table or any of the columns involved then no detail is
provided and a NULL value is returned from BuildIndexValueDescription
and ExecBuildSlotValueDescription. Note that, for key cases, the user
must have access to all of the columns for the key to be shown; a
partial key will not be returned.
Back-patch all the way, as column-level privileges are now in all
supported versions.
This has been assigned CVE-2014-8161, but since the issue and the patch
have already been publicized on pgsql-hackers, there's no point in trying
to hide this commit.
Tom Lane [Tue, 27 Jan 2015 17:06:38 +0000 (12:06 -0500)]
Fix NUMERIC field access macros to treat NaNs consistently.
Commit
145343534c153d1e6c3cff1fa1855787684d9a38 arranged to store numeric
NaN values as short-header numerics, but the field access macros did not
get the memo: they thought only "SHORT" numerics have short headers.
Most of the time this makes no difference because we don't access the
weight or dscale of a NaN; but numeric_send does that. As pointed out
by Andrew Gierth, this led to fetching uninitialized bytes.
AFAICS this could not have any worse consequences than that; in particular,
an unaligned stored numeric would have been detoasted by PG_GETARG_NUMERIC,
so that there's no risk of a fetch off the end of memory. Still, the code
is wrong on its own terms, and it's not hard to foresee future changes that
might expose us to real risks. So back-patch to all affected branches.
Tom Lane [Mon, 26 Jan 2015 20:17:39 +0000 (15:17 -0500)]
Fix volatile-safety issue in dblink's materializeQueryResult().
Some fields of the sinfo struct are modified within PG_TRY and then
referenced within PG_CATCH, so as with recent patch to async.c, "volatile"
is necessary for strict POSIX compliance; and that propagates to a couple
of subroutines as well as materializeQueryResult() itself. I think the
risk of actual issues here is probably higher than in async.c, because
storeQueryResult() is likely to get inlined into materializeQueryResult(),
leaving the compiler free to conclude that its stores into sinfo fields are
dead code.
Tom Lane [Mon, 26 Jan 2015 17:18:25 +0000 (12:18 -0500)]
Fix volatile-safety issue in pltcl_SPI_execute_plan().
The "callargs" variable is modified within PG_TRY and then referenced
within PG_CATCH, which is exactly the coding pattern we've now found
to be unsafe. Marking "callargs" volatile would be problematic because
it is passed by reference to some Tcl functions, so fix the problem
by not modifying it within PG_TRY. We can just postpone the free()
till we exit the PG_TRY construct, as is already done elsewhere in this
same file.
Also, fix failure to free(callargs) when exiting on too-many-arguments
error. This is only a minor memory leak, but a leak nonetheless.
In passing, remove some unnecessary "volatile" markings in the same
function. Those doubtless are there because gcc 2.95.3 whinged about
them, but we now know that its algorithm for complaining is many bricks
shy of a load.
This is certainly a live bug with compilers that optimize similarly
to current gcc, so back-patch to all active branches.
Tom Lane [Mon, 26 Jan 2015 16:57:40 +0000 (11:57 -0500)]
Fix volatile-safety issue in asyncQueueReadAllNotifications().
The "pos" variable is modified within PG_TRY and then referenced
within PG_CATCH, so for strict POSIX conformance it must be marked
volatile. Superficially the code looked safe because pos's address
was taken, which was sufficient to force it into memory ... but it's
not sufficient to ensure that the compiler applies updates exactly
where the program text says to. The volatility marking has to extend
into a couple of subroutines too, but I think that's probably a good
thing because the risk of out-of-order updates is mostly in those
subroutines not asyncQueueReadAllNotifications() itself. In principle
the compiler could have re-ordered operations such that an error could
be thrown while "pos" had an incorrect value.
It's unclear how real the risk is here, but for safety back-patch
to all active branches.
Tom Lane [Sat, 24 Jan 2015 18:05:49 +0000 (13:05 -0500)]
Replace a bunch more uses of strncpy() with safer coding.
strncpy() has a well-deserved reputation for being unsafe, so make an
effort to get rid of nearly all occurrences in HEAD.
A large fraction of the remaining uses were passing length less than or
equal to the known strlen() of the source, in which case no null-padding
can occur and the behavior is equivalent to memcpy(), though doubtless
slower and certainly harder to reason about. So just use memcpy() in
these cases.
In other cases, use either StrNCpy() or strlcpy() as appropriate (depending
on whether padding to the full length of the destination buffer seems
useful).
I left a few strncpy() calls alone in the src/timezone/ code, to keep it
in sync with upstream (the IANA tzcode distribution). There are also a
few such calls in ecpg that could possibly do with more analysis.
AFAICT, none of these changes are more than cosmetic, except for the four
occurrences in fe-secure-openssl.c, which are in fact buggy: an overlength
source leads to a non-null-terminated destination buffer and ensuing
misbehavior. These don't seem like security issues, first because no stack
clobber is possible and second because if your values of sslcert etc are
coming from untrusted sources then you've got problems way worse than this.
Still, it's undesirable to have unpredictable behavior for overlength
inputs, so back-patch those four changes to all active branches.
Tom Lane [Wed, 21 Jan 2015 02:21:34 +0000 (21:21 -0500)]
Improve documentation of random() function.
Move random() and setseed() to a separate table, to have them grouped
together. Also add a notice that random() is not cryptographically secure.
Back-patch of commit
75fdcec14543b60cc0c67483d8cc47d5c7adf1a8 into
all supported versions, per discussion of the need to document that
random() is just a wrapper around random(3).
Tom Lane [Tue, 20 Jan 2015 04:44:24 +0000 (23:44 -0500)]
In pg_regress, remove the temporary installation upon successful exit.
This results in a very substantial reduction in disk space usage during
"make check-world", since that sequence involves creation of numerous
temporary installations. It should also help a bit in the buildfarm, even
though the buildfarm script doesn't create as many temp installations,
because the current script misses deleting some of them; and anyway it
seems better to do this once in one place rather than expecting that
script to get it right every time.
In 9.4 and HEAD, also undo the unwise choice in commit
b1aebbb6a86e96d7
to report strerror(errno) after a rmtree() failure. rmtree has already
reported that, possibly for multiple failures with distinct errnos; and
what's more, by the time it returns there is no good reason to assume
that errno still reflects the last reportable error. So reporting errno
here is at best redundant and at worst badly misleading.
Back-patch to all supported branches, so that future revisions of the
buildfarm script can rely on this behavior.
Tom Lane [Tue, 20 Jan 2015 04:01:39 +0000 (23:01 -0500)]
Adjust "pgstat wait timeout" message to be a translatable LOG message.
Per discussion, change the log level of this message to be LOG not WARNING.
The main point of this change is to avoid causing buildfarm run failures
when the stats collector is exceptionally slow to respond, which it not
infrequently is on some of the smaller/slower buildfarm members.
This change does lose notice to an interactive user when his stats query
is looking at out-of-date stats, but the majority opinion (not necessarily
that of yours truly) is that WARNING messages would probably not get
noticed anyway on heavily loaded production systems. A LOG message at
least ensures that the problem is recorded somewhere where bulk auditing
for the issue is possible.
Also, instead of an untranslated "pgstat wait timeout" message, provide
a translatable and hopefully more understandable message "using stale
statistics instead of current ones because stats collector is not
responding". The original text was written hastily under the assumption
that it would never really happen in practice, which we now know to be
unduly optimistic.
Back-patch to all active branches, since we've seen the buildfarm issue
in all branches.
Andres Freund [Sun, 18 Jan 2015 14:57:55 +0000 (15:57 +0100)]
Fix use of already freed memory when dumping a database's security label.
pg_dump.c:dumDatabase() called ArchiveEntry() with the results of a a
query that was PQclear()ed a couple lines earlier.
Backpatch to 9.2 where security labels for shared objects where
introduced.
Peter Eisentraut [Wed, 7 Jan 2015 04:06:13 +0000 (23:06 -0500)]
Fix namespace handling in xpath function
Previously, the xml value resulting from an xpath query would not have
namespace declarations if the namespace declarations were attached to
an ancestor element in the input xml value. That means the output value
was not correct XML. Fix that by running the result value through
xmlCopyNode(), which produces the correct namespace declarations.
Author: Ali Akbar <the.apaan@gmail.com>
Heikki Linnakangas [Fri, 16 Jan 2015 10:12:49 +0000 (12:12 +0200)]
Another attempt at fixing Windows Norwegian locale.
Previous fix mapped "Norwegian (Bokmål)" locale, which contains a non-ASCII
character, to the pure ASCII alias "norwegian-bokmal". However, it turns
out that more recent versions of the CRT library, in particular MSVCR110
(Visual Studio 2012), changed the behaviour of setlocale() so that if
you pass "norwegian-bokmal" to setlocale, it returns "Norwegian_Norway".
That meant trouble, when setlocale(..., NULL) first returned
"Norwegian (Bokmål)_Norway", which we mapped to "norwegian-bokmal_Norway",
but another call to setlocale(..., "norwegian-bokmal_Norway") returned
"Norwegian_Norway". That caused PostgreSQL to think that they are different
locales, and therefore not compatible. That caused initdb to fail at
CREATE DATABASE.
Older CRT versions seem to accept "Norwegian_Norway" too, so change the
mapping to return "Norwegian_Norway" instead of "norwegian-bokmal".
Backpatch to 9.2 like the previous attempt. We haven't made a release that
includes the previous fix yet, so we don't need to worry about changing the
locale of existing clusters from "norwegian-bokmal" to "Norwegian_Norway".
(Doing any mapping like this at all requires changing the locale of
existing databases; the release notes need to include instructions for
that).
Noah Misch [Fri, 16 Jan 2015 06:27:31 +0000 (01:27 -0500)]
Update "pg_regress --no-locale" for Darwin and Windows.
Commit
894459e59ffa5c7fee297b246c17e1f72564db1d revealed this option to
be broken for NLS builds on Darwin, but "make -C contrib/unaccent check"
and the buildfarm client rely on it. Fix that configuration by
redefining the option to imply LANG=C on Darwin. In passing, use LANG=C
instead of LANG=en on Windows; since only postmaster startup uses that
value, testers are unlikely to notice the change. Back-patch to 9.0,
like the predecessor commit.
Tom Lane [Thu, 15 Jan 2015 23:52:28 +0000 (18:52 -0500)]
Fix use-of-already-freed-memory problem in EvalPlanQual processing.
Up to now, the "child" executor state trees generated for EvalPlanQual
rechecks have simply shared the ResultRelInfo arrays used for the original
execution tree. However, this leads to dangling-pointer problems, because
ExecInitModifyTable() is all too willing to scribble on some fields of the
ResultRelInfo(s) even when it's being run in one of those child trees.
This trashes those fields from the perspective of the parent tree, because
even if the generated subtree is logically identical to what was in use in
the parent, it's in a memory context that will go away when we're done
with the child state tree.
We do however want to share information in the direction from the parent
down to the children; in particular, fields such as es_instrument *must*
be shared or we'll lose the stats arising from execution of the children.
So the simplest fix is to make a copy of the parent's ResultRelInfo array,
but not copy any fields back at end of child execution.
Per report from Manuel Kniep. The added isolation test is based on his
example. In an unpatched memory-clobber-enabled build it will reliably
fail with "ctid is NULL" errors in all branches back to 9.1, as a
consequence of junkfilter->jf_junkAttNo being overwritten with $7f7f.
This test cannot be run as-is before that for lack of WITH syntax; but
I have no doubt that some variant of this problem can arise in older
branches, so apply the code change all the way back.
Tom Lane [Thu, 15 Jan 2015 18:18:19 +0000 (13:18 -0500)]
Improve performance of EXPLAIN with large range tables.
As of 9.3, ruleutils.c goes to some lengths to ensure that table and column
aliases used in its output are unique. Of course this takes more time than
was required before, which in itself isn't fatal. However, EXPLAIN was set
up so that recalculation of the unique aliases was repeated for each
subexpression printed in a plan. That results in O(N^2) time and memory
consumption for large plan trees, which did not happen in older branches.
Fortunately, the expensive work is the same across a whole plan tree,
so there is no need to repeat it; we can do most of the initialization
just once per query and re-use it for each subexpression. This buys
back most (not all) of the performance loss since 9.2.
We need an extra ExplainState field to hold the precalculated deparse
context. That's no problem in HEAD, but in the back branches, expanding
sizeof(ExplainState) seems risky because third-party extensions might
have local variables of that struct type. So, in 9.4 and 9.3, introduce
an auxiliary struct to keep sizeof(ExplainState) the same. We should
refactor the APIs to avoid such local variables in future, but that's
material for a separate HEAD-only commit.
Per gripe from Alexey Bashtanov. Back-patch to 9.3 where the issue
was introduced.
Robert Haas [Thu, 15 Jan 2015 14:26:03 +0000 (09:26 -0500)]
pg_standby: Avoid writing one byte beyond the end of the buffer.
Previously, read() might have returned a length equal to the buffer
length, and then the subsequent store to buf[len] would write a
zero-byte one byte past the end. This doesn't seem likely to be
a security issue, but there's some chance it could result in
pg_standby misbehaving.
Spotted by Coverity; patch by Michael Paquier, reviewed by me.
Andres Freund [Tue, 13 Jan 2015 20:02:47 +0000 (21:02 +0100)]
Make logging_collector=on work with non-windows EXEC_BACKEND again.
Commit
b94ce6e80 reordered postmaster's startup sequence so that the
tempfile directory is only cleaned up after all the necessary state
for pg_ctl is collected. Unfortunately the chosen location is after
the syslogger has been started; which normally is fine, except for
!WIN32 EXEC_BACKEND builds, which pass information to children via
files in the temp directory.
Move the call to RemovePgTempFiles() to just before the syslogger has
started. That's the first child we fork.
Luckily EXEC_BACKEND is pretty much only used by endusers on windows,
which has a separate method to pass information to children. That
means the real world impact of this bug is very small.
Discussion:
20150113182344.GF12272@alap3.anarazel.de
Backpatch to 9.1, just as the previous commit was.
Tom Lane [Mon, 12 Jan 2015 21:08:49 +0000 (16:08 -0500)]
Fix some functions that were declared static then defined not-static.
Per testing with a compiler that whines about this.
Tom Lane [Mon, 12 Jan 2015 20:13:34 +0000 (15:13 -0500)]
Avoid unexpected slowdown in vacuum regression test.
I noticed the "vacuum" regression test taking really significantly longer
than it used to on a slow machine. Investigation pointed the finger at
commit
e415b469b33ba328765e39fd62edcd28f30d9c3c, which added creation of
an index using an extremely expensive index function. That function was
evidently meant to be applied only twice ... but the test re-used an
existing test table, which up till a couple lines before that had had over
two thousand rows. Depending on timing of the concurrent regression tests,
the intervening VACUUMs might have been unable to remove those
recently-dead rows, and then the index build would need to create index
entries for them too, leading to the wrap_do_analyze() function being
executed 2000+ times not twice. Avoid this by using a different table
that is guaranteed to have only the intended two rows in it.
Back-patch to 9.0, like the commit that created the problem.
Tom Lane [Mon, 12 Jan 2015 17:40:16 +0000 (12:40 -0500)]
Use correct text domain for errcontext() appearing within ereport().
The mechanism added in commit
dbdf9679d7d61b03a3bf73af9b095831b7010eb5
for associating the correct translation domain with errcontext strings
potentially fails in cases where errcontext() is used within an ereport()
macro. Such usage was not originally envisioned for errcontext(), but we
do have a few places that do it. In this situation, the intended comma
expression becomes just a couple of arguments to errfinish(), which the
compiler might choose to evaluate right-to-left.
Fortunately, in such cases the textdomain for the errcontext string must
be the same as for the surrounding ereport. So we can fix this by letting
errstart initialize context_domain along with domain; then it will have
the correct value no matter which order the calls occur in. (Note that
error stack callback functions are not invoked until errfinish, so normal
usage of errcontext won't affect what happens for errcontext calls within
the ereport macro.)
In passing, make sure that errcontext calls within the main backend set
context_domain to something non-NULL. This isn't a live bug because
NULL would select the current textdomain() setting which should be the
right thing anyway --- but it seems better to handle this completely
consistently with the regular domain field.
Per report from Dmitry Voronin. Backpatch to 9.3; before that, there
wasn't any attempt to ensure that errcontext strings were translated
in an appropriate domain.
Stephen Frost [Mon, 12 Jan 2015 15:13:18 +0000 (10:13 -0500)]
Skip dead backends in MinimumActiveBackends
Back in
ed0b409, PGPROC was split and moved to static variables in
procarray.c, with procs in ProcArrayStruct replaced by an array of
integers representing process numbers (pgprocnos), with -1 indicating a
dead process which has yet to be removed. Access to procArray is
generally done under ProcArrayLock and therefore most code does not have
to concern itself with -1 entries.
However, MinimumActiveBackends intentionally does not take
ProcArrayLock, which means it has to be extra careful when accessing
procArray. Prior to
ed0b409, this was handled by checking for a NULL
in the pointer array, but that check was no longer valid after the
split. Coverity pointed out that the check could never happen and so
it was removed in
5592eba. That didn't make anything worse, but it
didn't fix the issue either.
The correct fix is to check for pgprocno == -1 and skip over that entry
if it is encountered.
Back-patch to 9.2, since there can be attempts to access the arrays
prior to their start otherwise. Note that the changes prior to 9.4 will
look a bit different due to the change in
5592eba.
Note that MinimumActiveBackends only returns a bool for heuristic
purposes and any pre-array accesses are strictly read-only and so there
is no security implication and the lack of fields complaints indicates
it's very unlikely to run into issues due to this.
Pointed out by Noah.
Alvaro Herrera [Fri, 9 Jan 2015 15:34:24 +0000 (12:34 -0300)]
xlogreader.c: Fix report_invalid_record translatability flag
For some reason I overlooked in GETTEXT_TRIGGERS that the right argument
be read by gettext in
7fcbf6a405ffc12a4546a25b98592ee6733783fc. This
will drop the translation percentages for the backend all the way back
to 9.3 ...
Problem reported by Heikki.
Noah Misch [Thu, 8 Jan 2015 03:35:44 +0000 (22:35 -0500)]
On Darwin, detect and report a multithreaded post
Darwin --enable-nls builds use a substitute setlocale() that may start a
thread. Buildfarm member orangutan experienced BackendList corruption
on account of different postmaster threads executing signal handlers
simultaneously. Furthermore, a multithreaded postmaster risks undefined
behavior from sigprocmask() and fork(). Emit LOG messages about the
problem and its workaround. Back-patch to 9.0 (all supported versions).
Noah Misch [Thu, 8 Jan 2015 03:34:57 +0000 (22:34 -0500)]
Always set the six locale category environment variables in main().
Typical server invocations already achieved that. Invalid locale
settings in the initial postmaster environment interfered, as could
malloc() failure. Setting "LC_MESSAGES=pt_BR.utf8 LC_ALL=invalid" in
the postmaster environment will now choose C-locale messages, not
Brazilian Portuguese messages. Most localized programs, including all
PostgreSQL frontend executables, do likewise. Users are unlikely to
observe changes involving locale categories other than LC_MESSAGES.
CheckMyDatabase() ensures that we successfully set LC_COLLATE and
LC_CTYPE; main() sets the remaining three categories to locale "C",
which almost cannot fail. Back-patch to 9.0 (all supported versions).
Noah Misch [Thu, 8 Jan 2015 03:33:58 +0000 (22:33 -0500)]
Reject ANALYZE commands during VACUUM FULL or another ANALYZE.
vacuum()'s static variable handling makes it non-reentrant; an ensuing
null pointer deference crashed the backend. Back-patch to 9.0 (all
supported versions).
Andres Freund [Tue, 6 Jan 2015 23:10:18 +0000 (00:10 +0100)]
Improve relcache invalidation handling of currently invisible relations.
The corner case where a relcache invalidation tried to rebuild the
entry for a referenced relation but couldn't find it in the catalog
wasn't correct.
The code tried to RelationCacheDelete/RelationDestroyRelation the
entry. That didn't work when assertions are enabled because the latter
contains an assertion ensuring the refcount is zero. It's also more
generally a bad idea, because by virtue of being referenced somebody
might actually look at the entry, which is possible if the error is
trapped and handled via a subtransaction abort.
Instead just error out, without deleting the entry. As the entry is
marked invalid, the worst that can happen is that the invalid (and at
some point unused) entry lingers in the relcache.
Discussion: 22459.
1418656530@sss.pgh.pa.us
There should be no way to hit this case < 9.4 where logical decoding
introduced a bug that can hit this. But since the code for handling
the corner case is there it should do something halfway sane, so
backpatch all the the way back. The logical decoding bug will be
handled in a separate commit.
Alvaro Herrera [Tue, 6 Jan 2015 18:16:29 +0000 (15:16 -0300)]
Fix thinko in plpython error message
Bruce Momjian [Tue, 6 Jan 2015 16:43:46 +0000 (11:43 -0500)]
Update copyright for 2015
Backpatch certain files through 9.0
Tom Lane [Tue, 6 Jan 2015 00:27:09 +0000 (19:27 -0500)]
Fix broken pg_dump code for dumping comments on event triggers.
This never worked, I think. Per report from Marc Munro.
In passing, fix funny spacing in the COMMENT ON command as a result of
excess space in the "label" string.
Alvaro Herrera [Sun, 4 Jan 2015 18:48:29 +0000 (15:48 -0300)]
Fix thinko in lock mode enum
Commit
0e5680f4737a9c6aa94aa9e77543e5de60411322 contained a thinko
mixing LOCKMODE with LockTupleMode. This caused misbehavior in the case
where a tuple is marked with a multixact with at most a FOR SHARE lock,
and another transaction tries to acquire a FOR NO KEY EXCLUSIVE lock;
this case should block but doesn't.
Include a new isolation tester spec file to explicitely try all the
tuple lock combinations; without the fix it shows the problem:
starting permutation: s1_begin s1_lcksvpt s1_tuplock2 s2_tuplock3 s1_commit
step s1_begin: BEGIN;
step s1_lcksvpt: SELECT * FROM multixact_conflict FOR KEY SHARE; SAVEPOINT foo;
a
1
step s1_tuplock2: SELECT * FROM multixact_conflict FOR SHARE;
a
1
step s2_tuplock3: SELECT * FROM multixact_conflict FOR NO KEY UPDATE;
a
1
step s1_commit: COMMIT;
With the fixed code, step s2_tuplock3 blocks until session 1 commits,
which is the correct behavior.
All other cases behave correctly.
Backpatch to 9.3, like the commit that introduced the problem.
Andres Freund [Sun, 4 Jan 2015 14:44:49 +0000 (15:44 +0100)]
Correctly handle test durations of more than 2147s in pg_test_timing.
Previously the computation of the total test duration, measured in
microseconds, accidentally overflowed due to accidentally using signed
32bit arithmetic. As the only consequence is that pg_test_timing
invocations with such, overly large, durations never finished the
practical consequences of this bug are minor.
Pointed out by Coverity.
Backpatch to 9.2 where pg_test_timing was added.
Andres Freund [Sun, 4 Jan 2015 14:35:47 +0000 (15:35 +0100)]
Fix off-by-one in pg_xlogdump's fuzzy_open_file().
In the unlikely case of stdin (fd 0) being closed, the off-by-one
would lead to pg_xlogdump failing to open files.
Spotted by Coverity.
Backpatch to 9.3 where pg_xlogdump was introduced.
Andres Freund [Sun, 4 Jan 2015 14:35:47 +0000 (15:35 +0100)]
Add missing va_end() call to a early exit in dmetaphone.c's StringAt().
Pointed out by Coverity.
Backpatch to all supported branches, the code has been that way for a
long while.
Andres Freund [Sun, 4 Jan 2015 13:36:22 +0000 (14:36 +0100)]
Fix inconsequential fd leak in the new mark_file_as_archived() function.
As every error in mark_file_as_archived() will lead to a failure of
pg_basebackup the FD leak couldn't ever lead to a real problem. It
seems better to fix the leak anyway though, rather than silence
Coverity, as the usage of the function might get extended or copied at
some point in the future.
Pointed out by Coverity.
Backpatch to 9.2, like the relevant part of the previous patch.
Andres Freund [Sat, 3 Jan 2015 19:51:52 +0000 (20:51 +0100)]
Prevent WAL files created by pg_basebackup -x/X from being archived again.
WAL (and timeline history) files created by pg_basebackup did not
maintain the new base backup's archive status. That's currently not a
problem if the new node is used as a standby - but if that node is
promoted all still existing files can get archived again. With a high
wal_keep_segment settings that can happen a significant time later -
which is quite confusing.
Change both the backend (for the -x/-X fetch case) and pg_basebackup
(for -X stream) itself to always mark WAL/timeline files included in
the base backup as .done. That's in line with walreceiver.c doing so.
The verbosity of the pg_basebackup changes show pretty clearly that it
needs some refactoring, but that'd result in not be backpatchable
changes.
Backpatch to 9.1 where pg_basebackup was introduced.
Discussion:
20141205002854.GE21964@awork2.anarazel.de
Andres Freund [Sat, 3 Jan 2015 19:51:52 +0000 (20:51 +0100)]
Add pg_string_endswith as the start of a string helper library in src/common.
Backpatch to 9.3 where src/common was introduce, because a bugfix that
needs to be backpatched, requires the function. Earlier branches will
have to duplicate the code.
Magnus Hagander [Sat, 3 Jan 2015 12:18:54 +0000 (13:18 +0100)]
Make path to pg_service.conf absolute in documentation
The system file is always in the absolute path /etc/, not relative.
David Fetter
Tom Lane [Wed, 31 Dec 2014 21:42:48 +0000 (16:42 -0500)]
Docs: improve descriptions of ISO week-numbering date features.
Use the phraseology "ISO 8601 week-numbering year" in place of just
"ISO year", and make related adjustments to other terminology.
The point of this change is that it seems some people see "ISO year"
and think "standard year", whereupon they're surprised when constructs
like to_char(..., "IYYY-MM-DD") produce nonsensical results. Perhaps
hanging a few more adjectives on it will discourage them from jumping
to false conclusions. I put in an explicit warning against that
specific usage, too, though the main point is to discourage people
who haven't read this far down the page.
In passing fix some nearby markup and terminology inconsistencies.
Tom Lane [Wed, 31 Dec 2014 17:17:00 +0000 (12:17 -0500)]
Improve consistency of parsing of psql's magic variables.
For simple boolean variables such as ON_ERROR_STOP, psql has for a long
time recognized variant spellings of "on" and "off" (such as "1"/"0"),
and it also made a point of warning you if you'd misspelled the setting.
But these conveniences did not exist for other keyword-valued variables.
In particular, though ECHO_HIDDEN and ON_ERROR_ROLLBACK include "on" and
"off" as possible values, none of the alternative spellings for those were
recognized; and to make matters worse the code would just silently assume
"on" was meant for any unrecognized spelling. Several people have reported
getting bitten by this, so let's fix it. In detail, this patch:
* Allows all spellings recognized by ParseVariableBool() for ECHO_HIDDEN
and ON_ERROR_ROLLBACK.
* Reports a warning for unrecognized values for COMP_KEYWORD_CASE, ECHO,
ECHO_HIDDEN, HISTCONTROL, ON_ERROR_ROLLBACK, and VERBOSITY.
* Recognizes all values for all these variables case-insensitively;
previously there was a mishmash of case-sensitive and case-insensitive
behaviors.
Back-patch to all supported branches. There is a small risk of breaking
existing scripts that were accidentally failing to malfunction; but the
consensus is that the chance of detecting real problems and preventing
future mistakes outweighs this.
Tatsuo Ishii [Tue, 30 Dec 2014 11:19:50 +0000 (20:19 +0900)]
Fix resource leak pointed out by Coverity.
Bruce Momjian [Tue, 30 Dec 2014 02:25:23 +0000 (21:25 -0500)]
Backpatch variable renaming in formatting.c
Backpatch
a9c22d1480aa8e6d97a000292d05ef2b31bbde4e to make future
backpatching easier.
Backpatch through 9.0
Tom Lane [Mon, 29 Dec 2014 19:21:00 +0000 (14:21 -0500)]
Assorted minor fixes for psql metacommand docs.
Document the long forms of \H \i \ir \o \p \r \w ... apparently, we have
a long and dishonorable history of leaving out the unabbreviated names of
psql backslash commands.
Avoid saying "Unix shell"; we can just say "shell" with equal clarity,
and not leave Windows users wondering whether the feature works for them.
Improve consistency of documentation of \g \o \w metacommands. There's
no reason to use slightly different wording or markup for each one.
Alvaro Herrera [Fri, 26 Dec 2014 16:52:27 +0000 (13:52 -0300)]
Grab heavyweight tuple lock only before sleeping
We were trying to acquire the lock even when we were subsequently
not sleeping in some other transaction, which opens us up unnecessarily
to deadlocks. In particular, this is troublesome if an update tries to
lock an updated version of a tuple and finds itself doing EvalPlanQual
update chain walking; more than two sessions doing this concurrently
will find themselves sleeping on each other because the HW tuple lock
acquisition in heap_lock_tuple called from EvalPlanQualFetch races with
the same tuple lock being acquired in heap_update -- one of these
sessions sleeps on the other one to finish while holding the tuple lock,
and the other one sleeps on the tuple lock.
Per trouble report from Andrew Sackville-West in
http://www.postgresql.org/message-id/
20140731233051.GN17765@andrew-ThinkPad-X230
His scenario can be simplified down to a relatively simple
isolationtester spec file which I don't include in this commit; the
reason is that the current isolationtester is not able to deal with more
than one blocked session concurrently and it blocks instead of raising
the expected deadlock. In the future, if we improve isolationtester, it
would be good to include the spec file in the isolation schedule. I
posted it in
http://www.postgresql.org/message-id/
20141212205254.GC1768@alvh.no-ip.org
Hat tip to Mark Kirkwood, who helped diagnose the trouble.
Noah Misch [Thu, 25 Dec 2014 18:52:03 +0000 (13:52 -0500)]
Have config_sspi_auth() permit IPv6 localhost connections.
Windows versions later than Windows Server 2003 map "localhost" to ::1.
Account for that in the generated pg_hba.conf, fixing another oversight
in commit
f6dc6dd5ba54d52c0733aaafc50da2fbaeabb8b0. Back-patch to 9.0,
like that commit.
David Rowley and Noah Misch
Tom Lane [Wed, 24 Dec 2014 21:35:23 +0000 (16:35 -0500)]
Add CST (China Standard Time) to our lists of timezone abbreviations.
For some reason this seems to have been missed when the lists in
src/timezone/tznames/ were first constructed. We can't put it in Default
because of the conflict with US CST, but we should certainly list it among
the alternative entries in Asia.txt. (I checked for other oversights, but
all the other abbreviations that are in current use according to the IANA
files seem to be accounted for.) Noted while responding to bug #12326.
Andrew Dunstan [Mon, 22 Dec 2014 23:31:38 +0000 (18:31 -0500)]
Further tidy up on json aggregate documentation
Andrew Dunstan [Mon, 22 Dec 2014 19:20:19 +0000 (14:20 -0500)]
Fix documentation of argument type of json_agg and jsonb_agg
json_agg was originally designed to aggregate records. However, it soon
became clear that it is useful for aggregating all kinds of values and
that's what we have on 9.3 and 9.4, and in head for it and jsonb_agg.
The documentation suggested otherwise, so this fixes it.
Tom Lane [Sun, 21 Dec 2014 20:30:39 +0000 (15:30 -0500)]
Docs: clarify treatment of variadic functions with zero variadic arguments.
Explain that you have to use "VARIADIC ARRAY[]" to pass an empty array
to a variadic parameter position. This was already implicit in the text
but it seems better to spell it out.
Per a suggestion from David Johnston, though I didn't use his proposed
wording. Back-patch to all supported branches.
Heikki Linnakangas [Fri, 19 Dec 2014 15:00:21 +0000 (17:00 +0200)]
Fix timestamp in end-of-recovery WAL records.
We used time(null) to set a TimestampTz field, which gave bogus results.
Noticed while looking at pg_xlogdump output.
Backpatch to 9.3 and above, where the fast promotion was introduced.
Andres Freund [Fri, 19 Dec 2014 13:29:52 +0000 (14:29 +0100)]
Prevent potentially hazardous compiler/cpu reordering during lwlock release.
In LWLockRelease() (and in 9.4+ LWLockUpdateVar()) we release enqueued
waiters using PGSemaphoreUnlock(). As there are other sources of such
unlocks backends only wake up if MyProc->lwWaiting is set to false;
which is only done in the aforementioned functions.
Before this commit there were dangers because the store to lwWaitLink
could become visible before the store to lwWaitLink. This could both
happen due to compiler reordering (on most compilers) and on some
platforms due to the CPU reordering stores.
The possible consequence of this is that a backend stops waiting
before lwWaitLink is set to NULL. If that backend then tries to
acquire another lock and has to wait there the list could become
corrupted once the lwWaitLink store is finally performed.
Add a write memory barrier to prevent that issue.
Unfortunately the barrier support has been only added in 9.2. Given
that the issue has not knowingly been observed in praxis it seems
sufficient to prohibit compiler reordering using volatile for 9.0 and
9.1. Actual problems due to compiler reordering are more likely
anyway.
Discussion:
20140210134625.GA15246@awork2.anarazel.de
Tom Lane [Thu, 18 Dec 2014 21:38:58 +0000 (16:38 -0500)]
Improve documentation about CASE and constant subexpressions.
The possibility that constant subexpressions of a CASE might be evaluated
at planning time was touched on in 9.17.1 (CASE expressions), but it really
ought to be explained in 4.2.14 (Expression Evaluation Rules) which is the
primary discussion of such topics. Add text and an example there, and
revise the <note> under CASE to link there.
Back-patch to all supported branches, since it's acted like this for a
long time (though 9.2+ is probably worse because of its more aggressive
use of constant-folding via replanning of nominally-prepared statements).
Pre-9.4, also back-patch text added in commit
0ce627d4 about CASE versus
aggregate functions.
Tom Lane and David Johnston, per discussion of bug #12273.
Noah Misch [Thu, 18 Dec 2014 08:55:17 +0000 (03:55 -0500)]
Recognize Makefile line continuations in fetchRegressOpts().
Back-patch to 9.0 (all supported versions). This is mere
future-proofing in the context of the master branch, but commit
f6dc6dd5ba54d52c0733aaafc50da2fbaeabb8b0 requires it of older branches.
Andres Freund [Thu, 18 Dec 2014 07:35:27 +0000 (08:35 +0100)]
Fix (re-)starting from a basebackup taken off a standby after a failure.
When starting up from a basebackup taken off a standby extra logic has
to be applied to compute the point where the data directory is
consistent. Normal base backups use a WAL record for that purpose, but
that isn't possible on a standby.
That logic had a error check ensuring that the cluster's control file
indicates being in recovery. Unfortunately that check was too strict,
disregarding the fact that the control file could also indicate that
the cluster was shut down while in recovery.
That's possible when the a cluster starting from a basebackup is shut
down before the backup label has been removed. When everything goes
well that's a short window, but when either restore_command or
primary_conninfo isn't configured correctly the window can get much
wider. That's because inbetween reading and unlinking the label we
restore the last checkpoint from WAL which can need additional WAL.
To fix simply also allow starting when the control file indicates
"shutdown in recovery". There's nicer fixes imaginable, but they'd be
more invasive.
Backpatch to 9.2 where support for taking basebackups from standbys
was added.
Noah Misch [Thu, 18 Dec 2014 03:48:40 +0000 (22:48 -0500)]
Lock down regression testing temporary clusters on Windows.
Use SSPI authentication to allow connections exclusively from the OS
user that launched the test suite. This closes on Windows the
vulnerability that commit
be76a6d39e2832d4b88c0e1cc381aa44a7f86881
closed on other platforms. Users of "make installcheck" or custom test
harnesses can run "pg_regress --config-auth=DATADIR" to activate the
same authentication configuration that "make check" would use.
Back-patch to 9.0 (all supported versions).
Security: CVE-2014-0067
Tom Lane [Wed, 17 Dec 2014 18:22:07 +0000 (13:22 -0500)]
Fix another poorly worded error message.
Spotted by Álvaro Herrera.
Magnus Hagander [Wed, 17 Dec 2014 10:55:22 +0000 (11:55 +0100)]
Update .gitignore for pg_upgrade
Add Windows versions of generated scripts, and make sure we only
ignore the scripts int he root directory.
Michael Paquier
Tom Lane [Tue, 16 Dec 2014 20:35:40 +0000 (15:35 -0500)]
Fix off-by-one loop count in MapArrayTypeName, and get rid of static array.
MapArrayTypeName would copy up to NAMEDATALEN-1 bytes of the base type
name, which of course is wrong: after prepending '_' there is only room for
NAMEDATALEN-2 bytes. Aside from being the wrong result, this case would
lead to overrunning the statically allocated work buffer. This would be a
security bug if the function were ever used outside bootstrap mode, but it
isn't, at least not in any currently supported branches.
Aside from fixing the off-by-one loop logic, this patch gets rid of the
static work buffer by having MapArrayTypeName pstrdup its result; the sole
caller was already doing that, so this just requires moving the pstrdup
call. This saves a few bytes but mainly it makes the API a lot cleaner.
Back-patch on the off chance that there is some third-party code using
MapArrayTypeName with less-secure input. Pushing pstrdup into the function
should not cause any serious problems for such hypothetical code; at worst
there might be a short term memory leak.
Per Coverity scanning.
Tom Lane [Tue, 16 Dec 2014 18:31:42 +0000 (13:31 -0500)]
Fix file descriptor leak after failure of a \setshell command in pgbench.
If the called command fails to return data, runShellCommand forgot to
pclose() the pipe before returning. This is fairly harmless in the current
code, because pgbench would then abandon further processing of that client
thread; so no more than nclients descriptors could be leaked this way. But
it's not hard to imagine future improvements whereby that wouldn't be true.
In any case, it's sloppy coding, so patch all branches. Found by Coverity.
Heikki Linnakangas [Tue, 16 Dec 2014 14:34:56 +0000 (16:34 +0200)]
Misc comment typo fixes.
Backpatch the applicable parts, just to make backpatching future patches
easier.
Tom Lane [Fri, 12 Dec 2014 17:41:55 +0000 (12:41 -0500)]
Revert misguided change to postgres_fdw FOR UPDATE/SHARE code.
In commit
462bd95705a0c23ba0b0ba60a78d32566a0384c1, I changed postgres_fdw
to rely on get_plan_rowmark() instead of get_parse_rowmark(). I still
think that's a good idea in the long run, but as Etsuro Fujita pointed out,
it doesn't work today because planner.c forces PlanRowMarks to have
markType = ROW_MARK_COPY for all foreign tables. There's no urgent reason
to change this in the back branches, so let's just revert that part of
yesterday's commit rather than trying to design a better solution under
time pressure.
Also, add a regression test case showing what postgres_fdw does with FOR
UPDATE/SHARE. I'd blithely assumed there was one already, else I'd have
realized yesterday that this code didn't work.
Tom Lane [Fri, 12 Dec 2014 02:02:31 +0000 (21:02 -0500)]
Fix planning of SELECT FOR UPDATE on child table with partial index.
Ordinarily we can omit checking of a WHERE condition that matches a partial
index's condition, when we are using an indexscan on that partial index.
However, in SELECT FOR UPDATE we must include the "redundant" filter
condition in the plan so that it gets checked properly in an EvalPlanQual
recheck. The planner got this mostly right, but improperly omitted the
filter condition if the index in question was on an inheritance child
table. In READ COMMITTED mode, this could result in incorrectly returning
just-updated rows that no longer satisfy the filter condition.
The cause of the error is using get_parse_rowmark() when get_plan_rowmark()
is what should be used during planning. In 9.3 and up, also fix the same
mistake in contrib/postgres_fdw. It's currently harmless there (for lack
of inheritance support) but wrong is wrong, and the incorrect code might
get copied to someplace where it's more significant.
Report and fix by Kyotaro Horiguchi. Back-patch to all supported branches.
Tom Lane [Fri, 12 Dec 2014 00:37:07 +0000 (19:37 -0500)]
Fix corner case where SELECT FOR UPDATE could return a row twice.
In READ COMMITTED mode, if a SELECT FOR UPDATE discovers it has to redo
WHERE-clause checking on rows that have been updated since the SELECT's
snapshot, it invokes EvalPlanQual processing to do that. If this first
occurs within a non-first child table of an inheritance tree, the previous
coding could accidentally re-return a matching row from an earlier,
already-scanned child table. (And, to add insult to injury, I think this
could make it miss returning a row that should have been returned, if the
updated row that this happens on should still have passed the WHERE qual.)
Per report from Kyotaro Horiguchi; the added isolation test is based on his
test case.
This has been broken for quite awhile, so back-patch to all supported
branches.
Tom Lane [Thu, 11 Dec 2014 20:41:23 +0000 (15:41 -0500)]
Fix assorted confusion between Oid and int32.
In passing, also make some debugging elog's in pgstat.c a bit more
consistently worded.
Back-patch as far as applicable (9.3 or 9.4; none of these mistakes are
really old).
Mark Dilger identified and patched the type violations; the message
rewordings are mine.
Heikki Linnakangas [Fri, 5 Dec 2014 12:27:56 +0000 (14:27 +0200)]
Give a proper error message if initdb password file is empty.
Used to say just "could not read password from file "...": Success", which
isn't very informative.
Mats Erik Andersson. Backpatch to all supported versions.
Tom Lane [Tue, 2 Dec 2014 20:02:43 +0000 (15:02 -0500)]
Fix JSON aggregates to work properly when final function is re-executed.
Davide S. reported that json_agg() sometimes produced multiple trailing
right brackets. This turns out to be because json_agg_finalfn() attaches
the final right bracket, and was doing so by modifying the aggregate state
in-place. That's verboten, though unfortunately it seems there's no way
for nodeAgg.c to check for such mistakes.
Fix that back to 9.3 where the broken code was introduced. In 9.4 and
HEAD, likewise fix json_object_agg(), which had copied the erroneous logic.
Make some cosmetic cleanups as well.
Tom Lane [Mon, 1 Dec 2014 20:25:08 +0000 (15:25 -0500)]
Guard against bad "dscale" values in numeric_recv().
We were not checking to see if the supplied dscale was valid for the given
digit array when receiving binary-format numeric values. While dscale can
validly be more than the number of nonzero fractional digits, it shouldn't
be less; that case causes fractional digits to be hidden on display even
though they're there and participate in arithmetic.
Bug #12053 from Tommaso Sala indicates that there's at least one broken
client library out there that sometimes supplies an incorrect dscale value,
leading to strange behavior. This suggests that simply throwing an error
might not be the best response; it would lead to failures in applications
that might seem to be working fine today. What seems the least risky fix
is to truncate away any digits that would be hidden by dscale. This
preserves the existing behavior in terms of what will be printed for the
transmitted value, while preventing subsequent arithmetic from producing
results inconsistent with that.
In passing, throw a specific error for the case of dscale being outside
the range that will fit into a numeric's header. Before you got "value
overflows numeric format", which is a bit misleading.
Back-patch to all supported branches.
Andrew Dunstan [Mon, 1 Dec 2014 17:48:35 +0000 (12:48 -0500)]
Fix backpatching error in commit
55c88079
Andrew Dunstan [Mon, 1 Dec 2014 16:28:45 +0000 (11:28 -0500)]
Fix hstore_to_json_loose's detection of valid JSON number values.
We expose a function IsValidJsonNumber that internally calls the lexer
for json numbers. That allows us to use the same test everywhere,
instead of inventing a broken test for hstore conversions. The new
function is also used in datum_to_json, replacing the code that is now
moved to the new function.
Backpatch to 9.3 where hstore_to_json_loose was introduced.
Magnus Hagander [Mon, 1 Dec 2014 11:12:07 +0000 (12:12 +0100)]
Fix missing space in documentation
Ian Barwick
Tom Lane [Sun, 30 Nov 2014 17:20:51 +0000 (12:20 -0500)]
Fix minor bugs in commit
30bf4689a96cd283af33edcdd6b7210df3f20cd8 et al.
Coverity complained that the "else" added to fillPGconn() was unreachable,
which it was. Remove the dead code. In passing, rearrange the tests so as
not to bother trying to fetch values for options that can't be assigned.
Pre-9.3 did not have that issue, but it did have a "return" that should be
"goto oom_error" to ensure that a suitable error message gets filled in.
Alvaro Herrera [Fri, 28 Nov 2014 21:06:18 +0000 (18:06 -0300)]
Update transaction README for persistent multixacts
Multixacts are now maintained during recovery, but the README didn't get
the memo. Backpatch to 9.3, where the divergence was introduced.
Fujii Masao [Thu, 27 Nov 2014 17:42:43 +0000 (02:42 +0900)]
Make \watch respect the user's \pset null setting.
Previously \watch always ignored the user's \pset null setting.
\pset null setting should be ignored for \d and similar queries.
For those, the code can reasonably have an opinion about what
the presentation should be like, since it knows what SQL query
it's issuing. This argument surely doesn't apply to \watch,
so this commit makes \watch use the user's \pset null setting.
Back-patch to 9.3 where \watch was added.
Fujii Masao [Thu, 27 Nov 2014 17:12:45 +0000 (02:12 +0900)]
Mark response messages for translation in pg_isready.
Back-patch to 9.3 where pg_isready was added.
Mats Erik Andersson
Tom Lane [Thu, 27 Nov 2014 16:12:51 +0000 (11:12 -0500)]
Free libxml2/libxslt resources in a safer order.
Mark Simonetti reported that libxslt sometimes crashes for him, and that
swapping xslt_process's object-freeing calls around to do them in reverse
order of creation seemed to fix it. I've not reproduced the crash, but
valgrind clearly shows a reference to already-freed memory, which is
consistent with the idea that shutdown of the xsltTransformContext is
trying to reference the already-freed stylesheet or input document.
With this patch, valgrind is no longer unhappy.
I have an inquiry in to see if this is a libxslt bug or if we're just
abusing the library; but even if it's a library bug, we'd want to adjust
our code so it doesn't fail with unpatched libraries.
Back-patch to all supported branches, because we've been doing this in
the wrong(?) order for a long time.
Heikki Linnakangas [Tue, 25 Nov 2014 15:12:07 +0000 (17:12 +0200)]
Allow "dbname" from connection string to be overridden in PQconnectDBParams
If the "dbname" attribute in PQconnectDBParams contained a connection string
or URI (and expand_dbname = TRUE), the database name from the connection
string could not be overridden by a subsequent "dbname" keyword in the
array. That was not intentional; all other options can be overridden.
Furthermore, any subsequent "dbname" caused the connection string from the
first dbname value to be processed again, overriding any values for the same
options that were given between the connection string and the second dbname
option.
In the passing, clarify in the docs that only the first dbname option in the
array is parsed as a connection string.
Alex Shulgin. Backpatch to all supported versions.
Heikki Linnakangas [Tue, 25 Nov 2014 10:55:00 +0000 (12:55 +0200)]
Check return value of strdup() in libpq connection option parsing.
An out-of-memory in most of these would lead to strange behavior, like
connecting to a different database than intended, but some would lead to
an outright segfault.
Alex Shulgin and me. Backpatch to all supported versions.
Tom Lane [Sat, 22 Nov 2014 21:01:12 +0000 (16:01 -0500)]
Fix mishandling of system columns in FDW queries.
postgres_fdw would send query conditions involving system columns to the
remote server, even though it makes no effort to ensure that system
columns other than CTID match what the remote side thinks. tableoid,
in particular, probably won't match and might have some use in queries.
Hence, prevent sending conditions that include non-CTID system columns.
Also, create_foreignscan_plan neglected to check local restriction
conditions while determining whether to set fsSystemCol for a foreign
scan plan node. This again would bollix the results for queries that
test a foreign table's tableoid.
Back-patch the first fix to 9.3 where postgres_fdw was introduced.
Back-patch the second to 9.2. The code is probably broken in 9.1 as
well, but the patch doesn't apply cleanly there; given the weak state
of support for FDWs in 9.1, it doesn't seem worth fixing.
Etsuro Fujita, reviewed by Ashutosh Bapat, and somewhat modified by me
Tom Lane [Wed, 19 Nov 2014 21:00:30 +0000 (16:00 -0500)]
Improve documentation's description of JOIN clauses.
In bug #12000, Andreas Kunert complained that the documentation was
misleading in saying "FROM T1 CROSS JOIN T2 is equivalent to FROM T1, T2".
That's correct as far as it goes, but the equivalence doesn't hold when
you consider three or more tables, since JOIN binds more tightly than
comma. I added a <note> to explain this, and ended up rearranging some
of the existing text so that the note would make sense in context.
In passing, rewrite the description of JOIN USING, which was unnecessarily
vague, and hadn't been helped any by somebody's reliance on markup as a
substitute for clear writing. (Mostly this involved reintroducing a
concrete example that was unaccountably removed by commit
032f3b7e166cfa28.)
Back-patch to all supported branches.
Robert Haas [Wed, 19 Nov 2014 16:57:54 +0000 (11:57 -0500)]
Avoid file descriptor leak in pg_test_fsync.
This can cause problems on Windows, where files that are still open
can't be unlinked.
Jeff Janes
Tom Lane [Wed, 19 Nov 2014 02:36:46 +0000 (21:36 -0500)]
Don't require bleeding-edge timezone data in timestamptz regression test.
The regression test cases added in commits
b2cbced9e et al depended in part
on the Russian timezone offset changes of Oct 2014. While this is of no
particular concern for a default Postgres build, it was possible for a
build using --with-system-tzdata to fail the tests if the system tzdata
database wasn't au courant. Bjorn Munch and Christoph Berg both complained
about this while packaging 9.4rc1, so we probably shouldn't insist on the
system tzdata being up-to-date. Instead, make an equivalent test using a
zone change that occurred in Venezuela in 2007. With this patch, the
regression tests should pass using any tzdata set from 2012 or later.
(I can't muster much sympathy for somebody using --with-system-tzdata
on a machine whose system tzdata is more than three years out-of-date.)
Tom Lane [Tue, 18 Nov 2014 18:28:13 +0000 (13:28 -0500)]
Fix some bogus direct uses of realloc().
pg_dump/parallel.c was using realloc() directly with no error check.
While the odds of an actual failure here seem pretty low, Coverity
complains about it, so fix by using pg_realloc() instead.
While looking for other instances, I noticed a couple of places in
psql that hadn't gotten the memo about the availability of pg_realloc.
These aren't bugs, since they did have error checks, but verbosely
inconsistent code is not a good thing.
Back-patch as far as 9.3. 9.2 did not have pg_dump/parallel.c, nor
did it have pg_realloc available in all frontend code.