Michael Paquier [Wed, 14 Nov 2018 01:33:10 +0000 (10:33 +0900)]
Add flag values in WAL description to all heap records
Hexadecimal is consistently used as format to not bloat too much the
output but keep it readable. This information is useful mainly for
debugging purposes with for example pg_waldump.
Author: Michael Paquier Reviewed-by: Nathan Bossart, Dmitry Dolgov, Andres Freund, Álvaro
Herrera
Discussion: https://postgr.es/m/20180413034734.GE1552@paquier.xyz
Michael Paquier [Wed, 14 Nov 2018 01:01:49 +0000 (10:01 +0900)]
Refactor code creating PartitionBoundInfo
The code building PartitionBoundInfo based on the constituent partition
data read from catalogs has been located in partcache.c, with a specific
set of routines dedicated to bound types, like sorting or bound data
creation. All this logic is moved to partbounds.c and relocates all the
bound-specific logistic into it, with partition_bounds_create() as
principal entry point.
Author: Amit Langote Reviewed-by: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/3f289da8-6d10-75fe-814a-635e8b191d43@lab.ntt.co.jp
Alvaro Herrera [Tue, 13 Nov 2018 21:12:39 +0000 (18:12 -0300)]
Add INSERT ON CONFLICT test on partitioned tables with transition table
This case was uncovered by existing tests, so breakage went undetected.
Make sure it remains stable.
Extracted from a larger patch by
Author: David Rowley Reviewed-by: Amit Langote
Discussion: https://postgr.es/m/CAKJS1f-aGCJ5H7_hiSs5PhWs6Obmj+vGARjGymqH1=o5PcrNnQ@mail.gmail.com
Tom Lane [Tue, 13 Nov 2018 19:54:41 +0000 (14:54 -0500)]
Fix realfailN lexer rules to not make assumptions about input format.
The realfail1 and realfail2 backup-prevention rules always returned
token type FCONST, ignoring the possibility that what we've scanned
is more appropriately described as ICONST. I think that at the
time that code was added, it might actually have been safe to not
distinguish; but since we started allowing AS-less aliases in SELECT
target lists, it's definitely legal to have a number immediately
followed by an identifier.
In the SELECT case, it seems there's no visible consequence because
make_const() will change the type back to integer anyway. But I'm
worried that there are other contexts, or will be in future, where
it's more important to get the constant's type right.
Hence, use process_integer_literal to correctly determine which
token type to return.
Arguably this is a bug fix, but given the lack of evidence of
user-visible problems, I'll refrain from back-patching.
Tom Lane [Tue, 13 Nov 2018 17:57:52 +0000 (12:57 -0500)]
Align ECPG lexer more closely with the core and psql lexers.
Make a bunch of basically-cosmetic changes to reduce the diffs between
the flex rules in scan.l, psqlscan.l, and pgc.l. Reorder some code,
adjust a lot of whitespace, sync some comments, make use of flex start
condition scopes to do that.
There are a few non-cosmetic changes in the ECPG lexer:
* Bring over the decimalfail rule (and support function
process_integer_literal) so that ECPG will lex "1..10" into
the same tokens as the backend would. I'm not sure this makes any
visible difference to users, but I'm not sure it doesn't, either.
* <xdc><<EOF>> gets its own rule so as to produce a more on-point
error message.
A table with OIDs that was the first in the dump output would not get
dumped with OIDs enabled. Fix that.
The reason was that the currWithOids flag was declared to be bool but
actually also takes a -1 value for "don't know yet". But under
stdbool.h semantics, that is coerced to true, so the required SET
default_with_oids command is not output again. Change the variable
type to char to fix that.
Amit Kapila [Tue, 13 Nov 2018 05:22:40 +0000 (10:52 +0530)]
Fix the initialization of atomic variables introduced by the
group clearing mechanism.
Commits 0e141c0fbb and baaf272ac9 introduced initialization of atomic
variables in InitProcess which means that it's not safe to look at those
for backends that aren't currently in use. Fix that by initializing them
during postmaster startup.
Reported-by: Andres Freund
Author: Amit Kapila
Backpatch-through: 9.6
Discussion: https://postgr.es/m/20181027104138.qmbbelopvy7cw2qv@alap3.anarazel.de
Thomas Munro [Tue, 13 Nov 2018 04:39:36 +0000 (17:39 +1300)]
Fix handling of HBA ldapserver with multiple hostnames.
Commit 35c0754f failed to handle space-separated lists of alternative
hostnames in ldapserver, when building a URI for ldap_initialize()
(OpenLDAP). Such lists need to be expanded to space-separated URIs.
Repair. Back-patch to 11, to fix bug report #15495.
Author: Thomas Munro Reported-by: Renaud Navarro
Discussion: https://postgr.es/m/15495-2c39fc196c95cd72%40postgresql.org
Thomas Munro [Tue, 13 Nov 2018 03:27:13 +0000 (16:27 +1300)]
Fix possible buffer overrun in hba.c.
Coverty reports a possible buffer overrun in the code that populates the
pg_hba_file_rules view. It may not be a live bug due to restrictions
on options that can be used together, but let's increase MAX_HBA_OPTIONS
and correct a nearby misleading comment.
Michael Paquier [Mon, 12 Nov 2018 23:59:41 +0000 (08:59 +0900)]
Remove CommandCounterIncrement() after processing ON COMMIT DELETE
This comes from f9b5b41, which is part of one the original commits that
implemented ON COMMIT actions. By looking at the truncation code, any
CCI needed happens locally when rebuilding indexes, so it looks safe to
just remove this final incrementation.
Author: Michael Paquier Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/20181109024731.GF2652@paquier.xyz
Tom Lane [Mon, 12 Nov 2018 16:50:28 +0000 (11:50 -0500)]
Simplify null-element handling in extension_config_remove().
There's no point in asking deconstruct_array() for a null-flags
array when we already checked the array has no nulls, and aren't
going to examine the output anyhow. Not asking for this output
should make the code marginally faster, and it's also more
robust since if there somehow were nulls, deconstruct_array()
would throw an error.
Tom Lane [Mon, 12 Nov 2018 16:19:04 +0000 (11:19 -0500)]
Limit the number of index clauses considered in choose_bitmap_and().
classify_index_clause_usage() is O(N^2) in the number of distinct index
qual clauses it considers, because of its use of a simple search list to
store them. For nearly all queries, that's fine because only a few clauses
will be considered. But Alexander Kuzmenkov reported a machine-generated
query with 80000 (!) index qual clauses, which caused this code to take
forever. Somewhat remarkably, this is the only O(N^2) behavior we now
have for such a query, so let's fix it.
We can get rid of the O(N^2) runtime for cases like this without much
damage to the functionality of choose_bitmap_and() by separating out
paths with "too many" qual or pred clauses, and deeming them to always
be nonredundant with other paths. Then their clauses needn't go into
the search list, so it doesn't get too long, but we don't lose the
ability to consider bitmap AND plans altogether. I set the threshold
for "too many" to be 100 clauses per path, which should be plenty to
ensure no change in planning behavior for normal queries.
There are other things we could do to make this go faster, but it's not
clear that it's worth any additional effort. 80000 qual clauses require
a whole lot of work in many other places, too.
The code's been like this for a long time, so back-patch to all supported
branches. The troublesome query only works back to 9.5 (in 9.4 it fails
with stack overflow in the parser); so I'm not sure that fixing this in
9.4 has any real-world benefit, but perhaps it does.
Peter Eisentraut [Mon, 12 Nov 2018 13:34:28 +0000 (14:34 +0100)]
doc: Small run-time pruning doc fix
A note in ddl.sgml used to mention that run-time pruning was only
implemented for Append. When we got MergeAppend support, this was
updated to mention that MergeAppend is supported too. This is
slightly weird as it's not all that obvious what exactly isn't
supported when we mention:
<para>
Both of these behaviors are likely to be changed in a future release
of <productname>PostgreSQL</productname>.
</para>
This patch updates this to mention that ModifyTable is unsupported,
which makes the above fragment make sense again.
Author: David Rowley <david.rowley@2ndquadrant.com>
Andres Freund [Sat, 10 Nov 2018 04:54:40 +0000 (20:54 -0800)]
Remove volatiles from {procarray,volatile}.c and fix memory ordering issue.
The use of volatiles in procarray.c largely originated from the time
when postgres did not have reliable compiler and memory
barriers. That's not the case anymore, so we can do better.
Several of the functions in procarray.c can be bottlenecks, and
removal of volatile yields mildly better code.
The new state, with explicit memory barriers, is also more
correct. The previous use of volatile did not actually deliver
sufficient guarantees on weakly ordered machines, in particular the
logic in GetNewTransactionId() does not look safe. It seems unlikely
to be a problem in practice, but worth fixing.
Thomas and I independently wrote a patch for this.
Reported-By: Andres Freund and Thomas Munro
Author: Andres Freund, with cherrypicked changes from a patch by Thomas Munro
Discussion:
https://postgr.es/m/20181005172955.wyjb4fzcdzqtaxjq@alap3.anarazel.de
https://postgr.es/m/CAEepm=1nff0x=7i3YQO16jLA2qw-F9O39YmUew4oq-xcBQBs0g@mail.gmail.com
Peter Eisentraut [Thu, 19 Jul 2018 06:37:32 +0000 (08:37 +0200)]
Apply RI trigger skipping tests also for DELETE
The tests added in cfa0f4255bb0f5550d37a01c4d8fe2966d20040c to skip
firing an RI trigger if any old key value is NULL can also be applied
for DELETE. This should give a performance gain in those cases, and it
also saves a lot of duplicate code in the actual RI triggers. (That
code was already dead code for the UPDATE cases.)
Peter Eisentraut [Thu, 19 Jul 2018 06:20:59 +0000 (08:20 +0200)]
Remove dead foreign key optimization code
The ri_KeysEqual() calls in the foreign-key trigger functions to
optimize away some updates are useless because since adfeef55cbcc5dc72a772777f88c1be05a70dfee those triggers are not enqueued
at all. (It's also not useful to keep these checks as some kind of
backstop, since it's also semantically correct to just run the full
check even with equal keys.)
Andres Freund [Sat, 10 Nov 2018 04:43:56 +0000 (20:43 -0800)]
Combine two flag tests in GetSnapshotData().
Previously the code checked PROC_IN_LOGICAL_DECODING and
PROC_IN_VACUUM separately. As the relevant variable is marked as
volatile, the compiler cannot combine the two tests. As
GetSnapshotData() is pretty hot in a number of workloads, it's
worthwhile to fix that.
It'd also be a good idea to get rid of the volatiles altogether. But
for one that's a larger patch, and for another, the code after this
change still seems at least as easy to read as before.
Author: Andres Freund
Discussion: https://postgr.es/m/20181005172955.wyjb4fzcdzqtaxjq@alap3.anarazel.de
Tom Lane [Sat, 10 Nov 2018 03:04:14 +0000 (22:04 -0500)]
Fix error-cleanup mistakes in exec_stmt_call().
Commit 15c729347 was a couple bricks shy of a load: we need to
ensure that expr->plan gets reset to NULL on any error exit,
if it's not supposed to be saved. Also ensure that the
stmt->target calculation gets redone if needed.
The easy way to exhibit a problem is to set up code that
violates the writable-argument restriction and then execute
it twice. But error exits out of, eg, setup_param_list()
could also break it. Make the existing PG_TRY block cover
all of that code to be sure.
Tom Lane [Sat, 10 Nov 2018 01:42:03 +0000 (20:42 -0500)]
Fix missing role dependencies for some schema and type ACLs.
This patch fixes several related cases in which pg_shdepend entries were
never made, or were lost, for references to roles appearing in the ACLs of
schemas and/or types. While that did no immediate harm, if a referenced
role were later dropped, the drop would be allowed and would leave a
dangling reference in the object's ACL. That still wasn't a big problem
for normal database usage, but it would cause obscure failures in
subsequent dump/reload or pg_upgrade attempts, taking the form of
attempts to grant privileges to all-numeric role names. (I think I've
seen field reports matching that symptom, but can't find any right now.)
Several cases are fixed here:
1. ALTER DOMAIN SET/DROP DEFAULT would lose the dependencies for any
existing ACL entries for the domain. This case is ancient, dating
back as far as we've had pg_shdepend tracking at all.
2. If a default type privilege applies, CREATE TYPE recorded the
ACL properly but forgot to install dependency entries for it.
This dates to the addition of default privileges for types in 9.2.
3. If a default schema privilege applies, CREATE SCHEMA recorded the
ACL properly but forgot to install dependency entries for it.
This dates to the addition of default privileges for schemas in v10
(commit ab89e465c).
Another somewhat-related problem is that when creating a relation
rowtype or implicit array type, TypeCreate would apply any available
default type privileges to that type, which we don't really want
since such an object isn't supposed to have privileges of its own.
(You can't, for example, drop such privileges once they've been added
to an array type.)
ab89e465c is also to blame for a race condition in the regression tests:
privileges.sql transiently installed globally-applicable default
privileges on schemas, which sometimes got absorbed into the ACLs of
schemas created by concurrent test scripts. This should have resulted
in failures when privileges.sql tried to drop the role holding such
privileges; but thanks to the bug fixed here, it instead led to dangling
ACLs in the final state of the regression database. We'd managed not to
notice that, but it became obvious in the wake of commit da906766c, which
allowed the race condition to occur in pg_upgrade tests.
To fix, add a function recordDependencyOnNewAcl to encapsulate what
callers of get_user_default_acl need to do; while the original call
sites got that right via ad-hoc code, none of the later-added ones
have. Also change GenerateTypeDependencies to generate these
dependencies, which requires adding the typacl to its parameter list.
(That might be annoying if there are any extensions calling that
function directly; but if there are, they're most likely buggy in the
same way as the core callers were, so they need work anyway.) While
I was at it, I changed GenerateTypeDependencies to accept most of its
parameters in the form of a Form_pg_type pointer, making its parameter
list a bit less unwieldy and mistake-prone.
The test race condition is fixed just by wrapping the addition and
removal of default privileges into a single transaction, so that that
state is never visible externally. We might eventually prefer to
separate out tests of default privileges into a script that runs by
itself, but that would be a bigger change and would make the tests
run slower overall.
Back-patch relevant parts to all supported branches.
Andres Freund [Sat, 10 Nov 2018 01:40:40 +0000 (17:40 -0800)]
Remove ineffective check against dropped columns from slot_getattr().
Before this commit slot_getattr() checked for dropped
columns (returning NULL in that case), but only after checking for
previously deformed columns. As slot_deform_tuple() does not contain
such a check, the check in slot_getattr() would often not have been
reached, depending on previous use of the slot.
These days locking and plan invalidation ought to ensure that dropped
columns are not accessed in query plans. Therefore this commit just
drops the insufficient check in slot_getattr(). It's possible that
we'll find some holes againt use of dropped columns, but if so, those
need to be addressed independent of slot_getattr(), as most accesses
don't go through that function anyway.
Author: Andres Freund
Discussion: https://postgr.es/m/20181107174403.zai7fedgcjoqx44p@alap3.anarazel.de
Andres Freund [Sat, 10 Nov 2018 01:19:39 +0000 (17:19 -0800)]
Don't require return slots for nodes without projection.
In a lot of nodes the return slot is not required. That can either be
because the node doesn't do any projection (say an Append node), or
because the node does perform projections but the projection is
optimized away because the projection would yield an identical row.
Slots aren't that small, especially for wide rows, so it's worthwhile
to avoid creating them. It's not possible to just skip creating the
slot - it's currently used to determine the tuple descriptor returned
by ExecGetResultType(). So separate the determination of the result
type from the slot creation. The work previously done internally
ExecInitResultTupleSlotTL() can now also be done separately with
ExecInitResultTypeTL() and ExecInitResultSlot(). That way nodes that
aren't guaranteed to need a result slot, can use
ExecInitResultTypeTL() to determine the result type of the node, and
ExecAssignScanProjectionInfo() (via
ExecConditionalAssignProjectionInfo()) determines that a result slot
is needed, it is created with ExecInitResultSlot().
Besides the advantage of avoiding to create slots that then are
unused, this is necessary preparation for later patches around tuple
table slot abstraction. In particular separating the return descriptor
and slot is a prerequisite to allow JITing of tuple deforming with
knowledge of the underlying tuple format, and to avoid unnecessarily
creating JITed tuple deforming for virtual slots.
This commit removes a redundant argument from
ExecInitResultTupleSlotTL(). While this commit touches a lot of the
relevant lines anyway, it'd normally still not worthwhile to cause
breakage, except that aforementioned later commits will touch *all*
ExecInitResultTupleSlotTL() callers anyway (but fits worse
thematically).
Author: Andres Freund
Discussion: https://postgr.es/m/20181105210039.hh4vvi4vwoq5ba2q@alap3.anarazel.de
Alvaro Herrera [Fri, 9 Nov 2018 16:08:00 +0000 (13:08 -0300)]
Indicate session name in isolationtester notices
When a session under isolationtester produces printable notices (NOTICE,
WARNING) we were just printing them unadorned, which can be confusing
when debugging. Prefix them with the session name, which makes things
clearer.
Author: Álvaro Herrera Reviewed-by: Hari Babu Kommi
Discussion: https://postgr.es/m/20181024213451.75nh3f3dctmcdbfq@alvherre.pgsql
Michael Paquier [Fri, 9 Nov 2018 01:03:22 +0000 (10:03 +0900)]
Fix dependency handling of partitions and inheritance for ON COMMIT
This commit fixes a set of issues with ON COMMIT actions when used on
partitioned tables and tables with inheritance children:
- Applying ON COMMIT DROP on a partitioned table with partitions or on a
table with inheritance children caused a failure at commit time, with
complains about the children being already dropped as all relations are
dropped one at the same time.
- Applying ON COMMIT DELETE on a partition relying on a partitioned
table which uses ON COMMIT DROP would cause the partition truncation to
fail as the parent is removed first.
The solution to the first problem is to handle the removal of all the
dependencies in one go instead of dropping relations one-by-one, based
on a suggestion from Álvaro Herrera. So instead all the relation OIDs
to remove are gathered and then processed in one round of multiple
deletions.
The solution to the second problem is to reorder the actions, with
truncation happening first and relation drop done after. Even if it
means that a partition could be first truncated, then immediately
dropped if its partitioned table is dropped, this has the merit to keep
the code simple as there is no need to do existence checks on the
relations to drop.
Contrary to a manual TRUNCATE on a partitioned table, ON COMMIT DELETE
does not cascade to its partitions. The ON COMMIT action defined on
each partition gets the priority.
Author: Michael Paquier Reviewed-by: Amit Langote, Álvaro Herrera, Robert Haas
Discussion: https://postgr.es/m/68f17907-ec98-1192-f99f-8011400517f5@lab.ntt.co.jp
Backpatch-through: 10
Tom Lane [Thu, 8 Nov 2018 22:33:25 +0000 (17:33 -0500)]
Disallow setting client_min_messages higher than ERROR.
Previously it was possible to set client_min_messages to FATAL or PANIC,
which had the effect of suppressing transmission of regular ERROR messages
to the client. Perhaps that seemed like a useful option in the past, but
the trouble with it is that it breaks guarantees that are explicitly made
in our FE/BE protocol spec about how a query cycle can end. While libpq
and psql manage to cope with the omission, that's mostly because they
are not very bright; client libraries that have more semantic knowledge
are likely to get confused. Notably, pgODBC doesn't behave very sanely.
Let's fix this by getting rid of the ability to set client_min_messages
above ERROR.
In HEAD, just remove the FATAL and PANIC options from the set of allowed
enum values for client_min_messages. (This change also affects
trace_recovery_messages, but that's OK since these aren't useful values
for that variable either.)
In the back branches, there was concern that rejecting these values might
break applications that are explicitly setting things that way. I'm
pretty skeptical of that argument, but accommodate it by accepting these
values and then internally setting the variable to ERROR anyway.
In all branches, this allows a couple of tiny simplifications in the
logic in elog.c, so do that.
Also respond to the point that was made that client_min_messages has
exactly nothing to do with the server's logging behavior, and therefore
does not belong in the "When To Log" subsection of the documentation.
The "Statement Behavior" subsection is a better match, so move it there.
Alvaro Herrera [Thu, 8 Nov 2018 19:22:09 +0000 (16:22 -0300)]
Revise attribute handling code on partition creation
The original code to propagate NOT NULL and default expressions
specified when creating a partition was mostly copy-pasted from
typed-tables creation, but not being a great match it contained some
duplicity, inefficiency and bugs.
This commit fixes the bug that NOT NULL constraints declared in the
parent table would not be honored in the partition. One reported issue
that is not fixed is that a DEFAULT declared in the child is not used
when inserting through the parent. That would amount to a behavioral
change that's better not back-patched.
This rewrite makes the code simpler:
1. instead of checking for duplicate column names in its own block,
reuse the original one that already did that;
2. instead of concatenating the list of columns from parent and the one
declared in the partition and scanning the result to (incorrectly)
propagate defaults and not-null constraints, just scan the latter
searching the former for a match, and merging sensibly. This works
because we know the list in the parent is already correct and there can
only be one parent.
This rewrite makes ColumnDef->is_from_parent unused, so it's removed
on branch master; on released branches, it's kept as an unused field in
order not to cause ABI incompatibilities.
This commit also adds a test case for creating partitions with
collations mismatching that on the parent table, something that is
closely related to the code being patched. No code change is introduced
though, since that'd be a behavior change that could break some (broken)
working applications.
Amit Langote wrote a less invasive fix for the original
NOT NULL/defaults bug, but while I kept the tests he added, I ended up
not using his original code. Ashutosh Bapat reviewed Amit's fix. Amit
reviewed mine.
Author: Álvaro Herrera, Amit Langote Reviewed-by: Ashutosh Bapat, Amit Langote Reported-by: Jürgen Strobel (bug #15212)
Discussion: https://postgr.es/m/152746742177.1291.9847032632907407358@wrigleys.postgresql.org
Andres Freund [Wed, 7 Nov 2018 19:08:45 +0000 (11:08 -0800)]
Move EEOP_*_SYSVAR evaluation out of line.
This mainly de-duplicates code. As evaluating a system variable isn't
the hottest path and the current inline implementation ends up calling
out to an external function anyway, this is OK from a performance POV.
The main motivation for de-duplicating is the upcoming slot
abstraction work, after which there's not guaranteed to be a HeapTuple
backing the slot.
Author: Andres Freund, Amit Khandekar
Discussion: https://postgr.es/m/20181105210039.hh4vvi4vwoq5ba2q@alap3.anarazel.de
Add another transfer mode --clone to pg_upgrade (besides the existing
--link and the default copy), using special file cloning calls. This
makes the file transfer faster and more space efficient, achieving
speed similar to --link mode without the associated drawbacks.
On Linux, file cloning is supported on Btrfs and XFS (if formatted with
reflink support). On macOS, file cloning is supported on APFS.
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Andres Freund [Wed, 7 Nov 2018 17:25:54 +0000 (09:25 -0800)]
Build HashState's hashkeys expression with the correct parent.
Previously the expressions were built with the HashJoinState as a
parent. That's incorrect.
Currently this does not appear to be harmful, but for the upcoming
'slot abstraction' work this proves to be problematic, as the
underlying slot types can differ between Hash and HashJoin. It's
possible that this already causes a problem, but I've not been able to
come up with a scenario. Therefore don't backpatch at this point.
Author: Andres Freund
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
Tom Lane [Wed, 7 Nov 2018 17:12:56 +0000 (12:12 -0500)]
Postpone calculating total_table_pages until after pruning/exclusion.
The planner doesn't make any use of root->total_table_pages until it
estimates costs of indexscans, so we don't need to compute it as
early as that's currently done. By doing the calculation between
set_base_rel_sizes and set_base_rel_pathlists, we can omit relations
that get removed from the query by partition pruning or constraint
exclusion, which seems like a more accurate basis for costing.
(Historical note: I think at the time this code was written, there
was not a separation between the "set sizes" and "set pathlists"
steps, so that this approach would have been impossible at the time.
But now that we have that separation, this is clearly the better way
to do things.)
Tom Lane [Tue, 6 Nov 2018 23:33:15 +0000 (18:33 -0500)]
Disable recheck_on_update optimization to avoid crashes.
The code added by commit c203d6cf8 causes a crash in at least one case,
where a potentially-optimizable expression index has a storage type
different from the input data type. A cursory code review turned up
numerous other problems that seem impractical to fix on short notice.
Andres argued for revert of that patch some time ago, and if additional
senior committers had been paying attention, that's likely what would
have happened, but we were not :-(
At this point we can't just revert, at least not in v11, because that would
mean an ABI break for code touching relcache entries. And we should not
remove the (also buggy) support for the recheck_on_update index reloption,
since it might already be used in some databases in the field. So this
patch just does the as-little-invasive-as-possible measure of disabling
the feature as though recheck_on_update were forced off for all indexes.
I also removed the related regression tests (which would otherwise fail)
and the user-facing documentation of the reloption.
We should undertake a more thorough code cleanup if the patch can't be
fixed, but not under the extreme time pressure of being already overdue
for 11.1 release.
Per report from Ondřej Bouda and subsequent private discussion among
pgsql-release.
Andrew Gierth [Tue, 6 Nov 2018 14:19:40 +0000 (14:19 +0000)]
Optimize nested ConvertRowtypeExpr nodes.
A ConvertRowtypeExpr is used to translate a whole-row reference of a
child to that of a parent. The planner produces nested
ConvertRowtypeExpr while translating whole-row reference of a leaf
partition in a multi-level partition hierarchy. Executor then
translates the whole-row reference from the leaf partition into all
the intermediate parent's whole-row references before arriving at the
final whole-row reference. It could instead translate the whole-row
reference from the leaf partition directly to the top-most parent's
whole-row reference skipping any intermediate translations.
Ashutosh Bapat, with tests by Kyotaro Horiguchi and some
editorialization by me. Reviewed by Andres Freund, Pavel Stehule,
Kyotaro Horiguchi, Dmitry Dolgov, Tom Lane.
Thomas Munro [Tue, 6 Nov 2018 20:51:50 +0000 (09:51 +1300)]
Use pg_pread() and pg_pwrite() for data files and WAL.
Cut down on system calls by doing random I/O using offset-based OS
routines where available. Remove the code for tracking the 'virtual'
seek position. The only reason left to call FileSeek() was to get
the file's size, so provide a new function FileSize() instead.
Author: Oskari Saarenmaa, Thomas Munro Reviewed-by: Thomas Munro, Jesper Pedersen, Tom Lane, Alvaro Herrera
Discussion: https://postgr.es/m/CAEepm=02rapCpPR3ZGF2vW=SBHSdFYO_bz_f-wwWJonmA3APgw@mail.gmail.com
Discussion: https://postgr.es/m/b8748d39-0b19-0514-a1b9-4e5a28e6a208%40gmail.com
Discussion: https://postgr.es/m/a86bd200-ebbe-d829-e3ca-0c4474b2fcb7%40ohmu.fi
Thomas Munro [Tue, 6 Nov 2018 20:50:01 +0000 (09:50 +1300)]
Provide pg_pread() and pg_pwrite() for random I/O.
Forward to POSIX pread() and pwrite(), or emulate them if unavailable.
The emulation is not perfect as the file position is changed, so
we'll put pg_ prefixes on the names to minimize the risk of confusion
in future patches that might inadvertently try to mix pread() and read()
on the same file descriptor.
Author: Thomas Munro Reviewed-by: Tom Lane, Jesper Pedersen
Discussion: https://postgr.es/m/CAEepm=02rapCpPR3ZGF2vW=SBHSdFYO_bz_f-wwWJonmA3APgw@mail.gmail.com
Tom Lane [Tue, 6 Nov 2018 18:25:24 +0000 (13:25 -0500)]
Rename rbtree.c functions to use "rbt" prefix not "rb" prefix.
The "rb" prefix is used by Ruby, so that our existing code results
in name collisions that break plruby. We discussed ways to prevent
that by adjusting dynamic linker options, but it seems that at best
we'd move the pain to other cases. Renaming to avoid the collision
is the only portable fix anyway. Fortunately, our rbtree code is
not (yet?) widely used --- in core, there's only a single usage
in GIN --- so it seems likely that we can get away with a rename.
I chose to do this basically as s/rb/rbt/g, except for places where
there already was a "t" after "rb". The patch could have been made
smaller by only touching linker-visible symbols, but it would have
resulted in oddly inconsistent-looking code. Better to make it look
like "rbt" was the plan all along.
Back-patch to v10. The rbtree.c code exists back to 9.5, but
rb_iterate() which is the actual immediate source of pain was added
in v10, so it seems like changing the names before that would have
more risk than benefit.
Tom Lane [Tue, 6 Nov 2018 15:57:51 +0000 (10:57 -0500)]
Remove useless symbol from Makefile.global.
I added HAVE_IPV6 to Makefile.global way back in commit 7703e55c3
so that we could transmit its value to the shell-script version of
initdb. Since initdb was rewritten in C, it's been finding that
out from pg_config.h instead, so this is useless. Keeping it here
just wastes configure and make cycles, plus it's a potential
two-sources-of-truth problem.
Michael Paquier [Tue, 6 Nov 2018 05:11:21 +0000 (14:11 +0900)]
Switch pg_promote to be parallel-safe
pg_promote uses nothing relying on a global state, so it is fine to mark
it as parallel-safe, conclusion based on a detailed analysis from Robert
Haas. This also fixes an inconsistency where pg_proc.dat missed to mark
the function with its previous value for proparallel, update which does
not matter now as the default is used.
Based on a discussion between multiple folks: Laurenz Albe, Robert Haas,
Amit Kapila, Tom Lane and myself.
Thomas Munro [Tue, 6 Nov 2018 03:11:12 +0000 (16:11 +1300)]
Remove dsm_resize() and dsm_remap().
These interfaces were never used in core, didn't handle failure of
posix_fallocate() correctly and weren't supported on all platforms.
We agreed to remove them in 12.
Author: Thomas Munro Reported-by: Andres Freund
Discussion: https://postgr.es/m/CAA4eK1%2B%3DyAFUvpFoHXFi_gm8YqmXN-TtkFH%2BVYjvDLS6-SFq-Q%40mail.gmail.com
Tom Lane [Mon, 5 Nov 2018 21:07:06 +0000 (16:07 -0500)]
Last-minute updates for release notes.
I removed the item about the pg_stat_statements change from
release-11.sgml, as part of a sweep to delete items already committed
in 11.0; but actually we'd best keep it to ensure that people who've
pg_upgraded their databases will take the requisite action. Also make
said action more visible by making it into its own para. Noted by
Jonathan Katz.
Andres Freund [Mon, 5 Nov 2018 20:02:25 +0000 (12:02 -0800)]
Fix copy-paste error in errhint() introduced in 691d79a07933.
Reported-By: Petr Jelinek
Discussion: https://postgr.es/m/c95a620b-34f0-7930-aeb5-f7ab804f26cb@2ndquadrant.com
Backpatch: 9.4-, like the previous commit
Tom Lane [Mon, 5 Nov 2018 17:02:27 +0000 (12:02 -0500)]
Remove unreferenced pg_opfamily entry.
The entry with OID 4035, for GIST jsonb_ops, is unused; apparently
it was added in preparation for index support that never materialized.
Remove it, and add a regression test case to detect future mistakes
of the same kind.
Michael Paquier [Mon, 5 Nov 2018 02:04:02 +0000 (11:04 +0900)]
Block creation of partitions with open references to its parent
When a partition is created as part of a trigger processing, it is
possible that the partition which just gets created changes the
properties of the table the executor of the ongoing command relies on,
causing a subsequent crash. This has been found possible when for
example using a BEFORE INSERT which creates a new partition for a
partitioned table being inserted to.
Any attempt to do so is blocked when working on a partition, with
regression tests added for both CREATE TABLE PARTITION OF and ALTER
TABLE ATTACH PARTITION.
Reported-by: Dmitry Shalashov
Author: Amit Langote Reviewed-by: Michael Paquier, Tom Lane
Discussion: https://postgr.es/m/15437-3fe01ee66bd1bae1@postgresql.org
Backpatch-through: 10
Michael Paquier [Mon, 5 Nov 2018 00:14:33 +0000 (09:14 +0900)]
Ignore partitioned tables when processing ON COMMIT DELETE ROWS
Those tables have no physical storage, making this option unusable with
partition trees as at commit time an actual truncation was attempted.
There are still issues with the way ON COMMIT actions are done when
mixing several action types, however this impacts as well inheritance
trees, so this issue will be dealt with later.
Reported-by: Rajkumar Raghuwanshi
Author: Amit Langote Reviewed-by: Michael Paquier, Tom Lane
Discussion: https://postgr.es/m/CAKcux6mhgcjSiB_egqEAEFgX462QZtncU8QCAJ2HZwM-wWGVew@mail.gmail.com
Tom Lane [Sun, 4 Nov 2018 19:50:55 +0000 (14:50 -0500)]
Fix ExecuteCallStmt to not scribble on the passed-in parse tree.
Modifying the parse tree at execution time is, or at least ought to be,
verboten. It seems quite difficult to actually cause a crash this way
in v11 (although you can exhibit it pretty easily in HEAD by messing
with plan_cache_mode). Nonetheless, it's risky, so fix and back-patch.
Tom Lane [Sun, 4 Nov 2018 18:25:39 +0000 (13:25 -0500)]
Fix bugs in plpgsql's handling of CALL argument lists.
exec_stmt_call() tried to extract information out of a CALL statement's
argument list without using expand_function_arguments(), apparently in
the hope of saving a few nanoseconds by not processing defaulted
arguments. It got that quite wrong though, leading to crashes with
named arguments, as well as failure to enforce writability of the
argument for a defaulted INOUT parameter. Fix and simplify the logic
by using expand_function_arguments() before examining the list.
Also, move the argument-examination to just after producing the CALL
command's plan, before invoking the called procedure. This ensures
that we'll track possible changes in the procedure's argument list
correctly, and avoids a hazard of the plan cache being flushed while
the procedure executes.
Also fix assorted falsehoods and omissions in associated documentation.
Per bug #15477 from Alexey Stepanov.
Patch by me, with some help from Pavel Stehule. Back-patch to v11.
Andres Freund [Sat, 3 Nov 2018 22:55:23 +0000 (15:55 -0700)]
Prevent generating EEOP_AGG_STRICT_INPUT_CHECK operations when nargs == 0.
This only became a problem with 4c640f4f38, which didn't synchronize
the value agg_strict_input_check.nargs is set to, with the guard
condition for emitting the operation.
Besides such instructions being unnecessary overhead, currently the
LLVM JIT provider doesn't support them. It seems more sensible to
avoid generating such instruction than supporting them. Add assertions
to make it easier to debug a potential further occurance.
Andres Freund [Sat, 3 Nov 2018 21:35:23 +0000 (14:35 -0700)]
Fix STRICT check for strict aggregates with NULL ORDER BY columns.
I (Andres) broke this unintentionally in 69c3936a14, by checking
strictness for all input expressions computed for an aggregate, rather
than just the input for the aggregate transition function.
Reported-By: Ondřej Bouda Bisected-By: Tom Lane Diagnosed-By: Andrew Gierth
Discussion: https://postgr.es/m/2a505161-2727-2473-7c46-591ed108ac52@email.cz
Backpatch: 11-, like 69c3936a14
Tom Lane [Sat, 3 Nov 2018 17:56:10 +0000 (13:56 -0400)]
Make ts_locale.c's character-type functions cope with UTF-16.
On Windows, in UTF8 database encoding, what char2wchar() produces is
UTF16 not UTF32, ie, characters above U+FFFF will be represented by
surrogate pairs. t_isdigit() and siblings did not account for this
and failed to provide a large enough result buffer. That in turn
led to bogus "invalid multibyte character for locale" errors, because
contrary to what you might think from char2wchar()'s documentation,
its Windows code path doesn't cope sanely with buffer overflow.
The solution for t_isdigit() and siblings is pretty clear: provide
a 3-wchar_t result buffer not 2.
char2wchar() also needs some work to provide more consistent, and more
accurately documented, buffer overrun behavior. But that's a bigger job
and it doesn't actually have any immediate payoff, so leave it for later.
Per bug #15476 from Kenji Uno, who deserves credit for identifying the
cause of the problem. Back-patch to all active branches.
Alvaro Herrera [Sat, 3 Nov 2018 16:23:40 +0000 (13:23 -0300)]
Fix tablespace handling for partitioned indexes
When creating partitioned indexes, the tablespace was not being saved
for the parent index. This meant that subsequently created partitions
would not use the right tablespace for their indexes.
ALTER INDEX SET TABLESPACE and ALTER INDEX ALL IN TABLESPACE raised
errors when tried; fix them too. This requires bespoke code for
ATExecCmd() that applies to the special case when the tablespace move is
just a catalog change.
Tom Lane [Sat, 3 Nov 2018 00:11:11 +0000 (20:11 -0400)]
First-draft release notes for 11.1.
As usual, the release notes for other branches will be made by cutting
these down, but put them up for community review first. Note that a
fair percentage of the entries apply only to prior branches because
their issue was already fixed in 11.0.
Tom Lane [Fri, 2 Nov 2018 22:54:00 +0000 (18:54 -0400)]
Yet further rethinking of build changes for macOS Mojave.
The solution arrived at in commit e74dd00f5 presumes that the compiler
has a suitable default -isysroot setting ... but further experience
shows that in many combinations of macOS version, XCode version, Xcode
command line tools version, and phase of the moon, Apple's compiler
will *not* supply a default -isysroot value.
We could potentially go back to the approach used in commit 68fc227dd,
but I don't have a lot of faith in the reliability or life expectancy of
that either. Let's just revert to the approach already shipped in 11.0,
namely specifying an -isysroot switch globally. As a partial response to
the concerns raised by Jakob Egger, adjust the contents of Makefile.global
to look like
This allows overriding the sysroot path at build time in a relatively
painless way.
Add documentation to installation.sgml about how to use the PG_SYSROOT
option. I also took the opportunity to document how to work around
macOS's "System Integrity Protection" feature.
Thomas Munro [Fri, 2 Nov 2018 22:05:35 +0000 (11:05 +1300)]
Fix NULL handling in multi-batch Parallel Hash Left Join.
NULL keys in left joins were skipped when building batch files.
Repair, by making the keep_nulls argument to ExecHashGetHashValue()
depend on whether this is a left outer join, as we do in other
paths.
Bug #15475. Thinko in 1804284042e. Back-patch to 11.
Reported-by: Paul Schaap Diagnosed-by: Andrew Gierth
Dicussion: https://postgr.es/m/15475-11a7a783fed72a36%40postgresql.org
Michael Paquier [Thu, 1 Nov 2018 22:59:24 +0000 (07:59 +0900)]
Lower error level from PANIC to FATAL when restoring slots at startup
When restoring slot information from disk at startup and filling in
shared memory information, the startup process would issue a PANIC
message if more slots are found than what max_replication_slots allows,
and then Postgres generates a core dump, recommending to increase
max_replication_slots. This gives users a switch to crash Postgres at
will by creating slots, lower the configuration to not support it, and
then restart it.
Making Postgres crash hard in this case is overdoing it just to give a
recommendation to users. So instead use a FATAL, which makes Postgres
fail to start without crashing, still giving the recommendation. This
is more consistent with what happens for prepared transactions for
example.
Author: Michael Paquier Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/20181030025109.GD1644@paquier.xyz
Peter Geoghegan [Thu, 1 Nov 2018 16:18:57 +0000 (09:18 -0700)]
Adjust trace_sort log messages.
The project message style guide dictates: "When citing the name of an
object, state what kind of object it is". The parallel CREATE INDEX
patch added a worker number to most of the trace_sort messages within
tuplesort.c without specifying the object type. Bring these messages
into compliance with the style guide.
We're still treating a leader or serial Tuplesortstate as having worker
number -1. trace_sort is a developer option, and these two cases are
highly comparable, so this seems appropriate.
Per complaint from Tom Lane.
Discussion: https://postgr.es/m/8330.1540831863@sss.pgh.pa.us
Backpatch: 11-, where parallel CREATE INDEX was introduced.
Andres Freund [Wed, 31 Oct 2018 21:47:41 +0000 (14:47 -0700)]
Disallow starting server with insufficient wal_level for existing slot.
Previously it was possible to create a slot, change wal_level, and
restart, even if the new wal_level was insufficient for the
slot. That's a problem for both logical and physical slots, because
the necessary WAL records are not generated.
This removes a few tests in newer versions that, somewhat
inexplicably, whether restarting with a too low wal_level worked (a
buggy behaviour!).
Reported-By: Joshua D. Drake
Author: Andres Freund
Discussion: https://postgr.es/m/20181029191304.lbsmhshkyymhw22w@alap3.anarazel.de
Backpatch: 9.4-, where replication slots where introduced
Tom Lane [Wed, 31 Oct 2018 21:04:42 +0000 (17:04 -0400)]
Fix memory leak in repeated SPGIST index scans.
spgendscan neglected to pfree all the memory allocated by spgbeginscan.
It's possible to get away with that in most normal queries, since the
memory is allocated in the executor's per-query context which is about
to get deleted anyway; but it causes severe memory leakage during
creation or filling of large exclusion-constraint indexes.
Also, document that amendscan is supposed to free what ambeginscan
allocates. The docs' lack of clarity on that point probably caused this
bug to begin with. (There is discussion of changing that API spec going
forward, but I don't think it'd be appropriate for the back branches.)
Per report from Bruno Wolff. It's been like this since the beginning,
so back-patch to all active branches.
In HEAD, also fix an independent leak caused by commit 2a6368343
(allocating memory during spgrescan instead of spgbeginscan, which
might be all right if it got cleaned up, but it didn't). And do a bit
of code beautification on that commit, too.
Tom Lane [Wed, 31 Oct 2018 13:47:53 +0000 (09:47 -0400)]
Sync our copy of the timezone library with IANA release tzcode2018g.
This patch absorbs an upstream fix to "zic" for a recently-introduced
bug that made it output data that some 32-bit clients couldn't read.
Given the current source data, the bug only manifests in zones with
leap seconds, which we don't generate, so that there's no actual
change in our installed timezone data files from this. Still, in
case somebody uses our copy of "zic" to do something else, it seems
best to apply the fix promptly.
Also, update the README's notes about converting upstream code to
our conventions.
Tom Lane [Tue, 30 Oct 2018 19:26:11 +0000 (15:26 -0400)]
Fix interaction of CASE and ArrayCoerceExpr.
An array-type coercion appearing within a CASE that has a constant
(after const-folding) test expression was mangled by the planner, causing
all the elements of the resulting array to be equal to the coerced value
of the CASE's test expression. This is my oversight in commit c12d570fa:
that changed ArrayCoerceExpr to use a subexpression involving a
CaseTestExpr, and I didn't notice that eval_const_expressions needed an
adjustment to keep from folding such a CaseTestExpr to a constant when
it's inside a suitable CASE.
This is another in what's getting to be a depressingly long line of bugs
associated with misidentification of the referent of a CaseTestExpr.
We're overdue to redesign that mechanism; but any such fix is unlikely
to be back-patchable into v11. As a stopgap, fix eval_const_expressions
to do what it must here. Also add a bunch of comments pointing out the
restrictions and assumptions that are needed to make this work at all.
Also fix a related oversight: contain_context_dependent_node() was not
aware of the relationship of ArrayCoerceExpr to CaseTestExpr. That was
somewhat fail-soft, in that the outcome of a wrong answer would be to
prevent optimizations that could have been made, but let's fix it while
we're at it.
Per bug #15471 from Matt Williams. Back-patch to v11 where the faulty
logic came in.
Michael Paquier [Tue, 30 Oct 2018 02:38:35 +0000 (11:38 +0900)]
Consolidate cross-option checks in pg_restore
This moves one check for conflicting options from the archive restore
code to the main function where other similar checks are performed.
Also reword the error message to be consistent with other messages.
The only option combination impacted is --create specified with
--single-transaction, and informing the caller at an early step saves
from opening the archive worked on. A TAP test is added for this
combination.
Author: Daniel Gustafsson Reviewed-by: Fabien Coelho
Discussion: https://postgr.es/m/616808BD-4B59-4E6C-97A9-7317F62D5570@yesql.se
Michael Paquier [Tue, 30 Oct 2018 01:25:06 +0000 (10:25 +0900)]
Add pg_partition_tree to display information about partitions
This new function is useful to display a full tree of partitions with a
partitioned table given in output, and avoids the need of any complex
WITH RECURSIVE query when looking at partition trees which are
deep multiple levels.
It returns a set of records, one for each partition, containing the
partition's name, its immediate parent's name, a boolean value telling
if the relation is a leaf in the tree and an integer telling its level
in the partition tree with given table considered as root, beginning at
zero for the root, and incrementing by one each time the scan goes one
level down.
Author: Amit Langote Reviewed-by: Jesper Pedersen, Michael Paquier, Robert Haas
Discussion: https://postgr.es/m/8d00e51a-9a51-ad02-d53e-ba6bf50b2e52@lab.ntt.co.jp
Peter Eisentraut [Mon, 29 Oct 2018 10:39:44 +0000 (11:39 +0100)]
Exclude temporary directories from pgindent
Exclude tmp_check and tmp_install from pgindent. In a fully-built
tree, pgindent would spend a lot of time digging through these
directories and ends up re-indenting installed header files.
Michael Paquier [Mon, 29 Oct 2018 07:38:54 +0000 (16:38 +0900)]
Improve description of pg_attrdef in documentation
The reference to pg_attribute is switched to a link, which is more
useful for the html documentation. The conditions under which a default
value is defined for a given column are made more general.
Author: Daniel Gustafsson Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/0E8748E3-8B7D-445E-9ABA-09DA5C7345CC@yesql.se