The previous coding forgot to apply shell quoting to the socket
directory and the data folder, leading to failures when running
pg_upgrade. This refactors the code generating the pg_ctl command
starting clusters to use a more correct shell quoting. Failures are
easier to trigger in 12 and newer versions by using a value of
--socketdir that includes quotes, but it is also possible to cause
failures with quotes included in the default socket directory used by
pg_upgrade or the data folders of the clusters involved in the
upgrade.
As 9.4 is going to be EOL'd with the next minor release, nobody is
likely going to upgrade to it now so this branch is not included in the
set of branches fixed.
Author: Michael Paquier
Reviewed-by: Álvaro Herrera, Noah Misch
Backpatch-through: 9.5
If we failed to fork a worker process, or create a communication pipe
for one, WaitForTerminatingWorkers would suffer an assertion failure
if assert-enabled, otherwise crash or go into an infinite loop. This
was a consequence of not accounting for the startup condition where
we've not yet forked all the workers.
The original bug was that ParallelBackupStart would set workerStatus to
WRKR_IDLE before it had successfully forked a worker. I made things
worse in commit b7b8cc0cf by not understanding the undocumented fact
that the WRKR_TERMINATED state was also meant to represent the case
where a worker hadn't been started yet: I changed enum T_WorkerStatus
so that *all* the worker slots were initially in WRKR_IDLE state. But
this wasn't any more broken in practice, since even one slot in the
wrong state would keep WaitForTerminatingWorkers from terminating.
In v10 and later, introduce an explicit T_WorkerStatus value for
worker-not-started, in hopes of preventing future oversights of the
same ilk. Before that, just document that WRKR_TERMINATED is supposed
to cover that case (partly because it wasn't actively broken, and
partly because the enum is exposed outside parallel.c in those branches,
so there's microscopically more risk involved in changing it).
In all branches, introduce a WORKER_IS_RUNNING status test macro
to hide which T_WorkerStatus values mean that, and be more careful
not to access ParallelSlot fields till we're sure they're valid.
Per report from Vignesh C, though this is my patch not his.
Back-patch to all supported branches.
Discussion: https://postgr.es/m/CALDaNm1Luv-E3sarR+-unz-BjchquHHyfP+YC+2FS2pt_J+wxg@mail.gmail.com
sigTermHandler() tried to be careful to invoke only operations that
are safe to do in a signal handler. But for some reason we forgot
that exit(3) is not among those, because it calls atexit handlers
that might do various random things. (pg_dump itself installs no
atexit handlers, but e.g. OpenSSL does.) That led to crashes or
lockups when attempting to terminate a parallel dump or restore
via a signal.
Fix by calling _exit() instead.
Per bug #16199 from Raúl Marín. Back-patch to all supported branches.
Discussion: https://postgr.es/m/16199-cb2f121146a96f9b@postgresql.org
On Windows, we use CMD.EXE to redirect the postmaster's stdout/stderr
into a log file. CMD.EXE will open that file with non-sharing-friendly
parameters, and the file will remain open for a short time after the
postmaster has removed postmaster.pid. This can result in an
ERROR_SHARING_VIOLATION failure if we attempt to start a new postmaster
immediately with the same log file (e.g. during "pg_ctl restart").
This seems to explain intermittent buildfarm failures we've been seeing
on Windows machines.
To fix, just open and close the log file using our own pgwin32_open(),
which will wait if necessary to avoid the failure. (Perhaps someday
we should stop using CMD.EXE, but that would be a far more complex
patch, and it doesn't seem worth the trouble ... yet.)
Back-patch to v12. This only solves the problem when frontend fopen()
is redirected to pgwin32_fopen(), which has only been true since commit
0ba06e0bf. Hence, no point in back-patching further, unless we care
to back-patch that change too.
Diagnosis and patch by Alexander Lakhin (bug #16154).
Discussion: https://postgr.es/m/16154-1ccf0b537b24d5e0@postgresql.org
On Windows, all the hosts spawned by the TAP tests bind to 127.0.0.1.
Hence, if there is a port conflict, starting a cluster would immediately
fail. One of the test scripts of pg_ctl initializes a node without
PostgresNode.pm, using the default port 5432. This could cause
unexpected startup failures in the tests if an independent server was up
and running on the same host (the reverse is also possible, though more
unlikely). Fix this issue by assigning properly a free port to the node
configured, in the same range used as for the other nodes part of the
tests.
Author: Michael Paquier
Reviewed-by: Andrew Dunstan
Discussion: https://postgr.es/m/20191202031444.GC1696@paquier.xyz
Backpatch-through: 11
pg_upgrade needs to check whether certain non-upgradable data types
appear anywhere on-disk in the source cluster. It knew that it has
to check for these types being contained inside domains and composite
types; but it somehow overlooked that they could be contained in
arrays and ranges, too. Extend the existing recursive-containment
query to handle those cases.
We probably should have noticed this oversight while working on
commit 0ccfc2822 and follow-ups, but we failed to :-(. The whole
thing's possibly a bit overdesigned, since we don't really expect
that any of these types will appear on disk; but if we're going to
the effort of doing a recursive search then it's silly not to cover
all the possibilities.
While at it, refactor so that we have only one copy of the search
logic, not three-and-counting. Also, to keep the branches looking
more alike, back-patch the output wording change of commit 1634d3615.
Back-patch to all supported branches.
Discussion: https://postgr.es/m/31473.1573412838@sss.pgh.pa.us
This patch adopts the overflow check logic introduced by commit cbdb8b4c0
into two more places. interval_mul() failed to notice if it computed a
new microseconds value that was one more than INT64_MAX, and pgbench's
double-to-int64 logic had the same sorts of edge-case problems that
cbdb8b4c0 fixed in the core code.
To make this easier to get right in future, put the guts of the checks
into new macros in c.h, and add commentary about how to use the macros
correctly.
Back-patch to all supported branches, as we did with the previous fix.
Yuya Watari
Discussion: https://postgr.es/m/CAJ2pMkbkkFw2hb9Qb1Zj8d06EhWAQXFLy73St4qWv6aX=vqnjw@mail.gmail.com
The code only compared two triggers' names and namespaces (the latter
being the owning table's schema). This could result in falling back
to an OID-based sort of similarly-named triggers on different tables.
We prefer to avoid that, so add a comparison of the table names too.
(The sort order is thus table namespace, trigger name, table name,
which is a bit odd, but it doesn't seem worth contorting the code
to work around that.)
Likewise for policy objects, in 9.5 and up.
Complaint and fix by Benjie Gillam. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/CAMThMzEEt2mvBbPgCaZ1Ap1N-moGn=Edxmadddjq89WG4NpPtQ@mail.gmail.com
The additional newline seems to have accidentally been introduced in
2c03216d831, in 9.5. The newline is only issued when an FPW is
present for the block reference.
While there could be an argument that removing the newlines in the
back branches could cause a problem for somebody parsing the
pg_waldump output, the likelihood of that seems small enough. It seems
at least equally likely that the randomness of when newlines are
issued causes problems.
Author: Andres Freund
Discussion: https://postgr.es/m/20191029233341.4gnyau7e5v2lh5sc@alap3.anarazel.de
Backpatch: 9.5, like 2c03216d831.
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.
Author: Takayuki Tsunakawa
Reviewed-by: Dilip Kumar, Kyotaro Horiguchi, Michael Paquier
Discussion: https://postgr.es/m/OSBPR01MB2503CE851940C17DE44AE3D9FE6F0@OSBPR01MB2503.jpnprd01.prod.outlook.com
Backpatch-through: 12
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.
Discussion: https://postgr.es/m/20191005224333.GA9738@alvherre.pgsql
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
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
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
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.)
Per bug #16042 from Denis Stuchalin.
Discussion: https://postgr.es/m/16042-c174ee692ac21cbd@postgresql.org
Even if --dry-run mode was specified, the control file was getting
updated, preventing follow-up runs of pg_rewind to work properly on the
target data folder. The origin of the problem came from the refactoring
done by ce6afc6.
Author: Alexey Kondratov
Discussion: https://postgr.es/m/7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru
Backpatch-through: 12
Modern versions of msys2 have changed the treatment of "cmd /c" so that
the runtime will try to convert the switch to a native file path. This
patch adds a setting to inhibit that behaviour.
Discussion: https://postgr.es/m/3227042f-cfcc-745a-57dd-fb8c471f8ddf@2ndQuadrant.com
Backpatch to all live branches.
Since the addition of fsync requests in bc34223 to make base backup data
consistent on disk once pg_basebackup finishes, each tablespace tar file
is individually flushed once completed, with an additional flush of the
parent directory when the base backup finishes. While holding a
connection to the server, a fsync request taking a long time may cause a
failure of the base backup, which is annoying for any integration. A
recent example of breakage can involve tcp_user_timeout, but
wal_sender_timeout can cause similar problems.
While reviewing the code, there was a second issue causing too many
fsync requests to be done for the same WAL data. As recursive fsyncs
are done at the end of the backup for both the plain and tar formats
from the base target directory where everything is written, it is fine
to disable fsyncs when fetching or streaming WAL.
Reported-by: Ryohei Takahashi
Author: Michael Paquier
Reviewed-by: Ryohei Takahashi
Discussion: https://postgr.es/m/OSBPR01MB4550DAE2F8C9502894A45AAB82BE0@OSBPR01MB4550.jpnprd01.prod.outlook.com
Backpatch-through: 10
Clarify in the help output and documentation that -n, -t etc. take a
"pattern" rather than a "schema" or "table" etc. This was especially
confusing now that the new pg_dumpall --exclude-database option was
documented with "pattern" and the others not, even though they all
behave the same.
Discussion: https://www.postgresql.org/message-id/flat/b85f3fa1-b350-38d1-1893-4f7911bd7310%402ndquadrant.com
After an unexpected connection loss and successful reconnection,
psql neglected to resynchronize its internal state about the server,
such as server version. Ordinarily we'd be reconnecting to the same
server and so this isn't really necessary, but there are scenarios
where we do need to update --- one example is where we have a list
of possible connection targets and they're not all alike.
Define "resynchronize" as including connection_warnings(), so that
this case acts the same as \connect. This seems useful; for example,
if the server version did change, the user might wish to know that.
An attuned user might also notice that the new connection isn't
SSL-encrypted, for example, though this approach isn't especially
in-your-face about such changes. Although this part is a behavioral
change, it only affects interactive sessions, so it should not break
any applications.
Also, in do_connect, make sure that we desynchronize correctly when
abandoning an old connection in non-interactive mode.
These problems evidently are the result of people patching only one
of the two places where psql deals with connection changes, so insert
some cross-referencing comments in hopes of forestalling future bugs
of the same ilk.
Lastly, in Windows builds, issue codepage mismatch warnings only at
startup, not during reconnections. psql's codepage can't change
during a reconnect, so complaining about it again seems like useless
noise.
Peter Billen and Tom Lane. Back-patch to all supported branches.
Discussion: https://postgr.es/m/CAMTXbE8e6U=EBQfNSe01Ej17CBStGiudMAGSOPaw-ALxM-5jXg@mail.gmail.com
In this case, the transfer uses a libpq connection, which is subject to
the timeout parameters set at system level, and this can make the rewind
operation suddenly canceled which is not good for automation. One
workaround to such issues would be to use PGOPTIONS to enforce the
wanted timeout parameters, but that's annoying, and for example pg_dump,
which can run potentially long-running queries disables all types of
timeouts.
lock_timeout and statement_timeout are the ones which can cause problems
now. Note that pg_rewind does not use transactions, so disabling
idle_in_transaction_session_timeout is optional, but it feels safer to
do so for the future.
This is back-patched down to 9.5. idle_in_transaction_session_timeout
is only present since 9.6.
Author: Alexander Kukushkin
Discussion: https://postgr.es/m/CAFh8B=krcVXksxiwVQh1SoY+ziJ-JC=6FcuoBL3yce_40Es5_g@mail.gmail.com
Backpatch-through: 9.5
FD_SETSIZE needs to be declared before winsock2.h, or it is possible to
run into buffer overflow issues when using --jobs. This is similar to
pgbench's solution done in a23c641.
This has been introduced by 71d84ef, and older versions have been using
the default value of FD_SETSIZE, defined at 64.
Per buildfarm member jacana, but this impacts all Windows animals
running the TAP tests. I have reproduced the failure locally to check
the patch.
Author: Michael Paquier
Reviewed-by: Andrew Dunstan
Discussion: https://postgr.es/m/20190826054000.GE7005@paquier.xyz
Backpatch-through: 9.5
Previously 'uname -r' on Msys2 reported a kernele release starting with
2. The latest version starts with 3. In commit 1638623f we specifically
looked for one starting with 2. This is now changed to look for any
digit between 2 and 9.
backpatch to release 10.
On msys2, 'uname -s' reports a string starting MSYS instead on MINGW
as happens on msys1. Treat these both the same way. This reverts
608a710195a4b in favor of a more general solution.
Backpatch to all live branches.
When trying to use a high number of jobs, vacuumdb has only checked for
a maximum number of jobs used, causing confusing failures when running
out of file descriptors when the jobs open connections to Postgres.
This commit changes the error handling so as we do not check anymore for
a maximum number of allowed jobs when parsing the option value with
FD_SETSIZE, but check instead if a file descriptor is within the
supported range when opening the connections for the jobs so as this is
detected at the earliest time possible.
Also, improve the error message to give a hint about the number of jobs
recommended, using a wording given by the reviewers of the patch.
Reported-by: Andres Freund
Author: Michael Paquier
Reviewed-by: Andres Freund, Álvaro Herrera, Tom Lane
Discussion: https://postgr.es/m/20190818001858.ho3ev4z57fqhs7a5@alap3.anarazel.de
Backpatch-through: 9.5
POSIX permits getopt() to advance optind beyond argc when the last
argv entry is an option that requires an argument and hasn't got one.
It seems that no major platforms actually do that, but musl does,
so that something like "psql -f" would crash with that libc.
Add a check that optind is in range before trying to look at the
possibly-bogus option.
Report and fix by Quentin Rameau. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/20190825100617.GA6087@fifth.space
Commit 07b39083c inserted an unconditional reference to pg_opfamily,
which of course fails on servers predating that catalog. Fortunately,
the case it's trying to solve can't occur on such old servers (AFAIK).
Hence, just skip the additional code when the source predates 8.3.
Per bug #15955 from sly. Back-patch to all supported branches,
like the previous patch.
Discussion: https://postgr.es/m/15955-1daa2e676e903d87@postgresql.org
OWNER_TO was used for the completion, which is not a supported grammar,
but OWNER TO is.
This error has been introduced by d37b816, so backpatch down to 9.6.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/7ab243e0-116d-3e44-d120-76b3df7abefd@gmail.com
Backpatch-through: 9.6
Since pg_dump doesn't treat the member operators and functions of operator
classes/families (that is, the pg_amop and pg_amproc entries, not the
underlying operators/functions) as separate dumpable objects, it missed
their dependency information. I think this was safe when the code was
designed, because the default object sorting rule emits operators and
functions before opclasses, and there were no dependency types that could
mess that up. However, the introduction of range types in 9.2 broke it:
now a type can have a dependency on an opclass, allowing dependency rules
to push the opclass before the type and hence before custom operators.
Lacking any information showing that it shouldn't do so, pg_dump emitted
the objects in the wrong order.
Fix by teaching getDependencies() to translate pg_depend entries for
pg_amop/amproc rows to look like dependencies for their parent opfamily.
I added a regression test for this in HEAD/v12, but not further back;
life is too short to fight with 002_pg_dump.pl.
Per bug #15934 from Tom Gottfried. Back-patch to all supported branches.
Discussion: https://postgr.es/m/15934-58b8c8ab7a09ea15@postgresql.org
This has been wrong ever since pg_rewind was added. The if-branch just
above this, where we print the same error with an extra message supplied
by XLogReadRecord() got this right, but the variable name was wrong in the
else-branch. As a consequence, the error printed the WAL position as
0/0 if there was an error reading a WAL file.
Backpatch to 9.5, where pg_rewind was added.
Some platforms create a file named "localtime" in the system
timezone directory, making it a copy or link to the active time
zone file. If Postgres is built with --with-system-tzdata, initdb
will see that file as an exact match to localtime(3)'s behavior,
and it may decide that "localtime" is the most preferred spelling of
the active zone. That's a very bad choice though, because it's
neither informative, nor portable, nor stable if someone changes
the system timezone setting. Extend the preference logic added by
commit e3846a00c so that we will prefer any other zone file that
matches localtime's behavior over "localtime".
On the same logic, also discriminate against "posixrules", which
is another not-really-a-zone file that is often present in the
timezone directory. (Since we install "posixrules" but not
"localtime", this change can affect the behavior of Postgres
with or without --with-system-tzdata.)
Note that this change doesn't prevent anyone from choosing these
pseudo-zones if they really want to (i.e., by setting TZ for initdb,
or modifying the timezone GUC later on). It just prevents initdb
from preferring these zone names when there are multiple matches to
localtime's behavior.
Since we generally prefer to keep timezone-related behavior the
same in all branches, and since this is arguably a bug fix,
back-patch to all supported branches.
Discussion: https://postgr.es/m/CADT4RqCCnj6FKLisvT8tTPfTP4azPhhDFJqDF1JfBbOH5w4oyQ@mail.gmail.com
Discussion: https://postgr.es/m/27991.1560984458@sss.pgh.pa.us
describeOneTableDetails issued a partition-constraint-fetching query
for every table, even ones it knows perfectly well are not partitions.
To add insult to injury, it then proceeded to leak the empty PGresult
if the table wasn't a partition. Doing that a lot of times might
amount to a meaningful leak, so this seems like a back-patchable bug.
Fix that, and also fix a related PGresult leak in the partition-parent
case (though that leak would occur only if we got no row, which is
unexpected).
Minor code beautification too, to make this code look more like the
pre-existing code around it.
Back-patch the whole change into v12. However, the fact that we already
know whether the table is a partition dates only to commit 1af25ca0c;
back-patching the relevant changes from that is probably more churn
than is justified in released branches. Hence, in v11 and v10, just
do the minimum to fix the PGresult leaks.
Noted while messing around with adjacent code for yesterday's \d
improvements.
