Reported-by: Alexander Law

Discussion: 5786638C.8080508@gmail.com

Backpatch-through: 9.4

TupleQueueReaderNext() leaks like a sieve if it has to do any tuple
disassembly/reconstruction.  While we could try to clean up its allocations
piecemeal, it seems like a better idea just to insist that it should be run
in a short-lived memory context, so that any transient space goes away
automatically.  I chose to have nodeGather.c switch into its existing
per-tuple context before the call, rather than inventing a separate
context inside tqueue.c.

This is sufficient to stop all leakage in the simple case I exhibited
earlier today (see link below), but it does not deal with leaks induced
in more complex cases by tqueue.c's insistence on using TopMemoryContext
for data that it's not actually trying hard to keep track of.  That issue
is intertwined with another major source of inefficiency, namely failure
to cache lookup results across calls, so it seems best to deal with it
separately.

In passing, improve some comments, and modify gather_readnext's method for
deciding when it's visited all the readers so that it's more obviously
correct.  (I'm not actually convinced that the previous code *is*
correct in the case of a reader deletion; it certainly seems fragile.)

Discussion: <32763.1469821037@sss.pgh.pa.us>

It's depressingly clear that nobody ever tested this.

An evident copy-and-pasteo in commit 2bd9e412 broke the non-blocking
aspect of pq_putmessage_noblock(), causing it to behave identically to
pq_putmessage().  That function is nowadays used only in walsender.c,
so that the net effect was to cause walsenders to hang up waiting for
the receiver in situations where they should not.

Kyotaro Horiguchi

Patch: <20160728.185228.58375982.horiguchi.kyotaro@lab.ntt.co.jp>

Michael Paquier

Per the fgets() specification, it cannot return without reading some data
unless it reports EOF or error.  So the code here assumed that the data
buffer would necessarily be nonempty when we go to check for a newline
having been read.  However, Agostino Sarubbo noticed that this could fail
to be true if the first byte of the data is a NUL (\0).  The fgets() API
doesn't really work for embedded NULs, which is something I don't feel
any great need for us to worry about since we generally don't allow NULs
in SQL strings anyway.  But we should not access off the end of our own
buffer if the case occurs.  Normally this would just be a harmless read,
but if you were unlucky the byte before the buffer would contain '\n'
and we'd overwrite it with '\0', and if you were really unlucky that
might be valuable data and psql would crash.

Agostino reported this to pgsql-security, but after discussion we concluded
that it isn't worth treating as a security bug; if you can control the
input to psql you can do far more interesting things than just maybe-crash
it.  Nonetheless, it is a bug, so back-patch to all supported versions.

Now that we've nailed down the principle that NullTest with !argisrow
is fully equivalent to SQL's IS [NOT] DISTINCT FROM NULL, let's teach
the parser about it.  This produces a slightly more compact parse tree
and is much more amenable to optimization than a DistinctExpr, since
the planner knows a good deal about NullTest and next to nothing about
DistinctExpr.

I'm not sure that there are all that many queries in the wild that could
be improved by this, but at least one source of such cases is the patch
just made to postgres_fdw to emit IS [NOT] DISTINCT FROM NULL when
IS [NOT] NULL isn't semantically correct.

No back-patch, since to the extent that this does affect planning results,
it might be considered undesirable plan destabilization.

Commits 4452000f et al established semantics for NullTest.argisrow that
are a bit different from its initial conception: rather than being merely
a cache of whether we've determined the input to have composite type,
the flag now has the further meaning that we should apply field-by-field
testing as per the standard's definition of IS [NOT] NULL.  If argisrow
is false and yet the input has composite type, the construct instead has
the semantics of IS [NOT] DISTINCT FROM NULL.  Update the comments in
primnodes.h to clarify this, and fix ruleutils.c and deparse.c to print
such cases correctly.  In the case of ruleutils.c, this merely results in
cosmetic changes in EXPLAIN output, since the case can't currently arise
in stored rules.  However, it represents a live bug for deparse.c, which
would formerly have sent a remote query that had semantics different
from the local behavior.  (From the user's standpoint, this means that
testing a remote nested-composite column for null-ness could have had
unexpected recursive behavior much like that fixed in 4452000f.)

In a related but somewhat independent fix, make plancat.c set argisrow
to false in all NullTest expressions constructed to represent "attnotnull"
constructs.  Since attnotnull is actually enforced as a simple null-value
check, this is a more accurate representation of the semantics; we were
previously overpromising what it meant for composite columns, which might
possibly lead to incorrect planner optimizations.  (It seems that what the
SQL spec expects a NOT NULL constraint to mean is an IS NOT NULL test, so
arguably we are violating the spec and should fix attnotnull to do the
other thing.  If we ever do, this part should get reverted.)

Back-patch, same as the previous commit.

Discussion: <10682.1469566308@sss.pgh.pa.us>

The docs failed to explain that LIKE INCLUDING INDEXES would not preserve
the names of indexes and associated constraints.  Also, it wasn't mentioned
that EXCLUDE constraints would be copied by this option.  The latter
oversight seems enough of a documentation bug to justify back-patching.

In passing, do some minor copy-editing in the same area, and add an entry
for LIKE under "Compatibility", since it's not exactly a faithful
implementation of the standard's feature.

Discussion: <20160728151154.AABE64016B@smtp.hushmail.com>

start_postmaster() registered stop_postmaster_atexit as an atexit(3)
callback each time through, although the obvious intention was to do
so only once per program run.  The extra registrations were harmless,
so long as we didn't exceed ATEXIT_MAX, but still it's a bug.

Artur Zakirov, with bikeshedding by Kyotaro Horiguchi and me

Discussion: <d279e817-02b5-caa6-215f-cfb05dce109a@postgrespro.ru>

The description of udt_privileges view contained an incorrect copy-pasted word.

Back-patch to 9.2 where udt_privileges view was added.

Author: Alexander Law

The keys are integers, not strings.  The code accidentally worked on
little-endian machines, at least up to 256 distinct record types within
a session, but failed utterly on big-endian.  This was unexpectedly
exposed by a test case added by commit 4452000f, which apparently is the
only parallelizable query in the regression suite that uses more than one
anonymous record type.  Fortunately, buildfarm member mandrill is
big-endian and is running with force_parallel_mode on, so it failed.

cost_rescan assumed that we don't need to rebuild the hash table when
rescanning a hash join.  However, that's currently only true for
single-batch joins; for a multi-batch join we must charge full freight.

This probably has escaped notice because we'd be unlikely to put a hash
join on the inside of a nestloop anyway.  Nonetheless, it's wrong.
Fix in HEAD, but don't backpatch for fear of destabilizing plans in
stable releases.

There's no point in consulting retval->paramMask; it's always NULL.
Instead, we should consult from->paramMask.

Reported by Andrew Gierth.

ExecMakeTableFunctionResult(), which is used in SELECT FROM function(...)
cases, formerly treated a simple NULL output from a function that both
returnsSet and returnsTuple as a violation of the SRF protocol.  What seems
better is to treat a NULL output as equivalent to ROW(NULL,NULL,...).
Without this, cases such as SELECT FROM unnest(...) on an array of
composite are vulnerable to unexpected and not-very-helpful failures.
Old code comments here suggested an alternative of just ignoring
simple-NULL outputs, but that doesn't seem very principled.

This change had been hung up for a long time due to uncertainty about
how much we wanted to buy into the equivalence of simple NULL and
ROW(NULL,NULL,...).  I think that's been mostly resolved by the discussion
around bug #14235, so let's go ahead and do it.

Per bug #7808 from Joe Van Dyk.  Although this is a pretty old report,
fixing it smells a bit more like a new feature than a bug fix, and the
lack of other similar complaints suggests that we shouldn't take much risk
of destabilization by back-patching.  (Maybe that could be revisited once
this patch has withstood some field usage.)

Andrew Gierth and Tom Lane

Report: <E1TurJE-0006Es-TK@wrigleys.postgresql.org>

Previously, some functions returned various fixed strings and others
failed with a cache lookup error.  Per discussion, standardize on
returning NULL.  Although user-exposed "cache lookup failed" error
messages might normally qualify for bug-fix treatment, no back-patch;
the risk of breaking user code which is accustomed to the current
behavior seems too high.

Michael Paquier

That script is incorrect in that it sets the combine function for
max(citext) twice instead of setting the combine function for
max(citext) once and the combine functon for min(citext) once.  The
consequence is that if you install 1.0 or 1.1 and then update to 1.2,
you end up with min(citext) not having a combine function, contrary to
what was intended.  If you install 1.2 directly, you're OK.

Fix things up by defining a new 1.3 version.  Upgrading from 1.2 to
1.3 won't change anything for people who first installed the 1.2
version, but people upgrading from 1.0 or 1.1 will get the right
catalog contents once they reach 1.3.

Report and patch by David Rowley, reviewed by Andreas Karlsson.

The SQL standard appears to specify that IS [NOT] NULL's tests of field
nullness are non-recursive, ie, we shouldn't consider that a composite
field with value ROW(NULL,NULL) is null for this purpose.
ExecEvalNullTest got this right, but eval_const_expressions did not,
leading to weird inconsistencies depending on whether the expression
was such that the planner could apply constant folding.

Also, adjust the docs to mention that IS [NOT] DISTINCT FROM NULL can be
used as a substitute test if a simple null check is wanted for a rowtype
argument.  That motivated reordering things so that IS [NOT] DISTINCT FROM
is described before IS [NOT] NULL.  In HEAD, I went a bit further and added
a table showing all the comparison-related predicates.

Per bug #14235.  Back-patch to all supported branches, since it's certainly
undesirable that constant-folding should change the semantics.

Report and patch by Andrew Gierth; assorted wordsmithing and revised
regression test cases by me.

Report: <20160708024746.1410.57282@wrigleys.postgresql.org>

In an example of TAP test scripts, there is the test checking whether
the result of the query is expected or not. But, in previous example,
the exit code of psql instead of the query result was checked unexpectedly.

Author: Ildar Musin

Author: Masahiko Sawada

These tests are currently only running in buildfarm member hamster,
which is purposefully very slow.  This suite has failed a couple of
times recently because of timeouts, so increase the allowed number of
iterations to avoid spurious failures.

Author: Michaël Paquier

Use our typical approach, from src/backend/parser.  Back-patch to 9.1
(all supported versions).

The aggfilter expression should be removed from the parent (combining)
Aggref, since it's not supposed to apply the filter, and indeed cannot
because any Vars used in the filter would not be available after the
lower-level aggregation step.  Per report from Jeff Janes.

(This has been broken since the introduction of partial aggregation,
I think.  The error became obvious after commit 59a3795c, when setrefs.c
began processing the parent Aggref's fields normally and thus would detect
such Vars.  The special-case coding previously used in setrefs.c skipped
over the parent's aggfilter field without processing it.  That was broken
in its own way because no other setrefs.c processing got applied either;
though since the executor would not execute the filter expression, only
initialize it, that oversight might not have had any visible symptoms at
present.)

Report: <CAMkU=1xfuPf2edAe4ZGXTmJpU7jxuKukKyvNtEXwu35B7dvejg@mail.gmail.com>

Welsh (cy_GB) apparently sorts 'dd' after 'f', creating problems
analogous to the odd sorting of 'aa' in Danish.  Adjust regression
test case to not use data that provokes that.

Jeff Janes

Patch: <CAMkU=1zx-pqcfSApL2pYDQurPOCfcYU0wJorsmY1OrYPiXRbLw@mail.gmail.com>

These functions were added in commits fbe5a3fb and a104a017,
but commit 45639a05 removed their only callers.  Put the related
code in foreign.c back to the way it was in 9.5, to avoid pointless
cross-version diffs.

Etsuro Fujita

Patch: <d674a3f1-6b63-519c-ef3f-f3188ed6a178@lab.ntt.co.jp>

In btree_gin and citext, avoid some not-particularly-interesting
dependencies on the sorting of 'aa'.  In tsearch2, use COLLATE "C" to
remove an uninteresting dependency on locale sort order (and thereby
allow removal of a variant expected-file).

Also, in citext, avoid assuming that lower('I') = 'i'.  This isn't relevant
to Danish but it does fail in Turkish.

Another peculiarity of Danish locale is that it has an unusual idea
of how to sort upper vs. lower case.  One of the pltcl test cases has
an issue with that.  Now that COLLATE works in all supported branches,
we can just change the test to be locale-independent, and get rid of
the variant expected file that used to support non-C locales.

Some tests added in 9.5 depended on 'aa' sorting before 'bb', which
doesn't hold true in Danish.  Use slightly different test data to
avoid the problem.

Jeff Janes

Report: <CAMkU=1w-cEDbA+XHdNb=YS_4wvZbs66Ni9KeSJKAJGNJyOsgQw@mail.gmail.com>

Michael Paquier

Alexander Law

runtime.sgml used to contain a table of estimated shared memory consumption
rates for max_connections and some other GUCs.  Commit 390bfc64 removed
that on the well-founded grounds that (a) we weren't maintaining the
entries well and (b) it no longer mattered so much once we got out from
under SysV shmem limits.  But it missed that there were even-more-obsolete
versions of some of those numbers in comments in postgresql.conf.sample.
Remove those too.  Back-patch to 9.3 where the aforesaid commit went in.

Omission noted by Andres Freund.

9.4 added a second description of GET DIAGNOSTICS that was totally
independent of the existing one, resulting in each description lying to the
extent that it claimed the set of status items it described was complete.
Fix that, and do some minor markup improvement.

Also some other small fixes per bug #14258 from Dilian Palauzov.

Discussion: <20160718181437.1414.40802@wrigleys.postgresql.org>

brin-extensibility-inclusion-table was confused in places about the
difference between strategy 4 (RTOverRight) and strategy 5 (RTRight).

Alexander Law