The early buildfarm returns for commit 1e165d05 are pretty awful:
not only does Windows not return a useful error, but it looks like
a lot of Unix-ish platforms don't either.  Given the number of
different errnos seen so far, guess that what's really going on is
that some newlocale() implementations fail to set errno at all.
Hence, let's try zeroing errno just before newlocale() and then
if it's still zero report as though it's ENOENT.  That should cover
the Windows case too.

It's clear that we'll have to drop the regression test case, unless
we want to maintain a separate expected-file for platforms without
HAVE_LOCALE_T.  But I'll leave it there awhile longer to see if this
actually improves matters or not.

Discussion: https://postgr.es/m/CAKKotZS-wcDcofXDCH=sidiuajE+nqHn2CGjLLX78anyDmi3gQ@mail.gmail.com

DROP CASCADE doesn't currently promise to visit dependent objects in
a fixed order, so when the regression tests use it, we typically need
to suppress the details of which objects get dropped in order to have
predictable test output.  Traditionally we've done that by setting
client_min_messages higher than NOTICE, but there's a better way:
we can "\set VERBOSITY terse" in psql.  That suppresses the DETAIL
message with the object list, but we still get the basic notice telling
how many objects were dropped.  So at least the test case can verify
that the expected number of objects were dropped.

The VERBOSITY method was already in use in a few places, but run
around and use it wherever it makes sense.

Discussion: https://postgr.es/m/10766.1501608885@sss.pgh.pa.us

We were just printing errno, which is certainly not gonna work on
Windows.  Now, it's not entirely clear from Microsoft's documentation
whether _create_locale() adheres to standard Windows error reporting
conventions, but let's assume it does and try to map the GetLastError
result to an errno.  If this turns out not to work, probably the best
thing to do will be to assume the error is always ENOENT on Windows.

This is a longstanding bug, but given the lack of previous field
complaints, I'm not excited about back-patching it.

Per report from Murtuza Zabuawala.

Discussion: https://postgr.es/m/CAKKotZS-wcDcofXDCH=sidiuajE+nqHn2CGjLLX78anyDmi3gQ@mail.gmail.com

Author: Fabien COELHO <coelho@cri.ensmp.fr>

Most of our collations code has special handling for the locale names
"C" and "POSIX", allowing those collations to be used whether or not
the system libraries think those locale names are valid, or indeed
whether said libraries even have any locale support.  But we missed
handling things that way in CREATE COLLATION.  This meant you couldn't
clone the C/POSIX collations, nor explicitly define a new collation
using those locale names, unless the libraries allow it.  That's pretty
pointless, as well as being a violation of pg_newlocale_from_collation's
API specification.

The practical effect of this change is quite limited: it allows creating
such collations even on platforms that don't HAVE_LOCALE_T, and it allows
making "POSIX" collation objects on Windows, which before this would only
let you make "C" collation objects.  Hence, even though this is a bug fix
IMO, it doesn't seem worth the trouble to back-patch.

In passing, suppress the DROP CASCADE detail messages at the end of the
collation regression test.  I'm surprised we've never been bit by
message ordering issues there.

Per report from Murtuza Zabuawala.

Discussion: https://postgr.es/m/CAKKotZS-wcDcofXDCH=sidiuajE+nqHn2CGjLLX78anyDmi3gQ@mail.gmail.com

Peter Eisentraut noted that commit 40b9f192 had broken a configure
behavior that some people might rely on: AC_CHECK_PROGS(FOO,...) will
allow the search to be overridden by specifying a value for FOO on
configure's command line or in its environment, but AC_PATH_PROGS(FOO,...)
accepts such an override only if it's an absolute path.  We had worked
around that behavior for some, but not all, of the pre-existing uses
of AC_PATH_PROGS by just skipping the macro altogether when FOO is
already set.  Let's standardize on that workaround for all uses of
AC_PATH_PROGS, new and pre-existing, by wrapping AC_PATH_PROGS in a
new macro PGAC_PATH_PROGS.  While at it, fix a deficiency of the old
workaround code by making sure we report the setting to configure's log.

Eventually I'd like to improve PGAC_PATH_PROGS so that it converts
non-absolute override inputs to absolute form, eg "PYTHON=python3"
becomes, say, PYTHON = /usr/bin/python3.  But that will take some
nontrivial coding so it doesn't seem like a thing to do in late beta.

Discussion: https://postgr.es/m/90a92a7d-938e-507a-3bd7-ecd2b4004689@2ndquadrant.com

This was an oversight in d363d42b.

Beena Emerson

XLByteToSeg and XLByteToPrevSeg calculate only a segment number.  The
definition of these macros were modified by commit
dfda6eba but the comment remain
unchanged.

Patch by Yugo Nagata. Back patched to 9.3 and beyond.

Author: Masahiko Sawada <sawada.mshk@gmail.com>

Author: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>

1024 bits is considered weak these days, but OpenSSL always passes 1024 as
the key length to the tmp_dh callback. All the code to handle other key
lengths is, in fact, dead.

To remedy those issues:

* Only include hard-coded 2048-bit parameters.
* Set the parameters directly with SSL_CTX_set_tmp_dh(), without the
  callback
* The name of the file containing the DH parameters is now a GUC. This
  replaces the old hardcoded "dh1024.pem" filename. (The files for other
  key lengths, dh512.pem, dh2048.pem, etc. were never actually used.)

This is not a new problem, but it doesn't seem worth the risk and churn to
backport. If you care enough about the strength of the DH parameters on
old versions, you can create custom DH parameters, with as many bits as you
wish, and put them in the "dh1024.pem" file.

Per report by Nicolas Guini and Damian Quiroga. Reviewed by Michael Paquier.

Discussion: https://www.postgresql.org/message-id/CAMxBoUyjOOautVozN6ofzym828aNrDjuCcOTcCquxjwS-L2hGQ@mail.gmail.com

Previously the docs just said "5.8 or later".  Experimentation shows
that while you can build on Unix from a git checkout with 5.8.0,
compiling recent PL/Perl requires at least 5.8.1, and you won't be
able to run the TAP tests with less than 5.8.3 because that's when
they added "prove".  (I do not have any information on just what the
MSVC build scripts require.)

Since all these versions are quite ancient, let's not split hairs
in the docs, but just say that 5.8.3 is the minimum requirement.

Discussion: https://postgr.es/m/16894.1501392088@sss.pgh.pa.us

Previously we had a mix of uses of AC_CHECK_PROG[S] and AC_PATH_PROG[S].
The only difference between those macros is that the latter emits the
full path to the program it finds, eg "/usr/bin/prove", whereas the
former emits just "prove".  Let's standardize on always emitting the
full path; this is better for documentation of the build, and it might
prevent some types of failures if later build steps are done with
a different PATH setting.

I did not touch the AC_CHECK_PROG[S] calls in ax_pthread.m4 and
ax_prog_perl_modules.m4.  There seems no need to make those diverge from
upstream, since we do not record the programs sought by the former, while
the latter's call to AC_CHECK_PROG(PERL,...) will never be reached.

Discussion: https://postgr.es/m/25937.1501433410@sss.pgh.pa.us

Coverity complained about this code's practice of using scalar variables
as single-element arrays.  While that's really just nitpicking, it probably
is more readable to declare them as arrays, so let's do that.  A more
important point is that the code was just blithely assuming that the
result tupledesc has exactly one column; if it doesn't, we'd likely get
a crash of some sort in tuplestore_putvalues.  Since the tupledesc is
manufactured outside of plperl, that seems like an uncomfortably long
chain of assumptions.  We can nail it down at little cost with a sanity
check earlier in the function.

The comment for dumpACL() got neglected when initacls and initracls were
added and the discussion of what 'racls' is wasn't very clear either.

Per complaint from Tom.

current_source requires to restart server to reflect the new
value. Per Yugo Nagata and Masahiko Sawada.

Back patched to 9.2 and beyond.

dynamic_shared_memory_type requires to restart server to reflect
the new value. Per Yugo Nagata and Masahiko Sawada.

Back pached to 9.4 and beyond.

max_logical_replication_workers requires to restart server to reflect
the new value. Per Yugo Nagata. Minor editing by me.

This allows us to add stack-depth checks the first time an executor
node is called, and skip that overhead on following
calls. Additionally it yields a nice speedup.

While it'd probably have been a good idea to have that check all
along, it has become more important after the new expression
evaluation framework in b8d7f053 - there's no stack depth
check in common paths anymore now. We previously relied on
ExecEvalExpr() being executed somewhere.

We should move towards that model for further routines, but as this is
required for v10, it seems better to only do the necessary (which
already is quite large).

Author: Andres Freund, Tom Lane
Reported-By: Julien Rouhaud
Discussion:
    https://postgr.es/m/22833.1490390175@sss.pgh.pa.us
    https://postgr.es/m/b0af9eaa-130c-60d0-9e4e-7a135b1e0c76@dalibo.com

In a followup commit ExecProcNode(), and especially the large switch
it contains, will largely be replaced by a function pointer directly
to the correct node. The node functions will then get invoked by a
thin inline function wrapper. To avoid having to include miscadmin.h
in headers - CHECK_FOR_INTERRUPTS() - move the interrupt checks into
the individual executor routines.

While looking through all executor nodes, I noticed a number of
arguably missing interrupt checks, add these too.

Author: Andres Freund, Tom Lane
Reviewed-By: Tom Lane
Discussion:
    https://postgr.es/m/22833.1490390175@sss.pgh.pa.us

Without this, \dRp prints information that \dRp+ does not, which
seems pretty odd.

Daniel Gustafsson

Discussion: https://postgr.es/m/3641F19B-336A-431A-86CE-A80562505C5E@yesql.se

The Perl documentation is very clear that stuff calling libperl should
be built with the compiler switches shown by Perl's $Config{ccflags}.
We'd been ignoring that up to now, and mostly getting away with it,
but recent Perl versions contain ABI compatibility cross-checks that
fail on some builds because of this omission.  In particular the
sizeof(PerlInterpreter) can come out different due to some fields being
added or removed; which means we have a live ABI hazard that we'd better
fix rather than continuing to sweep it under the rug.

However, it still seems like a bad idea to just absorb $Config{ccflags}
verbatim.  In some environments Perl was built with a different compiler
that doesn't even use the same switch syntax.  -D switch syntax is pretty
universal though, and absorbing Perl's -D switches really ought to be
enough to fix the problem.

Furthermore, Perl likes to inject stuff like -D_LARGEFILE_SOURCE and
-D_FILE_OFFSET_BITS=64 into $Config{ccflags}, which affect libc ABIs on
platforms where they're relevant.  Adopting those seems dangerous too.
It's unclear whether a build wherein Perl and Postgres have different ideas
of sizeof(off_t) etc would work, or whether anyone would care about making
it work.  But it's dead certain that having different stdio ABIs in
core Postgres and PL/Perl will not work; we've seen that movie before.
Therefore, let's also ignore -D switches for symbols beginning with
underscore.  The symbols that we actually need to import should be the ones
mentioned in perl.h's PL_bincompat_options stanza, and none of those start
with underscore, so this seems likely to work.  (If it turns out not to
work everywhere, we could consider intersecting the symbols mentioned in
PL_bincompat_options with the -D switches.  But that will be much more
complicated, so let's try this way first.)

This will need to be back-patched, but first let's see what the
buildfarm makes of it.

Ashutosh Sharma, some adjustments by me

Discussion: https://postgr.es/m/CANFyU97OVQ3+Mzfmt3MhuUm5NwPU=-FtbNH5Eb7nZL9ua8=rcA@mail.gmail.com

In Perl builds that define PERL_IMPLICIT_SYS, XSUB.h defines macros
that replace a whole lot of basic libc functions with Perl functions.
We can't tolerate that in plperl.c; it breaks at least PG_TRY and
probably other stuff.  The core idea of this patch is to include XSUB.h
only in the .xs files where it's really needed, and to move any code
broken by PERL_IMPLICIT_SYS out of the .xs files and into plperl.c.

The reason this hasn't been a problem before is that our build techniques
did not result in PERL_IMPLICIT_SYS appearing as a #define in PL/Perl,
even on some platforms where Perl thinks it is defined.  That's about to
change in order to fix a nasty portability issue, so we need this work to
make the code safe for that.

Rather unaccountably, the Perl people chose XSUB.h as the place to provide
the versions of the aTHX/aTHX_ macros that are needed by code that's not
explicitly aware of the MULTIPLICITY API conventions.  Hence, just removing
XSUB.h from plperl.c fails miserably.  But we can work around that by
defining PERL_NO_GET_CONTEXT (which would make the relevant stanza of
XSUB.h a no-op anyway).  As explained in perlguts.pod, that means we need
to add a "dTHX" macro call in every C function that calls a Perl API
function.  In most of them we just add this at the top; but since the
macro fetches the current Perl interpreter pointer, more care is needed
in functions that switch the active interpreter.  Lack of the macro is
easily recognized since it results in bleats about "my_perl" not being
defined.

(A nice side benefit of this is that it significantly reduces the number
of fetches of the current interpreter pointer.  On my machine, plperl.so
gets more than 10% smaller, and there's probably some performance win too.
We could reduce the number of fetches still more by decorating the code
with pTHX_/aTHX_ macros to pass the interpreter pointer around, as
explained by perlguts.pod; but that's a task for another day.)

Formatting note: pgindent seems happy to treat "dTHX;" as a declaration
so long as it's the first thing after the left brace, as we'd already
observed with respect to the similar macro "dSP;".  If you try to put
it later in a set of declarations, pgindent puts ugly extra space
around it.

Having removed XSUB.h from plperl.c, we need only move the support
functions for spi_return_next and util_elog (both of which use PG_TRY)
out of the .xs files and into plperl.c.  This seems sufficient to
avoid the known problems caused by PERL_IMPLICIT_SYS, although we
could move more code if additional issues emerge.

This will need to be back-patched, but first let's see what the
buildfarm makes of it.

Patch by me, with some help from Ashutosh Sharma

Discussion: https://postgr.es/m/CANFyU97OVQ3+Mzfmt3MhuUm5NwPU=-FtbNH5Eb7nZL9ua8=rcA@mail.gmail.com

After typing CREATE USER M..., it would not fill in MAPPING FOR,
even though that was clearly intended behavior.

Jeff Janes

Discussion: https://postgr.es/m/CAMkU=1wo2iQ6jWnN=egqOb5NxEPn0PpANEtKHr3uPooQ+nYPtw@mail.gmail.com

Most functions in this file are content to print an empty table if there
are no matching objects.  In some, the behavior is to loop over all
matching objects and print a table for each one; therefore, without any
extra logic, nothing at all would be printed if no objects match.
We accept that outcome in QUIET mode, but in normal mode it seems better
to print a helpful message.  The new \dRp+ command had not gotten that
memo; fix it.

listDbRoleSettings() is out of step on this, but I think it's better for
it to print a custom message rather than an empty table, because of the
possibility that the user is confused about what the pattern arguments mean
or which is which.  The original message wording was entirely useless for
clarifying that, though, not to mention being unlike the wordings used
elsewhere.  Improve the text, and also print the messages with psql_error
as is the general custom here.

listTables() is also out in left field, but since it's such a heavily
used function, I'm hesitant to change its behavior so much as to print
an empty table rather than a custom message.  People are probably used
to getting a message.  But we can make the wording more standardized and
helpful, and print it with psql_error rather than printing to stdout.

In both listDbRoleSettings and listTables, we play dumb and emit an
empty table, not a custom message, in QUIET mode.  That was true before
and I see no need to change it.

Several of the places printing such messages risked dumping core if
no pattern string had been provided; make them more wary.  (This case
is presently unreachable in describeTableDetails; but it shouldn't be
assuming that command.c will never pass it a null.  The text search
functions would only reach the case if a database contained no text
search objects, which is also currently impossible since we pin the
built-in objects, but again it seems unwise to assume that here.)

Daniel Gustafsson, tweaked a bit by me

Discussion: https://postgr.es/m/3641F19B-336A-431A-86CE-A80562505C5E@yesql.se

Most places were already using the PQExpBuffer library for constructing
variable-length strings; bring the two stragglers into line.
describeOneTSParser was living particularly dangerously since it wasn't
even using snprintf().

Daniel Gustafsson

Discussion: https://postgr.es/m/3641F19B-336A-431A-86CE-A80562505C5E@yesql.se

listDbRoleSettings() handled its server version check randomly differently
from every other comparable function in describe.c, not only as to code
layout but also message wording.  It also leaked memory, because its
PQExpBuffer management was also unlike everyplace else (and wrong).

Also fix an error-case leak in add_tablespace_footer().

In passing, standardize the format of function header comments in
describe.c --- we usually put "/*" alone on a line.

Daniel Gustafsson, memory leak fixes by me

Discussion: https://postgr.es/m/3641F19B-336A-431A-86CE-A80562505C5E@yesql.se

\drds leaked its second pattern argument if any, and \connect leaked
any empty-string or "-" arguments.  These are old bugs, but it's hard
to imagine any real use-case where the leaks could amount to anything
meaningful, so not bothering with a back-patch.

Daniel Gustafsson and Tom Lane

Discussion: https://postgr.es/m/3641F19B-336A-431A-86CE-A80562505C5E@yesql.se

When output of IPC::Run::run () is redirected to scalar references, in
certain circumstances the Msys perl does not correctly detect that the
end of file has been seen, making the test hang indefinitely. One such
circumstance is when the command is 'pg_ctl start', and such a change
was made in commit f13ea95f. The workaround, which only applies on
MSys, is to redirect the output to temporary files and then read them in
when the process has finished.

Patch by me, reviewed and tweaked by Tom Lane.

Fix assorted places that had not bothered with the convention of
prefixing catalog and function names with "pg_catalog.".  That
could possibly result in query failure when running with a nondefault
search_path.  Also fix two places that weren't quoting OID literals.
I think the latter hasn't mattered much since about 7.3, but it's still
a bad idea to be doing it in 99 places and not in 2 others.

Also remove a useless EXISTS sub-select that someone had stuck into
describeOneTableDetails' queries for child tables.  We just got the OID
out of pg_class, so I hardly see how checking that it exists in pg_class
was doing anything helpful.

In passing, try to improve the emitted formatting of a couple of
these queries, though I didn't work really hard on that.  And merge
unnecessarily duplicative coding in some other places.

Much of this was new in HEAD, but some was quite old; back-patch
as appropriate.

If several sessions are concurrently locking a tuple update chain with
nonconflicting lock modes using an old snapshot, and they all succeed,
it may happen that some of them fail because of restarting the loop (due
to a concurrent Xmax change) and getting an error in the subsequent pass
while trying to obtain a tuple lock that they already have in some tuple
version.

This can only happen with very high concurrency (where a row is being
both updated and FK-checked by multiple transactions concurrently), but
it's been observed in the field and can have unpleasant consequences
such as an FK check failing to see a tuple that definitely exists:
    ERROR:  insert or update on table "child_table" violates foreign key constraint "fk_constraint_name"
    DETAIL:  Key (keyid)=(123456) is not present in table "parent_table".
(where the key is observably present in the table).

Discussion: https://postgr.es/m/20170714210011.r25mrff4nxjhmf3g@alvherre.pgsql

Initial submitted versions of the functional dependencies patch ignored
row groups that were smaller than a configured size.  However, that
consideration was removed in late stages of the patch just before
commit, but some comments referring to it remained.  Remove them to
avoid confusion.

Author: Atsushi Torikoshi
Discussion: https://postgr.es/m/7cfb23fc-4493-9c02-5da9-e505fd0115d2@lab.ntt.co.jp

This module becomes much more useful if we allow it to be used as base
class for external projects.  To achieve this, change the exported
get_new_node function into a class method instead, and use the standard
Perl idiom of accepting the class as first argument.  This method works
as expected for subclasses.  The standalone function is kept for
backwards compatibility, though it could be removed in pg11.

Author: Chap Flackman, based on an earlier patch from Craig Ringer
Discussion: https://postgr.es/m/CAMsr+YF8kO+4+K-_U4PtN==2FndJ+5Bn6A19XHhMiBykEwv0wA@mail.gmail.com

It is relatively easy to get a replication slot to look as still active
while one process is in the process of getting rid of it; when some
other process tries to "acquire" the slot, it would fail with an error
message of "replication slot XYZ is active for PID N".

The error message in itself is fine, except that when the intention is
to drop the slot, it is unhelpful: the useful behavior would be to wait
until the slot is no longer acquired, so that the drop can proceed.  To
implement this, we use a condition variable so that slot acquisition can
be told to wait on that condition variable if the slot is already
acquired, and we make any change in active_pid broadcast a signal on the
condition variable.  Thus, as soon as the slot is released, the drop
will proceed properly.

Reported by: Tom Lane
Discussion: https://postgr.es/m/11904.1499039688@sss.pgh.pa.us
Authors: Petr Jelínek, Álvaro Herrera

In various places where we reverse-map a tuple before calling
ExecBuildSlotValueDescription, we neglected to ensure that the
slot descriptor matched the tuple stored in it.

Amit Langote and Amit Khandekar, reviewed by Etsuro Fujita

Discussion: http://postgr.es/m/CAJ3gD9cqpP=WvJj=dv1ONkPWjy8ZuUaOM4_x86i3uQPas=0_jg@mail.gmail.com

Trading a little too heavily on letting the code path be the same whether
we were creating shared data structures or only attaching to them,
InitPredicateLocks() inserted the "scratch" PredicateLockTargetHash entry
unconditionally.  This is just wrong if we're in a postmaster child,
which would only reach this code in EXEC_BACKEND builds.  Most of the
time, the hash_search(HASH_ENTER) call would simply report that the
entry already existed, causing no visible effect since the code did not
bother to check for that possibility.  However, if this happened while
some other backend had transiently removed the "scratch" entry, then
that other backend's eventual RestoreScratchTarget would suffer an
assert failure; this appears to be the explanation for a recent failure
on buildfarm member culicidae.  In non-assert builds, there would be
no visible consequences there either.  But nonetheless this is a pretty
bad bug for EXEC_BACKEND builds, for two reasons:

1. Each new backend would perform the hash_search(HASH_ENTER) call
without holding any lock that would prevent concurrent access to the
PredicateLockTargetHash hash table.  This creates a low but certainly
nonzero risk of corruption of that hash table.

2. In the event that the race condition occurred, by reinserting the
scratch entry too soon, we were defeating the entire purpose of the
scratch entry, namely to guarantee that transaction commit could move
hash table entries around with no risk of out-of-memory failure.
The odds of an actual OOM failure are quite low, but not zero, and if
it did happen it would again result in corruption of the hash table.

The user-visible symptoms of such corruption are a little hard to predict,
but would presumably amount to misbehavior of SERIALIZABLE transactions
that'd require a crash or postmaster restart to fix.

To fix, just skip the hash insertion if IsUnderPostmaster.  I also
inserted a bunch of assertions that the expected things happen
depending on whether IsUnderPostmaster is true.  That might be overkill,
since most comparable code in other functions isn't quite that paranoid,
but once burnt twice shy.

In passing, also move a couple of lines to places where they seemed
to make more sense.

Diagnosis of problem by Thomas Munro, patch by me.  Back-patch to
all supported branches.

Discussion: https://postgr.es/m/10593.1500670709@sss.pgh.pa.us

If the user modifies a view that has CHECK OPTIONs and this gets
translated into a modification to an underlying relation which happens
to be a foreign table, the check options should be enforced.  In the
normal code path, that was happening properly, but it was not working
properly for "direct" modification because the whole operation gets
pushed to the remote side in that case and we never have an option to
enforce the constraint against individual tuples.  Fix by disabling
direct modification when there is a need to enforce CHECK OPTIONs.

Etsuro Fujita, reviewed by Kyotaro Horiguchi and by me.

Discussion: http://postgr.es/m/f8a48f54-6f02-9c8a-5250-9791603171ee@lab.ntt.co.jp

Various cases involving renaming of view columns are handled by having
make_viewdef pass down the view's current relation tupledesc to
get_query_def, which then takes care to use the column names from the
tupledesc for the output column names of the SELECT.  For some reason
though, we'd missed teaching make_ruledef to do similarly when it is
printing an ON SELECT rule, even though this is exactly the same case.
The results from pg_get_ruledef would then be different and arguably wrong.
In particular, this breaks pre-v10 versions of pg_dump, which in some
situations would define views by means of emitting a CREATE RULE ... ON
SELECT command.  Third-party tools might not be happy either.

In passing, clean up some crufty code in make_viewdef; we'd apparently
modernized the equivalent code in make_ruledef somewhere along the way,
and missed this copy.

Per report from Gilles Darold.  Back-patch to all supported versions.

Discussion: https://postgr.es/m/ec05659a-40ff-4510-fc45-ca9d965d0838@dalibo.com