Alvaro Herrera [Fri, 18 Jan 2019 17:40:13 +0000 (14:40 -0300)]
Refactor duplicate code into DeconstructFkConstraintRow
My commit 3de241dba86f introduced some code (in tablecmds.c) to obtain
data from a pg_constraint row for a foreign key, that already existed in
ri_triggers.c. Split it out into its own routine in pg_constraint.c,
where it naturally belongs.
No functional code changes, only code movement.
Backpatch to pg11, because a future bugfix is simpler after this.
Tom Lane [Fri, 18 Jan 2019 16:05:11 +0000 (11:05 -0500)]
Avoid sometimes printing both tables and their columns in DROP CASCADE.
A cascaded drop might find independent reasons to drop both a table
and some column of the table (for instance, a schema drop might include
dropping a data type used in some table in the schema). Depending on
the order of visitation of pg_depend entries, we might report the
table column and the whole table as separate objects-to-be-dropped,
or we might only report the table. This is confusing and leads to
unstable regression test output, so fix it to report only the table
regardless of visitation order.
Per gripe from Peter Geoghegan. This is just cosmetic from a user's
standpoint, and we haven't actually seen regression test problems in
practice (yet), so I'll refrain from back-patching.
Peter Eisentraut [Fri, 18 Jan 2019 07:29:42 +0000 (08:29 +0100)]
configure: More use of AC_ARG_VAR
AC_ARG_VAR is necessary if an environment variable influences a
configure result that is then used by other tests that are cached.
With AC_ARG_VAR, a change in the variable is detected on subsequent
configure runs and the user is then advised to remove the cache.
This adds AC_ARG_VAR calls for: MSGFMT, PERL, PYTHON, TCLSH, XML2_CONFIG
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/30672.1546816567@sss.pgh.pa.us
Michael Paquier [Fri, 18 Jan 2019 01:51:39 +0000 (10:51 +0900)]
Enforce non-parallel plan when calling current_schema() in newly-added test
current_schema() gets called in the recently-added regression test from c5660e0, and can be used in a parallel context, causing its call to fail
when creating a temporary schema.
Tom Lane [Fri, 18 Jan 2019 00:31:03 +0000 (19:31 -0500)]
Avoid assuming that we know the spelling of getopt_long's error messages.
I've had enough of "fixing" this test case. Whatever value it has
is limited to verifying that pgbench fails for an unrecognized switch,
and we don't need to assume anything about what getopt_long prints in
order to do that.
Michael Paquier [Fri, 18 Jan 2019 00:21:44 +0000 (09:21 +0900)]
Restrict the use of temporary namespace in two-phase transactions
Attempting to use a temporary table within a two-phase transaction is
forbidden for ages. However, there have been uncovered grounds for
a couple of other object types and commands which work on temporary
objects with two-phase commit. In short, trying to create, lock or drop
an object on a temporary schema should not be authorized within a
two-phase transaction, as it would cause its state to create
dependencies with other sessions, causing all sorts of side effects with
the existing session or other sessions spawned later on trying to use
the same temporary schema name.
Regression tests are added to cover all the grounds found, the original
report mentioned function creation, but monitoring closer there are many
other patterns with LOCK, DROP or CREATE EXTENSION which are involved.
One of the symptoms resulting in combining both is that the session
which used the temporary schema is not able to shut down completely,
waiting for being able to drop the temporary schema, something that it
cannot complete because of the two-phase transaction involved with
temporary objects. In this case the client is able to disconnect but
the session remains alive on the backend-side, potentially blocking
connection backend slots from being used. Other problems reported could
also involve server crashes.
This is back-patched down to v10, which is where 9b013dc has introduced
MyXactFlags, something that this patch relies on.
Andrew Gierth [Thu, 17 Jan 2019 05:33:01 +0000 (05:33 +0000)]
Postpone aggregate checks until after collation is assigned.
Previously, parseCheckAggregates was run before
assign_query_collations, but this causes problems if any expression
has already had a collation assigned by some transform function (e.g.
transformCaseExpr) before parseCheckAggregates runs. The differing
collations would cause expressions not to be recognized as equal to
the ones in the GROUP BY clause, leading to spurious errors about
unaggregated column references.
The result was that CASE expr WHEN val ... would fail when "expr"
contained a GROUPING() expression or matched one of the group by
expressions, and where collatable types were involved; whereas the
supposedly identical CASE WHEN expr = val ... would succeed.
Backpatch all the way; this appears to have been wrong ever since
collations were introduced.
Per report from Guillaume Lelarge, analysis and patch by me.
Etsuro Fujita [Thu, 17 Jan 2019 05:37:33 +0000 (14:37 +0900)]
postgres_fdw: Remove duplicate code in DML execution callback functions.
postgresExecForeignInsert(), postgresExecForeignUpdate(), and
postgresExecForeignDelete() are coded almost identically, except that
postgresExecForeignInsert() does not need CTID. Extract that code into
a separate function and use it in all the three function implementations.
Author: Ashutosh Bapat Reviewed-By: Michael Paquier
Discussion: https://postgr.es/m/CAFjFpRcz8yoY7cBTYofcrCLwjaDeCcGKyTUivUbRiA57y3v-bw%40mail.gmail.com
It seems modules are better defined like this instead of the original
split.
Per complaints from David Rowley as well as Amit Langote's self review.
Discussion: https://postgr.es/m/CAKJS1f988rsyhwvLgfT-y1UCYUfXDOv67ENQk=v24OxhsZOzZw@mail.gmail.com
Peter Eisentraut [Wed, 16 Jan 2019 15:53:55 +0000 (16:53 +0100)]
Increase test coverage in RI_Initial_Check()
This covers the special error handling of FKCONSTR_MATCH_FULL.
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com> Reviewed-by: Mi Tar <mmitar@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/7ae17c95-0c99-d420-032a-c271f510112b@2ndquadrant.com/
Peter Eisentraut [Wed, 16 Jan 2019 15:53:47 +0000 (16:53 +0100)]
Increase test coverage in RI_FKey_fk_upd_check_required()
This checks the code path of FKCONSTR_MATCH_FULL and
RI_KEYS_SOME_NULL.
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com> Reviewed-by: Mi Tar <mmitar@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/7ae17c95-0c99-d420-032a-c271f510112b@2ndquadrant.com/
Peter Eisentraut [Wed, 16 Jan 2019 15:53:38 +0000 (16:53 +0100)]
Increase test coverage in RI_FKey_pk_upd_check_required()
This checks the case where the primary key has at least one null
column.
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com> Reviewed-by: Mi Tar <mmitar@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/7ae17c95-0c99-d420-032a-c271f510112b@2ndquadrant.com/
Peter Eisentraut [Wed, 16 Jan 2019 15:52:07 +0000 (16:52 +0100)]
Add test case for ON DELETE NO ACTION/RESTRICT
This was previously not covered at all; function
RI_FKey_restrict_del() was not exercised in the tests.
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com> Reviewed-by: Mi Tar <mmitar@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/7ae17c95-0c99-d420-032a-c271f510112b@2ndquadrant.com/
Andres Freund [Tue, 15 Jan 2019 19:59:32 +0000 (11:59 -0800)]
Fix parent of WCO qual.
The parent of some WCO expressions was, apparently by accident, set to
the the source of DML queries, rather than the target table. This
causes problems for the upcoming pluggable storage work, because the
target and source table might be of different storage types.
It's possible that this is already problematic, but neither
experimenting nor inquiries on -hackers have found them. So don't
backpatch for now.
Author: Andres Freund
Discussion: https://postgr.es/m/20181205225213.hiwa3kgoxeybqcqv@alap3.anarazel.de
Tom Lane [Tue, 15 Jan 2019 17:07:10 +0000 (12:07 -0500)]
Finish reverting "recheck_on_update" patch.
This reverts commit c203d6cf8 and some follow-on fixes, completing the
task begun in commit 5d28c9bd7. If that feature is ever resurrected,
the code will look quite a bit different from this, so it seems best
to start from a clean slate.
The v11 branch is not touched; in that branch, the recheck_on_update
storage option remains present, but nonfunctional and undocumented.
Andres Freund [Tue, 15 Jan 2019 01:02:12 +0000 (17:02 -0800)]
Don't include genam.h from execnodes.h and relscan.h anymore.
This is the genam.h equivalent of 4c850ecec649c (which removed
heapam.h from a lot of other headers). There's still a few header
includes of genam.h, but not from central headers anymore.
As a few headers are not indirectly included anymore, execnodes.h and
relscan.h need a few additional includes. Some of the depended on
types were replacable by using the underlying structs, but e.g. for
Snapshot in execnodes.h that'd have gotten more invasive than
reasonable in this commit.
Like the aforementioned commit 4c850ecec649c, this requires adding new
genam.h includes to a number of backend files, which likely is also
required in a few external projects.
Author: Andres Freund
Discussion: https://postgr.es/m/20190114000701.y4ttcb74jpskkcfb@alap3.anarazel.de
Andres Freund [Tue, 15 Jan 2019 00:15:20 +0000 (16:15 -0800)]
Make naming of tupdesc related structs more consistent with the rest of PG.
We usually don't change the name of structs between the struct name
itself and the name of the typedef. Additionally, structs that are
usually used via a typedef that hides being a pointer, are commonly
suffixed Data. Change tupdesc code to follow those convention.
This is triggered by a future patch that intends to forward declare
TupleDescData in another header - keeping with the naming scheme makes
that easier to understand.
Author: Andres Freund
Discussion: https://postgr.es/m/20190114000701.y4ttcb74jpskkcfb@alap3.anarazel.de
Andres Freund [Mon, 14 Jan 2019 23:54:18 +0000 (15:54 -0800)]
Don't include heapam.h from others headers.
heapam.h previously was included in a number of widely used
headers (e.g. execnodes.h, indirectly in executor.h, ...). That's
problematic on its own, as heapam.h contains a lot of low-level
details that don't need to be exposed that widely, but becomes more
problematic with the upcoming introduction of pluggable table storage
- it seems inappropriate for heapam.h to be included that widely
afterwards.
heapam.h was largely only included in other headers to get the
HeapScanDesc typedef (which was defined in heapam.h, even though
HeapScanDescData is defined in relscan.h). The better solution here
seems to be to just use the underlying struct (forward declared where
necessary). Similar for BulkInsertState.
Another problem was that LockTupleMode was used in executor.h - parts
of the file tried to cope without heapam.h, but due to the fact that
it indirectly included it, several subsequent violations of that goal
were not not noticed. We could just reuse the approach of declaring
parameters as int, but it seems nicer to move LockTupleMode to
lockoptions.h - that's not a perfect location, but also doesn't seem
bad.
As a number of files relied on implicitly included heapam.h, a
significant number of files grew an explicit include. It's quite
probably that a few external projects will need to do the same.
Author: Andres Freund Reviewed-By: Alvaro Herrera
Discussion: https://postgr.es/m/20190114000701.y4ttcb74jpskkcfb@alap3.anarazel.de
Michael Paquier [Mon, 14 Jan 2019 23:47:01 +0000 (08:47 +0900)]
Fix typos in documentation and for one wait event
These have been found while cross-checking for the use of unique words
in the documentation, and a wait event was not getting generated in a way
consistent to what the documentation provided.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/9b5a3a85-899a-ae62-dbab-1e7943aa5ab1@gmail.com
Andres Freund [Mon, 14 Jan 2019 23:30:24 +0000 (15:30 -0800)]
Re-add default_with_oids GUC to avoid breaking old dump files.
After 578b229718 / the removal of WITH OIDS support, older dump files
containing
SET default_with_oids = false;
either report unnecessary errors (as the subsequent tables have no
oids) or even fail to restore entirely (when using transaction mode).
To avoid that, re-add the GUC, but don't allow setting it to true.
Per complaint from Tom Lane.
Author: Amit Khandekar, editorialized by me
Discussion: https://postgr.es/m/CAJ3gD9dZyxrtL0rJfoNoOj6v7fJSDaXBngi9wy5XU8m-ioXhAA@mail.gmail.com
Detach postmaster process from pg_ctl's session at server startup.
pg_ctl is supposed to daemonize the postmaster process, so that it's not
affected by signals to the launching process group. Before this patch, if
you had a shell script that used "pg_ctl start", and you interrupted the
shell script after postmaster had been launched, postmaster was also
killed. To fix, call setsid() after forking the postmaster process.
Long time ago, we had a 'silent_mode' option, which daemonized the
postmaster process by calling setsid(), but that was removed back in 2011
(commit f7ea6beaf4). We discussed bringing that back in some form, but
pg_ctl is the documented way of launching postmaster to the background, so
putting the setsid() call in pg_ctl itself seems appropriate.
Just putting postmaster in a separate session would change the behavior
when you interrupt "pg_ctl -w start", e.g. with CTRL-C, while it's waiting
for postmaster to start. The historical behavior has been that
interrupting pg_ctl aborts the server launch, which is handy if the server
is stuck in recovery, for example, and won't fully start up. To keep that
behavior, install a signal handler in pg_ctl, to explicitly kill
postmaster, if pg_ctl is interrupted while it's waiting for the server to
start up. This isn't 100% watertight, there is a small window after
forking the postmaster process, where the signal handler doesn't know the
postmaster's PID yet, but seems good enough.
Arguably this is a long-standing bug, but I refrained from back-batching,
out of fear of breaking someone's scripts that depended on the old
behavior.
Reviewed by Tom Lane. Report and original patch by Paul Guo, with
feedback from Michael Paquier.
Noah Misch [Sun, 13 Jan 2019 19:32:31 +0000 (11:32 -0800)]
Make Emacs perl-mode indent more like perltidy.
This especially helps braces that surround code blocks. Back-patch to
v11, where commit 56fb890ace8ac0ca955ae0803c580c2074f876f6 first
appeared; before that, settings were even more distant from perltidy.
Tom Lane [Sun, 13 Jan 2019 18:33:50 +0000 (13:33 -0500)]
Improve missing-program error handling in make_ctags and make_etags.
If ctags (resp. etags) isn't installed, these scripts naturally fail,
but the error messages were less clear than one could wish.
It seems worth installing an explicit test to improve that.
Nikolay Shaplov, with suggestions from Michael Paquier and Andrew Dunstan
Peter Eisentraut [Sun, 13 Jan 2019 09:50:36 +0000 (10:50 +0100)]
Make INSTALL makefile rule more robust
With the previous rule, if pandoc was missing, a zero-length output
file would be created without an error from make. To improve that,
write the rule as two separate commands without a pipe.
Peter Eisentraut [Fri, 11 Jan 2019 09:36:10 +0000 (10:36 +0100)]
Change default of recovery_target_timeline to 'latest'
This is what one usually wants for recovery and almost always wants
for a standby.
Discussion: https://www.postgresql.org/message-id/flat/6dd2c23a-4162-8469-410f-bfe146e28c0c@2ndquadrant.com/ Reviewed-by: David Steele <david@pgmasters.net> Reviewed-by: Michael Paquier <michael@paquier.xyz>
Michael Paquier [Sun, 13 Jan 2019 07:39:49 +0000 (16:39 +0900)]
Improve error messages for incorrect types of logical replication targets
If trying to use something else than a plain table as logical
replication target, a rather-generic error message gets used to report
the problem. This can be confusing when it comes to foreign tables and
partitioned tables, so use more dedicated messages in these cases.
Author: Amit Langote Reviewed-by: Peter Eisentraut, Magnus Hagander, Michael Paquier
Discussion: https://postgr.es/m/41799bee-40eb-7bb5-80b1-325ce17518bc@lab.ntt.co.jp
Tom Lane [Fri, 11 Jan 2019 20:53:34 +0000 (15:53 -0500)]
Avoid sharing PARAM_EXEC slots between different levels of NestLoop.
Up to now, createplan.c attempted to share PARAM_EXEC slots for
NestLoopParams across different plan levels, if the same underlying Var
was being fed down to different righthand-side subplan trees by different
NestLoops. This was, I think, more of an artifact of using subselect.c's
PlannerParamItem infrastructure than an explicit design goal, but anyway
that was the end result.
This works well enough as long as the plan tree is executing synchronously,
but the feature whereby Gather can execute the parallelized subplan locally
breaks it. An upper NestLoop node might execute for a row retrieved from
a parallel worker, and assign a value for a PARAM_EXEC slot from that row,
while the leader's copy of the parallelized subplan is suspended with a
different active value of the row the Var comes from. When control
eventually returns to the leader's subplan, it gets the wrong answers if
the same PARAM_EXEC slot is being used within the subplan, as reported
in bug #15577 from Bartosz Polnik.
This is pretty reminiscent of the problem fixed in commit 46c508fbc, and
the proper fix seems to be the same: don't try to share PARAM_EXEC slots
across different levels of controlling NestLoop nodes.
This requires decoupling NestLoopParam handling from PlannerParamItem
handling, although the logic remains somewhat similar. To avoid bizarre
division of labor between subselect.c and createplan.c, I decided to move
all the param-slot-assignment logic for both cases out of those files
and put it into a new file paramassign.c. Hopefully it's a bit better
documented now, too.
A regression test case for this might be nice, but we don't know a
test case that triggers the problem with a suitably small amount
of data.
Back-patch to 9.6 where we added Gather nodes. It's conceivable that
related problems exist in older branches; but without some evidence
for that, I'll leave the older branches alone.
Peter Eisentraut [Fri, 11 Jan 2019 16:21:45 +0000 (17:21 +0100)]
doc: Correct documentation of install-time environment variables
Since approximately PostgreSQL 10, it is no longer required that
environment variables at installation time such as PERL, PYTHON, TCLSH
be "full path names", so change that phrasing in the installation
instructions. (The exact time of change appears to differ for PERL
and the others, but it works consistently in PostgreSQL 10.)
Also while we're here document the defaults for PERL and PYTHON, but
since the search list for TCLSH is so long, let's leave that out so we
don't need to maintain a copy of that list in the installation
instructions.
Peter Eisentraut [Fri, 11 Jan 2019 11:02:48 +0000 (12:02 +0100)]
Create INSTALL file using Pandoc
Replace using lynx with using pandoc. Pandoc creates better looking
output and it avoids the delicate locale/encoding issues of lynx because
it always uses UTF-8 for both input and output.
Note: requires Pandoc >=1.13
Discussion: https://www.postgresql.org/message-id/flat/dcfaa74d-8037-bb32-f9e0-3fea7ccf4551@2ndquadrant.com/ Reviewed-by: Mi Tar <mmitar@gmail.com>
Peter Eisentraut [Fri, 11 Jan 2019 09:25:06 +0000 (10:25 +0100)]
Add value 'current' for recovery_target_timeline
This value represents the default behavior of using the current
timeline. Previously, this was represented by an empty string.
(Before the removal of recovery.conf, this setting could not be chosen
explicitly but was used when recovery_target_timeline was not
mentioned at all.)
Discussion: https://www.postgresql.org/message-id/flat/6dd2c23a-4162-8469-410f-bfe146e28c0c@2ndquadrant.com/ Reviewed-by: David Steele <david@pgmasters.net> Reviewed-by: Michael Paquier <michael@paquier.xyz>
Amit Kapila [Fri, 11 Jan 2019 03:20:09 +0000 (08:50 +0530)]
Extend pg_stat_statements_reset to reset statistics specific to a
particular user/db/query.
The function pg_stat_statements_reset() is extended to accept userid, dbid,
and queryid as input parameters. Now, it can discard the statistics
gathered so far by pg_stat_statements corresponding to the specified
userid, dbid, and queryid. If no parameter is specified or all the
specified parameters have default value aka 0, it will discard all
statistics as per the old behavior.
The new behavior is useful to get the fresh statistics for a specific
user/database/query without resetting all the existing statistics.
Author: Haribabu Kommi, with few additional changes by me Reviewed-by: Michael Paquier, Amit Kapila and Fujii Masao
Discussion: https://postgr.es/m/CAJrrPGcyh-gkFswyc6C661K6cknL0XkNqVT0sQt2mFNMR4HRKA@mail.gmail.com
Andrew Dunstan [Thu, 10 Jan 2019 20:53:45 +0000 (15:53 -0500)]
Fix missing values when doing ALTER TABLE ALTER COLUMN TYPE
This was an oversight in commit 16828d5c. If the table is going to be
rewritten, we simply clear all the missing values from all the table's
attributes, since there will no longer be any rows with the attributes
missing. Otherwise, we repackage the missing value in an array
constructed with the new type specifications.
Backpatch to release 11.
This fixes bug #15446, reported by Dmitry Molotkov
Tom Lane [Thu, 10 Jan 2019 19:20:28 +0000 (14:20 -0500)]
Add .gitignore entry for a derived file created by "make distprep".
I chanced to notice that "make distprep" leaves a state of the
tree that git complains about. It's been like this for awhile,
but given the lack of complaints it probably doesn't need
back-patching.
Tom Lane [Thu, 10 Jan 2019 19:07:01 +0000 (14:07 -0500)]
Fix C++ compile failures in headers.
Avoid using "typeid" as a parameter name in header files, since that
is a C++ keyword. These cases were introduced recently, in 04fe805a1
and 586b98fdf.
Since I'm an incurable neatnik, also rename these parameters in the
underlying function definitions. That's not really necessary per
project rules, but I don't like function declarations that don't
quite agree with the underlying definitions.
Alvaro Herrera [Thu, 10 Jan 2019 17:54:31 +0000 (14:54 -0300)]
Move inheritance expansion code into its own file
This commit moves expand_inherited_tables and underlings from
optimizer/prep/prepunionc.c to optimizer/utils/inherit.c.
Also, all of the AppendRelInfo-based expression manipulation routines
are moved to optimizer/utils/appendinfo.c.
No functional code changes. One exception is the introduction of
make_append_rel_info, but that's still just moving around code.
Also, stop including <limits.h> in prepunion.c, which no longer needs
it since 3fc6e2d7f5b6. I (Álvaro) noticed this because Amit was copying
that to inherit.c, which likewise doesn't need it.
Author: Amit Langote
Discussion: https://postgr.es/m/3be67028-a00a-502c-199a-da00eec8fb6e@lab.ntt.co.jp
Alvaro Herrera [Thu, 10 Jan 2019 16:42:20 +0000 (13:42 -0300)]
pgbench: add \cset and \gset commands
These commands allow assignment of values produced by queries to pgbench
variables, where they can be used by further commands. \gset terminates
a command sequence (just like a bare semicolon); \cset separates
multiple queries in a compound command, like an escaped semicolon (\;).
A prefix can be provided to the \-command and is prepended to the name
of each output column to produce the final variable name.
This feature allows pgbench scripts to react meaningfully to the actual
database contents, allowing more powerful benchmarks to be written.
Authors: Fabien Coelho, Álvaro Herrera Reviewed-by: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> Reviewed-by: Stephen Frost <sfrost@snowman.net> Reviewed-by: Pavel Stehule <pavel.stehule@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Tatsuo Ishii <ishii@sraoss.co.jp> Reviewed-by: Rafia Sabih <rafia.sabih@enterprisedb.com>
Discussion: https://postgr.es/m/alpine.DEB.2.20.1607091005330.3412@sto
Michael Paquier [Thu, 10 Jan 2019 05:10:21 +0000 (14:10 +0900)]
Update unaccent rules with release 34 of CLDR for Latin-ASCII.xml
This has required an update of the python script generating the rules,
as its format has changed in release 29. This release has also added
new punctuation and symbols, and a new set of rules has been generated
to include them. The way to find newest versions of Latin-ASCII gets
also more clearly documented.
Author: Hugh Ranalli, Michael Paquier
Discussion: https://postgr.es/m/15548-cef1b3f8de190d4f@postgresql.org
Tom Lane [Thu, 10 Jan 2019 00:47:38 +0000 (19:47 -0500)]
Use perfect hashing, instead of binary search, for keyword lookup.
We've been speculating for a long time that hash-based keyword lookup
ought to be faster than binary search, but up to now we hadn't found
a suitable tool for generating the hash function. Joerg Sonnenberger
provided the inspiration, and sample code, to show us that rolling our
own generator wasn't a ridiculous idea. Hence, do that.
The method used here requires a lookup table of approximately 4 bytes
per keyword, but that's less than what we saved in the predecessor commit afb0d0712, so it's not a big problem. The time savings is indeed
significant: preliminary testing suggests that the total time for raw
parsing (flex + bison phases) drops by ~20%.
Patch by me, but it owes its existence to Joerg Sonnenberger;
thanks also to John Naylor for review.
Tom Lane [Wed, 9 Jan 2019 20:22:43 +0000 (15:22 -0500)]
Reduce the size of the fmgr_builtin_oid_index[] array.
This index array was originally defined to have 10000 entries (ranging
up to FirstGenbkiObjectId), but we really only need entries up to the
last existing builtin function OID, currently 6121. That saves close
to 8K of never-accessed space in the server executable, at the small
price of one more fetch in fmgr_isbuiltin().
We could reduce the array size still further by renumbering a few of
the highest-numbered builtin functions; but there's a small risk of
breaking clients that have chosen to hardwire those function OIDs,
so it's not clear if it'd be worth the trouble. (We should, however,
discourage future patches from choosing function OIDs above 6K as long
as there's still lots of space below that.)
Tom Lane [Wed, 9 Jan 2019 16:35:14 +0000 (11:35 -0500)]
Update docs & tests to reflect that unassigned OLD/NEW are now NULL.
For a long time, plpgsql has allowed trigger functions to parse
references to OLD and NEW even if the current trigger event type didn't
assign a value to one or the other variable; but actually executing such
a reference would fail. The v11 changes to use "expanded records" for
DTYPE_REC variables changed the behavior so that the unassigned variable
now reads as a null composite value. While this behavioral change was
more or less unintentional, it seems that leaving it like this is better
than adding code and complexity to be bug-compatible with the old way.
The change doesn't break any code that worked before, and it eliminates
a gotcha that often required extra code to work around.
Hence, update the docs to say that these variables are "null" not
"unassigned" when not relevant to the event type. And add a regression
test covering the behavior, so that we'll notice if we ever break it
again.
Tom Lane [Tue, 8 Jan 2019 17:03:53 +0000 (12:03 -0500)]
Doc: update our docs about kernel IPC parameters on *BSD.
runtime.sgml said that you couldn't change SysV IPC parameters on OpenBSD
except by rebuilding the kernel. That's definitely wrong in OpenBSD 6.x,
and excavation in their man pages says it changed in OpenBSD 3.3.
Update NetBSD and OpenBSD sections to recommend adjustment of the SEMMNI
and SEMMNS settings, which are painfully small by default on those
platforms. (The discussion thread contemplated recommending that
people select POSIX semaphores instead, but the performance consequences
of that aren't really clear, so I'll refrain.)
Remove pointless discussion of SEMMNU and SEMMAP from the FreeBSD
section. Minor other wordsmithing.
Peter Eisentraut [Sat, 29 Dec 2018 12:21:57 +0000 (13:21 +0100)]
initdb: Use atexit()
Replace exit_nicely() calls with standard exit() and register the
cleanup actions using atexit(). The coding pattern used here mirrors
existing use in pg_basebackup.c.
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com> Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/ec4135ba-84e9-28bf-b584-0e78d47448d5@2ndquadrant.com/
Tom Lane [Sun, 6 Jan 2019 22:02:57 +0000 (17:02 -0500)]
Replace the data structure used for keyword lookup.
Previously, ScanKeywordLookup was passed an array of string pointers.
This had some performance deficiencies: the strings themselves might
be scattered all over the place depending on the compiler (and some
quick checking shows that at least with gcc-on-Linux, they indeed
weren't reliably close together). That led to very cache-unfriendly
behavior as the binary search touched strings in many different pages.
Also, depending on the platform, the string pointers might need to
be adjusted at program start, so that they couldn't be simple constant
data. And the ScanKeyword struct had been designed with an eye to
32-bit machines originally; on 64-bit it requires 16 bytes per
keyword, making it even more cache-unfriendly.
Redesign so that the keyword strings themselves are allocated
consecutively (as part of one big char-string constant), thereby
eliminating the touch-lots-of-unrelated-pages syndrome. And get
rid of the ScanKeyword array in favor of three separate arrays:
uint16 offsets into the keyword array, uint16 token codes, and
uint8 keyword categories. That reduces the overhead per keyword
to 5 bytes instead of 16 (even less in programs that only need
one of the token codes and categories); moreover, the binary search
only touches the offsets array, further reducing its cache footprint.
This also lets us put the token codes somewhere else than the
keyword strings are, which avoids some unpleasant build dependencies.
While we're at it, wrap the data used by ScanKeywordLookup into
a struct that can be treated as an opaque type by most callers.
That doesn't change things much right now, but it will make it
less painful to switch to a hash-based lookup method, as is being
discussed in the mailing list thread.
Most of the change here is associated with adding a generator
script that can build the new data structure from the same
list-of-PG_KEYWORD header representation we used before.
The PG_KEYWORD lists that plpgsql and ecpg used to embed in
their scanner .c files have to be moved into headers, and the
Makefiles have to be taught to invoke the generator script.
This work is also necessary if we're to consider hash-based lookup,
since the generator script is what would be responsible for
constructing a hash table.
Aside from saving a few kilobytes in each program that includes
the keyword table, this seems to speed up raw parsing (flex+bison)
by a few percent. So it's worth doing even as it stands, though
we think we can gain even more with a follow-on patch to switch
to hash-based lookup.
Tom Lane [Sat, 5 Jan 2019 00:12:22 +0000 (19:12 -0500)]
Fix program build rule in src/bin/scripts/Makefile.
Commit 69ae9dcb4 added a globally-visible "%: %.o" rule, but we failed
to notice that src/bin/scripts/Makefile already had such a rule.
Apparently, the later occurrence of the same rule wins in nearly all
versions of gmake ... but not in the one used by buildfarm member jacana.
jacana is evidently using the global rule, which says to link "$<",
ie just the first dependency. But the scripts makefile needs to
link "$^", ie all the dependencies listed for the target.
There is, fortunately, no good reason not to use "$^" in the global
version of the rule, so we can just do that and get rid of the local
version.
Alvaro Herrera [Fri, 4 Jan 2019 17:51:17 +0000 (14:51 -0300)]
Don't create relfilenode for relations without storage
Some relation kinds had relfilenode set to some non-zero value, but
apparently the actual files did not really exist because creation was
prevented elsewhere. Get rid of the phony pg_class.relfilenode values.
Catversion bumped, but only because the sanity_test check will fail if
run in a system initdb'd with the previous version.
Reviewed-by: Kyotaro HORIGUCHI, Michael Paquier
Discussion: https://postgr.es/m/20181206215552.fm2ypuxq6nhpwjuc@alvherre.pgsql
Tom Lane [Fri, 4 Jan 2019 17:16:19 +0000 (12:16 -0500)]
Support plpgsql variable names that conflict with unreserved SQL keywords.
A variable name matching a statement-introducing keyword, such as
"comment" or "update", caused parse failures if one tried to write
a statement using that keyword. Commit bb1b8f69 already addressed
this scenario for the case of variable names matching unreserved
plpgsql keywords, but we didn't think about unreserved core-grammar
keywords. The same heuristic (viz, it can't be a variable name
unless the next token is assignment or '[') should work fine for
that case too, and as a bonus the code gets shorter and less
duplicative.
Per bug #15555 from Feike Steenbergen. Since this hasn't been
complained of before, and is easily worked around anyway,
I won't risk a back-patch.
Tom Lane [Fri, 4 Jan 2019 00:47:53 +0000 (19:47 -0500)]
Move the built-in conversions into the initial catalog data.
Instead of running a SQL script to create the standard conversion
functions and pg_conversion entries, put those entries into the
initial data in postgres.bki.
This shaves a few percent off the runtime of initdb, and also allows
accurate comments to be attached to the conversion functions; the
previous script labeled them with machine-generated comments that
were not quite right for multi-purpose conversion functions.
Also, we can get rid of the duplicative Makefile and MSVC perl
implementations of the generation code for that SQL script.
A functional change is that these pg_proc and pg_conversion entries
are now "pinned" by initdb. Leaving them unpinned was perhaps a
good thing back while the conversions feature was under development,
but there seems no valid reason for it now.
Also, the conversion functions are now marked as immutable, where
before they were volatile by virtue of lacking any explicit
specification. That seems like it was just an oversight.
To avoid using magic constants in pg_conversion.dat, extend
genbki.pl to allow encoding names to be converted, much as it
does for language, access method, etc names.
Tom Lane [Thu, 3 Jan 2019 23:38:49 +0000 (18:38 -0500)]
Use symbolic references for pg_language OIDs in the bootstrap data.
This patch teaches genbki.pl to replace pg_language names by OIDs
in much the same way as it already does for pg_am names etc, and
converts pg_proc.dat to use such symbolic references in the prolang
column.
Aside from getting rid of a few more magic numbers in the initial
catalog data, this means that Gen_fmgrtab.pl no longer needs to read
pg_language.dat, since it doesn't have to know the OID of the "internal"
language; now it's just looking for the string "internal".
No need for a catversion bump, since the contents of postgres.bki
don't actually change at all.
Tom Lane [Thu, 3 Jan 2019 22:00:08 +0000 (17:00 -0500)]
Improve ANALYZE's handling of concurrent-update scenarios.
This patch changes the rule for whether or not a tuple seen by ANALYZE
should be included in its sample.
When we last touched this logic, in commit 51e1445f1, we weren't
thinking very hard about tuples being UPDATEd by a long-running
concurrent transaction. In such a case, we might see the pre-image as
either LIVE or DELETE_IN_PROGRESS depending on timing; and we might see
the post-image not at all, or as INSERT_IN_PROGRESS. Since the existing
code will not sample either DELETE_IN_PROGRESS or INSERT_IN_PROGRESS
tuples, this leads to concurrently-updated rows being omitted from the
sample entirely. That's not very helpful, and it's especially the wrong
thing if the concurrent transaction ends up rolling back.
The right thing seems to be to sample DELETE_IN_PROGRESS rows just as if
they were live. This makes the "sample it" and "count it" decisions the
same, which seems good for consistency. It's clearly the right thing
if the concurrent transaction ends up rolling back; in effect, we are
sampling as though IN_PROGRESS transactions haven't happened yet.
Also, this combination of choices ensures maximum robustness against
the different combinations of whether and in which state we might see the
pre- and post-images of an update.
It's slightly annoying that we end up recording immediately-out-of-date
stats in the case where the transaction does commit, but on the other
hand the stats are fine for columns that didn't change in the update.
And the alternative of sampling INSERT_IN_PROGRESS rows instead seems
like a bad idea, because then the sampling would be inconsistent with
the way rows are counted for the stats report.
Per report from Mark Chambers; thanks to Jeff Janes for diagnosing
what was happening. Back-patch to all supported versions.
Tom Lane [Wed, 2 Jan 2019 21:33:48 +0000 (16:33 -0500)]
Don't believe MinMaxExpr is leakproof without checking.
MinMaxExpr invokes the btree comparison function for its input datatype,
so it's only leakproof if that function is. Many such functions are
indeed leakproof, but others are not, and we should not just assume that
they are. Hence, adjust contain_leaked_vars to verify the leakproofness
of the referenced function explicitly.
I didn't add a regression test because it would need to depend on
some particular comparison function being leaky, and that's a moving
target, per discussion.
This has been wrong all along, so back-patch to supported branches.
Tom Lane [Wed, 2 Jan 2019 18:57:42 +0000 (13:57 -0500)]
Ensure link commands list *.o files before LDFLAGS.
It's important for link commands to list *.o input files before -l
switches for libraries, as library code may not get pulled into the link
unless referenced by an earlier command-line entry. This is certainly
necessary for static libraries (.a style). Apparently on some platforms
it is also necessary for shared libraries, as reported by Donald Dong.
We often put -l switches for within-tree libraries into LDFLAGS, meaning
that link commands that list *.o files after LDFLAGS are hazardous.
Most of our link commands got this right, but a few did not. In
particular, places that relied on gmake's default implicit link rule
failed, because that puts LDFLAGS first. Fix that by overriding the
built-in rule with our own. The implicit link rules in
src/makefiles/Makefile.* for single-.o-file shared libraries mostly
got this wrong too, so fix them. I also changed the link rules for the
backend and a couple of other places for consistency, even though they
are not (currently) at risk because they aren't adding any -l switches
to LDFLAGS.
Arguably, the real problem here is that we're abusing LDFLAGS by
putting -l switches in it and we should stop doing that. But changing
that would be quite invasive, so I'm not eager to do so.
Perhaps this is a candidate for back-patching, but so far it seems
that problems can only be exhibited in test code we don't normally
build, and at least some of the problems are new in HEAD anyway.
So I'll refrain for now.
Michael Paquier [Tue, 1 Jan 2019 11:05:51 +0000 (20:05 +0900)]
Remove configure switch --disable-strong-random
This removes a portion of infrastructure introduced by fe0a0b5 to allow
compilation of Postgres in environments where no strong random source is
available, meaning that there is no linking to OpenSSL and no
/dev/urandom (Windows having its own CryptoAPI). No systems shipped
this century lack /dev/urandom, and the buildfarm is actually not
testing this switch at all, so just remove it. This simplifies
particularly some backend code which included a fallback implementation
using shared memory, and removes a set of alternate regression output
files from pgcrypto.
Author: Michael Paquier Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/20181230063219.GG608@paquier.xyz
Michael Paquier [Tue, 1 Jan 2019 01:39:19 +0000 (10:39 +0900)]
Fix generation of padding message before encrypting Elgamal in pgcrypto
fe0a0b5, which has added a stronger random source in Postgres, has
introduced a thinko when creating a padding message which gets encrypted
for Elgamal. The padding message cannot have zeros, which are replaced
by random bytes. However if pg_strong_random() failed, the message
would finish by being considered in correct shape for encryption with
zeros.
Author: Tom Lane Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/20186.1546188423@sss.pgh.pa.us
Backpatch-through: 10
Michael Paquier [Mon, 31 Dec 2018 23:53:02 +0000 (08:53 +0900)]
Improve comments and logs in do_pg_stop/start_backup
The function name pg_stop_backup() has been included for ages in some
log messages when stopping the backup, which is confusing for base
backups taken with the replication protocol because this function is
never called. Some other comments and messages in this area are
improved while on it.
The new wording is based on input and suggestions from several people,
all listed below.
Author: Michael Paquier Reviewed-by: Peter Eisentraut, Álvaro Herrera, Tom Lane
Discussion: https://postgr.es/m/20181221040510.GA12599@paquier.xyz
Noah Misch [Mon, 31 Dec 2018 21:54:38 +0000 (13:54 -0800)]
Process EXTRA_INSTALL serially, during the first temp-install.
This closes a race condition in "make -j check-world"; the symptom was
EEXIST errors. Back-patch to v10, before which parallel check-world had
worse problems.
Detect it the way pg_ctl's wait_for_postmaster() does. When pg_regress
spawned a postmaster that failed startup, we were detecting that only
with "pg_regress: postmaster did not respond within 60 seconds".
Back-patch to 9.4 (all supported versions).
Tom Lane [Mon, 31 Dec 2018 21:38:11 +0000 (16:38 -0500)]
Update leakproofness markings on some btree comparison functions.
Mark pg_lsn and oidvector comparison functions as leakproof. Per
discussion, these clearly are leakproof so we might as well mark them so.
On the other hand, remove leakproof markings from name comparison
functions other than equal/not-equal. Now that these depend on
varstr_cmp, they can't be considered leakproof if text comparison isn't.
(This was my error in commit 586b98fdf.)
While at it, add some opr_sanity queries to catch cases where related
functions do not have the same volatility and leakproof markings.
This would clearly be bogus for commutator or negator pairs. In the
domain of btree comparison functions, we do have some exceptions,
because text equality is leakproof but inequality comparisons are not.
That's odd on first glance but is reasonable (for now anyway) given
the much greater complexity of the inequality code paths.
Alvaro Herrera [Mon, 31 Dec 2018 17:40:33 +0000 (14:40 -0300)]
Remove some useless code
In commit 8b08f7d4820f I added member relationId to IndexStmt struct.
I'm now not sure why; DefineIndex doesn't need it, since the relation
OID is passed as a separate argument anyway. Remove it.
Also remove a redundant assignment to the relationId argument (it wasn't
redundant when added by commit e093dcdd285, but should have been removed
in commit 5f173040e3), and use relationId instead of stmt->relation when
locking the relation in the second phase of CREATE INDEX CONCURRENTLY,
which is not only confusing but it means we resolve the name twice for
no reason.
While rearranging code in tidpath.c, I overlooked the fact that we ought
to check restriction_is_securely_promotable when trying to use a join
clause as a TID qual. Since tideq itself is leakproof, this wouldn't
really allow any interesting leak AFAICT, but it still seems like we
had better check it.
For consistency with the corresponding logic in indxpath.c, also
check rinfo->pseudoconstant. I'm not sure right now that it's
possible for that to be set in a join clause, but if it were,
a match couldn't be made anyway.
Tom Lane [Sun, 30 Dec 2018 20:40:04 +0000 (15:40 -0500)]
Add a hash opclass for type "tid".
Up to now we've not worried much about joins where the join key is a
relation's CTID column, reasoning that storing a table's CTIDs in some
other table would be pretty useless. However, there are use-cases for
this sort of query involving self-joins, so that argument doesn't really
hold water.
With larger relations, a merge or hash join is desirable. We had a btree
opclass for type "tid", allowing merge joins on CTID, but no hash opclass
so that hash joins weren't possible. Add the missing infrastructure.
This also potentially enables hash aggregation on "tid", though the
use-cases for that aren't too clear.
Tom Lane [Sun, 30 Dec 2018 20:24:28 +0000 (15:24 -0500)]
Support parameterized TidPaths.
Up to now we've not worried much about joins where the join key is a
relation's CTID column, reasoning that storing a table's CTIDs in some
other table would be pretty useless. However, there are use-cases for
this sort of query involving self-joins, so that argument doesn't really
hold water.
This patch allows generating plans for joins on CTID that use a nestloop
with inner TidScan, similar to what we might do with an index on the join
column. This is the most efficient way to join when the outer side of
the nestloop is expected to yield relatively few rows.
This change requires upgrading tidpath.c and the generated TidPaths
to work with RestrictInfos instead of bare qual clauses, but that's
long-postponed technical debt anyway.
Tom Lane [Sun, 30 Dec 2018 18:42:04 +0000 (13:42 -0500)]
Teach eval_const_expressions to constant-fold LEAST/GREATEST expressions.
Doing this requires an assumption that the invoked btree comparison
function is immutable. We could check that explicitly, but in other
places such as contain_mutable_functions we just assume that it's true,
so we may as well do likewise here. (If the comparison function's
behavior isn't immutable, the sort order in indexes built with it would
be unstable, so it seems certainly wrong for it not to be so.)
Michael Paquier [Sun, 30 Dec 2018 05:35:15 +0000 (14:35 +0900)]
Trigger stmt_beg and stmt_end for top-level statement blocks of PL/pgSQL
PL/pgSQL provides a set of callbacks which can be used for extra
instrumentation of functions written in this language called at function
setup, begin and end, as well as statement begin and end. When calling
a routine, a trigger, or an event trigger, statement callbacks are not
getting called for the top-level statement block leading to an
inconsistent handling compared to the other statements. This
inconsistency can potentially complicate extensions doing
instrumentation work on top of PL/pgSQL, so this commit makes sure that
all statement blocks, including the top-level one, go through the
correct corresponding callbacks.
Author: Pavel Stehule Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/CAFj8pRArEANsaUjo5in9_iQt0vKf9ecwDAmsdN_EBwL13ps12A@mail.gmail.com
Tom Lane [Sat, 29 Dec 2018 22:56:06 +0000 (17:56 -0500)]
Use pg_strong_random() to select each server process's random seed.
Previously we just set the seed based on process ID and start timestamp.
Both those values are directly available within the session, and can
be found out or guessed by other users too, making the session's series
of random(3) values fairly predictable. Up to now, our backend-internal
uses of random(3) haven't seemed security-critical, but commit 88bdbd3f7
added one that potentially is: when using log_statement_sample_rate, a
user might be able to predict which of his SQL statements will get logged.
To improve this situation, upgrade the per-process seed initialization
method to use pg_strong_random() if available, greatly reducing the
predictability of the initial seed value. This adds a few tens of
microseconds to process start time, but since backend startup time is
at least a couple of milliseconds, that seems an acceptable price.
This means that pg_strong_random() needs to be able to run without
reliance on any backend infrastructure, since it will be invoked
before any of that is up. It was safe for that already, but adjust
comments and #include commands to make it clearer.
Tom Lane [Sat, 29 Dec 2018 22:33:27 +0000 (17:33 -0500)]
Use a separate random seed for SQL random()/setseed() functions.
Previously, the SQL random() function depended on libc's random(3),
and setseed() invoked srandom(3). This results in interference between
these functions and backend-internal uses of random(3). We'd never paid
too much mind to that, but in the wake of commit 88bdbd3f7 which added
log_statement_sample_rate, the interference arguably has a security
consequence: if log_statement_sample_rate is active then an unprivileged
user could probably control which if any of his SQL commands get logged,
by issuing setseed() at the right times. That seems bad.
To fix this reliably, we need random() and setseed() to use their own
private random state variable. Standard random(3) isn't amenable to such
usage, so let's switch to pg_erand48(). It's hard to say whether that's
more or less "random" than any particular platform's version of random(3),
but it does have a wider seed value and a longer period than are required
by POSIX, so we can hope that this isn't a big downgrade. Also, we should
now have uniform behavior of random() across platforms, which is worth
something.
While at it, upgrade the per-process seed initialization method to use
pg_strong_random() if available, greatly reducing the predictability
of the initial seed value. (I'll separately do something similar for
the internal uses of random().)
In addition to forestalling the possible security problem, this has a
benefit in the other direction, which is that we can now document
setseed() as guaranteeing a reproducible sequence of random() values.
Previously, because of the possibility of internal calls of random(3),
we could not promise any such thing.
Michael Paquier [Fri, 28 Dec 2018 23:24:11 +0000 (08:24 +0900)]
Improve description of DEFAULT_XLOG_SEG_SIZE in pg_config.h
This was incorrectly referring to --walsegsize, and its description is
rewritten in a clearer way.
Author: Ian Barwick, Tom Lane Reviewed-by: Álvaro Herrera, Michael Paquier
Discussion: https://postgr.es/m/08534fc6-119a-c498-254e-d5acc4e6bf85@2ndquadrant.com
Tom Lane [Fri, 28 Dec 2018 20:06:48 +0000 (15:06 -0500)]
Marginal performance hacking in erand48.c.
Get rid of the multiplier and addend variables in favor of hard-wired
constants. Do the multiply-and-add using uint64 arithmetic, rather
than manually combining several narrower multiplications and additions.
Make _dorand48 return the full-width new random value, and have its
callers use that directly (after suitable masking) rather than
reconstructing what they need from the unsigned short[] representation.
On my machine, this is good for a nearly factor-of-2 speedup of
pg_erand48(), probably mostly from needing just one call of ldexp()
rather than three. The wins for the other functions are smaller
but measurable. While none of the existing call sites are really
performance-critical, a cycle saved is a cycle earned; and besides
the machine code is smaller this way (at least on x86_64).
Patch by me, but the original idea to optimize this by switching
to int64 arithmetic is from Fabien Coelho.
projects / postgresql / log