Peter Geoghegan [Tue, 13 Aug 2019 18:54:26 +0000 (11:54 -0700)]
Use PageIndexTupleOverwrite() within nbtree.
Use the PageIndexTupleOverwrite() bufpage.c routine within nbtree
instead of deleting a tuple and re-inserting its replacement. This
makes the intent of affected code slightly clearer. It also makes
CREATE INDEX slightly faster, since there is no longer a need to shift
every leaf page's line pointer array back and forth during index builds.
Author: Peter Geoghegan, Anastasia Lubennikova Reviewed-By: Anastasia Lubennikova
Discussion: https://postgr.es/m/CAH2-Wz=Zk=B9+Vwm376WuO7YTjFc2SSskifQm4Nme3RRRPtOSQ@mail.gmail.com
Alvaro Herrera [Tue, 13 Aug 2019 14:26:04 +0000 (10:26 -0400)]
Don't constraint-exclude partitioned tables as much
We only need to invoke constraint exclusion on partitioned tables when
they are a partition, and they themselves contain a default partition;
it's not necessary otherwise, and it's expensive, so avoid it. Also, we
were trying once for each clause separately, but we can do it for all
the clauses at once.
While at it, centralize setting of RelOptInfo->partition_qual instead of
computing it in slightly different ways in different places.
Per complaints from Simon Riggs about 4e85642d935e; reviewed by Yuzuko
Hosoya, Kyotaro Horiguchi.
Author: Amit Langote. I (Álvaro) again mangled the patch somewhat.
Discussion: https://postgr.es/m/CANP8+j+tMCY=nEcQeqQam85=uopLBtX-2vHiLD2bbp7iQQUKpA@mail.gmail.com
Michael Paquier [Tue, 13 Aug 2019 04:53:41 +0000 (13:53 +0900)]
Fix inconsistencies and typos in the tree, take 10
This addresses some issues with unnecessary code comments, fixes various
typos in docs and comments, and removes some orphaned structures and
definitions.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/9aabc775-5494-b372-8bcb-4dfc0bd37c68@gmail.com
Michael Paquier [Tue, 13 Aug 2019 01:55:19 +0000 (10:55 +0900)]
Fix random regression failure in test case "temp"
This test case could fail because of an incorrect result ordering when
looking up at pg_class entries. This commit adds an ORDER BY to the
culprit query. The cause of the failure was likely caused by a plan
switch. By default, the planner would likely choose an index-only scan
or an index scan, but even a small change in the startup cost could have
caused a bitmap heap scan to be chosen, causing the failure.
While on it, switch some filtering quals to a regular expression as per
an idea of Tom Lane. As previously shaped, the quals would have
selected any relations whose name begins with "temp". And that could
cause failures if another test running in parallel began to use similar
relation names.
Per report from buildfarm member anole, though the failure was very
rare. This test has been introduced by 319a810, so backpatch down to
v10.
Peter Geoghegan [Mon, 12 Aug 2019 22:21:32 +0000 (15:21 -0700)]
amcheck: Skip unlogged relations during recovery.
contrib/amcheck failed to consider the possibility that unlogged
relations will not have any main relation fork files when running in hot
standby mode. This led to low-level "can't happen" errors that complain
about the absence of a relfilenode file.
To fix, simply skip verification of unlogged index relations during
recovery. In passing, add a direct check for the presence of a main
fork just before verification proper begins, so that we cleanly verify
the presence of the main relation fork file.
Author: Andrey Borodin, Peter Geoghegan Reported-By: Andrey Borodin Diagnosed-By: Andrey Borodin
Discussion: https://postgr.es/m/DA9B33AC-53CB-4643-96D4-7A0BBC037FA1@yandex-team.ru
Backpatch: 10-, where amcheck was introduced.
Tom Lane [Mon, 12 Aug 2019 17:15:47 +0000 (13:15 -0400)]
Fix planner's test for case-foldable characters in ILIKE with ICU.
As coded, the ICU-collation path in pattern_char_isalpha() failed
to consider regular ASCII letters to be case-varying. This led to
like_fixed_prefix treating too much of an ILIKE pattern as being a
fixed prefix, so that indexscans derived from an ILIKE clause might
miss entries that they should find.
Per bug #15892 from James Inform. This is an oversight in the original
ICU patch (commit eccfef81e), so back-patch to v10 where that came in.
Tom Lane [Mon, 12 Aug 2019 15:58:35 +0000 (11:58 -0400)]
Remove EState.es_range_table_array.
Now that list_nth is O(1), there's no good reason to maintain a
separate array of RTE pointers rather than indexing into
estate->es_range_table. Deleting the array doesn't save all that
much either; but just on cleanliness grounds, it's better not to
have duplicate representations of the identical information.
Tom Lane [Mon, 12 Aug 2019 15:20:18 +0000 (11:20 -0400)]
Rationalize use of list_concat + list_copy combinations.
In the wake of commit 1cff1b95a, the result of list_concat no longer
shares the ListCells of the second input. Therefore, we can replace
"list_concat(x, list_copy(y))" with just "list_concat(x, y)".
To improve call sites that were list_copy'ing the first argument,
or both arguments, invent "list_concat_copy()" which produces a new
list sharing no ListCells with either input. (This is a bit faster
than "list_concat(list_copy(x), y)" because it makes the result list
the right size to start with.)
In call sites that were not list_copy'ing the second argument, the new
semantics mean that we are usually leaking the second List's storage,
since typically there is no remaining pointer to it. We considered
inventing another list_copy variant that would list_free the second
input, but concluded that for most call sites it isn't worth worrying
about, given the relative compactness of the new List representation.
(Note that in cases where such leakage would happen, the old code
already leaked the second List's header; so we're only discussing
the size of the leak not whether there is one. I did adjust two or
three places that had been troubling to free that header so that
they manually free the whole second List.)
Take into account pg_server_to_any() may return input string "as is".
Reported-by: Andrew Dunstan, Thomas Munro
Discussion: https://postgr.es/m/0ed83a33-d900-466a-880a-70ef456c721f%402ndQuadrant.com
Author: Alexander Korotkov, Thomas Munro
Backpatch-through: 12
Tom Lane [Sun, 11 Aug 2019 22:55:32 +0000 (18:55 -0400)]
Partially revert "Insert temporary debugging output in regression tests."
This reverts much of commit f03a9ca4366d064d89b7cf7ed75d4e43f2ed0667,
but leaves the relpages/reltuples probe in select_parallel.sql.
The pg_stat_all_tables probes are unstable enough to be annoying,
and it no longer seems likely that they will teach us anything more
about the underlying problem. I'd still like some more confirmation
though that the observed plan instability is caused by VACUUM leaving
relpages/reltuples as zero for one of these tables.
We have implemented jsonpath string comparison using default database locale.
However, standard requires us to compare Unicode codepoints. This commit
implements that, but for performance reasons we still use per-byte comparison
for "==" operator. Thus, for consistency other comparison operators do per-byte
comparison if Unicode codepoints appear to be equal.
In some edge cases, when same Unicode codepoints have different binary
representations in database encoding, we diverge standard to achieve better
performance of "==" operator. In future to implement strict standard
conformance, we can do normalization of input JSON strings.
Original patch was written by Nikita Glukhov, rewritten by me.
Reported-by: Markus Winand
Discussion: https://postgr.es/m/8B7FA3B4-328D-43D7-95A8-37B8891B8C78%40winand.at
Author: Nikita Glukhov, Alexander Korotkov
Backpatch-through: 12
Tom Lane [Sat, 10 Aug 2019 15:30:11 +0000 (11:30 -0400)]
Fix "ANALYZE t, t" inside a transaction block.
This failed with either "tuple already updated by self" or "duplicate
key value violates unique constraint", depending on whether the table
had previously been analyzed or not. The reason is that ANALYZE tried
to insert or update the same pg_statistic rows twice, and there was no
CommandCounterIncrement between. So add one. The same case works fine
outside a transaction block, because then there's a whole transaction
boundary between, as a consequence of the way VACUUM works.
This issue has been latent all along, but the problem was unreachable
before commit 11d8d72c2 added the ability to specify multiple tables
in ANALYZE. We could, perhaps, alternatively fix it by adding code to
de-duplicate the list of VacuumRelations --- but that would add a
lot of overhead to work around dumb commands, so it's not attractive.
Per bug #15946 from Yaroslav Schekin. Back-patch to v11.
(Note: in v11 I also back-patched the test added by commit 23224563d;
otherwise the problem doesn't manifest in the test I added, because
"vactst" is empty when the tests for multiple ANALYZE targets are
reached. That seems like not a very good thing anyway, so I did this
rather than rethinking the choice of test case.)
Peter Geoghegan [Sat, 10 Aug 2019 00:06:45 +0000 (17:06 -0700)]
Rename tuplesort.c's SortTuple.tupindex field.
Rename the "tupindex" field from tuplesort.c's SortTuple struct to
"srctape", since it can only ever be used to store a source/input tape
number when merging external sort runs. This has been the case since
commit 8b304b8b72b, which removed replacement selection sort from
tuplesort.c.
Tom Lane [Fri, 9 Aug 2019 16:33:43 +0000 (12:33 -0400)]
Cosmetic improvements in setup of planner's per-RTE arrays.
Merge setup_append_rel_array into setup_simple_rel_arrays. There's no
particularly good reason to keep them separate, and it's inconsistent
with the lack of separation in expand_planner_arrays. The only apparent
benefit was that the fast path for trivial queries in query_planner()
doesn't need to set up the append_rel_array; but all we're saving there
is an if-test and NULL assignment, which surely ought to be negligible.
Michael Paquier [Fri, 9 Aug 2019 02:05:14 +0000 (11:05 +0900)]
Refactor logic to remove trailing CR/LF characters from strings
b654714 has reworked the way trailing CR/LF characters are removed from
strings. This commit introduces a new routine in common/string.c and
refactors the code so as the logic is in a single place, mostly.
Author: Michael Paquier Reviewed-by: Bruce Momjian
Discussion: https://postgr.es/m/20190801031820.GF29334@paquier.xyz
Peter Geoghegan [Thu, 8 Aug 2019 20:20:44 +0000 (13:20 -0700)]
Update obsolete tuplesort READTUP() comment.
READTUP() routines do not and cannot use the resettable "tuplecontext"
memory context, since it is deleted when merging begins. Update an
obsolete comment that claimed otherwise. This was an oversight in
commit e94568ecc10.
I left PG_CMD_PUTS around even though it could be handled by
PG_CMD_PRINTF since PG_CMD_PUTS is sometimes called with non-literal
arguments, and so that would create a potential problem if such a
string contained percent signs.
Tom Lane [Wed, 7 Aug 2019 22:09:28 +0000 (18:09 -0400)]
Doc: document permissions required for ANALYZE.
VACUUM's reference page had this text, but ANALYZE's didn't. That's
a clear oversight given that section 5.7 explicitly delegates the
responsibility to define permissions requirements to the individual
commands' man pages.
Per gripe from Isaac Morland. Back-patch to all supported branches.
Alvaro Herrera [Wed, 7 Aug 2019 16:21:22 +0000 (12:21 -0400)]
Apply constraint exclusion more generally in partitioning
We were applying constraint exclusion on the partition constraint when
generating pruning steps for a clause, but only for the rather
restricted situation of them being boolean OR operators; however it is
possible to have differently shaped clauses that also benefit from
constraint exclusion. This applies particularly to the default
partition since their constraints are in essence a long list of OR'ed
subclauses ... but it applies to other cases too. So in certain cases
we're scanning partitions that we don't need to.
Remove the specialized code in OR clauses, and add a generally
applicable test of the clause refuting the partition constraint; mark
the whole pruning operation as contradictory if it hits.
This has the unwanted side-effect of testing some (most? all?)
constraints more than once if constraint_exclusion=on. That seems
unavoidable as far as I can tell without some additional work, but
that's not the recommended setting for that parameter anyway.
However, because this imposes additional processing cost for all
queries using partitioned tables, I decided not to backpatch this
change.
Author: Amit Langote, Yuzuko Hosoya, Álvaro Herrera
Reviewers: Shawn Wang, Thibaut Madeleine, Yoshikazu Imai, Kyotaro
Horiguchi; they were also uncredited reviewers for commit 489247b0e615.
Discussion: https://postgr.es/m/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf@lab.ntt.co.jp
In serializable mode, heap_hot_search_buffer() incorrectly acquired a
predicate lock on the root tuple, not the returned tuple that satisfied
the visibility checks. As explained in README-SSI, the predicate lock does
not need to be copied or extended to other tuple versions, but for that to
work, the correct, visible, tuple version must be locked in the first
place.
The original SSI commit had this bug in it, but it was fixed back in 2013,
in commit 81fbbfe335. But unfortunately, it was reintroduced a few months
later in commit b89e151054. Wising up from that, add a regression test
to cover this, so that it doesn't get reintroduced again. Also, move the
code that sets 't_self', so that it happens at the same time that the
other HeapTuple fields are set, to make it more clear that all the code in
the loop operate on the "current" tuple in the chain, not the root tuple.
Bug spotted by Andres Freund, analysis and original fix by Thomas Munro,
test case and some additional changes to the fix by Heikki Linnakangas.
Backpatch to all supported versions (9.4).
Michael Paquier [Wed, 7 Aug 2019 09:16:31 +0000 (18:16 +0900)]
Fix some incorrect parsing of time with time zone strings
When parsing a timetz string with a dynamic timezone abbreviation or a
timezone not specified, it was possible to generate incorrect timestamps
based on a date which uses some non-initialized variables if the input
string did not specify fully a date to parse. This is already checked
when a full timezone spec is included in the input string, but the two
other cases mentioned above missed the same checks.
This gets fixed by generating an error as this input is invalid, or in
short when a date is not fully specified.
Valgrind was complaining about this problem.
Bug: #15910
Author: Alexander Lakhin
Discussion: https://postgr.es/m/15910-2eba5106b9aa0c61@postgresql.org
Backpatch-through: 9.4
Michael Paquier [Wed, 7 Aug 2019 01:28:16 +0000 (10:28 +0900)]
Adjust tuple data lookup logic in multi-insert logical decoding
As of now, logical decoding of a multi-insert has been scanning all
xl_multi_insert_tuple entries only if XLH_INSERT_CONTAINS_NEW_TUPLE was
getting set in the record. This is not an issue on HEAD as multi-insert
records are not used for system catalogs, but the logical decoding logic
includes all the code necessary to handle that properly, except that the
code missed to iterate correctly over all xl_multi_insert_tuple entries
when the flag is not set. Hence, when trying to use multi-insert for
system catalogs, an assertion would be triggered.
An upcoming patch is going to make use of multi-insert for system
catalogs, and this fixes the logic to make sure that all entries are
scanned correctly without softening the existing assertions.
Reported-by: Daniel Gustafsson
Author: Michael Paquier Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/CBFFD532-C033-49EB-9A5A-F67EAEE9EB0B@yesql.se
Tom Lane [Tue, 6 Aug 2019 22:04:51 +0000 (18:04 -0400)]
Fix intarray's GiST opclasses to not fail for empty arrays with <@.
contrib/intarray considers "arraycol <@ constant-array" to be indexable,
but its GiST opclass code fails to reliably find index entries for empty
array values (which of course should trivially match such queries).
This is because the test condition to see whether we should descend
through a non-leaf node is wrong.
Unfortunately, empty array entries could be anywhere in the index,
as these index opclasses are currently designed. So there's no way
to fix this except by lobotomizing <@ indexscans to scan the whole
index ... which is what this patch does. That's pretty unfortunate:
the performance is now actually worse than a seqscan, in most cases.
We'd be better off to remove <@ from the GiST opclasses entirely,
and perhaps a future non-back-patchable patch will do so.
In the meantime, applications whose performance is adversely impacted
have a couple of options. They could switch to a GIN index, which
doesn't have this bug, or they could replace "arraycol <@ constant-array"
with "arraycol <@ constant-array AND arraycol && constant-array".
That will provide about the same performance as before, and it will find
all non-empty subsets of the given constant-array, which is all that
could reliably be expected of the query before.
While at it, add some more regression test cases to improve code
coverage of contrib/intarray.
In passing, adjust resize_intArrayType so that when it's returning an
empty array, it uses construct_empty_array for that rather than
cowboy hacking on the input array. While the hack produces an array
that looks valid for most purposes, it isn't bitwise equal to empty
arrays produced by other code paths, which could have subtle odd
effects. I don't think this code path is performance-critical
enough to justify such shortcuts. (Back-patch this part only as far
as v11; before commit 01783ac36 we were not careful about this in
other intarray code paths either.)
Back-patch the <@ fixes to all supported versions, since this was
broken from day one.
Patch by me; thanks to Alexander Korotkov for review.
Tom Lane [Tue, 6 Aug 2019 21:08:07 +0000 (17:08 -0400)]
Save Kerberos and LDAP daemon logs where the buildfarm can find them.
src/test/kerberos and src/test/ldap try to run private authentication
servers, which of course might fail. The logs from these servers
were being dropped into the tmp_check/ subdirectory, but they should
be put in tmp_check/log/, because the buildfarm will only capture
log files in that subdirectory. Without the log output there's
little hope of diagnosing buildfarm failures related to these servers.
Backpatch to v11 where these test suites were added.
Peter Geoghegan [Mon, 5 Aug 2019 18:47:34 +0000 (11:47 -0700)]
Show specific OID suggestion in unused_oids output.
Commit a6417078 established a new project policy around OID assignment:
new patches are encouraged to choose a random OID in the 8000..9999
range when a manually-assigned OID is required (if multiple OIDs are
required, a consecutive block of OIDs starting from the random point
should be used). Catalog entries added by committed patches that use
OIDs from this "unstable" range are renumbered after feature freeze.
This practice minimizes OID collisions among concurrently-developed
patches.
Show a specific random OID suggestion when the unused_oids script is
run. This makes it easy for patch authors to use a random OID from the
unstable range, per the new policy.
Author: Julien Rouhaud, Peter Geoghegan Reviewed-By: Tom Lane
Discussion: https://postgr.es/m/CAH2-WzkkRs2ScmuBQ7xWi7xzp7fC1B3w0Nt8X+n4rBw5k+Z=zA@mail.gmail.com
Tom Lane [Mon, 5 Aug 2019 15:20:21 +0000 (11:20 -0400)]
Fix choice of comparison operators for cross-type hashed subplans.
Commit bf6c614a2 rearranged the lookup of the comparison operators
needed in a hashed subplan, and in so doing, broke the cross-type
case: it caused the original LHS-vs-RHS operator to be used to compare
hash table entries too (which of course are all of the RHS type).
This leads to C functions being passed a Datum that is not of the
type they expect, with the usual hazards of crashes and unauthorized
server memory disclosure.
For the set of hashable cross-type operators present in v11 core
Postgres, this bug is nearly harmless on 64-bit machines, which
may explain why it escaped earlier detection. But it is a live
security hazard on 32-bit machines; and of course there may be
extensions that add more hashable cross-type operators, which
would increase the risk.
Reported by Andreas Seltenreich. Back-patch to v11 where the
problem came in.
Noah Misch [Mon, 5 Aug 2019 14:48:41 +0000 (07:48 -0700)]
Require the schema qualification in pg_temp.type_name(arg).
Commit aa27977fe21a7dfa4da4376ad66ae37cb8f0d0b5 introduced this
restriction for pg_temp.function_name(arg); do likewise for types
created in temporary schemas. Programs that this breaks should add
"pg_temp." schema qualification or switch to arg::type_name syntax.
Back-patch to 9.4 (all supported versions).
Michael Paquier [Mon, 5 Aug 2019 06:35:16 +0000 (15:35 +0900)]
Add safeguards in LSN, numeric and float calculation for custom errors
Those data types use parsing and/or calculation wrapper routines which
can generate some generic error messages in the event of a failure. The
caller of these routines can also pass a pointer variable settable by
the routine to track if an error has happened, letting the caller decide
what to do in the event of an error and what error message to generate.
Those routines have been slacking the initialization of the tracking
flag, which can be confusing when reading the code, so add some
safeguards against calls of these parsing routines which could lead to a
dubious result.
The LSN parsing gains an assertion to make sure that the tracking flag
is set, while numeric and float paths initialize the flag to a saner
state.
Author: Jeevan Ladhe Reviewed-by: Álvaro Herrera, Michael Paquier
Discussion: https://postgr.es/m/CAOgcT0NOM9oR0Hag_3VpyW0uF3iCU=BDUFSPfk9JrWXRcWQHqw@mail.gmail.com
As committed, statement sampling used the existing duration threshold
(log_min_duration_statement) when decide which statements to sample.
The issue is that even the longest statements are subject to sampling,
and so may not end up logged. An improvement was proposed, introducing
a second duration threshold, but it would not be backwards compatible.
So we've decided to revert this feature - the separate threshold should
be part of the feature itself.
As committed, statement sampling used the existing duration threshold
(log_min_duration_statement) when decide which statements to sample.
The issue is that even the longest statements are subject to sampling,
and so may not end up logged. An improvement was proposed, introducing
a second duration threshold, but it would not be backwards compatible.
So we've decided to revert this feature - the separate threshold should
be part of the feature itself.
Tom Lane [Sun, 4 Aug 2019 18:05:34 +0000 (14:05 -0400)]
Fix handling of "undef" in contrib/jsonb_plperl.
Perl has multiple internal representations of "undef", and just
testing for SvTYPE(x) == SVt_NULL doesn't recognize all of them,
leading to "cannot transform this Perl type to jsonb" errors.
Use the approved test SvOK() instead.
Report and patch by Ivan Panchenko. Back-patch to v11 where
this module was added.
Tom Lane [Sun, 4 Aug 2019 17:07:12 +0000 (13:07 -0400)]
Avoid picking already-bound TCP ports in kerberos and ldap test suites.
src/test/kerberos and src/test/ldap need to run a private authentication
server of the relevant type, for which they need a free TCP port.
They were just picking a random port number in 48K-64K, which works
except when something's already using the particular port. Notably,
the probability of failure rises dramatically if one simply runs those
tests in a tight loop, because each test cycle leaves behind a bunch of
high ports that are transiently in TIME_WAIT state.
To fix, split out the code that PostgresNode.pm already had for
identifying a free TCP port number, so that it can be invoked to choose
a port for the KDC or LDAP server. This isn't 100% bulletproof, since
conceivably something else on the machine could grab the port between
the time we check and the time we actually start the server. But that's
a pretty short window, so in practice this should be good enough.
Back-patch to v11 where these test suites were added.
Alvaro Herrera [Sun, 4 Aug 2019 15:18:45 +0000 (11:18 -0400)]
Improve pruning of a default partition
When querying a partitioned table containing a default partition, we
were wrongly deciding to include it in the scan too early in the
process, failing to exclude it in some cases. If we reinterpret the
PruneStepResult.scan_default flag slightly, we can do a better job at
detecting that it can be excluded. The change is that we avoid setting
the flag for that pruning step unless the step absolutely requires the
default partition to be scanned (in contrast with the previous
arrangement, which was to set it unless the step was able to prune it).
So get_matching_partitions() must explicitly check the partition that
each returned bound value corresponds to in order to determine whether
the default one needs to be included, rather than relying on the flag
from the final step result.
Author: Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp> Reviewed-by: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Discussion: https://postgr.es/m/00e601d4ca86$932b8bc0$b982a340$@lab.ntt.co.jp
Michael Paquier [Sun, 4 Aug 2019 02:18:57 +0000 (11:18 +0900)]
Refactor BuildIndexInfo() with the new makeIndexInfo()
This portion of the code got forgotten in 7cce159 which has introduced a
new routine to build this node, and this finishes the unification of the
places where IndexInfo is initialized.
Author: Michael Paquier
Discussion: https://postgr.es/m/20190801041322.GA3435@paquier.xyz
Andres Freund [Fri, 2 Aug 2019 07:02:46 +0000 (00:02 -0700)]
Fix representation of hash keys in Hash/HashJoin nodes.
In 5f32b29c1819 I changed the creation of HashState.hashkeys to
actually use HashState as the parent (instead of HashJoinState, which
was incorrect, as they were executed below HashState), to fix the
problem of hashkeys expressions otherwise relying on slot types
appropriate for HashJoinState, rather than HashState as would be
correct. That reliance was only introduced in 12, which is why it
previously worked to use HashJoinState as the parent (although I'd be
unsurprised if there were problematic cases).
Unfortunately that's not a sufficient solution, because before this
commit, the to-be-hashed expressions referenced inner/outer as
appropriate for the HashJoin, not Hash. That didn't have obvious bad
consequences, because the slots containing the tuples were put into
ecxt_innertuple when hashing a tuple for HashState (even though Hash
doesn't have an inner plan).
There are less common cases where this can cause visible problems
however (rather than just confusion when inspecting such executor
trees). E.g. "ERROR: bogus varno: 65000", when explaining queries
containing a HashJoin where the subsidiary Hash node's hash keys
reference a subplan. While normally hashkeys aren't displayed by
EXPLAIN, if one of those expressions references a subplan, that
subplan may be printed as part of the Hash node - which then failed
because an inner plan was referenced, and Hash doesn't have that.
It seems quite possible that there's other broken cases, too.
Fix the problem by properly splitting the expression for the HashJoin
and Hash nodes at plan time, and have them reference the proper
subsidiary node. While other workarounds are possible, fixing this
correctly seems easy enough. It was a pretty ugly hack to have
ExecInitHashJoin put the expression into the already initialized
HashState, in the first place.
I decided to not just split inner/outer hashkeys inside
make_hashjoin(), but also to separate out hashoperators and
hashcollations at plan time. Otherwise we would have ended up having
two very similar loops, one at plan time and the other during executor
startup. The work seems to more appropriately belong to plan time,
anyway.
Reported-By: Nikita Glukhov, Alexander Korotkov
Author: Andres Freund Reviewed-By: Tom Lane, in an earlier version
Discussion: https://postgr.es/m/CAPpHfdvGVegF_TKKRiBrSmatJL2dR9uwFCuR+teQ_8tEXU8mxg@mail.gmail.com
Backpatch: 12-
Michael Paquier [Fri, 2 Aug 2019 00:51:12 +0000 (09:51 +0900)]
Fix format truncation issue from ECPG test
This fixes one warning generated by GCC and present in the test case
array part of ECPG. This likely got missed in past fixes like 3a4b891
because the compilation of those tests is not done by default.
Jeff Davis [Thu, 1 Aug 2019 21:52:43 +0000 (14:52 -0700)]
Allow simplehash to use already-calculated hash values.
Add _lookup_hash and _insert_hash functions for callers that have
already calculated the hash value of the key.
The immediate use case is for hash algorithms that write to disk in
partitions. The hash value can be calculated once, used to perform a
lookup, used to select the partition, then written to the partition
along with the tuple. When the tuple is read back, the hash value does
not need to be recalculated.
Tom Lane [Thu, 1 Aug 2019 22:50:22 +0000 (18:50 -0400)]
Allow functions-in-FROM to be pulled up if they reduce to constants.
This allows simplification of the plan tree in some common usage
patterns: we can get rid of a join to the function RTE.
In principle we could pull up any immutable expression, but restricting
it to Consts avoids the risk that multiple evaluations of the expression
might cost more than we can save. (Possibly this could be improved in
future --- but we've more or less promised people that putting a function
in FROM guarantees single evaluation, so we'd have to tread carefully.)
To do this, we need to rearrange when eval_const_expressions()
happens for expressions in function RTEs. I moved it to
inline_set_returning_functions(), which already has to iterate over
every function RTE, and in consequence renamed that function to
preprocess_function_rtes(). A useful consequence is that
inline_set_returning_function() no longer has to do this for itself,
simplifying that code.
In passing, break out pull_up_simple_subquery's code that knows where
everything that needs pullup_replace_vars() processing is, so that
the new pull_up_constant_function() routine can share it. We'd
gotten away with one-and-a-half copies of that code so far, since
pull_up_simple_values() could assume that a lot of cases didn't apply
to it --- but I don't think pull_up_constant_function() can make any
simplifying assumptions. Might as well make pull_up_simple_values()
use it too.
(Possibly this refactoring should go further: maybe we could share
some of the code to fill in the pullup_replace_vars_context struct?
For now, I left it that the callers fill that completely.)
Note: the one existing test case that this patch changes has to be
changed because inlining its function RTEs would destroy the point
of the test, namely to check join order.
Alexander Kuzmenkov and Aleksandr Parfenov, reviewed by
Antonin Houska and Anastasia Lubennikova, and whacked around
some more by me
Peter Geoghegan [Thu, 1 Aug 2019 16:34:14 +0000 (09:34 -0700)]
Add sort support routine for the inet data type.
Add sort support for inet, including support for abbreviated keys.
Testing has shown that this reduces the time taken to sort medium to
large inet/cidr inputs by ~50-60% in realistic cases.
Author: Brandur Leach Reviewed-By: Peter Geoghegan, Edmund Horner
Discussion: https://postgr.es/m/CABR_9B-PQ8o2MZNJ88wo6r-NxW2EFG70M96Wmcgf99G6HUQ3sw@mail.gmail.com
Tom Lane [Thu, 1 Aug 2019 15:50:00 +0000 (11:50 -0400)]
Add an isolation test to exercise parallel-worker deadlock resolution.
Commit a1c1af2a1 added logic in the deadlock checker to handle lock
grouping, but it was very poorly tested, as evidenced by the bug
fixed in 3420851a2. Add a test case that exercises that a bit better
(and catches the bug --- if you revert 3420851a2, this will hang).
Since it's pretty hard to get parallel workers to take exclusive
regular locks that their parents don't already have, this test operates
by creating a deadlock among advisory locks taken in parallel workers.
To make that happen, we must override the parallel-safety labeling of
the advisory-lock functions, which we do by putting them in mislabeled,
non-inlinable wrapper functions.
We also have to remove the redundant PreventAdvisoryLocksInParallelMode
checks in lockfuncs.c. That seems fine though; if some user accidentally
does what this test is intentionally doing, not much harm will ensue.
(If there are any remaining bugs that are reachable that way, they're
probably reachable in other ways too.)
Tom Lane [Thu, 1 Aug 2019 15:36:21 +0000 (11:36 -0400)]
Mark advisory-lock functions as parallel restricted, not parallel unsafe.
There seems no good reason not to allow a parallel leader to execute
these functions. (The workers still can't, though. Although the code
would work, any such lock would go away at worker exit, which is not
the documented behavior of advisory locks.)
Michael Paquier [Thu, 1 Aug 2019 00:37:28 +0000 (09:37 +0900)]
Fix handling of previous password hooks in passwordcheck
When piling up loading of modules using check_password_hook_type,
loading passwordcheck would remove any trace of a previously-loaded
hook. Unloading the module would also cause previous hooks to be
entirely gone.
Reported-by: Rafael Castro
Author: Michael Paquier Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/15932-78f48f9ef166778c@postgresql.org
Backpatch-through: 9.4
Tom Lane [Wed, 31 Jul 2019 19:42:49 +0000 (15:42 -0400)]
Fix pg_dump's handling of dependencies for custom opclasses.
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.
Peter Eisentraut [Wed, 31 Jul 2019 07:42:15 +0000 (09:42 +0200)]
Run UTF8-requiring collation tests by default
The tests collate.icu.utf8 and collate.linux.utf8 were previously only
run when explicitly selected via EXTRA_TESTS. They require a UTF8
database, because the error messages in the expected files refer to
that, and they use some non-ASCII characters in the tests. Since
users can select any locale and encoding for the regression test run,
it was not possible to include these tests automatically.
To fix, use psql's \if facility to check various prerequisites such as
platform and the server encoding and quit the tests at the very
beginning if the configuration is not adequate. We then need to
maintain alternative expected files for these tests, but they are very
tiny and never need to change after this.
These two tests are now run automatically as part of the regression
tests.
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/052295c2-a2e1-9a21-bd36-8fbff8686cf3%402ndquadrant.com
Andres Freund [Wed, 31 Jul 2019 07:05:21 +0000 (00:05 -0700)]
Remove superfluous newlines in function prototypes.
These were introduced by pgindent due to fixe to broken
indentation (c.f. 8255c7a5eeba8). Previously the mis-indentation of
function prototypes was creatively used to reduce indentation in a few
places.
As that formatting only exists in master and REL_12_STABLE, it seems
better to fix it in both, rather than having some odd indentation in
v12 that somebody might copy for future patches or such.
Author: Andres Freund
Discussion: https://postgr.es/m/20190728013754.jwcbe5nfyt3533vx@alap3.anarazel.de
Backpatch: 12-
The rd_amcache allows an index AM to cache arbitrary information in a
relcache entry. This commit moves the cleanup of rd_amcache so that it
can also be used by table AMs. Nothing takes advantage of that yet, but
I'm sure it'll come handy for anyone writing new table AMs.
Backpatch to v12, where table AM interface was introduced.
Print WAL position correctly in pg_rewind error message.
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.
Tomas Vondra [Tue, 30 Jul 2019 17:17:12 +0000 (19:17 +0200)]
Don't build extended statistics on inheritance trees
When performing ANALYZE on inheritance trees, we collect two samples for
each relation - one for the relation alone, and one for the inheritance
subtree (relation and its child relations). And then we build statistics
on each sample, so for each relation we get two sets of statistics.
For regular (per-column) statistics this works fine, because the catalog
includes a flag differentiating statistics built from those two samples.
But we don't have such flag in the extended statistics catalogs, and we
ended up updating the same row twice, triggering this error:
ERROR: tuple already updated by self
The simplest solution is to disable extended statistics on inheritance
trees, which is what this commit is doing. In the future we may need to
do something similar to per-column statistics, but that requires adding a
flag to the catalog - and that's not backpatchable. Moreover, the current
selectivity estimation code only works with individual relations, so
building statistics on inheritance trees would be pointless anyway.
Author: Tomas Vondra Backpatch-to: 10-
Discussion: https://postgr.es/m/20190618231233.GA27470@telsasoft.com Reported-by: Justin Pryzby
Michael Paquier [Tue, 30 Jul 2019 01:54:48 +0000 (10:54 +0900)]
Fix memory leak coming from simple lists built in reindexdb
When building a list of relations for a parallel processing of a schema
or a database (or just a single-entry list for the non-parallel case
with the database name), the list is allocated and built on-the-fly for
each database processed, leaking after one database-level reindex is
done. This accumulates leaks when processing all databases, and could
become a visible issue with thousands of relations.
This is fixed by introducing a new routine in simple_list.c to free all
the elements in a simple list made of strings or OIDs. The header of
the list may be using a variable declaration or an allocated pointer,
so we don't have a routine to free this part to keep the interface
simple.
Per report from coverity for an issue introduced by 5ab892c, and
valgrind complains about the leak as well. The idea to introduce a new
routine in simple_list.c is from Tom Lane.
Tom Lane [Mon, 29 Jul 2019 22:49:04 +0000 (18:49 -0400)]
Fix busted logic for parallel lock grouping in TopoSort().
A "break" statement erroneously left behind by commit a1c1af2a1
caused TopoSort to do the wrong thing if a lock's wait list
contained multiple members of the same locking group.
Because parallel workers don't normally need any locks not already
taken by their leader, this is very hard --- maybe impossible ---
to hit in production. Still, if it did happen, the queries involved
in an otherwise-resolvable deadlock would block until canceled.
In addition to removing the bogus "break", add an Assert showing
that the conflicting uses of the beforeConstraints[] array (for both
counts and flags) don't overlap, and add some commentary explaining
why not; because it's not obvious without explanation, IMHO.
Original report and patch from Rui Hai Jiang; additional assert
and commentary by me. Back-patch to 9.6 where the bug came in.
Michael Paquier [Mon, 29 Jul 2019 00:58:49 +0000 (09:58 +0900)]
Fix handling of expressions and predicates in REINDEX CONCURRENTLY
When copying the definition of an index rebuilt concurrently for the new
entry, the index information was taken directly from the old index using
the relation cache. In this case, predicates and expressions have
some post-processing to prepare things for the planner, which loses some
information including the collations added in any of them.
This inconsistency can cause issues when attempting for example a table
rewrite, and makes the new indexes rebuilt concurrently inconsistent
with the old entries.
In order to fix the problem, fetch expressions and predicates directly
from the catalog of the old entry, and fill in IndexInfo for the new
index with that. This makes the process more consistent with
DefineIndex(), and the code is refactored with the addition of a routine
to create an IndexInfo node.
Reported-by: Manuel Rigger
Author: Michael Paquier
Discussion: https://postgr.es/m/CA+u7OA5Hp0ra235F3czPom_FyAd-3+XwSJmX95r1+sRPOJc9VQ@mail.gmail.com
Backpatch-through: 12
Thomas Munro [Sun, 28 Jul 2019 22:12:37 +0000 (10:12 +1200)]
Avoid macro clash with LLVM 9.
Early previews of LLVM 9 reveal that our Min() macro causes compiler
errors in LLVM headers reached by the #include directives in
llvmjit_inline.cpp. Let's just undefine it. Per buildfarm animal
seawasp. Back-patch to 11.
Reviewed-by: Fabien Coelho, Tom Lane
Discussion: https://postgr.es/m/20190606173216.GA6306%40alvherre.pgsql
Tom Lane [Sun, 28 Jul 2019 16:02:27 +0000 (12:02 -0400)]
Improve test coverage for LISTEN/NOTIFY.
We had no actual end-to-end test of NOTIFY message delivery. In the
core async.sql regression test, testing this is problematic because psql
traditionally prints the PID of the sending backend, making the output
unstable. We also have an isolation test script, but it likewise
failed to prove that delivery worked, because isolationtester.c had
no provisions for detecting/reporting NOTIFY messages.
Hence, add such provisions to isolationtester.c, and extend
async-notify.spec to include direct tests of basic NOTIFY functionality.
I also added tests showing that NOTIFY de-duplicates messages normally,
but not across subtransaction boundaries. (That's the historical
behavior since we introduced subtransactions, though perhaps we ought
to change it.)
Patch by me, with suggestions/review by Andres Freund.
Tom Lane [Sun, 28 Jul 2019 00:21:54 +0000 (20:21 -0400)]
Fix isolationtester race condition for notices sent before blocking.
If a test sends a notice just before blocking, it's possible on
slow machines for isolationtester to detect the blocked state before
it's consumed the notice. (For this to happen, the notice would have
to arrive after isolationtester has waited for data for 10ms, so on
fast/lightly-loaded machines it's hard to reproduce the failure.)
But, if we have seen the backend as blocked, it's certainly already
sent any notices it's going to send. Therefore, one more round of
PQconsumeInput and PQisBusy should be enough to collect and process
any such notices.
This appears to explain the instability noted in commit ebd499282, so undo
the hack therein to not print notices from insert-conflict-specconflict.
Tom Lane [Sat, 27 Jul 2019 19:59:57 +0000 (15:59 -0400)]
Don't drop NOTICE messages in isolation tests.
For its entire existence, isolationtester.c has forced client_min_messages
to WARNING, but that seems like a very poor choice of test design. It
should be up to individual test scripts to manage whether they emit notices
and to ensure that the results are stable. (There were no NOTICE messages
in the original set of isolation tests, so this was certainly dead code
when committed, but perhaps it was needed at some earlier point.)
It's possible that the original motivation was due to platform-dependent
variations in the timing of stdout vs. stderr output. That should be
moot since commits 73bcb76b7/6eda3e9c2, but just in case, adjust
isotesterNoticeProcessor to print to stdout not stderr. (stderr seems
like the wrong thing anyway: it should be for error printouts not expected
test output.)
Testing shows that the notices in insert-conflict-specconflict are indeed
a bit timing-unstable on very slow machines, so hide them; maybe we can
improve that later. Also, make the notices in plpgsql-toast a bit less
verbose than the original code would've had them.
Michael Paquier [Sat, 27 Jul 2019 13:21:18 +0000 (22:21 +0900)]
Add support for --jobs in reindexdb
When doing a schema-level or a database-level operation, a list of
relations to build is created which gets processed in parallel using
multiple connections, based on the recent refactoring for parallel slots
in src/bin/scripts/. System catalogs are processed first in a
serialized fashion to prevent deadlocks, followed by the rest done in
parallel.
This new option is not compatible with --system as reindexing system
catalogs in parallel can lead to deadlocks, and with --index as there is
no conflict handling for indexes rebuilt in parallel depending in the
same relation.
Author: Julien Rouhaud Reviewed-by: Sergei Kornilov, Michael Paquier
Discussion: https://postgr.es/m/CAOBaU_YrnH_Jqo46NhaJ7uRBiWWEcS40VNRQxgFbqYo9kApUsg@mail.gmail.com
Peter Eisentraut [Sat, 27 Jul 2019 05:56:20 +0000 (07:56 +0200)]
pg_upgrade: Default new bindir to pg_upgrade location
Make the directory where the pg_upgrade binary resides the default for
new bindir, as running the pg_upgrade binary from where the new
cluster is installed is a very common scenario. Setting this as the
defauly bindir for the new cluster will remove the need to provide it
explicitly via -B in many cases.
To support directories being missing from option parsing, extend the
directory check with a missingOk mode where the path must be filled at
a later point before being used. Also move the exec_path check to
earlier in setup to make sure we know the new cluster bindir when we
scan for required executables.
This removes the exec_path from the OSInfo struct as it is not used
anywhere.
Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/9328.1552952117@sss.pgh.pa.us
Peter Eisentraut [Sat, 27 Jul 2019 05:48:08 +0000 (07:48 +0200)]
pg_upgrade: Check all used executables
Expand the validate_exec() calls to cover all the used binaries.
Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/9328.1552952117@sss.pgh.pa.us
Tom Lane [Fri, 26 Jul 2019 17:07:08 +0000 (13:07 -0400)]
Tweak our special-case logic for the IANA "Factory" timezone.
pg_timezone_names() tries to avoid showing the "Factory" zone in
the view, mainly because that has traditionally had a very long
"abbreviation" such as "Local time zone must be set--see zic manual page",
so that showing it messes up psql's formatting of the whole view.
Since tzdb version 2016g, IANA instead uses the abbreviation "-00",
which is sane enough that there's no reason to discriminate against it.
On the other hand, it emerges that FreeBSD and possibly other packagers
are so wedded to backwards compatibility that they hack the IANA data
to keep the old spelling --- and not just that old spelling, but even
older spellings that IANA used back in the stone age. This caused the
filter logic to fail to suppress "Factory" at all on such platforms,
though the formatting problem is definitely real in that case.
To solve both problems, get rid of the hard-wired assumption about
exactly what Factory's abbreviation is, and instead reject abbreviations
exceeding 31 characters. This will allow Factory to appear in the view
if and only if it's using the modern abbreviation.
In passing, simplify the code we add to zic.c to support "zic -P"
to remove its now-obsolete hacks to not print the Factory zone's
abbreviation. Unlike pg_timezone_names(), there's no reason for
that code to support old/nonstandard timezone data.
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.
Tom Lane [Fri, 26 Jul 2019 16:45:32 +0000 (12:45 -0400)]
Avoid choosing "localtime" or "posixrules" as TimeZone during initdb.
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.
Tom Lane [Fri, 26 Jul 2019 15:59:00 +0000 (11:59 -0400)]
Fix loss of fractional digits for large values in cash_numeric().
Money values exceeding about 18 digits (depending on lc_monetary)
could be inaccurately converted to numeric, due to select_div_scale()
deciding it didn't need to compute any fractional digits. Force
its hand by setting the dscale of one division input to equal the
number of fractional digits we need.
In passing, rearrange the logic to not do useless work in locales
where money values are considered integral.
Per bug #15925 from Slawomir Chodnicki. Back-patch to all supported
branches.
Peter Eisentraut [Thu, 25 Jul 2019 15:23:36 +0000 (17:23 +0200)]
doc: Make libpq documentation navigable between functions
Turn most mentions of libpq functions into links. At id attributes to
most libpq functions, where not existing yet, so that they can be
linked to. (In a handful of cases there were problems with the PDF
processing toolchain, so those instances were not changed.)
Author: Fabien COELHO <coelho@cri.ensmp.fr> Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1905121032330.27203@lancre
Peter Eisentraut [Thu, 25 Jul 2019 14:50:42 +0000 (16:50 +0200)]
doc: Fix some markup whitespace issues
When making an xref to a varlistentry, the stylesheets use the first
<term> as the link text. In the cases fixed here, the <term> element
contained extra whitespace that ended up being part of the link text,
which looked strange in the output in some cases. This whitespace is
significant, so remove it since we don't want it.
Peter Eisentraut [Thu, 25 Jul 2019 12:43:13 +0000 (14:43 +0200)]
doc: Change libpq function ids to mixed case
The ids for linking to libpq functions were previously all lower-case.
Change to mixed-case, matching the actual function name, for easier
readability in the source. The output isn't changed in a significant
way, since the ids are converted to lower or upper case for file names
and anchors.
Thomas Munro [Thu, 25 Jul 2019 22:01:18 +0000 (10:01 +1200)]
Fix LDAP test instability.
After starting slapd, wait until it can accept a connection before
beginning the real test work. This avoids occasional test failures.
Back-patch to 11, where the LDAP tests arrived.
Author: Thomas Munro Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/20190719033013.GI1859%40paquier.xyz
Andres Freund [Thu, 25 Jul 2019 21:22:52 +0000 (14:22 -0700)]
Fix slot type handling for Agg nodes performing internal sorts.
Since 15d8f8312 we assert that - and since 7ef04e4d2cb2, 4da597edf1
rely on - the slot type for an expression's
ecxt_{outer,inner,scan}tuple not changing, unless explicitly flagged
as such. That allows to either skip deforming (for a virtual tuple
slot) or optimize the code for JIT accelerated deforming
appropriately (for other known slot types).
This assumption was sometimes violated for grouping sets, when
nodeAgg.c internally uses tuplesorts, and the child node doesn't
return a TTSOpsMinimalTuple type slot. Detect that case, and flag that
the outer slot might not be "fixed".
It's probably worthwhile to optimize this further in the future, and
more granularly determine whether the slot is fixed. As we already
instantiate per-phase transition and equal expressions, we could
cheaply set the slot type appropriately for each phase. But that's a
separate change from this bugfix.
This commit does include a very minor optimization by avoiding to
create a slot for handling tuplesorts, if no such sorts are
performed. Previously we created that slot unnecessarily in the common
case of computing all grouping sets via hashing. The code looked too
confusing without that, as the conditions for needing a sort slot and
flagging that the slot type isn't fixed, are the same.
Reported-By: Ashutosh Sharma
Author: Andres Freund
Discussion: https://postgr.es/m/CAE9k0PmNaMD2oHTEAhRyxnxpaDaYkuBYkLa1dpOpn=RS0iS2AQ@mail.gmail.com
Backpatch: 12-, where the bug was introduced in 15d8f8312
Tom Lane [Thu, 25 Jul 2019 16:10:54 +0000 (12:10 -0400)]
Fix failures to ignore \r when reading Windows-style newlines.
libpq failed to ignore Windows-style newlines in connection service files.
This normally wasn't a problem on Windows itself, because fgets() would
convert \r\n to just \n. But if libpq were running inside a program that
changes the default fopen mode to binary, it would see the \r's and think
they were data. In any case, it's project policy to ignore \r in text
files unconditionally, because people sometimes try to use files with
DOS-style newlines on Unix machines, where the C library won't hide that
from us.
Hence, adjust parseServiceFile() to ignore \r as well as \n at the end of
the line. In HEAD, go a little further and make it ignore all trailing
whitespace, to match what it's always done with leading whitespace.
In HEAD, also run around and fix up everyplace where we have
newline-chomping code to make all those places look consistent and
uniformly drop \r. It is not clear whether any of those changes are
fixing live bugs. Most of the non-cosmetic changes are in places that
are reading popen output, and the jury is still out as to whether popen
on Windows can return \r\n. (The Windows-specific code in pipe_read_line
seems to think so, but our lack of support for this elsewhere suggests
maybe it's not a problem in practice.) Hence, I desisted from applying
those changes to back branches, except in run_ssl_passphrase_command()
which is new enough and little-tested enough that we'd probably not have
heard about any problems there.
Tom Lane and Michael Paquier, per bug #15827 from Jorge Gustavo Rocha.
Back-patch the parseServiceFile() change to all supported branches,
and the run_ssl_passphrase_command() change to v11 where that was added.
Andrew Dunstan [Thu, 25 Jul 2019 15:24:23 +0000 (11:24 -0400)]
Honor MSVC WindowsSDKVersion if set
Add a line to the project file setting the target SDK. Otherwise, in for
example VS2017, if the default but optional 8.1 SDK is not installed the
build will fail.
Tom Lane [Thu, 25 Jul 2019 15:02:43 +0000 (11:02 -0400)]
Fix contrib/sepgsql test policy to work with latest SELinux releases.
As of Fedora 30, it seems that the system-provided macros for setting
up user privileges in SELinux policies don't grant the ability to read
/etc/passwd, as they formerly did. This restriction breaks psql
(which tries to use getpwuid() to obtain the user name it's running
under) and thereby the contrib/sepgsql regression test. Add explicit
specifications that we need the right to read /etc/passwd.
Mike Palmiotto, per a report from me. Back-patch to all supported
branches.
Andres Freund [Thu, 25 Jul 2019 01:45:58 +0000 (18:45 -0700)]
Fix system column accesses in ON CONFLICT ... RETURNING.
After 277cb789836 ON CONFLICT ... SET ... RETURNING failed with
ERROR: virtual tuple table slot does not have system attributes
when taking the update path, as the slot used to insert into the
table (and then process RETURNING) was defined to be a virtual slot in
that commit. Virtual slots don't support system columns except for
tableoid and ctid, as the other system columns are AM dependent.
Fix that by using a slot of the table's type. Add tests for system
column accesses in ON CONFLICT ... RETURNING.
Reported-By: Roby, bisected to the relevant commit by Jeff Janes
Author: Andres Freund
Discussion: https://postgr.es/m/73436355-6432-49B1-92ED-1FE4F7E7E100@finefun.com.au
Backpatch: 12-, where the bug was introduced in 277cb789836
Tom Lane [Wed, 24 Jul 2019 22:14:26 +0000 (18:14 -0400)]
Fix infelicities in describeOneTableDetails' partitioned-table handling.
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.
Use full 64-bit XID for checking if a deleted GiST page is old enough.
Otherwise, after a deleted page gets even older, it becomes unrecyclable
again. B-tree has the same problem, and has had since time immemorial,
but let's at least fix this in GiST, where this is new.
Backpatch to v12, where GiST page deletion was introduced.
The explicit check in gistScanPage() isn't currently really necessary, as
a deleted page is always empty, so the loop would fall through without
doing anything, anyway. But it's a marginal optimization, and it gives a
nice place to attach a comment to explain how it works.
Backpatch to v12, where GiST page deletion was introduced.
projects / postgresql / log