]> granicus.if.org Git - postgresql/log
postgresql
8 years agoFix possible crash due to incorrect allocation context.
Robert Haas [Tue, 16 Aug 2016 17:23:32 +0000 (13:23 -0400)]
Fix possible crash due to incorrect allocation context.

Commit af33039317ddc4a0e38a02e2255c2bf453115fd2 aimed to reduce
leakage from tqueue.c, which is good.  Unfortunately, by changing the
memory context in which all of gather_readnext() executes, it also
changed the context in which ExecShutdownGatherWorkers executes, which
is not good, because that function eventually causes a call to
ExecParallelRetrieveInstrumentation, which proceeds to allocate
planstate->worker_instrument in a short-lived context, causing a
crash.

Rushabh Lathia, reviewed by Amit Kapila and by me.

8 years agoRemove separate version numbering for ecpg preprocessor.
Tom Lane [Tue, 16 Aug 2016 16:49:30 +0000 (12:49 -0400)]
Remove separate version numbering for ecpg preprocessor.

Once upon a time, it made sense for the ecpg preprocessor to have its
own version number, because it used a manually-maintained grammar that
wasn't always in sync with the core grammar.  But those days are
thankfully long gone, leaving only a maintenance nuisance behind.
Let's use the PG v10 version numbering changeover as an excuse to get
rid of the ecpg version number and just have ecpg identify itself by
PG_VERSION.  From the user's standpoint, ecpg will go from "4.12" in
the 9.6 branch to "10" in the 10 branch, so there's no failure of
monotonicity.

Discussion: <1471332659.4410.67.camel@postgresql.org>

8 years agoDoc: copy-editing in create_access_method.sgml.
Tom Lane [Tue, 16 Aug 2016 15:35:36 +0000 (11:35 -0400)]
Doc: copy-editing in create_access_method.sgml.

Improve shaky English grammar.  And markup.

8 years agoDoc: remove out-of-date claim that pg_am rows must be inserted by hand.
Tom Lane [Tue, 16 Aug 2016 14:59:14 +0000 (10:59 -0400)]
Doc: remove out-of-date claim that pg_am rows must be inserted by hand.

Commit 473b93287 added a sentence about that, but neglected to remove
the adjacent sentence it had falsified.  Per Alexander Law.

8 years agoOnce again allow LWLocks to be used within DSM segments.
Robert Haas [Mon, 15 Aug 2016 22:09:55 +0000 (18:09 -0400)]
Once again allow LWLocks to be used within DSM segments.

Prior to commit 7882c3b0b95640e361f1533fe0f2d02e4e5d8610, it was
possible to use LWLocks within DSM segments, but that commit broke
this use case by switching from a doubly linked list to a circular
linked list.  Switch back, using a new bit of general infrastructure
for maintaining lists of PGPROCs.

Thomas Munro, reviewed by me.

8 years agoAllow .so minor version numbers above 9 in .gitignore.
Tom Lane [Mon, 15 Aug 2016 21:35:35 +0000 (17:35 -0400)]
Allow .so minor version numbers above 9 in .gitignore.

Needed now that libpq.so's minor version has reached 10.

8 years agoUpdate git_changelog to know that there's a 9.6 branch.
Tom Lane [Mon, 15 Aug 2016 19:24:54 +0000 (15:24 -0400)]
Update git_changelog to know that there's a 9.6 branch.

Missed this in the main 10devel version stamping patch.

8 years agoStamp shared-library minor version numbers for v10.
Tom Lane [Mon, 15 Aug 2016 18:35:55 +0000 (14:35 -0400)]
Stamp shared-library minor version numbers for v10.

8 years agoStamp HEAD as 10devel.
Tom Lane [Mon, 15 Aug 2016 17:49:49 +0000 (13:49 -0400)]
Stamp HEAD as 10devel.

This is a good bit more complicated than the average new-version stamping
commit, because it includes various adjustments in pursuit of changing
from three-part to two-part version numbers.  It's likely some further
work will be needed around that change; but this is enough to get through
the regression tests, at least in Unix builds.

Peter Eisentraut and Tom Lane

8 years agoFinal pgindent + perltidy run for 9.6.
Tom Lane [Mon, 15 Aug 2016 17:42:51 +0000 (13:42 -0400)]
Final pgindent + perltidy run for 9.6.

8 years agoSimplify the process of perltidy'ing our Perl files.
Tom Lane [Mon, 15 Aug 2016 15:32:09 +0000 (11:32 -0400)]
Simplify the process of perltidy'ing our Perl files.

Wrap the perltidy invocation into a shell script to reduce the risk of
copy-and-paste errors.  Include removal of *.bak files in the script,
so they don't accidentally get committed.  Improve the directions in
the README file.

8 years agoRemove bogus dependencies on NUMERIC_MAX_PRECISION.
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>

8 years agoFix assorted bugs in contrib/bloom.
Tom Lane [Sun, 14 Aug 2016 02:24:48 +0000 (22:24 -0400)]
Fix assorted bugs in contrib/bloom.

In blinsert(), cope with the possibility that a page we pull from the
notFullPage list is marked BLOOM_DELETED.  This could happen if VACUUM
recently marked it deleted but hasn't (yet) updated the metapage.
We can re-use such a page safely, but we *must* reinitialize it so that
it's no longer marked deleted.

Fix blvacuum() so that it updates the notFullPage list even if it's
going to update it to empty.  The previous "optimization" of skipping
the update seems pretty dubious, since it means that the next blinsert()
will uselessly visit whatever pages we left in the list.

Uniformly treat PageIsNew pages the same as deleted pages.  This should
allow proper recovery if a crash occurs just after relation extension.

Properly use vacuum_delay_point, not assorted ad-hoc CHECK_FOR_INTERRUPTS
calls, in the blvacuum() main loop.

Fix broken tuple-counting logic: blvacuum.c counted the number of live
index tuples over again in each scan, leading to VACUUM VERBOSE reporting
some multiple of the actual number of surviving index tuples after any
vacuum that removed any tuples (since they'd be counted in blvacuum, maybe
more than once, and then again in blvacuumcleanup, without ever zeroing the
counter).  It's sufficient to count them in blvacuumcleanup.

stats->estimated_count is a boolean, not a counter, and we don't want
to set it true, so don't add tuple counts to it.

Add a couple of Asserts that we don't overrun available space on a bloom
page.  I don't think there's any bug there today, but the way the
FreeBlockNumberArray size calculation is set up is scarily fragile, and
BloomPageGetFreeSpace isn't much better.  The Asserts should help catch
any future mistakes.

Per investigation of a report from Jeff Janes.  I think the first item
above may explain his report; the other changes were things I noticed
while casting about for an explanation.

Report: <CAMkU=1xEUuBphDwDmB1WjN4+td4kpnEniFaTBxnk1xzHCw8_OQ@mail.gmail.com>

8 years agoAdd SQL-accessible functions for inspecting index AM properties.
Tom Lane [Sat, 13 Aug 2016 22:31:14 +0000 (18:31 -0400)]
Add SQL-accessible functions for inspecting index AM properties.

Per discussion, we should provide such functions to replace the lost
ability to discover AM properties by inspecting pg_am (cf commit
65c5fcd35).  The added functionality is also meant to displace any code
that was looking directly at pg_index.indoption, since we'd rather not
believe that the bit meanings in that field are part of any client API
contract.

As future-proofing, define the SQL API to not assume that properties that
are currently AM-wide or index-wide will remain so unless they logically
must be; instead, expose them only when inquiring about a specific index
or even specific index column.  Also provide the ability for an index
AM to override the behavior.

In passing, document pg_am.amtype, overlooked in commit 473b93287.

Andrew Gierth, with kibitzing by me and others

Discussion: <87mvl5on7n.fsf@news-spur.riddles.org.uk>

8 years agoDoc: clarify that DROP ... CASCADE is recursive.
Tom Lane [Fri, 12 Aug 2016 22:45:18 +0000 (18:45 -0400)]
Doc: clarify that DROP ... CASCADE is recursive.

Apparently that's not obvious to everybody, so let's belabor the point.

In passing, document that DROP POLICY has CASCADE/RESTRICT options (which
it does, per gram.y) but they do nothing (I assume, anyway).  Also update
some long-obsolete commentary in gram.y.

Discussion: <20160805104837.1412.84915@wrigleys.postgresql.org>

8 years agoFix inappropriate printing of never-measured times in EXPLAIN.
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>

8 years agoCode cleanup in SyncRepWaitForLSN()
Simon Riggs [Fri, 12 Aug 2016 11:43:45 +0000 (12:43 +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.

8 years agoCorrect TABLESAMPLE docs
Simon Riggs [Fri, 12 Aug 2016 09:34:43 +0000 (10:34 +0100)]
Correct TABLESAMPLE docs

Original wording was correct but not the intended meaning.

Reported by Patrik Wenger

8 years agoAdd ID property to replication slots' sect2
Alvaro Herrera [Thu, 11 Aug 2016 19:09:24 +0000 (15:09 -0400)]
Add ID property to replication slots' sect2

8 years agoTrivial cosmetic cleanup in bloom/blutils.c.
Tom Lane [Thu, 11 Aug 2016 16:23:35 +0000 (12:23 -0400)]
Trivial cosmetic cleanup in bloom/blutils.c.

Don't spell "InvalidOid" as "0".  Initialize method fields in the same
order as amapi.h declares them (and every other AM handler initializes
them).

8 years agoFix busted Assert for CREATE MATVIEW ... WITH NO DATA.
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>

8 years agodocs: my second pass over the 9.6 release notes
Bruce Momjian [Thu, 11 Aug 2016 03:08:44 +0000 (23:08 -0400)]
docs:  my second pass over the 9.6 release notes

8 years agoDoc: write some for adminpack.
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().

8 years agoFix typo
Peter Eisentraut [Tue, 9 Aug 2016 23:07:24 +0000 (19:07 -0400)]
Fix typo

8 years agodocs: my first pass over the 9.6 release notes
Bruce Momjian [Tue, 9 Aug 2016 22:36:16 +0000 (18:36 -0400)]
docs:  my first pass over the 9.6 release notes

8 years agoDoc: clarify description of CREATE/ALTER FUNCTION ... SET FROM CURRENT.
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.

8 years agoStamp 9.6beta4. REL9_6_BETA4
Tom Lane [Mon, 8 Aug 2016 20:25:04 +0000 (16:25 -0400)]
Stamp 9.6beta4.

8 years agodoc: update list of pg_trgm authors
Bruce Momjian [Mon, 8 Aug 2016 18:02:16 +0000 (14:02 -0400)]
doc:  update list of pg_trgm authors

Author: Oleg Bartunov

8 years agoUpdate 9.6 release notes through today.
Tom Lane [Mon, 8 Aug 2016 17:13:05 +0000 (13:13 -0400)]
Update 9.6 release notes through today.

8 years agoLast-minute updates for release notes.
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

8 years agoFix several one-byte buffer over-reads in to_number
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.

8 years agoTranslation updates
Peter Eisentraut [Mon, 8 Aug 2016 15:08:00 +0000 (11:08 -0400)]
Translation updates

Source-Git-URL: git://git.postgresql.org/git/pgtranslation/messages.git
Source-Git-Hash: cda21c1d7b160b303dc21dfe9d4169f2c8064c60

8 years agoFix two errors with nested CASE/WHEN constructs.
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.50602@enterprisedb.com>
Security: CVE-2016-5423

8 years agoObstruct shell, SQL, and conninfo injection via database and role names.
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

8 years agoPromote pg_dumpall shell/connstr quoting functions to src/fe_utils.
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

8 years agoFix Windows shell argument quoting.
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

8 years agoReject, in pg_dumpall, names containing CR or LF.
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

8 years agoField conninfo strings throughout src/bin/scripts.
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

8 years agoIntroduce a psql "\connect -reuse-previous=on|off" option.
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

8 years agoSort out paired double quotes in \connect, \password and \crosstabview.
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

8 years agodoc: Update benchmark results
Peter Eisentraut [Mon, 8 Aug 2016 13:27:20 +0000 (09:27 -0400)]
doc: Update benchmark results

From: Alexander Law <exclusion@gmail.com>

8 years agoMake format() error messages consistent again
Peter Eisentraut [Mon, 8 Aug 2016 12:15:41 +0000 (08:15 -0400)]
Make format() error messages consistent again

07d25a964 changed only one occurrence.  Change the other one as well.

8 years agoUpdate 9.6 release notes through today.
Tom Lane [Mon, 8 Aug 2016 02:24:44 +0000 (22:24 -0400)]
Update 9.6 release notes through today.

8 years agoCorrect column name in information schema
Peter Eisentraut [Mon, 8 Aug 2016 01:53:16 +0000 (21:53 -0400)]
Correct column name in information schema

Although the standard has routines.result_cast_character_set_name, given
the naming of the surrounding columns, we concluded that this must have
been a mistake and that result_cast_char_set_name was intended, so
change the implementation.  The documentation was already using the new
name.

found by Clément Prévost <prevostclement@gmail.com>

8 years agoRelease notes for 9.5.4, 9.4.9, 9.3.14, 9.2.18, 9.1.23.
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.

8 years agodoc: Move mention of rsync of temp tables to better place
Peter Eisentraut [Mon, 8 Aug 2016 01:27:36 +0000 (21:27 -0400)]
doc: Move mention of rsync of temp tables to better place

8 years agoFix misestimation of n_distinct for a nearly-unique column with many nulls.
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: <VisenaEmail.26.df42f82acae38a58.156463942b8@tc7-visena>
Discussion: <16143.1470350371@sss.pgh.pa.us>

8 years agoFix crash when pg_get_viewdef_name_ext() is passed a non-view relation.
Tom Lane [Sun, 7 Aug 2016 21:56:34 +0000 (17:56 -0400)]
Fix crash when pg_get_viewdef_name_ext() is passed a non-view relation.

Oversight in commit 976b24fb4.

Andreas Seltenreich

Report: <87y448l3ag.fsf@credativ.de>

8 years agoFix TOAST access failure in RETURNING queries.
Tom Lane [Sun, 7 Aug 2016 21:46:08 +0000 (17:46 -0400)]
Fix TOAST access failure in RETURNING queries.

Discussion of commit 3e2f3c2e4 exposed a problem that is of longer
standing: since we don't detoast data while sticking it into a portal's
holdStore for PORTAL_ONE_RETURNING and PORTAL_UTIL_SELECT queries, and we
release the query's snapshot as soon as we're done loading the holdStore,
later readout of the holdStore can do TOAST fetches against data that can
no longer be seen by any of the session's live snapshots.  This means that
a concurrent VACUUM could remove the TOAST data before we can fetch it.
Commit 3e2f3c2e4 exposed the problem by showing that sometimes we had *no*
live snapshots while fetching TOAST data, but we'd be at risk anyway.

I believe this code was all right when written, because our management of a
session's exposed xmin was such that the TOAST references were safe until
end of transaction.  But that's no longer true now that we can advance or
clear our PGXACT.xmin intra-transaction.

To fix, copy the query's snapshot during FillPortalStore() and save it in
the Portal; release it only when the portal is dropped.  This essentially
implements a policy that we must hold a relevant snapshot whenever we
access potentially-toasted data.  We had already come to that conclusion
in other places, cf commits 08e261cbc94ce9a7 and ec543db77b6b72f2.

I'd have liked to add a regression test case for this, but I didn't see
a way to make one that's not unreasonably bloated; it seems to require
returning a toasted value to the client, and those will be big.

In passing, improve PortalRunUtility() so that it positively verifies
that its ending PopActiveSnapshot() call will pop the expected snapshot,
removing a rather shaky assumption about which utility commands might
do their own PopActiveSnapshot().  There's no known bug here, but now
that we're actively referencing the snapshot it's almost free to make
this code a bit more bulletproof.

We might want to consider back-patching something like this into older
branches, but it would be prudent to let it prove itself more in HEAD
beforehand.

Discussion: <87vazemeda.fsf@credativ.de>

8 years agoAvoid crashing in GetOldestSnapshot() if there are no known snapshots.
Tom Lane [Sun, 7 Aug 2016 18:36:02 +0000 (14:36 -0400)]
Avoid crashing in GetOldestSnapshot() if there are no known snapshots.

The sole caller expects NULL to be returned in such a case, so make
it so and document it.

Per reports from Andreas Seltenreich and Regina Obe.  This doesn't
really fix their problem, as now their RETURNING queries will say
"ERROR: no known snapshots", but in any case this function should
not dump core in a reasonably-foreseeable situation.

Report: <87vazemeda.fsf@credativ.de>
Report: <20160807051854.1427.32414@wrigleys.postgresql.org>

8 years agoDon't propagate a null subtransaction snapshot up to parent transaction.
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.1430.39011@wrigleys.postgresql.org>

8 years agoFirst-draft release notes for 9.5.4.
Tom Lane [Sun, 7 Aug 2016 02:08:31 +0000 (22:08 -0400)]
First-draft release notes for 9.5.4.

As usual, the release notes for other branches will be made by cutting
these down, but put them up for community review first.

8 years agoIn B-tree page deletion, clean up properly after page deletion failure.
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.horiguchi.kyotaro@lab.ntt.co.jp>

8 years agoTeach libpq to decode server version correctly from future servers.
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>

8 years agoFix copy-and-pasteo in 81c766b3fd41c78c634d78ebae8d316808dfc630.
Tom Lane [Fri, 5 Aug 2016 20:21:38 +0000 (16:21 -0400)]
Fix copy-and-pasteo in 81c766b3fd41c78c634d78ebae8d316808dfc630.

Report: <57A4E6DF.8070209@dunslane.net>

8 years agoMake array_to_tsvector() sort and de-duplicate the given strings.
Tom Lane [Fri, 5 Aug 2016 20:09:06 +0000 (16:09 -0400)]
Make array_to_tsvector() sort and de-duplicate the given strings.

This is required for the result to be a legal tsvector value.
Noted while fooling with Andreas Seltenreich's ts_delete() crash.

Discussion: <87invhoj6e.fsf@credativ.de>

8 years agoFix ts_delete(tsvector, text[]) to cope with duplicate array entries.
Tom Lane [Fri, 5 Aug 2016 19:14:08 +0000 (15:14 -0400)]
Fix ts_delete(tsvector, text[]) to cope with duplicate array entries.

Such cases either failed an Assert, or produced a corrupt tsvector in
non-Assert builds, as reported by Andreas Seltenreich.  The reason is
that tsvector_delete_by_indices() just assumed that its input array had
no duplicates.  Fix by explicitly de-duping.

In passing, improve some comments, and fix a number of tests for null
values to use ERRCODE_NULL_VALUE_NOT_ALLOWED not
ERRCODE_INVALID_PARAMETER_VALUE.

Discussion: <87invhoj6e.fsf@credativ.de>

8 years agoRe-pgindent tsvector_op.c.
Tom Lane [Fri, 5 Aug 2016 18:58:13 +0000 (14:58 -0400)]
Re-pgindent tsvector_op.c.

Messed up by recent commits --- this is annoying me while trying to fix
some bugs here.

8 years agodocs: re-add spaces before units removed
Bruce Momjian [Fri, 5 Aug 2016 18:35:09 +0000 (14:35 -0400)]
docs:  re-add spaces before units removed

This reverts the spaces before k/M/G/TB units removed for consistency in
commit ca0c37b56f4a80ad758774e34c86cc4335583d29.

Discussion: 20160802165116.GC32575@momjian.us

8 years agoUpdate time zone data files to tzdata release 2016f.
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.

8 years agoChange InitToastSnapshot to a macro.
Robert Haas [Fri, 5 Aug 2016 15:57:00 +0000 (11:57 -0400)]
Change InitToastSnapshot to a macro.

tqual.h is included in some front-end compiles, and a static inline
breaks on buildfarm member castoroides.  Since the macro is never
referenced, it should dodge that problem, although this doesn't
seem like the cleanest way of hiding things from front-end compiles.

Report and review by Tom Lane; patch by me.

8 years agoFix hard to hit race condition in heapam's tuple locking code.
Andres Freund [Fri, 5 Aug 2016 03:07:16 +0000 (20:07 -0700)]
Fix hard to hit race condition in heapam's tuple locking code.

As mentioned in its commit message, eca0f1db left open a race condition,
where a page could be marked all-visible, after the code checked
PageIsAllVisible() to pin the VM, but before the page is locked.  Plug
that hole.

Reviewed-By: Robert Haas, Andres Freund
Author: Amit Kapila
Discussion: CAEepm=3fWAbWryVW9swHyLTY4sXVf0xbLvXqOwUoDiNCx9mBjQ@mail.gmail.com
Backpatch: -

8 years agodocs: mention rsync of temp and unlogged tables
Bruce Momjian [Thu, 4 Aug 2016 22:55:16 +0000 (18:55 -0400)]
docs:  mention rsync of temp and unlogged tables

This happens when using rsync to pg_upgrade slaves.

Reported-by: Jerry Sievers
Discussion: 20160726161946.GA3511@momjian.us

8 years agoFix bogus coding in WaitForBackgroundWorkerShutdown().
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>

8 years agodoc: Move indexterms to avoid whitespace issue in man pages
Peter Eisentraut [Wed, 3 Aug 2016 21:02:00 +0000 (17:02 -0400)]
doc: Move indexterms to avoid whitespace issue in man pages

8 years agoPrevent "snapshot too old" from trying to return pruned TOAST tuples.
Robert Haas [Wed, 3 Aug 2016 20:41:43 +0000 (16:41 -0400)]
Prevent "snapshot too old" from trying to return pruned TOAST tuples.

Previously, we tested for MVCC snapshots to see whether they were too
old, but not TOAST snapshots, which can lead to complaints about missing
TOAST chunks if those chunks are subject to early pruning.  Ideally,
the threshold lsn and timestamp for a TOAST snapshot would be that of
the corresponding MVCC snapshot, but since we have no way of deciding
which MVCC snapshot was used to fetch the TOAST pointer, use the oldest
active or registered snapshot instead.

Reported by Andres Freund, who also sketched out what the fix should
look like.  Patch by me, reviewed by Amit Kapila.

8 years agoMake INSERT-from-multiple-VALUES-rows handle targetlist indirection better.
Tom Lane [Wed, 3 Aug 2016 20:37:03 +0000 (16:37 -0400)]
Make INSERT-from-multiple-VALUES-rows handle targetlist indirection better.

Previously, if an INSERT with multiple rows of VALUES had indirection
(array subscripting or field selection) in its target-columns list, the
parser handled that by applying transformAssignedExpr() to each element
of each VALUES row independently.  This led to having ArrayRef assignment
nodes or FieldStore nodes in each row of the VALUES RTE.  That works for
simple cases, but in bug #14265 Nuri Boardman points out that it fails
if there are multiple assignments to elements/fields of the same target
column.  For such cases to work, rewriteTargetListIU() has to nest the
ArrayRefs or FieldStores together to produce a single expression to be
assigned to the column.  But it failed to find them in the top-level
targetlist and issued an error about "multiple assignments to same column".

We could possibly fix this by teaching the rewriter to apply
rewriteTargetListIU to each VALUES row separately, but that would be messy
(it would change the output rowtype of the VALUES RTE, for example) and
inefficient.  Instead, let's fix the parser so that the VALUES RTE outputs
are just the user-specified values, cast to the right type if necessary,
and then the ArrayRefs or FieldStores are applied in the top-level
targetlist to Vars representing the RTE's outputs.  This is the same
parsetree representation already used for similar cases with INSERT/SELECT
syntax, so it allows simplifications in ruleutils.c, which no longer needs
to treat INSERT-from-multiple-VALUES as its own special case.

This implementation works by applying transformAssignedExpr to the VALUES
entries as before, and then stripping off any ArrayRefs or FieldStores it
adds.  With lots of VALUES rows it would be noticeably more efficient to
not add those nodes in the first place.  But that's just an optimization
not a bug fix, and there doesn't seem to be any good way to do it without
significant refactoring.  (A non-invasive answer would be to apply
transformAssignedExpr + stripping to just the first VALUES row, and then
just forcibly cast remaining rows to the same data types exposed in the
first row.  But this way would lead to different, not-INSERT-specific
errors being reported in casting failure cases, so it doesn't seem very
nice.)  So leave that for later; this patch at least isn't making the
per-row parsing work worse, and it does make the finished parsetree
smaller, saving rewriter and planner work.

Catversion bump because stored rules containing such INSERTs would need
to change.  Because of that, no back-patch, even though this is a very
long-standing bug.

Report: <20160727005725.7438.26021@wrigleys.postgresql.org>
Discussion: <9578.1469645245@sss.pgh.pa.us>

8 years agoDo not let PostmasterContext survive into background workers.
Tom Lane [Wed, 3 Aug 2016 18:48:05 +0000 (14:48 -0400)]
Do not let PostmasterContext survive into background workers.

We don't want postmaster child processes to contain a copy of the
postmaster's PostmasterContext.  That would be a waste of memory at least,
and at worst a security issue, since there are copies of the semi-sensitive
pg_hba and pg_ident data in there.  All other child process types delete
the PostmasterContext after forking, but the original coding of the
background worker patch (commit da07a1e85) did not do so.  It appears
that the only reason for that was to avoid copying the bgworker's
MyBgworkerEntry out of that context; but the couple of additional
statements needed to do so are hardly good justification for it.  Hence,
copy that data and then clear the context as other child processes do.

Because this patch changes the memory context in which a bgworker function
gains control, back-patching it would be a bit risky, so we won't fix this
in back branches.  The "security" complaint is pretty thin anyway for
generic bgworkers; only with the introduction of parallel query is there
any question of running untrusted code in a bgworker process.

Discussion: <14111.1470082717@sss.pgh.pa.us>

8 years agoAdd missing casts in information schema
Peter Eisentraut [Wed, 3 Aug 2016 18:41:01 +0000 (14:41 -0400)]
Add missing casts in information schema

From: Clément Prévost <prevostclement@gmail.com>

8 years agodoc: Remove documentation of nonexistent information schema columns
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 <prevostclement@gmail.com>

8 years agoFix assorted problems in recovery tests
Alvaro Herrera [Wed, 3 Aug 2016 17:21:23 +0000 (13:21 -0400)]
Fix assorted problems in recovery tests

In test 001_stream_rep we're using pg_stat_replication.write_location to
determine catch-up status, but we care about xlog having been applied
not just received, so change that to apply_location.

In test 003_recovery_targets, we query the database for a recovery
target specification and later for the xlog position supposedly
corresponding to that recovery specification.  If for whatever reason
more WAL is written between the two queries, the recovery specification
is earlier than the xlog position used by the query in the test harness,
so we wait forever, leading to test failures.  Deal with this by using a
single query to extract both items.  In 2a0f89cd717 we tried to deal
with it by giving them more tests to run, but in hindsight that was
obviously doomed to failure (no revert of that, though).

Per hamster buildfarm failures.

Author: Michaël Paquier

8 years agodoc: Change recommendation to put NOTIFY into a rule
Peter Eisentraut [Wed, 3 Aug 2016 16:29:15 +0000 (12:29 -0400)]
doc: Change recommendation to put NOTIFY into a rule

Suggest a statement trigger instead.

8 years agoAdd OldSnapshotTimeMapLock to wait_event table in docs.
Kevin Grittner [Wed, 3 Aug 2016 14:58:50 +0000 (09:58 -0500)]
Add OldSnapshotTimeMapLock to wait_event table in docs.

Ashutosh Sharma with minor fixes by me.

8 years agoC comment: fix typo
Bruce Momjian [Wed, 3 Aug 2016 14:32:32 +0000 (10:32 -0400)]
C comment:  fix typo

Author: Amit Langote

8 years agodoc: Remove slightly confusing xreflabels
Peter Eisentraut [Wed, 3 Aug 2016 02:34:45 +0000 (22:34 -0400)]
doc: Remove slightly confusing xreflabels

It seems clearer to refer to these tables in the normal way.

8 years agoSmall wording tweaks
Peter Eisentraut [Wed, 3 Aug 2016 02:33:56 +0000 (22:33 -0400)]
Small wording tweaks

Dmitry Igrishin

8 years agoRemove duplicate InitPostmasterChild() call while starting a bgworker.
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: <CAEepm=1VOnbVx4wsgQFvj94hu9jVt2nVabCr7QiooUSvPJXkgQ@mail.gmail.com>

8 years agodoc: OS collation changes can break indexes
Bruce Momjian [Tue, 2 Aug 2016 21:13:10 +0000 (17:13 -0400)]
doc:  OS collation changes can break indexes

Discussion: 20160702155517.GD18610@momjian.us

Reviewed-by: Christoph Berg
Backpatch-through: 9.1

8 years agoBlock interrupts during HandleParallelMessages().
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>

8 years agoChange minimum max_worker_processes from 1 to 0
Peter Eisentraut [Tue, 2 Aug 2016 17:15:35 +0000 (13:15 -0400)]
Change minimum max_worker_processes from 1 to 0

Setting it to 0 is probably not useful in practice, but it allows
testing of situations without available background worker slots.

8 years agoFix pg_dump's handling of public schema with both -c and -C options.
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.2628.95990@wrigleys.postgresql.org>
Report: <20160801021955.1430.47434@wrigleys.postgresql.org>

8 years agodoc: Whitespace fixes in man pages
Peter Eisentraut [Tue, 2 Aug 2016 16:35:35 +0000 (12:35 -0400)]
doc: Whitespace fixes in man pages

8 years agoConsistently capitalize names of recovery tests
Peter Eisentraut [Tue, 2 Aug 2016 14:47:03 +0000 (10:47 -0400)]
Consistently capitalize names of recovery tests

8 years agoMinor cleanup for access/transam/parallel.c.
Tom Lane [Mon, 1 Aug 2016 20:12:01 +0000 (16:12 -0400)]
Minor cleanup for access/transam/parallel.c.

ParallelMessagePending *must* be marked volatile, because it's set
by a signal handler.  On the other hand, it's pointless for
HandleParallelMessageInterrupt to save/restore errno; that must be,
and is, done at the outer level of the SIGUSR1 signal handler.

Calling CHECK_FOR_INTERRUPTS() inside HandleParallelMessages, which itself
is called from CHECK_FOR_INTERRUPTS(), seems both useless and hazardous.
The comment claiming that this is needed to handle the error queue going
away is certainly misguided, in any case.

Improve a couple of error message texts, and use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE to report loss of parallel worker
connection, since that's what's used in e.g. tqueue.c.  (Maybe it would be
worth inventing a dedicated ERRCODE for this type of failure?  But I do not
think ERRCODE_INTERNAL_ERROR is appropriate.)

Minor stylistic cleanups.

8 years agoDon't CHECK_FOR_INTERRUPTS between WaitLatch and ResetLatch.
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>

8 years agoRemove unused arguments from pg_replication_origin_xact_reset function.
Fujii Masao [Mon, 1 Aug 2016 17:43:17 +0000 (02:43 +0900)]
Remove unused arguments from pg_replication_origin_xact_reset function.

The document specifies that pg_replication_origin_xact_reset function
doesn't have any argument variables. But previously it was actually
defined so as to have two argument variables, though they were not
used at all. That is, the pg_proc entry for that function was incorrect.
This patch fixes the pg_proc entry and removes those two arguments
from the function definition.

No back-patch because this change needs a catalog version bump
although the issue exists in 9.5 as well. Instead, a note about those
unused argument variables will be added to 9.5 document later.

Catalog version bumped due to the change of pg_proc.

8 years agopg_rewind docs: clarify handling of remote servers
Bruce Momjian [Mon, 1 Aug 2016 16:52:22 +0000 (12:52 -0400)]
pg_rewind docs: clarify handling of remote servers

8 years agoFixed array checking code for "unsigned long long" datatypes in libecpg.
Michael Meskes [Mon, 1 Aug 2016 04:36:27 +0000 (06:36 +0200)]
Fixed array checking code for "unsigned long long" datatypes in libecpg.

8 years agoFix pg_basebackup so that it accepts 0 as a valid compression level.
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

8 years agoDoc: remove claim that hash index creation depends on effective_cache_size.
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: <CAM3SWZRVANbj9GA9j40fAwheQCZQtSwqTN1GBTVwRrRbmSf7cg@mail.gmail.com>

8 years agoCode review for tqueue.c: fix memory leaks, speed it up, other fixes.
Tom Lane [Sun, 31 Jul 2016 20:05:12 +0000 (16:05 -0400)]
Code review for tqueue.c: fix memory leaks, speed it up, other fixes.

When doing record typmod remapping, tqueue.c did fresh catalog lookups
for each tuple it processed, which was pretty horrible performance-wise
(it seemed to about halve the already none-too-quick speed of bulk reads
in parallel mode).  Worse, it insisted on putting bits of that data into
TopMemoryContext, from where it never freed them, causing a
session-lifespan memory leak.  (I suppose this was coded with the idea
that the sender process would quit after finishing the query ---
but the receiver uses the same code.)

Restructure to avoid repetitive catalog lookups and to keep that data
in a query-lifespan context, in or below the context where the
TQueueDestReceiver or TupleQueueReader itself lives.

Fix some other bugs such as continuing to use a tupledesc after
releasing our refcount on it.  Clean up cavalier datatype choices
(typmods are int32, please, not int, and certainly not Oid).  Improve
comments and error message wording.

8 years agoCorrectly handle owned sequences with extensions
Stephen Frost [Sun, 31 Jul 2016 14:57:15 +0000 (10:57 -0400)]
Correctly handle owned sequences with extensions

With the refactoring of pg_dump to handle components, getOwnedSeqs needs
to be a bit more intelligent regarding which components to dump when.
Specifically, we can't simply use the owning table's components as the
set of components to dump as the table might only be including certain
components while all components of the sequence should be dumped, for
example, when the table is a member of an extension while the sequence
is not.

Handle this by combining the set of components to be dumped for the
sequence explicitly and those to be dumped for the table when setting
the components to be dumped for the sequence.

Also add a number of regression tests around this to, hopefully, catch
any future changes which break the expected behavior.

Discovered by: Philippe BEAUDOIN
Reviewed by: Michael Paquier

8 years agodoc: improve wording of Error Message Style Guide
Bruce Momjian [Sun, 31 Jul 2016 01:34:28 +0000 (21:34 -0400)]
doc:  improve wording of Error Message Style Guide

Reported-by: Daniel Gustafsson
Discussion: 48DB4EDA-96F8-4B2F-99C4-110900FC7540@yesql.se

Author: Daniel Gustafsson

8 years agopgbench docs: fix incorrect "last two" fields text
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

8 years agodocs: properly capitalize and space kB, MB, GB, TB
Bruce Momjian [Sat, 30 Jul 2016 16:27:27 +0000 (12:27 -0400)]
docs: properly capitalize and space kB, MB, GB, TB

8 years agoFix worst memory leaks in tqueue.c.
Tom Lane [Fri, 29 Jul 2016 23:31:06 +0000 (19:31 -0400)]
Fix worst memory leaks in tqueue.c.

TupleQueueReaderNext() leaks like a sieve if it has to do any tuple
disassembly/reconstruction.  While we could try to clean up its allocations
piecemeal, it seems like a better idea just to insist that it should be run
in a short-lived memory context, so that any transient space goes away
automatically.  I chose to have nodeGather.c switch into its existing
per-tuple context before the call, rather than inventing a separate
context inside tqueue.c.

This is sufficient to stop all leakage in the simple case I exhibited
earlier today (see link below), but it does not deal with leaks induced
in more complex cases by tqueue.c's insistence on using TopMemoryContext
for data that it's not actually trying hard to keep track of.  That issue
is intertwined with another major source of inefficiency, namely failure
to cache lookup results across calls, so it seems best to deal with it
separately.

In passing, improve some comments, and modify gather_readnext's method for
deciding when it's visited all the readers so that it's more obviously
correct.  (I'm not actually convinced that the previous code *is*
correct in the case of a reader deletion; it certainly seems fragile.)

Discussion: <32763.1469821037@sss.pgh.pa.us>

8 years agoFix tqueue.c's range-remapping code.
Tom Lane [Fri, 29 Jul 2016 18:13:19 +0000 (14:13 -0400)]
Fix tqueue.c's range-remapping code.

It's depressingly clear that nobody ever tested this.

8 years agoFix pq_putmessage_noblock() to not block.
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.horiguchi.kyotaro@lab.ntt.co.jp>

8 years agoEliminate a few more user-visible "cache lookup failed" errors.
Robert Haas [Fri, 29 Jul 2016 16:06:18 +0000 (12:06 -0400)]
Eliminate a few more user-visible "cache lookup failed" errors.

Michael Paquier

8 years agoDocumentation spell checking and markup improvements
Peter Eisentraut [Fri, 29 Jul 2016 02:46:15 +0000 (22:46 -0400)]
Documentation spell checking and markup improvements