users/rhaas/postgres.git
5 years agoAvoid holding a directory FD open across pg_ls_dir_files() calls.
Tom Lane [Wed, 11 Mar 2020 19:27:59 +0000 (15:27 -0400)]
Avoid holding a directory FD open across pg_ls_dir_files() calls.

This coding technique is undesirable because (a) it leaks the FD for
the rest of the transaction if the SRF is not run to completion, and
(b) allocated FDs are a scarce resource, but multiple interleaved
uses of the relevant functions could eat many such FDs.

In v11 and later, a query such as "SELECT pg_ls_waldir() LIMIT 1"
yields a warning about the leaked FD, and the only reason there's
no warning in earlier branches is that fd.c didn't whine about such
leaks before commit 9cb7db3f0.  Even disregarding the warning, it
wouldn't be too hard to run a backend out of FDs with careless use
of these SQL functions.

Hence, rewrite the function so that it reads the directory within
a single call, returning the results as a tuplestore rather than
via value-per-call mode.

There are half a dozen other built-in SRFs with similar problems,
but let's fix this one to start with, just to see if the buildfarm
finds anything wrong with the code.

In passing, fix bogus error report for stat() failure: it was
whining about the directory when it should be fingering the
individual file.  Doubtless a copy-and-paste error.

Back-patch to v10 where this function was added.

Justin Pryzby, with cosmetic tweaks and test cases by me

Discussion: https://postgr.es/m/20200308173103.GC1357@telsasoft.com

5 years agoRefactor ps_status.c API
Peter Eisentraut [Wed, 11 Mar 2020 15:36:40 +0000 (16:36 +0100)]
Refactor ps_status.c API

The init_ps_display() arguments were mostly lies by now, so to match
typical usage, just use one argument and let the caller assemble it
from multiple sources if necessary.  The only user of the additional
arguments is BackendInitialize(), which was already doing string
assembly on the caller side anyway.

Remove the second argument of set_ps_display() ("force") and just
handle that in init_ps_display() internally.

BackendInitialize() also used to set the initial status as
"authentication", but that was very far from where authentication
actually happened.  So now it's set to "initializing" and then
"authentication" just before the actual call to
ClientAuthentication().

Reviewed-by: Julien Rouhaud <rjuju123@gmail.com>
Reviewed-by: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/c65e5196-4f04-4ead-9353-6088c19615a3@2ndquadrant.com

5 years agoAvoid duplicates in ALTER ... DEPENDS ON EXTENSION
Alvaro Herrera [Wed, 11 Mar 2020 14:04:59 +0000 (11:04 -0300)]
Avoid duplicates in ALTER ... DEPENDS ON EXTENSION

If the command is attempted for an extension that the object already
depends on, silently do nothing.

In particular, this means that if a database containing multiple such
entries is dumped, the restore will silently do the right thing and
record just the first one.  (At least, in a world where pg_dump does
dump such entries -- which it doesn't currently, but it will.)

Backpatch to 9.6, where this kind of dependency was introduced.

Reviewed-by: Ibrar Ahmed, Tom Lane (offlist)
Discussion: https://postgr.es/m/20200217225333.GA30974@alvherre.pgsql

5 years agoClean up order in miscinit.c a bit
Peter Eisentraut [Wed, 11 Mar 2020 12:51:55 +0000 (13:51 +0100)]
Clean up order in miscinit.c a bit

The code around InitPostmasterChild() from commit 31c453165b5 somehow
ended up in the middle of a block of code related to "User ID state".
Move it into its own block instead.

5 years agoRemove HAVE_WORKING_LINK
Peter Eisentraut [Wed, 11 Mar 2020 09:58:02 +0000 (10:58 +0100)]
Remove HAVE_WORKING_LINK

Previously, hard links were not used on Windows and Cygwin, but they
support them just fine in currently supported OS versions, so we can
use them there as well.

Since all supported platforms now support hard links, we can remove
the alternative code paths.

Rename durable_link_or_rename() to durable_rename_excl() to make the
purpose more clear without referencing the implementation details.

Discussion: https://www.postgresql.org/message-id/flat/72fff73f-dc9c-4ef4-83e8-d2e60c98df48%402ndquadrant.com

5 years agoImprove checking of child pages in contrib/amcheck.
Alexander Korotkov [Wed, 11 Mar 2020 09:00:31 +0000 (12:00 +0300)]
Improve checking of child pages in contrib/amcheck.

This commit eliminates lossiness in check for missing parent downlinks in
B-tree.  Instead of collecting lossy bitmap, we check for missing downlinks
while visiting child pages referenced by downlinks of target level.  We
traverse from previous child page to the subsequent child page by right links.
Intermediate pages are candidates to have lost parent downlinks.

Also this commit introduces matching of child high key to the pivot key of
it's parent.

Discussion: https://postgr.es/m/CAPpHfduoF-c4RhOyOm%3D4-Y367%2B8txq9Q6iM_ty0OYc8si1Abww%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Peter Geoghegan
5 years agoRemove stray parenthesis in nbtree.h.
Peter Geoghegan [Wed, 11 Mar 2020 01:03:56 +0000 (18:03 -0700)]
Remove stray parenthesis in nbtree.h.

Oversight in commit 0d861bbb702.

5 years agonbtree: Move fastpath NULL descent stack assertion.
Peter Geoghegan [Wed, 11 Mar 2020 00:25:47 +0000 (17:25 -0700)]
nbtree: Move fastpath NULL descent stack assertion.

Commit 074251db added an assertion that verified the fastpath/rightmost
page insert optimization's assumption about free space: There should
always be enough free space on the page to insert the new item without
splitting the page.  Otherwise, we end up using the "concurrent root
page split" phony/fake stack path in _bt_insert_parent().  This does not
lead to incorrect behavior, but it is likely to be far slower than
simply using the regular _bt_search() path.  The assertion catches
serious performance bugs that would probably take a long time to detect
any other way.

It seems much more natural to make this assertion just before the point
that we generate a fake/phony descent stack.  Move the assert there.
This also makes _bt_insertonpg() a bit more readable.

5 years agoMarginal comments and docs cleanup.
Tom Lane [Tue, 10 Mar 2020 21:34:01 +0000 (17:34 -0400)]
Marginal comments and docs cleanup.

Fix up some imprecise comments and poor markup from ba79cb5dc.  Also try
to convert the documentation of log_min_duration_sample and friends into
passable English.

5 years agonbtree: Demote minus infinity "can't happen" error.
Peter Geoghegan [Tue, 10 Mar 2020 21:15:41 +0000 (14:15 -0700)]
nbtree: Demote minus infinity "can't happen" error.

Only a very basic logic bug in a _bt_insertonpg() caller could lead to a
violation of this invariant.  Besides, any newitemoff used for an
internal page is sanitized using other "can't happen" errors in
_bt_getstackbuf() or its callers, before _bt_insertonpg() even gets
called.

Also, move the error/assertion from the insert-without-split path of
_bt_insertonpg() to the top of the same function.  There is no reason
why this invariant only applies to insertions that happen to not result
in a page split; cover every insertion.  The assertion naturally belongs
next to the existing generic assertions that document relatively
high-level invariants for the item being inserted.

5 years agoEnsure that CREATE TABLE LIKE copies any NO INHERIT constraint property.
Tom Lane [Tue, 10 Mar 2020 18:54:00 +0000 (14:54 -0400)]
Ensure that CREATE TABLE LIKE copies any NO INHERIT constraint property.

Since the documentation about LIKE doesn't say that a copied constraint
has properties different from the original, it seems that ignoring
a NO INHERIT property doesn't meet the principle of least surprise.
So make it copy that.

(Note, however, that we still don't copy a NOT VALID property;
CREATE TABLE offers no way to do that, plus it seems pointless.)

Arguably this is a bug fix; but no back-patch, as it seems barely
possible somebody is depending on the current behavior.

Ildar Musin and Chris Travers; reviewed by Amit Langote and myself

Discussion: https://postgr.es/m/CAONYFtMC6C+3AWCVp7Yd8H87Zn0GxG1_iQG6_bQKbaqYZY0=-g@mail.gmail.com

5 years agoAvoid portability problem introduced in 0a42a2e9c.
Tom Lane [Tue, 10 Mar 2020 16:46:07 +0000 (12:46 -0400)]
Avoid portability problem introduced in 0a42a2e9c.

Apparently not all shells handle nested quotes quite the same.
But we don't need the inner double quotes in this case, so just
drop them.

Discussion: https://postgr.es/m/50ecb345-4dc5-1f7d-64ca-7018195fcc8d@2ndquadrant.com

5 years agoPreserve integer and float values accurately in (de)serialize_deflist.
Tom Lane [Tue, 10 Mar 2020 16:29:59 +0000 (12:29 -0400)]
Preserve integer and float values accurately in (de)serialize_deflist.

Previously, this code just smashed all types of DefElem values to
strings, cavalierly reasoning that nobody would care.  But in point of
fact, most of the defGetFoo functions do distinguish among different
input syntaxes; for instance defGetBoolean will accept 1 as an integer
but not "1" as a string.  This led to CREATE/ALTER TEXT SEARCH
DICTIONARY accepting 0 and 1 as values for boolean dictionary
properties, only to have the dictionary fail at runtime.

We can upgrade this behavior by teaching serialize_deflist that it
does not need to quote T_Integer or T_Float nodes' values on output,
and then teaching deserialize_deflist to restore unquoted integer or
float values as the appropriate node type.  This should not break
anything using pg_ts_dict.dictinitoption, since that field is just
defined as being something valid to include in CREATE TEXT SEARCH
DICTIONARY.

deserialize_deflist is also used to parse the options arguments
for the ts_headline family of functions, but so far as I can see
this won't cause any problems there either: the only consumer of
that output is prsd_headline which always uses defGetString.
(Really that's a bad idea, but I won't risk changing it here.)

This is surely a bug fix, but given the lack of field complaints
I don't think it's necessary to back-patch.

Discussion: https://postgr.es/m/CAMkU=1xRcs_BUPzR0+V3WndaCAv0E_m3h6aUEJ8NF-sY1nnHsw@mail.gmail.com

5 years agoSplit out CreateCast into src/backend/catalog/pg_cast.c
Alvaro Herrera [Tue, 10 Mar 2020 14:28:23 +0000 (11:28 -0300)]
Split out CreateCast into src/backend/catalog/pg_cast.c

This catalog-handling code was previously together with the rest of
CastCreate() in src/backend/commands/functioncmds.c.  A future patch
will need a way to add casts internally, so this will be useful to have
separate.

Also, move the nearby get_cast_oid() function from functioncmds.c to
lsyscache.c, which seems a more natural place for it.

Author: Paul Jungwirth, minor edits by Álvaro
Discussion: https://postgr.es/m/20200309210003.GA19992@alvherre.pgsql

5 years agoRemove win32ver.rc from version_stamp.pl
Peter Eisentraut [Tue, 10 Mar 2020 10:20:38 +0000 (11:20 +0100)]
Remove win32ver.rc from version_stamp.pl

This removes another relic from the old nmake-based Windows build.
version_stamp.pl put version number information into win32ver.rc.  But
win32ver.rc already gets other version number information from the
preprocessor at build time, so it would make more sense if all version
number information would be handled in the same way and we don't have
two places that do it.

What we need for this is having the major version number and the minor
version number as separate integer symbols.  Both configure and
Solution.pm already have that logic, because they compute
PG_VERSION_NUM.  So we just keep all the logic there now.  Put the
minor version number into a new symbol PG_MINORVERSION_NUM.  Also, add
a symbol PG_MAJORVERSION_NUM, which is a number, alongside the
existing PG_MAJORVERSION, which is a string.

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://www.postgresql.org/message-id/flat/1ee46ac4-a9b2-4531-bf54-5ec2e374634d@2ndquadrant.com

5 years agoRemove utils/acl.h from catalog/objectaddress.h
Peter Eisentraut [Tue, 10 Mar 2020 09:22:52 +0000 (10:22 +0100)]
Remove utils/acl.h from catalog/objectaddress.h

The need for this was removed by
8b9e9644dc6a9bd4b7a97950e6212f63880cf18b.

A number of files now need to include utils/acl.h or
parser/parse_node.h explicitly where they previously got it indirectly
somehow.

Since parser/parse_node.h already includes nodes/parsenodes.h, the
latter is then removed where the former was added.  Also, remove
nodes/pg_list.h from objectaddress.h, since that's included via
nodes/parsenodes.h.

Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Alvaro Herrera <alvherre@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/7601e258-26b2-8481-36d0-dc9dca6f28f1%402ndquadrant.com

5 years agoSupport adding partitioned tables to publication
Peter Eisentraut [Tue, 10 Mar 2020 07:42:59 +0000 (08:42 +0100)]
Support adding partitioned tables to publication

When a partitioned table is added to a publication, changes of all of
its partitions (current or future) are published via that publication.

This change only affects which tables a publication considers as its
members.  The receiving side still sees the data coming from the
individual leaf partitions.  So existing restrictions that partition
hierarchies can only be replicated one-to-one are not changed by this.

Author: Amit Langote <amitlangote09@gmail.com>
Reviewed-by: Rafia Sabih <rafia.pghackers@gmail.com>
Reviewed-by: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Discussion: https://www.postgresql.org/message-id/flat/CA+HiwqH=Y85vRK3mOdjEkqFK+E=ST=eQiHdpj43L=_eJMOOznQ@mail.gmail.com

5 years agoPrevent reindex of invalid indexes on TOAST tables
Michael Paquier [Tue, 10 Mar 2020 06:38:17 +0000 (15:38 +0900)]
Prevent reindex of invalid indexes on TOAST tables

Such indexes can only be duplicated leftovers of a previously failed
REINDEX CONCURRENTLY command, and a valid equivalent is guaranteed to
exist.  As toast indexes can only be dropped if invalid, reindexing
these would lead to useless duplicated indexes that can't be dropped
anymore, except if the parent relation is dropped.

Thanks to Justin Pryzby for reminding that this problem was reported
long ago during the review of the original patch of REINDEX
CONCURRENTLY, but the issue was never addressed.

Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12

5 years agoTidy up XLogSource code in xlog.c.
Fujii Masao [Tue, 10 Mar 2020 00:41:44 +0000 (09:41 +0900)]
Tidy up XLogSource code in xlog.c.

This commit replaces 0 used as an initial value of XLogSource variable,
with XLOG_FROM_ANY. Also this commit changes those variable so that
XLogSource instead of int is used as the type for them. These changes
are for code readability and debugger-friendliness.

Author: Kyotaro Horiguchi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/20200227.124830.2197604521555566121.horikyota.ntt@gmail.com

5 years agoFix pg_dump/pg_restore to restore event triggers later.
Tom Lane [Mon, 9 Mar 2020 18:58:11 +0000 (14:58 -0400)]
Fix pg_dump/pg_restore to restore event triggers later.

Previously, event triggers were restored just after regular triggers
(and FK constraints, which are basically triggers).  This is risky
since an event trigger, once installed, could interfere with subsequent
restore commands.  Worse, because event triggers don't have any
particular dependencies on any post-data objects, a parallel restore
would consider them eligible to be restored the moment the post-data
phase starts, allowing them to also interfere with restoration of a
whole bunch of objects that would have been restored before them in
a serial restore.  There's no way to completely remove the risk of a
misguided event trigger breaking the restore, since if nothing else
it could break other event triggers.  But we can certainly push them
to later in the process to minimize the hazard.

To fix, tweak the RestorePass mechanism introduced by commit 3eb9a5e7c
so that event triggers are handled as part of the post-ACL processing
pass (renaming the "REFRESH" pass to "POST_ACL" to reflect its more
general use).  This will cause them to restore after everything except
matview refreshes, which seems OK since matview refreshes really ought
to run in the post-restore state of the database.  In a parallel
restore, event triggers and matview refreshes might be intermixed,
but that seems all right as well.

Also update the code and comments in pg_dump_sort.c so that its idea
of how things are sorted agrees with what actually happens due to
the RestorePass mechanism.  This is mostly cosmetic: it'll affect the
order of objects in a dump's TOC, but not the actual restore order.
But not changing that would be quite confusing to somebody reading
the code.

Back-patch to all supported branches.

Fabrízio de Royes Mello, tweaked a bit by me

Discussion: https://postgr.es/m/CAFcNs+ow1hmFox8P--3GSdtwz-S3Binb6ZmoP6Vk+Xg=K6eZNA@mail.gmail.com

5 years agoIntroduce LogicalTapeSetExtend().
Jeff Davis [Sat, 7 Mar 2020 01:28:53 +0000 (17:28 -0800)]
Introduce LogicalTapeSetExtend().

Increases the number of tapes in a logical tape set. This will be
important for disk-based hash aggregation, because the maximum number
of tapes is not known ahead of time.

While discussing this change, it was observed to regress the
performance of Sort for at least one test case. The performance
regression was because some versions of GCC switch to an inlined
version of memcpy() in LogicalTapeWrite() after this change. No
performance regression for clang was observed.

Because the regression is due to an arbitrary decision by the
compiler, I decided it shouldn't hold up this change. If it needs to
be fixed, we can find a workaround.

Author: Adam Lee, Jeff Davis
Discussion: https://postgr.es/m/e54bfec11c59689890f277722aaaabd05f78e22c.camel%40j-davis.com

5 years agoFix bug that causes to report waiting in PS display twice, in hot standby.
Fujii Masao [Mon, 9 Mar 2020 15:14:43 +0000 (00:14 +0900)]
Fix bug that causes to report waiting in PS display twice, in hot standby.

Previously "waiting" could appear twice via PS in case of lock conflict
in hot standby mode. Specifically this issue happend when the delay
in WAL application determined by max_standby_archive_delay and
max_standby_streaming_delay had passed but it took more than 500 msec
to cancel all the conflicting transactions. Especially we can observe this
easily by setting those delay parameters to -1.

The cause of this issue was that WaitOnLock() and
ResolveRecoveryConflictWithVirtualXIDs() added "waiting" to
the process title in that case. This commit prevents
ResolveRecoveryConflictWithVirtualXIDs() from reporting waiting
in case of lock conflict, to fix the bug.

Back-patch to all back branches.

Author: Masahiko Sawada
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/CA+fd4k4mXWTwfQLS3RPwGr4xnfAEs1ysFfgYHvmmoUgv6Zxvmg@mail.gmail.com

5 years agoAdd tg_updatedcols to TriggerData
Peter Eisentraut [Mon, 9 Mar 2020 08:22:22 +0000 (09:22 +0100)]
Add tg_updatedcols to TriggerData

This allows a trigger function to determine for an UPDATE trigger
which columns were actually updated.  This allows some optimizations
in generic trigger functions such as lo_manage and
tsvector_update_trigger.

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/11c5f156-67a9-0fb5-8200-2a8018eb2e0c@2ndquadrant.com

5 years agoCode simplification
Peter Eisentraut [Mon, 24 Feb 2020 09:12:10 +0000 (10:12 +0100)]
Code simplification

Initialize TriggerData to 0 for the whole struct together, instead of
each field separately.

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/11c5f156-67a9-0fb5-8200-2a8018eb2e0c@2ndquadrant.com

5 years agoAvoid assertion failure with targeted recovery in standby mode.
Fujii Masao [Mon, 9 Mar 2020 06:31:31 +0000 (15:31 +0900)]
Avoid assertion failure with targeted recovery in standby mode.

At the end of recovery, standby mode is turned off to re-fetch the last
valid record from archive or pg_wal. Previously, if recovery target was
reached and standby mode was turned off while the current WAL source
was stream, recovery could try to retrieve WAL file containing the last
valid record unexpectedly from stream even though not in standby mode.
This caused an assertion failure. That is, the assertion test confirms that
WAL file should not be retrieved from stream if standby mode is not true.

This commit moves back the current WAL source to archive if it's stream
even though not in standby mode, to avoid that assertion failure.

This issue doesn't cause the server to crash when built with assertion
disabled. In this case, the attempt to retrieve WAL file from stream not
in standby mode just fails. And then recovery tries to retrieve WAL file
from archive or pg_wal.

Back-patch to all supported branches.

Author: Kyotaro Horiguchi
Reviewed-by: Fujii Masao
Discussion: https://postgr.es/m/20200227.124830.2197604521555566121.horikyota.ntt@gmail.com

5 years agoMark ssl_passphrase_command as GUC_SUPERUSER_ONLY.
Fujii Masao [Mon, 9 Mar 2020 02:41:31 +0000 (11:41 +0900)]
Mark ssl_passphrase_command as GUC_SUPERUSER_ONLY.

This commit changes the GUC ssl_passphrase_command so that
it's examinable by only superuser and a member of pg_read_all_settings.
Per discussion, we determined to do this because the parameter may
contain a sensitive informtaion like a passphrase itself.

Author: Insung Moon
Reviewed-by: Keisuke Kuroda
Discussion: https://postgr.es/m/CAEMmqBuHVGayc+QkYKgx3gWSdqwTAQGw+0DYn3WhcX-eNa2ntA@mail.gmail.com

5 years agoDoc: fix some description of environment variables with frontend tools
Michael Paquier [Mon, 9 Mar 2020 01:53:22 +0000 (10:53 +0900)]
Doc: fix some description of environment variables with frontend tools

This addresses a couple of issues in the documentation:
- Description of PG_COLOR was missing for some tools (pg_archivecleanup
and pg_test_fsync), while the other descriptions had grammar mistakes.
- pgbench supports more environment variables: PGUSER, PGHOST and
PGPORT.
- vacuumlo, oid2name and pgbench support coloring (HEAD only)

Author: Michael Paquier
Reviewed-by: Fabien Coelho, Daniel Gustafsson, Juan José Santamaría
Flecha
Discussion: https://postgr.es/m/20200304075418.GJ2593@paquier.xyz
Backpatch-through: 12

5 years agoAdd an "absval" parameter to allow contrib/dict_int to ignore signs.
Tom Lane [Sun, 8 Mar 2020 22:35:06 +0000 (18:35 -0400)]
Add an "absval" parameter to allow contrib/dict_int to ignore signs.

Jeff Janes

Discussion: https://postgr.es/m/CAMkU=1xRcs_BUPzR0+V3WndaCAv0E_m3h6aUEJ8NF-sY1nnHsw@mail.gmail.com

5 years agoAdd an explicit test to catch changes in checksumming calculations.
Tom Lane [Sun, 8 Mar 2020 19:09:14 +0000 (15:09 -0400)]
Add an explicit test to catch changes in checksumming calculations.

Seems like a good idea in view of 006517432 and addd034ae.

Michael Paquier, Tom Lane

Discussion: https://postgr.es/m/20200306075230.GA118430@paquier.xyz

5 years agoShow opclass and opfamily related information in psql
Alexander Korotkov [Sun, 8 Mar 2020 10:32:25 +0000 (13:32 +0300)]
Show opclass and opfamily related information in psql

This commit provides psql commands for listing operator classes, operator
families and its contents in psql.  New commands will be useful for exploring
capabilities of both builtin opclasses/opfamilies as well as
opclasses/opfamilies defined in extensions.

Discussion: https://postgr.es/m/1529675324.14193.5.camel%40postgrespro.ru
Author: Sergey Cherkashin, Nikita Glukhov, Alexander Korotkov
Reviewed-by: Michael Paquier, Alvaro Herrera, Arthur Zakirov
Reviewed-by: Kyotaro Horiguchi, Andres Freund
5 years agopageinspect: Fix types used for bt_metap() columns.
Peter Geoghegan [Sun, 8 Mar 2020 00:44:53 +0000 (16:44 -0800)]
pageinspect: Fix types used for bt_metap() columns.

The data types that contrib/pageinspect's bt_metap() function were
declared to return as OUT arguments were wrong in some cases.  For
example, the oldest_xact column (a TransactionId/xid field) was declared
integer/int4 within the pageinspect extension's sql file.  This led to
errors when an oldest_xact value that exceeded 2^31-1 was encountered.
Some of the other columns were defined incorrectly ever since
pageinspect was first introduced, though they were far less likely to
produce problems in practice.

Fix these issues by changing the declaration of bt_metap() to
consistently use data types that can reliably represent all possible
values.  This fixes things on HEAD only.  No backpatch, since it doesn't
seem like there is a safe way to fix the issue without including a new
version of the pageinspect extension (HEAD/Postgres 13 already
introduced a new version of the extension).  Besides, the oldest_xact
issue has been around since the release of Postgres 11, and we haven't
heard any complaints about it before now.

Also, throw an error when we detect a bt_metap() declaration that must
be from an old version of the pageinspect extension by examining the
number of attributes from the tuple descriptor for the return tuples.
It seems better to throw an error in a reliable and obvious way
following a Postgres upgrade, rather than letting bt_metap() fail
unpredictably.  The problem is fundamentally with the CREATE FUNCTION
declared data types themselves, so I see no sensible alternative.

Reported-By: Victor Yegorov
Bug: #16285
Discussion: https://postgr.es/m/16285-df8fc1000ab3d5fc@postgresql.org

5 years agoTab completion: offer parens as appropriate in CREATE/ALTER TEXT SEARCH.
Tom Lane [Sat, 7 Mar 2020 21:58:07 +0000 (16:58 -0500)]
Tab completion: offer parens as appropriate in CREATE/ALTER TEXT SEARCH.

Jeff Janes, Georgios Kokolatos

Discussion: https://postgr.es/m/CAMkU=1wU=vgxnvwy2HswLUVvoawrkrjZYeKXMr3w3p=_NNbGhQ@mail.gmail.com

5 years agoSimplify/speed up assertion cross-check in ginCompressPostingList().
Tom Lane [Sat, 7 Mar 2020 18:31:17 +0000 (13:31 -0500)]
Simplify/speed up assertion cross-check in ginCompressPostingList().

I noticed while testing some other stuff that the CHECK_ENCODING_ROUNDTRIP
logic in ginCompressPostingList could account for over 50% of the runtime
of an INSERT with a GIN index.  While that's not relevant to production
performance, it's still kind of annoying in a debug build.  Replacing
the loop around short memcmp's with one long memcmp works just as well
and is significantly faster, at least on my machine.

5 years agodoc: Remove unused ids
Peter Eisentraut [Sat, 7 Mar 2020 13:04:09 +0000 (14:04 +0100)]
doc: Remove unused ids

Some reference pages contained id attributes on refname elements.
These were apparently copied around from ancient times, but they don't
serve a purpose.  FOP issues minor warnings about them.  So it's
easiest to just remove them.

5 years agodoc: Add information about new SQL part SQL/MDA
Peter Eisentraut [Sat, 7 Mar 2020 09:57:03 +0000 (10:57 +0100)]
doc: Add information about new SQL part SQL/MDA

5 years agoFix typo
Peter Eisentraut [Sat, 7 Mar 2020 07:51:59 +0000 (08:51 +0100)]
Fix typo

5 years agoCreate contrib/bool_plperl to provide a bool transform for PL/Perl[U].
Tom Lane [Fri, 6 Mar 2020 22:11:23 +0000 (17:11 -0500)]
Create contrib/bool_plperl to provide a bool transform for PL/Perl[U].

plperl's default handling of bool arguments or results is not terribly
satisfactory, since Perl doesn't consider the string 'f' to be false.
Ideally we'd just fix that, but the backwards-compatibility hazard
would be substantial.  Instead, build a TRANSFORM module that can
be optionally applied to provide saner semantics.

Perhaps usefully, this is also about the minimum possible skeletal
example of a plperl transform module; so it might be a better starting
point for user-written transform modules than hstore_plperl or
jsonb_plperl.

Ivan Panchenko

Discussion: https://postgr.es/m/1583013317.881182688@f390.i.mail.ru

5 years agoAllow Unicode escapes in any server encoding, not only UTF-8.
Tom Lane [Fri, 6 Mar 2020 19:17:43 +0000 (14:17 -0500)]
Allow Unicode escapes in any server encoding, not only UTF-8.

SQL includes provisions for numeric Unicode escapes in string
literals and identifiers.  Previously we only accepted those
if they represented ASCII characters or the server encoding
was UTF-8, making the conversion to internal form trivial.
This patch adjusts things so that we'll call the appropriate
encoding conversion function in less-trivial cases, allowing
the escape sequence to be accepted so long as it corresponds
to some character available in the server encoding.

This also applies to processing of Unicode escapes in JSONB.
However, the old restriction still applies to client-side
JSON processing, since that hasn't got access to the server's
encoding conversion infrastructure.

This patch includes some lexer infrastructure that simplifies
throwing errors with error cursors pointing into the middle of
a string (or other complex token).  For the moment I only used
it for errors relating to Unicode escapes, but we might later
expand the usage to some other cases.

Patch by me, reviewed by John Naylor.

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

5 years agoAllow ALTER TYPE to change some properties of a base type.
Tom Lane [Fri, 6 Mar 2020 17:19:29 +0000 (12:19 -0500)]
Allow ALTER TYPE to change some properties of a base type.

Specifically, this patch allows ALTER TYPE to:
* Change the default TOAST strategy for a toastable base type;
* Promote a non-toastable type to toastable;
* Add/remove binary I/O functions for a type;
* Add/remove typmod I/O functions for a type;
* Add/remove a custom ANALYZE statistics functions for a type.

The first of these can be done by the type's owner; all the others
require superuser privilege since misuse could cause problems.

The main motivation for this patch is to allow extensions to
upgrade the feature sets of their data types, so the set of
alterable properties is biased towards that use-case.  However
it's also true that changing some other properties would be
a lot harder, as they get baked into physical storage and/or
stored expressions that depend on the type.

Along the way, refactor GenerateTypeDependencies() to make it easier
to call, refactor DefineType's volatility checks so they can be shared
by AlterType, and teach typcache.c that it might have to reload data
from the type's pg_type row, a scenario it never handled before.
Also rearrange alter_type.sgml a bit for clarity (put the
composite-type operations together).

Tomas Vondra and Tom Lane

Discussion: https://postgr.es/m/20200228004440.b23ein4qvmxnlpht@development

5 years agoFix page-level checksum calculation in checksum_impl.h
Michael Paquier [Thu, 5 Mar 2020 23:15:29 +0000 (08:15 +0900)]
Fix page-level checksum calculation in checksum_impl.h

Issue introduced by me, as of 0065174.

Reported-by: David Steele
Discussion: https://postgr.es/m/1cf30561-7dad-dc6e-9fc3-5c456948cfeb@pgmasters.net

5 years agoRemove the "opaque" pseudo-type and associated compatibility hacks.
Tom Lane [Thu, 5 Mar 2020 20:48:56 +0000 (15:48 -0500)]
Remove the "opaque" pseudo-type and associated compatibility hacks.

A long time ago, it was necessary to declare datatype I/O functions,
triggers, and language handler support functions in a very type-unsafe
way involving a single pseudo-type "opaque".  We got rid of those
conventions in 7.3, but there was still support in various places to
automatically convert such functions to the modern declaration style,
to be able to transparently re-load dumps from pre-7.3 servers.
It seems unnecessary to continue to support that anymore, so take out
the hacks; whereupon the "opaque" pseudo-type itself is no longer
needed and can be dropped.

This is part of a group of patches removing various server-side kluges
for transparently upgrading pre-8.0 dump files.  Since we've had few
complaints about dropping pg_dump's support for dumping from pre-8.0
servers (commit 64f3524e2), it seems okay to now remove these kluges.

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

5 years agoRemove ancient hacks to ignore certain opclass names in CREATE INDEX.
Tom Lane [Thu, 5 Mar 2020 20:36:06 +0000 (15:36 -0500)]
Remove ancient hacks to ignore certain opclass names in CREATE INDEX.

Twenty years ago, we removed certain operator classes in favor of
letting indexes over their data types be built with some other
binary-compatible, more standard opclass.  As a hack to allow existing
index definitions to be dumped and reloaded, we made CREATE INDEX ignore
the removed opclass names, so that such indexes would fall back to the
new default opclass for their data types.  This was never intended to
be a long-lived thing; it carries the obvious risk of breaking some
future developer's attempt to re-use those old opclass names.  Since
all of the cases in question are for opclasses that were removed
before PG 8.0, it seems okay to get rid of these hacks now.

This is part of a group of patches removing various server-side kluges
for transparently upgrading pre-8.0 dump files.  Since we've had few
complaints about dropping pg_dump's support for dumping from pre-8.0
servers (commit 64f3524e2), it seems okay to now remove these kluges.

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

5 years agoRemove ancient support for upgrading pre-7.3 foreign key constraints.
Tom Lane [Thu, 5 Mar 2020 20:25:45 +0000 (15:25 -0500)]
Remove ancient support for upgrading pre-7.3 foreign key constraints.

Before 7.3, foreign key constraints had no explicit catalog
representation, so that what pg_dump produced for them was (usually)
a set of three CREATE CONSTRAINT TRIGGER commands.  Commit a2899ebdc
and some follow-on fixes added an ugly hack in CreateTrigger() to
recognize that pattern and reconstruct the foreign key definition.
However, we've never had any test coverage for that code, so that it's
legitimate to wonder if it still works; and having to maintain it in
the face of upcoming trigger-related patches seems rather pointless.
Let's decree that its time has passed, and drop it.

This is part of a group of patches removing various server-side kluges
for transparently upgrading pre-8.0 dump files.  Since we've had few
complaints about dropping pg_dump's support for dumping from pre-8.0
servers (commit 64f3524e2), it seems okay to now remove these kluges.

Daniel Gustafsson

Discussion: https://postgr.es/m/805874E2-999C-4CDA-856F-1AFBD9DFE933@yesql.se

5 years agoRemove RangeIOData->typiofunc
Alvaro Herrera [Thu, 5 Mar 2020 14:35:02 +0000 (11:35 -0300)]
Remove RangeIOData->typiofunc

We used to carry the I/O function OID in RangeIOData, but it's not used
for anything.  Since the struct is not exposed to the world anyway, we
can simplify it a bit.  Also, rename the FmgrInfo member to match
the accompanying 'typioparam' and put them in a more sensible order.

Reviewed by Tom Lane and Paul Jungwirth.

Discussion: https://postgr.es/m/20200304215711.GA8732@alvherre.pgsql

5 years agoAvoid -Wconversion warnings when using checksum_impl.h
Michael Paquier [Thu, 5 Mar 2020 05:12:37 +0000 (14:12 +0900)]
Avoid -Wconversion warnings when using checksum_impl.h

This does not matter much when compiling Postgres proper as many
warnings exist when enabling this compilation flag, but it can be
annoying for external modules willing to use both.

Author: David Steele
Discussion: https://postgr.es/m/91d86c8a-11fc-7b88-43eb-5ca3f6fb8bd3@pgmasters.net

5 years agoFix issues around .pgpass file.
Fujii Masao [Thu, 5 Mar 2020 04:00:38 +0000 (13:00 +0900)]
Fix issues around .pgpass file.

This commit fixes the following two issues around .pgpass file.

(1) If the length of a line in .pgpass file was larger than 319B,
        libpq silently treated each 319B in the line as a separate
        setting line.

(2) The document explains that a line beginning with # is treated
        as a comment in .pgpass. But there was no code doing such
        special handling. Whether a line begins with # or not, libpq
        just checked that the first token in the line match with the host.

For (1), this commit makes libpq warn if the length of a line
is larger than 319B, and throw away the remaining part beginning
from 320B position.

For (2), this commit changes libpq so that it treats any lines
beginning with # as comments.

Author: Fujii Masao
Reviewed-by: Hamid Akhtar
Discussion: https://postgr.es/m/c0f0c01c-fa74-9749-2084-b73882fd5465@oss.nttdata.com

5 years agoFix more issues with dependency handling at swap phase of REINDEX CONCURRENTLY
Michael Paquier [Thu, 5 Mar 2020 03:50:15 +0000 (12:50 +0900)]
Fix more issues with dependency handling at swap phase of REINDEX CONCURRENTLY

When canceling a REINDEX CONCURRENTLY operation after swapping is done,
a drop of the parent table would leave behind old indexes.  This is a
consequence of 68ac9cf, which fixed the case of pg_depend bloat when
repeating REINDEX CONCURRENTLY on the same relation.

In order to take care of the problem without breaking the previous fix,
this uses a different strategy, possible even with the exiting set of
routines to handle dependency changes.  The dependencies of/on the
new index are additionally switched to the old one, allowing an old
invalid index remaining around because of a cancellation or a failure to
use the dependency links of the concurrently-created index.  This
ensures that dropping any objects the old invalid index depends on also
drops the old index automatically.

Reported-by: Julien Rouhaud
Author: Michael Paquier
Reviewed-by: Julien Rouhaud
Discussion: https://postgr.es/m/20200227080735.l32fqcauy73lon7o@nol
Backpatch-through: 12

5 years agoExtend ExecBuildAggTrans() to support a NULL pointer check.
Jeff Davis [Thu, 5 Mar 2020 01:20:20 +0000 (17:20 -0800)]
Extend ExecBuildAggTrans() to support a NULL pointer check.

Optionally push a step to check for a NULL pointer to the pergroup
state.

This will be important for disk-based hash aggregation in combination
with grouping sets. When memory limits are reached, a given tuple may
find its per-group state for some grouping sets but not others. For
the former, it advances the per-group state as normal; for the latter,
it skips evaluation and the calling code will have to spill the tuple
and reprocess it in a later batch.

Add the NULL check as a separate expression step because in some
common cases it's not needed.

Discussion: https://postgr.es/m/20200221202212.ssb2qpmdgrnx52sj%40alap3.anarazel.de

5 years agoIntroduce macros for typalign and typstorage constants.
Tom Lane [Wed, 4 Mar 2020 15:34:25 +0000 (10:34 -0500)]
Introduce macros for typalign and typstorage constants.

Our usual practice for "poor man's enum" catalog columns is to define
macros for the possible values and use those, not literal constants,
in C code.  But for some reason lost in the mists of time, this was
never done for typalign/attalign or typstorage/attstorage.  It's never
too late to make it better though, so let's do that.

The reason I got interested in this right now is the need to duplicate
some uses of the TYPSTORAGE constants in an upcoming ALTER TYPE patch.
But in general, this sort of change aids greppability and readability,
so it's a good idea even without any specific motivation.

I may have missed a few places that could be converted, and it's even
more likely that pending patches will re-introduce some hard-coded
references.  But that's not fatal --- there's no expectation that
we'd actually change any of these values.  We can clean up stragglers
over time.

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

5 years agoMove pg_upgrade's Windows link() implementation to AC_REPLACE_FUNCS
Peter Eisentraut [Wed, 4 Mar 2020 07:05:33 +0000 (08:05 +0100)]
Move pg_upgrade's Windows link() implementation to AC_REPLACE_FUNCS

This way we can make use of it in other components as well, and it
fits better with the rest of the build system.

Discussion: https://www.postgresql.org/message-id/flat/72fff73f-dc9c-4ef4-83e8-d2e60c98df48%402ndquadrant.com

5 years agoAllow to_date/to_timestamp to recognize non-English month/day names.
Tom Lane [Tue, 3 Mar 2020 16:06:47 +0000 (11:06 -0500)]
Allow to_date/to_timestamp to recognize non-English month/day names.

to_char() has long allowed the TM (translation mode) prefix to
specify output of translated month or day names; but that prefix
had no effect in input format strings.  Now it does.  to_date()
and to_timestamp() will now recognize the same month or day names
that to_char() would output for the same format code.  Matching is
case-insensitive (per the active collation's notion of what that
means), just as it has always been for English month/day names
without the TM prefix.

(As per the discussion thread, there are lots of cases that this
feature will not handle, such as alternate day names.  But being
able to accept what to_char() will output seems useful enough.)

In passing, fix some shaky English and violations of message
style guidelines in jsonpath errors for the .datetime() method,
which depends on this code.

Juan José Santamaría Flecha, reviewed and modified by me,
with other commentary from Alvaro Herrera, Tomas Vondra,
Arthur Zakirov, Peter Eisentraut, Mark Dilger.

Discussion: https://postgr.es/m/CAC+AXB3u1jTngJcoC1nAHBf=M3v-jrEfo86UFtCqCjzbWS9QhA@mail.gmail.com

5 years agopg_standby: Don't use HAVE_WORKING_LINK
Peter Eisentraut [Thu, 27 Feb 2020 15:33:05 +0000 (16:33 +0100)]
pg_standby: Don't use HAVE_WORKING_LINK

HAVE_WORKING_LINK is meant to indicate support for hard links, mainly
for Windows.  Here it is used for soft links on Unix, and the
functionality is optional anyway, so we can just make it error out
normally if needed.

Discussion: https://www.postgresql.org/message-id/flat/72fff73f-dc9c-4ef4-83e8-d2e60c98df48%402ndquadrant.com

5 years agoFix typo in monitoring.sgml.
Fujii Masao [Tue, 3 Mar 2020 06:01:47 +0000 (15:01 +0900)]
Fix typo in monitoring.sgml.

Author: Noriyoshi Shinoda

5 years agoRemove overzealous _bt_split() assertions.
Peter Geoghegan [Tue, 3 Mar 2020 05:40:11 +0000 (21:40 -0800)]
Remove overzealous _bt_split() assertions.

_bt_split() is passed NULL as its insertion scankey for internal page
splits.  Two recently added Assert() statements failed to consider this,
leading to a crash with pg_upgrade'd BREE_VERSION < 4 indexes.  Remove
the assertions.

The assertions in question were added by commit 0d861bbb, which added
nbtree deduplication.  It would be possible to fix the assertions
directly instead, but they weren't adding much anyway.

5 years agoFix assertion failure with ALTER TABLE ATTACH PARTITION and indexes
Michael Paquier [Tue, 3 Mar 2020 04:55:41 +0000 (13:55 +0900)]
Fix assertion failure with ALTER TABLE ATTACH PARTITION and indexes

Using ALTER TABLE ATTACH PARTITION causes an assertion failure when
attempting to work on a partitioned index, because partitioned indexes
cannot have partition bounds.

The grammar of ALTER TABLE ATTACH PARTITION requires partition bounds,
but not ALTER INDEX, so mixing ALTER TABLE with partitioned indexes is
confusing.  Hence, on HEAD, prevent ALTER TABLE to attach a partition if
the relation involved is a partitioned index.  On back-branches, as
applications may rely on the existing behavior, just remove the
culprit assertion.

Reported-by: Alexander Lakhin
Author: Amit Langote, Michael Paquier
Discussion: https://postgr.es/m/16276-5cd1dcc8fb8be7b5@postgresql.org
Backpatch-through: 11

5 years agoFix the name of the first WAL segment file, in docs.
Fujii Masao [Tue, 3 Mar 2020 03:22:04 +0000 (12:22 +0900)]
Fix the name of the first WAL segment file, in docs.

Previously the documentation explains that WAL segment files
start at 000000010000000000000000. But the first WAL segment file
that initdb creates is 000000010000000000000001 not
000000010000000000000000. This change was caused by old
commit 8c843fff2d, but the documentation had not been updated
a long time.

Back-patch to all supported branches.

Author: Fujii Masao
Reviewed-by: David Zhang
Discussion: https://postgr.es/m/CAHGQGwHOmGe2OqGOmp8cOfNVDivq7dbV74L5nUGr+3eVd2CU2Q@mail.gmail.com

5 years agoReport progress of streaming base backup.
Fujii Masao [Tue, 3 Mar 2020 03:03:43 +0000 (12:03 +0900)]
Report progress of streaming base backup.

This commit adds pg_stat_progress_basebackup view that reports
the progress while an application like pg_basebackup is taking
a base backup. This uses the progress reporting infrastructure
added by c16dc1aca5e0, adding support for streaming base backup.

Bump catversion.

Author: Fujii Masao
Reviewed-by: Kyotaro Horiguchi, Amit Langote, Sergei Kornilov
Discussion: https://postgr.es/m/9ed8b801-8215-1f3d-62d7-65bff53f6e94@oss.nttdata.com

5 years agoPreserve pg_index.indisclustered across REINDEX CONCURRENTLY
Michael Paquier [Tue, 3 Mar 2020 01:12:28 +0000 (10:12 +0900)]
Preserve pg_index.indisclustered across REINDEX CONCURRENTLY

If the flag value is lost, a CLUSTER query following REINDEX
CONCURRENTLY could fail.  Non-concurrent REINDEX is already handling
this case consistently.

Author: Justin Pryzby
Discussion: https://postgr.es/m/20200229024202.GH29456@telsasoft.com
Backpatch-through: 12

5 years agoFix possibly-uninitialized variable.
Tom Lane [Mon, 2 Mar 2020 23:41:50 +0000 (18:41 -0500)]
Fix possibly-uninitialized variable.

Thinko in 2f9661311.  Per buildfarm, as well as warning seen locally.

5 years agoRepresent command completion tags as structs
Alvaro Herrera [Mon, 2 Mar 2020 21:19:51 +0000 (18:19 -0300)]
Represent command completion tags as structs

The backend was using strings to represent command tags and doing string
comparisons in multiple places, but that's slow and unhelpful.  Create a
new command list with a supporting structure to use instead; this is
stored in a tag-list-file that can be tailored to specific purposes with
a caller-definable C macro, similar to what we do for WAL resource
managers.  The first first such uses are a new CommandTag enum and a
CommandTagBehavior struct.

Replace numerous occurrences of char *completionTag with a
QueryCompletion struct so that the code no longer stores information
about completed queries in a cstring.  Only at the last moment, in
EndCommand(), does this get converted to a string.

EventTriggerCacheItem no longer holds an array of palloc’d tag strings
in sorted order, but rather just a Bitmapset over the CommandTags.

Author: Mark Dilger, with unsolicited help from Álvaro Herrera
Reviewed-by: John Naylor, Tom Lane
Discussion: https://postgr.es/m/981A9DB4-3F0C-4DA5-88AD-CB9CFF4D6CAD@enterprisedb.com

5 years agoBlacklist port/win32_msvc/utime.h in cpluspluscheck and headerscheck.
Tom Lane [Mon, 2 Mar 2020 19:35:22 +0000 (14:35 -0500)]
Blacklist port/win32_msvc/utime.h in cpluspluscheck and headerscheck.

Since commit 481c8e923 it tends to produce "error: sys/utime.h: No such
file or directory" on non-Windows platforms.

5 years agoSilence nbtree.h cpluspluscheck warning.
Peter Geoghegan [Mon, 2 Mar 2020 18:29:30 +0000 (10:29 -0800)]
Silence nbtree.h cpluspluscheck warning.

Add a cast to size_t to silence "comparison between signed and unsigned
integer expressions" cpluspluscheck warning.

Reported-By: Tom Lane
Discussion: https://postgr.es/m/7971.1583171266@sss.pgh.pa.us

5 years agoAdd assertions to _bt_update_posting().
Peter Geoghegan [Mon, 2 Mar 2020 16:07:16 +0000 (08:07 -0800)]
Add assertions to _bt_update_posting().

Copy some assertions from _bt_form_posting() to its sibling function,
_bt_update_posting().

Discussion: https://postgr.es/m/CAH2-WzkPR8KMwkL0ap976kmXwBCeukTeHz6fB-U__wvuP1S9Zg@mail.gmail.com

5 years agoUpdate Microsoft documentation link
Peter Eisentraut [Mon, 2 Mar 2020 14:21:42 +0000 (15:21 +0100)]
Update Microsoft documentation link

Reported-by: Juan José Santamaría Flecha <juanjo.santamaria@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAC%2BAXB1EDXiRPmiVfh%2BWX79x5vXJDU17k0GkDjfyPgOWO4Y5og%40mail.gmail.com

5 years agoRemove long unused code behind a #if 0
Peter Eisentraut [Mon, 2 Mar 2020 07:55:31 +0000 (08:55 +0100)]
Remove long unused code behind a #if 0

Author: Vignesh C <vignesh21@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CALDaNm3sn4yOq-4rogb-CfE0EYw6b3mVzz8+DnS9BNRwPnhngw@mail.gmail.com

5 years agoFix command-line colorization on Windows with VT100-compatible environments
Michael Paquier [Mon, 2 Mar 2020 06:45:34 +0000 (15:45 +0900)]
Fix command-line colorization on Windows with VT100-compatible environments

When setting PG_COLOR to "always" or "auto" in a Windows terminal
VT100-compatible, the colorization output was not showing up correctly
because it is necessary to update the console's output handling mode.
This fix allows to detect automatically if the environment is compatible
with VT100.  Hence, PG_COLOR=auto is able to detect and handle both
compatible and non-compatible environments.  The behavior of
PG_COLOR=always remains unchanged, as it enforces the use of colorized
output even if the environment does not allow it.

This fix is based on an initial suggestion from Thomas Munro.

Reported-by: Haiying Tang
Author: Juan José Santamaría Flecha
Reviewed-by: Michail Nikolaev, Michael Paquier, Haiying Tang
Discussion: https://postgr.es/m/16108-134692e97146b7bc@postgresql.org
Backpatch-through: 12

5 years agoHandle logical decoding in multi-insert for catalog tuples
Michael Paquier [Mon, 2 Mar 2020 01:00:37 +0000 (10:00 +0900)]
Handle logical decoding in multi-insert for catalog tuples

The code path for multi-insert decoding is not stressed yet for
catalogs (a future patch may introduce this capability), so no
back-patch is needed.

Author: Daniel Gustafsson
Discussion: https://postgr.es/m/9690D72F-5C4F-4016-9572-6D16684E1D87@yesql.se

5 years agoRemove dead code from _bt_update_posting().
Peter Geoghegan [Sun, 1 Mar 2020 20:11:26 +0000 (12:11 -0800)]
Remove dead code from _bt_update_posting().

Discussion: https://postgr.es/m/CAH2-WzmAufHiOku6AGiFD=81VQs5nYJ1L2YkhW1t+BH4CMsgRw@mail.gmail.com

5 years agoFix corner-case loss of precision in numeric ln().
Dean Rasheed [Sun, 1 Mar 2020 14:49:25 +0000 (14:49 +0000)]
Fix corner-case loss of precision in numeric ln().

When deciding on the local rscale to use for the Taylor series
expansion, ln_var() neglected to account for the fact that the result
is subsequently multiplied by a factor of 2^(nsqrt+1), where nsqrt is
the number of square root operations performed in the range reduction
step, which can be as high as 22 for very large inputs. This could
result in a loss of precision, particularly when combined with large
rscale values, for which a large number of Taylor series terms is
required (up to around 400).

Fix by computing a few extra digits in the Taylor series, based on the
weight of the multiplicative factor log10(2^(nsqrt+1)). It remains to
be proven whether or not the other 8 extra digits used for the Taylor
series is appropriate, but this at least deals with the obvious
oversight of failing to account for the effects of the final
multiplication.

Per report from Justin AnyhowStep. Reviewed by Tom Lane.

Discussion: https://postgr.es/m/16280-279f299d9c06e56f@postgresql.org

5 years agoDoc: Fix pageinspect bt_page_items() example.
Peter Geoghegan [Sat, 29 Feb 2020 23:10:13 +0000 (15:10 -0800)]
Doc: Fix pageinspect bt_page_items() example.

Oversight in commit 93ee38ea.

5 years agoTeach pageinspect about nbtree deduplication.
Peter Geoghegan [Sat, 29 Feb 2020 20:10:17 +0000 (12:10 -0800)]
Teach pageinspect about nbtree deduplication.

Add a new bt_metap() column to display the metapage's allequalimage
field.  Also add three new columns to contrib/pageinspect's
bt_page_items() function:

* Add a boolean column ("dead") that displays the LP_DEAD bit value for
each non-pivot tuple.

* Add a TID column ("htid") that displays a single heap TID value for
each tuple.  This is the TID that is returned by BTreeTupleGetHeapTID(),
so comparable values are shown for pivot tuples, plain non-pivot tuples,
and posting list tuples.

* Add a TID array column ("tids") that displays TIDs from each tuple's
posting list, if any.  This works just like the "tids" column from
pageinspect's gin_leafpage_items() function.

No version bump for the pageinspect extension, since there hasn't been a
stable Postgres release since the last version bump (the last bump was
part of commit 58b4cb30).

Author: Peter Geoghegan
Discussion: https://postgr.es/m/CAH2-WzmSMmU2eNvY9+a4MNP+z02h6sa-uxZvN3un6jY02ZVBSw@mail.gmail.com

5 years agoCorrectly re-use hash tables in buildSubPlanHash().
Tom Lane [Sat, 29 Feb 2020 18:48:09 +0000 (13:48 -0500)]
Correctly re-use hash tables in buildSubPlanHash().

Commit 356687bd8 omitted to remove leftover code for destroying
a hashed subplan's hash tables, with the result that the tables
were always rebuilt not reused; this leads to severe memory
leakage if a hashed subplan is re-executed enough times.
Moreover, the code for reusing the hashnulls table had a typo
that would have made it do the wrong thing if it were reached.

Looking at the code coverage report shows severe under-coverage
of the potential callers of ResetTupleHashTable, so add some test
cases that exercise them.

Andreas Karlsson and Tom Lane, per reports from Ranier Vilela
and Justin Pryzby.

Backpatch to v11, as the faulty commit was.

Discussion: https://postgr.es/m/edb62547-c453-c35b-3ed6-a069e4d6b937@proxel.se
Discussion: https://postgr.es/m/CAEudQAo=DCebm1RXtig9OH+QivpS97sMkikt0A9qHmMUs+g6ZA@mail.gmail.com
Discussion: https://postgr.es/m/20200210032547.GA1412@telsasoft.com

5 years agoRemove obsolete comment.
Tom Lane [Sat, 29 Feb 2020 18:23:12 +0000 (13:23 -0500)]
Remove obsolete comment.

Noted while studying subplan hash issue.

5 years agoAvoid failure if autovacuum tries to access a just-dropped temp namespace.
Tom Lane [Sat, 29 Feb 2020 01:28:34 +0000 (20:28 -0500)]
Avoid failure if autovacuum tries to access a just-dropped temp namespace.

Such an access became possible when commit 246a6c8f7 added more
aggressive cleanup of orphaned temp relations by autovacuum.
Since autovacuum's snapshot might be slightly stale, it could
attempt to access an already-dropped temp namespace, resulting in
an assertion failure or null-pointer dereference.  (In practice,
since we don't drop temp namespaces automatically but merely
recycle them, this situation could only arise if a superuser does
a manual drop of a temp namespace.  Still, that should be allowed.)

The core of the bug, IMO, is that isTempNamespaceInUse and its callers
failed to think hard about whether to treat "temp namespace isn't there"
differently from "temp namespace isn't in use".  In hopes of forestalling
future mistakes of the same ilk, replace that function with a new one
checkTempNamespaceStatus, which makes the same tests but returns a
three-way enum rather than just a bool.  isTempNamespaceInUse is gone
entirely in HEAD; but just in case some external code is relying on it,
keep it in the back branches, as a bug-compatible wrapper around the
new function.

Per report originally from Prabhat Kumar Sahu, investigated by Mahendra
Singh and Michael Paquier; the final form of the patch is my fault.
This replaces the failed fix attempt in a052f6cbb.

Backpatch as far as v11, as 246a6c8f7 was.

Discussion: https://postgr.es/m/CAKYtNAr9Zq=1-ww4etHo-VCC-k120YxZy5OS01VkaLPaDbv2tg@mail.gmail.com

5 years agoFix commit c11cb17d.
Jeff Davis [Fri, 28 Feb 2020 17:32:35 +0000 (09:32 -0800)]
Fix commit c11cb17d.

I neglected to update copyfuncs/outfuncs/readfuncs.

Discussion: https://postgr.es/m/12491.1582833409%40sss.pgh.pa.us

5 years agoDoc: correct thinko in pg_buffercache documentation.
Tom Lane [Fri, 28 Feb 2020 16:29:58 +0000 (11:29 -0500)]
Doc: correct thinko in pg_buffercache documentation.

Access to this module is granted to the pg_monitor role, not
pg_read_all_stats.  (Given the view's performance impact,
it seems wise to be restrictive, so I think this was the
correct decision --- and anyway it was clearly intentional.)

Per bug #16279 from Philip Semanchuk.

Discussion: https://postgr.es/m/16279-fcaac33c68aab0ab@postgresql.org

5 years agoAdd comments on avoid reuse of parse-time snapshot
Alvaro Herrera [Fri, 28 Feb 2020 16:13:54 +0000 (13:13 -0300)]
Add comments on avoid reuse of parse-time snapshot

Apparently, reusing the parse-time query snapshot for later steps
(execution) is a frequently considered optimization ... but it doesn't
work, for reasons discovered in thread [1].  Adding some comments about
why it doesn't really work can relieve some future hackers from wasting
time reimplementing it again.

[1] https://postgr.es/m/flat/5075D8DF.6050500@fuzzy.cz

Author: Michail Nikolaev
Discussion: https://postgr.es/m/CANtu0ogp6cTvMJObXP8n=k+JtqxY1iT9UV5MbGCpjjPa5crCiw@mail.gmail.com

5 years agoAdd PostgreSQL home page to --help output
Peter Eisentraut [Fri, 28 Feb 2020 07:54:49 +0000 (08:54 +0100)]
Add PostgreSQL home page to --help output

Per emerging standard in GNU programs and elsewhere.  Autoconf already
has support for specifying a home page, so we can just that.

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/8d389c5f-7fb5-8e48-9a4a-68cec44786fa%402ndquadrant.com

5 years agoRefer to bug report address by symbol rather than hardcoding
Peter Eisentraut [Fri, 28 Feb 2020 07:54:49 +0000 (08:54 +0100)]
Refer to bug report address by symbol rather than hardcoding

Use the PACKAGE_BUGREPORT macro that is created by Autoconf for
referring to the bug reporting address rather than hardcoding it
everywhere.  This makes it easier to change the address and it reduces
translation work.

Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://www.postgresql.org/message-id/flat/8d389c5f-7fb5-8e48-9a4a-68cec44786fa%402ndquadrant.com

5 years agoCatversion bump for b9b408c48724
Alvaro Herrera [Thu, 27 Feb 2020 20:25:47 +0000 (17:25 -0300)]
Catversion bump for b9b408c48724

Per Tom Lane.

5 years agoSave calculated transitionSpace in Agg node.
Jeff Davis [Thu, 27 Feb 2020 18:46:58 +0000 (10:46 -0800)]
Save calculated transitionSpace in Agg node.

This will be useful in the upcoming Hash Aggregation work to improve
estimates for hash table sizing.

Discussion: https://postgr.es/m/37091115219dd522fd9ed67333ee8ed1b7e09443.camel%40j-davis.com

5 years agoDoc: Fix deduplicate_items index term.
Peter Geoghegan [Thu, 27 Feb 2020 17:32:34 +0000 (09:32 -0800)]
Doc: Fix deduplicate_items index term.

Reported-By: Fujii Masao
Discussion: https://postgr.es/m/18f07ae8-7d89-537c-b0a9-54100a1b46da@oss.nttdata.com

5 years agoRecord parents of triggers
Alvaro Herrera [Thu, 27 Feb 2020 16:23:33 +0000 (13:23 -0300)]
Record parents of triggers

This let us get rid of a recently introduced ugly hack (commit
1fa846f1c9af).

Author: Álvaro Herrera
Reviewed-by: Amit Langote, Tom Lane
Discussion: https://postgr.es/m/20200217215641.GA29784@alvherre.pgsql

5 years agoRemove TAP test for createdb --lc-ctype
Michael Paquier [Thu, 27 Feb 2020 12:58:37 +0000 (21:58 +0900)]
Remove TAP test for createdb --lc-ctype

OpenBSD falls back to "C" when using an incorrect input with setlocale()
and LC_CTYPE, causing this test, introduced by 008cf04, to fail.  This
removes the culprit test to avoid the portability issue.

Per report from Robert Haas, via buildfarm member curculio.

Discussion: https://postgr.es/m/CA+TgmoZ6ddh3mHD9gU8DvNYoFmuJaYYn1+4AvZNp25vTdRwCAQ@mail.gmail.com
Backpatch-through: 11

5 years agoSkip foreign tablespaces when running pg_checksums/pg_verify_checksums
Michael Paquier [Thu, 27 Feb 2020 06:31:27 +0000 (15:31 +0900)]
Skip foreign tablespaces when running pg_checksums/pg_verify_checksums

Attempting to use pg_checksums (pg_verify_checksums in 11) on a data
folder which includes tablespace paths used across multiple major
versions would cause pg_checksums to scan all directories present in
pg_tblspc, and not only marked with TABLESPACE_VERSION_DIRECTORY.  This
could lead to failures when for example running sanity checks on an
upgraded instance with --check.  Even worse, it was possible to rewrite
on-disk pages with --enable for a cluster potentially online.

This commit makes pg_checksums skip any directories not named
TABLESPACE_VERSION_DIRECTORY, similarly to what is done for base
backups.

Reported-by: Michael Banck
Author: Michael Banck, Bernd Helmle
Discussion: https://postgr.es/m/62031974fd8e941dd8351fbc8c7eff60d59c5338.camel@credativ.de
backpatch-through: 11

5 years agoMove src/backend/utils/hash/hashfn.c to src/common
Robert Haas [Thu, 27 Feb 2020 03:55:41 +0000 (09:25 +0530)]
Move src/backend/utils/hash/hashfn.c to src/common

This also involves renaming src/include/utils/hashutils.h, which
becomes src/include/common/hashfn.h. Perhaps an argument can be
made for keeping the hashutils.h name, but it seemed more
consistent to make it match the name of the file, and also more
descriptive of what is actually going on here.

Patch by me, reviewed by Suraj Kharage and Mark Dilger. Off-list
advice on how not to break the Windows build from Davinder Singh
and Amit Kapila.

Discussion: http://postgr.es/m/CA+TgmoaRiG4TXND8QuM6JXFRkM_1wL2ZNhzaUKsuec9-4yrkgw@mail.gmail.com

5 years agocreatedb: Fix quoting of --encoding, --lc-ctype and --lc-collate
Michael Paquier [Thu, 27 Feb 2020 02:20:46 +0000 (11:20 +0900)]
createdb: Fix quoting of --encoding, --lc-ctype and --lc-collate

The original coding failed to properly quote those arguments, leading to
failures when using quotes in the values used.  As the quoting can be
encoding-sensitive, the connection to the backend needs to be taken
before applying the correct quoting.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20200214041004.GB1998@paquier.xyz
Backpatch-through: 9.5

5 years agoSilence another compiler warning in nbtinsert.c.
Peter Geoghegan [Wed, 26 Feb 2020 23:15:45 +0000 (15:15 -0800)]
Silence another compiler warning in nbtinsert.c.

Per complaint from Álvaro Herrera.

5 years agoSuppress unnecessary RelabelType nodes in more cases.
Tom Lane [Wed, 26 Feb 2020 23:13:58 +0000 (18:13 -0500)]
Suppress unnecessary RelabelType nodes in more cases.

eval_const_expressions sometimes produced RelabelType nodes that
were useless because they just relabeled an expression to the same
exposed type it already had.  This is worth avoiding because it can
cause two equivalent expressions to not be equal(), preventing
recognition of useful optimizations.  In the test case added here,
an unpatched planner fails to notice that the "sqli = constant" clause
renders a sort step unnecessary, because one code path produces an
extra RelabelType and another doesn't.

Fix by ensuring that eval_const_expressions_mutator's T_RelabelType
case will not add in an unnecessary RelabelType.  Also save some
code by sharing a subroutine with the effectively-equivalent cases
for CollateExpr and CoerceToDomain.  (CollateExpr had no bug, and
I think that the case couldn't arise with CoerceToDomain, but
it seems prudent to do the same check for all three cases.)

Back-patch to v12.  In principle this has been wrong all along,
but I haven't seen a case where it causes visible misbehavior
before v12, so refrain from changing stable branches unnecessarily.

Per investigation of a report from Eric Gillum.

Discussion: https://postgr.es/m/CAMmjdmvAZsUEskHYj=KT9sTukVVCiCSoe_PBKOXsncFeAUDPCQ@mail.gmail.com

5 years agoFix docs regarding AFTER triggers on partitioned tables
Alvaro Herrera [Wed, 26 Feb 2020 22:57:14 +0000 (19:57 -0300)]
Fix docs regarding AFTER triggers on partitioned tables

In commit 86f575948c77 I forgot to update the trigger.sgml paragraph
that needs to explain that AFTER triggers are allowed in partitioned
tables.  Do so now.

Discussion: https://postgr.es/m/20200224185850.GA30899@alvherre.pgsql

5 years agoSilence compiler warning in nbtinsert.c.
Peter Geoghegan [Wed, 26 Feb 2020 21:17:36 +0000 (13:17 -0800)]
Silence compiler warning in nbtinsert.c.

Per buildfarm member longfin.

5 years agoAdd deduplication to nbtree.
Peter Geoghegan [Wed, 26 Feb 2020 21:05:30 +0000 (13:05 -0800)]
Add deduplication to nbtree.

Deduplication reduces the storage overhead of duplicates in indexes that
use the standard nbtree index access method.  The deduplication process
is applied lazily, after the point where opportunistic deletion of
LP_DEAD-marked index tuples occurs.  Deduplication is only applied at
the point where a leaf page split would otherwise be required.  New
posting list tuples are formed by merging together existing duplicate
tuples.  The physical representation of the items on an nbtree leaf page
is made more space efficient by deduplication, but the logical contents
of the page are not changed.  Even unique indexes make use of
deduplication as a way of controlling bloat from duplicates whose TIDs
point to different versions of the same logical table row.

The lazy approach taken by nbtree has significant advantages over a GIN
style eager approach.  Most individual inserts of index tuples have
exactly the same overhead as before.  The extra overhead of
deduplication is amortized across insertions, just like the overhead of
page splits.  The key space of indexes works in the same way as it has
since commit dd299df8 (the commit that made heap TID a tiebreaker
column).

Testing has shown that nbtree deduplication can generally make indexes
with about 10 or 15 tuples for each distinct key value about 2.5X - 4X
smaller, even with single column integer indexes (e.g., an index on a
referencing column that accompanies a foreign key).  The final size of
single column nbtree indexes comes close to the final size of a similar
contrib/btree_gin index, at least in cases where GIN's posting list
compression isn't very effective.  This can significantly improve
transaction throughput, and significantly reduce the cost of vacuuming
indexes.

A new index storage parameter (deduplicate_items) controls the use of
deduplication.  The default setting is 'on', so all new B-Tree indexes
automatically use deduplication where possible.  This decision will be
reviewed at the end of the Postgres 13 beta period.

There is a regression of approximately 2% of transaction throughput with
synthetic workloads that consist of append-only inserts into a table
with several non-unique indexes, where all indexes have few or no
repeated values.  The underlying issue is that cycles are wasted on
unsuccessful attempts at deduplicating items in non-unique indexes.
There doesn't seem to be a way around it short of disabling
deduplication entirely.  Note that deduplication of items in unique
indexes is fairly well targeted in general, which avoids the problem
there (we can use a special heuristic to trigger deduplication passes in
unique indexes, since we're specifically targeting "version bloat").

Bump XLOG_PAGE_MAGIC because xl_btree_vacuum changed.

No bump in BTREE_VERSION, since the representation of posting list
tuples works in a way that's backwards compatible with version 4 indexes
(i.e. indexes built on PostgreSQL 12).  However, users must still
REINDEX a pg_upgrade'd index to use deduplication, regardless of the
Postgres version they've upgraded from.  This is the only way to set the
new nbtree metapage flag indicating that deduplication is generally
safe.

Author: Anastasia Lubennikova, Peter Geoghegan
Reviewed-By: Peter Geoghegan, Heikki Linnakangas
Discussion:
    https://postgr.es/m/55E4051B.7020209@postgrespro.ru
    https://postgr.es/m/4ab6e2db-bcee-f4cf-0916-3a06e6ccbb55@postgrespro.ru

5 years agoAdd equalimage B-Tree support functions.
Peter Geoghegan [Wed, 26 Feb 2020 19:28:25 +0000 (11:28 -0800)]
Add equalimage B-Tree support functions.

Invent the concept of a B-Tree equalimage ("equality implies image
equality") support function, registered as support function 4.  This
indicates whether it is safe (or not safe) to apply optimizations that
assume that any two datums considered equal by an operator class's order
method must be interchangeable without any loss of semantic information.
This is static information about an operator class and a collation.

Register an equalimage routine for almost all of the existing B-Tree
opclasses.  We only need two trivial routines for all of the opclasses
that are included with the core distribution.  There is one routine for
opclasses that index non-collatable types (which returns 'true'
unconditionally), plus another routine for collatable types (which
returns 'true' when the collation is a deterministic collation).

This patch is infrastructure for an upcoming patch that adds B-Tree
deduplication.

Author: Peter Geoghegan, Anastasia Lubennikova
Discussion: https://postgr.es/m/CAH2-Wzn3Ee49Gmxb7V1VJ3-AC8fWn-Fr8pfWQebHe8rYRxt5OQ@mail.gmail.com

5 years agoInclude error code in message from pg_upgrade
Magnus Hagander [Wed, 26 Feb 2020 09:03:11 +0000 (10:03 +0100)]
Include error code in message from pg_upgrade

In passing, also quote the filename in one message where it wasn't.

Author: Dagfinn Ilmari Mannsåker
Discussion: https://postgr.es/m/87pne2w98h.fsf@wibble.ilmari.org

5 years agoFix build failure on header generation with repetitive builds of MSVC
Michael Paquier [Tue, 25 Feb 2020 04:57:40 +0000 (13:57 +0900)]
Fix build failure on header generation with repetitive builds of MSVC

GenerateConfigHeader() in Solution.pm was complaining about unused
define symbols even if a newer config header was not generated, causing
successive build attempts with MSVC to fail.

Oversight in commit 8f4fb4c.

Author: Kyotaro Horiguchi
Reviewed-by: Juan José Santamaría Flecha
Discussion: https://postgr.es/m/20200218.160500.44393633318853097.horikyota.ntt@gmail.com

5 years agoFix compile failure.
Tom Lane [Mon, 24 Feb 2020 23:43:23 +0000 (18:43 -0500)]
Fix compile failure.

I forgot that some compilers won't handle #if constructs within
ereport() calls.  Duplicating most of the call is annoying but simple.
Per buildfarm.

5 years agoexpression eval: Reduce number of steps for agg transition invocations.
Andres Freund [Mon, 24 Feb 2020 22:39:22 +0000 (14:39 -0800)]
expression eval: Reduce number of steps for agg transition invocations.

Do so by combining the various steps that are part of aggregate
transition function invocation into one larger step. As some of the
current steps are only necessary for some aggregates, have one variant
of the aggregate transition step for each possible combination.

To avoid further manual copies of code in the different transition
step implementations, move most of the code into helper functions
marked as "always inline".

The benefit of this change is an increase in performance when
aggregating lots of rows. This comes in part due to the reduced number
of indirect jumps due to the reduced number of steps, and in part by
reducing redundant setup code across steps. This mainly benefits
interpreted execution, but the code generated by JIT is also improved
a bit.

As a nice side-effect it also ends up making the code a bit simpler.

A small additional optimization is removing the need to set
aggstate->curaggcontext before calling ExecAggInitGroup, choosing to
instead passign curaggcontext as an argument. It was, in contrast to
other aggregate related functions, only needed to fetch a memory
context to copy the transition value into.

Author: Andres Freund
Discussion:
   https://postgr.es/m/20191023163849.sosqbfs5yenocez3@alap3.anarazel.de
   https://postgr.es/m/5c371df7cee903e8cd4c685f90c6c72086d3a2dc.camel@j-davis.com

5 years agoIssue properly WAL record for CID of first catalog tuple in multi-insert
Michael Paquier [Mon, 24 Feb 2020 22:55:22 +0000 (07:55 +0900)]
Issue properly WAL record for CID of first catalog tuple in multi-insert

Multi-insert for heap is not yet used actively for catalogs, but the
code to support this case is in place for logical decoding.  The
existing code forgot to issue a XLOG_HEAP2_NEW_CID record for the first
tuple inserted, leading to failures when attempting to use multiple
inserts for catalogs at decoding time.  This commit fixes the problem by
WAL-logging the needed CID.

This is not an active bug, so no back-patch is done.

Author: Daniel Gustafsson
Discussion: https://postgr.es/m/E0D4CC67-A1CF-4DF4-991D-B3AC2EB5FAE9@yesql.se

5 years agoAccount explicitly for long-lived FDs that are allocated outside fd.c.
Tom Lane [Mon, 24 Feb 2020 22:28:33 +0000 (17:28 -0500)]
Account explicitly for long-lived FDs that are allocated outside fd.c.

The comments in fd.c have long claimed that all file allocations should
go through that module, but in reality that's not always practical.
fd.c doesn't supply APIs for invoking some FD-producing syscalls like
pipe() or epoll_create(); and the APIs it does supply for non-virtual
FDs are mostly insistent on releasing those FDs at transaction end;
and in some cases the actual open() call is in code that can't be made
to use fd.c, such as libpq.

This has led to a situation where, in a modern server, there are likely
to be seven or so long-lived FDs per backend process that are not known
to fd.c.  Since NUM_RESERVED_FDS is only 10, that meant we had *very*
few spare FDs if max_files_per_process is >= the system ulimit and
fd.c had opened all the files it thought it safely could.  The
contrib/postgres_fdw regression test, in particular, could easily be
made to fall over by running it under a restrictive ulimit.

To improve matters, invent functions Acquire/Reserve/ReleaseExternalFD
that allow outside callers to tell fd.c that they have or want to allocate
a FD that's not directly managed by fd.c.  Add calls to track all the
fixed FDs in a standard backend session, so that we are honestly
guaranteeing that NUM_RESERVED_FDS FDs remain unused below the EMFILE
limit in a backend's idle state.  The coding rules for these functions say
that there's no need to call them in code that just allocates one FD over
a fairly short interval; we can dip into NUM_RESERVED_FDS for such cases.
That means that there aren't all that many places where we need to worry.
But postgres_fdw and dblink must use this facility to account for
long-lived FDs consumed by libpq connections.  There may be other places
where it's worth doing such accounting, too, but this seems like enough
to solve the immediate problem.

Internally to fd.c, "external" FDs are limited to max_safe_fds/3 FDs.
(Callers can choose to ignore this limit, but of course it's unwise
to do so except for fixed file allocations.)  I also reduced the limit
on "allocated" files to max_safe_fds/3 FDs (it had been max_safe_fds/2).
Conceivably a smarter rule could be used here --- but in practice,
on reasonable systems, max_safe_fds should be large enough that this
isn't much of an issue, so KISS for now.  To avoid possible regression
in the number of external or allocated files that can be opened,
increase FD_MINFREE and the lower limit on max_files_per_process a
little bit; we now insist that the effective "ulimit -n" be at least 64.

This seems like pretty clearly a bug fix, but in view of the lack of
field complaints, I'll refrain from risking a back-patch.

Discussion: https://postgr.es/m/E1izCmM-0005pV-Co@gemulon.postgresql.org

5 years agoChange client-side fsync_fname() to report errors fatally
Peter Eisentraut [Mon, 24 Feb 2020 15:32:34 +0000 (16:32 +0100)]
Change client-side fsync_fname() to report errors fatally

Given all we have learned about fsync() error handling in the last few
years, reporting an fsync() error non-fatally is not useful,
unless you don't care much about the file, in which case you probably
don't need to use fsync() in the first place.

Change fsync_fname() and durable_rename() to exit(1) on fsync() errors
other than those that we specifically chose to ignore.

This affects initdb, pg_basebackup, pg_checksums, pg_dump, pg_dumpall,
and pg_rewind.

Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://www.postgresql.org/message-id/flat/d239d1bd-aef0-ca7c-dc0a-da14bdcf0392%402ndquadrant.com