users/gsingh/postgres.git
5 years agoRemove duplicated progress reporting during heap scan of VACUUM
Michael Paquier [Sun, 15 Dec 2019 13:05:33 +0000 (22:05 +0900)]
Remove duplicated progress reporting during heap scan of VACUUM

This has been introduced by c16dc1a since progress reporting for VACUUM
has been added.  As this issue just causes some extra work and is
harmless, no backpatch is done.

Author: Justin Pryzby
Discussion: https://postgr.es/m/20191213030831.GT2082@telsasoft.com

5 years agoTry to stabilize results of new tuplesort regression test.
Tom Lane [Sat, 14 Dec 2019 20:01:56 +0000 (15:01 -0500)]
Try to stabilize results of new tuplesort regression test.

It appears that a concurrent autovacuum/autoanalyze run can cause
changes in the plans expected by this test.  To prevent that, change
the tables it uses to be temp tables --- there's no need for them
to be permanent, and this should save a few cycles too.

Discussion: https://postgr.es/m/3244.1576160824@sss.pgh.pa.us

5 years agoPrevent overly-aggressive collapsing of joins to RTE_RESULT relations.
Tom Lane [Sat, 14 Dec 2019 18:49:15 +0000 (13:49 -0500)]
Prevent overly-aggressive collapsing of joins to RTE_RESULT relations.

The RTE_RESULT simplification logic added by commit 4be058fe9 had a
flaw: it would collapse out a RTE_RESULT that is due to compute a
PlaceHolderVar, and reassign the PHV to the parent join level, even if
another input relation of the join contained a lateral reference to
the PHV.  That can't work because the PHV would be computed too late.
In practice it led to failures of internal sanity checks later in
planning (either assertion failures or errors such as "failed to
construct the join relation").

To fix, add code to check for the presence of such PHVs in relevant
portions of the query tree.  Notably, this required refactoring
range_table_walker so that a caller could ask to walk individual RTEs
not the whole list.  (It might be a good idea to refactor
range_table_mutator in the same way, if only to keep those functions
looking similar; but I didn't do so here as it wasn't necessary for
the bug fix.)

This exercise also taught me that find_dependent_phvs(), as it stood,
could only safely be used on the entire Query, not on subtrees.
Adjust its API to reflect that; which in passing allows it to have
a fast path for the common case of no PHVs anywhere.

Per report from Will Leinweber.  Back-patch to v12 where the bug
was introduced.

Discussion: https://postgr.es/m/CALLb-4xJMd4GZt2YCecMC95H-PafuWNKcmps4HLRx2NHNBfB4g@mail.gmail.com

5 years agoFix memory leak when initializing DH parameters in backend
Michael Paquier [Sat, 14 Dec 2019 09:17:31 +0000 (18:17 +0900)]
Fix memory leak when initializing DH parameters in backend

When loading DH parameters used for the generation of ephemeral DH keys
in the backend, the code has never bothered releasing the memory used
for the DH information loaded from a file or from libpq's default.  This
commit makes sure that the information is properly free()'d.

Note that as SSL parameters can be reloaded, this can cause an accumulation
of memory leaked.  As the leak is minor, no backpatch is done.

Reported-by: Dmitry Uspenskiy
Discussion: https://postgr.es/m/16160-18367e56e9a28264@postgresql.org

5 years agoFix mdsyncfiletag(), take II.
Thomas Munro [Sat, 14 Dec 2019 04:38:09 +0000 (17:38 +1300)]
Fix mdsyncfiletag(), take II.

The previous commit failed to consider that FileGetRawDesc() might
not return a valid fd, as discovered on the build farm.  Switch to
using the File interface only.

Back-patch to 12, like the previous commit.

5 years agoDon't use _mdfd_getseg() in mdsyncfiletag().
Thomas Munro [Sat, 14 Dec 2019 02:54:31 +0000 (15:54 +1300)]
Don't use _mdfd_getseg() in mdsyncfiletag().

_mdfd_getseg() opens all segments up to the requested one.  That
causes problems for mdsyncfiletag(), if mdunlinkfork() has
already unlinked other segment files.  Open the file we want
directly by name instead, if it's not already open.

The consequence of this bug was a rare panic in the checkpointer,
made more likely if you saturated the sync request queue so that
the SYNC_FORGET_REQUEST messages for a given relation were more
likely to be absorbed in separate cycles by the checkpointer.

Back-patch to 12.  Defect in commit 3eb77eba.

Author: Thomas Munro
Reported-by: Justin Pryzby
Discussion: https://postgr.es/m/20191119115759.GI30362%40telsasoft.com

5 years agoFix crash when a page was split during GiST index creation.
Heikki Linnakangas [Fri, 13 Dec 2019 21:58:10 +0000 (23:58 +0200)]
Fix crash when a page was split during GiST index creation.

The bug was similar to the one that was fixed in commit 22251686f0. When
we split page X and insert the downlink for the new page, the parent page
might also need to be split. When that happens, the downlink offset number
we remembered for X is no longer valid. We correctly called
gistFindCorrectParent() to re-find it, but gistFindCorrectParent() doesn't
do anything if the LSN of the page hasn't changed, and we stopped updating
LSNs during index build in commit 9155580fd5. The buggy codepath was taken
if the page was split into three or more pages, and inserting the downlink
caused the parent page to split. To fix, explicitly mark the downlink
offset number as invalid, to force gistFindCorrectParent() to re-find it.

Fixes bug #16134 reported by Alexander Lakhin, reported again as #16162 by
Andreas Kunert. Thanks to Jeff Janes, Tom Lane and Tomas Vondra for
debugging. Backpatch to v12, where we stopped WAL-logging during index
build.

Discussion: https://www.postgresql.org/message-id/16134-0423f729671dec64%40postgresql.org
Discussion: https://www.postgresql.org/message-id/16162-45d21b7b6c1a3105%40postgresql.org

5 years agoModernize our readline API a tad.
Tom Lane [Fri, 13 Dec 2019 16:16:33 +0000 (11:16 -0500)]
Modernize our readline API a tad.

Prefer to call "rl_filename_completion_function" and
"rl_completion_matches", rather than using the names without the rl_
prefix.  This matches Readline's documentation, and makes our code
a little clearer about which names are external.  On platforms that
only have the un-prefixed names (just some very ancient versions of
libedit, AFAICT), reverse the direction of the compatibility macro
definitions to match.

Also, remove our extern declaration of "filename_completion_function";
whatever libedit versions may have failed to declare that are surely
dead and buried.

Discussion: https://postgr.es/m/23608.1576248145@sss.pgh.pa.us

5 years agoPut back regression test case in a more robust form.
Tom Lane [Thu, 12 Dec 2019 18:49:54 +0000 (13:49 -0500)]
Put back regression test case in a more robust form.

This undoes my hurried commit 776a2c887, restoring the removed test case
in a form that passes with or without force_parallel_mode = regress.

It turns out that force_parallel_mode = regress simply fails to mask
the Worker lines that will be produced by EXPLAIN (ANALYZE, VERBOSE).
I'd say that's a bug in that feature, as its entire alleged reason
for existence is to make the EXPLAIN output the same.  It's certainly
not a bug in the plan node pruning logic.  Fortunately, this test case
doesn't really need to use ANALYZE, so just drop that.

Discussion: https://postgr.es/m/18891.1576109690@sss.pgh.pa.us

5 years agoFix EXTRACT(ISOYEAR FROM timestamp) for years BC.
Tom Lane [Thu, 12 Dec 2019 17:30:43 +0000 (12:30 -0500)]
Fix EXTRACT(ISOYEAR FROM timestamp) for years BC.

The test cases added by commit 26ae3aa80 exposed an old oversight in
timestamp[tz]_part: they didn't correct the result of date2isoyear()
for BC years, so that we produced an off-by-one answer for such years.
Fix that, and back-patch to all supported branches.

Discussion: https://postgr.es/m/SG2PR06MB37762CAE45DB0F6CA7001EA9B6550@SG2PR06MB3776.apcprd06.prod.outlook.com

5 years agoRemove redundant function calls in timestamp[tz]_part().
Tom Lane [Thu, 12 Dec 2019 17:12:35 +0000 (12:12 -0500)]
Remove redundant function calls in timestamp[tz]_part().

The DTK_DOW/DTK_ISODOW and DTK_DOY switch cases in timestamp_part() and
timestamptz_part() contained calls of timestamp2tm() that were fully
redundant with the ones done just above the switch.  This evidently crept
in during commit 258ee1b63, which relocated that code from another place
where the calls were indeed needed.  Just delete the redundant calls.

I (tgl) noted that our test coverage of these functions left quite a
bit to be desired, so extend timestamp.sql and timestamptz.sql to
cover all the branches.

Back-patch to all supported branches, as the previous commit was.
There's no real issue here other than some wasted cycles in some
not-too-heavily-used code paths, but the test coverage seems valuable.

Report and patch by Li Japin; test case adjustments by me.

Discussion: https://postgr.es/m/SG2PR06MB37762CAE45DB0F6CA7001EA9B6550@SG2PR06MB3776.apcprd06.prod.outlook.com

5 years ago(Blindly) tweak new test regex
Alvaro Herrera [Thu, 12 Dec 2019 16:45:15 +0000 (13:45 -0300)]
(Blindly) tweak new test regex

gcc-based Windows buildfarm animals are not happy about a multiline
regular expression I added recently.  Try to accomodate; existing
pg_basebackup tests suggest that \n should work instead of a bare
newline, but throw in \r also.  This being perl, TIMTOWTDI.
Also remove the pointless $ at the end of the pattern, for extra luck.

(If this doesn't work, I'll probably just split the regex in two.)

Per buildfarm members jacana and fairywren.

Discussion: https://postgr.es/m/3562.1576161217@sss.pgh.pa.us

5 years agoRemove extra parenthesis from comment.
Etsuro Fujita [Thu, 12 Dec 2019 06:45:00 +0000 (15:45 +0900)]
Remove extra parenthesis from comment.

5 years agoAdd readfuncs.c support for AppendRelInfo.
Tom Lane [Thu, 12 Dec 2019 00:08:16 +0000 (19:08 -0500)]
Add readfuncs.c support for AppendRelInfo.

This is made necessary by the fact that commit 6ef77cf46 added
AppendRelInfos to plan trees.  I'd concluded that this extra code was
not necessary because we don't transmit that data to parallel workers
... but I forgot about -DWRITE_READ_PARSE_PLAN_TREES.  Per buildfarm.

5 years agoRemove unstable test case added in commit 5935917ce.
Tom Lane [Wed, 11 Dec 2019 23:53:53 +0000 (18:53 -0500)]
Remove unstable test case added in commit 5935917ce.

The buildfarm says this produces some unexpected output with
force_parallel_mode = regress.  There's probably a bug underneath
that, but for the moment just delete the test case to make the
buildfarm green again.

(I now notice that the case had also failed to get updated to follow
commit d52eaa094, which made plan_cache_mode = force_generic_plan
prevail throughout partition_prune.sql; it was thereby managing to
break a later test.  When/if we put this back in, *don't* include the
SET and RESET commands.)

5 years agoAllow executor startup pruning to prune all child nodes.
Tom Lane [Wed, 11 Dec 2019 22:05:30 +0000 (17:05 -0500)]
Allow executor startup pruning to prune all child nodes.

Previously, if the startup pruning logic proved that all child nodes
of an Append or MergeAppend could be pruned, we still kept one, just
to keep EXPLAIN from failing.  The previous commit removed the
ruleutils.c limitation that required this kluge, so drop it.  That
results in less-confusing EXPLAIN output, as per a complaint from
Yuzuko Hosoya.

David Rowley

Discussion: https://postgr.es/m/001001d4f44b$2a2cca50$7e865ef0$@lab.ntt.co.jp

5 years agoFurther adjust EXPLAIN's choices of table alias names.
Tom Lane [Wed, 11 Dec 2019 22:05:18 +0000 (17:05 -0500)]
Further adjust EXPLAIN's choices of table alias names.

This patch causes EXPLAIN to always assign a separate table alias to the
parent RTE of an append relation (inheritance set); before, such RTEs
were ignored if not actually scanned by the plan.  Since the child RTEs
now always have that same alias to start with (cf. commit 55a1954da),
the net effect is that the parent RTE usually gets the alias used or
implied by the query text, and the children all get that alias with "_N"
appended.  (The exception to "usually" is if there are duplicate aliases
in different subtrees of the original query; then some of those original
RTEs will also have "_N" appended.)

This results in more uniform output for partitioned-table plans than
we had before: the partitioned table itself gets the original alias,
and all child tables have aliases with "_N", rather than the previous
behavior where one of the children would get an alias without "_N".

The reason for giving the parent RTE an alias, even if it isn't scanned
by the plan, is that we now use the parent's alias to qualify Vars that
refer to an appendrel output column and appear above the Append or
MergeAppend that computes the appendrel.  But below the append, Vars
refer to some one of the child relations, and are displayed that way.
This seems clearer than the old behavior where a Var that could carry
values from any child relation was displayed as if it referred to only
one of them.

While at it, change ruleutils.c so that the code paths used by EXPLAIN
deal in Plan trees not PlanState trees.  This effectively reverts a
decision made in commit 1cc29fe7c, which seemed like a good idea at
the time to make ruleutils.c consistent with explain.c.  However,
it's problematic because we'd really like to allow executor startup
pruning to remove all the children of an append node when possible,
leaving no child PlanState to resolve Vars against.  (That's not done
here, but will be in the next patch.)  This requires different handling
of subplans and initplans than before, but is otherwise a pretty
straightforward change.

Discussion: https://postgr.es/m/001001d4f44b$2a2cca50$7e865ef0$@lab.ntt.co.jp

5 years agoEmit parameter values during query bind/execute errors
Alvaro Herrera [Wed, 11 Dec 2019 21:03:35 +0000 (18:03 -0300)]
Emit parameter values during query bind/execute errors

This makes such log entries more useful, since the cause of the error
can be dependent on the parameter values.

Author: Alexey Bashtanov, Álvaro Herrera
Discussion: https://postgr.es/m/0146a67b-a22a-0519-9082-bc29756b93a2@imap.cc
Reviewed-by: Peter Eisentraut, Andres Freund, Tom Lane
5 years agoUse only one thread to handle incoming signals on Windows.
Tom Lane [Wed, 11 Dec 2019 20:09:54 +0000 (15:09 -0500)]
Use only one thread to handle incoming signals on Windows.

Since its inception, our Windows signal emulation code has worked by
running a main signal thread that just watches for incoming signal
requests, and then spawns a new thread to handle each such request.
That design is meant for servers in which requests can take substantial
effort to process, and it's worth parallelizing the handling of
requests.  But those assumptions are just bogus for our signal code.
It's not much more than pg_queue_signal(), which is cheap and can't
parallelize at all, plus we don't really expect lots of signals to
arrive at the same backend at once.  More importantly, this approach
creates failure modes that we could do without: either inability to
spawn a new thread or inability to create a new pipe handle will risk
loss of signals.  Hence, dispense with the separate per-signal threads
and just service each request in-line in the main signal thread.  This
should be a bit faster (for the normal case of one signal at a time)
as well as more robust.

Patch by me; thanks to Andrew Dunstan for testing and Amit Kapila
for review.

Discussion: https://postgr.es/m/4412.1575748586@sss.pgh.pa.us

5 years agoRemove ATPrepSetStatistics
Peter Eisentraut [Wed, 11 Dec 2019 07:59:18 +0000 (08:59 +0100)]
Remove ATPrepSetStatistics

It was once possible to do ALTER TABLE ... SET STATISTICS on system
tables without allow_sytem_table_mods.  This was changed apparently by
accident between PostgreSQL 9.1 and 9.2, but a code comment still
claimed this was possible.  Without that functionality, having a
separate ATPrepSetStatistics() is useless, so use the generic
ATSimplePermissions() instead and move the remaining custom code into
ATExecSetStatistics().

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/cc8d2648-a0ec-7a86-13e5-db473484e19e%402ndquadrant.com

5 years agoFix output of Unicode normalization test
Peter Eisentraut [Wed, 11 Dec 2019 07:42:17 +0000 (08:42 +0100)]
Fix output of Unicode normalization test

Several off-by-more-than-one errors caused the output in case of a
test failure to be truncated and unintelligible.

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/6a7a8516-7d11-8fbd-0e8b-eadb4f0679eb%402ndquadrant.com

5 years agoFix some compiler warnings with timestamp parsing in formatting.c
Michael Paquier [Wed, 11 Dec 2019 01:01:06 +0000 (10:01 +0900)]
Fix some compiler warnings with timestamp parsing in formatting.c

gcc-7 used with a sufficient optimization level complains about warnings
around do_to_timestamp() regarding the initialization and handling of
some of its variables.  Recent commits 66c74f8 and d589f94 made things
made the interface more confusing, so document which variables are
always expected and initialize properly the optional ones when they are
set.

Author: Andrey Lepikhov, Michael Paquier
Discussion: https://postgr.es/m/a7e28b83-27b1-4e1c-c76b-4268c4b785bc@postgrespro.ru

5 years agoFix tuple column count in pg_control_init().
Tom Lane [Tue, 10 Dec 2019 22:51:46 +0000 (17:51 -0500)]
Fix tuple column count in pg_control_init().

Oversight in commit 2e4db241b.

Nathan Bossart

Discussion: https://postgr.es/m/1B616360-396A-4482-AA28-375566C86160@amazon.com

5 years agoCosmetic cleaning of pg_config.h.win32
Peter Eisentraut [Tue, 10 Dec 2019 20:15:30 +0000 (21:15 +0100)]
Cosmetic cleaning of pg_config.h.win32

Clean up some comments (some generated by old versions of autoconf)
and some random ordering differences, so it's easier to diff this
against the default pg_config.h or pg_config.h.in.  Remove LOCALEDIR
handling from pg_config.h.win32 altogether because it's already in
pg_config_paths.h.

5 years agoAdd backend-only appendStringInfoStringQuoted
Alvaro Herrera [Tue, 10 Dec 2019 20:09:32 +0000 (17:09 -0300)]
Add backend-only appendStringInfoStringQuoted

This provides a mechanism to emit literal values in informative
messages, such as query parameters.  The new code is more complex than
what it replaces, primarily because it wants to be more efficient.
It also has the (currently unused) additional optional capability of
specifying a maximum size to print.

The new function lives out of common/stringinfo.c so that frontend users
of that file need not pull in unnecessary multibyte-encoding support
code.

Author: Álvaro Herrera and Alexey Bashtanov, after a suggestion from Andres Freund
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20190920203905.xkv5udsd5dxfs6tr@alap3.anarazel.de

5 years agoIn pg_ctl, work around ERROR_SHARING_VIOLATION on the postmaster log file.
Tom Lane [Tue, 10 Dec 2019 18:17:08 +0000 (13:17 -0500)]
In pg_ctl, work around ERROR_SHARING_VIOLATION on the postmaster log file.

On Windows, we use CMD.EXE to redirect the postmaster's stdout/stderr
into a log file.  CMD.EXE will open that file with non-sharing-friendly
parameters, and the file will remain open for a short time after the
postmaster has removed postmaster.pid.  This can result in an
ERROR_SHARING_VIOLATION failure if we attempt to start a new postmaster
immediately with the same log file (e.g. during "pg_ctl restart").
This seems to explain intermittent buildfarm failures we've been seeing
on Windows machines.

To fix, just open and close the log file using our own pgwin32_open(),
which will wait if necessary to avoid the failure.  (Perhaps someday
we should stop using CMD.EXE, but that would be a far more complex
patch, and it doesn't seem worth the trouble ... yet.)

Back-patch to v12.  This only solves the problem when frontend fopen()
is redirected to pgwin32_fopen(), which has only been true since commit
0ba06e0bf.  Hence, no point in back-patching further, unless we care
to back-patch that change too.

Diagnosis and patch by Alexander Lakhin (bug #16154).

Discussion: https://postgr.es/m/16154-1ccf0b537b24d5e0@postgresql.org

5 years agoFix handling of multiple AFTER ROW triggers on a foreign table.
Etsuro Fujita [Tue, 10 Dec 2019 09:00:30 +0000 (18:00 +0900)]
Fix handling of multiple AFTER ROW triggers on a foreign table.

AfterTriggerExecute() retrieves a fresh tuple or pair of tuples from a
tuplestore and then stores the tuple(s) in the passed-in slot(s) if
AFTER_TRIGGER_FDW_FETCH, while it uses the most-recently-retrieved
tuple(s) stored in the slot(s) if AFTER_TRIGGER_FDW_REUSE.  This was
done correctly before 12, but commit ff11e7f4b broke it by mistakenly
clearing the tuple(s) stored in the slot(s) in that function, leading to
an assertion failure as reported in bug #16139 from Alexander Lakhin.

Also, fix some other issues with the aforementioned commit in passing:

* For tg_newslot, which is a slot added to the TriggerData struct by the
  commit to store new updated tuples, it didn't ensure the slot was NULL
  if there was no such tuple.
* The commit failed to update the documentation about the trigger
  interface.

Author: Etsuro Fujita
Backpatch-through: 12
Discussion: https://postgr.es/m/16139-94f9ccf0db6119ec%40postgresql.org

5 years agoFix race condition in our Windows signal emulation.
Tom Lane [Mon, 9 Dec 2019 20:03:51 +0000 (15:03 -0500)]
Fix race condition in our Windows signal emulation.

pg_signal_dispatch_thread() responded to the client (signal sender)
and disconnected the pipe before actually setting the shared variables
that make the signal visible to the backend process's main thread.
In the worst case, it seems, effective delivery of the signal could be
postponed for as long as the machine has any other work to do.

To fix, just move the pg_queue_signal() call so that we do it before
responding to the client.  This essentially makes pgkill() synchronous,
which is a stronger guarantee than we have on Unix.  That may be
overkill, but on the other hand we have not seen comparable timing bugs
on any Unix platform.

While at it, add some comments to this sadly underdocumented code.

Problem diagnosis and fix by Amit Kapila; I just added the comments.

Back-patch to all supported versions, as it appears that this can cause
visible NOTIFY timing oddities on all of them, and there might be
other misbehavior due to slow delivery of other signals.

Discussion: https://postgr.es/m/32745.1575303812@sss.pgh.pa.us

5 years agoImprove isolationtester's timeout management.
Tom Lane [Mon, 9 Dec 2019 19:31:57 +0000 (14:31 -0500)]
Improve isolationtester's timeout management.

isolationtester.c had a hard-wired limit of 3 minutes per test step.
It now emerges that this isn't quite enough for some of the slowest
buildfarm animals.  This isn't the first time we've had to raise
this limit (cf. 1db439ad4), so let's make it configurable.  This
patch raises the default to 5 minutes, and introduces an environment
variable PGISOLATIONTIMEOUT that can be set if more time is needed,
following the precedent of PGCTLTIMEOUT.

Also, modify isolationtester so that when the timeout is hit,
it explicitly reports having sent a cancel.  This makes the regression
failure log considerably more intelligible.  (In the worst case, a
timed-out test might actually be reported as "passing" without this
extra output, so arguably this is a bug fix in itself.)

In passing, update the README file, which had apparently not gotten
touched when we added "make check" support here.

Back-patch to 9.6; older versions don't have comparable timeout logic.

Discussion: https://postgr.es/m/22964.1575842935@sss.pgh.pa.us

5 years agoFix typos in miscinit.c.
Amit Kapila [Mon, 9 Dec 2019 03:09:34 +0000 (08:39 +0530)]
Fix typos in miscinit.c.

Commit f13ea95f9e moved the description of postmaster.pid file contents
from miscadmin.h to pidfile.h, but missed to update the comments in
miscinit.c.

Author: Hadi Moshayedi
Reviewed-by: Amit Kapila
Backpatch-through: 10
Discussion: https://postgr.es/m/CAK=1=WpYEM9x3LGkaxgXaxeYQjnkdW8XLsxrYRTE2Gq-H83FMw@mail.gmail.com

5 years agoDocument search_path security with untrusted dbowner or CREATEROLE.
Noah Misch [Sun, 8 Dec 2019 19:06:26 +0000 (11:06 -0800)]
Document search_path security with untrusted dbowner or CREATEROLE.

Commit 5770172cb0c9df9e6ce27c507b449557e5b45124 wrote, incorrectly, that
certain schema usage patterns are secure against CREATEROLE users and
database owners.  When an untrusted user is the database owner or holds
CREATEROLE privilege, a query is secure only if its session started with
SELECT pg_catalog.set_config('search_path', '', false) or equivalent.
Back-patch to 9.4 (all supported versions).

Discussion: https://postgr.es/m/20191013013512.GC4131753@rfd.leadboat.com

5 years agoDoc: improve documentation about run-time pruning's effects on EXPLAIN.
Tom Lane [Sun, 8 Dec 2019 15:36:29 +0000 (10:36 -0500)]
Doc: improve documentation about run-time pruning's effects on EXPLAIN.

Tatsuo Ishii complained that this para wasn't very intelligible.
Try to make it better.

Discussion: https://postgr.es/m/20191207.200500.989741087350666720.t-ishii@sraoss.co.jp

5 years agoRemove PQsslpassword function
Andrew Dunstan [Sat, 7 Dec 2019 14:20:53 +0000 (09:20 -0500)]
Remove PQsslpassword function

This partially reverts commit 4dc6355210.

The information returned by the function can be obtained by calling
PQconninfo(), so the function is redundant.

5 years agoImprove test coverage of ruleutils.c.
Tom Lane [Fri, 6 Dec 2019 22:40:24 +0000 (17:40 -0500)]
Improve test coverage of ruleutils.c.

While fooling around with the EXPLAIN improvements I've been working
on, I noticed that there were some large gaps in our test coverage
of ruleutils.c, according to the code coverage report.  This commit
just adds a few test cases to improve coverage of:
get_name_for_var_field()
get_update_query_targetlist_def()
isSimpleNode()
get_sublink_expr()

5 years agoFix comments in execGrouping.c
Jeff Davis [Fri, 6 Dec 2019 19:47:59 +0000 (11:47 -0800)]
Fix comments in execGrouping.c

Commit 5dfc1981 missed updating some comments.

Also, fix a comment typo found in passing.

Author: Jeff Davis
Discussion: https://postgr.es/m/9723131d247b919f94699152647fa87ee0bc02c2.camel%40j-davis.com

5 years agoDisallow non-default collation in ADD PRIMARY KEY/UNIQUE USING INDEX.
Tom Lane [Fri, 6 Dec 2019 16:25:09 +0000 (11:25 -0500)]
Disallow non-default collation in ADD PRIMARY KEY/UNIQUE USING INDEX.

When creating a uniqueness constraint using a pre-existing index,
we have always required that the index have the same properties you'd
get if you just let a new index get built.  However, when collations
were added, we forgot to add the index's collation to that check.

It's hard to trip over this without intentionally trying to break it:
you'd have to explicitly specify a different collation in CREATE
INDEX, then convert it to a pkey or unique constraint.  Still, if you
did that, pg_dump would emit a script that fails to reproduce the
index's collation.  The main practical problem is that after a
pg_upgrade the index would be corrupt, because its actual physical
order wouldn't match what pg_index says.  A more theoretical issue,
which is new as of v12, is that if you create the index with a
nondeterministic collation then it wouldn't be enforcing the normal
notion of uniqueness, causing the constraint to mean something
different from a normally-created constraint.

To fix, just add collation to the conditions checked for index
acceptability in ADD PRIMARY KEY/UNIQUE USING INDEX.  We won't try
to clean up after anybody who's already created such a situation;
it seems improbable enough to not be worth the effort involved.
(If you do get into trouble, a REINDEX should be enough to fix it.)

In principle this is a long-standing bug, but I chose not to
back-patch --- the odds of causing trouble seem about as great
as the odds of preventing it, and both risks are very low anyway.

Per report from Alexey Bashtanov, though this is not his preferred
fix.

Discussion: https://postgr.es/m/b05ce36a-cefb-ca5e-b386-a400535b1c0b@imap.cc

5 years agoFix handling of OpenSSL's SSL_clear_options
Michael Paquier [Fri, 6 Dec 2019 06:13:55 +0000 (15:13 +0900)]
Fix handling of OpenSSL's SSL_clear_options

This function is supported down to OpenSSL 0.9.8, which is the oldest
version supported since 593d4e4 (from Postgres 10 onwards), and is used
since e3bdb2d (from 11 onwards).  It is defined as a macro from OpenSSL
0.9.8 to 1.0.2, and as a function in 1.1.0 and newer versions.  However,
the configure check present is only adapted for functions.  So, even if
the code would be able to compile, configure fails to detect the macro,
causing it to be ignored when compiling the code with OpenSSL from 0.9.8
to 1.0.2.

The code needs a configure check as per a364dfa, which has fixed a
compilation issue with a past version of LibreSSL in NetBSD 5.1.  On
HEAD, just remove the configure check as the last release of NetBSD 5 is
from 2014 (and we have no more buildfarm members for it).  In 11 and 12,
improve the configure logic so as both macros and functions are
correctly detected.  This makes NetBSD 5 still work on already-released
branches, but not for 13 onwards.

The patch for HEAD is from me, and Daniel has written the version to use
for the back-branches.

Author: Michael Paquier, Daniel Gustaffson
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
Discussion: https://postgr.es/m/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se
Backpatch-through: 11

5 years agoImprove some comments in pg_upgrade.c
Michael Paquier [Fri, 6 Dec 2019 02:55:04 +0000 (11:55 +0900)]
Improve some comments in pg_upgrade.c

When restoring database schemas on a new cluster, database "template1"
is processed first, followed by all other databases in parallel,
including "postgres".  Both "postgres" and "template1" have some extra
handling to propagate each one's properties, but comments were confusing
regarding which one is processed where.

Author: Julien Rouhaud
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/CAOBaU_a2iviTG7FE10yO_gcW+zQCHNFhRA_NDiktf3UR65BHdw@mail.gmail.com

5 years agoRemove configure check for OpenSSL's SSL_get_current_compression()
Michael Paquier [Fri, 6 Dec 2019 00:41:32 +0000 (09:41 +0900)]
Remove configure check for OpenSSL's SSL_get_current_compression()

This function has been added in OpenSSL 0.9.8, which is the oldest
version supported on HEAD, so checking for it at configure time is
useless.  Both the frontend and backend code did not even bother to use
it.

Reported-by: Daniel Gustafsson
Author: Michael Paquier
Reviewed-by: Daniel Gustafsson, Tom Lane
Discussion: https://postgr.es/m/20191205083252.GE5064@paquier.xyz
Discussion: https://postgr.es/m/98F7F99E-1129-41D8-B86B-FE3B1E286881@yesql.se

5 years agopg_basebackup: Refactor code for reading COPY and tar data.
Robert Haas [Thu, 5 Dec 2019 20:14:09 +0000 (15:14 -0500)]
pg_basebackup: Refactor code for reading COPY and tar data.

Add a new function ReceiveCopyData that does just that, taking a
callback as an argument to specify what should be done with each chunk
as it is received. This allows a single copy of the logic to be shared
between ReceiveTarFile and ReceiveAndUnpackTarFile, and eliminates
a few #ifdef conditions based on HAVE_LIBZ.

While this is slightly more code, it's arguably clearer, and
there is a pending patch that introduces additional calls to
ReceiveCopyData.

This commit is not intended to result in any functional change.

Discussion: http://postgr.es/m/CA+TgmoYZDTHbSpwZtW=JDgAhwVAYvmdSrRUjOd+AYdfNNXVBDg@mail.gmail.com

5 years agoMinor comment improvements for instrumentation.h
Robert Haas [Thu, 5 Dec 2019 12:53:12 +0000 (07:53 -0500)]
Minor comment improvements for instrumentation.h

Remove a duplicated word. Add "of" or "# of" in a couple places
for clarity and consistency. Start comments with a lower case
letter as we do elsewhere in this file.

Rafia Sabih

5 years agoBlind attempt at fixing ecpg/compatlib's build
Alvaro Herrera [Wed, 4 Dec 2019 23:15:11 +0000 (20:15 -0300)]
Blind attempt at fixing ecpg/compatlib's build

It now needs libpgcommon in order to get pnstrdup.

Per buildfarm.

5 years agoOffer pnstrdup to frontend code
Alvaro Herrera [Wed, 4 Dec 2019 22:36:06 +0000 (19:36 -0300)]
Offer pnstrdup to frontend code

We already had it on the backend.  Frontend can also use it now.

Discussion: https://postgr.es/m/20191204144021.GA17976@alvherre.pgsql

5 years agoUpdate minimum SSL version
Peter Eisentraut [Wed, 4 Dec 2019 20:40:17 +0000 (21:40 +0100)]
Update minimum SSL version

Change default of ssl_min_protocol_version to TLSv1.2 (from TLSv1,
which means 1.0).  Older versions are still supported, just not by
default.

TLS 1.0 is widely deprecated, and TLS 1.1 only slightly less so.  All
OpenSSL versions that support TLS 1.1 also support TLS 1.2, so there
would be very little reason to, say, set the default to TLS 1.1
instead on grounds of better compatibility.

The test suite overrides this new setting, so it can still run with
older OpenSSL versions.

Discussion: https://www.postgresql.org/message-id/flat/b327f8df-da98-054d-0cc5-b76a857cfed9%402ndquadrant.com

5 years agoFix whitespace.
Etsuro Fujita [Wed, 4 Dec 2019 03:45:00 +0000 (12:45 +0900)]
Fix whitespace.

5 years agoUse carriage returns for data insertion logs in pgbench on terminal
Michael Paquier [Wed, 4 Dec 2019 02:33:14 +0000 (11:33 +0900)]
Use carriage returns for data insertion logs in pgbench on terminal

This is similar to what pg_basebackup and pg_rewind do when reporting
cumulative data, and that's more user-friendly.  Carriage returns are
now used when stderr points to a terminal, and newlines are used in
other cases, like a redirection to a log file.

Author: Amit Langote
Reviewed-by: Fabien Coelho
Discussion: https://postgr.es/m/CA+HiwqFNwEjPeVaQsp2L7DyCPv1Eg1guwhrVhzMYqUJUk8ULKg@mail.gmail.com

5 years agoRemove unnecessary definition of CancelRequested in bin/scripts/
Michael Paquier [Wed, 4 Dec 2019 01:06:45 +0000 (10:06 +0900)]
Remove unnecessary definition of CancelRequested in bin/scripts/

This variable is now part of the refactored code for query cancellation
in fe_utils.  This fixes an oversight in commit a4fd3aa.  While on it,
improve some header includes in bin/scripts/.

Author: Michael Paquier
Reviewed-by: Fabien Coelho
Discussion: https://postgr.es/m/20191203101625.GF1634@paquier.xyz

5 years agoEnsure maxlen is at leat 1 in dict_int
Tomas Vondra [Tue, 3 Dec 2019 15:55:51 +0000 (16:55 +0100)]
Ensure maxlen is at leat 1 in dict_int

The dict_int text search dictionary template accepts maxlen parameter,
which is then used to cap the length of input strings. The value was
not properly checked, and the code simply does

    txt[d->maxlen] = '\0';

to insert a terminator, leading to segfaults with negative values.

This commit simply rejects values less than 1. The issue was there since
dct_int was introduced in 9.3, so backpatch all the way back to 9.4
which is the oldest supported version.

Reported-by: cili
Discussion: https://postgr.es/m/16144-a36a5bef7657047d@postgresql.org
Backpatch-through: 9.4

5 years agoFurther sync postgres_fdw's "Relations" output with the rest of EXPLAIN.
Tom Lane [Tue, 3 Dec 2019 17:25:56 +0000 (12:25 -0500)]
Further sync postgres_fdw's "Relations" output with the rest of EXPLAIN.

EXPLAIN generally only adds schema qualifications to table names when
VERBOSE is specified.  In postgres_fdw's "Relations" output, table
names were always so qualified, but that was an implementation
restriction: in the original coding, we didn't have access to the
verbose flag at the time the string was generated.  After the code
rearrangement of commit 4526951d5, we do have that info available
at the right time, so make this output follow the normal rule.

Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us

5 years agoFix thinkos from commit 9989d37
Michael Paquier [Tue, 3 Dec 2019 09:59:09 +0000 (18:59 +0900)]
Fix thinkos from commit 9989d37

Error messages referring to incorrect WAL segment names could have been
generated for a fsync() failure or when creating a new segment at the
end of recovery.

5 years agoFix alter_system_table test
Peter Eisentraut [Tue, 3 Dec 2019 08:14:35 +0000 (09:14 +0100)]
Fix alter_system_table test

Add workaround for disabling ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
warning for the test that tries to create a tablespace with a reserved
name.

Discussion: https://www.postgresql.org/message-id/flat/E1iacW7-0003h6-6U%40gemulon.postgresql.org

5 years agoRemove XLogFileNameP() from the tree
Michael Paquier [Tue, 3 Dec 2019 06:06:04 +0000 (15:06 +0900)]
Remove XLogFileNameP() from the tree

XLogFileNameP() is a wrapper routine able to build a palloc'd string for
a WAL segment name, which is used for error string generation.  There
were several code paths where it gets called in a critical section,
where memory allocation is not allowed.  This results in triggering
an assertion failure instead of generating the wanted error message.

Another, more annoying, problem is that if the allocation to generate
the WAL segment name fails on OOM, then the failure would be escalated
to a PANIC.

This removes the routine and all its callers are replaced with a logic
using a fixed-size buffer.  This way, all the existing mistakes are
fixed and future ones are prevented.

Author: Masahiko Sawada
Reviewed-by: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/CA+fd4k5gC9H4uoWMLg9K_QfNrnkkdEw+-AFveob9YX7z8JnKTA@mail.gmail.com

5 years agoFix failures with TAP tests of pg_ctl on Windows
Michael Paquier [Tue, 3 Dec 2019 04:01:06 +0000 (13:01 +0900)]
Fix failures with TAP tests of pg_ctl on Windows

On Windows, all the hosts spawned by the TAP tests bind to 127.0.0.1.
Hence, if there is a port conflict, starting a cluster would immediately
fail.  One of the test scripts of pg_ctl initializes a node without
PostgresNode.pm, using the default port 5432.  This could cause
unexpected startup failures in the tests if an independent server was up
and running on the same host (the reverse is also possible, though more
unlikely).  Fix this issue by assigning properly a free port to the node
configured, in the same range used as for the other nodes part of the
tests.

Author: Michael Paquier
Reviewed-by: Andrew Dunstan
Discussion: https://postgr.es/m/20191202031444.GC1696@paquier.xyz
Backpatch-through: 11

5 years agoFix EXPLAIN's column alias output for mismatched child tables.
Tom Lane [Tue, 3 Dec 2019 00:08:10 +0000 (19:08 -0500)]
Fix EXPLAIN's column alias output for mismatched child tables.

If an inheritance/partitioning parent table is assigned some column
alias names in the query, EXPLAIN mapped those aliases onto the
child tables' columns by physical position, resulting in bogus output
if a child table's columns aren't one-for-one with the parent's.

To fix, make expand_single_inheritance_child() generate a correctly
re-mapped column alias list, rather than just copying the parent
RTE's alias node.  (We have to fill the alias field, not just
adjust the eref field, because ruleutils.c will ignore eref in
favor of looking at the real column names.)

This means that child tables will now always have alias fields in
plan rtables, where before they might not have.  That results in
a rather substantial set of regression test output changes:
EXPLAIN will now always show child tables with aliases that match
the parent table (usually with "_N" appended for uniqueness).
But that seems like a net positive for understandability, since
the parent alias corresponds to something that actually appeared
in the original query, while the child table names didn't.
(Note that this does not change anything for cases where an explicit
table alias was written in the query for the parent table; it
just makes cases without such aliases behave similarly to that.)
Hence, while we could avoid these subsidiary changes if we made
inherit.c more complicated, we choose not to.

Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us

5 years agoAdd a reverse-translation column number array to struct AppendRelInfo.
Tom Lane [Mon, 2 Dec 2019 23:05:29 +0000 (18:05 -0500)]
Add a reverse-translation column number array to struct AppendRelInfo.

This provides for cheaper mapping of child columns back to parent
columns.  The one existing use-case in examine_simple_variable()
would hardly justify this by itself; but an upcoming bug fix will
make use of this array in a mainstream code path, and it seems
likely that we'll find other uses for it as we continue to build
out the partitioning infrastructure.

Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us

5 years agoMake postgres_fdw's "Relations" output agree with the rest of EXPLAIN.
Tom Lane [Mon, 2 Dec 2019 21:31:03 +0000 (16:31 -0500)]
Make postgres_fdw's "Relations" output agree with the rest of EXPLAIN.

The relation aliases shown in the "Relations" line for a foreign scan
didn't always agree with those used in the rest of EXPLAIN's output.
The regression test result changes appearing here provide examples.

It's really impossible for postgres_fdw to duplicate EXPLAIN's alias
assignment logic during postgresGetForeignRelSize(), because of the
de-duplication that EXPLAIN does on a global basis --- and anyway,
trying to duplicate that would be unmaintainable.  Instead, just put
numeric rangetable indexes into the string, and convert those to
table names/aliases in postgresExplainForeignScan, which does have
access to the results of ruleutils.c's alias assignment logic.
Aside from being more reliable, this shifts some work from planning
to EXPLAIN, which is a good tradeoff for performance.  (I also
changed from using StringInfo to using psprintf, which makes the
code slightly simpler and reduces its memory consumption.)

A kluge required by this solution is that we have to reverse-engineer
the rtoffset applied by setrefs.c.  If that logic ever fails
(presumably because the member tables of a join got offset by
different amounts), we'll need some more cooperation with setrefs.c
to keep things straight.  But for now, there's no need for that.

Arguably this is a back-patchable bug fix, but since this is a mostly
cosmetic issue and there have been no field complaints, I'll refrain
for now.

Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us

5 years agoAdd query cancellation capabilities in pgbench init phase
Michael Paquier [Mon, 2 Dec 2019 02:42:28 +0000 (11:42 +0900)]
Add query cancellation capabilities in pgbench init phase

This can be useful to stop data generation happening on the server for
long-running queries caused by large scale factors.  This cannot happen
by default as data is generated on the client, but it is possible to
control the initialization steps of pgbench to do that.

Reported-by: Fujii Masao
Author: Fabien Coelho
Discussion: https://postgr.es/m/alpine.DEB.2.21.1910311939430.27369@lancre
Discussion: https://postgr.es/m/CAHGQGwHWEyTXxZh46qgFY8a2bDF_EYeUdp3+_Hy=qLZSzwVPKg@mail.gmail.com

5 years agoRefactor query cancellation code into src/fe_utils/
Michael Paquier [Mon, 2 Dec 2019 02:18:56 +0000 (11:18 +0900)]
Refactor query cancellation code into src/fe_utils/

Originally, this code was duplicated in src/bin/psql/ and
src/bin/scripts/, but it can be useful for other frontend applications,
like pgbench.  This refactoring offers the possibility to setup a custom
callback which would get called in the signal handler for SIGINT or when
the interruption console events happen on Windows.

Author: Fabien Coelho, with contributions from Michael Paquier
Reviewed-by: Álvaro Herrera, Ibrar Ahmed
Discussion: https://postgr.es/m/alpine.DEB.2.21.1910311939430.27369@lancre

5 years agoAdd dummy versions of new SSL functions for non-SSL builds
Andrew Dunstan [Sun, 1 Dec 2019 22:49:43 +0000 (17:49 -0500)]
Add dummy versions of new SSL functions for non-SSL builds

This rectifies an oversight in commit 4dc6355210, which caused certain
builds to fail, especially on Windows.

5 years agoFix misbehavior with expression indexes on ON COMMIT DELETE ROWS tables.
Tom Lane [Sun, 1 Dec 2019 18:09:26 +0000 (13:09 -0500)]
Fix misbehavior with expression indexes on ON COMMIT DELETE ROWS tables.

We implement ON COMMIT DELETE ROWS by truncating tables marked that
way, which requires also truncating/rebuilding their indexes.  But
RelationTruncateIndexes asks the relcache for up-to-date copies of any
index expressions, which may cause execution of eval_const_expressions
on them, which can result in actual execution of subexpressions.
This is a bad thing to have happening during ON COMMIT.  Manuel Rigger
reported that use of a SQL function resulted in crashes due to
expectations that ActiveSnapshot would be set, which it isn't.
The most obvious fix perhaps would be to push a snapshot during
PreCommit_on_commit_actions, but I think that would just open the door
to more problems: CommitTransaction explicitly expects that no
user-defined code can be running at this point.

Fortunately, since we know that no tuples exist to be indexed, there
seems no need to use the real index expressions or predicates during
RelationTruncateIndexes.  We can set up dummy index expressions
instead (we do need something that will expose the right data type,
as there are places that build index tupdescs based on this), and
just ignore predicates and exclusion constraints.

In a green field it'd likely be better to reimplement ON COMMIT DELETE
ROWS using the same "init fork" infrastructure used for unlogged
relations.  That seems impractical without catalog changes though,
and even without that it'd be too big a change to back-patch.
So for now do it like this.

Per private report from Manuel Rigger.  This has been broken forever,
so back-patch to all supported branches.

5 years agolibq support for sslpassword connection param, DER format keys
Andrew Dunstan [Sat, 30 Nov 2019 20:27:13 +0000 (15:27 -0500)]
libq support for sslpassword connection param,  DER format keys

This patch providies for support for password protected SSL client
keys in libpq, and for DER format keys, both encrypted and unencrypted.
There is a new connection parameter sslpassword, which is supplied to
the OpenSSL libraries via a callback function. The callback function can
also be set by an application by calling PQgetSSLKeyPassHook(). There is
also a function to retreive the connection setting, PQsslpassword().

Craig Ringer and Andrew Dunstan

Reviewed by: Greg Nancarrow

Discussion: https://postgr.es/m/f7ee88ed-95c4-95c1-d4bf-7b415363ab62@2ndQuadrant.com

5 years agoFix off-by-one error in PGTYPEStimestamp_fmt_asc
Tomas Vondra [Sat, 30 Nov 2019 13:51:27 +0000 (14:51 +0100)]
Fix off-by-one error in PGTYPEStimestamp_fmt_asc

When using %b or %B patterns to format a date, the code was simply using
tm_mon as an index into array of month names. But that is wrong, because
tm_mon is 1-based, while array indexes are 0-based. The result is we
either use name of the next month, or a segfault (for December).

Fix by subtracting 1 from tm_mon for both patterns, and add a regression
test triggering the issue. Backpatch to all supported versions (the bug
is there far longer, since at least 2003).

Reported-by: Paul Spencer
Backpatch-through: 9.4
Discussion: https://postgr.es/m/16143-0d861eb8688d3fef%40postgresql.org

5 years agoRevert commits 290acac92b and 8a7e9e9dad.
Amit Kapila [Sat, 30 Nov 2019 02:13:42 +0000 (07:43 +0530)]
Revert commits 290acac92b and 8a7e9e9dad.

This commit revert the commits to add a test case that tests the 'force'
option when there is an active backend connected to the database being
dropped.

This feature internally sends SIGTERM to all the backends connected to the
database being dropped and then the same is reported to the client.  We
found that on Windows, the client end of the socket is not able to read
the data once we close the socket in the server which leads to loss of
error message which is not what we expect.  We also observed  similar
behavior in other cases like pg_terminate_backend(),
pg_ctl kill TERM <pid>.  There are probably a few others like that.  The
fix for this requires further study.

Discussion: https://postgr.es/m/E1iaD8h-0004us-K9@gemulon.postgresql.org

5 years agoSmall code simplification
Peter Eisentraut [Fri, 29 Nov 2019 09:55:31 +0000 (10:55 +0100)]
Small code simplification

FLOAT8PASSBYVAL can be used instead of USE_FLOAT8_BYVAL here.

5 years agoAdd a regression test for allow_system_table_mods
Peter Eisentraut [Fri, 29 Nov 2019 09:04:45 +0000 (10:04 +0100)]
Add a regression test for allow_system_table_mods

Add a regression test file that exercises the kinds of commands that
allow_system_table_mods allows.

This is put in the "unsafe_tests" suite, so it won't accidentally
create a mess if someone runs the normal regression tests against an
instance that they care about.

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com

5 years agoMake allow_system_table_mods settable at run time
Peter Eisentraut [Fri, 29 Nov 2019 09:04:45 +0000 (10:04 +0100)]
Make allow_system_table_mods settable at run time

Make allow_system_table_mods settable at run time by superusers.  It
was previously postmaster start only.

We don't want to make system catalog DDL wide-open, but there are
occasionally useful things to do like setting reloptions or statistics
on a busy system table, and blocking those doesn't help anyone.  Also,
this enables the possibility of writing a test suite for this setting.

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com

5 years agoRemove any-user DML capability from allow_system_table_mods
Peter Eisentraut [Fri, 29 Nov 2019 09:04:45 +0000 (10:04 +0100)]
Remove any-user DML capability from allow_system_table_mods

Previously, allow_system_table_mods allowed a non-superuser to do DML
on a system table without further permission checks.  This has been
removed, as it was quite inconsistent with the rest of the meaning of
this setting.  (Since allow_system_table_mods was previously only
accessible with a server restart, it is unlikely that anyone was using
this possibility.)

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/8b00ea5e-28a7-88ba-e848-21528b632354%402ndquadrant.com

5 years agoAdd error position to an error message
Peter Eisentraut [Fri, 29 Nov 2019 08:10:17 +0000 (09:10 +0100)]
Add error position to an error message

Reviewed-by: Pavel Stehule <pavel.stehule@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/6e7aa4a1-be6a-1a75-b1f9-83a678e5184a@2ndquadrant.com

5 years agoUse memcpy instead of a byte loop in pglz_decompress
Tomas Vondra [Thu, 28 Nov 2019 22:29:30 +0000 (23:29 +0100)]
Use memcpy instead of a byte loop in pglz_decompress

The byte loop used in pglz_decompress() because of possible overlap may
be quite inefficient, so this commit replaces it with memcpy. The gains
do depend on the data (compressibility) and hardware, but seem to be
quite significant.

Author: Andrey Borodin
Reviewed-by: Michael Paquier, Konstantin Knizhnik, Tels
Discussion: https://postgr.es/m/469C9ED9-348C-4FE7-A7A7-B0FA671BEE4C@yandex-team.ru

5 years agoRemove unnecessary clauses_attnums variable
Tomas Vondra [Thu, 28 Nov 2019 22:25:14 +0000 (23:25 +0100)]
Remove unnecessary clauses_attnums variable

Commit c676e659b2 reworked how choose_best_statistics() picks the best
extended statistics, but failed to remove clauses_attnums which is now
unnecessary. So get rid of it and backpatch to 12, same as c676e659b2.

Author: Tomas Vondra
Discussion: https://postgr.es/m/CA+u7OA7H5rcE2=8f263w4NZD6ipO_XOrYB816nuLXbmSTH9pQQ@mail.gmail.com
Backpatch-through: 12

5 years agoFix choose_best_statistics to check clauses individually
Tomas Vondra [Thu, 28 Nov 2019 21:20:28 +0000 (22:20 +0100)]
Fix choose_best_statistics to check clauses individually

When picking the best extended statistics object for a list of clauses,
it's not enough to look at attnums extracted from the clause list as a
whole. Consider for example this query with OR clauses:

   SELECT * FROM t WHERE (t.a = 1) OR (t.b = 1) OR (t.c = 1)

with a statistics defined on columns (a,b). Relying on attnums extracted
from the whole OR clause, we'd consider the statistics usable. That does
not work, as we see the conditions as a single OR-clause, referencing an
attribute not covered by the statistic, leading to empty list of clauses
to be estimated using the statistics and an assert failure.

This changes choose_best_statistics to check which clauses are actually
covered, and only using attributes from the fully covered ones. For the
previous example this means the statistics object will not be considered
as compatible with the OR-clause.

Backpatch to 12, where MCVs were introduced. The issue does not affect
older versions because functional dependencies don't handle OR clauses.

Author: Tomas Vondra
Reviewed-by: Dean Rasheed
Reported-By: Manuel Rigger
Discussion: https://postgr.es/m/CA+u7OA7H5rcE2=8f263w4NZD6ipO_XOrYB816nuLXbmSTH9pQQ@mail.gmail.com
Backpatch-through: 12

5 years agoRemove useless "return;" lines
Alvaro Herrera [Thu, 28 Nov 2019 19:48:37 +0000 (16:48 -0300)]
Remove useless "return;" lines

Discussion: https://postgr.es/m/20191128144653.GA27883@alvherre.pgsql

5 years agoAdd tests for '-f' option in dropdb utility.
Amit Kapila [Thu, 28 Nov 2019 06:16:57 +0000 (11:46 +0530)]
Add tests for '-f' option in dropdb utility.

This will test that the force option works when there is an active backend
connected to the database being dropped.

Author: Pavel Stehule and Vignesh C
Reviewed-by: Amit Kapila and Vignesh C
Discussion: https://postgr.es/m/CAP_rwwmLJJbn70vLOZFpxGw3XD7nLB_7+NKz46H5EOO2k5H7OQ@mail.gmail.com

5 years agoMove pump_until to TestLib.pm.
Amit Kapila [Thu, 28 Nov 2019 02:58:17 +0000 (08:28 +0530)]
Move pump_until to TestLib.pm.

The subroutine pump_until provides the functionality to poll until the
given string is matched, or a timeout occurs.  This can be used from other
places as well, so moving it to TestLib.pm.  The immediate need is for an
upcoming regression test patch for dropdb utility.

Author: Vignesh C
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CAP_rwwmLJJbn70vLOZFpxGw3XD7nLB_7+NKz46H5EOO2k5H7OQ@mail.gmail.com

5 years agopg_upgrade: adjust error paragraph width to be consistent
Bruce Momjian [Thu, 28 Nov 2019 01:36:33 +0000 (20:36 -0500)]
pg_upgrade:  adjust error paragraph width to be consistent

Previously these error paragraphs were too wide.

Backpatch-through: 13

5 years agopg_upgrade: improve instructions for fixing incompatible isn use
Bruce Momjian [Thu, 28 Nov 2019 01:24:57 +0000 (20:24 -0500)]
pg_upgrade: improve instructions for fixing incompatible isn use

This clarifies instructions on how to dump/reload databases that use
incompatible isn versions.

Reported-by: Alvaro Herrera
Discussion: https://postgr.es/m/20191114190652.GA23586@alvherre.pgsql

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Backpatch-through: 13

5 years agoDon't use native methods in TestLib::slurp_file on Msys
Andrew Dunstan [Wed, 27 Nov 2019 20:45:44 +0000 (15:45 -0500)]
Don't use native methods in TestLib::slurp_file on Msys

Commit 114541d58e has upset some buildfarm members running Msys, that
weren't previously having problems, so the use of native Windows methods
to open files is restricted by this patch to only MSVC builds.

5 years agoRevert "Close stdin where it's not needed in TestLib.pm procedures"
Andrew Dunstan [Wed, 27 Nov 2019 12:19:22 +0000 (07:19 -0500)]
Revert "Close stdin where it's not needed in TestLib.pm procedures"

This reverts commits 9af34f3c6b and 792dba73c8.

The code has been found not to be portable.

Discussion: https://postgr.es/m/2658c496-f885-02db-13bb-228423782524@2ndQuadrant.com

5 years agoMove configure --disable-float8-byval to pg_config_manual.h
Peter Eisentraut [Wed, 27 Nov 2019 10:21:02 +0000 (11:21 +0100)]
Move configure --disable-float8-byval to pg_config_manual.h

This build option was once useful to maintain compatibility with
version-0 functions, but those are no longer supported, so this option
is no longer useful for end users.  We keep the option available to
developers in pg_config_manual.h so that it is easy to test the
pass-by-reference code paths without having to fire up a 32-bit
machine.

Discussion: https://www.postgresql.org/message-id/flat/f3e1e576-2749-bbd7-2d57-3f9dcf75255a@2ndquadrant.com

5 years agoFix typo in comment.
Etsuro Fujita [Wed, 27 Nov 2019 07:00:45 +0000 (16:00 +0900)]
Fix typo in comment.

5 years agoFix closing stdin in TestLib.pm
Andrew Dunstan [Tue, 26 Nov 2019 21:23:00 +0000 (16:23 -0500)]
Fix closing stdin in TestLib.pm

Commit 9af34f3c6b naively assumed that all non-windows platforms would
have pseudoterminals and that perl would have IO::Pty. Such is not the
case. Instead of assumung anything, test for the presence of IO::Pty and
only use code that might depend on it if it's present. The test result is
exposed in $TestLib::have_io_pty so that it can be conveniently used in
SKIP tests.

Discussion: https://postgr.es/m/20191126044110.GB5435@paquier.xyz

5 years agoAllow access to child table statistics if user can read parent table.
Tom Lane [Tue, 26 Nov 2019 19:41:48 +0000 (14:41 -0500)]
Allow access to child table statistics if user can read parent table.

The fix for CVE-2017-7484 disallowed use of pg_statistic data for
planning purposes if the user would not be able to select the associated
column and a non-leakproof function is to be applied to the statistics
values.  That turns out to disable use of pg_statistic data in some
common cases involving inheritance/partitioning, where the user does
have permission to select from the parent table that was actually named
in the query, but not from a child table whose stats are needed.  Since,
in non-corner cases, the user *can* select the child table's data via
the parent, this restriction is not actually useful from a security
standpoint.  Improve the logic so that we also check the permissions of
the originally-named table, and allow access if select permission exists
for that.

When checking access to stats for a simple child column, we can map
the child column number back to the parent, and perform this test
exactly (including not allowing access if the child column isn't
exposed by the parent).  For expression indexes, the current logic
just insists on whole-table select access, and this patch allows
access if the user can select the whole parent table.  In principle,
if the child table has extra columns, this might allow access to
stats on columns the user can't read.  In practice, it's unlikely
that the planner is going to do any stats calculations involving
expressions that are not visible to the query, so we'll ignore that
fine point for now.  Perhaps someday we'll improve that logic to
detect exactly which columns are used by an expression index ...
but today is not that day.

Back-patch to v11.  The issue was created in 9.2 and up by the
CVE-2017-7484 fix, but this patch depends on the append_rel_array[]
planner data structure which only exists in v11 and up.  In
practice the issue is most urgent with partitioned tables, so
fixing v11 and later should satisfy much of the practical need.

Dilip Kumar and Amit Langote, with some kibitzing by me

Discussion: https://postgr.es/m/3876.1531261875@sss.pgh.pa.us

5 years agoAdd safeguards for pg_fsync() called with incorrectly-opened fds
Michael Paquier [Tue, 26 Nov 2019 04:32:52 +0000 (13:32 +0900)]
Add safeguards for pg_fsync() called with incorrectly-opened fds

On some platforms, fsync() returns EBADFD when opening a file descriptor
with O_RDONLY (read-only), leading ultimately now to a PANIC to prevent
data corruption.

This commit adds a new sanity check in pg_fsync() based on fcntl() to
make sure that we don't repeat again mistakes with incorrectly-set file
descriptors so as problems are detected at an early stage.  Without
that, such errors could only be detected after running Postgres on a
specific supported platform for the culprit code path, which could take
some time before being found.  b8e19b93 was a fix for such a problem,
which got undetected for more than 5 years, and a586cc4b fixed another
similar issue.

Note that the new check added works as well when fsync=off is
configured, so as all regression tests would detect problems as long as
assertions are enabled.  fcntl() being not available on Windows, the
new checks do not happen there.

Author: Michael Paquier
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/20191009062640.GB21379@paquier.xyz

5 years agoDon't shut down Gather[Merge] early under Limit.
Amit Kapila [Mon, 18 Nov 2019 08:47:41 +0000 (14:17 +0530)]
Don't shut down Gather[Merge] early under Limit.

Revert part of commit 19df1702f5.

Early shutdown was added by that commit so that we could collect
statistics from workers, but unfortunately, it interacted badly with
rescans.  The problem is that we ended up destroying the parallel context
which is required for rescans.  This leads to rescans of a Limit node over
a Gather node to produce unpredictable results as it tries to access
destroyed parallel context.  By reverting the early shutdown code, we
might lose statistics in some cases of Limit over Gather [Merge], but that
will require further study to fix.

Reported-by: Jerry Sievers
Diagnosed-by: Thomas Munro
Author: Amit Kapila, testcase by Vignesh C
Backpatch-through: 9.6
Discussion: https://postgr.es/m/87ims2amh6.fsf@jsievers.enova.com

5 years agoUse procsignal_sigusr1_handler for auxiliary processes.
Robert Haas [Mon, 25 Nov 2019 21:08:53 +0000 (16:08 -0500)]
Use procsignal_sigusr1_handler for auxiliary processes.

AuxiliaryProcessMain does ProcSignalInit, so one might expect that
auxiliary processes would need to respond to SendProcSignal, but none
of the auxiliary processes do that. Change them to use
procsignal_sigusr1_handler instead of their own private handlers so
that they do. Besides seeming more correct, this is also less code. It
shouldn't make any functional difference right now because, as far as
we know, there are no current cases where SendProcSignal targets an
auxiliary process, but there are plans to change that in the future.

Andres Freund

Discussion: http://postgr.es/m/20181030051643.elbxjww5jjgnjaxg@alap3.anarazel.de

5 years agoClose stdin where it's not needed in TestLib.pm procedures
Andrew Dunstan [Mon, 25 Nov 2019 20:51:51 +0000 (15:51 -0500)]
Close stdin where it's not needed in TestLib.pm procedures

Where possible, do this using a pseudoterminal, so that things like
openssl that want to open /dev/tty if stdin isn't a tty won't.
Elsewhere, i.e. Windows, just close by providing an empty string using
the standard IPC::Run pipe mechanism.

Patch by Andrew Dunstan, based on an idea from Craig Ringer.

Reviewed by Mark Dilger.

Discussion: https://postgr.es/m/873ebb57-fc98-340d-949d-691b1810bf66@2ndQuadrant.com

5 years agoRefactor WAL file-reading code into WALRead()
Alvaro Herrera [Mon, 25 Nov 2019 18:04:54 +0000 (15:04 -0300)]
Refactor WAL file-reading code into WALRead()

XLogReader, walsender and pg_waldump all had their own routines to read
data from WAL files to memory, with slightly different approaches
according to the particular conditions of each environment.  There's a
lot of commonality, so we can refactor that into a single routine
WALRead in XLogReader, and move the differences to a separate (simpler)
callback that just opens the next WAL-segment.  This results in a
clearer (ahem) code flow.

The error reporting needs are covered by filling in a new error-info
struct, WALReadError, and it's the caller's responsibility to act on it.
The backend has WALReadRaiseError() to do so.

We no longer ever need to seek in this interface; switch to using
pg_pread().

Author: Antonin Houska, with contributions from Álvaro Herrera
Reviewed-by: Michaël Paquier, Kyotaro Horiguchi
Discussion: https://postgr.es/m/14984.1554998742@spoje.net

5 years agoFix unportable printf format introduced in commit 9290ad198.
Tom Lane [Mon, 25 Nov 2019 15:48:36 +0000 (10:48 -0500)]
Fix unportable printf format introduced in commit 9290ad198.

"%ld" is not an acceptable format spec for int64 variables, though
it accidentally works on most non-Windows 64-bit platforms.  Follow
the lead of commit 6a1cd8b92, and use "%lld" with an explicit cast
to long long.  Per buildfarm.

5 years agoMake the order of the header file includes consistent.
Amit Kapila [Mon, 25 Nov 2019 02:38:57 +0000 (08:08 +0530)]
Make the order of the header file includes consistent.

Similar to commits 14aec035027e735035f2 and dddf4cdc33, this commit
makes the order of header file inclusion consistent in more places.

Author: Vignesh C
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CALDaNm2Sznv8RR6Ex-iJO6xAdsxgWhCoETkaYX=+9DW3q0QCfA@mail.gmail.com

5 years agoFix inconsistent variable name in static function of mac8.c
Michael Paquier [Mon, 25 Nov 2019 00:57:35 +0000 (09:57 +0900)]
Fix inconsistent variable name in static function of mac8.c

Both argument names were reversed in the declaration of the function.

Author: Ranier Vilela
Discussion: https://postgr.es/m/MN2PR18MB292755AEFF9A9144B220ABEEE34B0@MN2PR18MB2927.namprd18.prod.outlook.com

5 years agoRefactor reloption handling for index AMs in-core
Michael Paquier [Mon, 25 Nov 2019 00:40:53 +0000 (09:40 +0900)]
Refactor reloption handling for index AMs in-core

This reworks the reloption parsing and build of a couple of index AMs by
creating new structures for each index AM's options.  This split was
already done for BRIN, GIN and GiST (which actually has a fillfactor
parameter), but not for hash, B-tree and SPGiST which relied on
StdRdOptions due to an overlap with the default option set.

This saves a couple of bytes for rd_options in each relcache entry with
indexes making use of relation options, and brings more consistency
between all index AMs.  While on it, add a couple of AssertMacro() calls
to make sure that utility macros to grab values of reloptions are used
with the expected index AM.

Author: Nikolay Shaplov
Reviewed-by: Amit Langote, Michael Paquier, Álvaro Herrera, Dent John
Discussion: https://postgr.es/m/4127670.gFlpRb6XCm@x200m

5 years agoUse native methods to open input in TestLib::slurp_file on Windows.
Andrew Dunstan [Sun, 24 Nov 2019 23:25:22 +0000 (18:25 -0500)]
Use native methods to open input in TestLib::slurp_file on Windows.

It is hoped that this will avoid some errors that we have seen before,
but lately with greater frequency, in buildfarm animals.

For now apply only to master. If this proves effective it can be
backpatched.

Discussion: https://postgr.es/m/13900.1572839580@sss.pgh.pa.us

Author: Juan José Santamaría Flecha

5 years agoDoc: improve discussion of race conditions involved in LISTEN.
Tom Lane [Sun, 24 Nov 2019 23:03:39 +0000 (18:03 -0500)]
Doc: improve discussion of race conditions involved in LISTEN.

The user docs didn't really explain how to use LISTEN safely,
so clarify that.  Also clean up some fuzzy-headed explanations
in comments.  No code changes.

Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com

5 years agoAvoid assertion failure with LISTEN in a serializable transaction.
Tom Lane [Sun, 24 Nov 2019 20:57:31 +0000 (15:57 -0500)]
Avoid assertion failure with LISTEN in a serializable transaction.

If LISTEN is the only action in a serializable-mode transaction,
and the session was not previously listening, and the notify queue
is not empty, predicate.c reported an assertion failure.  That
happened because we'd acquire the transaction's initial snapshot
during PreCommit_Notify, which was called *after* predicate.c
expects any such snapshot to have been established.

To fix, just swap the order of the PreCommit_Notify and
PreCommit_CheckForSerializationFailure calls during CommitTransaction.
This will imply holding the notify-insertion lock slightly longer,
but the difference could only be meaningful in serializable mode,
which is an expensive option anyway.

It appears that this is just an assertion failure, with no
consequences in non-assert builds.  A snapshot used only to scan
the notify queue could not have been involved in any serialization
conflicts, so there would be nothing for
PreCommit_CheckForSerializationFailure to do except assign it a
prepareSeqNo and set the SXACT_FLAG_PREPARED flag.  And given no
conflicts, neither of those omissions affect the behavior of
ReleasePredicateLocks.  This admittedly once-over-lightly analysis
is backed up by the lack of field reports of trouble.

Per report from Mark Dilger.  The bug is old, so back-patch to all
supported branches; but the new test case only goes back to 9.6,
for lack of adequate isolationtester infrastructure before that.

Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com
Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us

5 years agodoc: Fix whitespace in syntax.
Thomas Munro [Sun, 24 Nov 2019 20:20:28 +0000 (09:20 +1300)]
doc: Fix whitespace in syntax.

Back-patch to 10.

Author: Andreas Karlsson
Discussion: https://postgr.es/m/043acae2-a369-b7fa-be48-1933aa2e82d1%40proxel.se

5 years agoStabilize NOTIFY behavior by transmitting notifies before ReadyForQuery.
Tom Lane [Sun, 24 Nov 2019 19:42:59 +0000 (14:42 -0500)]
Stabilize NOTIFY behavior by transmitting notifies before ReadyForQuery.

This patch ensures that, if any notify messages were received during
a just-finished transaction, they get sent to the frontend just before
not just after the ReadyForQuery message.  With libpq and other client
libraries that act similarly, this guarantees that the client will see
the notify messages as available as soon as it thinks the transaction
is done.

This probably makes no difference in practice, since in realistic
use-cases the application would have to cope with asynchronous
arrival of notify events anyhow.  However, it makes it a lot easier
to build cross-session-notify test cases with stable behavior.
I'm a bit surprised now that we've not seen any buildfarm instability
with the test cases added by commit b10f40bf0.  Tests that I intend
to add in an upcoming bug fix are definitely unstable without this.

Back-patch to 9.6, which is as far back as we can do NOTIFY testing
with the isolationtester infrastructure.

Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us

5 years agoStabilize the results of pg_notification_queue_usage().
Tom Lane [Sun, 24 Nov 2019 19:09:33 +0000 (14:09 -0500)]
Stabilize the results of pg_notification_queue_usage().

This function wasn't touched in commit 51004c717, but that turns out
to be a bad idea, because its results now include any dead space
that exists in the NOTIFY queue on account of our being lazy about
advancing the queue tail.  Notably, the isolation tests now fail
if run twice without a server restart between, because async-notify's
first test of the function will already show a positive value.
It seems likely that end users would be equally unhappy about the
result's instability.  To fix, just make the function call
asyncQueueAdvanceTail before computing its result.  That should end
in producing the same value as before, and it's hard to believe that
there's any practical use-case where pg_notification_queue_usage()
is called so often as to create a performance degradation, especially
compared to what we did before.

Out of paranoia, also mark this function parallel-restricted (it
was volatile, but parallel-safe by default, before).  Although the
code seems to work fine when run in a parallel worker, that's outside
the design scope of async.c, and it's a bit scary to have intentional
side-effects happening in a parallel worker.  There seems no plausible
use-case where it'd be important to try to parallelize this, so let's
not take any risk of introducing new bugs.

In passing, re-pgindent async.c and run reformat-dat-files on
pg_proc.dat, just because I'm a neatnik.

Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us

5 years agoRemove a couple of unnecessary if-tests.
Tom Lane [Sun, 24 Nov 2019 17:03:16 +0000 (12:03 -0500)]
Remove a couple of unnecessary if-tests.

Commit abd9ca377 replaced a couple of while-loops in fmtfloat()
with calls to dopr_outchmulti, but I (tgl) failed to notice that
the new if-tests guarding those calls were really unnecessary,
because they're inside a larger if-block checking the same thing.

Ranier Vilela

Discussion: https://postgr.es/m/MN2PR18MB2927850AB00CF39CC370D107E34B0@MN2PR18MB2927.namprd18.prod.outlook.com

5 years agoRemove debugging aid
Alvaro Herrera [Sat, 23 Nov 2019 16:19:20 +0000 (13:19 -0300)]
Remove debugging aid

This Assert(false) was not supposed to be in the committed copy.

Reported by: Tom Lane
Discussion: https://postgr.es/m/26476.1574525468@sss.pgh.pa.us

5 years agoUpdate sepgsql to add mandatory access control for TRUNCATE
Joe Conway [Sat, 23 Nov 2019 15:41:52 +0000 (10:41 -0500)]
Update sepgsql to add mandatory access control for TRUNCATE

Use SELinux "db_table: { truncate }" to check if permission is granted to
TRUNCATE. Update example SELinux policy to grant needed permission for
TRUNCATE. Add new regression test to demonstrate a positive and negative
cases. Test will only be run if the loaded SELinux policy has the
"db_table: { truncate }" permission. Makes use of recent commit which added
object TRUNCATE hook. Patch by Yuli Khodorkovskiy with minor
editorialization by me. Not back-patched because the object TRUNCATE hook
was not.

Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com