Tom Lane [Tue, 19 Mar 2019 16:49:27 +0000 (12:49 -0400)]
Make checkpoint requests more robust.
Commit 6f6a6d8b1 introduced a delay of up to 2 seconds if we're trying
to request a checkpoint but the checkpointer hasn't started yet (or,
much less likely, our kill() call fails). However buildfarm experience
shows that that's not quite enough for slow or heavily-loaded machines.
There's no good reason to assume that the checkpointer won't start
eventually, so we may as well make the timeout much longer, say 60 sec.
However, if the caller didn't say CHECKPOINT_WAIT, it seems like a bad
idea to be waiting at all, much less for as long as 60 sec. We can
remove the need for that, and make this whole thing more robust, by
adjusting the code so that the existence of a pending checkpoint
request is clear from the contents of shared memory, and making sure
that the checkpointer process will notice it at startup even if it did
not get a signal. In this way there's no need for a non-CHECKPOINT_WAIT
call to wait at all; if it can't send the signal, it can nonetheless
assume that the checkpointer will eventually service the request.
A potential downside of this change is that "kill -INT" on the checkpointer
process is no longer enough to trigger a checkpoint, should anyone be
relying on something so hacky. But there's no obvious reason to do it
like that rather than issuing a plain old CHECKPOINT command, so we'll
assume that nobody is. There doesn't seem to be a way to preserve this
undocumented quasi-feature without introducing race conditions.
Since a principal reason for messing with this is to prevent intermittent
buildfarm failures, back-patch to all supported branches.
Peter Eisentraut [Tue, 19 Mar 2019 09:48:03 +0000 (10:48 +0100)]
Ignore attempts to add TOAST table to shared or catalog tables
Running ALTER TABLE on any table will check if a TOAST table needs to be
added. On shared tables, this would previously fail, thus effectively
disabling ALTER TABLE for those tables. On (non-shared) system
catalogs, on the other hand, it would add a TOAST table, even though we
don't really want TOAST tables on some system catalogs. In some cases,
it would also fail with an error "AccessExclusiveLock required to add
toast table.", depending on what locks the ALTER TABLE actions had
already taken.
So instead, just ignore attempts to add TOAST tables to such tables,
outside of bootstrap mode, pretending they don't need one.
This allows running ALTER TABLE on such tables without messing up the
TOAST situation. Legitimate uses for ALTER TABLE on system catalogs
include setting reloptions (say, fillfactor or autovacuum settings).
(All this still requires allow_system_table_mods, which is independent
of this.)
Peter Eisentraut [Tue, 19 Mar 2019 08:37:46 +0000 (09:37 +0100)]
Fix bug in support for collation attributes on older ICU versions
Unrecognized attribute names are supposed to be ignored. But the code
would error out on an unrecognized attribute value even if it did not
recognize the attribute name. So unrecognized attributes wouldn't
really be ignored unless the value happened to be one that matched a
recognized value. This would break some important cases where the
attribute would be processed by ucol_open() directly. Fix that and
add a test case.
The restructured code should also avoid compiler warnings about
initializing a UColAttribute value to -1, because the type might be an
unsigned enum. (reported by Andres Freund)
Andrew Gierth [Tue, 19 Mar 2019 01:16:50 +0000 (01:16 +0000)]
Implement OR REPLACE option for CREATE AGGREGATE.
Aggregates have acquired a dozen or so optional attributes in recent
years for things like parallel query and moving-aggregate mode; the
lack of an OR REPLACE option to add or change these for an existing
agg makes extension upgrades gratuitously hard. Rectify.
Tom Lane [Mon, 18 Mar 2019 21:54:24 +0000 (17:54 -0400)]
Fix memory leak in printtup.c.
Commit f2dec34e1 changed things so that printtup's output stringinfo
buffer was allocated outside the per-row temporary context, not inside
it. This creates a need to free that buffer explicitly when the temp
context is freed, but that was overlooked. In most cases, this is all
happening inside a portal or executor context that will go away shortly
anyhow, but that's not always true. Notably, the stringinfo ends up
getting leaked when JDBC uses row-at-a-time fetches. For a query
that returns wide rows, that adds up after awhile.
Per bug #15700 from Matthias Otterbach. Back-patch to v11 where the
faulty code was added.
We should try to prewarm each database only once. Otherwise, if
prewarming fails for some reason, it will just keep retrying in an
infnite loop. This can happen if, for example, the database has been
dropped. The existing code was intended to implement the try-once
behavior, but failed to do so because it neglected to set
worker.bgw_restart_time to BGW_NEVER_RESTART.
Robert Haas [Mon, 18 Mar 2019 19:14:52 +0000 (15:14 -0400)]
Revise parse tree representation for VACUUM and ANALYZE.
Like commit f41551f61f9cf4eedd5b7173f985a3bdb4d9858c, this aims
to make it easier to add non-Boolean options to VACUUM (or, in
this case, to ANALYZE). Instead of building up a bitmap of
options directly in the parser, build up a list of DefElem
objects and let ExecVacuum() sort it out; right now, we make
no use of the fact that a DefElem can carry an associated value,
but it will be easy to make that change in the future.
Robert Haas [Mon, 18 Mar 2019 17:57:33 +0000 (13:57 -0400)]
Fold vacuum's 'int options' parameter into VacuumParams.
Many places need both, so this allows a few functions to take one
fewer parameter. More importantly, as soon as we add a VACUUM
option that takes a non-Boolean parameter, we need to replace
'int options' with a struct, and it seems better to think
of adding more fields to VacuumParams rather than passing around
both VacuumParams and a separate struct as well.
Peter Eisentraut [Mon, 18 Mar 2019 16:01:40 +0000 (17:01 +0100)]
Fix optimization of foreign-key on update actions
In RI_FKey_pk_upd_check_required(), we check among other things
whether the old and new key are equal, so that we don't need to run
cascade actions when nothing has actually changed. This was using the
equality operator. But the effect of this is that if a value in the
primary key is changed to one that "looks" different but compares as
equal, the update is not propagated. (Examples are float -0 and 0 and
case-insensitive text.) This appears to violate the SQL standard, and
it also behaves inconsistently if in a multicolumn key another key is
also updated that would cause the row to compare as not equal.
To fix, if we are looking at the PK table in ri_KeysEqual(), then do a
bytewise comparison similar to record_image_eq() instead of using the
equality operators. This only makes a difference for ON UPDATE
CASCADE, but for consistency we treat all changes to the PK the same. For
the FK table, we continue to use the equality operators.
Michael Paquier [Mon, 18 Mar 2019 03:59:35 +0000 (12:59 +0900)]
Refactor more code logic to update the control file
ce6afc6 has begun the refactoring work by plugging pg_rewind into a
central routine to update the control file, and left around two extra
copies, with one in xlog.c for the backend and one in pg_resetwal.c. By
adding an extra option to the central routine in controldata_utils.c to
control if a flush of the control file needs to be done, it is proving
to be straight-forward to make xlog.c and pg_resetwal.c use the central
code path at the condition of moving the wait event tracking there.
Hence, this allows to have only one central code path to update the
control file, shaving the code from the duplicates.
This refactoring actually fixes a problem in pg_resetwal. Previously,
the control file was first removed before being recreated. So if a
crash happened between the moment the file was removed and the moment
the file was created, then it would have been possible to not have a
control file anymore in the database folder.
Author: Fabien Coelho Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/alpine.DEB.2.21.1903170935210.2506@lancre
Michael Paquier [Mon, 18 Mar 2019 01:34:45 +0000 (10:34 +0900)]
Fix pg_rewind when rewinding new database with tables included
This fixes an issue introduced by 266b6ac, which has added filters to
exclude file patterns on the target and source data directories to
reduce the number of files transferred. Filters get applied to both
the target and source data files, and include pg_internal.init which is
present for each database once relations are created on it. However, if
the target differed from the source with at least one new database with
relations, the rewind would fail due to the exclusion filters applied on
the target files, causing pg_internal.init to still be present on the
target database folder, while its contents should have been completely
removed so as there is nothing remaining inside at the time of the
folder deletion.
Applying exclusion filters on the source files is fine, because this way
the amount of data copied from the source to the target is reduced. And
actually, not applying the filters on the target is what pg_rewind
should do, because this causes such files to be automatically removed
during the rewind on the target. Exclusion filters apply to paths which
are removed or recreated automatically at startup, so removing all those
files on the target during the rewind is a win.
The existing set of TAP tests already stresses the rewind of databases,
but it did not include any tables on those newly-created databases.
Creating extra tables in this case is enough to reproduce the failure,
so the existing tests are extended to close the gap.
Reported-by: Mithun Cy
Author: Michael Paquier
Discussion: https://postgr.es/m/CADq3xVYt6_pO7ZzmjOqPgY9HWsL=kLd-_tNyMtdfjKqEALDyTA@mail.gmail.com
Backpatch-through: 11
Michael Paquier [Mon, 18 Mar 2019 00:11:52 +0000 (09:11 +0900)]
Error out in pg_checksums on incompatible block size
pg_checksums is compiled with a given block size and has a hard
dependency to it per the way checksums are calculated via
checksum_impl.h, and trying to use the tool on a data folder which has
not the same block size would result in incorrect checksum calculations
and/or block read errors, meaning that the data folder is corrupted.
This is harmless as checksums are only checked now, but very confusing
for the user so issue an error properly if the block size used at
compilation and the block size used in the data folder do not match.
Reported-by: Sergei Kornilov
Author: Michael Banck, Michael Paquier Reviewed-by: Fabien Coelho, Magnus Hagander
Discussion: https://postgr.es/m/20190317054657.GA3357@paquier.xyz
ackpatch-through: 11
Peter Eisentraut [Sun, 17 Mar 2019 07:16:33 +0000 (08:16 +0100)]
Add support for collation attributes on older ICU versions
Starting in ICU 54, collation customization attributes can be
specified in the locale string, for example
"@colStrength=primary;colCaseLevel=yes". Add support for this for
older ICU versions as well, by adding some minimal parsing of the
attributes in the locale string and calling ucol_setAttribute() on
them. This is essentially what never ICU versions do internally in
ucol_open(). This was we can offer this functionality in a consistent
way in all ICU versions supported by PostgreSQL.
Also add some tests for ICU collation customization.
Reported-by: Daniel Verite <daniel@manitou-mail.org>
Discussion: https://www.postgresql.org/message-id/0270ebd4-f67c-8774-1a5a-91adfb9bb41f@2ndquadrant.com
Tom Lane [Sat, 16 Mar 2019 19:50:13 +0000 (15:50 -0400)]
Further adjust the tests for the hyperbolic functions.
It looks like we can leave in most of the test cases for Infinity/NaN
inputs, but buildfarm member jacana gets the wrong answer for acosh(Inf).
It's not worth carrying a variant expected file for that, so just disable
that one test.
Add support of numeric error suppression to jsonpath as it's required by
standard. This commit doesn't use PG_TRY()/PG_CATCH() in order to implement
that. Instead, it provides internal versions of numeric functions used, which
support error suppression.
Discussion: https://postgr.es/m/fcc6fc6a-b497-f39a-923d-aa34d0c588e8%402ndQuadrant.com
Author: Alexander Korotkov, Nikita Glukhov Reviewed-by: Tomas Vondra
SQL 2016 standards among other things contains set of SQL/JSON features for
JSON processing inside of relational database. The core of SQL/JSON is JSON
path language, allowing access parts of JSON documents and make computations
over them. This commit implements partial support JSON path language as
separate datatype called "jsonpath". The implementation is partial because
it's lacking datetime support and suppression of numeric errors. Missing
features will be added later by separate commits.
Support of SQL/JSON features requires implementation of separate nodes, and it
will be considered in subsequent patches. This commit includes following
set of plain functions, allowing to execute jsonpath over jsonb values:
This commit also implements "jsonb @? jsonpath" and "jsonb @@ jsonpath", which
are wrappers over jsonpath_exists(jsonb, jsonpath) and jsonpath_predicate(jsonb,
jsonpath) correspondingly. These operators will have an index support
(implemented in subsequent patches).
Catversion bumped, to add new functions and operators.
Code was written by Nikita Glukhov and Teodor Sigaev, revised by me.
Documentation was written by Oleg Bartunov and Liudmila Mantrova. The work
was inspired by Oleg Bartunov.
Discussion: https://postgr.es/m/fcc6fc6a-b497-f39a-923d-aa34d0c588e8%402ndQuadrant.com
Author: Nikita Glukhov, Teodor Sigaev, Alexander Korotkov, Oleg Bartunov, Liudmila Mantrova Reviewed-by: Tomas Vondra, Andrew Dunstan, Pavel Stehule, Alexander Korotkov
Peter Eisentraut [Fri, 15 Mar 2019 20:24:05 +0000 (21:24 +0100)]
Don't propagate PGAPPNAME through pg_ctl in tests
When libpq is loaded in the server (for instance, by
libpqwalreceiver), it may use libpq environment variables set in the
postmaster environment for connection parameter defaults. This has
some confusing effects in our test suites. For example, the TAP test
infrastructure sets PGAPPNAME to allow identifying clients in the
server log. But this environment variable is also inherited by
temporary servers started with pg_ctl and is then in turn used by
libpqwalreceiver as the application_name for connecting to remote
servers where it then shows up in pg_stat_replication and is relevant
for things like synchronous_standby_names. Replication already has a
suitable default for application_name, and overriding that
accidentally then requires the individual test cases to re-override
that, which is all very confusing and unnecessary.
To fix, unset PGAPPNAME temporarily before running pg_ctl start or
restart in the tests.
More comprehensive approaches like unsetting all environment variables
in pg_ctl were considered but might be too complicated to achieve
portably.
The now unnecessary re-overriding of application_name by test cases is
also removed.
Tom Lane [Fri, 15 Mar 2019 17:46:26 +0000 (13:46 -0400)]
Further reduce memory footprint of CLOBBER_CACHE_ALWAYS testing.
Some buildfarm members using CLOBBER_CACHE_ALWAYS have been having OOM
problems of late. Commit 2455ab488 addressed this problem by recovering
space transiently used within RelationBuildPartitionDesc, but it turns
out that leaves quite a lot on the table, because other subroutines of
RelationBuildDesc also leak memory like mad. Let's move the temp-context
management into RelationBuildDesc so that leakage from the other
subroutines is also recovered.
I examined this issue by arranging for postgres.c to dump the size of
MessageContext just before resetting it in each command cycle, and
then running the update.sql regression test (which is one of the two
that are seeing buildfarm OOMs) with and without CLOBBER_CACHE_ALWAYS.
Before 2455ab488, the peak space usage with CCA was as much as 250MB.
That patch got it down to ~80MB, but with this patch it's about 0.5MB,
and indeed the space usage now seems nearly indistinguishable from a
non-CCA build.
RelationBuildDesc's traditional behavior of not worrying about leaking
transient data is of many years' standing, so I'm pretty hesitant to
change that without more evidence that it'd be useful in a normal build.
(So far as I can see, non-CCA memory consumption is about the same with
or without this change, whuch if anything suggests that it isn't useful.)
Hence, configure the patch so that we recover space only when
CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVELY is defined. However,
that choice can be overridden at compile time, in case somebody would
like to do some performance testing and try to develop evidence for
changing that decision.
It's possible that we ought to back-patch this change, but in the
absence of back-branch OOM problems in the buildfarm, I'm not in
a hurry to do that.
Peter Eisentraut [Fri, 15 Mar 2019 10:21:01 +0000 (11:21 +0100)]
PL/Tcl: Improve trigger tests organization
The trigger tests for PL/Tcl were spread aroud pltcl_setup.sql and
pltcl_queries.sql, mixed with other tests, which makes them hard to
follow and edit. Move all the trigger-related pieces to a new file
pltcl_trigger.sql. This also makes the test setup more similar to
plperl and plpython.
Peter Eisentraut [Fri, 15 Mar 2019 09:16:26 +0000 (10:16 +0100)]
Add walreceiver API to get remote server version
Add a separate walreceiver API function walrcv_server_version() to get
the version of the remote server, instead of doing it as part of
walrcv_identify_system(). This allows the server version to be
available even for uses that don't call IDENTIFY_SYSTEM, and it seems
cleaner anyway.
This is for an upcoming patch, not currently used.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/20190115071359.GF1433@paquier.xyz
Thomas Munro [Fri, 15 Mar 2019 03:23:46 +0000 (16:23 +1300)]
Enable parallel query with SERIALIZABLE isolation.
Previously, the SERIALIZABLE isolation level prevented parallel query
from being used. Allow the two features to be used together by
sharing the leader's SERIALIZABLEXACT with parallel workers.
An extra per-SERIALIZABLEXACT LWLock is introduced to make it safe to
share, and new logic is introduced to coordinate the early release
of the SERIALIZABLEXACT required for the SXACT_FLAG_RO_SAFE
optimization, as follows:
The first backend to observe the SXACT_FLAG_RO_SAFE flag (set by
some other transaction) will 'partially release' the SERIALIZABLEXACT,
meaning that the conflicts and locks it holds are released, but the
SERIALIZABLEXACT itself will remain active because other backends
might still have a pointer to it.
Whenever any backend notices the SXACT_FLAG_RO_SAFE flag, it clears
its own MySerializableXact variable and frees local resources so that
it can skip SSI checks for the rest of the transaction. In the
special case of the leader process, it transfers the SERIALIZABLEXACT
to a new variable SavedSerializableXact, so that it can be completely
released at the end of the transaction after all workers have exited.
Remove the serializable_okay flag added to CreateParallelContext() by
commit 9da0cc35, because it's now redundant.
Author: Thomas Munro Reviewed-by: Haribabu Kommi, Robert Haas, Masahiko Sawada, Kevin Grittner
Discussion: https://postgr.es/m/CAEepm=0gXGYhtrVDWOTHS8SQQy_=S9xo+8oCxGLWZAOoeJ=yzQ@mail.gmail.com
Amit Kapila [Fri, 15 Mar 2019 02:55:57 +0000 (08:25 +0530)]
During pg_upgrade, conditionally skip transfer of FSMs.
If a heap on the old cluster has 4 pages or fewer, and the old cluster
was PG v11 or earlier, don't copy or link the FSM. This will shrink
space usage for installations with large numbers of small tables.
This will allow pg_upgrade to take advantage of commit b0eaa4c51b where
we have avoided creation of the free space map for small heap relations.
Author: John Naylor Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CACPNZCu4cOdm3uGnNEGXivy7Gz8UWyQjynDpdkPGabQ18_zK6g%40mail.gmail.com
Peter Eisentraut [Thu, 14 Mar 2019 23:16:45 +0000 (00:16 +0100)]
Reorder identity regression test
The previous test order had the effect that if something was wrong
with the identity functionality, the create_table_like test would
likely fail or crash first, which is confusing. Reorder so that the
identity test comes before create_table_like.
The idea was to generate all the junk in a destroyable subcontext rather
than leaking it in the caller's context, but partition_bounds_create was
still being called in the caller's context, allowing plenty of scope for
leakage. Also, get_rel_relkind() was still being called in the rel's
rd_pdcxt, creating a risk of session-lifespan memory wastage.
Simplify the logic a bit while at it. Also, reduce rd_pdcxt to
ALLOCSET_SMALL_SIZES, since it seems likely to not usually be big.
Probably something like this needs to be back-patched into v11,
but for now let's get some buildfarm testing on this.
Tom Lane [Thu, 14 Mar 2019 16:16:09 +0000 (12:16 -0400)]
Ensure dummy paths have correct required_outer if rel is parameterized.
The assertions added by commits 34ea1ab7f et al found another problem:
set_dummy_rel_pathlist and mark_dummy_rel were failing to label
the dummy paths they create with the correct outer_relids, in case
the relation is necessarily parameterized due to having lateral
references in its tlist. It's likely that this has no user-visible
consequences in production builds, at the moment; but still an assertion
failure is a bad thing, so back-patch the fix.
Per bug #15694 from Roman Zharkov (via Alexander Lakhin)
and an independent report by Tushar Ahuja.
Robert Haas [Thu, 14 Mar 2019 16:03:31 +0000 (12:03 -0400)]
Defend against leaks into RelationBuildPartitionDesc.
In normal builds, this isn't very important, because the leaks go
into fairly short-lived contexts, but under CLOBBER_CACHE_ALWAYS,
this can result in leaking hundreds of megabytes into MessageContext,
which probably explains recent failures on hyrax.
This may or may not be the best long-term strategy for dealing
with this leak, but we can change it later if we come up with
something better. For now, do this to make the buildfarm green
again (hopefully). Commit 898e5e3290a72d288923260143930fb32036c00c
seems to have exacerbated this problem for reasons that are not
quite clear, but I don't believe it's actually the cause.
Peter Eisentraut [Thu, 14 Mar 2019 07:25:25 +0000 (08:25 +0100)]
Fix volatile vs. pointer confusion
Variables used after a longjmp() need to be declared volatile. In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value. So we need
Michael Paquier [Thu, 14 Mar 2019 05:14:49 +0000 (14:14 +0900)]
Fix thinko when bumping on temporary directories in pg_checksums
This fixes an oversight from 5c99513. This has no actual consequence as
PG_TEMP_FILE_PREFIX and PG_TEMP_FILES_DIR have the same value so when
bumping on a temporary path the directory scan was still moving on to
the next entry instead of skipping the rest of the scan, but let's keep
the logic correct.
Author: Michael Banck Reviewed-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/20190314.115417.58230569.horiguchi.kyotaro@lab.ntt.co.jp
Backpatch-through: 11
Tom Lane [Thu, 14 Mar 2019 04:23:33 +0000 (00:23 -0400)]
Sync commentary in transam.h and bki.sgml.
Commit a6417078c missed updating some comments in transam.h about
reservation of high OIDs for development purposes. Also tamp down
an over-optimistic comment there about how easy it'd be to change
FirstNormalObjectId.
Earlier, commit 09568ec3d failed to update bki.sgml for the split
between genbki.pl-assigned OIDs and those assigned during initdb.
Also fix genbki.pl so that it will complain if it overruns
that split. It's possible that doing so would have no very bad
consequences, but that's no excuse for not detecting it.
Michael Paquier [Thu, 14 Mar 2019 03:41:45 +0000 (12:41 +0900)]
Fix race condition in recently-added TAP test for recovery consistency
A couple of queries are run on the primary to create and fill in a test
table, which gets checked on the standby afterwards. However the test
was not waiting for the confirmation that the necessary records have
been replayed on the standby, leading to spurious failures.
Per buildfarm member loach. Thanks to Thomas Munro for the report and
Tom Lane for the failure analysis.
Tom Lane [Thu, 14 Mar 2019 01:05:33 +0000 (21:05 -0400)]
Adjust the tests for the hyperbolic functions.
Preliminary results from the buildfarm suggest that no platform gets
commit c6f153dcf's test cases wrong by more than one or two units in
the last place, so setting extra_float_digits = 0 should be plenty
to hide the cross-platform variations.
Also, add tests for Infinity/NaN inputs. I think it highly likely
that we'll end up removing these again, rather than adding code to
make ancient platforms conform. But it seems useful to find out
just how many platforms have such issues before we make a decision.
Tom Lane [Wed, 13 Mar 2019 22:13:38 +0000 (18:13 -0400)]
Rethink how to test the hyperbolic functions.
The initial commit tried to test them on trivial cases such as 0,
reasoning that we shouldn't hit any portability issues that way.
The buildfarm immediately proved that hope ill-founded, and anyway
it's not a great testing scheme because it doesn't prove that we're
even calling the right library function for each SQL function.
Instead, let's test them at inputs such as 1 (or something within
the valid range, as needed), so that each function should produce
a different output.
As committed, this is just about certain to show portability
failures, because it's very unlikely that every platform computes
these functions the same as mine down to the last bit. However,
I want to put it through a buildfarm cycle this way, so that
we can see how big the variations are. The plan is to add
"set extra_float_digits = -1", or whatever we need in order to
hide the variations; but first we need data.
Thomas Munro [Wed, 13 Mar 2019 21:25:27 +0000 (10:25 +1300)]
Use condition variables to wait for checkpoints.
Previously we used a polling/sleeping loop to wait for checkpoints
to begin and end, which leads to up to a couple hundred milliseconds
of needless thumb-twiddling. Use condition variables instead.
Author: Thomas Munro Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/CA%2BhUKGLY7sDe%2Bbg1K%3DbnEzOofGoo4bJHYh9%2BcDCXJepb6DQmLw%40mail.gmail.com
Robert Haas [Wed, 13 Mar 2019 17:14:42 +0000 (13:14 -0400)]
Revert setting client_min_messages to 'debug1' in new tests.
The buildfarm doesn't like this, because some buildfarm members have
log_statement = 'all'. We could change the log level of the messages
instead, but Tom doesn't like that. So let's do this instead, at
least for now.
Patch by Sergei Kornilov, applied here in reverse.
Peter Eisentraut [Wed, 13 Mar 2019 13:15:37 +0000 (14:15 +0100)]
Include all columns in default names for foreign key constraints
When creating a name for a foreign key constraint when none is
specified, use all column names instead of only the first one, similar
to how it is already done for index names.
Author: Paul Martinez <hellopfm@gmail.com> Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/CAF+2_SFjky6XRfLNRXpkG97W6PRbOO_mjAxqXzAAimU=c7w7_A@mail.gmail.com
Michael Paquier [Wed, 13 Mar 2019 05:58:24 +0000 (14:58 +0900)]
Add TAP test to check consistency of minimum recovery LSN
c186ba13 has fixed an issue related to the updates of the minimum
recovery LSN across multiple processes on standbys, but we never really
had a test case able to reliably check its logic.
This commit introduces a new test case to close the gap, and is designed
to check the consistency of data based on the minimum recovery point set
by either the startup process or the checkpointer for both an offline
cluster (by looking at the on-disk page headers) and an online cluster
(using pageinspect).
Note that with c186ba13 reverted, this test fails badly for both the
online and offline cases, as designed.
Author: Michael Paquier, Andrew Gierth Reviewed-by: Andrew Gierth, Georgios Kokolatos, Arthur Zakirov
Discussion: https://postgr.es/m/20181108044525.GA17482@paquier.xyz
Michael Paquier [Wed, 13 Mar 2019 01:43:20 +0000 (10:43 +0900)]
Rename pg_verify_checksums to pg_checksums
The current tool name is too restrictive and focuses only on verifying
checksums. As more options to control checksums for an offline cluster
are planned to be added, switch to a more generic name. Documentation
as well as all past references to the tool are updated.
Author: Michael Paquier Reviewed-by: Michael Banck, Fabien Coelho, Seigei Kornilov
Discussion: https://postgr.es/m/20181221201616.GD4974@nighthawk.caipicrew.dd-dns.de
Michael Paquier [Wed, 13 Mar 2019 00:51:02 +0000 (09:51 +0900)]
Fix cross-version compatibility checks of pg_verify_checksums
pg_verify_checksums performs a read of the control file, and the data it
fetches should be from a data folder compatible with the major version
of Postgres the binary has been compiled with, but we never actually
checked that compatibility.
Reported-by: Sergei Kornilov
Author: Michael Paquier Reviewed-by: Sergei Kornilov
Discussion: https://postgr.es/m/155231347133.16480.11453587097036807558.pgcf@coridan.postgresql.org
Backpatch-through: 11
Peter Geoghegan [Tue, 12 Mar 2019 23:40:05 +0000 (16:40 -0700)]
Correct obsolete nbtree page split comment.
Commit 40dae7ec537, which made the nbtree page split algorithm more
robust, made _bt_insert_parent() only unlock the right child of the
parent page before inserting a new downlink into the parent. Update a
comment from the Berkeley days claiming that both left and right child
pages are unlocked before the new downlink actually gets inserted.
The claim that it is okay to release both locks early based on Lehman
and Yao's say-so never made much sense. Lehman and Yao must sometimes
"couple" buffer locks across a pair of internal pages when relocating a
downlink, unlike the corresponding code within _bt_getstack().
Tom Lane [Tue, 12 Mar 2019 19:55:09 +0000 (15:55 -0400)]
Add support for hyperbolic functions, as well as log10().
The SQL:2016 standard adds support for the hyperbolic functions
sinh(), cosh(), and tanh(). POSIX has long required libm to
provide those functions as well as their inverses asinh(),
acosh(), atanh(). Hence, let's just expose the libm functions
to the SQL level. As with the trig functions, we only implement
versions for float8, not numeric.
For the moment, we'll assume that all platforms actually do have
these functions; if experience teaches otherwise, some autoconf
effort may be needed.
SQL:2016 also adds support for base-10 logarithm, but with the
function name log10(), whereas the name we've long used is log().
Add aliases named log10() for the float8 and numeric versions.
Tom Lane [Tue, 12 Mar 2019 16:30:35 +0000 (12:30 -0400)]
Remove remaining hard-wired OID references in the initial catalog data.
In the v11-era commits that taught genbki.pl to resolve symbolic
OID references in the initial catalog data, we didn't bother to
make every last reference symbolic; some of the catalogs have so
few initial rows that it didn't seem worthwhile.
However, the new project policy that OIDs assigned by new patches
should be automatically renumberable changes this calculus.
A patch that wants to add a row in one of these catalogs would have
a problem when the OID it assigns gets renumbered. Hence, do the
mop-up work needed to make all OID references in initial data be
symbolic, and establish an associated project policy that we'll
never again write a hard-wired OID reference there.
No catversion bump since the contents of postgres.bki aren't
actually changed by this commit.
Tom Lane [Tue, 12 Mar 2019 14:50:48 +0000 (10:50 -0400)]
Create a script that can renumber manually-assigned OIDs.
This commit adds a Perl script renumber_oids.pl, which can reassign a
range of manually-assigned OIDs to someplace else by modifying OID
fields of the catalog *.dat files and OID-assigning macros in the
catalog *.h files.
Up to now, we've encouraged new patches that need manually-assigned
OIDs to use OIDs just above the range of existing OIDs. Predictably,
this leads to patches stepping on each others' toes, as whichever
one gets committed first creates an OID conflict that other patch
author(s) have to resolve manually. With the availability of
renumber_oids.pl, we can eliminate a lot of this hassle.
The new project policy, therefore, is:
* Encourage new patches to use high OIDs (the documentation suggests
choosing a block of OIDs at random in 8000..9999).
* After feature freeze in each development cycle, run renumber_oids.pl
to move all such OIDs down to lower numbers, thus freeing the high OID
range for the next development cycle.
This plan should greatly reduce the risk of OID collisions between
concurrently-developed patches. Also, if such a collision happens
anyway, we have the option to resolve it without much effort by doing
an off-schedule OID renumbering to get the first-committed patch out
of the way. Or a patch author could use renumber_oids.pl to change
their patch's assignments without much pain.
This approach does put a premium on not hard-wiring any OID values
in places where renumber_oids.pl and genbki.pl can't fix them.
Project practice in that respect seems to be pretty good already,
but a follow-on patch will sand down some rough edges.
John Naylor and Tom Lane, per an idea of Peter Geoghegan's
Etsuro Fujita [Tue, 12 Mar 2019 07:21:57 +0000 (16:21 +0900)]
Fix testing of parallel-safety of scan/join target.
In commit 960df2a971 ("Correctly assess parallel-safety of tlists when
SRFs are used."), the testing of scan/join target was done incorrectly,
which caused a plan-quality problem. Backpatch through to v11 where
the aforementioned commit went in, since this is a regression from v10.
Author: Etsuro Fujita Reviewed-by: Robert Haas and Tom Lane
Discussion: https://postgr.es/m/5C75303E.8020303@lab.ntt.co.jp
Amit Kapila [Tue, 12 Mar 2019 02:44:28 +0000 (08:14 +0530)]
Add more tests for FSM.
In commit b0eaa4c51bb, we left out a test that used a vacuum to remove dead
rows as the behavior of test was not predictable. This test has been
rewritten to use fillfactor instead to control free space. Since we no
longer need to remove dead rows as part of the test, put the fsm regression
test in a parallel group.
Author: John Naylor Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CAA4eK1L=qWp_bJ5aTc9+fy4Ewx2LPaLWY-RbR4a60g_rupCKnQ@mail.gmail.com
Michael Paquier [Tue, 12 Mar 2019 01:03:33 +0000 (10:03 +0900)]
Add routine able to update the control file to src/common/
This adds a new routine to src/common/ which is compatible with both the
frontend and backend code, able to update the control file's contents.
This is now getting used only by pg_rewind, but some upcoming patches
which add more control on checksums for offline instances will make use
of it. This could also get used more by the backend as xlog.c has its
own flavor of the same logic with some wait events and an additional
flush phase before closing the opened file descriptor, but this is let
as separate work.
Author: Michael Banck, Michael Paquier Reviewed-by: Fabien Coelho, Sergei Kornilov
Discussion: https://postgr.es/m/20181221201616.GD4974@nighthawk.caipicrew.dd-dns.de
Tom Lane [Mon, 11 Mar 2019 23:13:46 +0000 (19:13 -0400)]
Allow fractional input values for integer GUCs, and improve rounding logic.
Historically guc.c has just refused examples like set work_mem = '30.1GB',
but it seems more useful for it to take that and round off the value to
some reasonable approximation of what the user said. Just rounding to
the parameter's native unit would work, but it would lead to rather
silly-looking settings, such as 31562138kB for this example. Instead
let's round to the nearest multiple of the next smaller unit (if any),
producing 30822MB.
Also, do the units conversion math in floating point and round to integer
(if needed) only at the end. This produces saner results for inputs that
aren't exact multiples of the parameter's native unit, and removes another
difference in the behavior for integer vs. float parameters.
In passing, document the ability to use hex or octal input where it
ought to be documented.
Andrew Dunstan [Mon, 11 Mar 2019 22:14:05 +0000 (18:14 -0400)]
Document incompatibility of comparison expressions with VARIADIC array arguments
COALESCE, GREATEST and LEAST all look like functions taking variable
numbers of arguments, but in fact they are not functions, and so
VARIADIC array arguments don't work with them. Add a note to the docs
explaining this fact.
The consensus is not to try to make this work, but just to document the
limitation.
Tom Lane [Mon, 11 Mar 2019 21:53:09 +0000 (17:53 -0400)]
Give up on testing guc.c's behavior for "infinity" inputs.
Further buildfarm testing shows that on the machines that are failing ac75959cd's test case, what we're actually getting from strtod("-infinity")
is a syntax error (endptr == value) not ERANGE at all. This test case
is not worth carrying two sets of expected output for, so just remove it,
and revert commit b212245f9's misguided attempt to work around the platform
dependency.
Andres Freund [Mon, 11 Mar 2019 21:26:43 +0000 (14:26 -0700)]
Ensure sufficient alignment for ParallelTableScanDescData in BTShared.
Previously ParallelTableScanDescData was just a member in BTShared,
but after c2fe139c2 that doesn't guarantee sufficient alignment as
specific AMs might (are likely to) need atomic variables in the
struct.
One might think that MAXALIGNing would be sufficient, but as a
comment in shm_toc_allocate() explains, that's not enough. For now,
copy the hack described there.
For parallel sequential scans no such change is needed, as its
allocations go through shm_toc_allocate().
An alternative approach would have been to allocate the parallel scan
descriptor in a separate TOC entry, but there seems little benefit in
doing so.
Per buildfarm member dromedary.
Author: Andres Freund
Discussion: https://postgr.es/m/20190311203126.ty5gbfz42gjbm6i6@alap3.anarazel.de
Andres Freund [Mon, 11 Mar 2019 19:46:41 +0000 (12:46 -0700)]
tableam: Add and use scan APIs.
Too allow table accesses to be not directly dependent on heap, several
new abstractions are needed. Specifically:
1) Heap scans need to be generalized into table scans. Do this by
introducing TableScanDesc, which will be the "base class" for
individual AMs. This contains the AM independent fields from
HeapScanDesc.
The previous heap_{beginscan,rescan,endscan} et al. have been
replaced with a table_ version.
There's no direct replacement for heap_getnext(), as that returned
a HeapTuple, which is undesirable for a other AMs. Instead there's
table_scan_getnextslot(). But note that heap_getnext() lives on,
it's still used widely to access catalog tables.
This is achieved by new scan_begin, scan_end, scan_rescan,
scan_getnextslot callbacks.
2) The portion of parallel scans that's shared between backends need
to be able to do so without the user doing per-AM work. To achieve
that new parallelscan_{estimate, initialize, reinitialize}
callbacks are introduced, which operate on a new
ParallelTableScanDesc, which again can be subclassed by AMs.
As it is likely that several AMs are going to be block oriented,
block oriented callbacks that can be shared between such AMs are
provided and used by heap. table_block_parallelscan_{estimate,
intiialize, reinitialize} as callbacks, and
table_block_parallelscan_{nextpage, init} for use in AMs. These
operate on a ParallelBlockTableScanDesc.
3) Index scans need to be able to access tables to return a tuple, and
there needs to be state across individual accesses to the heap to
store state like buffers. That's now handled by introducing a
sort-of-scan IndexFetchTable, which again is intended to be
subclassed by individual AMs (for heap IndexFetchHeap).
The relevant callbacks for an AM are index_fetch_{end, begin,
reset} to create the necessary state, and index_fetch_tuple to
retrieve an indexed tuple. Note that index_fetch_tuple
implementations need to be smarter than just blindly fetching the
tuples for AMs that have optimizations similar to heap's HOT - the
currently alive tuple in the update chain needs to be fetched if
appropriate.
Similar to table_scan_getnextslot(), it's undesirable to continue
to return HeapTuples. Thus index_fetch_heap (might want to rename
that later) now accepts a slot as an argument. Core code doesn't
have a lot of call sites performing index scans without going
through the systable_* API (in contrast to loads of heap_getnext
calls and working directly with HeapTuples).
Index scans now store the result of a search in
IndexScanDesc->xs_heaptid, rather than xs_ctup->t_self. As the
target is not generally a HeapTuple anymore that seems cleaner.
To be able to sensible adapt code to use the above, two further
callbacks have been introduced:
a) slot_callbacks returns a TupleTableSlotOps* suitable for creating
slots capable of holding a tuple of the AMs
type. table_slot_callbacks() and table_slot_create() are based
upon that, but have additional logic to deal with views, foreign
tables, etc.
While this change could have been done separately, nearly all the
call sites that needed to be adapted for the rest of this commit
also would have been needed to be adapted for
table_slot_callbacks(), making separation not worthwhile.
b) tuple_satisfies_snapshot checks whether the tuple in a slot is
currently visible according to a snapshot. That's required as a few
places now don't have a buffer + HeapTuple around, but a
slot (which in heap's case internally has that information).
Additionally a few infrastructure changes were needed:
I) SysScanDesc, as used by systable_{beginscan, getnext} et al. now
internally uses a slot to keep track of tuples. While
systable_getnext() still returns HeapTuples, and will so for the
foreseeable future, the index API (see 1) above) now only deals with
slots.
The remainder, and largest part, of this commit is then adjusting all
scans in postgres to use the new APIs.
Author: Andres Freund, Haribabu Kommi, Alvaro Herrera
Discussion:
https://postgr.es/m/20180703070645.wchpu5muyto5n647@alap3.anarazel.de
https://postgr.es/m/20160812231527.GA690404@alvherre.pgsql
Alvaro Herrera [Mon, 11 Mar 2019 16:17:50 +0000 (13:17 -0300)]
Move hash_any prototype from access/hash.h to utils/hashutils.h
... as well as its implementation from backend/access/hash/hashfunc.c to
backend/utils/hash/hashfn.c.
access/hash is the place for the hash index AM, not really appropriate
for generic facilities, which is what hash_any is; having things the old
way meant that anything using hash_any had to include the AM's include
file, pointlessly polluting its namespace with unrelated, unnecessary
cruft.
Also move the HTEqual strategy number to access/stratnum.h from
access/hash.h.
To avoid breaking third-party extension code, add an #include
"utils/hashutils.h" to access/hash.h. (An easily removed line by
committers who enjoy their asbestos suits to protect them from angry
extension authors.)
Tom Lane [Mon, 11 Mar 2019 15:25:23 +0000 (11:25 -0400)]
In guc.c, ignore ERANGE errors from strtod().
Instead, just proceed with the infinity or zero result that it should
return for overflow/underflow. This avoids a platform dependency,
in that various versions of strtod are inconsistent about whether they
signal ERANGE for a value that's specified as infinity.
It's possible this won't be enough to remove the buildfarm failures
we're seeing from ac75959cd, in which case I'll take out the infinity
test case that commit added. But first let's see if we can fix it.
Michael Paquier [Mon, 11 Mar 2019 00:31:25 +0000 (09:31 +0900)]
Adjust error message for partial writes in WAL segments
93473c6 has removed openLogOff, changing on the way the error message
which is used to report partial writes to WAL segments. The
newly-introduced error message used the offset up to which the write has
happened, keeping always the same total length to write. This changes
the error message so as the number of bytes left to write are reported.
Reported-by: Michael Paquier
Author: Robert Haas
Discussion: https://postgr.es/m/20190306235251.GA17293@paquier.xyz
Alvaro Herrera [Sun, 10 Mar 2019 22:45:29 +0000 (19:45 -0300)]
Fix documentation on partitioning vs. foreign tables
1. The PARTITION OF clause of CREATE FOREIGN TABLE was not explained in
the CREATE FOREIGN TABLE reference page. Add it.
(Postgres 10 onwards)
2. The limitation that tuple routing cannot target partitions that are
foreign tables was not documented clearly enough. Improve wording.
(Postgres 10 onwards)
3. The UPDATE tuple re-routing concurrency behavior was explained in
the DDL chapter, which doesn't seem the right place. Move it to the
UPDATE reference page instead. (Postgres 11 onwards).
Authors: Amit Langote, David Rowley. Reviewed-by: Etsuro Fujita. Reported-by: Derek Hans
Discussion: https://postgr.es/m/CAGrP7a3Xc1Qy_B2WJcgAD8uQTS_NDcJn06O5mtS_Ne1nYhBsyw@mail.gmail.com
Per discussion, it seems like what we should do instead is to
reduce the default value of autovacuum_vacuum_cost_delay by the
same factor. That's functionally equivalent as long as the
platform can accurately service the smaller delay request, which
should be true on anything released in the last 10 years or more.
And smaller, more-closely-spaced delays are better in terms of
providing a steady I/O load.
Tom Lane [Sun, 10 Mar 2019 19:01:39 +0000 (15:01 -0400)]
Convert [autovacuum_]vacuum_cost_delay into floating-point GUCs.
This change makes it possible to specify sub-millisecond delays,
which work well on most modern platforms, though that was not true
when the cost-delay feature was designed.
To support this without breaking existing configuration entries,
improve guc.c to allow floating-point GUCs to have units. Also,
allow "us" (microseconds) as an input/output unit for time-unit GUCs.
(It's not allowed as a base unit, at least not yet.)
Likewise change the autovacuum_vacuum_cost_delay reloption to be
floating-point; this forces a catversion bump because the layout of
StdRdOptions changes.
This patch doesn't in itself change the default values or allowed
ranges for these parameters, and it should not affect the behavior
for any already-allowed setting for them.
Tom Lane [Sun, 10 Mar 2019 17:18:17 +0000 (13:18 -0400)]
Include GUC's unit, if it has one, in out-of-range error messages.
This should reduce confusion in cases where we've applied a units
conversion, so that the number being reported (and the quoted range
limits) are in some other units than what the user gave in the
setting we're rejecting.
Some of the changes here assume that float GUCs can have units,
which isn't true just yet, but will be shortly.
Tom Lane [Sun, 10 Mar 2019 16:58:51 +0000 (12:58 -0400)]
Disallow NaN as a value for floating-point GUCs.
None of the code that uses GUC values is really prepared for them to
hold NaN, but parse_real() didn't have any defense against accepting
such a value. Treat it the same as a syntax error.
I haven't attempted to analyze the exact consequences of setting any
of the float GUCs to NaN, but since they're quite unlikely to be good,
this seems like a back-patchable bug fix.
Note: we don't need an explicit test for +-Infinity because those will
be rejected by existing range checks. I added a regression test for
that in HEAD, but not older branches because the spelling of the value
in the error message will be platform-dependent in branches where we
don't always use port/snprintf.c.
Alvaro Herrera [Sun, 10 Mar 2019 16:20:58 +0000 (13:20 -0300)]
pg_upgrade: Ignore TOAST for partitioned tables
Since partitioned tables in pg12 do not have toast tables, trying to set
the toast OID confuses pg_upgrade. Have pg_dump omit those values to
avoid the problem.
Per Andres Freund and buildfarm members crake and snapper
Discussion: https://postgr.es/m/20190306204104.yle5jfbnqkcwykni@alap3.anarazel.de
Similarly to B-tree, GiST index access method gets support of INCLUDE
attributes. These attributes aren't used for tree navigation and aren't
present in non-leaf pages. But they are present in leaf pages and can be
fetched during index-only scan.
The point of having INCLUDE attributes in GiST indexes is slightly different
from the point of having them in B-tree. The main point of INCLUDE attributes
in B-tree is to define UNIQUE constraint over part of attributes enabled for
index-only scan. In GiST the main point of INCLUDE attributes is to use
index-only scan for attributes, whose data types don't have GiST opclasses.
Discussion: https://postgr.es/m/73A1A452-AD5F-40D4-BD61-978622FF75C1%40yandex-team.ru
Author: Andrey Borodin, with small changes by me Reviewed-by: Andreas Karlsson
Magnus Hagander [Sat, 9 Mar 2019 20:09:10 +0000 (12:09 -0800)]
Add new clientcert hba option verify-full
This allows a login to require both that the cn of the certificate
matches (like authentication type cert) *and* that another
authentication method (such as password or kerberos) succeeds as well.
The old value of clientcert=1 maps to the new clientcert=verify-ca,
clientcert=0 maps to the new clientcert=no-verify, and the new option
erify-full will add the validation of the CN.
Author: Julian Markwort, Marius Timmer
Reviewed by: Magnus Hagander, Thomas Munro
Magnus Hagander [Sat, 9 Mar 2019 18:45:17 +0000 (10:45 -0800)]
Track block level checksum failures in pg_stat_database
This adds a column that counts how many checksum failures have occurred
on files belonging to a specific database. Both checksum failures
during normal backend processing and those created when a base backup
detects a checksum failure are counted.
Author: Magnus Hagander
Reviewed by: Julien Rouhaud
projects / postgresql / log