Simon Riggs [Mon, 29 Aug 2016 11:18:57 +0000 (12:18 +0100)]
Fix pg_receivexlog --synchronous
Make pg_receivexlog work correctly with —-synchronous without slots
Backpatch to 9.5
Gabriele Bartolini, reviewed by Michael Paquier and Simon Riggs
Fujii Masao [Mon, 29 Aug 2016 05:34:58 +0000 (14:34 +0900)]
Fix pg_xlogdump so that it handles cross-page XLP_FIRST_IS_CONTRECORD record.
Previously pg_xlogdump failed to dump the contents of the WAL file
if the file starts with the continuation WAL record which spans
more than one pages. Since pg_xlogdump assumed that the continuation
record always fits on a page, it could not find the valid WAL record to
start reading from in that case.
This patch changes pg_xlogdump so that it can handle a continuation
WAL record which crosses a page boundary and find the valid record
to start reading from.
Back-patch to 9.3 where pg_xlogdump was introduced.
Author: Pavan Deolasee
Reviewed-By: Michael Paquier and Craig Ringer
Discussion: CABOikdPsPByMiG6J01DKq6om2+BNkxHTPkOyqHM2a4oYwGKsqQ@mail.gmail.com
Tom Lane [Fri, 26 Aug 2016 19:04:05 +0000 (15:04 -0400)]
Fix potential memory leakage from HandleParallelMessages().
HandleParallelMessages leaked memory into the caller's context. Since it's
called from ProcessInterrupts, there is basically zero certainty as to what
CurrentMemoryContext is, which means we could be leaking into long-lived
contexts. Over the processing of many worker messages that would grow to
be a problem. Things could be even worse than just a leak, if we happened
to service the interrupt while ErrorContext is current: elog.c thinks it
can reset that on its own whim, possibly yanking storage out from under
HandleParallelMessages.
Give HandleParallelMessages its own dedicated context instead, which we can
reset during each call to ensure there's no accumulation of wasted memory.
Discussion: <16610.
1472222135@sss.pgh.pa.us>
Tom Lane [Fri, 26 Aug 2016 18:15:47 +0000 (14:15 -0400)]
Fix assorted small bugs in ThrowErrorData().
Copy the palloc'd strings into the correct context, ie ErrorContext
not wherever the source ErrorData is. This would be a large bug,
except that it appears that all catchers of thrown errors do either
EmitErrorReport or CopyErrorData before doing anything that would
cause transient memory contexts to be cleaned up. Still, it's wrong
and it will bite somebody someday.
Fix failure to copy cursorpos and internalpos.
Utter the appropriate incantations involving recursion_depth, so that
we'll behave sanely if we get an error inside pstrdup. (In general,
the body of this function ought to act like, eg, errdetail().)
Per code reading induced by Jakob Egger's report.
Tom Lane [Thu, 25 Aug 2016 13:57:09 +0000 (09:57 -0400)]
Fix instability in parallel regression tests.
Commit
f0c7b789a added a test case in case.sql that creates and then drops
both an '=' operator and the type it's for. Given the right timing, that
can cause a "cache lookup failed for type" failure in concurrent sessions,
which see the '=' operator as a potential match for '=' in a query, but
then the type is gone by the time they inquire into its properties.
It might be nice to make that behavior more robust someday, but as a
back-patchable solution, adjust the new test case so that the operator
is never visible to other sessions. Like the previous commit, back-patch
to all supported branches.
Discussion: <5983.
1471371667@sss.pgh.pa.us>
Tom Lane [Thu, 25 Aug 2016 02:20:01 +0000 (22:20 -0400)]
Fix small query-lifespan memory leak in bulk updates.
When there is an identifiable REPLICA IDENTITY index on the target table,
heap_update leaks the id_attrs bitmapset. That's not many bytes, but it
adds up over enough rows, since the code typically runs in a query-lifespan
context. Bug introduced in commit
e55704d8b, which did a rather poor job
of cloning the existing use-pattern for RelationGetIndexAttrBitmap().
Per bug #14293 from Zhou Digoal. Back-patch to 9.4 where the bug was
introduced.
Report: <
20160824114320[email protected]>
Tom Lane [Wed, 24 Aug 2016 18:37:50 +0000 (14:37 -0400)]
Fix improper repetition of previous results from a hashed aggregate.
ExecReScanAgg's check for whether it could re-use a previously calculated
hashtable neglected the possibility that the Agg node might reference
PARAM_EXEC Params that are not referenced by its input plan node. That's
okay if the Params are in upper tlist or qual expressions; but if one
appears in aggregate input expressions, then the hashtable contents need
to be recomputed when the Param's value changes.
To avoid unnecessary performance degradation in the case of a Param that
isn't within an aggregate input, add logic to the planner to determine
which Params are within aggregate inputs. This requires a new field in
struct Agg, but fortunately we never write plans to disk, so this isn't
an initdb-forcing change.
Per report from Jeevan Chalke. This has been broken since forever,
so back-patch to all supported branches.
Andrew Gierth, with minor adjustments by me
Report:
Robert Haas [Mon, 22 Aug 2016 19:22:11 +0000 (15:22 -0400)]
Fix possible sorting error when aborting use of abbreviated keys.
Due to an error in the abbreviated key abort logic, the most recently
processed SortTuple could be incorrectly marked NULL, resulting in an
incorrect final sort order.
In the worst case, this could result in a corrupt btree index, which
would need to be rebuild using REINDEX. However, abbrevation doesn't
abort very often, not all data types use it, and only one tuple would
end up in the wrong place, so the practical impact of this mistake may
be somewhat limited.
Report and patch by Peter Geoghegan.
Alvaro Herrera [Fri, 19 Aug 2016 17:38:55 +0000 (14:38 -0300)]
reorderbuffer: preserve errno while reporting error
Clobbering errno during cleanup after an error is an oft-repeated, easy
to make mistake. Deal with it here as everywhere else, by saving it
aside and restoring after cleanup, before ereport'ing.
In passing, add a missing errcode declaration in another ereport() call
in the same file, which I noticed while skimming the file looking for
similar problems.
Backpatch to 9.4, where this code was introduced.
Tom Lane [Thu, 18 Aug 2016 20:04:35 +0000 (16:04 -0400)]
Update line count totals for psql help displays.
As usual, we've been pretty awful about maintaining these counts.
They're not all that critical, perhaps, but let's get them right
at release time. Also fix 9.5, which I notice is just as bad.
It's probably wrong further back, but the lack of --help=foo
options before 9.5 makes it too painful to count.
Tom Lane [Thu, 18 Aug 2016 18:48:51 +0000 (14:48 -0400)]
In plpgsql, don't try to convert int2vector or oidvector to expanded array.
These types are storage-compatible with real arrays, but they don't support
toasting, so of course they can't support expansion either.
Per bug #14289 from Michael Overmeyer. Back-patch to 9.5 where expanded
arrays were introduced.
Report: <
20160818174414[email protected]>
Magnus Hagander [Thu, 18 Aug 2016 10:32:42 +0000 (12:32 +0200)]
Update Windows timezone mapping from Windows 7 and 10
This adds a couple of new timezones that are present in the newer
versions of Windows. It also updates comments to reference UTC rather
than GMT, as this change has been made in Windows.
Michael Paquier
Andres Freund [Thu, 18 Aug 2016 00:03:36 +0000 (17:03 -0700)]
Fix deletion of speculatively inserted TOAST on conflict
INSERT .. ON CONFLICT runs a pre-check of the possible conflicting
constraints before performing the actual speculative insertion. In case
the inserted tuple included TOASTed columns the ON CONFLICT condition
would be handled correctly in case the conflict was caught by the
pre-check, but if two transactions entered the speculative insertion
phase at the same time, one would have to re-try, and the code for
aborting a speculative insertion did not handle deleting the
speculatively inserted TOAST datums correctly.
TOAST deletion would fail with "ERROR: attempted to delete invisible
tuple" as we attempted to remove the TOAST tuples using
simple_heap_delete which reasoned that the given tuples should not be
visible to the command that wrote them.
This commit updates the heap_abort_speculative() function which aborts
the conflicting tuple to use itself, via toast_delete, for deleting
associated TOAST datums. Like before, the inserted toast rows are not
marked as being speculative.
This commit also adds a isolationtester spec test, exercising the
relevant code path. Unfortunately 9.5 cannot handle two waiting
sessions, and thus cannot execute this test.
Reported-By: Viren Negi, Oskari Saarenmaa
Author: Oskari Saarenmaa, edited a bit by me
Bug: #14150
Discussion: <
20160519123338[email protected]>
Backpatch: 9.5, where ON CONFLICT was introduced
Andres Freund [Wed, 17 Aug 2016 20:15:03 +0000 (13:15 -0700)]
Properly re-initialize replication slot shared memory upon creation.
Slot creation did not clear all fields upon creation. After start the
memory is zeroed, but when a physical replication slot was created in
the shared memory of a previously existing logical slot, catalog_xmin
would not be cleared. That in turn would prevent vacuum from doing its
duties.
To fix initialize all the fields. To make similar future bugs less
likely, zero all of ReplicationSlotPersistentData, and re-order the
rest of the initialization to be in struct member order.
Analysis: Andrew Gierth
Reported-By: [email protected]
Author: Michael Paquier
Discussion: <
20160705173502[email protected]>
Backpatch: 9.4, where replication slots were introduced
Tom Lane [Wed, 17 Aug 2016 19:51:10 +0000 (15:51 -0400)]
Fix -e option in contrib/intarray/bench/bench.pl.
As implemented, -e ran an EXPLAIN but then discarded the output, which
certainly seems pointless. Make it print to stdout instead. It's been
like that forever, so back-patch to all supported branches.
Daniel Gustafsson, reviewed by Andreas Scherbaum
Patch: <
B97BDCB7-A3B3-4734-90B5-
EDD586941629@yesql.se>
Tom Lane [Tue, 16 Aug 2016 19:58:30 +0000 (15:58 -0400)]
Fix assorted places in psql to print version numbers >= 10 in new style.
This is somewhat cosmetic, since as long as you know what you are looking
at, "10.0" is a serviceable substitute for "10". But there is a potential
for confusion between version numbers with minor numbers and those without
--- we don't want people asking "why is psql saying 10.0 when my server is
10.2". Therefore, back-patch as far as practical, which turns out to be
9.3. I could have redone the patch to use fprintf(stderr) in place of
psql_error(), but it seems more work than is warranted for branches that
will be EOL or nearly so by the time v10 comes out.
Although only psql seems to contain any code that needs this, I chose
to put the support function into fe_utils, since it seems likely we'll
need it in other client programs in future. (In 9.3-9.5, use dumputils.c,
the predecessor of fe_utils/string_utils.c.)
In HEAD, also fix the backend code that whines about loadable-library
version mismatch. I don't see much need to back-patch that.
Tom Lane [Sun, 14 Aug 2016 19:06:01 +0000 (15:06 -0400)]
Remove bogus dependencies on NUMERIC_MAX_PRECISION.
NUMERIC_MAX_PRECISION is a purely arbitrary constraint on the precision
and scale you can write in a numeric typmod. It might once have had
something to do with the allowed range of a typmod-less numeric value,
but at least since 9.1 we've allowed, and documented that we allowed,
any value that would physically fit in the numeric storage format;
which is something over 100000 decimal digits, not 1000.
Hence, get rid of numeric_in()'s use of NUMERIC_MAX_PRECISION as a limit
on the allowed range of the exponent in scientific-format input. That was
especially silly in view of the fact that you can enter larger numbers as
long as you don't use 'e' to do it. Just constrain the value enough to
avoid localized overflow, and let make_result be the final arbiter of what
is too large. Likewise adjust ecpg's equivalent of this code.
Also get rid of numeric_recv()'s use of NUMERIC_MAX_PRECISION to limit the
number of base-NBASE digits it would accept. That created a dump/restore
hazard for binary COPY without doing anything useful; the wire-format
limit on number of digits (65535) is about as tight as we would want.
In HEAD, also get rid of pg_size_bytes()'s unnecessary intimacy with what
the numeric range limit is. That code doesn't exist in the back branches.
Per gripe from Aravind Kumar. Back-patch to all supported branches,
since they all contain the documentation claim about allowed range of
NUMERIC (cf commit
cabf5d84b).
Discussion: <2895.
1471195721@sss.pgh.pa.us>
Tom Lane [Fri, 12 Aug 2016 16:13:04 +0000 (12:13 -0400)]
Fix inappropriate printing of never-measured times in EXPLAIN.
EXPLAIN (ANALYZE, TIMING OFF) would print an elapsed time of zero for
a trigger function, because no measurement has been taken but it printed
the field anyway. This isn't what EXPLAIN does elsewhere, so suppress it.
In the same vein, EXPLAIN (ANALYZE, BUFFERS) with non-text output format
would print buffer I/O timing numbers even when no measurement has been
taken because track_io_timing is off. That seems not per policy, either,
so change it.
Back-patch to 9.2 where these features were introduced.
Maksim Milyutin
Discussion: <
081c0540-ecaa-bd29-3fd2-
6358f3b359a9@postgrespro.ru>
Simon Riggs [Fri, 12 Aug 2016 11:49:38 +0000 (12:49 +0100)]
Code cleanup in SyncRepWaitForLSN()
Commit
14e8803f1 removed LWLocks when accessing MyProc->syncRepState
but didn't clean up the surrounding code and comments.
Cleanup and backpatch to 9.5, to keep code similar.
Julien Rouhaud, improved by suggestion from Michael Paquier,
implemented trivially by myself.
Simon Riggs [Fri, 12 Aug 2016 09:36:20 +0000 (10:36 +0100)]
Correct TABLESAMPLE docs
Original wording was correct but not the intended meaning.
Reported by Patrik Wenger
Alvaro Herrera [Thu, 11 Aug 2016 19:09:24 +0000 (15:09 -0400)]
Add ID property to replication slots' sect2
Tom Lane [Thu, 11 Aug 2016 15:22:25 +0000 (11:22 -0400)]
Fix busted Assert for CREATE MATVIEW ... WITH NO DATA.
Commit
874fe3aea changed the command tag returned for CREATE MATVIEW/CREATE
TABLE AS ... WITH NO DATA, but missed that there was code in spi.c that
expected the command tag to always be "SELECT". Fortunately, the
consequence was only an Assert failure, so this oversight should have no
impact in production builds.
Since this code path was evidently un-exercised, add a regression test.
Per report from Shivam Saxena. Back-patch to 9.3, like the previous commit.
Michael Paquier
Report: <
97218716-480B-4527-B5CD-
D08D798A0C7B@dresources.com>
Tom Lane [Thu, 11 Aug 2016 01:39:50 +0000 (21:39 -0400)]
Doc: write some for adminpack.
Previous contents of adminpack.sgml were rather far short of project norms.
Not to mention being outright wrong about the signature of pg_file_read().
Peter Eisentraut [Tue, 9 Aug 2016 23:07:24 +0000 (19:07 -0400)]
Fix typo
Tom Lane [Tue, 9 Aug 2016 17:39:24 +0000 (13:39 -0400)]
Doc: clarify description of CREATE/ALTER FUNCTION ... SET FROM CURRENT.
Per discussion with David Johnston.
Tom Lane [Mon, 8 Aug 2016 20:27:53 +0000 (16:27 -0400)]
Stamp 9.5.4.
Tom Lane [Mon, 8 Aug 2016 15:56:10 +0000 (11:56 -0400)]
Last-minute updates for release notes.
Security: CVE-2016-5423, CVE-2016-5424
Peter Eisentraut [Mon, 8 Aug 2016 15:12:59 +0000 (11:12 -0400)]
Fix several one-byte buffer over-reads in to_number
Several places in NUM_numpart_from_char(), which is called from the SQL
function to_number(text, text), could accidentally read one byte past
the end of the input buffer (which comes from the input text datum and
is not null-terminated).
1. One leading space character would be skipped, but there was no check
that the input was at least one byte long. This does not happen in
practice, but for defensiveness, add a check anyway.
2. Commit
4a3a1e2cf apparently accidentally doubled that code that skips
one space character (so that two spaces might be skipped), but there
was no overflow check before skipping the second byte. Fix by
removing that duplicate code.
3. A logic error would allow a one-byte over-read when looking for a
trailing sign (S) placeholder.
In each case, the extra byte cannot be read out directly, but looking at
it might cause a crash.
The third item was discovered by Piotr Stefaniak, the first two were
found and analyzed by Tom Lane and Peter Eisentraut.
Peter Eisentraut [Mon, 8 Aug 2016 15:02:52 +0000 (11:02 -0400)]
Translation updates
Source-Git-URL: git://git.postgresql.org/git/pgtranslation/messages.git
Source-Git-Hash:
f1a1631efd7a51f9b1122f22cf688a3124bf1342
Tom Lane [Mon, 8 Aug 2016 14:33:46 +0000 (10:33 -0400)]
Fix two errors with nested CASE/WHEN constructs.
ExecEvalCase() tried to save a cycle or two by passing
&econtext->caseValue_isNull as the isNull argument to its sub-evaluation of
the CASE value expression. If that subexpression itself contained a CASE,
then *isNull was an alias for econtext->caseValue_isNull within the
recursive call of ExecEvalCase(), leading to confusion about whether the
inner call's caseValue was null or not. In the worst case this could lead
to a core dump due to dereferencing a null pointer. Fix by not assigning
to the global variable until control comes back from the subexpression.
Also, avoid using the passed-in isNull pointer transiently for evaluation
of WHEN expressions. (Either one of these changes would have been
sufficient to fix the known misbehavior, but it's clear now that each of
these choices was in itself dangerous coding practice and best avoided.
There do not seem to be any similar hazards elsewhere in execQual.c.)
Also, it was possible for inlining of a SQL function that implements the
equality operator used for a CASE comparison to result in one CASE
expression's CaseTestExpr node being inserted inside another CASE
expression. This would certainly result in wrong answers since the
improperly nested CaseTestExpr would be caused to return the inner CASE's
comparison value not the outer's. If the CASE values were of different
data types, a crash might result; moreover such situations could be abused
to allow disclosure of portions of server memory. To fix, teach
inline_function to check for "bare" CaseTestExpr nodes in the arguments of
a function to be inlined, and avoid inlining if there are any.
Heikki Linnakangas, Michael Paquier, Tom Lane
Report: https://github.com/greenplum-db/gpdb/pull/327
Report: <
4DDCEEB8[email protected]>
Security: CVE-2016-5423
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Obstruct shell, SQL, and conninfo injection via database and role names.
Due to simplistic quoting and confusion of database names with conninfo
strings, roles with the CREATEDB or CREATEROLE option could escalate to
superuser privileges when a superuser next ran certain maintenance
commands. The new coding rule for PQconnectdbParams() calls, documented
at conninfo_array_parse(), is to pass expand_dbname=true and wrap
literal database names in a trivial connection string. Escape
zero-length values in appendConnStrVal(). Back-patch to 9.1 (all
supported versions).
Nathan Bossart, Michael Paquier, and Noah Misch. Reviewed by Peter
Eisentraut. Reported by Nathan Bossart.
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Promote pg_dumpall shell/connstr quoting functions to src/fe_utils.
Rename these newly-extern functions with terms more typical of their new
neighbors. No functional changes; a subsequent commit will use them in
more places. Back-patch to 9.1 (all supported versions). Back branches
lack src/fe_utils, so instead rename the functions in place; the
subsequent commit will copy them into the other programs using them.
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Fix Windows shell argument quoting.
The incorrect quoting may have permitted arbitrary command execution.
At a minimum, it gave broader control over the command line to actors
supposed to have control over a single argument. Back-patch to 9.1 (all
supported versions).
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Reject, in pg_dumpall, names containing CR or LF.
These characters prematurely terminate Windows shell command processing,
causing the shell to execute a prefix of the intended command. The
chief alternative to rejecting these characters was to bypass the
Windows shell with CreateProcess(), but the ability to use such names
has little value. Back-patch to 9.1 (all supported versions).
This change formally revokes support for these characters in database
names and roles names. Don't document this; the error message is
self-explanatory, and too few users would benefit. A future major
release may forbid creation of databases and roles so named. For now,
check only at known weak points in pg_dumpall. Future commits will,
without notice, reject affected names from other frontend programs.
Also extend the restriction to pg_dumpall --dbname=CONNSTR arguments and
--file arguments. Unlike the effects on role name arguments and
database names, this does not reflect a broad policy change. A
migration to CreateProcess() could lift these two restrictions.
Reviewed by Peter Eisentraut.
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Field conninfo strings throughout src/bin/scripts.
These programs nominally accepted conninfo strings, but they would
proceed to use the original dbname parameter as though it were an
unadorned database name. This caused "reindexdb dbname=foo" to issue an
SQL command that always failed, and other programs printed a conninfo
string in error messages that purported to print a database name. Fix
both problems by using PQdb() to retrieve actual database names.
Continue to print the full conninfo string when reporting a connection
failure. It is informative there, and if the database name is the sole
problem, the server-side error message will include the name. Beyond
those user-visible fixes, this allows a subsequent commit to synthesize
and use conninfo strings without that implementation detail leaking into
messages. As a side effect, the "vacuuming database" message now
appears after, not before, the connection attempt. Back-patch to 9.1
(all supported versions).
Reviewed by Michael Paquier and Peter Eisentraut.
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Introduce a psql "\connect -reuse-previous=on|off" option.
The decision to reuse values of parameters from a previous connection
has been based on whether the new target is a conninfo string. Add this
means of overriding that default. This feature arose as one component
of a fix for security vulnerabilities in pg_dump, pg_dumpall, and
pg_upgrade, so back-patch to 9.1 (all supported versions). In 9.3 and
later, comment paragraphs that required update had already-incorrect
claims about behavior when no connection is open; fix those problems.
Security: CVE-2016-5424
Noah Misch [Mon, 8 Aug 2016 14:07:46 +0000 (10:07 -0400)]
Sort out paired double quotes in \connect, \password and \crosstabview.
In arguments, these meta-commands wrongly treated each pair as closing
the double quoted string. Make the behavior match the documentation.
This is a compatibility break, but I more expect to find software with
untested reliance on the documented behavior than software reliant on
today's behavior. Back-patch to 9.1 (all supported versions).
Reviewed by Tom Lane and Peter Eisentraut.
Security: CVE-2016-5424
Tom Lane [Mon, 8 Aug 2016 01:31:01 +0000 (21:31 -0400)]
Release notes for 9.5.4, 9.4.9, 9.3.14, 9.2.18, 9.1.23.
Tom Lane [Sun, 7 Aug 2016 22:52:02 +0000 (18:52 -0400)]
Fix misestimation of n_distinct for a nearly-unique column with many nulls.
If ANALYZE found no repeated non-null entries in its sample, it set the
column's stadistinct value to -1.0, intending to indicate that the entries
are all distinct. But what this value actually means is that the number
of distinct values is 100% of the table's rowcount, and thus it was
overestimating the number of distinct values by however many nulls there
are. This could lead to very poor selectivity estimates, as for example
in a recent report from Andreas Joseph Krogh. We should discount the
stadistinct value by whatever we've estimated the nulls fraction to be.
(That is what will happen if we choose to use a negative stadistinct for
a column that does have repeated entries, so this code path was just
inconsistent.)
In addition to fixing the stadistinct entries stored by several different
ANALYZE code paths, adjust the logic where get_variable_numdistinct()
forces an "all distinct" estimate on the basis of finding a relevant unique
index. Unique indexes don't reject nulls, so there's no reason to assume
that the null fraction doesn't apply.
Back-patch to all supported branches. Back-patching is a bit of a judgment
call, but this problem seems to affect only a few users (else we'd have
identified it long ago), and it's bad enough when it does happen that
destabilizing plan choices in a worse direction seems unlikely.
Patch by me, with documentation wording suggested by Dean Rasheed
Report:
df42f82acae38a58.156463942b8@tc7-visena>
Discussion: <16143.1470350371@sss.pgh.pa.us>
Tom Lane [Sun, 7 Aug 2016 17:15:55 +0000 (13:15 -0400)]
Don't propagate a null subtransaction snapshot up to parent transaction.
This oversight could cause logical decoding to fail to decode an outer
transaction containing changes, if a subtransaction had an XID but no
actual changes. Per bug #14279 from Marko Tiikkaja. Patch by Marko
based on analysis by Andrew Gierth.
Discussion: <
20160804191757[email protected]>
Tom Lane [Sat, 6 Aug 2016 18:28:37 +0000 (14:28 -0400)]
In B-tree page deletion, clean up properly after page deletion failure.
In _bt_unlink_halfdead_page(), we might fail to find an immediate left
sibling of the target page, perhaps because of corruption of the page
sibling links. The code intends to cope with this by just abandoning
the deletion attempt; but what actually happens is that it fails outright
due to releasing the same buffer lock twice. (And error recovery masks
a second problem, which is possible leakage of a pin on another page.)
Seems to have been introduced by careless refactoring in commit
efada2b8e.
Since there are multiple cases to consider, let's make releasing the buffer
lock in the failure case the responsibility of _bt_unlink_halfdead_page()
not its caller.
Also, avoid fetching the leaf page's left-link again after we've dropped
lock on the page. This is probably harmless, but it's not exactly good
coding practice.
Per report from Kyotaro Horiguchi. Back-patch to 9.4 where the faulty code
was introduced.
Discussion: <
20160803.173116.
111915228[email protected]>
Tom Lane [Fri, 5 Aug 2016 22:58:12 +0000 (18:58 -0400)]
Teach libpq to decode server version correctly from future servers.
Beginning with the next development cycle, PG servers will report two-part
not three-part version numbers. Fix libpq so that it will compute the
correct numeric representation of such server versions for reporting by
PQserverVersion(). It's desirable to get this into the field and
back-patched ASAP, so that older clients are more likely to understand the
new server version numbering by the time any such servers are in the wild.
(The results with an old client would probably not be catastrophic anyway
for a released server; for example "10.1" would be interpreted as 100100
which would be wrong in detail but would not likely cause an old client to
misbehave badly. But "10devel" or "10beta1" would result in sversion==0
which at best would result in disabling all use of modern features.)
Extracted from a patch by Peter Eisentraut; comments added by me
Patch: <
802ec140-635d-ad86-5fdf-
d3af0e260c22@2ndquadrant.com>
Fujii Masao [Fri, 5 Aug 2016 18:23:41 +0000 (03:23 +0900)]
Add note about unused arguments for pg_replication_origin_xact_reset() in docs.
In 9.5, two arguments were introduced into pg_replication_origin_xact_reset()
by mistake while they are actually not used at all. We cannot fix this issue
for 9.5 anymore because it needs a catalog version bump. Instead, we add
a note about those unused arguments into the document.
Reviewed-By: Andres Freund
Tom Lane [Fri, 5 Aug 2016 16:58:17 +0000 (12:58 -0400)]
Update time zone data files to tzdata release 2016f.
DST law changes in Kemerovo and Novosibirsk. Historical corrections for
Azerbaijan, Belarus, and Morocco. Asia/Novokuznetsk and Asia/Novosibirsk
now use numeric time zone abbreviations instead of invented ones. Zones
for Antarctic bases and other locations that have been uninhabited for
portions of the time span known to the tzdata database now report "-00"
rather than "zzz" as the zone abbreviation for those time spans.
Also, I decided to remove some of the timezone/data/ files that we don't
use. At one time that subdirectory was a complete copy of what IANA
distributes in the tzdata tarballs, but that hasn't been true for a long
time. There seems no good reason to keep shipping those specific files
but not others; they're just bloating our tarballs.
Tom Lane [Thu, 4 Aug 2016 20:06:14 +0000 (16:06 -0400)]
Fix bogus coding in WaitForBackgroundWorkerShutdown().
Some conditions resulted in "return" directly out of a PG_TRY block,
which left the exception stack dangling, and to add insult to injury
failed to restore the state of set_latch_on_sigusr1.
This is a bug only in 9.5; in HEAD it was accidentally fixed by commit
db0f6cad4, which removed the surrounding PG_TRY block. However, I (tgl)
chose to apply the patch to HEAD as well, because the old coding was
gratuitously different from WaitForBackgroundWorkerStartup(), and there
would indeed have been no bug if it were done like that to start with.
Dmitry Ivanov
Discussion: <
1637882.WfYN5gPf1A@abook>
Peter Eisentraut [Wed, 3 Aug 2016 17:45:55 +0000 (13:45 -0400)]
doc: Remove documentation of nonexistent information schema columns
These were probably copied in by accident.
From: Clément Prévost
Tom Lane [Tue, 2 Aug 2016 22:39:14 +0000 (18:39 -0400)]
Remove duplicate InitPostmasterChild() call while starting a bgworker.
This is apparently harmless on Windows, but on Unix it results in an
assertion failure. We'd not noticed because this code doesn't get
used on Unix unless you build with -DEXEC_BACKEND. Bug was evidently
introduced by sloppy refactoring in commit
31c453165.
Thomas Munro
Discussion:
Bruce Momjian [Tue, 2 Aug 2016 21:13:10 +0000 (17:13 -0400)]
doc: OS collation changes can break indexes
Discussion:
20160702155517[email protected]
Reviewed-by: Christoph Berg
Backpatch-through: 9.1
Tom Lane [Tue, 2 Aug 2016 20:39:16 +0000 (16:39 -0400)]
Block interrupts during HandleParallelMessages().
As noted by Alvaro, there are CHECK_FOR_INTERRUPTS() calls in the shm_mq.c
functions called by HandleParallelMessages(). I believe they're all
unreachable since we always pass nowait = true, but it doesn't seem like
a great idea to assume that no such call will ever be reachable from
HandleParallelMessages(). If that did happen, there would be a risk of a
recursive call to HandleParallelMessages(), which it does not appear to be
designed for --- for example, there's nothing that would prevent
out-of-order processing of received messages. And certainly such cases
cannot easily be tested. So let's prevent it by holding off interrupts for
the duration of the function. Back-patch to 9.5 which contains identical
code.
Discussion: <14869.
1470083848@sss.pgh.pa.us>
Tom Lane [Tue, 2 Aug 2016 20:09:09 +0000 (16:09 -0400)]
Sync 9.5 version of access/transam/parallel.c with HEAD.
This back-patches commit
a5fe473ad (notably, marking ParallelMessagePending
as volatile, which is not particularly optional). I also back-patched some
previous cosmetic changes to remove unnecessary diffs between the two
branches. I'm unsure how much of this code is actually reachable in 9.5,
but to the extent that it is reachable, it needs to be maintained, and
minimizing cross-branch diffs will make that easier.
Tom Lane [Tue, 2 Aug 2016 16:48:51 +0000 (12:48 -0400)]
Fix pg_dump's handling of public schema with both -c and -C options.
Since -c plus -C requests dropping and recreating the target database
as a whole, not dropping individual objects in it, we should assume that
the public schema already exists and need not be created. The previous
coding considered only the state of the -c option, so it would emit
"CREATE SCHEMA public" anyway, leading to an unexpected error in restore.
Back-patch to 9.2. Older versions did not accept -c with -C so the
issue doesn't arise there. (The logic being patched here dates to 8.0,
cf commit
2193121fa, so it's not really wrong that it didn't consider
the case at the time.)
Note that versions before 9.6 will still attempt to emit REVOKE/GRANT
on the public schema; but that happens without -c/-C too, and doesn't
seem to be the focus of this complaint. I considered extending this
stanza to also skip the public schema's ACL, but that would be a
misfeature, as it'd break cases where users intentionally changed that
ACL. The real fix for this aspect is Stephen Frost's work to not dump
built-in ACLs, and that's not going to get back-ported.
Per bugs #13804 and #14271. Solution found by David Johnston and later
rediscovered by me.
Report: <
20151207163520[email protected]>
Report: <
20160801021955[email protected]>
Peter Eisentraut [Tue, 2 Aug 2016 16:35:35 +0000 (12:35 -0400)]
doc: Whitespace fixes in man pages
Tom Lane [Mon, 1 Aug 2016 19:13:53 +0000 (15:13 -0400)]
Don't CHECK_FOR_INTERRUPTS between WaitLatch and ResetLatch.
This coding pattern creates a race condition, because if an interesting
interrupt happens after we've checked InterruptPending but before we reset
our latch, the latch-setting done by the signal handler would get lost,
and then we might block at WaitLatch in the next iteration without ever
noticing the interrupt condition. You can put the CHECK_FOR_INTERRUPTS
before WaitLatch or after ResetLatch, but not between them.
Aside from fixing the bugs, add some explanatory comments to latch.h
to perhaps forestall the next person from making the same mistake.
In HEAD, also replace gather_readnext's direct call of
HandleParallelMessages with CHECK_FOR_INTERRUPTS. It does not seem clean
or useful for this one caller to bypass ProcessInterrupts and go straight
to HandleParallelMessages; not least because that fails to consider the
InterruptPending flag, resulting in useless work both here
(if InterruptPending isn't set) and in the next CHECK_FOR_INTERRUPTS call
(if it is).
This thinko seems to have been introduced in the initial coding of
storage/ipc/shm_mq.c (commit
ec9037df2), and then blindly copied into all
the subsequent parallel-query support logic. Back-patch relevant hunks
to 9.4 to extirpate the error everywhere.
Discussion: <1661.
1469996911@sss.pgh.pa.us>
Michael Meskes [Mon, 1 Aug 2016 04:36:27 +0000 (06:36 +0200)]
Fixed array checking code for "unsigned long long" datatypes in libecpg.
Fujii Masao [Mon, 1 Aug 2016 08:36:14 +0000 (17:36 +0900)]
Fix pg_basebackup so that it accepts 0 as a valid compression level.
The help message for pg_basebackup specifies that the numbers 0 through 9
are accepted as valid values of -Z option. But, previously -Z 0 was rejected
as an invalid compression level.
Per discussion, it's better to make pg_basebackup treat 0 as valid
compression level meaning no compression, like pg_dump.
Back-patch to all supported versions.
Reported-By: Jeff Janes
Reviewed-By: Amit Kapila
Discussion: CAMkU=1x+GwjSayc57v6w87ij6iRGFWt=hVfM0B64b1_bPVKRqg@mail.gmail.com
Tom Lane [Sun, 31 Jul 2016 22:32:34 +0000 (18:32 -0400)]
Doc: remove claim that hash index creation depends on effective_cache_size.
This text was added by commit
ff213239c, and not long thereafter obsoleted
by commit
4adc2f72a (which made the test depend on NBuffers instead); but
nobody noticed the need for an update. Commit
9563d5b5e adds some further
dependency on maintenance_work_mem, but the existing verbiage seems to
cover that with about as much precision as we really want here. Let's
just take it all out rather than leaving ourselves open to more errors of
omission in future. (That solution makes this change back-patchable, too.)
Noted by Peter Geoghegan.
Discussion:
Bruce Momjian [Sat, 30 Jul 2016 20:59:34 +0000 (16:59 -0400)]
pgbench docs: fix incorrect "last two" fields text
Reported-by: Alexander Law
Discussion:
5786638C.
8080508@gmail.com
Backpatch-through: 9.4
Bruce Momjian [Sat, 30 Jul 2016 18:52:17 +0000 (14:52 -0400)]
doc: apply hypen fix that was not backpatched
Head patch was
42ec6c2da699e8e0b1774988fa97297a2cdf716c.
Reported-by: Alexander Law
Discussion:
5785FBE7.
7060508@gmail.com
Backpatch-through: 9.1
Tom Lane [Fri, 29 Jul 2016 16:52:57 +0000 (12:52 -0400)]
Fix pq_putmessage_noblock() to not block.
An evident copy-and-pasteo in commit
2bd9e412f broke the non-blocking
aspect of pq_putmessage_noblock(), causing it to behave identically to
pq_putmessage(). That function is nowadays used only in walsender.c,
so that the net effect was to cause walsenders to hang up waiting for
the receiver in situations where they should not.
Kyotaro Horiguchi
Patch: <
20160728.185228.
58375982[email protected]>
Tom Lane [Thu, 28 Jul 2016 22:57:24 +0000 (18:57 -0400)]
Guard against empty buffer in gets_fromFile()'s check for a newline.
Per the fgets() specification, it cannot return without reading some data
unless it reports EOF or error. So the code here assumed that the data
buffer would necessarily be nonempty when we go to check for a newline
having been read. However, Agostino Sarubbo noticed that this could fail
to be true if the first byte of the data is a NUL (\0). The fgets() API
doesn't really work for embedded NULs, which is something I don't feel
any great need for us to worry about since we generally don't allow NULs
in SQL strings anyway. But we should not access off the end of our own
buffer if the case occurs. Normally this would just be a harmless read,
but if you were unlucky the byte before the buffer would contain '\n'
and we'd overwrite it with '\0', and if you were really unlucky that
might be valuable data and psql would crash.
Agostino reported this to pgsql-security, but after discussion we concluded
that it isn't worth treating as a security bug; if you can control the
input to psql you can do far more interesting things than just maybe-crash
it. Nonetheless, it is a bug, so back-patch to all supported versions.
Tom Lane [Thu, 28 Jul 2016 20:09:15 +0000 (16:09 -0400)]
Fix assorted fallout from IS [NOT] NULL patch.
Commits
4452000f3 et al established semantics for NullTest.argisrow that
are a bit different from its initial conception: rather than being merely
a cache of whether we've determined the input to have composite type,
the flag now has the further meaning that we should apply field-by-field
testing as per the standard's definition of IS [NOT] NULL. If argisrow
is false and yet the input has composite type, the construct instead has
the semantics of IS [NOT] DISTINCT FROM NULL. Update the comments in
primnodes.h to clarify this, and fix ruleutils.c and deparse.c to print
such cases correctly. In the case of ruleutils.c, this merely results in
cosmetic changes in EXPLAIN output, since the case can't currently arise
in stored rules. However, it represents a live bug for deparse.c, which
would formerly have sent a remote query that had semantics different
from the local behavior. (From the user's standpoint, this means that
testing a remote nested-composite column for null-ness could have had
unexpected recursive behavior much like that fixed in
4452000f3.)
In a related but somewhat independent fix, make plancat.c set argisrow
to false in all NullTest expressions constructed to represent "attnotnull"
constructs. Since attnotnull is actually enforced as a simple null-value
check, this is a more accurate representation of the semantics; we were
previously overpromising what it meant for composite columns, which might
possibly lead to incorrect planner optimizations. (It seems that what the
SQL spec expects a NOT NULL constraint to mean is an IS NOT NULL test, so
arguably we are violating the spec and should fix attnotnull to do the
other thing. If we ever do, this part should get reverted.)
Back-patch, same as the previous commit.
Discussion: <10682.
1469566308@sss.pgh.pa.us>
Tom Lane [Thu, 28 Jul 2016 17:26:59 +0000 (13:26 -0400)]
Improve documentation about CREATE TABLE ... LIKE.
The docs failed to explain that LIKE INCLUDING INDEXES would not preserve
the names of indexes and associated constraints. Also, it wasn't mentioned
that EXCLUDE constraints would be copied by this option. The latter
oversight seems enough of a documentation bug to justify back-patching.
In passing, do some minor copy-editing in the same area, and add an entry
for LIKE under "Compatibility", since it's not exactly a faithful
implementation of the standard's feature.
Discussion: <
20160728151154.
AABE64016B@smtp.hushmail.com>
Tom Lane [Thu, 28 Jul 2016 15:39:10 +0000 (11:39 -0400)]
Register atexit hook only once in pg_upgrade.
start_postmaster() registered stop_postmaster_atexit as an atexit(3)
callback each time through, although the obvious intention was to do
so only once per program run. The extra registrations were harmless,
so long as we didn't exceed ATEXIT_MAX, but still it's a bug.
Artur Zakirov, with bikeshedding by Kyotaro Horiguchi and me
Discussion: <
d279e817-02b5-caa6-215f-
cfb05dce109a@postgrespro.ru>
Fujii Masao [Thu, 28 Jul 2016 13:34:42 +0000 (22:34 +0900)]
Fix incorrect description of udt_privileges view in documentation.
The description of udt_privileges view contained an incorrect copy-pasted word.
Back-patch to 9.2 where udt_privileges view was added.
Author: Alexander Law
Tom Lane [Tue, 26 Jul 2016 19:25:02 +0000 (15:25 -0400)]
Fix constant-folding of ROW(...) IS [NOT] NULL with composite fields.
The SQL standard appears to specify that IS [NOT] NULL's tests of field
nullness are non-recursive, ie, we shouldn't consider that a composite
field with value ROW(NULL,NULL) is null for this purpose.
ExecEvalNullTest got this right, but eval_const_expressions did not,
leading to weird inconsistencies depending on whether the expression
was such that the planner could apply constant folding.
Also, adjust the docs to mention that IS [NOT] DISTINCT FROM NULL can be
used as a substitute test if a simple null check is wanted for a rowtype
argument. That motivated reordering things so that IS [NOT] DISTINCT FROM
is described before IS [NOT] NULL. In HEAD, I went a bit further and added
a table showing all the comparison-related predicates.
Per bug #14235. Back-patch to all supported branches, since it's certainly
undesirable that constant-folding should change the semantics.
Report and patch by Andrew Gierth; assorted wordsmithing and revised
regression test cases by me.
Report: <
20160708024746[email protected]>
Noah Misch [Sun, 24 Jul 2016 00:30:03 +0000 (20:30 -0400)]
Make the AIX case of Makefile.shlib safe for parallel make.
Use our typical approach, from src/backend/parser. Back-patch to 9.1
(all supported versions).
Tom Lane [Fri, 22 Jul 2016 19:41:40 +0000 (15:41 -0400)]
Fix regression tests to work in Welsh locale.
Welsh (cy_GB) apparently sorts 'dd' after 'f', creating problems
analogous to the odd sorting of 'aa' in Danish. Adjust regression
test case to not use data that provokes that.
Jeff Janes
Patch:
Tom Lane [Thu, 21 Jul 2016 20:52:36 +0000 (16:52 -0400)]
Make contrib regression tests safe for Danish locale.
In btree_gin and citext, avoid some not-particularly-interesting
dependencies on the sorting of 'aa'. In tsearch2, use COLLATE "C" to
remove an uninteresting dependency on locale sort order (and thereby
allow removal of a variant expected-file).
Also, in citext, avoid assuming that lower('I') = 'i'. This isn't relevant
to Danish but it does fail in Turkish.
Tom Lane [Thu, 21 Jul 2016 18:24:07 +0000 (14:24 -0400)]
Make pltcl regression tests safe for Danish locale.
Another peculiarity of Danish locale is that it has an unusual idea
of how to sort upper vs. lower case. One of the pltcl test cases has
an issue with that. Now that COLLATE works in all supported branches,
we can just change the test to be locale-independent, and get rid of
the variant expected file that used to support non-C locales.
Tom Lane [Thu, 21 Jul 2016 17:11:00 +0000 (13:11 -0400)]
Make core regression tests safe for Danish locale.
Some tests added in 9.5 depended on 'aa' sorting before 'bb', which
doesn't hold true in Danish. Use slightly different test data to
avoid the problem.
Jeff Janes
Report:
Magnus Hagander [Wed, 20 Jul 2016 08:39:18 +0000 (10:39 +0200)]
Fix typos
Alexander Law
Tom Lane [Tue, 19 Jul 2016 22:41:30 +0000 (18:41 -0400)]
Remove very-obsolete estimates of shmem usage from postgresql.conf.sample.
runtime.sgml used to contain a table of estimated shared memory consumption
rates for max_connections and some other GUCs. Commit
390bfc643 removed
that on the well-founded grounds that (a) we weren't maintaining the
entries well and (b) it no longer mattered so much once we got out from
under SysV shmem limits. But it missed that there were even-more-obsolete
versions of some of those numbers in comments in postgresql.conf.sample.
Remove those too. Back-patch to 9.3 where the aforesaid commit went in.
Tom Lane [Tue, 19 Jul 2016 21:53:31 +0000 (17:53 -0400)]
Fix MSVC build for changes in zic.
Ooops, I missed back-patching commit
f5f15ea6a along with the other stuff.
Tom Lane [Tue, 19 Jul 2016 19:59:36 +0000 (15:59 -0400)]
Sync back-branch copies of the timezone code with IANA release tzcode2016c.
Back-patch commit
1c1a7cbd6a1600c9, along with subsequent portability
fixes, into all active branches. Also, back-patch commits
696027727 and
596857043 (addition of zic -P option) into 9.1 and 9.2, just to reduce
differences between the branches. src/timezone/ is now largely identical
in all active branches, except that in 9.1, pgtz.c retains the
initial-timezone-selection code that was moved over to initdb in 9.2.
Ordinarily we wouldn't risk this much code churn in back branches, but it
seems necessary in this case, because among the changes are two feature
additions in the "zic" zone data file compiler (a larger limit on the
number of allowed DST transitions, and addition of a "%z" escape in zone
abbreviations). IANA have not yet started to use those features in their
tzdata files, but presumably they will before too long. If we don't update
then we'll be unable to adopt new timezone data. Also, installations built
with --with-system-tzdata (which includes most distro-supplied builds, I
believe) might fail even if we don't update our copies of the data files.
There are assorted bug fixes too, mostly affecting obscure timezones or
post-2037 dates.
Discussion: <13601.
1468868947@sss.pgh.pa.us>
Tom Lane [Mon, 18 Jul 2016 20:52:06 +0000 (16:52 -0400)]
Doc: improve discussion of plpgsql's GET DIAGNOSTICS, other minor fixes.
9.4 added a second description of GET DIAGNOSTICS that was totally
independent of the existing one, resulting in each description lying to the
extent that it claimed the set of status items it described was complete.
Fix that, and do some minor markup improvement.
Also some other small fixes per bug #14258 from Dilian Palauzov.
Discussion: <
20160718181437[email protected]>
Peter Eisentraut [Sun, 17 Jul 2016 13:15:37 +0000 (09:15 -0400)]
Use correct symbol for minimum int64 value
The old code used SEQ_MINVALUE to get the smallest int64 value. This
was done as a convenience to avoid having to deal with INT64_IS_BUSTED,
but that is obsolete now. Also, it is incorrect because the smallest
int64 value is actually SEQ_MINVALUE-1. Fix by using PG_INT64_MIN.
Tom Lane [Sat, 16 Jul 2016 18:42:37 +0000 (14:42 -0400)]
Fix crash in close_ps() for NaN input coordinates.
The Assert() here seems unreasonably optimistic. Andreas Seltenreich
found that it could fail with NaNs in the input geometries, and it
seems likely to me that it might fail in corner cases due to roundoff
error, even for ordinary input values. As a band-aid, make the function
return SQL NULL instead of crashing.
Report: <
[email protected]>
Andres Freund [Sat, 16 Jul 2016 00:49:48 +0000 (17:49 -0700)]
Fix torn-page, unlogged xid and further risks from heap_update().
When heap_update needs to look for a page for the new tuple version,
because the current one doesn't have sufficient free space, or when
columns have to be processed by the tuple toaster, it has to release the
lock on the old page during that. Otherwise there'd be lock ordering and
lock nesting issues.
To avoid concurrent sessions from trying to update / delete / lock the
tuple while the page's content lock is released, the tuple's xmax is set
to the current session's xid.
That unfortunately was done without any WAL logging, thereby violating
the rule that no XIDs may appear on disk, without an according WAL
record. If the database were to crash / fail over when the page level
lock is released, and some activity lead to the page being written out
to disk, the xid could end up being reused; potentially leading to the
row becoming invisible.
There might be additional risks by not having t_ctid point at the tuple
itself, without having set the appropriate lock infomask fields.
To fix, compute the appropriate xmax/infomask combination for locking
the tuple, and perform WAL logging using the existing XLOG_HEAP_LOCK
record. That allows the fix to be backpatched.
This issue has existed for a long time. There appears to have been
partial attempts at preventing dangers, but these never have fully been
implemented, and were removed a long time ago, in
11919160 (cf. HEAP_XMAX_UNLOGGED).
In master / 9.6, there's an additional issue, namely that the
visibilitymap's freeze bit isn't reset at that point yet. Since that's a
new issue, introduced only in
a892234f830, that'll be fixed in a
separate commit.
Author: Masahiko Sawada and Andres Freund
Reported-By: Different aspects by Thomas Munro, Noah Misch, and others
Discussion: CAEepm=3fWAbWryVW9swHyLTY4sXVf0xbLvXqOwUoDiNCx9mBjQ@mail.gmail.com
Backpatch: 9.1/all supported versions
Andres Freund [Fri, 15 Jul 2016 21:37:06 +0000 (14:37 -0700)]
Make HEAP_LOCK/HEAP2_LOCK_UPDATED replay reset HEAP_XMAX_INVALID.
0ac5ad5 started to compress infomask bits in WAL records. Unfortunately
the replay routines for XLOG_HEAP_LOCK/XLOG_HEAP2_LOCK_UPDATED forgot to
reset the HEAP_XMAX_INVALID (and some other) hint bits.
Luckily that's not problematic in the majority of cases, because after a
crash/on a standby row locks aren't meaningful. Unfortunately that does
not hold true in the presence of prepared transactions. This means that
after a crash, or after promotion, row level locks held by a prepared,
but not yet committed, prepared transaction might not be enforced.
Discussion:
20160715192319[email protected]
Backpatch: 9.3, the oldest branch on which
0ac5ad5 is present.
Alvaro Herrera [Fri, 15 Jul 2016 18:17:20 +0000 (14:17 -0400)]
Avoid serializability errors when locking a tuple with a committed update
When key-share locking a tuple that has been not-key-updated, and the
update is a committed transaction, in some cases we raised
serializability errors:
ERROR: could not serialize access due to concurrent update
Because the key-share doesn't conflict with the update, the error is
unnecessary and inconsistent with the case that the update hasn't
committed yet. This causes problems for some usage patterns, even if it
can be claimed that it's sufficient to retry the aborted transaction:
given a steady stream of updating transactions and a long locking
transaction, the long transaction can be starved indefinitely despite
multiple retries.
To fix, we recognize that HeapTupleSatisfiesUpdate can return
HeapTupleUpdated when an updating transaction has committed, and that we
need to deal with that case exactly as if it were a non-committed
update: verify whether the two operations conflict, and if not, carry on
normally. If they do conflict, however, there is a difference: in the
HeapTupleBeingUpdated case we can just sleep until the concurrent
transaction is gone, while in the HeapTupleUpdated case this is not
possible and we must raise an error instead.
Per trouble report from Olivier Dony.
In addition to a couple of test cases that verify the changed behavior,
I added a test case to verify the behavior that remains unchanged,
namely that errors are raised when a update that modifies the key is
used. That must still generate serializability errors. One
pre-existing test case changes behavior; per discussion, the new
behavior is actually the desired one.
Discussion: https://www.postgresql.org/message-id/
560AA479.
4080807@odoo.com
https://www.postgresql.org/message-id/
20151014164844[email protected]
Backpatch to 9.3, where the problem appeared.
Peter Eisentraut [Fri, 15 Jul 2016 02:28:41 +0000 (22:28 -0400)]
doc: Fix typos
From: Alexander Law
Tom Lane [Thu, 14 Jul 2016 22:46:00 +0000 (18:46 -0400)]
Fix GiST index build for NaN values in geometric types.
GiST index build could go into an infinite loop when presented with boxes
(or points, circles or polygons) containing NaN component values. This
happened essentially because the code assumed that x == x is true for any
"double" value x; but it's not true for NaNs. The looping behavior was not
the only problem though: we also attempted to sort the items using simple
double comparisons. Since NaNs violate the trichotomy law, qsort could
(in principle at least) get arbitrarily confused and mess up the sorting of
ordinary values as well as NaNs. And we based splitting choices on box size
calculations that could produce NaNs, again resulting in undesirable
behavior.
To fix, replace all comparisons of doubles in this logic with
float8_cmp_internal, which is NaN-aware and is careful to sort NaNs
consistently, higher than any non-NaN. Also rearrange the box size
calculation to not produce NaNs; instead it should produce an infinity
for a box with NaN on one side and not-NaN on the other.
I don't by any means claim that this solves all problems with NaNs in
geometric values, but it should at least make GiST index insertion work
reliably with such data. It's likely that the index search side of things
still needs some work, and probably regular geometric operations too.
But with this patch we're laying down a convention for how such cases
ought to behave.
Per bug #14238 from Guang-Dih Lei. Back-patch to 9.2; the code used before
commit
7f3bd86843e5aad8 is quite different and doesn't lock up on my simple
test case, nor on the submitter's dataset.
Report: <
20160708151747[email protected]>
Discussion: <28685.
1468246504@sss.pgh.pa.us>
Tom Lane [Wed, 13 Jul 2016 15:17:15 +0000 (11:17 -0400)]
Fix obsolete header-file reference in pg_buffercache docs.
Commit
2d0019049 moved enum ForkNumber from relfilenode.h into relpath.h,
but missed updating this documentation reference.
Alexander Law
Tom Lane [Tue, 12 Jul 2016 22:06:50 +0000 (18:06 -0400)]
Allow IMPORT FOREIGN SCHEMA within pl/pgsql.
Since IMPORT FOREIGN SCHEMA has an INTO clause, pl/pgsql needs to be
aware of that and avoid capturing the INTO as an INTO-variables clause.
This isn't hard, though it's annoying to have to make IMPORT a plpgsql
keyword just for this. (Fortunately, we have the infrastructure now
to make it an unreserved keyword, so at least this shouldn't break any
existing pl/pgsql code.)
Per report from Merlin Moncure. Back-patch to 9.5 where IMPORT FOREIGN
SCHEMA was introduced.
Report:
Peter Eisentraut [Mon, 11 Jul 2016 16:12:04 +0000 (12:12 -0400)]
doc: Update URL for PL/PHP
Magnus Hagander [Mon, 11 Jul 2016 11:53:17 +0000 (13:53 +0200)]
Add missing newline in error message
Tom Lane [Sat, 9 Jul 2016 20:47:39 +0000 (16:47 -0400)]
Fix TAP tests and MSVC scripts for pathnames with spaces.
Change assorted places in our Perl code that did things like
system("prog $path/file");
to do it more like
system('prog', "$path/file");
which is safe against spaces and other special characters in the path
variable. The latter was already the prevailing style, but a few bits
of code hadn't gotten this memo. Back-patch to 9.4 as relevant.
Michael Paquier, Kyotaro Horiguchi
Discussion: <
20160704.160213.
111134711[email protected]>
Tom Lane [Fri, 8 Jul 2016 16:46:04 +0000 (12:46 -0400)]
Docs: improve examples about not repeating table name in UPDATE ... SET.
Alexander Law
Tom Lane [Mon, 4 Jul 2016 20:09:11 +0000 (16:09 -0400)]
Fix failure to handle conflicts in non-arbiter exclusion constraints.
ExecInsertIndexTuples treated an exclusion constraint as subject to
noDupErr processing even when it was not listed in arbiterIndexes, and
would therefore not error out for a conflict in such a constraint, instead
returning it as an arbiter-index failure. That led to an infinite loop in
ExecInsert, since ExecCheckIndexConstraints ignored the index as-intended
and therefore didn't throw the expected error. To fix, make the exclusion
constraint code path use the same condition as the index_insert call does
to decide whether no-error-for-duplicates behavior is appropriate. While
at it, refactor a little bit to avoid unnecessary list_member_oid calls.
(That surely wouldn't save anything worth noticing, but I find the code
a bit clearer this way.)
Per bug report from Heikki Rauhala. Back-patch to 9.5 where ON CONFLICT
was introduced.
Report: <
4C976D6B-76B4-434C-8052-
D009F7B7AEDA@reaktor.fi>
Bruce Momjian [Sat, 2 Jul 2016 15:22:36 +0000 (11:22 -0400)]
doc: mention dependency on collation libraries
Document that index storage is dependent on the operating system's
collation library ordering, and any change in that ordering can create
invalid indexes.
Discussion:
20160617154311[email protected]
Backpatch-through: 9.1
Tom Lane [Fri, 1 Jul 2016 15:40:22 +0000 (11:40 -0400)]
Be more paranoid in ruleutils.c's get_variable().
We were merely Assert'ing that the Var matched the RTE it's supposedly
from. But if the user passes incorrect information to pg_get_expr(),
the RTE might in fact not match; this led either to Assert failures
or core dumps, as reported by Chris Hanks in bug #14220. To fix, just
convert the Asserts to test-and-elog. Adjust an existing test-and-elog
elsewhere in the same function to be consistent in wording.
(If we really felt these were user-facing errors, we might promote them to
ereport's; but I can't convince myself that they're worth translating.)
Back-patch to 9.3; the problematic code doesn't exist before that, and
a quick check says that 9.2 doesn't crash on such cases.
Michael Paquier and Thomas Munro
Report: <
20160629224349[email protected]>
Robert Haas [Fri, 1 Jul 2016 12:51:58 +0000 (08:51 -0400)]
Fix crash bug in RestoreSnapshot.
If serialized_snapshot->subxcnt > 0 and serialized_snapshot->xcnt == 0,
the old coding would do the wrong thing and crash. This can happen
on standby servers.
Report by Andreas Seltenreich. Patch by Thomas Munro, reviewed by
Amit Kapila and tested by Andreas Seltenreich.
Tom Lane [Thu, 30 Jun 2016 16:37:02 +0000 (12:37 -0400)]
Fix typo in ReorderBufferIterTXNInit().
This looks like it would cause changes from subtransactions to be missed
by the iterator being constructed, if those changes had been spilled to
disk previously. This implies that large subtransactions might be lost
(in whole or in part) by logical replication. Found and fixed by
Petru-Florin Mihancea, per bug #14208.
Report: <
20160622144830[email protected]>
Tom Lane [Mon, 27 Jun 2016 19:57:21 +0000 (15:57 -0400)]
Fix CREATE MATVIEW/CREATE TABLE AS ... WITH NO DATA to not plan the query.
Previously, these commands always planned the given query and went through
executor startup before deciding not to actually run the query if WITH NO
DATA is specified. This behavior is problematic for pg_dump because it
may cause errors to be raised that we would rather not see before a
REFRESH MATERIALIZED VIEW command is issued. See for example bug #13907
from Marian Krucina. This change is not sufficient to fix that particular
bug, because we also need to tweak pg_dump to issue the REFRESH later,
but it's a necessary step on the way.
A user-visible side effect of doing things this way is that the returned
command tag for WITH NO DATA cases will now be "CREATE MATERIALIZED VIEW"
or "CREATE TABLE AS", not "SELECT 0". We could preserve the old behavior
but it would take more code, and arguably that was just an implementation
artifact not intended behavior anyhow.
In 9.5 and HEAD, also get rid of the static variable CreateAsReladdr, which
was trouble waiting to happen; there is not any prohibition on nested
CREATE commands.
Back-patch to 9.3 where CREATE MATERIALIZED VIEW was introduced.
Michael Paquier and Tom Lane
Report: <
20160202161407[email protected]>
Alvaro Herrera [Fri, 24 Jun 2016 22:29:28 +0000 (18:29 -0400)]
Fix handling of multixacts predating pg_upgrade
After pg_upgrade, it is possible that some tuples' Xmax have multixacts
corresponding to the old installation; such multixacts cannot have
running members anymore. In many code sites we already know not to read
them and clobber them silently, but at least when VACUUM tries to freeze
a multixact or determine whether one needs freezing, there's an attempt
to resolve it to its member transactions by calling GetMultiXactIdMembers,
and if the multixact value is "in the future" with regards to the
current valid multixact range, an error like this is raised:
ERROR: MultiXactId 123 has not been created yet -- apparent wraparound
and vacuuming fails. Per discussion with Andrew Gierth, it is completely
bogus to try to resolve multixacts coming from before a pg_upgrade,
regardless of where they stand with regards to the current valid
multixact range.
It's possible to get from under this problem by doing SELECT FOR UPDATE
of the problem tuples, but if tables are large, this is slow and
tedious, so a more thorough solution is desirable.
To fix, we realize that multixacts in xmax created in 9.2 and previous
have a specific bit pattern that is never used in 9.3 and later (we
already knew this, per comments and infomask tests sprinkled in various
places, but we weren't leveraging this knowledge appropriately).
Whenever the infomask of the tuple matches that bit pattern, we just
ignore the multixact completely as if Xmax wasn't set; or, in the case
of tuple freezing, we act as if an unwanted value is set and clobber it
without decoding. This guarantees that no errors will be raised, and
that the values will be progressively removed until all tables are
clean. Most callers of GetMultiXactIdMembers are patched to recognize
directly that the value is a removable "empty" multixact and avoid
calling GetMultiXactIdMembers altogether.
To avoid changing the signature of GetMultiXactIdMembers() in back
branches, we keep the "allow_old" boolean flag but rename it to
"from_pgupgrade"; if the flag is true, we always return an empty set
instead of looking up the multixact. (I suppose we could remove the
argument in the master branch, but I chose not to do so in this commit).
This was broken all along, but the error-facing message appeared first
because of commit
8e9a16ab8f7f and was partially fixed in
a25c2b7c4db3.
This fix, backpatched all the way back to 9.3, goes approximately in the
same direction as
a25c2b7c4db3 but should cover all cases.
Bug analysis by Andrew Gierth and Álvaro Herrera.
A number of public reports match this bug:
https://www.postgresql.org/message-id/
20140330040029[email protected]
https://www.postgresql.org/message-id/
538F3D70.
6080902@publicrelay.com
https://www.postgresql.org/message-id/
556439CF.
7070109@pscs.co.uk
https://www.postgresql.org/message-id/SG2PR06MB0760098A111C88E31BD4D96FB3540@SG2PR06MB0760.apcprd06.prod.outlook.com
https://www.postgresql.org/message-id/
20160615203829[email protected]
Tom Lane [Fri, 24 Jun 2016 20:57:36 +0000 (16:57 -0400)]
Fix building of large (bigger than shared_buffers) hash indexes.
When the index is predicted to need more than NBuffers buckets,
CREATE INDEX attempts to sort the index entries by hash key before
insertion, so as to reduce thrashing. This code path got broken by
commit
9f03ca915196dfc8, which overlooked that _hash_form_tuple() is not
just an alias for index_form_tuple(). The index got built anyway, but
with garbage data, so that searches for pre-existing tuples always
failed. Fix by refactoring to separate construction of the indexable
data from calling index_form_tuple().
Per bug #14210 from Daniel Newman. Back-patch to 9.5 where the
bug was introduced.
Report: <
20160623162507[email protected]>
Andrew Dunstan [Thu, 23 Jun 2016 20:10:15 +0000 (16:10 -0400)]
Add tab completion for pager_min_lines to psql.
This was inadvertantly omitted from commit
7655f4ccea570d57c4d473cd66b755c03c904942. Mea culpa.
Backpatched to 9.5 where pager_min_lines was introduced.
Tom Lane [Wed, 22 Jun 2016 15:55:18 +0000 (11:55 -0400)]
Make "postgres -C guc" print "" not "(null)" for null-valued GUCs.
Commit
0b0baf262 et al made this case print "(null)" on the grounds that
that's what happened on platforms that didn't crash. But neither behavior
was actually intentional. What we should print is just an empty string,
for compatibility with the behavior of SHOW and other ways of examining
string GUCs. Those code paths don't distinguish NULL from empty strings,
so we should not here either. Per gripe from Alain Radix.
Like the previous patch, back-patch to 9.2 where -C option was introduced.
Discussion:
Tom Lane [Wed, 22 Jun 2016 00:07:58 +0000 (20:07 -0400)]
Document that dependency tracking doesn't consider function bodies.
If there's anyplace in our SGML docs that explains this behavior, I can't
find it right at the moment. Add an explanation in "Dependency Tracking"
which seems like the authoritative place for such a discussion. Per
gripe from Michelle Schwan.
While at it, update this section's example of a dependency-related
error message: they last looked like that in 8.3. And remove the
explanation of dependency updates from pre-7.3 installations, which
is probably no longer worth anybody's brain cells to read.
The bogus error message example seems like an actual documentation bug,
so back-patch to all supported branches.
Discussion: <
20160620160047[email protected]>
Tom Lane [Mon, 20 Jun 2016 19:36:54 +0000 (15:36 -0400)]
Add missing check for malloc failure in plpgsql_extra_checks_check_hook().
Per report from Andreas Seltenreich. Back-patch to affected versions.
Report: <
[email protected]>