On recent AIX it's necessary to configure gcc to use the native assembler
(because the GNU assembler hasn't been updated to handle AIX 6+). This
caused PG builds to fail with assembler syntax errors, because we'd try
to compile s_lock.h's gcc asm fragment for PPC, and that assembly code
relied on GNU-style local labels. We can't substitute normal labels
because it would fail in any file containing more than one inlined use of
tas(). Fortunately, that code is stable enough, and the PPC ISA is simple
enough, that it doesn't seem like too much of a maintenance burden to just
hand-code the branch offsets, removing the need for any labels.
Note that the AIX assembler only accepts "$" for the location counter
pseudo-symbol. The usual GNU convention is "."; but it appears that all
versions of gas for PPC also accept "$", so in theory this patch will not
break any other PPC platforms.
This has been reported by a few people, but Steve Underwood gets the credit
for being the first to pursue the problem far enough to understand why it
was failing. Thanks also to Noah Misch for additional testing.
During fireRIRrules(), get_row_security_policies can add to
securityQuals and withCheckOptions. Make sure to lock any relations
added at that point and before firing RIR rules on those expressions.
Back-patch to 9.5 where RLS was added.
Rather than consulting TransactionIdIsInProgress to see if an in-doubt
transaction is still running, consult XidInMVCCSnapshot. That requires
the same or fewer cycles as TransactionIdIsInProgress, and what's far
more important, it does not access shared data structures (at least in the
no-subxip-overflow case) so it incurs no contention. Furthermore, we would
have had to check XidInMVCCSnapshot anyway before deciding that we were
allowed to see the tuple.
There should never be a case where XidInMVCCSnapshot says a transaction is
done while TransactionIdIsInProgress says it's still running. The other
way around is quite possible though. The result of that difference is that
HeapTupleSatisfiesMVCC will no longer set hint bits on tuples whose source
transactions recently finished but are still running according to our
snapshot. The main cost of delaying the hint-bit setting is that repeated
visits to a just-committed tuple, by transactions none of which have
snapshots new enough to see the source transaction as done, will each
execute TransactionIdIsCurrentTransactionId, which they need not have done
before. However, that's normally just a small overhead, and no contention
costs are involved; so it seems well worth the benefit of removing
TransactionIdIsInProgress calls during the life of the source transaction.
The core idea for this patch is due to Jeff Janes, who also did the legwork
proving its performance benefits. His original proposal was to swap the
order of TransactionIdIsInProgress and XidInMVCCSnapshot calls in some
cases within HeapTupleSatisfiesMVCC. That was a bit messy though.
The idea that we could dispense with calling TransactionIdIsInProgress
altogether was mine, as is the final patch.
Until 9.4, pg_controldata output was all aligned. At some point
during 9.5 development, a new item was added, namely
"Current track_commit_timestamp setting:" which is two characters
too long to be aligned with the rest of the output. Fix this by
removing the noise word "Current" and adding the requisite number
of padding spaces. Since the six preceding items are also similar
in nature, remove "Current" and pad those as well in order to
maintain overall consistency. Backpatch to 9.5 where new offending
item was added.
The default argument, if given, has to be of exactly the same datatype
as the first argument; but this was not stated in so many words, and
the error message you get about it might not lead your thought in the
right direction. Per bug #13587 from Robert McGehee.
A quick scan says that these are the only two built-in functions with two
anyelement arguments and no other polymorphic arguments. There are plenty
of cases of, eg, anyarray and anyelement, but those seem less likely to
confuse. For instance this doesn't seem terribly hard to figure out:
"function array_remove(integer[], numeric) does not exist". So I've
contented myself with fixing these two cases.
We had a report from Stefan Kaltenbrunner of a case in which postmaster
log files overran available disk space because multiple backends spewed
enormous context stats dumps upon hitting an out-of-memory condition.
Given the lack of similar reports, this isn't a common problem, but it
still seems worth doing something about. However, we don't want to just
blindly truncate the output, because that might prevent diagnosis of OOM
problems. What seems like a workable compromise is to limit the dump to
100 child contexts per parent, and summarize the space used within any
additional child contexts. That should help because practical cases where
the dump gets long will typically be huge numbers of siblings under the
same parent context; while the additional debugging value from seeing
details about individual siblings beyond 100 will not be large, we hope.
Anyway it doesn't take much code or memory space to do this, so let's try
it like this and see how things go.
Since the summarization mechanism requires passing totals back up anyway,
I took the opportunity to add a "grand total" line to the end of the
printout.
The results of the KNN-search test cases were indeterminate, as they asked
the system to sort pairs of points that are exactly equidistant from the
query reference point. It's a bit surprising that we've seen no
platform-specific failures from this in the buildfarm. Perhaps IEEE-float
math is well enough standardized that no such failures will ever occur on
supported platforms ... but since this entire regression test has yet to be
shipped in any non-alpha release, that seems like an unduly optimistic
assumption. Tweak the queries so that the correct output is uniquely
defined.
(The other queries in this test are also underdetermined; but it looks like
they are regurgitating index rows in insertion order, so for the moment
assume that that behavior is stable enough.)
Per Greg Stark's experiments with VAX. Back-patch to 9.5 where this test
script was introduced.
Previously, convert_one_string_to_scalar() would examine up to 20 bytes of
the input string, producing a scalar conversion with theoretical precision
far greater than is of any possible use considering the other limitations
on the accuracy of the resulting selectivity estimate. (I think this
choice might pre-date the caller-level logic that strips any common prefix
of the strings; before that, there could have been value in scanning the
strings far enough to use all the precision available in a double.)
Aside from wasting cycles to little purpose, this choice meant that the
"denom" variable could grow to as much as 256^21 = 3.74e50, which could
overflow in some non-IEEE float arithmetics. While we don't really support
any machines with non-IEEE arithmetic anymore, this still seems like quite
an unnecessary platform dependency. Limit the scan to 12 bytes instead,
thus limiting "denom" to 256^13 = 2.03e31, a value more likely to be
computable everywhere.
Per testing by Greg Stark, which showed overflow failures in our standard
regression tests on VAX.
Since the distances used in this algorithm are small integers (not more
than the size of the U set, in fact), there is no good reason to use float
arithmetic for them. Use short ints instead: they're smaller, faster, and
require no special portability assumptions.
Per testing by Greg Stark, which disclosed that the code got into an
infinite loop on VAX for lack of IEEE-style float infinities. We don't
really care all that much whether Postgres can run on a VAX anymore,
but there seems sufficient reason to change this code anyway.
In passing, make a few other small adjustments to make the code match
usual Postgres coding style a bit better.
For no obvious reason, spi_printtup() was coded to enlarge the tuple
pointer table by just 256 slots at a time, rather than doubling the size at
each reallocation, as is our usual habit. For very large SPI results, this
makes for O(N^2) time spent in repalloc(), which of course soon comes to
dominate the runtime. Use the standard doubling approach instead.
This is a longstanding performance bug, so back-patch to all active
branches.
Neil Conway
With a bit of tweaking of the compile namestack data structure, we can
verify at compile time whether a CONTINUE or EXIT is legal. This is
surely better than leaving it to runtime, both because earlier is better
and because we can issue a proper error pointer. Also, we can get rid
of the ad-hoc old way of detecting the problem, which only took care of
CONTINUE not EXIT.
Jim Nasby, adjusted a bit by me
Having the roles remain after the test ends up causing repeated 'make
installcheck' runs to fail and may be risky from a security perspective
also, so remove them at the end of the test.
The code had bugs that would cause crashes if NULL was passed as that
argument (originally intended to mean not to bother returning its
value), and after inspection it turns out that nothing seems interested
in the case that *ts is NULL anyway. Therefore, remove the partial
checks intended to support that case.
Author: Michael Paquier
though I didn't include a proposed Assert.
Backpatch to 9.5.
PLyString_ToComposite() blithely overwrote proc->result.out.d, even though
for a composite result type the other union variant proc->result.out.r is
the one that should be valid. This could result in a crash if out.r had
in fact been filled in (proc->result.is_rowtype == 1) and then somebody
later attempted to use that data; as per bug #13579 from Paweł Michalak.
Just to add insult to injury, it didn't work for RECORD results anyway,
because record_in() would refuse the case.
Fix by doing the I/O function lookup in a local PLyTypeInfo variable,
as we were doing already in PLyObject_ToComposite(). This is not a great
technique because any fn_extra data allocated by the input function will
be leaked permanently (thanks to using TopMemoryContext as fn_mcxt).
But that's a pre-existing issue that is much less serious than a crash,
so leave it to be fixed separately.
This bug would be a potential security issue, except that plpython is
only available to superusers and the crash requires coding the function
in a way that didn't work before today's patches.
Add regression test cases covering all the supported methods of converting
composite results.
Back-patch to 9.1 where the faulty coding was introduced.
If we have the typmod that identifies a registered record type, there's no
reason that record_in() should refuse to perform input conversion for it.
Now, in direct SQL usage, record_in() will always be passed typmod = -1
with type OID RECORDOID, because no typmodin exists for type RECORD, so the
case can't arise. However, some InputFunctionCall users such as PLs may be
able to supply the right typmod, so we should allow this to support them.
Note: the previous coding and comment here predate commit 59c016aa9f.
There has been no case since 8.1 in which the passed type OID wouldn't be
valid; and if it weren't, this error message wouldn't be apropos anyway.
Better to let lookup_rowtype_tupdesc complain about it.
Back-patch to 9.1, as this is necessary for my upcoming plpython fix.
I'm committing it separately just to make it a bit more visible in the
commit history.
To avoid confusion, rename CreatePolicyStmt's 'cmd' to 'cmd_name',
parse_policy_command's 'cmd' to 'polcmd', and AlterPolicy's 'cmd_datum'
to 'polcmd_datum', per discussion with Noah and as a follow-up to his
correction of copynodes/equalnodes handling of the CreatePolicyStmt
'cmd' field.
Back-patch to 9.5 where the CreatePolicyStmt was introduced, as we
are still only in alpha.
When reworking bypassrls in AlterRole to operate the same way the other
attribute handling is done, I missed that the variable was incorrectly a
bool rather than an int. This meant that on platforms with an unsigned
char, we could end up with incorrect behavior during ALTER ROLE.
Pointed out by Andres thanks to tests he did changing our bool to be the
one from stdbool.h which showed this and a number of other issues.
Add regression tests to test CREATE/ALTER role for the various role
attributes. Arrange to leave roles behind for testing pg_dumpall, but
none which have the LOGIN attribute.
Back-patch to 9.5 where the AlterRole bug exists.
Although commit 79af9a1d2 was initially applied to HEAD only, we later
back-patched the change into all branches (commits 6bbf75192 et al).
So it's not a new behavior in 9.5 and should not be release-noted here.
Commit 8cce08f168 used a left-shift
on a literal of 1 that could (in large allocations) be shifted by
31 or more bits. This was assigned to a local variable that was
already declared to be a long to protect against overruns of int,
but the literal in this shift needs to be declared long to allow it
to work correctly in some compilers.
Backpatch to 9.5, where the bug was introduced.
Report and patch by KaiGai Kohei, slighly modified based on
discussion.
plpgsql's error location context messages ("PL/pgSQL function fn-name line
line-no at stmt-type") would misreport a CONTINUE statement as being an
EXIT, and misreport a MOVE statement as being a FETCH. These are clear
bugs that have been there a long time, so back-patch to all supported
branches.
In addition, in 9.5 and HEAD, change the description of EXECUTE from
"EXECUTE statement" to just plain EXECUTE; there seems no good reason why
this statement type should be described differently from others that have
a well-defined head keyword. And distinguish GET STACKED DIAGNOSTICS from
plain GET DIAGNOSTICS. These are a bit more of a judgment call, and also
affect existing regression-test outputs, so I did not back-patch into
stable branches.
Pavel Stehule and Tom Lane
My expanded-objects patch (commit 1dc5ebc907) included code to make
plpgsql pass expanded-object variables as R/W pointers to certain functions
that are trusted for modifying such variables in-place. However, that
optimization got broken by commit 6c82d8d1fd, which arranged to share
a single ParamListInfo across most expressions evaluated by a plpgsql
function. We don't want a R/W pointer to be passed to other functions
just because we decided one function was safe! Fortunately, the breakage
was in the other direction, of never passing a R/W pointer at all, because
we'd always have pre-initialized the shared array slot with a R/O pointer.
So it was still functionally correct, but we were back to O(N^2)
performance for repeated use of "a := a || x". To fix, force an unshared
param array to be used when the R/W param optimization is active.
Commit 6c82d8d1fd is in HEAD only, so no need for a back-patch.
With optimizations enabled at least one compiler, clang 3.7, optimized
away the crc intrinsics knowing that the result went on unused and has
no side effects. That can trigger errors in code generation when the
intrinsic is used, as we chose to use the intrinsics without any
additional compiler flag. Return the computed value to prevent that.
With some more pedantic warning flags (-Wold-style-definition) the
configure test failed to recognize the existence of _mm_crc32_u*
intrinsics due to an independent warning in the test because the test
turned on -Werror, but that's not actually needed here.
Discussion: 20150814092039.GH4955@awork2.anarazel.de
Backpatch: 9.5, where the use of crc intrinsics was integrated.
This behavior wasn't documented, but it should be because it's user-visible
in triggers and other functions executed on the remote server.
Per question from Adam Fuchs.
Back-patch to 9.3 where postgres_fdw was added.
The table-rewriting forms of ALTER TABLE are MVCC-unsafe, in much the same
way as TRUNCATE, because they replace all rows of the table with newly-made
rows with a new xmin. (Ideally, concurrent transactions with old snapshots
would continue to see the old table contents, but the data is not there
anymore --- and if it were there, it would be inconsistent with the table's
updated rowtype, so there would be serious implementation problems to fix.)
This was nowhere documented though, and the problem was only documented for
TRUNCATE in a note in the TRUNCATE reference page. Create a new "Caveats"
section in the MVCC chapter that can be home to this and other limitations
on serializable consistency.
In passing, fix a mistaken statement that VACUUM and CLUSTER would reclaim
space occupied by a dropped column. They don't reconstruct existing tuples
so they couldn't do that.
Back-patch to all supported branches.
DO blocks use private simple_eval_estates to avoid intra-transaction memory
leakage, cf commit c7b849a89. I had forgotten about that while writing
commit 0fc94a5ba, but it means that expression execution trees created
within a DO block disappear immediately on exiting the DO block, and hence
can't safely be linked into plpgsql's session-wide cast hash table.
To fix, give a DO block a private cast hash table to go with its private
simple_eval_estate. This is less efficient than one could wish, since
DO blocks can no longer share any cast lookup work with other plpgsql
execution, but it shouldn't be too bad; in any case it's no worse than
what happened in DO blocks before commit 0fc94a5ba.
Per bug #13571 from Feike Steenbergen. Preliminary analysis by
Oleksandr Shulgin.
This fixes a bunch of somewhat pedantic warnings with new
compilers. Since by far the majority of other functions definitions use
the (void) style it just seems to be consistent to do so as well in the
remaining few places.
It was a bool, even though it should be CEOUC_WAIT_MODE. That's unlikely
to have a negative effect with the current definition of bool (char),
but it's definitely wrong.
Discussion: 20150812084351.GD8470@awork2.anarazel.de
Backpatch: 9.5, where ON CONFLICT was merged
Since a179232047 (vacuumdb: enable parallel mode) -1 has been assigned
to a boolean. That can, justifiedly, trigger compiler warnings. There's
also no need for ternary logic, result was only ever set to 0 or -1. So
don't.
Discussion: 20150812084351.GD8470@awork2.anarazel.de
Backpatch: 9.5
Doing so doesn't work if bool is a macro rather than a typedef.
Although c.h spends some effort to support configurations where bool is
a preexisting macro, help_config.c has existed this way since
2003 (b700a6), and there have not been any reports of
problems. Backpatch anyway since this is as riskless as it gets.
Discussion: 20150812084351.GD8470@awork2.anarazel.de
Backpatch: 9.0-master
Mistakenly relreplident was stored as a bool. That works today as c.h
typedefs bool to a char, but isn't very future proof.
Discussion: 20150812084351.GD8470@awork2.anarazel.de
Backpatch: 9.4 where replica identity was introduced.
Commit 43b4a16817 made the isolation
tester reject duplicate step names, and it turns out that the
test_decoding module's concurrent_ddl_dml isolation test has a
duplicate name. I think the second definition isn't actually getting
used, so just remove it.
Per buildfarm.
