Commit Graph

41924 Commits

Author SHA1 Message Date
Andrew Dunstan
289bafdbc4 Clear severity 5 perlcritic warnings from vcregress.pl
My recent update for python3 support used some idioms that are
unapproved. This fixes them. Backpatch to all live branches like the
original.
2018-05-06 07:39:37 -04:00
Peter Eisentraut
ab7825eadf Tweak tests to support Python 3.7
Python 3.7 removes the trailing comma in the repr() of
BaseException (see <https://bugs.python.org/issue30399>), leading to
test output differences.  Work around that by composing the equivalent
test output in a more manual way.
2018-05-05 23:34:41 -04:00
Peter Eisentraut
7ff50cf60b Remove extra newlines after PQerrorMessage() 2018-05-05 10:53:28 -04:00
Heikki Linnakangas
7b7521d657 Fix scenario where streaming standby gets stuck at a continuation record.
If a continuation record is split so that its first half has already been
removed from the master, and is only present in pg_wal, and there is a
recycled WAL segment in the standby server that looks like it would
contain the second half, recovery would get stuck. The code in
XLogPageRead() incorrectly started streaming at the beginning of the
WAL record, even if we had already read the first page.

Backpatch to 9.4. In principle, older versions have the same problem, but
without replication slots, there was no straightforward mechanism to
prevent the master from recycling old WAL that was still needed by standby.
Without such a mechanism, I think it's reasonable to assume that there's
enough slack in how many old segments are kept around to not run into this,
or you have a WAL archive.

Reported by Jonathon Nelson. Analysis and patch by Kyotaro HORIGUCHI, with
some extra comments by me.

Discussion: https://www.postgresql.org/message-id/CACJqAM3xVz0JY1XFDKPP%2BJoJAjoGx%3DGNuOAshEDWCext7BFvCQ%40mail.gmail.com
2018-05-05 01:35:12 +03:00
Alvaro Herrera
3a11485a52 Don't mark pages all-visible spuriously
Dan Wood diagnosed a long-standing problem that pages containing tuples
that are locked by multixacts containing live lockers may spuriously end
up as candidates for getting their all-visible flag set.  This has the
long-term effect that multixacts remain unfrozen; this may previously
pass undetected, but since commit XYZ it would be reported as
  "ERROR: found multixact 134100944 from before relminmxid 192042633"
because when a later vacuum tries to freeze the page it detects that a
multixact that should have gotten frozen, wasn't.

Dan proposed a (correct) patch that simply sets a variable to its
correct value, after a bogus initialization.  But, per discussion, it
seems better coding to avoid the bogus initializations altogether, since
they could give rise to more bugs later.  Therefore this fix rewrites
the logic a little bit to avoid depending on the bogus initializations.

This bug was part of a family introduced in 9.6 by commit a892234f830e;
later, commit 38e9f90a22 fixed most of them, but this one was
unnoticed.

Authors: Dan Wood, Pavan Deolasee, Álvaro Herrera
Reviewed-by: Masahiko Sawada, Pavan Deolasee, Álvaro Herrera
Discussion: https://postgr.es/m/84EBAC55-F06D-4FBE-A3F3-8BDA093CE3E3@amazon.com
2018-05-04 18:23:30 -03:00
Andrew Dunstan
a9fbf550b1 Provide for testing on python3 modules when under MSVC
This should have been done some years ago as promised in commit
c4dcdd0c2. However, better late than never.

Along the way do a little housekeeping, including using a simpler test
for the python version being tested, and removing a redundant subroutine
parameter. These changes only apply back to release 9.5.

Backpatch to all live releases.
2018-05-04 15:33:06 -04:00
Andrew Dunstan
679b07469a Allow MSYS as well as MINGW in Msys uname
Msys2's uname -s outputs a string beginning MSYS rather than MINGW as is
output by Msys. Allow either in pg_upgrade's test.sh.

Backpatch to all live branches.
2018-05-04 15:01:57 -04:00
Tom Lane
7a83323f2d Sync our copy of the timezone library with IANA release tzcode2018e.
The non-cosmetic changes involve teaching the "zic" tzdata compiler about
negative DST.  While I'm not currently intending that we start using
negative-DST data right away, it seems possible that somebody would try
to use our copy of zic with bleeding-edge IANA data.  So we'd better be
out in front of this change code-wise, even though it doesn't matter for
the data file we're shipping.

Discussion: https://postgr.es/m/30996.1525445902@sss.pgh.pa.us
2018-05-04 12:26:39 -04:00
Teodor Sigaev
d9b3bc5520 Add HOLD_INTERRUPTS section into FinishPreparedTransaction.
If an interrupt arrives in the middle of FinishPreparedTransaction
and any callback decide to call CHECK_FOR_INTERRUPTS (e.g.
RemoveTwoPhaseFile can write a warning with ereport, which checks for
interrupts) then it's possible to leave current GXact undeleted.

Backpatch to all supported branches

Stas Kelvich

Discussion: ihttps://www.postgresql.org/message-id/3AD85097-A3F3-4EBA-99BD-C38EDF8D2949@postgrespro.ru
2018-05-03 20:09:28 +03:00
Tom Lane
eab8d6312f Revert back-branch changes in power()'s behavior for NaN inputs.
Per discussion, the value of fixing these bugs in the back branches
doesn't outweigh the downsides of changing corner-case behavior in
a minor release.  Hence, revert commits 217d8f3a1 and 4d864de48 in
the v10 branch and the corresponding commits in 9.3-9.6.

Discussion: https://postgr.es/m/75DB81BEEA95B445AE6D576A0A5C9E936A73E741@BPXM05GP.gisp.nec.co.jp
2018-05-02 17:32:40 -04:00
Tom Lane
938c6f42d8 Fix bogus list-iteration code in pg_regress.c, affecting ecpg tests only.
While looking at a recent buildfarm failure in the ecpg tests, I wondered
why the pg_regress output claimed the stderr part of the test failed, when
the regression diffs were clearly for the stdout part.  Looking into it,
the reason is that pg_regress.c's logic for iterating over three parallel
lists is wrong, and has been wrong since it was written: it advances the
"tag" pointer at a different place in the loop than the other two pointers.
Fix that.
2018-04-29 21:56:28 -04:00
Tom Lane
d6ec3d2375 Avoid wrong results for power() with NaN input on more platforms.
Buildfarm results show that the modern POSIX rule that 1 ^ NaN = 1 is not
honored on *BSD until relatively recently, and really old platforms don't
believe that NaN ^ 0 = 1 either.  (This is unsurprising, perhaps, since
SUSv2 doesn't require either behavior.)  In hopes of getting to platform
independent behavior, let's deal with all the NaN-input cases explicitly
in dpow().

Note that numeric_power() doesn't know either of these special cases.
But since that behavior is platform-independent, I think it should be
addressed separately, and probably not back-patched.

Discussion: https://postgr.es/m/75DB81BEEA95B445AE6D576A0A5C9E936A73E741@BPXM05GP.gisp.nec.co.jp
2018-04-29 18:15:16 -04:00
Tom Lane
2acbeea48c Update time zone data files to tzdata release 2018d.
DST law changes in Palestine and Antarctica (Casey Station).  Historical
corrections for Portugal and its colonies, as well as Enderbury, Jamaica,
Turks & Caicos Islands, and Uruguay.
2018-04-29 15:50:31 -04:00
Tom Lane
48e0f8cbe0 Avoid wrong results for power() with NaN input on some platforms.
Per spec, the result of power() should be NaN if either input is NaN.
It appears that on some versions of Windows, the libc function does
return NaN, but it also sets errno = EDOM, confusing our code that
attempts to work around shortcomings of other platforms.  Hence, add
guard tests to avoid substituting a wrong result for the right one.

It's been like this for a long time (and the odd behavior only appears
in older MSVC releases, too) so back-patch to all supported branches.

Dang Minh Huong, reviewed by David Rowley

Discussion: https://postgr.es/m/75DB81BEEA95B445AE6D576A0A5C9E936A73E741@BPXM05GP.gisp.nec.co.jp
2018-04-29 15:21:45 -04:00
Bruce Momjian
1d91af81ca docs: remove "III" version text from pgAdmin link
Reported-by: vodevsh@gmail.com

Discussion: https://postgr.es/m/152404286919.19366.7988650271505173666@wrigleys.postgresql.org

Backpatch-through: 9.3
2018-04-26 11:10:43 -04:00
Noah Misch
32c2476293 Correct pg_recvlogical server version test.
The predecessor test boiled down to "PQserverVersion(NULL) >= 100000",
which is always false.  No release includes that, so it could not have
reintroduced CVE-2018-1058.  Back-patch to 9.4, like the addition of the
predecessor in commit 8d2814f274.

Discussion: https://postgr.es/m/20180422215551.GB2676194@rfd.leadboat.com
2018-04-25 18:50:33 -07:00
Tom Lane
c76d0eed27 Fix race conditions when an event trigger is added concurrently with DDL.
EventTriggerTableRewrite crashed if there were table_rewrite triggers
present, but there had not been when the calling command started.

EventTriggerDDLCommandEnd called ddl_command_end triggers if present,
even if there had been no such triggers when the calling command started,
which would lead to a failure in pg_event_trigger_ddl_commands.

In both cases, fix by doing nothing; it's better to wait till the next
command when things will be properly initialized.

In passing, remove an elog(DEBUG1) call that might have seemed interesting
four years ago but surely isn't today.

We found this because of intermittent failures in the buildfarm.  Thanks
to Alvaro Herrera and Andrew Gierth for analysis.

Back-patch to 9.5; some of this code exists before that, but the specific
hazards we need to guard against don't.

Discussion: https://postgr.es/m/5767.1523995174@sss.pgh.pa.us
2018-04-20 17:15:31 -04:00
Tom Lane
64ad85860c Change more places to be less trusting of RestrictInfo.is_pushed_down.
On further reflection, commit e5d83995e didn't go far enough: pretty much
everywhere in the planner that examines a clause's is_pushed_down flag
ought to be changed to use the more complicated behavior where we also
check the clause's required_relids.  Otherwise we could make incorrect
decisions about whether, say, a clause is safe to use as a hash clause.

Some (many?) of these places are safe as-is, either because they are
never reached while considering a parameterized path, or because there
are additional checks that would reject a pushed-down clause anyway.
However, it seems smarter to just code them all the same way rather
than rely on easily-broken reasoning of that sort.

In support of that, invent a new macro RINFO_IS_PUSHED_DOWN that should
be used in place of direct tests on the is_pushed_down flag.

Like the previous patch, back-patch to all supported branches.

Discussion: https://postgr.es/m/f8128b11-c5bf-3539-48cd-234178b2314d@proxel.se
2018-04-20 15:19:17 -04:00
Tom Lane
306d6e59f7 Fix broken extract_actual_join_clauses call in 9.6 postgres_fdw.
In commits e5d83995e et al, I changed the signature of
extract_actual_join_clauses, thinking that it was not called from
anywhere but createplan.c.  I missed that postgres_fdw uses it
in the 9.6 branch only.

This opens up the question of whether any third-party modules might
be calling it, and whether we need to take steps to avoid an API break
for them.  But for the moment, just get the buildfarm green again.
2018-04-19 18:29:39 -04:00
Tom Lane
0c141fcaa7 Fix incorrect handling of join clauses pushed into parameterized paths.
In some cases a clause attached to an outer join can be pushed down into
the outer join's RHS even though the clause is not degenerate --- this
can happen if we choose to make a parameterized path for the RHS.  If
the clause ends up attached to a lower outer join, we'd misclassify it
as being a "join filter" not a plain "filter" condition at that node,
leading to wrong query results.

To fix, teach extract_actual_join_clauses to examine each join clause's
required_relids, not just its is_pushed_down flag.  (The latter now
seems vestigial, or at least in need of rethinking, but we won't do
anything so invasive as redefining it in a bug-fix patch.)

This has been wrong since we introduced parameterized paths in 9.2,
though it's evidently hard to hit given the lack of previous reports.
The test case used here involves a lateral function call, and I think
that a lateral reference may be required to get the planner to select
a broken plan; though I wouldn't swear to that.  In any case, even if
LATERAL is needed to trigger the bug, it still affects all supported
branches, so back-patch to all.

Per report from Andreas Karlsson.  Thanks to Andrew Gierth for
preliminary investigation.

Discussion: https://postgr.es/m/f8128b11-c5bf-3539-48cd-234178b2314d@proxel.se
2018-04-19 15:49:12 -04:00
Alvaro Herrera
b3f4742aab Enlarge find_other_exec's meager fgets buffer
The buffer was 100 bytes long, which is barely sufficient when the
version string gets longer (such as by configure --with-extra-version).
Set it to MAXPGPATH.

Author: Nikhil Sontakke
Discussion: https://postgr.es/m/CAMGcDxfLfpYU_Jru++L6ARPCOyxr0W+2O3Q54TDi5XdYeU36ow@mail.gmail.com
2018-04-19 10:45:15 -03:00
Tom Lane
69e3a548e9 Better fix for deadlock hazard in CREATE INDEX CONCURRENTLY.
Commit 54eff5311 did not account for the possibility that we'd have
a transaction snapshot due to default_transaction_isolation being
set high enough to require one.  The transaction snapshot is enough
to hold back our advertised xmin and thus risk deadlock anyway.
The only way to get rid of that snap is to start a new transaction,
so let's do that instead.  Also throw in an assert checking that we
really have gotten to a state where no xmin is being advertised.

Back-patch to 9.4, like the previous commit.

Discussion: https://postgr.es/m/CAMkU=1ztk3TpQdcUNbxq93pc80FrXUjpDWLGMeVBDx71GHNwZQ@mail.gmail.com
2018-04-18 12:07:37 -04:00
Tom Lane
d90b2904c7 Fix broken collation-aware searches in SP-GiST text opclass.
spg_text_leaf_consistent() supposed that it should compare only
Min(querylen, entrylen) bytes of the two strings, and then deal with
any excess bytes in one string or the other by assuming the longer
string is greater if the prefixes are equal.  Quite aside from the
fact that that's just wrong in some locales (e.g., 'ch' is not less
than 'd' in cs_CZ), it also risked passing incomplete multibyte
characters to strcoll(), with ensuing bad results.

Instead, just pass the full strings to varstr_cmp, and let it decide
what to do about unequal-length strings.

Fortunately, this error doesn't imply any index corruption, it's just
that searches might return the wrong set of entries.

Per report from Emre Hasegeli, though this is not his patch.
Thanks to Peter Geoghegan for review and discussion.

This code was born broken, so back-patch to all supported branches.
In HEAD, I failed to resist the temptation to do a bit of cosmetic
cleanup/pgindent'ing on 710d90da1, too.

Discussion: https://postgr.es/m/CAE2gYzzb6K51VnTq5i5p52z+j9p2duEa-K1T3RrC_GQEynAKEg@mail.gmail.com
2018-04-16 16:06:47 -04:00
Tom Lane
65f2e868b2 Fix potentially-unportable code in contrib/adminpack.
Spelling access(2)'s second argument as "2" is just horrid.
POSIX makes no promises as to the numeric values of W_OK and related
macros.  Even if it accidentally works as intended on every supported
platform, it's still unreadable and inconsistent with adjacent code.

In passing, don't spell "NULL" as "0" either.  Yes, that's legal C;
no, it's not project style.

Back-patch, just in case the unportability is real and not theoretical.
(Most likely, even if a platform had different bit assignments for
access()'s modes, there'd not be an observable behavior difference
here; but I'm being paranoid today.)
2018-04-15 13:02:11 -04:00
Tom Lane
131f6a9583 In libpq, free any partial query result before collecting a server error.
We'd throw away the partial result anyway after parsing the error message.
Throwing it away beforehand costs nothing and reduces the risk of
out-of-memory failure.  Also, at least in systems that behave like
glibc/Linux, if the partial result was very large then the error PGresult
would get allocated at high heap addresses, preventing the heap storage
used by the partial result from being released to the OS until the error
PGresult is freed.

In psql >= 9.6, we hold onto the error PGresult until another error is
received (for \errverbose), so that this behavior causes a seeming
memory leak to persist for awhile, as in a recent complaint from
Darafei Praliaskouski.  This is a potential performance regression from
older versions, justifying back-patching at least that far.  But similar
behavior may occur in other client applications, so it seems worth just
back-patching to all supported branches.

Discussion: https://postgr.es/m/CAC8Q8tJ=7cOkPePyAbJE_Pf691t8nDFhJp0KZxHvnq_uicfyVg@mail.gmail.com
2018-04-13 12:53:46 -04:00
Tom Lane
0f439c8dd2 Fix bogus affix-merging code.
NISortAffixes() compared successive compound affixes incorrectly,
thus possibly failing to merge identical affixes, or (less likely)
merging ones that shouldn't be merged.  The user-visible effects
of this are unclear, to me anyway.

Per bug #15150 from Alexander Lakhin.  It's been broken for a long time,
so back-patch to all supported branches.

Arthur Zakirov

Discussion: https://postgr.es/m/152353327780.31225.13445405496721177988@wrigleys.postgresql.org
2018-04-12 18:39:51 -04:00
Tom Lane
060bb38d07 Ignore nextOid when replaying an ONLINE checkpoint.
The nextOid value is from the start of the checkpoint and may well be stale
compared to values from more recent XLOG_NEXTOID records.  Previously, we
adopted it anyway, allowing the OID counter to go backwards during a crash.
While this should be harmless, it contributed to the severity of the bug
fixed in commit 0408e1ed5, by allowing duplicate TOAST OIDs to be assigned
immediately following a crash.  Without this error, that issue would only
have arisen when TOAST objects just younger than a multiple of 2^32 OIDs
were deleted and then not vacuumed in time to avoid a conflict.

Pavan Deolasee

Discussion: https://postgr.es/m/CABOikdOgWT2hHkYG3Wwo2cyZJq2zfs1FH0FgX-=h4OLosXHf9w@mail.gmail.com
2018-04-11 18:11:30 -04:00
Tom Lane
8bba10f7e8 Do not select new object OIDs that match recently-dead entries.
When selecting a new OID, we take care to avoid picking one that's already
in use in the target table, so as not to create duplicates after the OID
counter has wrapped around.  However, up to now we used SnapshotDirty when
scanning for pre-existing entries.  That ignores committed-dead rows, so
that we could select an OID matching a deleted-but-not-yet-vacuumed row.
While that mostly worked, it has two problems:

* If recently deleted, the dead row might still be visible to MVCC
snapshots, creating a risk for duplicate OIDs when examining the catalogs
within our own transaction.  Such duplication couldn't be visible outside
the object-creating transaction, though, and we've heard few if any field
reports corresponding to such a symptom.

* When selecting a TOAST OID, deleted toast rows definitely *are* visible
to SnapshotToast, and will remain so until vacuumed away.  This leads to
a conflict that will manifest in errors like "unexpected chunk number 0
(expected 1) for toast value nnnnn".  We've been seeing reports of such
errors from the field for years, but the cause was unclear before.

The fix is simple: just use SnapshotAny to search for conflicting rows.
This results in a slightly longer window before object OIDs can be
recycled, but that seems unlikely to create any large problems.

Pavan Deolasee

Discussion: https://postgr.es/m/CABOikdOgWT2hHkYG3Wwo2cyZJq2zfs1FH0FgX-=h4OLosXHf9w@mail.gmail.com
2018-04-11 17:41:25 -04:00
Heikki Linnakangas
74dc05e01e Make local copy of client hostnames in backend status array.
The other strings, application_name and query string, were snapshotted to
local memory in pgstat_read_current_status(), but we forgot to do that for
client hostnames. As a result, the client hostname would appear to change in
the local copy, if the client disconnected.

Backpatch to all supported versions.

Author: Edmund Horner
Reviewed-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/CAMyN-kA7aOJzBmrYFdXcc7Z0NmW%2B5jBaf_m%3D_-77uRNyKC9r%3DA%40mail.gmail.com
2018-04-11 23:40:13 +03:00
Tom Lane
494f3cb5bb Fix incorrect close() call in dsm_impl_mmap().
One improbable error-exit path in this function used close() where
it should have used CloseTransientFile().  This is unlikely to be
hit in the field, and I think the consequences wouldn't be awful
(just an elog(LOG) bleat later).  But a bug is a bug, so back-patch
to 9.4 where this code came in.

Pan Bian

Discussion: https://postgr.es/m/152056616579.4966.583293218357089052@wrigleys.postgresql.org
2018-04-10 18:34:40 -04:00
Teodor Sigaev
5b0fa06f65 Remove wrongly backpatched piece of code in cube.c
Due to sloppy division of changes between f50c80dbb (which was not
back-patched) and 563a053bd, this piece of code was wrongly backpatched to
REL_10_STABLE and REL9_6_STABLE.  This code never causes real error because
its condition is never satisfied, but it's a dead code, which needs to be
removed.

Alexander Korotkov per gripe from Tom Lane
2018-04-10 14:59:27 +03:00
Tom Lane
3d465826f2 Doc: clarify explanation of pg_dump usage.
This section confusingly used both "infile" and "outfile" to refer
to the same file, i.e. the textual output of pg_dump.  Use "dumpfile"
for both cases, per suggestion from Jonathan Katz.

Discussion: https://postgr.es/m/152311295239.31235.6487236091906987117@wrigleys.postgresql.org
2018-04-08 16:35:43 -04:00
Andres Freund
5faead30d8 Remove overzeleous assertions in pg_atomic_flag code.
The atomics code asserts proper alignment in various places. That's
mainly because the alignment of 64bit integers is not sufficient for
atomic operations on all platforms. Some ABIs only have four byte
alignment, but don't have atomic behavior when crossing page
boundaries.

The flags code isn't affected by that however, as the type alignment
always is sufficient for atomic operations. Nevertheless the code
asserted alignment requirements. Before 8c3debbb it was only broken on
hppa, after it probably affect further platforms.

Thus remove the assertions for pg_atomic_flag operators.

Per buildfarm animal pademelon.

Discussion: https://postgr.es/m/7223.1523124425@sss.pgh.pa.us
Backpatch: 9.5-
2018-04-07 18:30:15 -07:00
Andres Freund
1f3bbe00b7 Fix and improve pg_atomic_flag fallback implementation.
The atomics fallback implementation for pg_atomic_flag was broken,
returning the inverted value from pg_atomic_test_set_flag().  This was
unnoticed because a) atomic flags were unused until recently b) the
test code wasn't run when the fallback implementation was in
use (because it didn't allow to test for some edge cases).

Fix the bug, and improve the fallback so it has the same behaviour as
the non-fallback implementation in the problematic edge cases. That
breaks ABI compatibility in the back branches when fallbacks are in
use, but given they were broken until now...

Author: Andres Freund
Reported-by: Daniel Gustafsson
Discussion:
    https://postgr.es/m/FB948276-7B32-4B77-83E6-D00167F8EEB4@yesql.se
    https://postgr.es/m/20180406233854.uni2h3mbnveczl32@alap3.anarazel.de
Backpatch: 9.5-, where the atomics abstraction was introduced.
2018-04-06 20:02:02 -07:00
Bruce Momjian
e177450708 doc: remove mention of the DMOZ catalog in ltree docs
Discussion: https://postgr.es/m/CAF4Au4xYem_W3KOuxcKct7=G4j8Z3uO9j3DUKTFJqUsfp_9pQg@mail.gmail.com

Author: Oleg Bartunov

Backpatch-through: 9.3
2018-04-05 15:55:41 -04:00
Bruce Momjian
374204ce81 docs: update ltree URL for the DMOZ catalog
Reported-by: bbrincat@gmail.com

Discussion: https://postgr.es/m/152283596377.1441.11672249301622760943@wrigleys.postgresql.org

Author: Oleg Bartunov

Backpatch-through: 9.3
2018-04-04 15:06:21 -04:00
Heikki Linnakangas
0d2012c9f0 Also fix the descriptions in pg_config.h.win32.
I missed pg_config.h.win32 in the previous commit that fixed these in
pg_config.h.in.
2018-04-04 11:34:28 +03:00
Heikki Linnakangas
7be511a767 Fix incorrect description of USE_SLICING_BY_8_CRC32C.
And a typo in the description of USE_SSE42_CRC32C_WITH_RUNTIME_CHECK,
spotted by Daniel Gustafsson.
2018-04-04 11:25:26 +03:00
Bruce Momjian
77b9c507d4 doc: document "IS NOT DOCUMENT"
Reported-by: scott.ure@caseware.com

Discussion: https://postgr.es/m/152056505045.4963.16783351661813640274@wrigleys.postgresql.org

Author: Euler Taveira

Backpatch-through: 9.3
2018-04-02 16:41:46 -04:00
Tom Lane
6cd110c477 Fix assorted issues in parallel vacuumdb.
Avoid storing the result of PQsocket() in a pgsocket variable; it's
declared as int, and the no-socket test is properly written as "x < 0"
not "x == PGINVALID_SOCKET".  This accidentally had no bad effect
because we never got to init_slot() with a bad connection, but it's
still wrong.

Actually, it seems like we should avoid storing the result for a long
period at all.  The function's not so expensive that it's worth avoiding,
and the existing coding technique here would fail if anyone tried to
PQreset the connection during the life of the program.  Hence, just
re-call PQsocket every time we construct a select(2) mask.

Speaking of select(), GetIdleSlot imagined that it could compute the
select mask once and continue to use it over multiple calls to
select_loop(), which is pretty bogus since that would stomp on the
mask on return.  This could only matter if the function's outer loop
iterated more than once, which is unlikely (it'd take some connection
receiving data, but not enough to complete its command).  But if it
did happen, we'd acquire "tunnel vision" and stop watching the other
connections for query termination, with the effect of losing parallelism.

Another way in which GetIdleSlot could lose parallelism is that once
PQisBusy returns false, it would lock in on that connection and do
PQgetResult until that returns NULL; in some cases that could result
in blocking.  (Perhaps this can never happen in vacuumdb due to the
limited set of commands that it can issue, but I'm not quite sure
of that, and even if true today it's not a future-proof assumption.)
Refactor the code to do that properly, so that it risks blocking in
PQgetResult only in cases where we need to wait anyway.

Another loss-of-parallelism problem, which *is* easily demonstrable,
is that any setup queries issued during prepare_vacuum_command() were
always issued on the last-to-be-created connection, whether or not
that was idle.  Long-running operations on that connection thus
prevented issuance of additional operations on the other ones, except
in the limited cases where no preparatory query was needed.  Instead,
wait till we've identified a free connection and use that one.

Also, avoid core dump due to undersized malloc request in the case
that no tables are identified to be vacuumed.

The bogus no-socket test was noted by CharSyam, the other problems
identified in my own code review.  Back-patch to 9.5 where parallel
vacuumdb was introduced.

Discussion: https://postgr.es/m/CAMrLSE6etb33-192DTEUGkV-TsvEcxtBDxGWG1tgNOMnQHwgDA@mail.gmail.com
2018-03-31 16:28:52 -04:00
Tom Lane
91d82317d2 Fix bogus provolatile/proparallel markings on a few built-in functions.
Richard Yen reported that pg_upgrade failed if the target cluster had
force_parallel_mode = on, because binary_upgrade_create_empty_extension()
is marked parallel restricted, allowing it to be executed in parallel
mode, which complains because it tries to acquire an XID.

In general, no function that might try to modify database data should
be considered parallel safe or restricted, since execution of it might
force XID acquisition.  We found several other examples of this mistake.

Furthermore, functions that execute user-supplied SQL queries or query
fragments, or pull data from user-supplied cursors, had better be marked
both volatile and parallel unsafe, because we don't know what the supplied
query or cursor might try to do.  There were several tsquery and XML
functions that had the wrong proparallel marking for this, and some of
them were even mislabeled as to volatility.

All these bugs are old, dating back to 9.6 for the proparallel mistakes
and much further for the provolatile mistakes.  We can't force a
catversion bump in the back branches, but we can at least ensure that
installations initdb'd in future have the right values.

Thomas Munro and Tom Lane

Discussion: https://postgr.es/m/CAEepm=2sNDScSLTfyMYu32Q=ob98ZGW-vM_2oLxinzSABGQ6VA@mail.gmail.com
2018-03-30 18:14:51 -04:00
Bruce Momjian
64f76ac689 docs: add parameter with brackets around varbit()
Reported-by: scott.ure@caseware.com

Discussion: https://postgr.es/m/152074343671.1853.18284519607571497106@wrigleys.postgresql.org

Author: Euler Taveira

Backpatch-through: 9.3
2018-03-30 13:34:12 -04:00
Fujii Masao
52c32d8d8d Fix handling of files that source server removes during pg_rewind is running.
After processing the filemap to build the list of chunks that will be
fetched from the source to rewing the target server, it is possible that
a file which was previously processed is removed from the source.  A
simple example of such an occurence is a WAL segment which gets recycled
on the target in-between.  When the filemap is processed, files not
categorized as relation files are first truncated to prepare for its
full copy of which is going to be taken from the source, divided into a
set of junks.  However, for a recycled WAL segment, this would result in
a segment which has a zero-byte size.  With such an empty file,
post-rewind recovery thinks that records are saved but they are actually
not because of the truncation which happened when processing the
filemap, resulting in data loss.

In order to fix the problem, make sure that files which are found as
removed on the source when receiving chunks of them are as well deleted
on the target server for consistency.

Back-patch to 9.5 where pg_rewind was added.

Author: Tsunakawa Takayuki
Reviewed-by: Michael Paquier
Reported-by: Tsunakawa Takayuki

Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F8DAAA2%40G01JPEXMBYT05
2018-03-29 04:02:08 +09:00
Tom Lane
90decdba37 Fix actual and potential double-frees around tuplesort usage.
tuplesort_gettupleslot() passed back tuples allocated in the tuplesort's
own memory context, even when the caller was responsible to free them.
This created a double-free hazard, because some callers might destroy
the tuplesort object (via tuplesort_end) before trying to clean up the
last returned tuple.  To avoid this, change the API to specify that the
tuple is allocated in the caller's memory context.  v10 and HEAD already
did things that way, but in 9.5 and 9.6 this is a live bug that can
demonstrably cause crashes with some grouping-set usages.

In 9.5 and 9.6, this requires doing an extra tuple copy in some cases,
which is unfortunate.  But the amount of refactoring needed to avoid it
seems excessive for a back-patched change, especially since the cases
where an extra copy happens are less performance-critical.

Likewise change tuplesort_getdatum() to return pass-by-reference Datums
in the caller's context not the tuplesort's context.  There seem to be
no live bugs among its callers, but clearly the same sort of situation
could happen in future.

For other tuplesort fetch routines, continue to allocate the memory in
the tuplesort's context.  This is a little inconsistent with what we now
do for tuplesort_gettupleslot() and tuplesort_getdatum(), but that's
preferable to adding new copy overhead in the back branches where it's
clearly unnecessary.  These other fetch routines provide the weakest
possible guarantees about tuple memory lifespan from v10 on, anyway,
so this actually seems more consistent overall.

Adjust relevant comments to reflect these API redefinitions.

Arguably, we should change the pre-9.5 branches as well, but since
there are no known failure cases there, it seems not worth the risk.

Peter Geoghegan, per report from Bernd Helmle.  Reviewed by Kyotaro
Horiguchi; thanks also to Andreas Seltenreich for extracting a
self-contained test case.

Discussion: https://postgr.es/m/1512661638.9720.34.camel@oopsware.de
2018-03-28 13:26:43 -04:00
Alvaro Herrera
43cd7abdcc Fix thinko in comment
The listed numbers disagreed with the ones being used in the symbols;
but instead of just fixing the numbers in the comment, use the symbolic
name instead, which seems clearer.

This has been wrong all along, so apply back to 9.5 where BRIN was
introduced.

Reported-by: Tomas Vondra
Discussion: https://postgr.es/m/5ff514f2-8b1e-6366-b11c-8e2ed442562d@2ndquadrant.com
2018-03-26 12:03:03 -03:00
Tom Lane
356f85f95c Doc: add example of type resolution in nested UNIONs.
Section 10.5 didn't say explicitly that multiple UNIONs are resolved
pairwise.  Since the resolution algorithm is described as taking any
number of inputs, readers might well think that a query like
"select x union select y union select z" would be resolved by
considering x, y, and z in one resolution step.  But that's not what
happens (and I think that behavior is per SQL spec).  Add an example
clarifying this point.

Per bug #15129 from Philippe Beaudoin.

Discussion: https://postgr.es/m/152196085023.32649.9916472370480121694@wrigleys.postgresql.org
2018-03-25 16:15:16 -04:00
Tom Lane
7b55a3b167 Doc: remove extra comma in syntax summary for array_fill().
Noted by Scott Ure.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/152199346794.4544.1888397173908716912@wrigleys.postgresql.org
2018-03-25 12:38:36 -04:00
Noah Misch
2c8974e6a0 Don't qualify type pg_catalog.text in extend-extensions-example.
Extension scripts begin execution with pg_catalog at the front of the
search path, so type names reliably refer to pg_catalog.  Remove these
superfluous qualifications.  Earlier <programlisting> of this <sect1>
already omitted them.  Back-patch to 9.3 (all supported versions).
2018-03-23 20:31:06 -07:00
Tom Lane
36c07fc299 Fix make rules that generate multiple output files.
For years, our makefiles have correctly observed that "there is no correct
way to write a rule that generates two files".  However, what we did is to
provide empty rules that "generate" the secondary output files from the
primary one, and that's not right either.  Depending on the details of
the creating process, the primary file might end up timestamped later than
one or more secondary files, causing subsequent make runs to consider the
secondary file(s) out of date.  That's harmless in a plain build, since
make will just re-execute the empty rule and nothing happens.  But it's
fatal in a VPATH build, since make will expect the secondary file to be
rebuilt in the build directory.  This would manifest as "file not found"
failures during VPATH builds from tarballs, if we were ever unlucky enough
to ship a tarball with apparently out-of-date secondary files.  (It's not
clear whether that has ever actually happened, but it definitely could.)

To ensure that secondary output files have timestamps >= their primary's,
change our makefile convention to be that we provide a "touch $@" action
not an empty rule.  Also, make sure that this rule actually gets invoked
during a distprep run, else the hazard remains.

It's been like this a long time, so back-patch to all supported branches.

In HEAD, I skipped the changes in src/backend/catalog/Makefile, because
those rules are due to get replaced soon in the bootstrap data format
patch, and there seems no need to create a merge issue for that patch.
If for some reason we fail to land that patch in v11, we'll need to
back-fill the changes in that one makefile from v10.

Discussion: https://postgr.es/m/18556.1521668179@sss.pgh.pa.us
2018-03-23 13:45:38 -04:00
Tom Lane
db35bf507f Fix tuple counting in SP-GiST index build.
Count the number of tuples in the index honestly, instead of assuming
that it's the same as the number of tuples in the heap.  (It might be
different if the index is partial.)

Back-patch to all supported versions.

Tomas Vondra

Discussion: https://postgr.es/m/3b3d8eac-c709-0d25-088e-b98339a1b28a@2ndquadrant.com
2018-03-22 13:23:48 -04:00