postgres-xl.git
8 years agoAdd pgtcl back to the list of externally-maintained client interfaces.
Tom Lane [Wed, 2 Aug 2017 20:55:03 +0000 (16:55 -0400)]
Add pgtcl back to the list of externally-maintained client interfaces.

FlightAware is still maintaining this, and indeed is seemingly being
more active with it than the pgtclng fork is.  List both, for the
time being anyway.

In the back branches, also back-port commit e20f679f6 and other
recent updates to the client-interfaces list.  I think these are
probably of current interest to users of back branches.  I did
not touch the list of externally maintained PLs in the back
branches, though.  Those are much more likely to be server version
sensitive, and I don't know which of these PLs work all the way back.

Discussion: https://postgr.es/m/20170730162612.1449.58796@wrigleys.postgresql.org

8 years agoRemove broken and useless entry-count printing in HASH_DEBUG code.
Tom Lane [Wed, 2 Aug 2017 16:16:50 +0000 (12:16 -0400)]
Remove broken and useless entry-count printing in HASH_DEBUG code.

init_htab(), with #define HASH_DEBUG, prints a bunch of hashtable
parameters.  It used to also print nentries, but commit 44ca4022f changed
that to "hash_get_num_entries(hctl)", which is wrong (the parameter should
be "hashp").

Rather than correct the coding, though, let's just remove that field from
the printout.  The table must be empty, since we just finished building
it, so expensively calculating the number of entries is rather pointless.
Moreover hash_get_num_entries makes assumptions (about not needing locks)
which we could do without in debugging code.

Noted by Choi Doo-Won in bug #14764.  Back-patch to 9.6 where the
faulty code was introduced.

Discussion: https://postgr.es/m/20170802032353.8424.12274@wrigleys.postgresql.org

8 years agoGet a snapshot before COPY in table sync
Peter Eisentraut [Wed, 2 Aug 2017 14:59:01 +0000 (10:59 -0400)]
Get a snapshot before COPY in table sync

This fixes a crash if the local table has a function index and the
function makes non-immutable calls.

Reported-by: Scott Milliken <scott@deltaex.com>
Author: Masahiko Sawada <sawada.mshk@gmail.com>

8 years agoRemove duplicate setting of SSL_OP_SINGLE_DH_USE option.
Tom Lane [Wed, 2 Aug 2017 15:28:46 +0000 (11:28 -0400)]
Remove duplicate setting of SSL_OP_SINGLE_DH_USE option.

Commit c0a15e07c moved the setting of OpenSSL's SSL_OP_SINGLE_DH_USE option
into a new subroutine initialize_dh(), but forgot to remove it from where
it was.  SSL_CTX_set_options() is a trivial function, amounting indeed to
just "ctx->options |= op", hence there's no reason to contort the code or
break separation of concerns to avoid calling it twice.  So separating the
DH setup from disabling of old protocol versions is a good change, but we
need to finish the job.

Noted while poking into the question of SSL session tickets.

8 years agoFix OBJECT_TYPE/OBJECT_DOMAIN confusion
Peter Eisentraut [Wed, 2 Aug 2017 14:40:32 +0000 (10:40 -0400)]
Fix OBJECT_TYPE/OBJECT_DOMAIN confusion

This doesn't have a significant impact except that now SECURITY LABEL ON
DOMAIN rejects types that are not domains.

Reported-by: 高增琦 <pgf00a@gmail.com>
8 years agoMake temporary tables use shared storage on datanodes
Pavan Deolasee [Wed, 2 Aug 2017 06:52:14 +0000 (12:22 +0530)]
Make temporary tables use shared storage on datanodes

Since a temporary table may be accessed by multiple backends on a datanode, XL
mostly treats such tables as regular tables. But the technique that was used to
distingush between temporary tables that may need shared storage vs those which
are accessed only by a single backend, wasn't very full proof. We were relying
on global session activation to make that distinction. This clearly fails when
a background process, such as autovacuuum process, tries to figure out whether
a table is using local or shared storage. This was leading to various problems,
such as, when the underlying file system objects for the table were getting
cleaned up, but without first discarding all references to the table from the
shared buffers.

We now make all temp tables to use shared storage on the datanodes and thus
simplify things. Only EXECUTE DIRECT anyways does not set up global session, so
I don't think this will have any meaningful impact on the performance.

This should fix the checkpoint failures during regression tests.

8 years agoRevert test case added by commit 1e165d05fe06a9072867607886f818bc255507db.
Tom Lane [Wed, 2 Aug 2017 00:15:10 +0000 (20:15 -0400)]
Revert test case added by commit 1e165d05fe06a9072867607886f818bc255507db.

The buildfarm is still showing at least three distinct behaviors for
a bad locale name in CREATE COLLATION.  Although this test was helpful
for getting the error reporting code into some usable shape, it doesn't
seem worth carrying multiple expected-files in order to support the
test in perpetuity.  So pull it back out.

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

8 years agoSecond try at getting useful errors out of newlocale/_create_locale.
Tom Lane [Tue, 1 Aug 2017 21:17:20 +0000 (17:17 -0400)]
Second try at getting useful errors out of newlocale/_create_locale.

The early buildfarm returns for commit 1e165d05f 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

8 years agoSuppress less info in regression tests using DROP CASCADE.
Tom Lane [Tue, 1 Aug 2017 20:49:23 +0000 (16:49 -0400)]
Suppress less info in regression tests using DROP CASCADE.

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

8 years agoTry to deliver a sane message for _create_locale() failure on Windows.
Tom Lane [Tue, 1 Aug 2017 20:11:51 +0000 (16:11 -0400)]
Try to deliver a sane message for _create_locale() failure on Windows.

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

8 years agodoc: Fix typo
Peter Eisentraut [Tue, 1 Aug 2017 18:33:55 +0000 (14:33 -0400)]
doc: Fix typo

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

8 years agoAllow creation of C/POSIX collations without depending on libc behavior.
Tom Lane [Tue, 1 Aug 2017 17:51:05 +0000 (13:51 -0400)]
Allow creation of C/POSIX collations without depending on libc behavior.

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

8 years agoFurther improve consistency of configure's program searching.
Tom Lane [Tue, 1 Aug 2017 15:40:00 +0000 (11:40 -0400)]
Further improve consistency of configure's program searching.

Peter Eisentraut noted that commit 40b9f1921 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

8 years agoComment fix for partition_rbound_cmp().
Dean Rasheed [Tue, 1 Aug 2017 08:40:45 +0000 (09:40 +0100)]
Comment fix for partition_rbound_cmp().

This was an oversight in d363d42.

Beena Emerson

8 years agoFix comment.
Tatsuo Ishii [Mon, 31 Jul 2017 23:00:11 +0000 (08:00 +0900)]
Fix comment.

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

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

8 years agoFix typo
Peter Eisentraut [Mon, 31 Jul 2017 21:22:47 +0000 (17:22 -0400)]
Fix typo

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

8 years agoFix typo
Peter Eisentraut [Mon, 31 Jul 2017 21:08:14 +0000 (17:08 -0400)]
Fix typo

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

8 years agoDoc: add v10 release notes entries for the DH parameter changes.
Heikki Linnakangas [Mon, 31 Jul 2017 19:47:07 +0000 (22:47 +0300)]
Doc: add v10 release notes entries for the DH parameter changes.

8 years agoAlways use 2048 bit DH parameters for OpenSSL ephemeral DH ciphers.
Heikki Linnakangas [Mon, 31 Jul 2017 19:36:09 +0000 (22:36 +0300)]
Always use 2048 bit DH parameters for OpenSSL ephemeral DH ciphers.

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

8 years agoDoc: specify that the minimum supported version of Perl is 5.8.3.
Tom Lane [Mon, 31 Jul 2017 17:42:48 +0000 (13:42 -0400)]
Doc: specify that the minimum supported version of Perl is 5.8.3.

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

8 years agoRecord full paths of programs sought by "configure".
Tom Lane [Mon, 31 Jul 2017 17:02:49 +0000 (13:02 -0400)]
Record full paths of programs sought by "configure".

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

8 years agoTighten coding for non-composite case in plperl's return_next.
Tom Lane [Mon, 31 Jul 2017 15:33:46 +0000 (11:33 -0400)]
Tighten coding for non-composite case in plperl's return_next.

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.

8 years agoFix function comment for dumpACL()
Stephen Frost [Mon, 31 Jul 2017 14:37:08 +0000 (10:37 -0400)]
Fix function comment for dumpACL()

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.

8 years agoDon't run ALTER ENUM in an autocommit block on remote nodes
Pavan Deolasee [Mon, 31 Jul 2017 14:21:58 +0000 (19:51 +0530)]
Don't run ALTER ENUM in an autocommit block on remote nodes

Before PG 10, Postgres did not allow ALTER ENUM to be run inside a transaction
block. So we used to run these commands in auto-commit mode on the remote
nodes. But now Postgres has removed the restriction. So we also run the
statements in transaction block.

This fixes regression failures in the 'enum' test case.

8 years agoCopy distribution information correctly to ProjectSet path
Pavan Deolasee [Mon, 31 Jul 2017 12:06:38 +0000 (17:36 +0530)]
Copy distribution information correctly to ProjectSet path

ProjectSet is a new path type in PG 10 and we'd missed to copy the distribution
information correctly to the path. This was resulting in failures in many
regression test cases. Lack of distribution information, prevented the
distributed query planner from adding a Remote Subplan node on top of the plan,
thus resulting in local execution of the plan. Since the underlying table is
actually a distributed table, local execution fails to fetch any data.

Fix this by properly copying distribution info. Several regression failures are
fixed automatically with this patch.

8 years agoAdd missing comment in postgresql.conf.
Tatsuo Ishii [Mon, 31 Jul 2017 02:24:51 +0000 (11:24 +0900)]
Add missing comment in postgresql.conf.

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

Back patched to 9.2 and beyond.

8 years agoAdd missing comment in postgresql.conf.
Tatsuo Ishii [Mon, 31 Jul 2017 02:06:37 +0000 (11:06 +0900)]
Add missing comment in postgresql.conf.

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.

8 years agoAdd missing comment in postgresql.conf.
Tatsuo Ishii [Mon, 31 Jul 2017 01:46:32 +0000 (10:46 +0900)]
Add missing comment in postgresql.conf.

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

8 years agoPartially accept plan changes in updatable_views
Tomas Vondra [Sun, 30 Jul 2017 21:52:54 +0000 (23:52 +0200)]
Partially accept plan changes in updatable_views

Upstream commit 215b43cdc8d6b4a1700886a39df1ee735cb0274d significantly
reworked planning of leaky functions. In practice that change means we
no longer have to push leaky functions into a subquery. Which greatly
simplifies some plans, including the two in this patch.

This commit accepts the plans only partially, though. It uses the plans
from upstream, and adds a Remote Subquery Scan node at the top, so we
accept the general plan shape change.

But there are a few additional differences that need futher evaluation,
particularly in target lists (Postgres-XL generating more entries than
upstream) and SubPlans (Postgres-XL only generating one subplan, while
upstream generates two alternative ones).

8 years agoAccept aggregation plan changes in xc_remote tests
Tomas Vondra [Sun, 30 Jul 2017 18:41:44 +0000 (20:41 +0200)]
Accept aggregation plan changes in xc_remote tests

The plans changed mainly due to abandoning the custom implementation
two-phase aggregation code, and using the upstream parallel aggregation.

That means we have stopped showing schema name in target lists, so
instead of

    Output: pg_catalog.avg((avg(xcrem_employee.salary)))

the EXPLAIN now shows

    Output: avg(xcrem_employee.salary)

and we also do projection at the scan nodes, so the target list only
shows the necessary subset of columns.

A somewhat surprising change is that the plans switch from distributed
aggregate plans like this one

    ->  Aggregate
        ->  Remote Subquery Scan
            ->  Aggregate
                -> Seq Scan

to always performing simple (non-distributed) aggregate like this

    ->  Aggregate
        ->  Remote Subquery Scan
            -> Seq Scan

This happens due to create_grouping_paths() relying on consider_parallel
flag when setting try_distributed_aggregate, disabling distributed
aggregation when consider_parallel=false. Both affected plans are however
for UPDATE queries, and PostgreSQL disables parallelism for queries that
do writes, so we end up with try_distributed_aggregate=false.

We should probably enable distributed aggregates in these cases, but we
can't ignore consider_parallel entirely, as we likely need some of the
checks. We will probably end up with consider_distributed flag, set in
a similar way to consider_parallel, but that's more an enhancement than
a bug fix.

8 years agoReject SQL functions containing utility statements
Tomas Vondra [Sun, 30 Jul 2017 16:17:27 +0000 (18:17 +0200)]
Reject SQL functions containing utility statements

The check was not effective for the same reason as 5a54abb7acd, that is
not accounting for XL wrapping the original command into RawStmt. Fix
that by checking parsetree->stmt, and also add an assert checking we
actually got a RawStmt in the first place.

8 years agoProduce proper error message for COPY (SELECT INTO)
Tomas Vondra [Sun, 30 Jul 2017 15:27:36 +0000 (17:27 +0200)]
Produce proper error message for COPY (SELECT INTO)

Produce the right error message for COPY (SELECT INTO) queries, that is

    ERROR: COPY (SELECT INTO) is not supported

instead of the incorrect

    ERROR: COPY query must have a RETURNING clause

The root cause is that the check in BeginCopy() was testing raw_query,
but XL wraps the original command in RawStmt, so we should be checking
raw_query->stmt instead.

8 years agoAdd explicit VACUUM to inet test to actually do IOS
Tomas Vondra [Sun, 30 Jul 2017 13:25:29 +0000 (15:25 +0200)]
Add explicit VACUUM to inet test to actually do IOS

Some of the queries in inet test are meant to exercise Index Only Scans.
Postgres-XL was not however picking those plans due to stale stats on
the coordinator (reltuples and relpages in pg_class).

On plain PostgreSQL the tests work fine, as CREATE INDEX also updates
statistics stored in the pg_class catalog. For example this

    CREATE TABLE t (a INT);

    INSERT INTO t SELECT i FROM generate_series(1,1000) s(i);

    SELECT relpages, reltuples FROM pg_class
     WHERE relname = 't';

    CREATE INDEX ON t(a);

    SELECT relpages, reltuples FROM pg_class
     WHERE relname = 't';

will show zeroes before the CREATE INDEX command, and accurate values
after it completes.

On Postgres-XL that is not the case, and we will return zeroes even after
the CREATE INDEX command. To actually update the statistics we need to
fetch information from the datanodes the way VACUUM does it.

Fixed by adding an explicit VACUUM call right after the CREATE INDEX, to
fetch the stats from the datanodes and update the coordinator catalogs.

8 years agoTweak the query plan check in join regression test
Tomas Vondra [Sat, 29 Jul 2017 22:29:24 +0000 (00:29 +0200)]
Tweak the query plan check in join regression test

The test expects the plan to use Index Scan, but with 1000 rows the
differences are very small. With two data nodes, we however compute
the estimates as if the tables had 500 rows, making the cost difference
even smaller.

Fixed by increasing the total number of rows to 2000, which means each
datanode has about 1000 and uses the same cost estimates as upstream.

8 years agoRemove extra snprintf call in pg_tablespace_databases
Tomas Vondra [Sat, 29 Jul 2017 16:54:59 +0000 (18:54 +0200)]
Remove extra snprintf call in pg_tablespace_databases

The XL code did two function calls in the else branch, about like this:

    else
        /* Postgres-XC tablespaces also include node name in path */
        sprintf(fctx->location, "pg_tblspc/%u/%s_%s", tablespaceOid,
                TABLESPACE_VERSION_DIRECTORY, PGXCNodeName);
        fctx->location = psprintf("pg_tblspc/%u/%s_%s", tablespaceOid,
                                  TABLESPACE_VERSION_DIRECTORY,
                                  PGXCNodeName);

which is wrong, as only the first call is actually the else branch, the
second call is executed unconditionally.

In fact, the two calls attempt to construct the same location string,
but the sprintf call assumes the 'fctx->location' string is already
allocated. But it actually is not, so it's likely to cause a segfault.

Fixed by removing the sprintf() call, keeping just the psprintf() one.

Noticed thanks to GCC 6.3 complaining about incorrect indentation.

Backpatch to XL 9.5.

8 years agoFix confusing indentation in gtm_client.c
Tomas Vondra [Sat, 29 Jul 2017 16:51:55 +0000 (18:51 +0200)]
Fix confusing indentation in gtm_client.c

GCC 6.3 complains that the indentation in gtm_sync_standby() is somewhat
confusing, as it might mislead people to think that a command is part of
an if branch. So fix that by removing the unnecessary indentation.

8 years agoRefactor the construction of distributed grouping paths
Tomas Vondra [Fri, 28 Jul 2017 22:34:53 +0000 (00:34 +0200)]
Refactor the construction of distributed grouping paths

The code generating distributed grouping paths was originally structured
like this:

    if (try_distributed_aggregation)
    { ... }

    if (can_sort && try_distributed_aggregation)
    { ... }

    if (can_hash && try_distributed_aggregation)
    { ... }

It's refactored like this, to resemble the upstream part of the code:

    if (try_distributed_aggregation)
    {
        ...

        if (can_sort)
        { ... }

        if (can_hash)
        { ... }
    }

8 years agoAccept plan change in xc_groupby regression test
Tomas Vondra [Fri, 28 Jul 2017 22:00:09 +0000 (00:00 +0200)]
Accept plan change in xc_groupby regression test

The plan changed in two ways. Firstly, the targetlists changed due to
abandoning the custom distributed aggregation and reusing the upstream
partial aggregation code. That means we're not prefixing the aggregate
with schema name, etc.

The plan also switches from distributed aggregation to plain aggregation
with all the work done on top of a remote query. This happens simply due
to costing, as the tables are tiny and two-phase aggregation has some
overhead. The original implementation (as in XL 9.5) distributed the
aggregate unconditionally, ignoring the costing.

Parf of the problem is that the query groups by two columns from two
different tables, resulting in overestimation of the number of groups.
That means the optimizer thinks distributing the aggregation would not
reduce the number of rows, which increases the cost estimate as each
row requires network transfer and the finalize aggregate also depends
on the number of input rows.

We could make the tables larger and the optimizer would eventually
switch to distributed aggregate. For example this seems to do the
trick:

    insert into xc_groupby_tab1 select 1, mod(i,1000)
      from generate_series(1,20000) s(i);

    insert into xc_groupby_tab2 select 1, mod(i,1000)
      from generate_series(1,20000) s(i);

But it does not seem worth it, considering it's just a workaround
for the estimation issue and the increased duration. And we already
have other regression tests testing plausible queries benefiting from
distributed aggregation. So just accept the plan change.

8 years agoMove ExecProcNode from dispatch to function pointer based model.
Andres Freund [Mon, 17 Jul 2017 07:33:49 +0000 (00:33 -0700)]
Move ExecProcNode from dispatch to function pointer based model.

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 b8d7f053c5c2bf2a7e - 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

8 years agoMove interrupt checking from ExecProcNode() to executor nodes.
Andres Freund [Wed, 26 Jul 2017 00:37:17 +0000 (17:37 -0700)]
Move interrupt checking from ExecProcNode() to executor nodes.

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

8 years agoInclude publication owner's name in the output of \dRp+.
Tom Lane [Fri, 28 Jul 2017 21:44:48 +0000 (17:44 -0400)]
Include publication owner's name in the output of \dRp+.

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

8 years agoPL/Perl portability fix: absorb relevant -D switches from Perl.
Tom Lane [Fri, 28 Jul 2017 18:25:28 +0000 (14:25 -0400)]
PL/Perl portability fix: absorb relevant -D switches from Perl.

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

8 years agoPL/Perl portability fix: avoid including XSUB.h in plperl.c.
Tom Lane [Fri, 28 Jul 2017 16:25:43 +0000 (12:25 -0400)]
PL/Perl portability fix: avoid including XSUB.h in plperl.c.

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

8 years agoFix psql tab completion for CREATE USER MAPPING.
Tom Lane [Thu, 27 Jul 2017 18:13:15 +0000 (14:13 -0400)]
Fix psql tab completion for CREATE USER MAPPING.

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

8 years agoStandardize describe.c's behavior for no-matching-objects a bit more.
Tom Lane [Thu, 27 Jul 2017 17:30:59 +0000 (13:30 -0400)]
Standardize describe.c's behavior for no-matching-objects a bit more.

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

8 years agoAvoid use of sprintf/snprintf in describe.c.
Tom Lane [Thu, 27 Jul 2017 16:12:37 +0000 (12:12 -0400)]
Avoid use of sprintf/snprintf in describe.c.

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

8 years agoSync listDbRoleSettings() with the rest of the world.
Tom Lane [Thu, 27 Jul 2017 15:57:29 +0000 (11:57 -0400)]
Sync listDbRoleSettings() with the rest of the world.

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

8 years agoFix very minor memory leaks in psql's command.c.
Tom Lane [Thu, 27 Jul 2017 15:10:38 +0000 (11:10 -0400)]
Fix very minor memory leaks in psql's command.c.

\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

8 years agoWork around Msys weakness in Testlib.pm's command_like()
Andrew Dunstan [Wed, 26 Jul 2017 21:15:59 +0000 (17:15 -0400)]
Work around Msys weakness in Testlib.pm's command_like()

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 f13ea95f9e. 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.

8 years agoClean up SQL emitted by psql/describe.c.
Tom Lane [Wed, 26 Jul 2017 23:35:35 +0000 (19:35 -0400)]
Clean up SQL emitted by psql/describe.c.

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.

8 years agoUpdate copyright in recently added files
Alvaro Herrera [Wed, 26 Jul 2017 22:17:18 +0000 (18:17 -0400)]
Update copyright in recently added files

8 years agoFix concurrent locking of tuple update chain
Alvaro Herrera [Wed, 26 Jul 2017 21:24:16 +0000 (17:24 -0400)]
Fix concurrent locking of tuple update chain

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

8 years agoRemove obsolete comments about functional dependencies
Alvaro Herrera [Wed, 26 Jul 2017 15:40:39 +0000 (11:40 -0400)]
Remove obsolete comments about functional dependencies

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

8 years agoAccept some of the differences in 'tidscan' test case
Pavan Deolasee [Wed, 26 Jul 2017 07:26:00 +0000 (12:56 +0530)]
Accept some of the differences in 'tidscan' test case

Most of these differences arise from the the fact that rows are fetched from
multiple datanodes in XL and hence TIDs can be duplicated. There are some other
differences because of EXPLAIN output.

The test case does not yet pass because of other unaddressed failures.

8 years agoFix remaining problems in 'triggers' test case.
Pavan Deolasee [Wed, 26 Jul 2017 06:17:28 +0000 (11:47 +0530)]
Fix remaining problems in 'triggers' test case.

We don't support different column ordering for partitions in XL. So enforce
that either by ensuring consistent column ordering or accepting errors. Also we
don't support triggers in XL, so some changes were necessary to take that into
account.

8 years agoFix 'union' test case.
Pavan Deolasee [Wed, 26 Jul 2017 05:13:24 +0000 (10:43 +0530)]
Fix 'union' test case.

Since 93cbab90b0c6fc3fc4aa515b93057127c0ee8a1b we expect that the child table
has columns at the same position as the parent table. So fix the test case to
follow the rule and resolve expected output differences arising from that.

The test case should pass with this change.

8 years agoDon't try to fetch table details using the old name after ExecRenameStmt
Pavan Deolasee [Wed, 26 Jul 2017 04:48:51 +0000 (10:18 +0530)]
Don't try to fetch table details using the old name after ExecRenameStmt

This used to work before PG 10, but some changes must have caused
non-deterministic behaviour. It anyways seems unsafe to lookup the catalogs
using the old name once ExecRenameStmt has finished. The lookup may or may not
see the old tuple, depending on whether CommandCounterIncrement has happened in
between. We now fetch the requried details before calling ExecRenameStmt and
use that info for subsequent processing.

This fixes some wierd issues in 'alter_table' test case where we were failing
to send ALTER TABLE RENAME TO command to remote nodes and causing inconsistent
catalog entries between the coordinator and the remote nodes.

8 years agoMake PostgresNode easily subclassable
Alvaro Herrera [Tue, 25 Jul 2017 22:39:44 +0000 (18:39 -0400)]
Make PostgresNode easily subclassable

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

8 years agoFix race conditions in replication slot operations
Alvaro Herrera [Tue, 25 Jul 2017 17:26:49 +0000 (13:26 -0400)]
Fix race conditions in replication slot operations

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

8 years agoFix partitioning crashes during error reporting.
Robert Haas [Mon, 24 Jul 2017 22:08:08 +0000 (18:08 -0400)]
Fix partitioning crashes during error reporting.

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

8 years agoFix race condition in predicate-lock init code in EXEC_BACKEND builds.
Tom Lane [Mon, 24 Jul 2017 20:45:46 +0000 (16:45 -0400)]
Fix race condition in predicate-lock init code in EXEC_BACKEND builds.

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

8 years agoWhen WCOs are present, disable direct foreign table modification.
Robert Haas [Mon, 24 Jul 2017 19:57:24 +0000 (15:57 -0400)]
When WCOs are present, disable direct foreign table modification.

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

8 years agoEnsure that pg_get_ruledef()'s output matches pg_get_viewdef()'s.
Tom Lane [Mon, 24 Jul 2017 19:16:31 +0000 (15:16 -0400)]
Ensure that pg_get_ruledef()'s output matches pg_get_viewdef()'s.

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

8 years agoBe more consistent about errors for opfamily member lookup failures.
Tom Lane [Mon, 24 Jul 2017 15:23:27 +0000 (11:23 -0400)]
Be more consistent about errors for opfamily member lookup failures.

Add error checks in some places that were calling get_opfamily_member
or get_opfamily_proc and just assuming that the call could never fail.
Also, standardize the wording for such errors in some other places.

None of these errors are expected in normal use, hence they're just
elog not ereport.  But they may be handy for diagnosing omissions in
custom opclasses.

Rushabh Lathia found the oversight in RelationBuildPartitionKey();
I found the others by grepping for all callers of these functions.

Discussion: https://postgr.es/m/CAGPqQf2R9Nk8htpv0FFi+FP776EwMyGuORpc9zYkZKC8sFQE3g@mail.gmail.com

8 years agoMSVC: Finish clean.bat build artifact coverage.
Noah Misch [Mon, 24 Jul 2017 07:13:23 +0000 (00:13 -0700)]
MSVC: Finish clean.bat build artifact coverage.

With this, "git clean -dnx" is clear after a "clean dist" following a
build.  Preserve sql_help.h in non-dist cleans, like the Makefile does.

8 years agoMSVC: Accept tcl86.lib in addition to tcl86t.lib.
Noah Misch [Mon, 24 Jul 2017 06:53:27 +0000 (23:53 -0700)]
MSVC: Accept tcl86.lib in addition to tcl86t.lib.

ActiveTcl8.6.4.1.299124-win32-x86_64-threaded.exe ships just tcl86.lib.
Back-patch to 9.2, like the commit recognizing tcl86t.lib.

8 years agoAccept some obvious regression differences in the 'join' test case
Pavan Deolasee [Mon, 24 Jul 2017 05:42:57 +0000 (11:12 +0530)]
Accept some obvious regression differences in the 'join' test case

These are only placements of Remote FQS or Remote Subplan nodes in the newly
added explain plans in the test case. There are some remaining failures in the
test case which will need more scrutiny.

8 years agoFix pg_dump's handling of event triggers.
Tom Lane [Sun, 23 Jul 2017 00:20:09 +0000 (20:20 -0400)]
Fix pg_dump's handling of event triggers.

pg_dump with the --clean option failed to emit DROP EVENT TRIGGER
commands for event triggers.  In a closely related oversight,
it also did not emit ALTER OWNER commands for event triggers.
Since only superusers can create event triggers, the latter oversight
is of little practical consequence ... but if we're going to record
an owner for event triggers, then surely pg_dump should preserve it.

Per complaint from Greg Atkins.  Back-patch to 9.3 where event triggers
were introduced.

Discussion: https://postgr.es/m/20170722191142.yi4e7tzcg3iacclg@gmail.com

8 years agoImprove comments about partitioned hash table freelists.
Tom Lane [Sat, 22 Jul 2017 22:02:26 +0000 (18:02 -0400)]
Improve comments about partitioned hash table freelists.

While I couldn't find any live bugs in commit 44ca4022f, the comments
seemed pretty far from adequate; in particular it was not made plain that
"borrowing" entries from other freelists is critical for correctness.
Try to improve the commentary.  A couple of very minor code style
tweaks, as well.

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

8 years agoUpdate expected results for collate.linux.utf8 regression test.
Tom Lane [Sat, 22 Jul 2017 16:15:19 +0000 (12:15 -0400)]
Update expected results for collate.linux.utf8 regression test.

I believe this changed as a consequence of commit 54baa4813: trying to
clone the "C" collation now produces a true clone with collencoding -1,
hence the error message if it's duplicate no longer specifies an encoding.

Per buildfarm member crake, which apparently hadn't been running this
test for the last few weeks.

8 years agoFix typo in comment
Alvaro Herrera [Mon, 17 Jul 2017 13:51:42 +0000 (09:51 -0400)]
Fix typo in comment

Commit fd31cd265138 renamed the variable to skipping_blocks, but forgot
to update this comment.

Noticed while inspecting code.

8 years agoDoc: update versioning information in libpq.sgml.
Tom Lane [Fri, 21 Jul 2017 21:50:57 +0000 (17:50 -0400)]
Doc: update versioning information in libpq.sgml.

The descriptions of PQserverVersion and PQlibVersion hadn't been updated
for the new two-part version-numbering approach.  Fix that.

In passing, remove some trailing whitespace elsewhere in the file.

8 years agopg_rewind: Fix some problems when copying files >2GB.
Robert Haas [Fri, 21 Jul 2017 18:25:36 +0000 (14:25 -0400)]
pg_rewind: Fix some problems when copying files >2GB.

When incrementally updating a file larger than 2GB, the old code could
either fail outright (if the client asked the server for bytes beyond
the 2GB boundary) or fail to copy all the blocks that had actually
been modified (if the server reported a file size to the client in
excess of 2GB), resulting in data corruption.  Generally, such files
won't occur anyway, but they might if using a non-default segment size
or if there the directory contains stray files unrelated to
PostgreSQL.  Fix by a more prudent choice of data types.

Even with these improvements, this code still uses a mix of different
types (off_t, size_t, uint64, int64) to represent file sizes and
offsets, not all of which necessarily have the same width or
signedness, so further cleanup might be in order here.  However, at
least now they all have the potential to be 64 bits wide on 64-bit
platforms.

Kuntal Ghosh and Michael Paquier, with a tweak by me.

Discussion: http://postgr.es/m/CAGz5QC+8gbkz=Brp0TgoKNqHWTzonbPtPex80U0O6Uh_bevbaA@mail.gmail.com

8 years agoStabilize postgres_fdw regression tests.
Tom Lane [Fri, 21 Jul 2017 18:20:43 +0000 (14:20 -0400)]
Stabilize postgres_fdw regression tests.

The new test cases added in commit 8bf58c0d9 turn out to have output
that can vary depending on the lc_messages setting prevailing on the
test server.  Hide the remote end's error messages to ensure stable
output.  This isn't a terribly desirable solution; we'd rather know
that the connection failed for the expected reason and not some other
one.  But there seems little choice for the moment.

Per buildfarm.

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

8 years agopg_rewind: Fix busted sanity check.
Robert Haas [Fri, 21 Jul 2017 16:48:22 +0000 (12:48 -0400)]
pg_rewind: Fix busted sanity check.

As written, the code would only fail the sanity check if none of the
columns returned by the server were of the expected type, but we want
it to fail if even one column is not of the expected type.

Discussion: http://postgr.es/m/CA+TgmoYuY5zW7JEs+1hSS1D=V5K8h1SQuESrq=bMNeo0B71Sfw@mail.gmail.com

8 years agoRe-establish postgres_fdw connections after server or user mapping changes.
Tom Lane [Fri, 21 Jul 2017 16:51:38 +0000 (12:51 -0400)]
Re-establish postgres_fdw connections after server or user mapping changes.

Previously, postgres_fdw would keep on using an existing connection even
if the user did ALTER SERVER or ALTER USER MAPPING commands that should
affect connection parameters.  Teach it to watch for catcache invals
on these catalogs and re-establish connections when the relevant catalog
entries change.  Per bug #14738 from Michal Lis.

In passing, clean up some rather crufty decisions in commit ae9bfc5d6
about where fields of ConnCacheEntry should be reset.  We now reset
all the fields whenever we open a new connection.

Kyotaro Horiguchi, reviewed by Ashutosh Bapat and myself.
Back-patch to 9.3 where postgres_fdw appeared.

Discussion: https://postgr.es/m/20170710113917.7727.10247@wrigleys.postgresql.org

8 years agoFix double shared memory allocation.
Teodor Sigaev [Fri, 21 Jul 2017 10:31:20 +0000 (13:31 +0300)]
Fix double shared memory allocation.

SLRU buffer lwlocks are allocated twice by oversight in commit
fe702a7b3f9f2bc5bf6d173166d7d55226af82c8 where that locks were moved to
separate tranche. The bug doesn't have user-visible effects except small
overspending of shared memory.

Backpatch to 9.6 where it was introduced.

Alexander Korotkov with small editorization by me.

8 years agoMake the new partition regression tests locale-independent.
Dean Rasheed [Fri, 21 Jul 2017 09:18:01 +0000 (10:18 +0100)]
Make the new partition regression tests locale-independent.

The order of partitions listed by \d+ is in general locale-dependent.
Rename the partitions in the test added by d363d42bb9 to force them to
be listed in a consistent order.

8 years agoUse MINVALUE/MAXVALUE instead of UNBOUNDED for range partition bounds.
Dean Rasheed [Fri, 21 Jul 2017 08:20:47 +0000 (09:20 +0100)]
Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition bounds.

Previously, UNBOUNDED meant no lower bound when used in the FROM list,
and no upper bound when used in the TO list, which was OK for
single-column range partitioning, but problematic with multiple
columns. For example, an upper bound of (10.0, UNBOUNDED) would not be
collocated with a lower bound of (10.0, UNBOUNDED), thus making it
difficult or impossible to define contiguous multi-column range
partitions in some cases.

Fix this by using MINVALUE and MAXVALUE instead of UNBOUNDED to
represent a partition column that is unbounded below or above
respectively. This syntax removes any ambiguity, and ensures that if
one partition's lower bound equals another partition's upper bound,
then the partitions are contiguous.

Also drop the constraint prohibiting finite values after an unbounded
column, and just document the fact that any values after MINVALUE or
MAXVALUE are ignored. Previously it was necessary to repeat UNBOUNDED
multiple times, which was needlessly verbose.

Note: Forces a post-PG 10 beta2 initdb.

Report by Amul Sul, original patch by Amit Langote with some
additional hacking by me.

Discussion: https://postgr.es/m/CAAJ_b947mowpLdxL3jo3YLKngRjrq9+Ej4ymduQTfYR+8=YAYQ@mail.gmail.com

8 years agoIn v10 release notes, call out sequence changes as a compatibility item.
Tom Lane [Thu, 20 Jul 2017 19:28:42 +0000 (15:28 -0400)]
In v10 release notes, call out sequence changes as a compatibility item.

The previous description didn't make it clear that this change
potentially breaks applications, partly because the entry wasn't even
in the compatibility-hazard section.  Move and clarify.

Discussion: https://postgr.es/m/603f3f0a-f89d-ae8b-1da9-a92fac16086d@enterprisedb.com

8 years agoDoc: clarify description of degenerate NATURAL joins.
Tom Lane [Thu, 20 Jul 2017 16:41:26 +0000 (12:41 -0400)]
Doc: clarify description of degenerate NATURAL joins.

Claiming that NATURAL JOIN is equivalent to CROSS JOIN when there are
no common column names is only strictly correct if it's an inner join;
you can't say e.g. CROSS LEFT JOIN.  Better to explain it as meaning
JOIN ON TRUE, instead.  Per a suggestion from David Johnston.

Discussion: https://postgr.es/m/CAKFQuwb+mYszQhDS9f_dqRrk1=Pe-S6D=XMkAXcDf4ykKPmgKQ@mail.gmail.com

8 years agoFix dumping of outer joins with empty qual lists.
Tom Lane [Thu, 20 Jul 2017 15:29:36 +0000 (11:29 -0400)]
Fix dumping of outer joins with empty qual lists.

Normally, a JoinExpr would have empty "quals" only if it came from CROSS
JOIN syntax.  However, it's possible to get to this state by specifying
NATURAL JOIN between two tables with no common column names, and there
might be other ways too.  The code previously printed no ON clause if
"quals" was empty; that's right for CROSS JOIN but syntactically invalid
if it's some type of outer join.  Fix by printing ON TRUE in that case.

This got broken by commit 2ffa740be, which stopped using NATURAL JOIN
syntax in ruleutils output due to its brittleness in the face of
column renamings.  Back-patch to 9.3 where that commit appeared.

Per report from Tushar Ahuja.

Discussion: https://postgr.es/m/98b283cd-6dda-5d3f-f8ac-87db8c76a3da@enterprisedb.com

8 years agoAdd static assertions about pg_control fitting into one disk sector.
Tom Lane [Wed, 19 Jul 2017 20:16:57 +0000 (16:16 -0400)]
Add static assertions about pg_control fitting into one disk sector.

When pg_control was first designed, sizeof(ControlFileData) was small
enough that a comment seemed like plenty to document the assumption that
it'd fit into one disk sector.  Now it's nearly 300 bytes, raising the
possibility that somebody would carelessly add enough stuff to create
a problem.  Let's add a StaticAssertStmt() to ensure that the situation
doesn't pass unnoticed if it ever occurs.

While at it, rename PG_CONTROL_SIZE to PG_CONTROL_FILE_SIZE to make it
clearer what that symbol means, and convert the existing runtime
comparisons of sizeof(ControlFileData) vs. PG_CONTROL_FILE_SIZE to be
static asserts --- we didn't have that technology when this code was
first written.

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

8 years agoDoc: add missing note about permissions needed to change log_lock_waits.
Tom Lane [Wed, 19 Jul 2017 16:58:36 +0000 (12:58 -0400)]
Doc: add missing note about permissions needed to change log_lock_waits.

log_lock_waits is PGC_SUSET, but config.sgml lacked the standard
boilerplate sentence noting that.

Report: https://postgr.es/m/20170719100838.19352.16320@wrigleys.postgresql.org

8 years agoImprove make_tsvector() to handle empty input, and simplify its callers.
Tom Lane [Tue, 18 Jul 2017 17:13:47 +0000 (13:13 -0400)]
Improve make_tsvector() to handle empty input, and simplify its callers.

It seemed a bit silly that each caller of make_tsvector() was laboriously
special-casing the situation where no lexemes were found, when it would
be easy and much more bullet-proof to make make_tsvector() handle that.

8 years agoFix serious performance problems in json(b) to_tsvector().
Tom Lane [Tue, 18 Jul 2017 16:45:51 +0000 (12:45 -0400)]
Fix serious performance problems in json(b) to_tsvector().

In an off-list followup to bug #14745, Bob Jones complained that
to_tsvector() on a 2MB jsonb value took an unreasonable amount of
time and space --- enough to draw the wrath of the OOM killer on
his machine.  On my machine, his example proved to require upwards
of 18 seconds and 4GB, which seemed pretty bogus considering that
to_tsvector() on the same data treated as text took just a couple
hundred msec and 10 or so MB.

On investigation, the problem is that the implementation scans each
string element of the json(b) and converts it to tsvector separately,
then applies tsvector_concat() to join those separate tsvectors.
The unreasonable memory usage came from leaking every single one of
the transient tsvectors --- but even without that mistake, this is an
O(N^2) or worse algorithm, because tsvector_concat() has to repeatedly
process the words coming from earlier elements.

We can fix it by accumulating all the lexeme data and applying
make_tsvector() just once.  As a side benefit, that also makes the
desired adjustment of lexeme positions far cheaper, because we can
just tweak the running "pos" counter between JSON elements.

In passing, try to make the explanation of that tweak more intelligible.
(I didn't think that a barely-readable comment far removed from the
actual code was helpful.)  And do some minor other code beautification.

8 years agoDoc: fix thinko in v10 release notes.
Tom Lane [Tue, 18 Jul 2017 14:17:15 +0000 (10:17 -0400)]
Doc: fix thinko in v10 release notes.

s/log_destination/log_directory/, per Jov in bug #14749.

Report: https://postgr.es/m/20170718082444.9229.99690@wrigleys.postgresql.org

8 years agoReverse-convert row types in ExecWithCheckOptions.
Robert Haas [Tue, 18 Jul 2017 01:56:31 +0000 (21:56 -0400)]
Reverse-convert row types in ExecWithCheckOptions.

Just as we already do in ExecConstraints, and for the same reason:
to improve the quality of error messages.

Etsuro Fujita, reviewed by Amit Langote

Discussion: http://postgr.es/m/56e0baa8-e458-2bbb-7936-367f7d832e43@lab.ntt.co.jp

8 years agoUse a real RT index when setting up partition tuple routing.
Robert Haas [Tue, 18 Jul 2017 01:29:45 +0000 (21:29 -0400)]
Use a real RT index when setting up partition tuple routing.

Before, we always used a dummy value of 1, but that's not right when
the partitioned table being modified is inside of a WITH clause
rather than part of the main query.

Amit Langote, reported and reviewd by Etsuro Fujita, with a comment
change by me.

Discussion: http://postgr.es/m/ee12f648-8907-77b5-afc0-2980bcb0aa37@lab.ntt.co.jp

8 years agoAccept changes in foreign_data test due to unsupported FDW
Tomas Vondra [Mon, 17 Jul 2017 23:22:26 +0000 (01:22 +0200)]
Accept changes in foreign_data test due to unsupported FDW

Postgres-XL does not support Foreign Data Wrappers, so accept failures
and output differences in the foreign_data regression test as expected.

8 years agoAccept failures in triggers test du to unsupported features
Tomas Vondra [Mon, 17 Jul 2017 22:35:22 +0000 (00:35 +0200)]
Accept failures in triggers test du to unsupported features

Postgres-XL does not support triggers, naturally causing many failures
in the triggers regression test. So treat the error messages as expected
behavior and remove the NOTICEs produced by the triggers.

Another failure was caused by INSERT in a subquery, which is another
feature unsupported by Postgres-XL.

8 years agoAccept two query plans in the join regression test
Tomas Vondra [Mon, 17 Jul 2017 22:01:31 +0000 (00:01 +0200)]
Accept two query plans in the join regression test

Both plans keep the same join algorithm as on PostgreSQL, and the plans
match those produced by Postgres-XL 9.5.

8 years agoResolve failures related to pg_node_tree type input
Tomas Vondra [Mon, 17 Jul 2017 21:26:25 +0000 (23:26 +0200)]
Resolve failures related to pg_node_tree type input

The input function for pg_node_tree pseudotype simply returns an error,
essentially disabling input from text. Some of the regression tests do

    CREATE TABLE t AS SELECT * FROM pg_class;

or something like that, which works fine on PostgreSQL, but it fails on
Postgres-XL as we need to send data between nodes. Unfortunately the
pg_class now contains pg_node_tree column (relpartbound), causing falure
when reading the column on the remote node.

So instead use another catalogue, for example pg_attribute, which does
not contain any such column.

8 years agoDoc: explain dollar quoting in the intro part of the pl/pgsql chapter.
Tom Lane [Mon, 17 Jul 2017 20:43:03 +0000 (16:43 -0400)]
Doc: explain dollar quoting in the intro part of the pl/pgsql chapter.

We're throwing people into the guts of the syntax with not much context;
let's back up one step and point out that this goes inside a literal in
a CREATE FUNCTION command.  Per suggestion from Kurt Kartaltepe.

Discussion: https://postgr.es/m/CACawnnyWAmH+au8nfZhLiFfWKjXy4d0kY+eZWfcxPRnjVfaa_Q@mail.gmail.com

8 years agoDrop the foo table in rangefuncs regression tes
Tomas Vondra [Mon, 17 Jul 2017 20:32:18 +0000 (22:32 +0200)]
Drop the foo table in rangefuncs regression tes

The table was made non-temporary by commit e98209019b4d2012, but it was
not dropped, wich caused failures in the returning regression test. So
add the missing DROP TABLE.

8 years agoImprove legibility of numeric literal
Andrew Dunstan [Mon, 17 Jul 2017 19:35:19 +0000 (15:35 -0400)]
Improve legibility of numeric literal

8 years agoMerge large_object.sql test into largeobject.source.
Tom Lane [Mon, 17 Jul 2017 19:28:16 +0000 (15:28 -0400)]
Merge large_object.sql test into largeobject.source.

It seems pretty confusing to have tests named both largeobject and
large_object.  The latter is of very recent vintage (commit ff992c074),
so get rid of it in favor of merging into the former.

Also, enable the LO comment test that was added by commit 70ad7ed4e,
since the later commit added the then-missing pg_upgrade functionality.
The large_object.sql test case is almost completely redundant with that,
but not quite: it seems like creating a user-defined LO with an OID in
the system range might be an interesting case for pg_upgrade, so let's
keep it.

Like the earlier patch, back-patch to all supported branches.

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

8 years agoUse usleep instead of select for timeouts in PostgresNode.pm
Andrew Dunstan [Mon, 17 Jul 2017 19:22:37 +0000 (15:22 -0400)]
Use usleep instead of select for timeouts in PostgresNode.pm

select() for pure timeouts is not portable, and in particular doesn't
work on Windows.

Discussion: https://postgr.es/m/186943e0-3405-978d-b19d-9d3335427c86@2ndQuadrant.com

8 years agohash: Fix write-ahead logging bugs related to init forks.
Robert Haas [Mon, 17 Jul 2017 16:03:35 +0000 (12:03 -0400)]
hash: Fix write-ahead logging bugs related to init forks.

One, logging for CREATE INDEX was oblivious to the fact that when
an unlogged table is created, *only* operations on the init fork
should be logged.

Two, init fork buffers need to be flushed after they are written;
otherwise, a filesystem-level copy following recovery may do the
wrong thing.  (There may be a better fix for this issue than the
one used here, but this is transposed from the similar logic already
present in XLogReadBufferForRedoExtended, and a broader refactoring
after beta2 seems inadvisable.)

Amit Kapila, reviewed by Ashutosh Sharma, Kyotaro Horiguchi,
and Michael Paquier

Discussion: http://postgr.es/m/CAA4eK1JpcMsEtOL_J7WODumeEfyrPi7FPYHeVdS7fyyrCrgp4w@mail.gmail.com

8 years agoMSVC: Don't link libpgcommon into pgcrypto.
Noah Misch [Mon, 17 Jul 2017 06:13:58 +0000 (23:13 -0700)]
MSVC: Don't link libpgcommon into pgcrypto.

Doing so was useful in 273c458a2b3a0fb73968020ea5e9e35eb6928967 but
became obsolete when 818fd4a67d610991757b610755e3065fb99d80a5 caused
postgres.exe to provide the relevant symbols.  No other loadable module
links to libpgcommon directly.