Michael Paquier [Sun, 27 Oct 2019 04:54:12 +0000 (13:54 +0900)]
Fix initialization of fake LSN for unlogged relations
9155580 has changed the value of the first fake LSN for unlogged
relations from 1 to FirstNormalUnloggedLSN (aka 1000), GiST requiring a
non-zero LSN on some pages to allow an interlocking logic to work, but
its value was still initialized to 1 at the beginning of recovery or
after running pg_resetwal. This fixes the initialization for both code
paths.
Tom Lane [Sat, 26 Oct 2019 16:30:41 +0000 (12:30 -0400)]
Doc: improve documentation of configuration settings that have units.
When we added the GUC units feature, we didn't make any great effort
to adjust the documentation of individual GUCs; they tended to still
say things like "this is the number of milliseconds that ...", even
though users might prefer to write some other units, and SHOW might
even show the value in other units. Commit 6c9fb69f2 made an effort
to improve this situation, but I thought it made things less readable
by injecting units information in mid-sentence. It also wasn't very
consistent, and did not touch all the GUCs that have units.
To improve matters, standardize on the phrasing "If this value is
specified without units, it is taken as <units>". Also, try to
standardize where this is mentioned, right before the specification
of the default. (In a couple of places, doing that would've required
more rewriting than seemed justified, so I wasn't 100% consistent
about that.) I also tried to use the phrases "amount of time",
"amount of memory", etc rather than describing the contents of GUCs
in other ways, as those were the majority usage in places that weren't
overcommitting to a particular unit. (I left "length of time" alone
in a couple of places, though.)
I failed to resist the temptation to copy-edit some awkward text, too.
Backpatch to v12, like 6c9fb69f2, mainly because v12 hasn't diverged
much from HEAD yet.
Peter Eisentraut [Fri, 25 Oct 2019 19:11:48 +0000 (21:11 +0200)]
Remove obsolete information schema tables
Remove SQL_LANGUAGES, which was eliminated in SQL:2008, and
SQL_PACKAGES and SQL_SIZING_PROFILES, which were eliminated in
SQL:2011. Since they were dropped by the SQL standard, the
information in them was no longer updated and therefore no longer
useful.
This also removes the feature-package association information in
sql_feature_packages.txt, but for the time begin we are keeping the
information which features are in the Core package (that is, mandatory
SQL features). Maybe at some point someone wants to invent a way to
store that that does not involve using the "package" mechanism
anymore.
Tom Lane [Fri, 25 Oct 2019 19:22:40 +0000 (15:22 -0400)]
Avoid failure when selecting a namespace node in XMLTABLE.
It appears that libxml2 doesn't bother to set the "children" field of
an XML_NAMESPACE_DECL node to null; that field just contains garbage.
In v10 and v11, this can result in a crash in XMLTABLE(). The rewrite
done in commit 251cf2e27 fixed this, somewhat accidentally, in v12.
We're not going to back-patch 251cf2e27, however. The case apparently
doesn't have wide use, so rather than risk introducing other problems,
just add a safety check to throw an error.
Even though no bug manifests in v12/HEAD, add the relevant test case
there too, to prevent future regressions.
Tom Lane [Fri, 25 Oct 2019 16:17:41 +0000 (12:17 -0400)]
Get rid of useless/dangerous redefinition of bool in ECPG.
pgtypeslib_extern.h contained fallback definitions of "bool", "FALSE",
and "TRUE". The latter two are just plain unused, and have been for
awhile. The former came into play only if there wasn't a macro
definition of "bool", which is true only if we aren't using <stdbool.h>.
However, it then defined bool as "char"; since commit d26a810eb that
conflicts with c.h's desire to use "unsigned char". We'd missed seeing
any bad effects of that due to accidental header inclusion order choices,
but dddf4cdc3 exposed that it was problematic.
To fix, let's just get rid of these definitions. They should not be
needed because everyplace in Postgres should be relying on c.h to
provide a definition for type bool. (Note that despite its name,
pgtypeslib_extern.h isn't exposed to any outside code; we don't
install it.)
This doesn't fully resolve the issue, because ecpglib.h is doing
similar things, but that seems to require more thought to fix.
Back-patch to v12 where d26a810eb came in, to forestall any unpleasant
surprises from future back-patched bug fixes.
Tom Lane [Fri, 25 Oct 2019 15:41:16 +0000 (11:41 -0400)]
Improve management of statement timeouts.
Commit f8e5f156b added private state in postgres.c to track whether
a statement timeout is running. This seems like bad design to me;
timeout.c's private state should be the single source of truth about
that. We already fixed one bug associated with failure to keep those
states in sync (cf. be42015fc), and I've got little faith that we
won't find more in future. So get rid of postgres.c's local variable
by exposing a way to ask timeout.c whether a timeout is running.
(Obviously, such an inquiry is subject to race conditions, but it
seems fine for the purpose at hand.)
To make get_timeout_active() as cheap as possible, add a flag in
the per-timeout struct showing whether that timeout is active.
This allows some small savings elsewhere in timeout.c, mainly
elimination of unnecessary searches of the active_timeouts array.
While at it, fix enable_statement_timeout to not call disable_timeout
when statement_timeout is 0 and the timeout is not running. This
avoids a useless deschedule-and-reschedule-timeouts cycle, which
represents a significant savings (at least one kernel call) when
there is any other active timeout. Right now, there usually isn't,
but there are proposals around to change that.
Tom Lane [Fri, 25 Oct 2019 15:15:50 +0000 (11:15 -0400)]
Reset statement_timeout between queries of a multi-query string.
Historically, we started the timer (if StatementTimeout > 0) at the
beginning of a simple-Query message and usually let it run until the
end, so that the timeout limit applied to the entire query string,
and intra-string changes of the statement_timeout GUC had no effect.
But, confusingly, a COMMIT within the string would reset the state
and allow a fresh timeout cycle to start with the current setting.
Commit f8e5f156b changed the behavior of statement_timeout for extended
query protocol, and as an apparently-unintended side effect, a change in
the statement_timeout GUC during a multi-statement simple-Query message
might have an effect immediately --- but only if it was going from
"disabled" to "enabled".
This is all pretty confusing, not to mention completely undocumented.
Let's change things so that the timeout is always reset between queries
of a multi-query string, whether they're transaction control commands
or not. Thus the active timeout setting is applied to each query in
the string, separately. This costs a few more cycles if statement_timeout
is active, but it provides much more intuitive behavior, especially if one
changes statement_timeout in one of the queries of the string.
Also, add something to the documentation to explain all this.
Per bug #16035 from Raj Mohite. Although this is a bug fix, I'm hesitant
to back-patch it; conceivably somebody has worked out the old behavior
and is depending on it. (But note that this change should make the
behavior less restrictive in most cases, since the timeout will now
be applied to shorter segments of code.)
The commit dddf4cdc3 tries to ensure that the Postgres header file
inclusions are in order based on their ASCII value. However, in one of
the case there is a header file dependency due to which we can't maintain
such order.
Author: Amit Kapila
Discussion: https://postgr.es/m/E1iNpHW-000855-1u@gemulon.postgresql.org
Michael Paquier [Fri, 25 Oct 2019 01:20:08 +0000 (10:20 +0900)]
Handle interrupts within a transaction context in REINDEX CONCURRENTLY
Phases 2 (building the new index) and 3 (validating the new index)
checked for interrupts outside a transaction context, having as
consequence to not release session-level locks taken on the parent
relation and the old and new indexes processed. This could for example
be triggered with statement_timeout and a bad timing, and would issue
confusing error messages when shutting down the session still holding
the locks (note that an assertion failure would be triggered first), on
top of more issues with concurrent sessions trying to take a lock that
would interfere with the SHARE UPDATE EXCLUSIVE locks hold here.
This moves all the interruption checks inside a transaction context.
Note that I have manually tested all interruptions to make sure that
invalid indexes can be cleaned up properly. Partition indexes still
have issues on their own with some missing dependency handling, which
will be dealt with in a follow-up patch.
Reported-by: Justin Pryzby
Author: Michael Paquier
Discussion: https://postgr.es/m/20191013025145.GC4475@telsasoft.com
Backpatch-through: 12
Amit Kapila [Wed, 23 Oct 2019 03:56:22 +0000 (09:26 +0530)]
Make the order of the header file includes consistent in contrib modules.
The basic rule we follow here is to always first include 'postgres.h' or
'postgres_fe.h' whichever is applicable, then system header includes and
then Postgres header includes. In this, we also follow that all the
Postgres header includes are in order based on their ASCII value. We
generally follow these rules, but the code has deviated in many places.
This commit makes it consistent just for contrib modules. The later
commits will enforce similar rules in other parts of code.
Author: Vignesh C Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CALDaNm2Sznv8RR6Ex-iJO6xAdsxgWhCoETkaYX=+9DW3q0QCfA@mail.gmail.com
Bruce Momjian [Wed, 23 Oct 2019 22:06:38 +0000 (18:06 -0400)]
pg_upgrade: adjust error output to use new database list format
Commit a524f50d0f added
old_11_check_for_sql_identifier_data_type_usage(), but it did not use
the clearer database error list format added to the master branch in
commit 1634d36157. This commit fixes that.
Michael Paquier [Wed, 23 Oct 2019 06:04:48 +0000 (15:04 +0900)]
Acquire properly session-level lock on new index in REINDEX CONCURRENTLY
In the first transaction run for REINDEX CONCURRENTLY, a thinko in the
existing logic caused two session locks to be taken on the old index,
causing the session lock on the newly-created index to be missed. This
made possible concurrent DDL commands (like ALTER INDEX) on the new
index while REINDEX CONCURRENTLY was processing from the point where the
first internal transaction committed.
This issue has been discovered while digging into another bug.
Author: Michael Paquier
Discussion: https://postgr.es/m/20191021074323.GB1869@paquier.xyz
Backpatch-through: 12
Michael Paquier [Wed, 23 Oct 2019 03:27:03 +0000 (12:27 +0900)]
Remove last traces of --adduser/--no-adduser in createuser
8ae0d47 marked those options as obsolete back in 2005, with the options
removed from the documentation. This removes the last references to
both options in the code which were kept around for compatibility
purposes with past commands.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/5da284a2-62d9-e338-88d1-26ee5009d93e@gmail.com
Michael Paquier [Wed, 23 Oct 2019 02:34:18 +0000 (11:34 +0900)]
Fix thinkos from 4f4061b for libpq integer parsing
A check was redundant. While on it, add an assertion to make sure that
the parsing routine is never called with a NULL input. All the code
paths currently calling the parsing routine are careful with NULL inputs
already, but future callers may forget that.
Reported-by: Peter Eisentraut, Lars Kanis
Discussion: https://postgr.es/m/ec64956b-4597-56b6-c3db-457d15250fe4@2ndquadrant.com
Backpatch-through: 12
Michael Paquier [Wed, 23 Oct 2019 01:25:06 +0000 (10:25 +0900)]
Clean up properly error_context_stack in autovacuum worker on exception
Any callback set would have no meaning in the context of an exception.
As an autovacuum worker exits quickly in this context, this could be
only an issue within EmitErrorReport(), where the elog hook is for
example called. That's unlikely to going to be a problem, but let's be
clean and consistent with other code paths handling exceptions. This is
present since 2909419, which introduced autovacuum.
Author: Ashwin Agrawal Reviewed-by: Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/CALfoeisM+_+dgmAdAOHAu0k-ZpEHHqSSG=GRf3pKJGm8OqWX0w@mail.gmail.com
Backpatch-through: 9.4
Peter Eisentraut [Tue, 22 Oct 2019 07:58:20 +0000 (09:58 +0200)]
Fix comment
The last argument of smgrextend() was renamed from isTemp to skipFsync
in debcec7dc31a992703911a9953e299c8d730c778, but the comments at two
call sites were not updated.
This commit refactors come ridiculous coding in compareDatetime(). Also, it
provides correct cross-datatype comparison even when one of values overflows
during cast. That eliminates dilemma on whether we should suppress overflow
errors during cast.
Reported-by: Tom Lane
Discussion: https://postgr.es/m/32308.1569455803%40sss.pgh.pa.us
Discussion: https://postgr.es/m/a5629d0c-8162-7559-16aa-0c8390d6ba5f%40postgrespro.ru
Author: Nikita Glukhov, Alexander Korotkov
While casting from timestamp to timestamptz we do timestamp2tm() then
tm2timestamp(). This commit eliminates call to tm2timestamp(). Instead, it
directly applies timezone offset to the original timestamp value. That makes
upcoming datetime overflow handling in jsonpath easier. That should also save
us some CPU cycles.
Discussion: https://postgr.es/m/CAPpHfdvRPRh_mTGar5WmDeRZ%3DU5dOXHdxspYYD%3D76m3knNGjXA%40mail.gmail.com
Author: Alexander Korotkov Reviewed-by: Tom Lane
Tom Lane [Mon, 21 Oct 2019 18:18:01 +0000 (14:18 -0400)]
Deal with yet another issue related to "Norwegian (Bokmål)" locale.
It emerges that recent versions of Windows (at least 2016 Standard)
spell this locale name as "Norwegian Bokmål_Norway.1252", defeating
our mapping code that translates "Norwegian (Bokmål)_Norway" to
something that's all-ASCII (cf commits db29620d4 and aa1d2fc5e).
Add another mapping entry to handle this spelling.
Per bug #16068 from Robert Ford. Like the previous patches,
back-patch to all supported branches.
Tom Lane [Mon, 21 Oct 2019 17:52:25 +0000 (13:52 -0400)]
Use CFLAGS_SL while probing linkability of libperl.
On recent Red Hat platforms (at least RHEL 8 and Fedora 30, maybe older),
configure's probe for libperl failed if the user forces CFLAGS to be -O0.
This is because some code in perl's inline.h fails to be optimized away
at -O0, and said code doesn't work if compiled without -fPIC.
To fix, add CFLAGS_SL to the compile flags used during the libperl probe.
This is a better simulation of the way that plperl is built, anyway,
so it might forestall other issues in future.
Per gripe from Kyotaro Horiguchi. Back-patch to all supported branches,
since people might want to build older branches on these platforms.
Tom Lane [Mon, 21 Oct 2019 16:32:35 +0000 (12:32 -0400)]
Select CFLAGS_SL at configure time, not in platform-specific Makefiles.
Move the platform-dependent logic that sets CFLAGS_SL from
src/makefiles/Makefile.foo to src/template/foo, so that the value
is determined at configure time and thus is available while running
configure's tests.
On a couple of platforms this might save a few microseconds of build
time by eliminating a test that make otherwise has to do over and over.
Otherwise it's pretty much a wash for build purposes; in particular,
this makes no difference to anyone who might be overriding CFLAGS_SL
via a make option.
This patch in itself does nothing with the value and thus should not
change any behavior, though you'll probably have to re-run configure
to get a correctly updated Makefile.global. We'll use the new
configure variable in a follow-on patch.
Per gripe from Kyotaro Horiguchi. Back-patch to all supported branches,
because the follow-on patch is a portability bug fix.
Etsuro Fujita [Mon, 21 Oct 2019 08:30:00 +0000 (17:30 +0900)]
Update obsolete comment.
Commit b52b7dc25, which moved code creating PartitionBoundInfo in
RelationBuildPartitionDesc() in partcache.c (relocated to partdesc.c
afterwards) to partbounds.c, should have updated this, but didn't.
We memorize all internal and empty leaf pages in the 1st vacuum stage for
gist indexes. They are used in the 2nd stage, to delete all the empty
pages. There was a memory context page_set_context for this purpose, but
we never used it.
Reported-by: Amit Kapila
Author: Dilip Kumar Reviewed-by: Amit Kapila
Backpatch-through: 12, where it got introduced
Discussion: https://postgr.es/m/CAA4eK1LGr+MN0xHZpJ2dfS8QNQ1a_aROKowZB+MPNep8FVtwAA@mail.gmail.com
Michael Paquier [Mon, 21 Oct 2019 02:39:15 +0000 (11:39 +0900)]
Fix error reporting of connect_timeout in libpq for value parsing
The logic was correctly detecting a parsing failure, but the parsing
error did not get reported back to the client properly.
Reported-by: Ed Morley
Author: Lars Kanis Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/a9b4cbd7-4ecb-06b2-ebd7-1739bbff3217@greiz-reinsdorf.de
Backpatch-through: 12
Michael Paquier [Mon, 21 Oct 2019 02:17:13 +0000 (11:17 +0900)]
Fix parsing of integer values for connection parameters in libpq
Commit e7a2217 has introduced stricter checks for integer values in
connection parameters for libpq. However this failed to correctly check
after trailing whitespaces, while leading whitespaces were discarded per
the use of strtol(3). This fixes and refactors the parsing logic to
handle both cases consistently. Note that trying to restrict the use of
trailing whitespaces can easily break connection strings like in ECPG
regression tests (these have allowed me to catch the parsing bug with
connect_timeout).
Author: Michael Paquier Reviewed-by: Lars Kanis
Discussion: https://postgr.es/m/a9b4cbd7-4ecb-06b2-ebd7-1739bbff3217@greiz-reinsdorf.de
Backpatch-through: 12
Peter Eisentraut [Sun, 20 Oct 2019 08:19:13 +0000 (10:19 +0200)]
Clean up MinGW def file generation
There were some leftovers from ancient ad-hoc ways to build on
Windows, prior to the standardization on MSVC and MinGW. We don't
need to build a lib$(NAME)ddll.def (debug build, as opposed to
lib$(NAME)dll.def) for MinGW, since nothing uses that. We also don't
need to build the regular .def file during distprep, since the MinGW
build environment is perfectly capable of creating that normally at
build time.
Peter Eisentraut [Sat, 19 Oct 2019 16:21:58 +0000 (18:21 +0200)]
Fix most -Wundef warnings
In some cases #if was used instead of #ifdef in an inconsistent style.
Cleaning this up also helps when analyzing cases like 38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd where this makes a
difference.
There are no behavior changes here, but the change in pg_bswap.h would
prevent possible accidental misuse by third-party code.
Noah Misch [Sat, 19 Oct 2019 03:20:28 +0000 (20:20 -0700)]
For PowerPC instruction "addi", use constraint "b".
Without "b", a variant of the tas() code miscompiles on macOS 10.4.
This may also fix a compilation failure involving macOS 10.1. Today's
compilers have been allocating acceptable registers with or without this
change, but this future-proofs the code by precisely conveying the
acceptable registers. Back-patch to 9.4 (all supported versions).
Michael Paquier [Sat, 19 Oct 2019 02:18:15 +0000 (11:18 +0900)]
Remove last traces of heap_open/close in the tree
Since pluggable storage has been introduced, those two routines have
been replaced by table_open/close, with some compatibility macros still
present to allow extensions to compile correctly with v12.
Some code paths using the old routines still remained, so replace them.
Based on the discussion done, the consensus reached is that it is better
to remove those compatibility macros so as nothing new uses the old
routines, so remove also the compatibility macros.
Fujii Masao [Fri, 18 Oct 2019 13:32:18 +0000 (22:32 +0900)]
Fix failure of archive recovery with recovery_min_apply_delay enabled.
recovery_min_apply_delay parameter is intended for use with streaming
replication deployments. However, the document clearly explains that
the parameter will be honored in all cases if it's specified. So it should
take effect even if in archive recovery. But, previously, archive recovery
with recovery_min_apply_delay enabled always failed, and caused assertion
failure if --enable-caasert is enabled.
The cause of this problem is that; the ownership of recoveryWakeupLatch
that recovery_min_apply_delay uses was taken only when standby mode
is requested. So unowned latch could be used in archive recovery, and
which caused the failure.
This commit changes recovery code so that the ownership of
recoveryWakeupLatch is taken even in archive recovery. Which prevents
archive recovery with recovery_min_apply_delay from failing.
Back-patch to v9.4 where recovery_min_apply_delay was added.
Author: Fujii Masao Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/CAHGQGwEyD6HdZLfdWc+95g=VQFPR4zQL4n+yHxQgGEGjaSVheQ@mail.gmail.com
Fujii Masao [Fri, 18 Oct 2019 13:24:18 +0000 (22:24 +0900)]
Make crash recovery ignore recovery_min_apply_delay setting.
In v11 or before, this setting could not take effect in crash recovery
because it's specified in recovery.conf and crash recovery always
starts without recovery.conf. But commit 2dedf4d9a8 integrated
recovery.conf into postgresql.conf and which unexpectedly allowed
this setting to take effect even in crash recovery. This is definitely
not good behavior.
To fix the issue, this commit makes crash recovery always ignore
recovery_min_apply_delay setting.
Back-patch to v12 where the issue was added.
Author: Fujii Masao Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/CAHGQGwEyD6HdZLfdWc+95g=VQFPR4zQL4n+yHxQgGEGjaSVheQ@mail.gmail.com
Discussion: https://postgr.es/m/e445616d-023e-a268-8aa1-67b8b335340c@pgmasters.net
Alvaro Herrera [Fri, 18 Oct 2019 12:49:39 +0000 (14:49 +0200)]
Fix typo
Apparently while this code was being developed,
ReindexRelationConcurrently operated on multiple relations. The version
that was ultimately pushed doesn't, so this comment's use of plural is
inaccurate.
Michael Paquier [Fri, 18 Oct 2019 05:26:29 +0000 (14:26 +0900)]
Fix timeout handling in logical replication worker
The timestamp tracking the last moment a message is received in a
logical replication worker was initialized in each loop checking if a
message was received or not, causing wal_receiver_timeout to be ignored
in basically any logical replication deployments. This also broke the
ping sent to the server when reaching half of wal_receiver_timeout.
This simply moves the initialization of the timestamp out of the apply
loop to the beginning of LogicalRepApplyLoop().
Alvaro Herrera [Thu, 17 Oct 2019 13:06:06 +0000 (15:06 +0200)]
Fix minor bug in logical-replication walsender shutdown
Logical walsender should exit when it catches up with sending WAL during
shutdown; but there was a rare corner case when it failed to because of
a race condition that puts it back to wait for more WAL instead -- but
since there wasn't any, it'd not shut down immediately. It would only
continue the shutdown when wal_sender_timeout terminates the sleep,
which causes annoying waits during shutdown procedure. Restructure the
code so that we no longer forget to set WalSndCaughtUp in that case.
Alvaro Herrera [Thu, 17 Oct 2019 07:58:01 +0000 (09:58 +0200)]
Fix parallel restore of FKs to partitioned tables
When an FK constraint is created, it needs the index on the referenced
table to exist and be valid. When doing parallel pg_restore and the
referenced table was partitioned, this condition can sometimes not be
met, because pg_dump didn't emit sufficient object dependencies to
ensure so; this means that parallel pg_restore would fail in certain
conditions. Fix by having pg_dump make the FK constraint object
dependent on the partition attachment objects for the constraint's
referenced index.
This has been broken since f56f8f8da6af, so backpatch to Postgres 12.
Thomas Munro [Thu, 17 Oct 2019 00:24:50 +0000 (13:24 +1300)]
When restoring GUCs in parallel workers, show an error context.
Otherwise it can be hard to see where an error is coming from, when
the parallel worker sets all the GUCs that it received from the
leader. Bug #15726. Back-patch to 9.5, where RestoreGUCState()
appeared.
Reported-by: Tiago Anastacio Reviewed-by: Daniel Gustafsson, Tom Lane
Discussion: https://postgr.es/m/15726-6d67e4fa14f027b3%40postgresql.org
Thomas Munro [Wed, 16 Oct 2019 20:59:21 +0000 (09:59 +1300)]
Fix bug that could try to freeze running multixacts.
Commits 801c2dc7 and 801c2dc7 made it possible for vacuum to
try to freeze a multixact that is still running. That was
prevented by a check, but raised an error. Repair.
Back-patch all the way.
Author: Nathan Bossart, Jeremy Schneider Reported-by: Jeremy Schneider Reviewed-by: Jim Nasby, Thomas Munro
Discussion: https://postgr.es/m/DAFB8AFF-2F05-4E33-AD7F-FF8B0F760C17%40amazon.com
Alvaro Herrera [Wed, 16 Oct 2019 12:51:34 +0000 (14:51 +0200)]
Fix crash when reporting CREATE INDEX progress
A race condition can make us try to dereference a NULL pointer to the
PGPROC struct of a process that's already finished. That results in
crashes during REINDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY.
This was introduced in ab0dfc961b6a, so backpatch to pg12.
Tomas Vondra [Wed, 16 Oct 2019 11:23:18 +0000 (13:23 +0200)]
Improve the check for pg_catalog.unknown data type in pg_upgrade
The pg_upgrade check for pg_catalog.unknown type when upgrading from 9.6
had a couple of issues with domains and composite types - it detected
even composite types unused in objects with storage. So for example this
was enough to trigger an unnecessary pg_upgrade failure:
CREATE TYPE unknown_composite AS (u pg_catalog.unknown)
On the other hand, this only happened with composite types directly on
the pg_catalog.unknown data type, but not with a domain. So this was not
detected
CREATE DOMAIN unknown_domain AS pg_catalog.unknown;
CREATE TYPE unknown_composite_2 AS (u unknown_domain);
unlike the first example. These false positives and inconsistencies are
unfortunate, but what's worse we've failed to detected objects using the
pg_catalog.unknown type through a domain. So we missed cases like this
CREATE TABLE t (u unknown_composite_2);
The consequence is clusters broken after a pg_upgrade.
This fixes these false positives and false negatives by using the same
recursive CTE introduced by eaf900e842 for sql_identifier. Backpatch all
the way to 10, where the of pg_catalog.unknown data type was restricted.
Author: Tomas Vondra Backpatch-to: 10-
Discussion: https://postgr.es/m/16045-673e8fa6b5ace196%40postgresql.org
Tomas Vondra [Wed, 16 Oct 2019 11:23:14 +0000 (13:23 +0200)]
Improve the check for pg_catalog.line data type in pg_upgrade
The pg_upgrade check for pg_catalog.line data type when upgrading from
9.3 had a couple of issues with domains and composite types. Firstly, it
triggered false positives for composite types unused in objects with
storage. This was enough to trigger an unnecessary pg_upgrade failure:
CREATE TYPE line_composite AS (l pg_catalog.line)
On the other hand, this only happened with composite types directly on
the pg_catalog.line data type, but not with a domain. So this was not
detected
CREATE DOMAIN line_domain AS pg_catalog.line;
CREATE TYPE line_composite_2 AS (l line_domain);
unlike the first example. These false positives and inconsistencies are
unfortunate, but what's worse we've failed to detected objects using the
pg_catalog.line data type through a domain. So we missed cases like this
CREATE TABLE t (l line_composite_2);
The consequence is clusters broken after a pg_upgrade.
This fixes these false positives and false negatives by using the same
recursive CTE introduced by eaf900e842 for sql_identifier. 9.3 did not
support domains on composite types, but we can still have multi-level
composite types.
Backpatch all the way to 9.4, where the format for pg_catalog.line data
type changed.
Author: Tomas Vondra Backpatch-to: 9.4-
Discussion: https://postgr.es/m/16045-673e8fa6b5ace196%40postgresql.org
Andres Freund [Wed, 16 Oct 2019 09:37:34 +0000 (02:37 -0700)]
Replace alter_table.sql test usage of event triggers.
The test in 93765bd956b added an event trigger to ensure that the
tested table rewrites do not get optimized away (as happened in the
past). But doing so would require running the tests in isolation, as
otherwise the trigger might also fire in concurrent sessions, causing
test failures there.
Reported-By: Tom Lane
Discussion: https://postgr.es/m/3328.1570740683@sss.pgh.pa.us
Backpatch: 12, just as 93765bd956b
Thomas Munro [Wed, 16 Oct 2019 03:51:40 +0000 (16:51 +1300)]
Use libc version as a collation version on glibc systems.
Using glibc's version string to detect potential collation definition
changes is not 100% reliable, but it's better than nothing. Currently
this affects only collations explicitly provided by "libc". More work
will be needed to handle the default collation.
Author: Thomas Munro, based on a suggestion from Christoph Berg Reviewed-by: Peter Eisentraut
Discussion: https://postgr.es/m/4b76c6d4-ae5e-0dc6-7d0d-b5c796a07e34%402ndquadrant.com
Michael Paquier [Wed, 16 Oct 2019 04:09:52 +0000 (13:09 +0900)]
Doc: Fix various inconsistencies
This fixes multiple areas of the documentation:
- COPY for its past compatibility section.
- SET ROLE mentioning INHERITS instead of INHERIT
- PREPARE referring to stmt_name, that is not present.
- Extension documentation about format name with upgrade scripts.
Backpatch down to 9.4 for the relevant parts.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/bf95233a-9943-b341-e2ff-a860c28af481@gmail.com
Backpatch-through: 9.4
Andres Freund [Tue, 15 Oct 2019 17:40:13 +0000 (10:40 -0700)]
Fix CLUSTER on expression indexes.
Since the introduction of different slot types, in 1a0586de3657, we
create a virtual slot in tuplesort_begin_cluster(). While that looks
right, it unfortunately doesn't actually work, as ExecStoreHeapTuple()
is used to store tuples in the slot. Unfortunately no regression tests
for CLUSTER on expression indexes existed so far.
Fix the slot type, and add bare bones tests for CLUSTER on expression
indexes.
Reported-By: Justin Pryzby
Author: Andres Freund
Discussion: https://postgr.es/m/20191011210320.GS10470@telsasoft.com
Backpatch: 12, like 1a0586de3657
Tomas Vondra [Mon, 14 Oct 2019 20:31:56 +0000 (22:31 +0200)]
Check for tables with sql_identifier during pg_upgrade
Commit 7c15cef86d changed sql_identifier data type to be based on name
instead of varchar. Unfortunately, this breaks on-disk format for this
data type. Luckily, that should be a very rare problem, as this data
type is used only in information_schema views, so this only affects user
objects (tables, materialized views and indexes). One way to end in
such situation is to do CTAS with a query on those system views.
There are two options to deal with this - we can either abort pg_upgrade
if there are user objects with sql_identifier columns in pg_upgrade, or
we could replace the sql_identifier type with varchar. Considering how
rare the issue is expected to be, and the complexity of replacing the
data type (e.g. in matviews), we've decided to go with the simple check.
The query is somewhat complex - the sql_identifier data type may be used
indirectly - through a domain, a composite type or both, possibly in
multiple levels. Detecting this requires a recursive CTE.
Backpatch to 12, where the sql_identifier definition changed.
Reported-by: Hans Buschmann
Author: Tomas Vondra Reviewed-by: Tom Lane Backpatch-to: 12
Discussion: https://postgr.es/m/16045-673e8fa6b5ace196%40postgresql.org
Michael Paquier [Sun, 13 Oct 2019 23:58:38 +0000 (08:58 +0900)]
Update test output of sepgsql for ALTER TABLE COLUMN DROP
1df5875 has changed the way dependencies are dropped for this command
with inheritance trees, which impacts sepgsql. This just updates the
regression test output to take care of the failures and adapt to the new
code.
Reported by buildfarm member rhinoceros.
Author: Michael Paquier Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20191013101331.GC1434@paquier.xyz
Backpatch-through: 12
Tom Lane [Sun, 13 Oct 2019 19:48:26 +0000 (15:48 -0400)]
In the postmaster, rely on the signal infrastructure to block signals.
POSIX sigaction(2) can be told to block a set of signals while a
signal handler executes. Make use of that instead of manually
blocking and unblocking signals in the postmaster's signal handlers.
This should save a few cycles, and it also prevents recursive
invocation of signal handlers when many signals arrive in close
succession. We have seen buildfarm failures that seem to be due to
postmaster stack overflow caused by such recursion (exacerbated by
a Linux PPC64 kernel bug).
This doesn't change anything about the way that it works on Windows.
Somebody might consider adjusting port/win32/signal.c to let it work
similarly, but I'm not in a position to do that.
For the moment, just apply to HEAD. Possibly we should consider
back-patching this, but it'd be good to let it age awhile first.
Michael Paquier [Sun, 13 Oct 2019 08:51:55 +0000 (17:51 +0900)]
Fix dependency handling of column drop with partitioned tables
When dropping a column on a partitioned table which has one or more
partitioned indexes, the operation was failing as dependencies with
partitioned indexes using the column dropped were not getting removed in
a way consistent with the columns involved across all the relations part
of an inheritance tree.
This commit refactors the code executing column drop so as all the
columns from an inheritance tree to remove are gathered first, and
dropped all at the end. This way, we let the dependency machinery sort
out by itself the deletion of all the columns with the partitioned
indexes across a partition tree.
This issue has been introduced by 1d92a0c, so backpatch down to
REL_12_STABLE.
Author: Amit Langote, Michael Paquier Reviewed-by: Álvaro Herrera, Ashutosh Sharma
Discussion: https://postgr.es/m/CA+HiwqE9kuBsZ3b5pob2-cvE8ofzPWs-og+g8bKKGnu6b4-yTQ@mail.gmail.com
Backpatch-through: 12
Peter Eisentraut [Sat, 12 Oct 2019 19:17:34 +0000 (21:17 +0200)]
Fix use of term "verifier"
Within the context of SCRAM, "verifier" has a specific meaning in the
protocol, per RFCs. The existing code used "verifier" differently, to
mean whatever is or would be stored in pg_auth.rolpassword.
Fix this by using the term "secret" for this, following RFC 5803.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/be397b06-6e4b-ba71-c7fb-54cae84a7e18%402ndquadrant.com
Noah Misch [Sat, 12 Oct 2019 07:21:47 +0000 (00:21 -0700)]
AIX: Stop adding option -qsrcmsg.
With xlc v16.1.0, it causes internal compiler errors. With xlc versions
not exhibiting that bug, removing -qsrcmsg merely changes the compiler
error reporting format. Back-patch to 9.4 (all supported versions).
Fujii Masao [Fri, 11 Oct 2019 06:47:59 +0000 (15:47 +0900)]
Make crash recovery ignore restore_command and recovery_end_command settings.
In v11 or before, those settings could not take effect in crash recovery
because they are specified in recovery.conf and crash recovery always
starts without recovery.conf. But commit 2dedf4d9a8 integrated
recovery.conf into postgresql.conf and which unexpectedly allowed
those settings to take effect even in crash recovery. This is definitely
not good behavior.
To fix the issue, this commit makes crash recovery always ignore
restore_command and recovery_end_command settings.
Back-patch to v12 where the issue was added.
Author: Fujii Masao Reviewed-by: Peter Eisentraut
Discussion: https://postgr.es/m/e445616d-023e-a268-8aa1-67b8b335340c@pgmasters.net
Tom Lane [Thu, 10 Oct 2019 18:24:56 +0000 (14:24 -0400)]
Put back pqsignal() as an exported libpq symbol.
This reverts commit f7ab80285. Per discussion, we can't remove an
exported symbol without a SONAME bump, which we don't want to do.
In particular that breaks usage of current libpq.so with pre-9.3
versions of psql etc, which need libpq to export pqsignal().
As noted in that commit message, exporting the symbol from libpgport.a
won't work reliably; but actually we don't want to export src/port's
implementation anyway. Any pre-9.3 client is going to be expecting the
definition that pqsignal() had before 9.3, which was that it didn't
set SA_RESTART for SIGALRM. Hence, put back pqsignal() in a separate
source file in src/interfaces/libpq, and give it the old semantics.
Andres Freund [Thu, 10 Oct 2019 05:00:50 +0000 (22:00 -0700)]
Fix table rewrites that include a column without a default.
In c2fe139c201c I made ATRewriteTable() use tuple slots. Unfortunately
I did not notice that columns can be added in a rewrite that do not
have a default, when another column is added/altered requiring one.
Initialize columns to NULL again, and add tests.
Bug: #16038 Reported-By: anonymous
Author: Andres Freund
Discussion: https://postgr.es/m/16038-5c974541f2bf6749@postgresql.org
Backpatch: 12, where the bug was introduced in c2fe139c201c
Michael Paquier [Wed, 9 Oct 2019 04:30:43 +0000 (13:30 +0900)]
Flush logical mapping files with fd opened for read/write at checkpoint
The file descriptor was opened with read-only to fsync a regular file,
which would cause EBADFD errors on some platforms.
This is similar to the recent fix done by a586cc4b (which was broken by
me with 82a5649), except that I noticed this issue while monitoring the
backend code for similar mistakes. Backpatch to 9.4, as this has been
introduced since logical decoding exists as of b89e151.
Author: Michael Paquier Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20191006045548.GA14532@paquier.xyz
Backpatch-through: 9.4
Bruce Momjian [Wed, 9 Oct 2019 02:16:48 +0000 (22:16 -0400)]
pg_upgrade: clarify the database names in error files
Previously, the "Database:" label in the error file was unclear if the
label was a status report or the problem was _in_ the database. New
text is "In database:".
Bruce Momjian [Wed, 9 Oct 2019 01:49:08 +0000 (21:49 -0400)]
doc: improve docs so config value default units are clearer
Previously, our docs would say "Specifies the number of milliseconds"
but it wasn't clear that "milliseconds" was merely the default unit.
New text says "Specifies duration (defaults to milliseconds)", which is
clearer.
Remove some code for old unsupported versions of MSVC
As of d9dd406fe281d22d5238d3c26a7182543c711e74, we require MSVC 2013,
which means _MSC_VER >= 1800. This means that conditionals about
older versions of _MSC_VER can be removed or simplified.
Previous code was also in some cases handling MinGW, where _MSC_VER is
not defined at all, incorrectly, such as in pg_ctl.c and win32_port.h,
leading to some compiler warnings. This should now be handled better.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Michael Paquier [Tue, 8 Oct 2019 02:46:30 +0000 (11:46 +0900)]
Improve test coverage of pg_rewind
This includes new TAP tests for a couple of areas not covered yet and
some improvements:
- More coverage for --no-ensure-shutdown, the enforced recovery step and
--dry-run.
- Failures with option combinations and basic option checks.
- Removal of a duplicated comment.
Author: Alexey Kondratov, Michael Paquier
Discussion: https://postgr.es/m/20191007010651.GD14532@paquier.xyz
Tom Lane [Mon, 7 Oct 2019 16:39:09 +0000 (12:39 -0400)]
Check for too many postmaster children before spawning a bgworker.
The postmaster's code path for spawning a bgworker neglected to check
whether we already have the max number of live child processes. That's
a bit hard to hit, since it would necessarily be a transient condition;
but if we do, AssignPostmasterChildSlot() fails causing a postmaster
crash, as seen in a report from Bhargav Kamineni.
To fix, invoke canAcceptConnections() in the bgworker code path, as we
do in the other code paths that spawn children. Since we don't want
the same pmState tests in this case, add a child-process-type parameter
to canAcceptConnections() so that it can know what to do.
Back-patch to 9.5. In principle the same hazard exists in 9.4, but the
code is enough different that this patch wouldn't quite fix it there.
Given the tiny usage of bgworkers in that branch it doesn't seem worth
creating a variant patch for it.
Since 63bd0db12199c5df043e1dea0f2b574f622b3a4c we don't use tzname
anymore, so we don't need to check for it. Instead, just keep the
part of PGAC_STRUCT_TIMEZONE that we need, which is the check for
struct tm.tm_zone.
Tom Lane [Mon, 7 Oct 2019 14:39:07 +0000 (10:39 -0400)]
Hack pg_ctl to report postmaster's exit status.
Temporarily change pg_ctl so that the postmaster's exit status will
be printed (to the postmaster's stdout). This is to help identify
the cause of intermittent "postmaster exited during a parallel
transaction" failures seen on a couple of buildfarm members. This
change degrades pg_ctl's functionality in a couple of minor ways,
so we'll revert it once we've obtained the desired info.
Michael Paquier [Mon, 7 Oct 2019 00:07:22 +0000 (09:07 +0900)]
Improve handling and coverage of --no-ensure-shutdown in pg_rewind
This includes a couple of changes around the new behavior of pg_rewind
which enforces recovery to happen once on a cluster not shut down
cleanly:
- Some comments and documentation improvements.
- Shutdown the cluster to rewind with immediate mode in all the tests,
this allows to check after the forced recovery behavior which is wanted
as new default.
- Use -F for the forced recovery step, so as postgres does not use
fsync. This was useless as a final sync is done once the tool is done.
Author: Michael Paquier Reviewed-by: Alexey Kondratov
Discussion: https://postgr.es/m/20191004083721.GA1829@paquier.xyz
Tom Lane [Sun, 6 Oct 2019 18:14:45 +0000 (14:14 -0400)]
Doc: improve docs about pg_statistic_ext_data.
Commit aa087ec64 was a bit over-hasty about the doc changes needed
while splitting pg_statistic_ext_data off from pg_statistic_ext.
It duplicated one para and inserted another in what seems to me
to be the wrong section. Fix that up, and in passing do some minor
copy-editing.
Tom Lane [Sun, 6 Oct 2019 16:06:30 +0000 (12:06 -0400)]
Avoid trying to release a List's initial allocation via repalloc().
Commit 1cff1b95a included some code that supposed it could repalloc()
a memory chunk to a smaller size without risk of the chunk moving.
That was not a great idea, because it depended on undocumented behavior
of AllocSetRealloc, which commit c477f3e44 changed thereby breaking it.
(Not to mention that this code ought to work with other memory context
types, which might not work the same...) So get rid of the repalloc
calls, and instead just wipe the now-unused ListCell array and/or tell
Valgrind it's NOACCESS, as if we'd freed it.
In cases where the initial list allocation had been quite large, this
could represent an annoying waste of space. In principle we could
ameliorate that by allocating the initial cell array separately when
it exceeds some threshold. But that would complicate new_list() which
is hot code, and the returns would materialize only in narrow cases.
On balance I don't think it'd be worth it.
Tomas Vondra [Sat, 5 Oct 2019 18:49:39 +0000 (20:49 +0200)]
Change MemoryContextMemAllocated to return Size
Commit f2369bc610 switched most of the memory accounting from int64 to
Size, but it forgot to change the MemoryContextMemAllocated return type.
So this fixes that omission.
Noah Misch [Sat, 5 Oct 2019 17:05:05 +0000 (10:05 -0700)]
Report test_atomic_ops() failures consistently, via macros.
This prints the unexpected value in more failure cases, and it removes
forty-eight hand-maintained error messages. Back-patch to 9.5, which
introduced these tests.
Reviewed (in an earlier version) by Andres Freund.
Tom Lane [Sat, 5 Oct 2019 16:26:55 +0000 (12:26 -0400)]
Avoid use of wildcard in pg_waldump's .gitignore.
This would be all right, maybe, if it didn't also match a file that
definitely should not be ignored. We don't add rmgrs so often that
manual maintenance of this file list is impractical, so just write
out the list.
(I find the equivalent wildcard use in the Makefile pretty lazy and
unsafe as well, but will leave that alone until it actually causes a
problem.)
One of the upsert related tests is unstable (sometimes even hanging
until isolationtester's step timeout is reached). Based on preliminary
analysis that might be a problem outside of just that test, but not
really related to EPQ and triggers. Disable for now, to get the
buildfarm greener again.
Discussion: https://postgr.es/m/20191004222437.45qmglpto43pd3jb@alap3.anarazel.de
Backpatch: 9.6-, just like c8841199509.
Andres Freund [Fri, 4 Oct 2019 20:56:04 +0000 (13:56 -0700)]
Add isolation tests for the combination of EPQ and triggers.
As evidenced by bug #16036 this area is woefully under-tested. Add
fairly extensive tests for the combination.
Backpatch back to 9.6 - before that isolationtester was not capable
enough. While we don't backpatch tests all the time, future fixes to
trigger.c would potentially look different enough in 12+ from the
earlier branches that introducing bugs during backpatching is more
likely than normal. Also, it's just a crucial and undertested area of
the code.
Author: Andres Freund
Discussion: https://postgr.es/m/16036-28184c90d952fb7f@postgresql.org
Backpatch: 9.6-, the earliest these tests work
Andres Freund [Fri, 4 Oct 2019 18:59:34 +0000 (11:59 -0700)]
Fix crash caused by EPQ happening with a before update trigger present.
When ExecBRUpdateTriggers()'s GetTupleForTrigger() follows an EPQ
chain the former needs to run the result tuple through the junkfilter
again, and update the slot containing the new version of the tuple to
contain that new version. The input tuple may already be in the
junkfilter's output slot, which used to be OK - we don't need the
previous version anymore. Unfortunately ff11e7f4b9ae started to use
ExecCopySlot() to update newslot, and ExecCopySlot() doesn't support
copying a slot into itself, leading to a slot in a corrupt
state, which then can cause crashes or other symptoms.
Fix this by skipping the ExecCopySlot() when copying into itself.
While we could have easily made ExecCopySlot() handle that case, it
seems better to add an assert forbidding doing so instead. As the goal
of copying might be to make the contents of one slot independent from
another, it seems failure prone to handle doing so silently.
A follow-up commit will add tests for the obviously under-covered
combination of EPQ and triggers. Done as a separate commit as it might
make sense to backpatch them further than this bug.
Also remove confusion with confusing variable names for slots in
ExecBRDeleteTriggers() and ExecBRUpdateTriggers().
Bug: #16036 Reported-By: Антон Власов
Author: Andres Freund
Discussion: https://postgr.es/m/16036-28184c90d952fb7f@postgresql.org
Backpatch: 12-, where ff11e7f4b9ae was merged
Andres Freund [Fri, 4 Oct 2019 20:08:51 +0000 (13:08 -0700)]
Use a fd opened for read/write when syncing slots during startup, take 2.
Cribbing from dfbaed45975:
Some operating systems, including the reporter's windows, return EBADFD
or similar when fsync() is invoked on a O_RDONLY file descriptor.
Unfortunately RestoreSlotFromDisk() does exactly that; which causes
failures after restarts in at least some scenarios.
If you hit the bug the error message will be something like
ERROR: could not fsync file "pg_replslot/$name/state": Bad file descriptor
Simply use O_RDWR instead of O_RDONLY when opening the relevant file
descriptor to fix the bug.
Unfortunately this fix was undone in 82a5649fb9db. Re-apply, and add a
comment.
Bug: 16039 Reported-By: Hans Buschmann
Author: Andres Freund
Discussion: https://postgr.es/m/16039-196fc97cc05e141c@postgresql.org
Backpatch: 12-, as 82a5649fb9db
Andrew Dunstan [Fri, 4 Oct 2019 19:34:40 +0000 (15:34 -0400)]
Handle spaces in OpenSSL install location for MSVC
First, make sure that the .exe name is quoted when trying to get the
version number. Also, don't quote the lib name for using in the project
files if it's already been quoted. This second change applies to all
libraries, not just OpenSSL.
This has clearly been broken forever, so backpatch to all live branches.
projects / postgresql / log