Commit Graph

45699 Commits

Author SHA1 Message Date
Andres Freund
cc2905e963 Use slots more widely in tuple mapping code and make naming more consistent.
It's inefficient to use a single slot for mapping between tuple
descriptors for multiple tuples, as previously done when using
ConvertPartitionTupleSlot(), as that means the slot's tuple descriptors
change for every tuple.

Previously we also, via ConvertPartitionTupleSlot(), built new tuples
after the mapping even in cases where we, immediately afterwards,
access individual columns again.

Refactor the code so one slot, on demand, is used for each
partition. That avoids having to change the descriptor (and allows to
use the more efficient "fixed" tuple slots). Then use slot->slot
mapping, to avoid unnecessarily forming a tuple.

As the naming between the tuple and slot mapping functions wasn't
consistent, rename them to execute_attr_map_{tuple,slot}.  It's likely
that we'll also rename convert_tuples_by_* to denote that these
functions "only" build a map, but that's left for later.

Author: Amit Khandekar and Amit Langote, editorialized by me
Reviewed-By: Amit Langote, Amit Khandekar, Andres Freund
Discussion:
    https://postgr.es/m/CAJ3gD9fR0wRNeAE8VqffNTyONS_UfFPRpqxhnD9Q42vZB+Jvpg@mail.gmail.com
    https://postgr.es/m/e4f9d743-cd4b-efb0-7574-da21d86a7f36%40lab.ntt.co.jp
Backpatch: -
2018-10-02 11:14:26 -07:00
Tom Lane
625b38ea0e Set snprintf.c's maximum number of NL arguments to be 31.
Previously, we used the platform's NL_ARGMAX if any, otherwise 16.
The trouble with this is that the platform value is hugely variable,
ranging from the POSIX-minimum 9 to as much as 64K on recent FreeBSD.
Values of more than a dozen or two have no practical use and slow down
the initialization of the argtypes array.  Worse, they cause snprintf.c
to consume far more stack space than was the design intention, possibly
resulting in stack-overflow crashes.

Standardize on 31, which is comfortably more than we need (it looks like
no existing translatable message has more than about 10 parameters).
I chose that, not 32, to make the array sizes powers of 2, for some
possible small gain in speed of the memset.

The lack of reported crashes suggests that the set of platforms we
use snprintf.c on (in released branches) may have no overlap with
the set where NL_ARGMAX has unreasonably large values.  But that's
not entirely clear, so back-patch to all supported branches.

Per report from Mateusz Guzik (via Thomas Munro).

Discussion: https://postgr.es/m/CAEepm=3VF=PUp2f8gU8fgZB22yPE_KBS0+e1AHAtQ=09schTHg@mail.gmail.com
2018-10-02 12:41:28 -04:00
Tom Lane
3d0f68dd30 Fix corner-case failures in has_foo_privilege() family of functions.
The variants of these functions that take numeric inputs (OIDs or
column numbers) are supposed to return NULL rather than failing
on bad input; this rule reduces problems with snapshot skew when
queries apply the functions to all rows of a catalog.

has_column_privilege() had careless handling of the case where the
table OID didn't exist.  You might get something like this:
	select has_column_privilege(9999,'nosuchcol','select');
	ERROR:  column "nosuchcol" of relation "(null)" does not exist
or you might get a crash, depending on the platform's printf's response
to a null string pointer.

In addition, while applying the column-number variant to a dropped
column returned NULL as desired, applying the column-name variant
did not:
	select has_column_privilege('mytable','........pg.dropped.2........','select');
	ERROR:  column "........pg.dropped.2........" of relation "mytable" does not exist
It seems better to make this case return NULL as well.

Also, the OID-accepting variants of has_foreign_data_wrapper_privilege,
has_server_privilege, and has_tablespace_privilege didn't follow the
principle of returning NULL for nonexistent OIDs.  Superusers got TRUE,
everybody else got an error.

Per investigation of Jaime Casanova's report of a new crash in HEAD.
These behaviors have been like this for a long time, so back-patch to
all supported branches.

Patch by me; thanks to Stephen Frost for discussion and review

Discussion: https://postgr.es/m/CAJGNTeP=-6Gyqq5TN9OvYEydi7Fv1oGyYj650LGTnW44oAzYCg@mail.gmail.com
2018-10-02 11:54:12 -04:00
Michael Paquier
80810ca629 Fix documentation of pgrowlocks using "lock_type" instead of "modes"
The example used in the documentation is outdated as well.  This is an
oversight from 0ac5ad5, which bumped up pgrowlocks but forgot some bits
of the documentation.

Reported-by: Chris Wilson
Discussion: https://postgr.es/m/153838692816.2950.12001142346234155699@wrigleys.postgresql.org
Backpatch-through: 9.3
2018-10-02 16:34:41 +09:00
Amit Kapila
0fd6a8a7d0 Test passing expanded-value representations to workers.
Currently, we don't have an explicit test to pass expanded-value
representations to workers, so we don't know whether it works on all kind
of platforms.  We suspect that the current code won't work on
alignment-sensitive hardware.  This commit will test that aspect and can
lead to failure on some of the buildfarm machines which we will fix in the
later commit.

Author: Tom Lane and Amit Kapila
Discussion: https://postgr.es/m/11629.1536550032@sss.pgh.pa.us
2018-10-02 11:01:33 +05:30
Michael Paquier
e3a25ab9ea Refactor relation opening for VACUUM and ANALYZE
VACUUM and ANALYZE share similar logic when it comes to opening a
relation to work on in terms of how the relation is opened, in which
order locks are tried and how logs should be generated when something
does not work as expected.

This commit refactors things so as both use the same code path to handle
the way a relation is opened, so as the integration of new options
becomes easier.

Author: Michael Paquier
Reviewed-by: Nathan Bossart
Discussion: https://postgr.es/m/20180927075152.GT1659@paquier.xyz
2018-10-02 08:53:38 +09:00
Peter Eisentraut
cf3dfea45b Change PROCEDURE to FUNCTION in CREATE EVENT TRIGGER syntax
This was claimed to have been done in
0a63f996e0, but that actually only
changed the documentation and not the grammar.  (That commit did fully
change it for CREATE TRIGGER.)
2018-10-01 23:02:55 +02:00
Tom Lane
b04aeb0a05 Add assertions that we hold some relevant lock during relation open.
Opening a relation with no lock at all is unsafe; there's no guarantee
that we'll see a consistent state of the relevant catalog entries.
While use of MVCC scans to read the catalogs partially addresses that
complaint, it's still possible to switch to a new catalog snapshot
partway through loading the relcache entry.  Moreover, whether or not
you trust the reasoning behind sometimes using less than
AccessExclusiveLock for ALTER TABLE, that reasoning is certainly not
valid if concurrent users of the table don't hold a lock corresponding
to the operation they want to perform.

Hence, add some assertion-build-only checks that require any caller
of relation_open(x, NoLock) to hold at least AccessShareLock.  This
isn't a full solution, since we can't verify that the lock level is
semantically appropriate for the action --- but it's definitely of
some use, because it's already caught two bugs.

We can also assert that callers of addRangeTableEntryForRelation()
hold at least the lock level specified for the new RTE.

Amit Langote and Tom Lane

Discussion: https://postgr.es/m/16565.1538327894@sss.pgh.pa.us
2018-10-01 12:43:21 -04:00
Tom Lane
b66827ca7c Fix tuple_data_split() to not open a relation without any lock.
contrib/pageinspect's tuple_data_split() function thought it could get
away with opening the referenced relation with NoLock.  In practice
there's no guarantee that the current session holds any lock on that
rel (even if we just read a page from it), so that this is unsafe.

Switch to using AccessShareLock.  Also, postpone closing the relation,
so that we needn't copy its tupdesc.  Also, fix unsafe use of
att_isnull() for attributes past the end of the tuple.

Per testing with a patch that complains if we open a relation without
holding any lock on it.  I don't plan to back-patch that patch, but we
should close the holes it identifies in all supported branches.

Discussion: https://postgr.es/m/2038.1538335244@sss.pgh.pa.us
2018-10-01 11:51:07 -04:00
Tom Lane
e27453bd83 Fix ALTER COLUMN TYPE to not open a relation without any lock.
If the column being modified is referenced by a foreign key constraint
of another table, ALTER TABLE would open the other table (to re-parse
the constraint's definition) without having first obtained a lock on it.
This was evidently intentional, but that doesn't mean it's really safe.
It's especially not safe in 9.3, which pre-dates use of MVCC scans for
catalog reads, but even in current releases it doesn't seem like a good
idea.

We know we'll need AccessExclusiveLock shortly to drop the obsoleted
constraint, so just get that a little sooner to close the hole.

Per testing with a patch that complains if we open a relation without
holding any lock on it.  I don't plan to back-patch that patch, but we
should close the holes it identifies in all supported branches.

Discussion: https://postgr.es/m/2038.1538335244@sss.pgh.pa.us
2018-10-01 11:39:13 -04:00
Peter Eisentraut
a6949ca34d doc: Clarify CREATE TABLESPACE documentation
Be more specific about when and how to create the directory and what
permissions it should have.

Discussion: https://www.postgresql.org/message-id/flat/5ca60e1a-26f9-89fd-e912-021dd2b8afe2%40gmail.com
2018-10-01 14:07:01 +02:00
Tom Lane
fdba460a26 Create an RTE field to record the query's lock mode for each relation.
Add RangeTblEntry.rellockmode, which records the appropriate lock mode for
each RTE_RELATION rangetable entry (either AccessShareLock, RowShareLock,
or RowExclusiveLock depending on the RTE's role in the query).

This patch creates the field and makes all creators of RTE nodes fill it
in reasonably, but for the moment nothing much is done with it.  The plan
is to replace assorted post-parser logic that re-determines the right
lockmode to use with simple uses of rte->rellockmode.  For now, just add
Asserts in each of those places that the rellockmode matches what they are
computing today.  (In some cases the match isn't perfect, so the Asserts
are weaker than you might expect; but this seems OK, as per discussion.)

This passes check-world for me, but it seems worth pushing in this state
to see if the buildfarm finds any problems in cases I failed to test.

catversion bump due to change of stored rules.

Amit Langote, reviewed by David Rowley and Jesper Pedersen,
and whacked around a bit more by me

Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp
2018-09-30 13:55:51 -04:00
Stephen Frost
8bddc86400 Add application_name to connection authorized msg
The connection authorized message has quite a bit of useful information
in it, but didn't include the application_name (when provided), so let's
add that as it can be very useful.

Note that at the point where we're emitting the connection authorized
message, we haven't processed GUCs, so it's not possible to get this by
using log_line_prefix (which pulls from the GUC).  There's also
something to be said for having this included in the connection
authorized message and then not needing to repeat it for every line, as
having it in log_line_prefix would do.

The GUC cleans the application name to pure-ascii, so do that here too,
but pull out the logic for cleaning up a string into its own function
in common and re-use it from those places, and check_cluster_name which
was doing the same thing.

Author: Don Seiler <don@seiler.us>
Discussion: https://postgr.es/m/CAHJZqBB_Pxv8HRfoh%2BAB4KxSQQuPVvtYCzMg7woNR3r7dfmopw%40mail.gmail.com
2018-09-28 19:04:50 -04:00
Tom Lane
2b04dfc472 Improve error reporting for unsupported effective_io_concurrency setting.
Give a specific error complaining about lack of posix_fadvise() when
someone tries to set effective_io_concurrency > 0 on platforms
without that.

This probably isn't worth extensive back-patching, but I (tgl) felt
cramming it into v11 was reasonable.

James Robinson

Discussion: https://postgr.es/m/153771876450.14994.560017943128223619@wrigleys.postgresql.org
Discussion: https://postgr.es/m/A3942987-5BC7-4F05-B54D-2A0EC2914B33@jlr-photo.com
2018-09-28 16:12:13 -04:00
Tom Lane
61f14cc8c8 Tweak MSVC build system to match changes in 7143b3e82.
Looks like we need to pull in $libpgcommon in a couple more
places than before.

Per buildfarm.
2018-09-28 15:49:05 -04:00
Tom Lane
97c6852ff7 Tweak MSVC build system to match changes in 7143b3e82.
Also try to make the comment suggesting that this might be needed
more intelligible.

Per buildfarm.
2018-09-28 15:17:07 -04:00
Tom Lane
7143b3e821 Build src/common files as a library with -fPIC.
Build a third version of libpgcommon.a, with -fPIC and -DFRONTEND,
as commit ea53100d5 did for src/port.  Use that in libpq to avoid
symlinking+rebuilding source files retail.

Also adjust ecpg to use the new src/port and src/common libraries.

Arrange to install these libraries, too, to simplify out-of-tree
builds of shared libraries that need any of these modules.

Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us
Discussion: https://postgr.es/m/E1g5Y8r-0006vs-QA@gemulon.postgresql.org
2018-09-28 14:28:19 -04:00
Tom Lane
f7ab802855 Remove pqsignal() from libpq's official exports list.
Client applications should get this function, if they need it, from
libpgport.

The fact that it's exported from libpq is a hack left over from before
we set up libpgport.  It's never been documented, and there's no good
reason for non-PG code to be calling it anyway, so hopefully this won't
cause any problems.  Moreover, with the previous setup it was not real
clear whether our clients that use the function were getting it from
libpgport or libpq, so this might actually prevent problems.

The reason for changing it now is that in the wake of commit ea53100d5,
some linkers won't export the symbol, apparently because it's coming from
a .a library instead of a .o file.  We could get around that by continuing
to symlink pqsignal.c into libpq as before; but unless somebody complains
very hard, I don't want to adopt such a kluge.

Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us
Discussion: https://postgr.es/m/E1g5Y8r-0006vs-QA@gemulon.postgresql.org
2018-09-28 12:38:10 -04:00
Amit Kapila
a86bf6057e Fix assertion failure when updating full_page_writes for checkpointer.
When the checkpointer receives a SIGHUP signal to update its configuration,
it may need to update the shared memory for full_page_writes and need to
write a WAL record for it.  Now, it is quite possible that the XLOG
machinery has not been initialized by that time and it will lead to
assertion failure while doing that.  Fix is to allow the initialization of
the XLOG machinery outside critical section.

This bug has been introduced by the commit 2c03216d83 which added the XLOG
machinery initialization in RecoveryInProgress code path.

Reported-by: Dilip Kumar
Author: Dilip Kumar
Reviewed-by: Michael Paquier and Amit Kapila
Backpatch-through: 9.5
Discussion: https://postgr.es/m/CAFiTN-u4BA8KXcQUWDPNgaKAjDXC=C2whnzBM8TAcv=stckYUw@mail.gmail.com
2018-09-28 16:40:04 +05:30
Andres Freund
92a0342a90 Correct overflow handling in pgbench.
This patch attempts, although it's quite possible there are a few
holes, to properly detect and reported signed integer overflows in
pgbench.

Author: Fabien Coelho
Reviewed-By: Andres Freund
Discussion: https://postgr.es/m/20171212052943.k2hlckfkeft3eiio@alap3.anarazel.de
2018-09-27 21:50:57 -07:00
Michael Paquier
78ea8b5daa Fix WAL recycling on standbys depending on archive_mode
A restart point or a checkpoint recycling WAL segments treats segments
marked with neither ".done" (archiving is done) or ".ready" (segment is
ready to be archived) in archive_status the same way for archive_mode
being "on" or "always".  While for a primary this is fine, a standby
running a restart point with archive_mode = on would try to mark such a
segment as ready for archiving, which is something that will never
happen except after the standby is promoted.

Note that this problem applies only to WAL segments coming from the
local pg_wal the first time archive recovery is run.  Segments part of a
self-contained base backup are the most common case where this could
happen, however even in this case normally the .done markers would be
most likely part of the backup.  Segments recovered from an archive are
marked as .ready or .done by the startup process, and segments finished
streaming are marked as such by the WAL receiver, so they are handled
already.

Reported-by: Haruka Takatsuka
Author: Michael Paquier
Discussion: https://postgr.es/m/15402-a453c90ed4cf88b2@postgresql.org
Backpatch-through: 9.5, where archive_mode = always has been added.
2018-09-28 11:54:38 +09:00
Tom Lane
aaf10f32a3 Fix assorted bugs in pg_get_partition_constraintdef().
It failed if passed a nonexistent relation OID, or one that was a non-heap
relation, because of blindly applying heap_open to a user-supplied OID.
This is not OK behavior for a SQL-exposed function; we have a project
policy that we should return NULL in such cases.  Moreover, since
pg_get_partition_constraintdef ought now to work on indexes, restricting
it to heaps is flat wrong anyway.

The underlying function generate_partition_qual() wasn't on board with
indexes having partition quals either, nor for that matter with rels
having relispartition set but yet null relpartbound.  (One wonders
whether the person who wrote the function comment blocks claiming that
these functions allow a missing relpartbound had ever tested it.)

Fix by testing relispartition before opening the rel, and by using
relation_open not heap_open.  (If any other relkinds ever grow the
ability to have relispartition set, the code will work with them
automatically.)  Also, don't reject null relpartbound in
generate_partition_qual.

Back-patch to v11, and all but the null-relpartbound change to v10.
(It's not really necessary to change generate_partition_qual at all
in v10, but I thought s/heap_open/relation_open/ would be a good
idea anyway just to keep the code in sync with later branches.)

Per report from Justin Pryzby.

Discussion: https://postgr.es/m/20180927200020.GJ776@telsasoft.com
2018-09-27 18:15:17 -04:00
Alexander Korotkov
4ec90f53f1 Minor formatting cleanup for 2a6368343f 2018-09-27 23:29:50 +03:00
Alexander Korotkov
0f64595894 Remove extra usage of BoxPGetDatum() macro
Author: Mark Dilger
Discussion: https://postgr.es/m/B2AEFCD0-836D-4654-9D59-3DF616E0A6F3%40gmail.com
2018-09-27 23:25:22 +03:00
Andres Freund
27e082b0c6 Clean up in the wake of TupleDescGetSlot() removal / 10763358c3.
The previous commit wasn't careful enough to remove all traces of
TupleDescGetSlot().

Besides fixing the oversight of not removing TupleDescGetSlot()'s
declaration, this also removes FuncCallContext->slot. That was
documented to be for use in combination with TupleDescGetSlot(), a
cursory search over extensions finds no users, and there doesn't seem
to be convincing reasons to keep it around. If we later in the v12
release cycle find users, we can re-consider this part of the commit.

Reported-By: Michael Paquier
Discussion: https://postgr.es/m/20180926000413.GC1659@paquier.xyz
2018-09-27 11:38:11 -07:00
Tom Lane
ea53100d56 Build src/port files as a library with -fPIC, and use that in libpq.
libpq and ecpg need shared-library-friendly versions of assorted src/port/
and src/common/ modules.  Up to now, they got those by symlinking the
individual source files and compiling them locally.  That's baroque, and a
pain to maintain, and it results in some amount of duplicated compile work.
It might've made sense when only a couple of files were needed, but the
list has grown and grown and grown :-(

It makes more sense to have the originating directory build a third variant
of libpgport.a/libpgcommon.a containing modules built with $(CFLAGS_SL),
and just link that into the shared library.  Unused files won't get linked,
so the end result should be the same.

This patch makes a down payment on that idea by having src/port/ build
such a library and making libpq use it.  If the buildfarm doesn't expose
fatal problems with the approach, I'll extend it to the other cases.

Discussion: https://postgr.es/m/13022.1538003440@sss.pgh.pa.us
2018-09-27 11:23:43 -04:00
Tom Lane
ce4887bd02 Fix another portability issue from commit 758ce9b77.
strerror.c now requires strlcpy() in some cases, and a couple of the
ecpg libraries did not have that at hand.  Pull it in from src/port/
following the usual recipe.  Per buildfarm.
2018-09-26 19:03:40 -04:00
Michael Paquier
ba16aade33 Switch flags tracking pending interrupts to sig_atomic_t
Those previously used bool, which should be safe on any modern
platforms, however the C standard is clear that it is better to use
sig_atomic_t for variables manipulated in signal handlers.  This commit
adds at the same time PGDLLIMPORT to ClientConnectionLost.

Author: Michael Paquier
Reviewed-by: Tom Lane, Chris Travers, Andres Freund
Discussion: https://postgr.es/m/20180925011311.GD1354@paquier.xyz
2018-09-27 07:47:20 +09:00
Tom Lane
751f532b97 Try another way to detect the result type of strerror_r().
The method we've traditionally used, of redeclaring strerror_r() to
see if the compiler complains of inconsistent declarations, turns out
not to work reliably because some compilers only report a warning,
not an error.  Amazingly, this has gone undetected for years, even
though it certainly breaks our detection of whether strerror_r
succeeded.

Let's instead test whether the compiler will take the result of
strerror_r() as a switch() argument.  It's possible this won't
work universally either, but it's the best idea I could come up with
on the spur of the moment.

We should probably back-patch this once the dust settles, but
first let's see what the buildfarm thinks of it.

Discussion: https://postgr.es/m/10877.1537993279@sss.pgh.pa.us
2018-09-26 18:23:13 -04:00
Tom Lane
8b91d25884 Clean up *printf macros to avoid conflict with format archetypes.
We must define the macro "printf" with arguments, else it can mess
up format archetype attributes in builds where PG_PRINTF_ATTRIBUTE
is just "printf".  Fortunately, that's easy to do now that we're
requiring C99; we can use __VA_ARGS__.

On the other hand, it's better not to use __VA_ARGS__ for the rest
of the *printf crew, so that one can take the addresses of those
functions without surprises.

I'd proposed doing this some time ago, but forgot to make it happen;
buildfarm failures subsequent to 96bf88d52 reminded me.

Discussion: https://postgr.es/m/22709.1535135640@sss.pgh.pa.us
Discussion: https://postgr.es/m/20180926190934.ea4xvzhkayuw7gkx@alap3.anarazel.de
2018-09-26 17:35:01 -04:00
Tom Lane
a6b88d682c Fix link failures due to snprintf/strerror changes.
snprintf.c requires isnan(), which requires -lm on some platforms.
libpq never bothered with -lm before, but now it needs it.

strerror.c tries to translate a string or two, which requires -lintl.
We'd managed never to need that anywhere in ecpg/pgtypeslib/ before,
but now we do.

Per buildfarm and a report from Peter Eisentraut.

Discussion: https://postgr.es/m/20180926190934.ea4xvzhkayuw7gkx@alap3.anarazel.de
Discussion: https://postgr.es/m/f67b5008-9f01-057f-2bff-558cb53af851@2ndquadrant.com
2018-09-26 16:47:44 -04:00
Peter Eisentraut
0320ddaf3c Recurse to sequences on ownership change for all relkinds
When a table ownership is changed, we must apply that also to any owned
sequences.  (Otherwise, it would result in a situation that cannot be
restored, because linked sequences must have the same owner as the
table.)  But this was previously only applied to regular tables and
materialized views.  But it should also apply to at least foreign
tables.  This patch removes the relkind check altogether, because it
doesn't save very much and just introduces the possibility of similar
omissions.

Bug: #15238
Reported-by: Christoph Berg <christoph.berg@credativ.de>
2018-09-26 20:19:15 +02:00
Tom Lane
d6c55de1f9 Implement %m in src/port/snprintf.c, and teach elog.c to rely on that.
I started out with the idea that we needed to detect use of %m format specs
in contexts other than elog/ereport calls, because we couldn't rely on that
working in *printf calls.  But a better answer is to fix things so that it
does work.  Now that we're using snprintf.c all the time, we can implement
%m in that and we've fixed the problem.

This requires also adjusting our various printf-wrapping functions so that
they ensure "errno" is preserved when they call snprintf.c.

Remove elog.c's handmade implementation of %m, and let it rely on
snprintf to support the feature.  That should provide some performance
gain, though I've not attempted to measure it.

There are a lot of places where we could now simplify 'printf("%s",
strerror(errno))' into 'printf("%m")', but I'm not in any big hurry
to make that happen.

Patch by me, reviewed by Michael Paquier

Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
2018-09-26 13:31:56 -04:00
Tom Lane
96bf88d527 Always use our own versions of *printf().
We've spent an awful lot of effort over the years in coping with
platform-specific vagaries of the *printf family of functions.  Let's just
forget all that mess and standardize on always using src/port/snprintf.c.
This gets rid of a lot of configure logic, and it will allow a saner
approach to dealing with %m (though actually changing that is left for
a follow-on patch).

Preliminary performance testing suggests that as it stands, snprintf.c is
faster than the native printf functions for some tasks on some platforms,
and slower for other cases.  A pending patch will improve that, though
cases with floating-point conversions will doubtless remain slower unless
we want to put a *lot* of effort into that.  Still, we've not observed
that *printf is really a performance bottleneck for most workloads, so
I doubt this matters much.

Patch by me, reviewed by Michael Paquier

Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
2018-09-26 13:13:57 -04:00
Tom Lane
758ce9b779 Incorporate strerror_r() into src/port/snprintf.c, too.
This provides the features that used to exist in useful_strerror()
for users of strerror_r(), too.  Also, standardize on the GNU convention
that strerror_r returns a char pointer that may not be NULL.

I notice that libpq's win32.c contains a variant version of strerror_r
that probably ought to be folded into strerror.c.  But lacking a
Windows environment, I should leave that to somebody else.

Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
2018-09-26 12:35:57 -04:00
Tom Lane
26e9d4d4ef Convert elog.c's useful_strerror() into a globally-used strerror wrapper.
elog.c has long had a private strerror wrapper that handles assorted
possible failures or deficiencies of the platform's strerror.  On Windows,
it also knows how to translate Winsock error codes, which the native
strerror does not.  Move all this code into src/port/strerror.c and
define strerror() as a macro that invokes it, so that both our frontend
and backend code will have all of this behavior.

I believe this constitutes an actual bug fix on Windows, since AFAICS
our frontend code did not report Winsock error codes properly before this.
However, the main point is to lay the groundwork for implementing %m
in src/port/snprintf.c: the behavior we want %m to have is this one,
not the native strerror's.

Note that this throws away the prior use of src/port/strerror.c,
which was to implement strerror() on platforms lacking it.  That's
been dead code for nigh twenty years now, since strerror() was
already required by C89.

We should likewise cause strerror_r to use this behavior, but
I'll tackle that separately.

Patch by me, reviewed by Michael Paquier

Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
2018-09-26 11:06:42 -04:00
Peter Eisentraut
a49ceda6a0 Update dummy CREATE ASSERTION grammar
While we are probably still far away from fully implementing
assertions, all patch proposals appear to take issue with the existing
dummy grammar CREATE/DROP ASSERTION productions, so update those a
little bit.  Rename the rule, use any_name instead of name, and remove
some unused code.  Also remove the production for DROP ASSERTION,
since that would most likely be handled via the generic DROP support.

extracted from a patch by Joe Wildish
2018-09-26 13:26:24 +02:00
Tomas Vondra
a3d2844852 Improve test coverage of geometric types
This commit significantly increases test coverage of geo_ops.c, adding
tests for various issues addressed by 2e2a392de3 (which went undetected
for a long time, at least partially due to not being covered).

This also removes alternative results expecting -0 on some platforms.
Instead the functions are should return the same results everywhere,
transforming -0 to 0 if needed.

The tests are added to geometric.sql file, sorted by the left hand side
of the operators. There are many cross datatype operators, so this seems
like the best solution.

Author: Emre Hasegeli
Reviewed-by: Tomas Vondra

Discussion: https://www.postgresql.org/message-id/CAE2gYzxF7-5djV6-cEvqQu-fNsnt%3DEqbOURx7ZDg%2BVv6ZMTWbg%40mail.gmail.com
2018-09-26 10:45:21 +02:00
Tomas Vondra
2e2a392de3 Fix problems in handling the line data type
According to the source history, the internal format of line data type
has changed, but various functions working with it did were not updated
and thus were producing wrong results.

This patch addresses various such issues, in particular:

* Reject invalid specification A=B=0 on receive
* Reject same points on line_construct_pp()
* Fix perpendicular operator when negative values are involved
* Avoid division by zero on perpendicular operator
* Fix intersection and distance operators when neither A nor B are 1
* Return NULL for closest point when objects are parallel
* Check whether closest point of line segments is the intersection point
* Fix closest point of line segments being on the wrong segment

Aside from handling those issues, the patch also aims to make operators
more symmetric and less sen to precision loss.  The EPSILON interferes
with even minor changes, but the least we can do is applying it to both
sides of the operators equally.

Author: Emre Hasegeli
Reviewed-by: Tomas Vondra

Discussion: https://www.postgresql.org/message-id/CAE2gYzxF7-5djV6-cEvqQu-fNsnt%3DEqbOURx7ZDg%2BVv6ZMTWbg%40mail.gmail.com
2018-09-26 10:25:24 +02:00
Michael Paquier
f535d5f0c1 Add basic regression tests for default monitoring roles
The following default roles gain some coverage:
- pg_read_all_stats
- pg_read_all_settings

Author: Alexandra Ryzhevich
Discussion: https://postgr.es/m/CAOt4E5S5WJmDc9YpS1BfyAMQ5C1NEmiYynD6nUz42qVxphqkpA@mail.gmail.com
2018-09-26 15:26:45 +09:00
Michael Paquier
8d28bf500f Rework activation of commit timestamps during recovery
The activation and deactivation of commit timestamp tracking has not
been handled consistently for a primary or standbys at recovery.  The
facility can be activated at three different moments of recovery:
- The beginning, where a primary would use the GUC value for the
decision-making, and where a standby relies on the contents of the
control file.
- When replaying a XLOG_PARAMETER_CHANGE record at redo.
- The end, where both primary and standby rely on the GUC value.

Using the GUC value for a primary at the beginning of recovery causes
problems with commit timestamp access when doing crash recovery.
Particularly, when replaying transaction commits, it could be possible
that an attempt to read commit timestamps is done for a transaction
which committed at a moment when track_commit_timestamp was disabled.

A test case is added to reproduce the failure.  The test works down to
v11 as it takes advantage of transaction commits within procedures.

Reported-by: Hailong Li
Author: Masahiko Sawasa, Michael Paquier
Reviewed-by: Kyotaro Horiguchi
Discussion: https://postgr.es/m/11224478-a782-203b-1f17-e4797b39bdf0@qunar.com
Backpatch-through: 9.5, where commit timestamps have been introduced.
2018-09-26 10:25:54 +09:00
Andres Freund
10763358c3 Remove absolete function TupleDescGetSlot().
TupleDescGetSlot() was kept around for backward compatibility for
user-written SRFs. With the TupleTableSlot abstraction work, that code
will need to be version specific anyway, so there's no point in
keeping the function around any longer.

Author: Ashutosh Bapat
Reviewed-By: Andres Freund
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
2018-09-25 16:28:57 -07:00
Andres Freund
29c94e03c7 Split ExecStoreTuple into ExecStoreHeapTuple and ExecStoreBufferHeapTuple.
Upcoming changes introduce further types of tuple table slots, in
preparation of making table storage pluggable. New storage methods
will have different representation of tuples, therefore the slot
accessor should refer explicitly to heap tuples.

Instead of just renaming the functions, split it into one function
that accepts heap tuples not residing in buffers, and one accepting
ones in buffers.  Previously one function was used for both, but that
was a bit awkward already, and splitting will allow us to represent
slot types for tuples in buffers and normal memory separately.

This is split out from the patch introducing abstract slots, as this
largely consists out of mechanical changes.

Author: Ashutosh Bapat
Reviewed-By: Andres Freund
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
2018-09-25 16:27:48 -07:00
Andres Freund
bbdfbb9154 Remove function list from prologue of execTuples.c.
That section is never in sync with the actual routines available and
their functionality.

Author: Ashutosh Bapat
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
2018-09-25 16:27:48 -07:00
Andres Freund
a598708ffa Change TupleTableSlot->tts_nvalid to type AttrNumber.
Previously it was an int / 4 bytes. The maximum number of attributes
in a tuple is restricted by the maximum value Var->varattno, which is
an AttrNumber/int16. Hence use the same data type for
TupleTableSlot->tts_nvalid.

Author: Ashutosh Bapat
Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
2018-09-25 15:59:46 -07:00
Alvaro Herrera
5913b9bbf3 Remove obsolete comment
The documented shortcoming was actually fixed in 4c728f3829
so the comment is not true anymore.
2018-09-25 17:55:22 -03:00
Alvaro Herrera
62e533d3f1 Remove fmgr.h inclusion from partition.h
It's not needed anymore.
2018-09-25 17:52:07 -03:00
Andres Freund
33001fd7a7 Collect JIT instrumentation from workers.
Previously, when using parallel query, EXPLAIN (ANALYZE)'s JIT
compilation timings did not include the overhead from doing so on the
workers.  Fix that.

We do so by simply aggregating the cost of doing JIT compilation on
workers and the leader together. Arguably that's not quite accurate,
because the total time spend doing so is spent in parallel - but it's
hard to do much better.  For additional detail, when VERBOSE is
specified, the stats for workers are displayed separately.

Author: Amit Khandekar and Andres Freund
Discussion: https://postgr.es/m/CAJ3gD9eLrz51RK_gTkod+71iDcjpB_N8eC6vU2AW-VicsAERpQ@mail.gmail.com
Backpatch: 11-
2018-09-25 13:12:44 -07:00
Tom Lane
5e22171310 Make some fixes to allow building Postgres on macOS 10.14 ("Mojave").
Apple's latest rearrangements of the system-supplied headers have broken
building of PL/Perl and PL/Tcl.  The only practical way to fix PL/Tcl is to
start using the "-isysroot" compiler flag to point to SDK-supplied headers,
as Apple expects.  We must also start distinguishing where to find Perl's
headers from where to find its shared library; but that seems like good
cleanup anyway.

Extensions that formerly did something like -I$(perl_archlibexp)/CORE
should now do -I$(perl_includedir)/CORE instead.  perl_archlibexp
is still the place to look for libperl.so, though.

If for some reason you don't like the default -isysroot setting, you can
override that by setting PG_SYSROOT in configure's arguments.  I don't
currently think people would need to do so, unless maybe for cross-version
build purposes.

In addition, teach configure where to find tclConfig.sh.  Our traditional
method of searching $auto_path hasn't worked for the last couple of macOS
releases, and it now seems clear that Apple's not going to change that.
The workaround of manually specifying --with-tclconfig was annoying
already, but Mojave's made it a lot more so because the sysroot path now
has to be included as well.  Let's just wire the knowledge into configure
instead.  To avoid breaking builds against non-default Tcl installations
(e.g. MacPorts) wherein the $auto_path method probably still works,
arrange to try the additional case only after all else has failed.

Back-patch to all supported versions, since at least the buildfarm
cares about that.  The changes are set up to not do anything on macOS
releases that are old enough to not have functional sysroot trees.
2018-09-25 13:23:29 -04:00
Tom Lane
5b7e036707 Avoid unnecessary precision loss for pgbench's --rate target.
It's fairly silly to truncate the throttle_delay to integer when the only
math we ever do with it requires converting back to double.  Furthermore,
given that people are starting to complain about restrictions like only
supporting 1K client connections, I don't think we're very far away from
situations where the precision loss matters.  As the code stood, for
example, there's no difference between --rate 100001 and --rate 111111;
both get converted to throttle_delay = 9.  Somebody trying to run 100
threads and have each one dispatch around 1K TPS would find this lack of
precision rather surprising, especially since the required per-thread
delays are around 1ms, well within the timing precision of modern systems.
2018-09-25 11:09:18 -04:00