We previously took a hard-line attitude that callers should never print
a null string pointer, and doing so is worthy of an assertion failure
or crash. However, we've long since flushed out any easy-to-find bugs
of that nature. What remains is a lot of code that perhaps could fail
that way in hard-to-reach corner cases. For example, in something as
simple as
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("constraint \"%s\" for table \"%s\" does not exist",
conname, get_rel_name(relid))));
one must wonder whether it's completely guaranteed that get_rel_name
cannot return NULL in this context. If such a situation did occur,
the existing policy converts what might be a pretty minor bug into
a server crash condition. This is not good for robustness.
Hence, let's follow the lead of glibc and print "(null)" instead
of failing. We should, of course, still consider it a bug if that
behavior is reachable in ordinary use; but crashing seems less
desirable than not crashing.
This fix works across-the-board in v12 and up, where we always use
src/port/snprintf.c. Before that, on most platforms we're at the mercy
of the local libc, but it appears that Solaris 10 is the only supported
platform where we'd still get a crash. Most other platforms such as
*BSD, macOS, and Solaris 11 have adopted glibc's behavior at some
point. (AIX and HPUX just print "" not "(null)", but that's close
enough.) I've not checked what Windows' native printf would do, but
it doesn't matter because we've long used snprintf.c on that platform.
In v12 and up, also const-ify related code so that we're not casting
away const on the constant string. This is just neatnik-ism, since
next to no compilers will warn about that.
Discussion: https://postgr.es/m/17098-b960f3616c861f83@postgresql.org
Modern gcc and clang compilers offer alignment sanitizers, which help to detect
pointer misalignment. However, our codebase already contains x86-specific
crc32 computation code, which uses unalignment access. Thankfully, those
compilers also support the attribute, which disables alignment sanitizers at
the function level. This commit adds pg_attribute_no_sanitize_alignment(),
which wraps this attribute, and applies it to pg_comp_crc32c_sse42() function.
Back-patch of commits 993bdb9f9 and ad2ad698a, to enable doing
alignment testing in all supported branches.
Discussion: https://postgr.es/m/CAPpHfdsne3%3DT%3DfMNU45PtxdhSL_J2PjLTeS8rwKnJzUR4YNd4w%40mail.gmail.com
Discussion: https://postgr.es/m/475514.1612745257%40sss.pgh.pa.us
Author: Alexander Korotkov, revised by Tom Lane
Reviewed-by: Tom Lane
POSIX sigaction(2) can be told to block a set of signals while a
signal handler executes. Make use of that instead of manually
blocking and unblocking signals in the postmaster's signal handlers.
This should save a few cycles, but more importantly it prevents
recursive invocation of signal handlers when many signals arrive in
close succession. (Assuming that the platform's signal infrastructure
is designed to avoid consuming stack space in that case, but this is
demonstrably true at least on Linux.) The existing code has been seen
to recurse to the point of stack overflow, either in the postmaster
or in a forked-off child.
Back-patch of commit 9abb2bfc0. At the time, we'd only seen excess
postmaster stack consumption in the buildfarm; but we now have a
user report of it, and that commit has aged enough to have a fair
amount of confidence that it doesn't break anything.
This still doesn't change anything about the way that it works on
Windows. Perhaps someone else would like to fix that?
Per bug #16673 from David Geier. Back-patch to 9.6. Although
the problem exists in principle before that, we've only seen it
actually materialize in connection with heavy use of parallel
workers, so it doesn't seem necessary to do anything in 9.5;
and the relevant code is different there, too.
Discussion: https://postgr.es/m/16673-d278c604f8e34ec0@postgresql.org
Discussion: https://postgr.es/m/14878.1570820201@sss.pgh.pa.us
This commit back-patches the v12-era commits a73d08319, cc92cca43,
and 7570df0f3 into supported pre-v12 branches. The net effect is to
eliminate our former dependency on the never-standard sys_siglist[]
array, instead using POSIX-standard strsignal(3).
What motivates doing this now is that glibc just removed sys_siglist[]
from the set of symbols available to newly-built programs. While our
code can survive without sys_siglist[], it then fails to print any
description of the signal that killed a child process, which is a
non-negligible loss of friendliness. We can expect that people will
be wanting to build the back branches on platforms that include this
change, so we need to do something.
Since strsignal(3) has existed for quite a long time, and we've not
had any trouble with these patches so far in v12, it seems safe to
back-patch into older branches.
Discussion: https://postgr.es/m/3179114.1594853308@sss.pgh.pa.us
Writing a trailing semicolon in a macro is almost never the right thing,
because you almost always want to write a semicolon after each macro
call instead. (Even if there was some reason to prefer not to, pgindent
would probably make a hash of code formatted that way; so within PG the
rule should basically be "don't do it".) Thus, if we have a semi inside
the macro, the compiler sees "something;;". Much of the time the extra
empty statement is harmless, but it could lead to mysterious syntax
errors at call sites. In perhaps an overabundance of neatnik-ism, let's
run around and get rid of the excess semicolons whereever possible.
The only thing worse than a mysterious syntax error is a mysterious
syntax error that only happens in the back branches; therefore,
backpatch these changes where relevant, which is most of them because
most of these mistakes are old. (The lack of reported problems shows
that this is largely a hypothetical issue, but still, it could bite
us in some future patch.)
John Naylor and Tom Lane
Discussion: https://postgr.es/m/CACPNZCs0qWTqJ2QUSGJ07B7uvAvzMb-KbG2q+oo+J3tsWN5cqw@mail.gmail.com
GetLocaleInfoEx() can fail on strings that setlocale() was perfectly
happy with. A common way for that to happen is if the locale string
is actually a Unix-style string, say "et_EE.UTF-8". In that case,
what's after the dot is an encoding name, not a Windows codepage number;
blindly treating it as a codepage number led to failure, with a fairly
silly error message. Hence, check to see if what's after the dot is
all digits, and if not, treat it as a literal encoding name rather than
a codepage number. This will do the right thing with many Unix-style
locale strings, and produce a more sensible error message otherwise.
Somewhat independently of that, treat a zero (CP_ACP) result from
GetLocaleInfoEx() as meaning that we must use UTF-8 encoding.
Back-patch to all supported branches.
Juan José Santamaría Flecha
Discussion: https://postgr.es/m/24905.1585445371@sss.pgh.pa.us
src/port/getopt_long.c failed on such an argument, always seeing it
as an unrecognized switch. This is unhelpful; better is to treat such
an item as a non-switch argument. That behavior is what we find in
GNU's getopt_long(); it's what src/port/getopt.c does; and it is
required by POSIX for getopt(), which getopt_long() ought to be
generally a superset of. Moreover, it's expected by ecpg, which
intends an argument of "-" to mean "read from stdin". So fix it.
Also add some documentation about ecpg's behavior in this area, since
that was miserably underdocumented. I had to reverse-engineer it
from the code.
Per bug #16304 from James Gray. Back-patch to all supported branches,
since this has been broken forever.
Discussion: https://postgr.es/m/16304-c662b00a1322db7f@postgresql.org
This fixes a performance problem introduced by commit 6d7547c21.
ERROR_ACCESS_DENIED is returned in some other cases besides the
delete-pending case considered by that commit; notably, if the
given path names a directory instead of a plain file. In that
case we'll uselessly loop for 1 second before returning the
failure condition. That slows down some usage scenarios enough
to cause test timeout failures on our Windows buildfarm critters.
To fix, try to stat() the file, and sleep/loop only if that fails.
It will fail in the delete-pending case, and also in the case where
the deletion completed before we could stat(), so we have the cases
where we want to loop covered. In the directory case, the stat()
should succeed, letting us exit without a wait.
One case where we'll still wait uselessly is if the access-denied
problem pertains to a directory in the given pathname. But we don't
expect that to happen in any performance-critical code path.
There might be room to refine this further, but I'll push it now
in hopes of making the buildfarm green again.
Back-patch, like the preceding commit.
Alexander Lakhin and Tom Lane
Discussion: https://postgr.es/m/23073.1576626626@sss.pgh.pa.us
Attempting to open a file fails with ERROR_ACCESS_DENIED if the file
is flagged for deletion but not yet actually gone (another in a long
list of reasons why Windows is broken, if you ask me). This seems
likely to explain a lot of irreproducible failures we see in the
buildfarm. This state generally persists for only a millisecond or so,
so just wait a bit and retry. If it's a real permissions problem,
we'll eventually give up and report it as such. If it's the pending
deletion case, we'll see file-not-found and report that after the
deletion completes, and the caller will treat that in an appropriate
way.
In passing, rejigger the existing retry logic for some other error
cases so that we don't uselessly wait an extra time when we're
not going to retry anymore.
Alexander Lakhin (with cosmetic tweaks by me). Back-patch to all
supported branches, since this seems like a pretty safe change and
the problem is definitely real.
Discussion: https://postgr.es/m/16161-7a985d2f1bbe8f71@postgresql.org
It emerges that recent versions of Windows (at least 2016 Standard)
spell this locale name as "Norwegian Bokmål_Norway.1252", defeating
our mapping code that translates "Norwegian (Bokmål)_Norway" to
something that's all-ASCII (cf commits db29620d4 and aa1d2fc5e).
Add another mapping entry to handle this spelling.
Per bug #16068 from Robert Ford. Like the previous patches,
back-patch to all supported branches.
Discussion: https://postgr.es/m/16068-4cb6eeaa7eb46d93@postgresql.org
The implementation of readdir() in src/port/ which gets used by MSVC has
been added in 399a36a, and since the beginning it considers all errors
on the first file lookup as ENOENT, setting errno accordingly and
letting the routine caller think that the directory is empty. While
this is normally enough for the case of the backend, this can confuse
callers of this routine on Windows as all errors would map to the same
behavior. So, for example, even permission errors would be thought as
having an empty directory, while there could be contents in it.
This commit changes the error handling so as readdir() gets a behavior
similar to native implementations: force errno=0 when seeing
ERROR_FILE_NOT_FOUND as error and consider other errors as plain
failures.
While looking at the patch, I noticed that MinGW does not enforce
errno=0 when looking at the first file, but it gets enforced on the next
file lookups. A comment related to that was incorrect in the code.
Reported-by: Yuri Kurenkov
Diagnosed-by: Yuri Kurenkov, Grigory Smolkin
Author: Konstantin Knizhnik
Reviewed-by: Andrew Dunstan, Michael Paquier
Discussion: https://postgr.es/m/2cad7829-8d66-e39c-b937-ac825db5203d@postgrespro.ru
Backpatch-through: 9.4
Recent OpenBSD (at least 5.9 and up) has a version of getopt(3)
that will not cope with the "-:" spec we use to accept double-dash
options in postgres.c and postmaster.c. Admittedly, that's a hack
because POSIX only requires getopt() to allow alphanumeric option
characters. I have no desire to find another way, however, so
let's just do what we were already doing on Solaris: force use
of our own src/port/getopt.c implementation.
In passing, improve some of the comments around said implementation.
Per buildfarm and local testing. Back-patch to all supported branches.
Discussion: https://postgr.es/m/30197.1547835700@sss.pgh.pa.us
POSIX specifies that jrand48() returns a signed 32-bit value (in the
range [-2^31, 2^31)), but our code was returning an unsigned 32-bit
value (in the range [0, 2^32)). This doesn't actually matter to any
existing call site, because they all cast the "long" result to int32
or uint32; but it will doubtless bite somebody in the future.
To fix, cast the arithmetic result to int32 explicitly before the
compiler widens it to long (if widening is needed).
While at it, upgrade this file's far-short-of-project-style comments.
Had there been some peer pressure to document pg_jrand48() properly,
maybe this thinko wouldn't have gotten committed to begin with.
Backpatch to v10 where pg_jrand48() was added, just in case somebody
back-patches a fix that uses it and depends on the standard behavior.
Discussion: https://postgr.es/m/17235.1545951602@sss.pgh.pa.us
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.
Per report from Mateusz Guzik (via Thomas Munro).
Discussion: https://postgr.es/m/CAEepm=3VF=PUp2f8gU8fgZB22yPE_KBS0+e1AHAtQ=09schTHg@mail.gmail.com
C99 says that the result should be the number of bytes that would have
been emitted given a large enough buffer, not the number we actually
were able to put in the buffer. It's time to make our substitute
implementation comply with that. Not doing so results in inefficiency
in buffer-enlargement cases, and also poses a portability hazard for
third-party code that might expect C99-compliant snprintf behavior
within Postgres.
In passing, remove useless tests for str == NULL; neither C99 nor
predecessor standards ever allowed that except when count == 0,
so I see no reason to expend cycles on making that a non-crash case
for this implementation. Also, don't waste a byte in pg_vfprintf's
local I/O buffer; this might have performance benefits by allowing
aligned writes during flushbuffer calls.
Back-patch of commit 805889d7d. There was some concern about this
possibly breaking code that assumes pre-C99 behavior, but there is
much more risk (and reality, in our own code) of code that assumes
C99 behavior and hence fails to detect buffer overrun without this.
Discussion: https://postgr.es/m/17245.1534289329@sss.pgh.pa.us
Fix a small number of places that were testing the result of snprintf()
but doing so incorrectly. The right test for buffer overrun, per C99,
is "result >= bufsize" not "result > bufsize". Some places were also
checking for failure with "result == -1", but the standard only says
that a negative value is delivered on failure.
(Note that this only makes these places correct if snprintf() delivers
C99-compliant results. But at least now these places are consistent
with all the other places where we assume that.)
Also, make psql_start_test() and isolation_start_test() check for
buffer overrun while constructing their shell commands. There seems
like a higher risk of overrun, with more severe consequences, here
than there is for the individual file paths that are made elsewhere
in the same functions, so this seemed like a worthwhile change.
Also fix guc.c's do_serialize() to initialize errno = 0 before
calling vsnprintf. In principle, this should be unnecessary because
vsnprintf should have set errno if it returns a failure indication ...
but the other two places this coding pattern is cribbed from don't
assume that, so let's be consistent.
These errors are all very old, so back-patch as appropriate. I think
that only the shell command overrun cases are even theoretically
reachable in practice, but there's not much point in erroneous error
checks.
Discussion: https://postgr.es/m/17245.1534289329@sss.pgh.pa.us
When built using OpenSSL, pg_strong_random() uses RAND_bytes() to
generate the random number. On very rare occasions that can fail, if
its PRNG has not been seeded with enough data. Additionally, once it
does fail, all subsequent calls will also fail until more seed data is
added. Since this is required during backend startup, this can result
in all new backends failing to start until a postmaster restart.
Guard against that by checking the state of OpenSSL's PRNG using
RAND_status(), and if necessary (very rarely), seeding it using
RAND_poll().
Back-patch to v10, where pg_strong_random() was introduced.
Dean Rasheed and Michael Paquier.
Discussion: https://postgr.es/m/CAEZATCXMtxbzSAvyKKk5uCRf9pNt4UV%2BF_5v%3DgLfJUuPxU4Ytg%40mail.gmail.com
If echo = false, simple_prompt() is supposed to prevent echoing the
input (for password input). However, the Windows implementation applied
the mode change to STD_INPUT_HANDLE. That would not have the desired
effect if stdin isn't actually the terminal, for instance if the user
is piping something into psql. Fix it to apply the mode change to
the correct input file, so that passwords do not echo in such cases.
In passing, shorten and de-uglify this code by using #elif rather than
an #if nest and removing some duplicated code.
Back-patch to all supported versions. To simplify that, also back-patch
the portions of commit 9daec77e1 that got rid of an unnecessary
malloc/free in the same area.
Matthew Stickney (cosmetic changes by me)
Discussion: https://postgr.es/m/502a1fff-862b-da52-1031-f68df6ed5a2d@gmail.com
Andrew Gierth pointed out that commit 1c72ec6f4 would yield the wrong
answer on big-endian ARM systems, because the data being CRC'd would be
different. To fix that, and avoid the rather unsightly hard-wired
constant, simply compare the hardware and software implementations'
results.
While we're at it, also log the resulting decision at DEBUG1, and error
out if the hw and sw results unexpectedly differ. Also, since this
file must compile for both frontend and backend, avoid incorrect
dependencies on backend-only headers.
In passing, add a comment to postmaster.c about when the CRC function
pointer will get initialized.
Thomas Munro, based on complaints from Andrew Gierth and Tom Lane
Discussion: https://postgr.es/m/HE1PR0801MB1323D171938EABC04FFE7FA9E3110@HE1PR0801MB1323.eurprd08.prod.outlook.com
Update typedefs.list from current buildfarm results. Adjust pgindent's
typedef blacklist to block some more unfortunate typedef names that have
snuck in since last time. Manually tweak a few places where I didn't
like the initial results of pgindent'ing.
Commit 372728b0d created some problems for usages like building a
subdirectory without having first done "make all" at the top level,
or for proceeding directly to "make install" without "make all".
The only reasonably clean way to fix this seems to be to force the
submake-generated-headers rule to fire in *any* "make all" or "make
install" command anywhere in the tree. To avoid lots of redundant work,
as well as parallel make jobs possibly clobbering each others' output, we
still need to be sure that the rule fires only once in a recursive build.
For that, adopt the same MAKELEVEL hack previously used for "temp-install".
But try to document it a bit better.
The submake-errcodes mechanism previously used in src/port/ and src/common/
is subsumed by this, so we can get rid of those special cases. It was
inadequate for src/common/ anyway after the aforesaid commit, and it always
risked parallel attempts to build errcodes.h.
Discussion: https://postgr.es/m/E1f5FAB-0006LU-MB@gemulon.postgresql.org
The code before the main loop, to handle the possible 1-7 unaligned bytes
at the beginning of the input, was broken, and read past the input, if the
the input was very short.
ARMv8 introduced special CPU instructions for calculating CRC-32C. Use
them, when available, for speed.
Like with the similar Intel CRC instructions, several factors affect
whether the instructions can be used. The compiler intrinsics for them must
be supported by the compiler, and the instructions must be supported by the
target architecture. If the compilation target architecture does not
support the instructions, but adding "-march=armv8-a+crc" makes them
available, then we compile the code with a runtime check to determine if
the host we're running on supports them or not.
For the runtime check, use glibc getauxval() function. Unfortunately,
that's not very portable, but I couldn't find any more portable way to do
it. If getauxval() is not available, the CRC instructions will still be
used if the target architecture supports them without any additional
compiler flags, but the runtime check will not be available.
Original patch by Yuqi Gu, heavily modified by me. Reviewed by Andres
Freund, Thomas Munro.
Discussion: https://www.postgresql.org/message-id/HE1PR0801MB1323D171938EABC04FFE7FA9E3110%40HE1PR0801MB1323.eurprd08.prod.outlook.com
Per commit e748e902d, we appear to have little or no coverage in the
buildfarm of machines that will dump core when asked to printf a
null string pointer. Let's try to improve that situation by adding
an assertion that will make src/port/snprintf.c behave that way.
Since it's just an assertion, it won't break anything in production
builds, but it will help developers find this type of oversight.
Note that while our buildfarm coverage of machines that use that
snprintf implementation is pretty thin on the Unix side (apparently
amounting only to gaur/pademelon), all of the MSVC critters use it.
Discussion: https://postgr.es/m/156b989dbc6fe7c4d3223cf51da61195@postgrespro.ru
The previous placement of the fallback implementation in libpgcommon
was problematic, because libpqport functions need strnlen
functionality.
Move replacement into libpgport. Provide strnlen() under its posix
name, instead of pg_strnlen(). Fix stupid configure bug, executing the
test only when compiled with threading support.
Author: Andres Freund
Discussion: https://postgr.es/m/E1e1gR2-0005fB-SI@gemulon.postgresql.org
All postgres internal usages are replaced, it's just libpq example
usages that haven't been converted. External users of libpq can't
generally rely on including postgres internal headers.
Note that this includes replacing open-coded byte swapping of 64bit
integers (using two 32 bit swaps) with a single 64bit swap.
Where it looked applicable, I have removed netinet/in.h and
arpa/inet.h usage, which previously provided the relevant
functionality. It's perfectly possible that I missed other reasons for
including those, the buildfarm will tell.
Author: Andres Freund
Discussion: https://postgr.es/m/20170927172019.gheidqy6xvlxb325@alap3.anarazel.de
Don't move parenthesized lines to the left, even if that means they
flow past the right margin.
By default, BSD indent lines up statement continuation lines that are
within parentheses so that they start just to the right of the preceding
left parenthesis. However, traditionally, if that resulted in the
continuation line extending to the right of the desired right margin,
then indent would push it left just far enough to not overrun the margin,
if it could do so without making the continuation line start to the left of
the current statement indent. That makes for a weird mix of indentations
unless one has been completely rigid about never violating the 80-column
limit.
This behavior has been pretty universally panned by Postgres developers.
Hence, disable it with indent's new -lpl switch, so that parenthesized
lines are always lined up with the preceding left paren.
This patch is much less interesting than the first round of indent
changes, but also bulkier, so I thought it best to separate the effects.
Discussion: https://postgr.es/m/E1dAmxK-0006EE-1r@gemulon.postgresql.org
Discussion: https://postgr.es/m/30527.1495162840@sss.pgh.pa.us
Change pg_bsd_indent to follow upstream rules for placement of comments
to the right of code, and remove pgindent hack that caused comments
following #endif to not obey the general rule.
Commit e3860ffa4d wasn't actually using
the published version of pg_bsd_indent, but a hacked-up version that
tried to minimize the amount of movement of comments to the right of
code. The situation of interest is where such a comment has to be
moved to the right of its default placement at column 33 because there's
code there. BSD indent has always moved right in units of tab stops
in such cases --- but in the previous incarnation, indent was working
in 8-space tab stops, while now it knows we use 4-space tabs. So the
net result is that in about half the cases, such comments are placed
one tab stop left of before. This is better all around: it leaves
more room on the line for comment text, and it means that in such
cases the comment uniformly starts at the next 4-space tab stop after
the code, rather than sometimes one and sometimes two tabs after.
Also, ensure that comments following #endif are indented the same
as comments following other preprocessor commands such as #else.
That inconsistency turns out to have been self-inflicted damage
from a poorly-thought-through post-indent "fixup" in pgindent.
This patch is much less interesting than the first round of indent
changes, but also bulkier, so I thought it best to separate the effects.
Discussion: https://postgr.es/m/E1dAmxK-0006EE-1r@gemulon.postgresql.org
Discussion: https://postgr.es/m/30527.1495162840@sss.pgh.pa.us
The new indent version includes numerous fixes thanks to Piotr Stefaniak.
The main changes visible in this commit are:
* Nicer formatting of function-pointer declarations.
* No longer unexpectedly removes spaces in expressions using casts,
sizeof, or offsetof.
* No longer wants to add a space in "struct structname *varname", as
well as some similar cases for const- or volatile-qualified pointers.
* Declarations using PG_USED_FOR_ASSERTS_ONLY are formatted more nicely.
* Fixes bug where comments following declarations were sometimes placed
with no space separating them from the code.
* Fixes some odd decisions for comments following case labels.
* Fixes some cases where comments following code were indented to less
than the expected column 33.
On the less good side, it now tends to put more whitespace around typedef
names that are not listed in typedefs.list. This might encourage us to
put more effort into typedef name collection; it's not really a bug in
indent itself.
There are more changes coming after this round, having to do with comment
indentation and alignment of lines appearing within parentheses. I wanted
to limit the size of the diffs to something that could be reviewed without
one's eyes completely glazing over, so it seemed better to split up the
changes as much as practical.
Discussion: https://postgr.es/m/E1dAmxK-0006EE-1r@gemulon.postgresql.org
Discussion: https://postgr.es/m/30527.1495162840@sss.pgh.pa.us
