Tom Lane [Tue, 16 Oct 2018 20:27:15 +0000 (16:27 -0400)]
Back off using -isysroot on Darwin.
Rethink the solution applied in commit 5e2217131 to get PL/Tcl to
build on macOS Mojave. I feared that adding -isysroot globally might
have undesirable consequences, and sure enough Jakob Egger reported
one: it complicates building extensions with a different Xcode version
than was used for the core server. (I find that a risky proposition
in general, but apparently it works most of the time, so we shouldn't
break it if we don't have to.)
We'd already adopted the solution for PL/Perl of inserting the sysroot
path directly into the -I switches used to find Perl's headers, and we
can do the same thing for PL/Tcl by changing the -iwithsysroot switch
that Apple's tclConfig.sh reports. This restricts the risks to PL/Perl
and PL/Tcl themselves and directly-dependent extensions, which is a lot
more pleasing in general than a global -isysroot switch.
Along the way, tighten the test to see if we need to inject the sysroot
path into $perl_includedir, as I'd speculated about upthread but not
gotten round to doing.
Andres Freund [Tue, 16 Oct 2018 19:05:50 +0000 (12:05 -0700)]
Mark constantly allocated dest receiver as const.
This allows the compiler / linker to mark affected pages as read-only.
Doing so requires casting constness away, as CreateDestReceiver()
returns both constant and non-constant dest receivers. That's fine
though, as any modification of the statically allocated receivers
would already have been a bug (and would now be caught on some
platforms).
Andres Freund [Tue, 16 Oct 2018 19:05:50 +0000 (12:05 -0700)]
Add macro to cast away const without allowing changes to underlying type.
The new unconsitify(underlying_type, var) macro allows to cast
constness away from a variable, but doesn't allow changing the
underlying type. Enforcement of the latter currently only works for
gcc like compilers.
Please note IT IS NOT SAFE to cast constness away if the variable will ever
be modified (it would be undefined behaviour). Doing so anyway can cause
compiler misoptimizations or runtime crashes (modifying readonly memory).
It is only safe to use when the the variable will not be modified, but API
design or language restrictions prevent you from declaring that
(e.g. because a function returns both const and non-const variables).
This'll be used in an upcoming change, but seems like it's independent
infrastructure.
Author: Andres Freund
Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
Tom Lane [Tue, 16 Oct 2018 18:57:14 +0000 (14:57 -0400)]
Be smarter about age-counter overflow in formatting.c caches.
The previous code here simply threw away whatever it knew about cache
entry ages whenever a counter overflow occurred. Since the counter
is int width and will be bumped once per format function execution,
overflows are not really so rare as to not be worth thinking about.
Instead, let's deal with the situation by halving all the age values,
essentially rescaling the age metric. In that way, we retain a
pretty accurate (if not quite perfect) idea of which entries are oldest.
Tom Lane [Tue, 16 Oct 2018 17:56:58 +0000 (13:56 -0400)]
Avoid rare race condition in privileges.sql regression test.
We created a temp table, then switched to a new session, leaving
the old session to clean up its temp objects in background. If that
took long enough, the eventual attempt to drop the user that owns
the temp table could fail, as exhibited today by sidewinder.
Fix by dropping the temp table explicitly when we're done with it.
It's been like this for quite some time, so back-patch to all
supported branches.
Tom Lane [Tue, 16 Oct 2018 17:11:05 +0000 (13:11 -0400)]
Avoid statically allocating formatting.c's format string caches.
This eliminates circa 120KB of static data from Postgres' memory
footprint. In some usage patterns that space will get allocated
anyway, but in many processes it never will be allocated.
We can improve matters further by allocating only as many cache
entries as we actually use, rather than allocating the whole array
on first use. However, to avoid wasting lots of space due to
palloc's habit of rounding requests up to power-of-2 sizes, tweak
the maximum cacheable format string length to make the struct sizes
be powers of 2 or just less. The sizes I chose make the maximums
a little bit less than they were before, but I doubt it matters much.
While at it, rearrange struct FormatNode to avoid wasting quite so
much padding space. This change actually halves the size of that
struct on 64-bit machines.
Andres Freund [Tue, 16 Oct 2018 16:44:43 +0000 (09:44 -0700)]
Correct constness of system attributes in heap.c & prerequisites.
This allows the compiler / linker to mark affected pages as read-only.
There's a fair number of pre-requisite changes, to allow the const
properly be propagated. Most of consts were already required for
correctness anyway, just not represented on the type-level. Arguably
we could be more aggressive in using consts in related code, but..
This requires using a few of the types underlying typedefs that
removes pointers (e.g. const NameData *) as declaring the typedefed
type constant doesn't have the same meaning (it makes the variable
const, not what it points to).
Tom Lane [Tue, 16 Oct 2018 16:27:14 +0000 (12:27 -0400)]
Make PostgresNode.pm's poll_query_until() more chatty about failures.
Reporting only the stderr is unhelpful when the problem is that the
server output we're getting doesn't match what was expected. So we
should report the query output too; and just for good measure, let's
print the query we used and the output we expected.
Back-patch to 9.5 where poll_query_until was introduced.
Tom Lane [Tue, 16 Oct 2018 16:01:18 +0000 (12:01 -0400)]
Improve stability of recently-added regression test case.
Commit b5febc1d1 added a contrib/btree_gist test case that has been
observed to fail in the buildfarm as a result of background auto-analyze
updating stats and changing the selected plan. Forestall that by
forcibly analyzing in foreground, instead. The new plan choice is
just as good for our purposes, since we really only care that an
index-only plan does not get selected.
localtime.c's "struct state" is a rather large object, ~23KB. We were
statically allocating one for gmtsub() to use to represent the GMT
timezone, even though that function is not at all heavily used and is
never reached in most backends. Let's malloc it on-demand, instead.
This does pose the question of how to handle a malloc failure, but
there's already a well-defined error report convention here, ie
set errno and return NULL.
We have but one caller of pg_gmtime in HEAD, and two in back branches,
neither of which were troubling to check for error. Make them do so.
The possible errors are sufficiently unlikely (out-of-range timestamp,
and now malloc failure) that I think elog() is adequate.
Back-patch to all supported branches to keep our copies of the IANA
timezone code in sync. This particular change is in a stanza that
already differs from upstream, so it's a wash for maintenance purposes
--- but only as long as we keep the branches the same.
Andres Freund [Mon, 15 Oct 2018 22:24:33 +0000 (15:24 -0700)]
Move TupleTableSlots boolean member into one flag variable.
There's several reasons for this change:
1) It reduces the total size of TupleTableSlot / reduces alignment
padding, making the commonly accessed members fit into a single
cacheline (but we currently do not force proper alignment, so
that's not yet guaranteed to be helpful)
2) Combining the booleans into a flag allows to combine read/writes
from memory.
3) With the upcoming slot abstraction changes, it allows to have core
and extended flags, in a memory efficient way.
Author: Ashutosh Bapat and Andres Freund
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
Andres Freund [Sun, 14 Oct 2018 22:18:16 +0000 (15:18 -0700)]
Move generic slot support functions from heaptuple.c into execTuples.c.
heaptuple.c was never a particular good fit for slot_getattr(),
slot_getsomeattrs() and slot_getmissingattrs(), but in upcoming
changes slots will be made more abstract (allowing slots that contain
different types of tuples), making it clearly the wrong place.
Note that slot_deform_tuple() remains in it's current place, as it
clearly deals with a HeapTuple. getmissingattrs() also remains, but
it's less clear that that's correct - but execTuples.c wouldn't be the
right place.
Thomas Munro [Mon, 15 Oct 2018 22:04:41 +0000 (11:04 +1300)]
Move the replication lag tracker into heap memory.
Andres Freund complained about the 128KB of .bss occupied by LagTracker.
It's only needed in the walsender process, so allocate it in heap
memory there.
Author: Thomas Munro
Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya%40alap3.anarazel.de
Tom Lane [Mon, 15 Oct 2018 18:01:38 +0000 (14:01 -0400)]
Check for stack overrun in standard_ProcessUtility().
ProcessUtility can recurse, and indeed can be driven to infinite
recursion, so it ought to have a check_stack_depth() call. This
covers the reported bug (portal trying to execute itself) and a bunch
of other cases that could perhaps arise somewhere.
Per bug #15428 from Malthe Borch. Back-patch to all supported branches.
When an error occurs during a benchmark run, exit with a nonzero exit
code and write a message at the end. Previously, it would just print
the error message when it happened but then proceed to print the run
summary normally and exit with status 0. To still allow
distinguishing setup from run-time errors, we use exit status 2 for
the new state, whereas existing errors during pgbench initialization
use exit status 1.
Peter Eisentraut [Mon, 15 Oct 2018 07:48:49 +0000 (09:48 +0200)]
Fixes for "Glyph not available" warnings from FOP
With the PostgreSQL 11 release notes acknowledgments list, FOP reported
WARNING: Glyph "?" (0x144, nacute) not available in font "Times-Roman".
WARNING: Glyph "?" (0x15e, Scedilla) not available in font "Times-Roman".
WARNING: Glyph "?" (0x15f, scedilla) not available in font "Times-Roman".
WARNING: Glyph "?" (0x131, dotlessi) not available in font "Times-Roman".
This is because we have some new contributors whose names use letters
that we haven't used before, and apparently FOP can't handle them out
of the box.
For now, just fix this by "unaccenting" those names. In the future,
maybe this can be fixed better with a different font configuration.
There is also another warning
WARNING: Glyph "?" (0x3c0, pi) not available in font "Times-Roman".
but that existed in previous releases and is not touched here.
This commit documents rounding of "length" parameter and absence of support
for unique indexes and NULLs searching. Backpatch to 9.6 where contrib/bloom
was introduced.
Discussion: https://postgr.es/m/CAF4Au4wPQQ7EHVSnzcLjsbY3oLSzVk6UemZLD1Sbmwysy3R61g%40mail.gmail.com
Author: Oleg Bartunov with minor editorialization by me
Backpatch-through: 9.6
Tom Lane [Sun, 14 Oct 2018 18:02:59 +0000 (14:02 -0400)]
Make some subquery-using test cases a bit more robust.
These test cases could be adversely affected by an upcoming change
to allow pullup of FROM-less subqueries. Tweak them to ensure that
they'll continue to test what they did before.
Tom Lane [Sun, 14 Oct 2018 17:07:29 +0000 (13:07 -0400)]
Use PlaceHolderVars within the quals of a FULL JOIN.
This prevents failures in cases where we pull up a constant or var-free
expression from a subquery and put it into a full join's qual. That can
result in not recognizing the qual as containing a mergejoin-able or
hashjoin-able condition. A PHV prevents the problem because it is still
recognized as belonging to the side of the join the subquery is in.
I'm not very sure about the net effect of this change on plan quality.
In "typical" cases where the join keys are Vars, nothing changes.
In an affected case, the PHV-wrapped expression is less likely to be seen
as equal to PHV-less instances below the join, but more likely to be seen
as equal to similar expressions above the join, so it may end up being a
wash. In the one existing case where there's any visible change in a
regression-test plan, it amounts to referencing a lower computation of a
COALESCE result instead of recomputing it, which seems like a win.
Given my uncertainty about that and the lack of field complaints,
no back-patch, even though this is a very ancient problem.
Tom Lane [Sun, 14 Oct 2018 16:11:16 +0000 (12:11 -0400)]
Clean up/tighten up coercibility checks in opr_sanity regression test.
With the removal of the old abstime type, there are no longer any cases
in this test where we need to use the weaker castcontext-ignoring form
of binary coercibility check. (The other major source of such headaches,
apparently-incompatible hash functions, is now hashvalidate()'s problem
not this test script's problem.) Hence, just use binary_coercible()
everywhere, and remove the comments explaining why we don't do so ---
which were broken anyway by cda6a8d01.
I left physically_coercible() in place but renamed it to better
match what it's actually testing, and added some comments.
Also, in test queries that have an assumption about the maximum number
of function arguments they need to handle, add a clause to make them fail
if someday there's a relevant function with more arguments. Otherwise
we're likely not to notice that we need to extend the queries.
Michael Paquier [Sun, 14 Oct 2018 13:23:21 +0000 (22:23 +0900)]
Avoid duplicate XIDs at recovery when building initial snapshot
On a primary, sets of XLOG_RUNNING_XACTS records are generated on a
periodic basis to allow recovery to build the initial state of
transactions for a hot standby. The set of transaction IDs is created
by scanning all the entries in ProcArray. However it happens that its
logic never counted on the fact that two-phase transactions finishing to
prepare can put ProcArray in a state where there are two entries with
the same transaction ID, one for the initial transaction which gets
cleared when prepare finishes, and a second, dummy, entry to track that
the transaction is still running after prepare finishes. This way
ensures a continuous presence of the transaction so as callers of for
example TransactionIdIsInProgress() are always able to see it as alive.
So, if a XLOG_RUNNING_XACTS takes a standby snapshot while a two-phase
transaction finishes to prepare, the record can finish with duplicated
XIDs, which is a state expected by design. If this record gets applied
on a standby to initial its recovery state, then it would simply fail,
so the odds of facing this failure are very low in practice. It would
be tempting to change the generation of XLOG_RUNNING_XACTS so as
duplicates are removed on the source, but this requires to hold on
ProcArrayLock for longer and this would impact all workloads,
particularly those using heavily two-phase transactions.
XLOG_RUNNING_XACTS is also actually used only to initialize the standby
state at recovery, so instead the solution is taken to discard
duplicates when applying the initial snapshot.
Diagnosed-by: Konstantin Knizhnik
Author: Michael Paquier
Discussion: https://postgr.es/m/0c96b653-4696-d4b4-6b5d-78143175d113@postgrespro.ru
Backpatch-through: 9.3
Tom Lane [Sat, 13 Oct 2018 20:31:09 +0000 (16:31 -0400)]
Make an editing pass over v11 release notes.
Set the release date. Do a bunch of copy-editing and markup improvement,
rearrange some stuff into what seemed a more sensible order, move some
things that did not seem to be in the right section.
Tom Lane [Fri, 12 Oct 2018 22:08:47 +0000 (18:08 -0400)]
Another round of portability hacking on ECPG regression tests.
Removing the separate Windows expected-files in commit f1885386f
turns out to have been too optimistic: on most (but not all!) of our
Windows buildfarm members, the tests still print floats with three
exponent digits, because they're invoking the native printf()
not snprintf.c.
But rather than put back the extra expected-files, let's hack
the three tests in question so that they adjust float formatting
the same way snprintf.c does.
Tom Lane [Fri, 12 Oct 2018 18:26:56 +0000 (14:26 -0400)]
Simplify use of AllocSetContextCreate() wrapper macro.
We can allow this macro to accept either abbreviated or non-abbreviated
allocation parameters by making use of __VA_ARGS__. As noted by Andres
Freund, it's unlikely that any compiler would have __builtin_constant_p
but not __VA_ARGS__, so this gives up little or no error checking, and
it avoids a minor but annoying API break for extensions.
With this change, there is no reason for anybody to call
AllocSetContextCreateExtended directly, so in HEAD I renamed it to
AllocSetContextCreateInternal. It's probably too late for an ABI
break like that in 11, though.
Alvaro Herrera [Fri, 12 Oct 2018 15:36:26 +0000 (12:36 -0300)]
Correct attach/detach logic for FKs in partitions
There was no code to handle foreign key constraints on partitioned
tables in the case of ALTER TABLE DETACH; and if you happened to ATTACH
a partition that already had an equivalent constraint, that one was
ignored and a new constraint was created. Adding this to the fact that
foreign key cloning reuses the constraint name on the partition instead
of generating a new name (as it probably should, to cater to SQL
standard rules about constraint naming within schemas), the result was a
pretty poor user experience -- the most visible failure was that just
detaching a partition and re-attaching it failed with an error such as
because it would try to create an identically-named constraint in the
partition. To make matters worse, if you tried to drop the constraint
in the now-independent partition, that would fail because the constraint
was still seen as dependent on the constraint in its former parent
partitioned table:
ERROR: cannot drop inherited constraint "test_result_asset_id_fkey" of relation "test_result_cbsystem_0001_0050_monthly_2018_09"
This fix attacks the problem from two angles: first, when the partition
is detached, the constraint is also marked as independent, so the drop
now works. Second, when the partition is re-attached, we scan existing
constraints searching for one matching the FK in the parent, and if one
exists, we link that one to the parent constraint. So we don't end up
with a duplicate -- and better yet, we don't need to scan the referenced
table to verify that the constraint holds.
To implement this I made a small change to previously planner-only
struct ForeignKeyCacheInfo to contain the constraint OID; also relcache
now maintains the list of FKs for partitioned tables too.
Backpatch to 11.
Reported-by: Michael Vitale (bug #15425)
Discussion: https://postgr.es/m/15425-2dbc9d2aa999f816@postgresql.org
Tom Lane [Fri, 12 Oct 2018 15:14:27 +0000 (11:14 -0400)]
Make float exponent output on Windows look the same as elsewhere.
Windows, alone among our supported platforms, likes to emit three-digit
exponent fields even when two digits would do. Adjust such results to
look like the way everyone else does it. Eliminate a bunch of variant
expected-output files that were needed only because of this quirk.
Michael Paquier [Fri, 12 Oct 2018 00:12:31 +0000 (09:12 +0900)]
Add TAP tests for pg_verify_checksums
All options available in the utility get coverage:
- Tests with disabled page checksums.
- Tests with enabled test checksums.
- Emulation of corruption and broken checksums with a full scan and
single relfilenode scan.
This patch has been contributed mainly by Michael Banck and Magnus
Hagander with things presented on various threads, and I have gathered
all the contents into a single patch.
Author: Michael Banck, Magnus Hagander, Michael Paquier Reviewed-by: Peter Eisentraut
Discussion: https://postgr.es/m/20181005012645.GE1629@paquier.xyz
Andres Freund [Fri, 28 Sep 2018 22:13:42 +0000 (15:13 -0700)]
Remove timetravel extension.
The extension depended on old types which are about to be removed. As
the code additionally was pretty crufty and didn't provide much in the
way of functionality, removing the extension seems to be the best way
forward. It's fairly trivial to write functionality in plpgsql that
more than covers what timetravel did.
Author: Andres Freund
Discussion:
https://postgr.es/m/20171213080506.cwjkpcz3bkk6yz2u@alap3.anarazel.de
https://postgr.es/m/25615.1513115237@sss.pgh.pa.us
Andres Freund [Thu, 11 Oct 2018 18:30:59 +0000 (11:30 -0700)]
Move timeofday() implementation out of nabstime.c.
nabstime.c is about to be removed, but timeofday() isn't related to
the rest of the functionality therein, and some find it useful. Move
to timestamp.c.
Andres Freund [Wed, 10 Oct 2018 20:53:02 +0000 (13:53 -0700)]
Fix logical decoding error when system table w/ toast is repeatedly rewritten.
Repeatedly rewriting a mapped catalog table with VACUUM FULL or
CLUSTER could cause logical decoding to fail with:
ERROR, "could not map filenode \"%s\" to relation OID"
To trigger the problem the rewritten catalog had to have live tuples
with toasted columns.
The problem was triggered as during catalog table rewrites the
heap_insert() check that prevents logical decoding information to be
emitted for system catalogs, failed to treat the new heap's toast table
as a system catalog (because the new heap is not recognized as a
catalog table via RelationIsLogicallyLogged()). The relmapper, in
contrast to the normal catalog contents, does not contain historical
information. After a single rewrite of a mapped table the new relation
is known to the relmapper, but if the table is rewritten twice before
logical decoding occurs, the relfilenode cannot be mapped to a
relation anymore. Which then leads us to error out. This only
happens for toast tables, because the main table contents aren't
re-inserted with heap_insert().
The fix is simple, add a new heap_insert() flag that prevents logical
decoding information from being emitted, and accept during decoding
that there might not be tuple data for toast tables.
Unfortunately that does not fix pre-existing logical decoding
errors. Doing so would require not throwing an error when a filenode
cannot be mapped to a relation during decoding, and that seems too
likely to hide bugs. If it's crucial to fix decoding for an existing
slot, temporarily changing the ERROR in ReorderBufferCommit() to a
WARNING appears to be the best fix.
Author: Andres Freund
Discussion: https://postgr.es/m/20180914021046.oi7dm4ra3ot2g2kt@alap3.anarazel.de
Backpatch: 9.4-, where logical decoding was introduced
Andres Freund [Wed, 10 Oct 2018 20:53:02 +0000 (13:53 -0700)]
Force synchronous commit to be enabled for all test_decoding tests.
Without that the tests fail when forced to be run against a cluster
with synchronous_commit = off (as the WAL might not yet be flushed to
disk by the point logical decoding gets called, and thus the expected
output breaks). Most tests already do that, add it to a few newer tests.
The previous check for a "complete query" omitted the new
PROCESS_UTILITY_QUERY_NONATOMIC value. This didn't actually make a
difference in practice, because only CALL and SET from PL/pgSQL run in
this state, but it's more correct to include it anyway.
It was previously a string setting that was converted into an enum by
custom code, but using the GUC enum facility seems much simpler and
doesn't change any functionality, except that
set transaction_isolation='default';
no longer works, but that was never documented and doesn't work with
any other transaction characteristics. (Note that this is not the
same as RESET or SET TO DEFAULT, which still work.)
Reviewed-by: Heikki Linnakangas <hlinnaka@iki.fi>
Discussion: https://www.postgresql.org/message-id/457db615-e84c-4838-310e-43841eb806e5@iki.fi
Tom Lane [Tue, 9 Oct 2018 17:36:16 +0000 (13:36 -0400)]
Make src/common/exec.c's error logging less ugly.
This code used elog where it really ought to use ereport, mainly so that
it can report a SQLSTATE different from ERRCODE_INTERNAL_ERROR. There
were some other random deviations from typical error report practice too.
In addition, we can make some cleanups that were impractical six months
ago:
* Use one variadic macro, instead of several with different numbers
of arguments, reducing the temptation to force-fit messages into
particular numbers of arguments;
* Use %m, even in the frontend case, simplifying the code.
Tom Lane [Tue, 9 Oct 2018 15:31:30 +0000 (11:31 -0400)]
Remove no-longer-needed variant expected regression result files.
numerology_1.out and float8-small-is-zero_1.out differ from their
base files only in showing plain zero rather than minus zero for
some results. I believe that in the wake of commit 6eb3eb577,
we will print minus zero as such on all IEEE-float platforms
(and non-IEEE floats are going to cause many more regression diffs
than this, anyway). Hence we should be able to remove these and
eliminate a bit of maintenance pain. Let's see if the buildfarm
agrees.
Tom Lane [Tue, 9 Oct 2018 15:10:07 +0000 (11:10 -0400)]
Select appropriate PG_PRINTF_ATTRIBUTE for recent NetBSD.
NetBSD-current generates a large number of warnings about "%m" not
being appropriate to use with *printf functions. While that's true
for their native printf, it's surely not true for snprintf.c, so I
think they have misunderstood gcc's definition of the "gnu_printf"
archetype. Nonetheless, choosing "__syslog__" instead silences the
warnings; so teach configure about that.
Since this is only a cosmetic warning issue (and anyway it depends
on previous hacking to be self-consistent), no back-patch.
Michael Paquier [Tue, 9 Oct 2018 13:29:09 +0000 (22:29 +0900)]
Add pg_ls_archive_statusdir function
This function lists the contents of the WAL archive status directory,
and is intended to be used by monitoring tools. Unlike pg_ls_dir(),
access to it can be granted to non-superusers so that those monitoring
tools can observe the principle of least privilege. Access is also
given by default to members of pg_monitor.
Author: Christoph Moench-Tegeder Reviewed-by: Aya Iwata
Discussion: https://postgr.es/m/20180930205920.GA64534@elch.exwg.net
Tom Lane [Tue, 9 Oct 2018 04:04:27 +0000 (00:04 -0400)]
Convert some long lists in configure.in to one-line-per-entry style.
The idea here is that patches that add items to these lists will often
be easier to rebase over other additions to the same lists, because
they won't be trying to touch the very same line of configure.in.
There will still be merge conflicts in the configure script, but that
can be fixed just by re-running autoconf (or by leaving configure out
of the submitted patch to begin with ...)
Implementation note: use of m4_normalize() is necessary to get rid of
the newlines, else incorrect shell syntax will be emitted. But with
that hack, the generated configure script is identical to what it
was before.
Thomas Munro [Mon, 8 Oct 2018 23:51:01 +0000 (12:51 +1300)]
Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux).
Originally committed as 15bc038f (plus some follow-ups), this was
reverted in 28e07270 due to a problem discovered in parallel
workers. This new version corrects that problem by sending the
list of uncommitted enum values to parallel workers.
Here follows the original commit message describing the change:
To prevent possibly breaking indexes on enum columns, we must keep
uncommitted enum values from getting stored in tables, unless we
can be sure that any such column is new in the current transaction.
Formerly, we enforced this by disallowing ALTER TYPE ... ADD VALUE
from being executed at all in a transaction block, unless the target
enum type had been created in the current transaction. This patch
removes that restriction, and instead insists that an uncommitted enum
value can't be referenced unless it belongs to an enum type created
in the same transaction as the value. Per discussion, this should be
a bit less onerous. It does require each function that could possibly
return a new enum value to SQL operations to check this restriction,
but there aren't so many of those that this seems unmaintainable.
Author: Andrew Dunstan and Tom Lane, with parallel query fix by Thomas Munro Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/CAEepm%3D0Ei7g6PaNTbcmAh9tCRahQrk%3Dr5ZWLD-jr7hXweYX3yg%40mail.gmail.com
Discussion: https://postgr.es/m/4075.1459088427%40sss.pgh.pa.us
Tom Lane [Mon, 8 Oct 2018 23:15:55 +0000 (19:15 -0400)]
Fix omissions in snprintf.c's coverage of standard *printf functions.
A warning on a NetBSD box revealed to me that pg_waldump/compat.c
is using vprintf(), which snprintf.c did not provide coverage for.
This is not good if we want to have uniform *printf behavior, and
it's pretty silly to omit when it's a one-line function.
I also noted that snprintf.c has pg_vsprintf() but for some reason
it was not exposed to the outside world, creating another way in
which code might accidentally invoke the platform *printf family.
Let's just make sure that we replace all eight of the POSIX-standard
printf family.
Also, upgrade plperl.h and plpython.h to make sure that they do
their undefine/redefine rain dance for all eight, not some random
maybe-sufficient subset thereof.
Tom Lane [Mon, 8 Oct 2018 16:19:20 +0000 (12:19 -0400)]
Improve snprintf.c's handling of NaN, Infinity, and minus zero.
Up to now, float4out/float8out handled NaN and Infinity cases explicitly,
and invoked psprintf only for ordinary float values. This was done because
platform implementations of snprintf produce varying representations of
these special cases. But now that we use snprintf.c always, it's better
to give it the responsibility to produce a uniform representation of
these cases, so that we have uniformity across the board not only in
float4out/float8out. Hence, move that work into fmtfloat().
Also, teach fmtfloat() to recognize IEEE minus zero and handle it
correctly. The previous coding worked only accidentally, and would
fail for e.g. "%+f" format (it'd print "+-0.00000"). Now that we're
using snprintf.c everywhere, it's not acceptable for it to do weird
things in corner cases. (This incidentally avoids a portability
problem we've seen on some really ancient platforms, that native
sprintf does the wrong thing with minus zero.)
Also, introduce a new entry point in snprintf.c to allow float[48]out
to bypass the work of interpreting a well-known format spec, as well
as bypassing the overhead of the psprintf layer. I modeled this API
loosely on strfromd(). In my testing, this brings float[48]out back
to approximately the same speed they had when using native snprintf,
fixing one of the main performance issues caused by using snprintf.c.
(There is some talk of more aggressive work to improve the speed of
floating-point output conversion, but these changes seem to provide
a better starting point for such work anyway.)
Getting rid of the previous ad-hoc hack for Infinity/NaN in fmtfloat()
allows removing <ctype.h> from snprintf.c's #includes. I also removed
a few other #includes that I think are historical, though the buildfarm
may expose that as wrong.
Tom Lane [Mon, 8 Oct 2018 14:41:34 +0000 (10:41 -0400)]
Avoid O(N^2) cost in ExecFindRowMark().
If there are many ExecRowMark structs, we spent O(N^2) time in
ExecFindRowMark during executor startup. Once upon a time this was
not of great concern, but the addition of native partitioning has
squeezed out enough other costs that this can become the dominant
overhead in some use-cases for tables with many partitions.
To fix, simply replace that List data structure with an array.
This adds a little bit of cost to execCurrentOf(), but not much,
and anyway that code path is neither of large importance nor very
efficient now. If we ever decide it is a bottleneck, constructing a
hash table for lookup-by-tableoid would likely be the thing to do.
Per complaint from Amit Langote, though this is different from
his fix proposal.
Alvaro Herrera [Mon, 8 Oct 2018 13:37:21 +0000 (10:37 -0300)]
Silence compiler warning in Assert()
gcc 6.3 does not whine about this mistake I made in 39808e8868c8 but
evidently lots of other compilers do, according to Michael Paquier,
Peter Eisentraut, Arthur Zakirov, Tomas Vondra.
Michael Paquier [Mon, 8 Oct 2018 08:56:13 +0000 (17:56 +0900)]
Improve two error messages related to foreign keys on partitioned tables
Error messages for creating a foreign key on a partitioned table using
ONLY or NOT VALID were wrong in mentioning the objects they worked on.
This commit adds on the way some regression tests missing for those
cases.
Tom Lane [Sun, 7 Oct 2018 18:33:17 +0000 (14:33 -0400)]
Remove some unnecessary fields from Plan trees.
In the wake of commit f2343653f, we no longer need some fields that
were used before to control executor lock acquisitions:
* PlannedStmt.nonleafResultRelations can go away entirely.
* partitioned_rels can go away from Append, MergeAppend, and ModifyTable.
However, ModifyTable still needs to know the RT index of the partition
root table if any, which was formerly kept in the first entry of that
list. Add a new field "rootRelation" to remember that. rootRelation is
partly redundant with nominalRelation, in that if it's set it will have
the same value as nominalRelation. However, the latter field has a
different purpose so it seems best to keep them distinct.
Amit Langote, reviewed by David Rowley and Jesper Pedersen,
and whacked around a bit more by me
Alvaro Herrera [Sun, 7 Oct 2018 01:13:19 +0000 (22:13 -0300)]
Fix catalog insertion order for ATTACH PARTITION
Commit 2fbdf1b38bc changed the order in which we inserted catalog rows
when creating partitions, so that we could remove an unsightly hack
required for untimely relcache invalidations. However, that commit only
changed the ordering for CREATE TABLE PARTITION OF, and left ALTER TABLE
ATTACH PARTITION unchanged, so the latter can be affected when catalog
invalidations occur, for instance when the partition key involves an SQL
function.
Alvaro Herrera [Sat, 6 Oct 2018 22:17:46 +0000 (19:17 -0300)]
Fix event triggers for partitioned tables
Index DDL cascading on partitioned tables introduced a way for ALTER
TABLE to be called reentrantly. This caused an an important deficiency
in event trigger support to be exposed: on exiting the reentrant call,
the alter table state object was clobbered, causing a crash when the
outer alter table tries to finalize its processing. Fix the crash by
creating a stack of event trigger state objects. There are still ways
to cause things to misbehave (and probably other crashers) with more
elaborate tricks, but at least it now doesn't crash in the obvious
scenario.
Backpatch to 9.5, where DDL deparsing of event triggers was introduced.
Reported-by: Marco Slot
Authors: Michaël Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/CANNhMLCpi+HQ7M36uPfGbJZEQLyTy7XvX=5EFkpR-b1bo0uJew@mail.gmail.com
Tom Lane [Sat, 6 Oct 2018 19:49:37 +0000 (15:49 -0400)]
Restore sane locking behavior during parallel query.
Commit 9a3cebeaa changed things so that parallel workers didn't obtain
any lock of their own on tables they access. That was clearly a bad
idea, but I'd mistakenly supposed that it was the intended end result
of the series of patches for simplifying the executor's lock management.
Undo that change in relation_open(), and adjust ExecOpenScanRelation()
so that it gets the correct lock if inside a parallel worker.
In passing, clean up some more obsolete comments about when locks
are acquired.
Tom Lane [Sat, 6 Oct 2018 19:12:51 +0000 (15:12 -0400)]
Remove more redundant relation locking during executor startup.
We already have appropriate locks on every relation listed in the
query's rangetable before we reach the executor. Take the next step
in exploiting that knowledge by removing code that worries about
taking locks on non-leaf result relations in a partitioned table.
In particular, get rid of ExecLockNonLeafAppendTables and a stanza in
InitPlan that asserts we already have locks on certain such tables.
In passing, clean up some now-obsolete comments in InitPlan.
Amit Langote, reviewed by David Rowley and Jesper Pedersen,
and whacked around a bit more by me
Tom Lane [Sat, 6 Oct 2018 17:18:38 +0000 (13:18 -0400)]
Don't use is_infinite() where isinf() will do.
Places that aren't testing for sign should not use the more expensive
function; it's just wasteful, not to mention being a cognitive load
for readers who may know what isinf() is but not is_infinite().
As things stand, we actually don't need is_infinite() anyplace except
float4out/float8out, which means it could potentially go away altogether
after the changes I proposed in <13178.1538794717@sss.pgh.pa.us>.
Tom Lane [Sat, 6 Oct 2018 16:00:09 +0000 (12:00 -0400)]
Propagate xactStartTimestamp and stmtStartTimestamp to parallel workers.
Previously, a worker process would establish values for these based on
its own start time. In v10 and up, this can trivially be shown to cause
misbehavior of transaction_timestamp(), timestamp_in(), and related
functions which are (perhaps unwisely?) marked parallel-safe. It seems
likely that other behaviors might diverge from what happens in the parent
as well.
It's not as trivial to demonstrate problems in 9.6 or 9.5, but I'm sure
it's still possible, so back-patch to all branches containing parallel
worker infrastructure.
In HEAD only, mark now() and statement_timestamp() as parallel-safe
(other affected functions already were). While in theory we could
still squeeze that change into v11, it doesn't seem important enough
to force a last-minute catversion bump.
Dean Rasheed [Sat, 6 Oct 2018 10:20:09 +0000 (11:20 +0100)]
Improve the accuracy of floating point statistical aggregates.
When computing statistical aggregates like variance, the common
schoolbook algorithm which computes the sum of the squares of the
values and subtracts the square of the mean can lead to a large loss
of precision when using floating point arithmetic, because the
difference between the two terms is often very small relative to the
terms themselves.
To avoid this, re-work these aggregates to use the Youngs-Cramer
algorithm, which is a proven, numerically stable algorithm that
directly aggregates the sum of the squares of the differences of the
values from the mean in a single pass over the data.
While at it, improve the test coverage to test the aggregate combine
functions used during parallel aggregation.
Per report and suggested algorithm from Erich Schubert.
Michael Paquier [Sat, 6 Oct 2018 05:59:36 +0000 (14:59 +0900)]
Assign constraint name when cloning FK definition for partitions
This is for example used when attaching a partition to a partitioned
table which includes foreign keys, and in this case the constraint name
has been missing in the data cloned. This could lead to hard crashes,
as when validating the foreign key constraint, the constraint name is
always expected. Particularly, when using log_min_messages >= DEBUG1, a
log message would be generated with this unassigned constraint name,
leading to an assertion failure on HEAD.
While on it, rename a variable in ATExecAttachPartition which was
declared twice with the same name.
Author: Michael Paquier Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/20181005042236.GG1629@paquier.xyz
Backpatch-through: 11
Tom Lane [Fri, 5 Oct 2018 20:01:29 +0000 (16:01 -0400)]
Allow btree comparison functions to return INT_MIN.
Historically we forbade datatype-specific comparison functions from
returning INT_MIN, so that it would be safe to invert the sort order
just by negating the comparison result. However, this was never
really safe for comparison functions that directly return the result
of memcmp(), strcmp(), etc, as POSIX doesn't place any such restriction
on those library functions. Buildfarm results show that at least on
recent Linux on s390x, memcmp() actually does return INT_MIN sometimes,
causing sort failures.
The agreed-on answer is to remove this restriction and fix relevant
call sites to not make such an assumption; code such as "res = -res"
should be replaced by "INVERT_COMPARE_RESULT(res)". The same is needed
in a few places that just directly negated the result of memcmp or
strcmp.
To help find places having this problem, I've also added a compile option
to nbtcompare.c that causes some of the commonly used comparators to
return INT_MIN/INT_MAX instead of their usual -1/+1. It'd likely be
a good idea to have at least one buildfarm member running with
"-DSTRESS_SORT_INT_MIN". That's far from a complete test of course,
but it should help to prevent fresh introductions of such bugs.
This is a longstanding portability hazard, so back-patch to all supported
branches.
Tom Lane [Fri, 5 Oct 2018 16:45:37 +0000 (12:45 -0400)]
Ensure that PLPGSQL_DTYPE_ROW variables have valid refname fields.
Without this, the syntax-tree-dumping functions in pl_funcs.c crash,
and there are other places that might be at risk too. Per report
from Pavel Stehule.
Looks like I broke this in commit f9263006d, so back-patch to v11.
Michael Paquier [Fri, 5 Oct 2018 00:21:48 +0000 (09:21 +0900)]
Add pg_ls_tmpdir function
This lists the contents of a temporary directory associated to a given
tablespace, useful to get information about on-disk consumption caused
by temporary files used by a session query. By default, pg_default is
scanned, and a tablespace can be specified as argument.
This function is intended to be used by monitoring tools, and, unlike
pg_ls_dir(), access to them can be granted to non-superusers so that
those monitoring tools can observe the principle of least privilege.
Access is also given by default to members of pg_monitor.
Tom Lane [Thu, 4 Oct 2018 19:48:17 +0000 (15:48 -0400)]
In the executor, use an array of pointers to access the rangetable.
Instead of doing a lot of list_nth() accesses to es_range_table,
create a flattened pointer array during executor startup and index
into that to get at individual RangeTblEntrys.
This eliminates one source of O(N^2) behavior with lots of partitions.
(I'm not exactly convinced that it's the most important source, but
it's an easy one to fix.)
Tom Lane [Thu, 4 Oct 2018 18:03:37 +0000 (14:03 -0400)]
Centralize executor's opening/closing of Relations for rangetable entries.
Create an array estate->es_relations[] paralleling the es_range_table,
and store references to Relations (relcache entries) there, so that any
given RT entry is opened and closed just once per executor run. Scan
nodes typically still call ExecOpenScanRelation, but ExecCloseScanRelation
is no more; relation closing is now done centrally in ExecEndPlan.
This is slightly more complex than one would expect because of the
interactions with relcache references held in ResultRelInfo nodes.
The general convention is now that ResultRelInfo->ri_RelationDesc does
not represent a separate relcache reference and so does not need to be
explicitly closed; but there is an exception for ResultRelInfos in the
es_trig_target_relations list, which are manufactured by
ExecGetTriggerResultRel and have to be cleaned up by
ExecCleanUpTriggerState. (That much was true all along, but these
ResultRelInfos are now more different from others than they used to be.)
To allow the partition pruning logic to make use of es_relations[] rather
than having its own relcache references, adjust PartitionedRelPruneInfo
to store an RT index rather than a relation OID.
Amit Langote, reviewed by David Rowley and Jesper Pedersen,
some mods by me
Alvaro Herrera [Thu, 4 Oct 2018 14:37:20 +0000 (11:37 -0300)]
Fix duplicate primary keys in partitions
When using the CREATE TABLE .. PARTITION OF syntax, it's possible to
cause a partition to get two primary keys if the parent already has one.
Tighten the check to disallow that.
Reported-by: Rajkumar Raghuwanshi
Author: Amul Sul
Discussion: https://postgr.es/m/CAKcux6=OnSV3-qd8Gb6W=KPPwcCz6Fe_O_MQYjTa24__Xn8XxA@mail.gmail.com
This moves the system administration functions for signalling backends
from backend/utils/adt/misc.c into a separate file dedicated to backend
signalling. No new functionality is introduced in this commit.
Author: Daniel Gustafsson Reviewed-by: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/C2C7C3EC-CC5F-44B6-9C78-637C88BD7D14@yesql.se
Michael Paquier [Thu, 4 Oct 2018 00:00:33 +0000 (09:00 +0900)]
Add option SKIP_LOCKED to VACUUM and ANALYZE
When specified, this option allows VACUUM to skip the work on a relation
if there is a conflicting lock on it when trying to open it at the
beginning of its processing.
Similarly to autovacuum, this comes with a couple of limitations while
the relation is processed which can cause the process to still block:
- when opening the relation indexes.
- when acquiring row samples for table inheritance trees, partition trees
or certain types of foreign tables, and that a lock is taken on some
leaves of such trees.
Author: Nathan Bossart Reviewed-by: Michael Paquier, Andres Freund, Masahiko Sawada
Discussion: https://postgr.es/m/9EF7EBE4-720D-4CF1-9D0E-4403D7E92990@amazon.com
Discussion: https://postgr.es/m/20171201160907.27110.74730@wrigleys.postgresql.org
Tom Lane [Wed, 3 Oct 2018 20:05:05 +0000 (16:05 -0400)]
Change executor to just Assert that table locks were already obtained.
Instead of locking tables during executor startup, just Assert that
suitable locks were obtained already during the parse/plan pipeline
(or re-obtained by the plan cache). This must be so, else we have a
hazard that concurrent DDL has invalidated the plan.
This is pretty inefficient as well as undercommented, but it's all going
to go away shortly, so I didn't try hard. This commit is just another
attempt to use the buildfarm to see if we've missed anything in the plan
to simplify the executor's table management.
Note that the change needed here in relation_open() exposes that
parallel workers now really are accessing tables without holding any
lock of their own, whereas they were not doing that before this commit.
This does not give me a warm fuzzy feeling about that aspect of parallel
query; it does not seem like a good design, and we now know that it's
had exactly no actual testing. I think that we should modify parallel
query so that that change can be reverted.
Andres Freund [Wed, 3 Oct 2018 19:48:37 +0000 (12:48 -0700)]
Fix issues around EXPLAIN with JIT.
I (Andres) was more than a bit hasty in committing 33001fd7a7072d48327
after last minute changes, leading to a number of problems (jit output
was only shown for JIT in parallel workers, and just EXPLAIN without
ANALYZE didn't work). Lukas luckily found these issues quickly.
Instead of combining instrumentation in in standard_ExecutorEnd(), do
so on demand in the new ExplainPrintJITSummary().
Also update a documentation example of the JIT output, changed in 52050ad8ebec8d831.
Author: Lukas Fittl, with minor changes by me
Discussion: https://postgr.es/m/CAP53PkxmgJht69pabxBXJBM+0oc6kf3KHMborLP7H2ouJ0CCtQ@mail.gmail.com
Backpatch: 11, where JIT compilation was introduced
Tom Lane [Wed, 3 Oct 2018 18:33:13 +0000 (14:33 -0400)]
Rationalize snprintf.c's handling of "ll" formats.
Although all known platforms define "long long" as 64 bits, it still feels
a bit shaky to be using "va_arg(args, int64)" to pull out an argument that
the caller thought was declared "long long". The reason it was coded like
this, way back in commit 3311c7669, was to work around the possibility that
the compiler had no type named "long long" --- and, at the time, that it
maybe didn't have 64-bit ints at all. Now that we're requiring compilers
to support C99, those concerns are moot. Let's make the code clearer and
more bulletproof by writing "long long" where we mean "long long".
This does introduce a hazard that we'd inefficiently use 128-bit arithmetic
to convert plain old integers. The way to tackle that would be to provide
two versions of fmtint(), one for "long long" and one for narrower types.
Since, as of today, no platforms require that, we won't bother with the
extra code for now.
Tom Lane [Wed, 3 Oct 2018 17:05:01 +0000 (13:05 -0400)]
Provide fast path in snprintf.c for conversion specs that are just "%s".
This case occurs often enough (around 45% of conversion specs executed
in our regression tests are just "%s") that it's worth an extra test
per conversion spec to allow skipping all the logic associated with
field widths and padding when it happens.
Tom Lane [Wed, 3 Oct 2018 14:18:15 +0000 (10:18 -0400)]
Make assorted performance improvements in snprintf.c.
In combination, these changes make our version of snprintf as fast
or faster than most platforms' native snprintf, except for cases
involving floating-point conversion (which we still delegate to
the native sprintf). The speed penalty for a float conversion
is down to around 10% though, much better than before.
Notable changes:
* Rather than always parsing the format twice to see if it contains
instances of %n$, do the extra scan only if we actually find a $.
This obviously wins for non-localized formats, and even when there
is use of %n$, we can avoid scanning text before the first % twice.
* Use strchrnul() if available to find the next %, and emit the
literal text between % escapes as strings rather than char-by-char.
* Create a bespoke function (dopr_outchmulti) for the common case
of emitting N copies of the same character, in place of writing
loops around dopr_outch.
* Simplify construction of the format string for invocations of sprintf
for floats.
* Const-ify some internal functions, and avoid unnecessary use of
pass-by-reference arguments.
Amit Kapila [Wed, 3 Oct 2018 03:34:54 +0000 (09:04 +0530)]
MAXALIGN the target address where we store flattened value.
The API (EOH_flatten_into) that flattens the expanded value representation
expects the target address to be maxaligned. All it's usage adhere to that
principle except when serializing datums for parallel query. Fix that
usage.
Diagnosed-by: Tom Lane
Author: Tom Lane and Amit Kapila
Backpatch-through: 9.6
Discussion: https://postgr.es/m/11629.1536550032@sss.pgh.pa.us
Andrew Dunstan [Tue, 2 Oct 2018 20:46:57 +0000 (16:46 -0400)]
Don't build static libraries on Cygwin
Cygwin has been building and linking against static libraries. Although
a bug this has been relatively harmless until now, when this has caused
errors due to changes in the way we build certain libraries. So this
patch makes things work the way we always intended, namely that we would
link against the dynamic libraries (cygpq.dll etc.) and just not build
the static libraries. The downstream packagers have been doing this for
some time, so this just aligns with their practice.
Extracted from a patch by Marco Atzeri, with a suggestion from Tom Lane.
Tom Lane [Tue, 2 Oct 2018 18:43:01 +0000 (14:43 -0400)]
Change rewriter/planner/executor/plancache to depend on RTE rellockmode.
Instead of recomputing the required lock levels in all these places,
just use what commit fdba460a2 made the parser store in the RTE fields.
This already simplifies the code measurably in these places, and
follow-on changes will remove a bunch of no-longer-needed infrastructure.
In a few cases, this change causes us to acquire a higher lock level
than we did before. This is OK primarily because said higher lock level
should've been acquired already at query parse time; thus, we're saving
a useless extra trip through the shared lock manager to acquire a lesser
lock alongside the original lock. The only known exception to this is
that re-execution of a previously planned SELECT FOR UPDATE/SHARE query,
for a table that uses ROW_MARK_REFERENCE or ROW_MARK_COPY methods, might
have gotten only AccessShareLock before. Now it will get RowShareLock
like the first execution did, which seems fine.
While there's more to do, push it in this state anyway, to let the
buildfarm help verify that nothing bad happened.
Amit Langote, reviewed by David Rowley and Jesper Pedersen,
and whacked around a bit more by me
Andres Freund [Tue, 2 Oct 2018 18:14:26 +0000 (11:14 -0700)]
Use slots more widely in tuple mapping code and make naming more consistent.
It's inefficient to use a single slot for mapping between tuple
descriptors for multiple tuples, as previously done when using
ConvertPartitionTupleSlot(), as that means the slot's tuple descriptors
change for every tuple.
Previously we also, via ConvertPartitionTupleSlot(), built new tuples
after the mapping even in cases where we, immediately afterwards,
access individual columns again.
Refactor the code so one slot, on demand, is used for each
partition. That avoids having to change the descriptor (and allows to
use the more efficient "fixed" tuple slots). Then use slot->slot
mapping, to avoid unnecessarily forming a tuple.
As the naming between the tuple and slot mapping functions wasn't
consistent, rename them to execute_attr_map_{tuple,slot}. It's likely
that we'll also rename convert_tuples_by_* to denote that these
functions "only" build a map, but that's left for later.
Author: Amit Khandekar and Amit Langote, editorialized by me Reviewed-By: Amit Langote, Amit Khandekar, Andres Freund
Discussion:
https://postgr.es/m/CAJ3gD9fR0wRNeAE8VqffNTyONS_UfFPRpqxhnD9Q42vZB+Jvpg@mail.gmail.com
https://postgr.es/m/e4f9d743-cd4b-efb0-7574-da21d86a7f36%40lab.ntt.co.jp
Backpatch: -
Tom Lane [Tue, 2 Oct 2018 16:41:28 +0000 (12:41 -0400)]
Set snprintf.c's maximum number of NL arguments to be 31.
Previously, we used the platform's NL_ARGMAX if any, otherwise 16.
The trouble with this is that the platform value is hugely variable,
ranging from the POSIX-minimum 9 to as much as 64K on recent FreeBSD.
Values of more than a dozen or two have no practical use and slow down
the initialization of the argtypes array. Worse, they cause snprintf.c
to consume far more stack space than was the design intention, possibly
resulting in stack-overflow crashes.
Standardize on 31, which is comfortably more than we need (it looks like
no existing translatable message has more than about 10 parameters).
I chose that, not 32, to make the array sizes powers of 2, for some
possible small gain in speed of the memset.
The lack of reported crashes suggests that the set of platforms we
use snprintf.c on (in released branches) may have no overlap with
the set where NL_ARGMAX has unreasonably large values. But that's
not entirely clear, so back-patch to all supported branches.
Tom Lane [Tue, 2 Oct 2018 15:54:12 +0000 (11:54 -0400)]
Fix corner-case failures in has_foo_privilege() family of functions.
The variants of these functions that take numeric inputs (OIDs or
column numbers) are supposed to return NULL rather than failing
on bad input; this rule reduces problems with snapshot skew when
queries apply the functions to all rows of a catalog.
has_column_privilege() had careless handling of the case where the
table OID didn't exist. You might get something like this:
select has_column_privilege(9999,'nosuchcol','select');
ERROR: column "nosuchcol" of relation "(null)" does not exist
or you might get a crash, depending on the platform's printf's response
to a null string pointer.
In addition, while applying the column-number variant to a dropped
column returned NULL as desired, applying the column-name variant
did not:
select has_column_privilege('mytable','........pg.dropped.2........','select');
ERROR: column "........pg.dropped.2........" of relation "mytable" does not exist
It seems better to make this case return NULL as well.
Also, the OID-accepting variants of has_foreign_data_wrapper_privilege,
has_server_privilege, and has_tablespace_privilege didn't follow the
principle of returning NULL for nonexistent OIDs. Superusers got TRUE,
everybody else got an error.
Per investigation of Jaime Casanova's report of a new crash in HEAD.
These behaviors have been like this for a long time, so back-patch to
all supported branches.
Patch by me; thanks to Stephen Frost for discussion and review
Michael Paquier [Tue, 2 Oct 2018 07:34:41 +0000 (16:34 +0900)]
Fix documentation of pgrowlocks using "lock_type" instead of "modes"
The example used in the documentation is outdated as well. This is an
oversight from 0ac5ad5, which bumped up pgrowlocks but forgot some bits
of the documentation.
Reported-by: Chris Wilson
Discussion: https://postgr.es/m/153838692816.2950.12001142346234155699@wrigleys.postgresql.org
Backpatch-through: 9.3
Amit Kapila [Tue, 2 Oct 2018 05:31:33 +0000 (11:01 +0530)]
Test passing expanded-value representations to workers.
Currently, we don't have an explicit test to pass expanded-value
representations to workers, so we don't know whether it works on all kind
of platforms. We suspect that the current code won't work on
alignment-sensitive hardware. This commit will test that aspect and can
lead to failure on some of the buildfarm machines which we will fix in the
later commit.
Author: Tom Lane and Amit Kapila
Discussion: https://postgr.es/m/11629.1536550032@sss.pgh.pa.us
Michael Paquier [Mon, 1 Oct 2018 23:53:38 +0000 (08:53 +0900)]
Refactor relation opening for VACUUM and ANALYZE
VACUUM and ANALYZE share similar logic when it comes to opening a
relation to work on in terms of how the relation is opened, in which
order locks are tried and how logs should be generated when something
does not work as expected.
This commit refactors things so as both use the same code path to handle
the way a relation is opened, so as the integration of new options
becomes easier.
Author: Michael Paquier Reviewed-by: Nathan Bossart
Discussion: https://postgr.es/m/20180927075152.GT1659@paquier.xyz
Change PROCEDURE to FUNCTION in CREATE EVENT TRIGGER syntax
This was claimed to have been done in 0a63f996e018ac508c858e87fa39cc254a5db49f, but that actually only
changed the documentation and not the grammar. (That commit did fully
change it for CREATE TRIGGER.)
Tom Lane [Mon, 1 Oct 2018 16:43:21 +0000 (12:43 -0400)]
Add assertions that we hold some relevant lock during relation open.
Opening a relation with no lock at all is unsafe; there's no guarantee
that we'll see a consistent state of the relevant catalog entries.
While use of MVCC scans to read the catalogs partially addresses that
complaint, it's still possible to switch to a new catalog snapshot
partway through loading the relcache entry. Moreover, whether or not
you trust the reasoning behind sometimes using less than
AccessExclusiveLock for ALTER TABLE, that reasoning is certainly not
valid if concurrent users of the table don't hold a lock corresponding
to the operation they want to perform.
Hence, add some assertion-build-only checks that require any caller
of relation_open(x, NoLock) to hold at least AccessShareLock. This
isn't a full solution, since we can't verify that the lock level is
semantically appropriate for the action --- but it's definitely of
some use, because it's already caught two bugs.
We can also assert that callers of addRangeTableEntryForRelation()
hold at least the lock level specified for the new RTE.
Tom Lane [Mon, 1 Oct 2018 15:51:07 +0000 (11:51 -0400)]
Fix tuple_data_split() to not open a relation without any lock.
contrib/pageinspect's tuple_data_split() function thought it could get
away with opening the referenced relation with NoLock. In practice
there's no guarantee that the current session holds any lock on that
rel (even if we just read a page from it), so that this is unsafe.
Switch to using AccessShareLock. Also, postpone closing the relation,
so that we needn't copy its tupdesc. Also, fix unsafe use of
att_isnull() for attributes past the end of the tuple.
Per testing with a patch that complains if we open a relation without
holding any lock on it. I don't plan to back-patch that patch, but we
should close the holes it identifies in all supported branches.
Tom Lane [Mon, 1 Oct 2018 15:39:13 +0000 (11:39 -0400)]
Fix ALTER COLUMN TYPE to not open a relation without any lock.
If the column being modified is referenced by a foreign key constraint
of another table, ALTER TABLE would open the other table (to re-parse
the constraint's definition) without having first obtained a lock on it.
This was evidently intentional, but that doesn't mean it's really safe.
It's especially not safe in 9.3, which pre-dates use of MVCC scans for
catalog reads, but even in current releases it doesn't seem like a good
idea.
We know we'll need AccessExclusiveLock shortly to drop the obsoleted
constraint, so just get that a little sooner to close the hole.
Per testing with a patch that complains if we open a relation without
holding any lock on it. I don't plan to back-patch that patch, but we
should close the holes it identifies in all supported branches.
Tom Lane [Sun, 30 Sep 2018 17:55:51 +0000 (13:55 -0400)]
Create an RTE field to record the query's lock mode for each relation.
Add RangeTblEntry.rellockmode, which records the appropriate lock mode for
each RTE_RELATION rangetable entry (either AccessShareLock, RowShareLock,
or RowExclusiveLock depending on the RTE's role in the query).
This patch creates the field and makes all creators of RTE nodes fill it
in reasonably, but for the moment nothing much is done with it. The plan
is to replace assorted post-parser logic that re-determines the right
lockmode to use with simple uses of rte->rellockmode. For now, just add
Asserts in each of those places that the rellockmode matches what they are
computing today. (In some cases the match isn't perfect, so the Asserts
are weaker than you might expect; but this seems OK, as per discussion.)
This passes check-world for me, but it seems worth pushing in this state
to see if the buildfarm finds any problems in cases I failed to test.
catversion bump due to change of stored rules.
Amit Langote, reviewed by David Rowley and Jesper Pedersen,
and whacked around a bit more by me
Stephen Frost [Fri, 28 Sep 2018 23:04:50 +0000 (19:04 -0400)]
Add application_name to connection authorized msg
The connection authorized message has quite a bit of useful information
in it, but didn't include the application_name (when provided), so let's
add that as it can be very useful.
Note that at the point where we're emitting the connection authorized
message, we haven't processed GUCs, so it's not possible to get this by
using log_line_prefix (which pulls from the GUC). There's also
something to be said for having this included in the connection
authorized message and then not needing to repeat it for every line, as
having it in log_line_prefix would do.
The GUC cleans the application name to pure-ascii, so do that here too,
but pull out the logic for cleaning up a string into its own function
in common and re-use it from those places, and check_cluster_name which
was doing the same thing.
Author: Don Seiler <don@seiler.us>
Discussion: https://postgr.es/m/CAHJZqBB_Pxv8HRfoh%2BAB4KxSQQuPVvtYCzMg7woNR3r7dfmopw%40mail.gmail.com
projects / postgresql / log