Commit 9af34f3c6b naively assumed that all non-windows platforms would
have pseudoterminals and that perl would have IO::Pty. Such is not the
case. Instead of assumung anything, test for the presence of IO::Pty and
only use code that might depend on it if it's present. The test result is
exposed in $TestLib::have_io_pty so that it can be conveniently used in
SKIP tests.
Discussion: https://postgr.es/m/20191126044110.GB5435@paquier.xyz
The fix for CVE-2017-7484 disallowed use of pg_statistic data for
planning purposes if the user would not be able to select the associated
column and a non-leakproof function is to be applied to the statistics
values. That turns out to disable use of pg_statistic data in some
common cases involving inheritance/partitioning, where the user does
have permission to select from the parent table that was actually named
in the query, but not from a child table whose stats are needed. Since,
in non-corner cases, the user *can* select the child table's data via
the parent, this restriction is not actually useful from a security
standpoint. Improve the logic so that we also check the permissions of
the originally-named table, and allow access if select permission exists
for that.
When checking access to stats for a simple child column, we can map
the child column number back to the parent, and perform this test
exactly (including not allowing access if the child column isn't
exposed by the parent). For expression indexes, the current logic
just insists on whole-table select access, and this patch allows
access if the user can select the whole parent table. In principle,
if the child table has extra columns, this might allow access to
stats on columns the user can't read. In practice, it's unlikely
that the planner is going to do any stats calculations involving
expressions that are not visible to the query, so we'll ignore that
fine point for now. Perhaps someday we'll improve that logic to
detect exactly which columns are used by an expression index ...
but today is not that day.
Back-patch to v11. The issue was created in 9.2 and up by the
CVE-2017-7484 fix, but this patch depends on the append_rel_array[]
planner data structure which only exists in v11 and up. In
practice the issue is most urgent with partitioned tables, so
fixing v11 and later should satisfy much of the practical need.
Dilip Kumar and Amit Langote, with some kibitzing by me
Discussion: https://postgr.es/m/3876.1531261875@sss.pgh.pa.us
On some platforms, fsync() returns EBADFD when opening a file descriptor
with O_RDONLY (read-only), leading ultimately now to a PANIC to prevent
data corruption.
This commit adds a new sanity check in pg_fsync() based on fcntl() to
make sure that we don't repeat again mistakes with incorrectly-set file
descriptors so as problems are detected at an early stage. Without
that, such errors could only be detected after running Postgres on a
specific supported platform for the culprit code path, which could take
some time before being found. b8e19b93 was a fix for such a problem,
which got undetected for more than 5 years, and a586cc4b fixed another
similar issue.
Note that the new check added works as well when fsync=off is
configured, so as all regression tests would detect problems as long as
assertions are enabled. fcntl() being not available on Windows, the
new checks do not happen there.
Author: Michael Paquier
Reviewed-by: Mark Dilger
Discussion: https://postgr.es/m/20191009062640.GB21379@paquier.xyz
Revert part of commit 19df1702f5.
Early shutdown was added by that commit so that we could collect
statistics from workers, but unfortunately, it interacted badly with
rescans. The problem is that we ended up destroying the parallel context
which is required for rescans. This leads to rescans of a Limit node over
a Gather node to produce unpredictable results as it tries to access
destroyed parallel context. By reverting the early shutdown code, we
might lose statistics in some cases of Limit over Gather [Merge], but that
will require further study to fix.
Reported-by: Jerry Sievers
Diagnosed-by: Thomas Munro
Author: Amit Kapila, testcase by Vignesh C
Backpatch-through: 9.6
Discussion: https://postgr.es/m/87ims2amh6.fsf@jsievers.enova.com
AuxiliaryProcessMain does ProcSignalInit, so one might expect that
auxiliary processes would need to respond to SendProcSignal, but none
of the auxiliary processes do that. Change them to use
procsignal_sigusr1_handler instead of their own private handlers so
that they do. Besides seeming more correct, this is also less code. It
shouldn't make any functional difference right now because, as far as
we know, there are no current cases where SendProcSignal targets an
auxiliary process, but there are plans to change that in the future.
Andres Freund
Discussion: http://postgr.es/m/20181030051643.elbxjww5jjgnjaxg@alap3.anarazel.de
Where possible, do this using a pseudoterminal, so that things like
openssl that want to open /dev/tty if stdin isn't a tty won't.
Elsewhere, i.e. Windows, just close by providing an empty string using
the standard IPC::Run pipe mechanism.
Patch by Andrew Dunstan, based on an idea from Craig Ringer.
Reviewed by Mark Dilger.
Discussion: https://postgr.es/m/873ebb57-fc98-340d-949d-691b1810bf66@2ndQuadrant.com
XLogReader, walsender and pg_waldump all had their own routines to read
data from WAL files to memory, with slightly different approaches
according to the particular conditions of each environment. There's a
lot of commonality, so we can refactor that into a single routine
WALRead in XLogReader, and move the differences to a separate (simpler)
callback that just opens the next WAL-segment. This results in a
clearer (ahem) code flow.
The error reporting needs are covered by filling in a new error-info
struct, WALReadError, and it's the caller's responsibility to act on it.
The backend has WALReadRaiseError() to do so.
We no longer ever need to seek in this interface; switch to using
pg_pread().
Author: Antonin Houska, with contributions from Álvaro Herrera
Reviewed-by: Michaël Paquier, Kyotaro Horiguchi
Discussion: https://postgr.es/m/14984.1554998742@spoje.net
"%ld" is not an acceptable format spec for int64 variables, though
it accidentally works on most non-Windows 64-bit platforms. Follow
the lead of commit 6a1cd8b92, and use "%lld" with an explicit cast
to long long. Per buildfarm.
This reworks the reloption parsing and build of a couple of index AMs by
creating new structures for each index AM's options. This split was
already done for BRIN, GIN and GiST (which actually has a fillfactor
parameter), but not for hash, B-tree and SPGiST which relied on
StdRdOptions due to an overlap with the default option set.
This saves a couple of bytes for rd_options in each relcache entry with
indexes making use of relation options, and brings more consistency
between all index AMs. While on it, add a couple of AssertMacro() calls
to make sure that utility macros to grab values of reloptions are used
with the expected index AM.
Author: Nikolay Shaplov
Reviewed-by: Amit Langote, Michael Paquier, Álvaro Herrera, Dent John
Discussion: https://postgr.es/m/4127670.gFlpRb6XCm@x200m
It is hoped that this will avoid some errors that we have seen before,
but lately with greater frequency, in buildfarm animals.
For now apply only to master. If this proves effective it can be
backpatched.
Discussion: https://postgr.es/m/13900.1572839580@sss.pgh.pa.us
Author: Juan José Santamaría Flecha
If LISTEN is the only action in a serializable-mode transaction,
and the session was not previously listening, and the notify queue
is not empty, predicate.c reported an assertion failure. That
happened because we'd acquire the transaction's initial snapshot
during PreCommit_Notify, which was called *after* predicate.c
expects any such snapshot to have been established.
To fix, just swap the order of the PreCommit_Notify and
PreCommit_CheckForSerializationFailure calls during CommitTransaction.
This will imply holding the notify-insertion lock slightly longer,
but the difference could only be meaningful in serializable mode,
which is an expensive option anyway.
It appears that this is just an assertion failure, with no
consequences in non-assert builds. A snapshot used only to scan
the notify queue could not have been involved in any serialization
conflicts, so there would be nothing for
PreCommit_CheckForSerializationFailure to do except assign it a
prepareSeqNo and set the SXACT_FLAG_PREPARED flag. And given no
conflicts, neither of those omissions affect the behavior of
ReleasePredicateLocks. This admittedly once-over-lightly analysis
is backed up by the lack of field reports of trouble.
Per report from Mark Dilger. The bug is old, so back-patch to all
supported branches; but the new test case only goes back to 9.6,
for lack of adequate isolationtester infrastructure before that.
Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com
Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
This patch ensures that, if any notify messages were received during
a just-finished transaction, they get sent to the frontend just before
not just after the ReadyForQuery message. With libpq and other client
libraries that act similarly, this guarantees that the client will see
the notify messages as available as soon as it thinks the transaction
is done.
This probably makes no difference in practice, since in realistic
use-cases the application would have to cope with asynchronous
arrival of notify events anyhow. However, it makes it a lot easier
to build cross-session-notify test cases with stable behavior.
I'm a bit surprised now that we've not seen any buildfarm instability
with the test cases added by commit b10f40bf0. Tests that I intend
to add in an upcoming bug fix are definitely unstable without this.
Back-patch to 9.6, which is as far back as we can do NOTIFY testing
with the isolationtester infrastructure.
Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
This function wasn't touched in commit 51004c717, but that turns out
to be a bad idea, because its results now include any dead space
that exists in the NOTIFY queue on account of our being lazy about
advancing the queue tail. Notably, the isolation tests now fail
if run twice without a server restart between, because async-notify's
first test of the function will already show a positive value.
It seems likely that end users would be equally unhappy about the
result's instability. To fix, just make the function call
asyncQueueAdvanceTail before computing its result. That should end
in producing the same value as before, and it's hard to believe that
there's any practical use-case where pg_notification_queue_usage()
is called so often as to create a performance degradation, especially
compared to what we did before.
Out of paranoia, also mark this function parallel-restricted (it
was volatile, but parallel-safe by default, before). Although the
code seems to work fine when run in a parallel worker, that's outside
the design scope of async.c, and it's a bit scary to have intentional
side-effects happening in a parallel worker. There seems no plausible
use-case where it'd be important to try to parallelize this, so let's
not take any risk of introducing new bugs.
In passing, re-pgindent async.c and run reformat-dat-files on
pg_proc.dat, just because I'm a neatnik.
Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
Use SELinux "db_table: { truncate }" to check if permission is granted to
TRUNCATE. Update example SELinux policy to grant needed permission for
TRUNCATE. Add new regression test to demonstrate a positive and negative
cases. Test will only be run if the loaded SELinux policy has the
"db_table: { truncate }" permission. Makes use of recent commit which added
object TRUNCATE hook. Patch by Yuli Khodorkovskiy with minor
editorialization by me. Not back-patched because the object TRUNCATE hook
was not.
Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com
All operations with acl permissions checks should have a corresponding hook
so that, for example, mandatory access control (MAC) may be enforced by an
extension. The command TRUNCATE is missing this hook, so add it. Patch by
Yuli Khodorkovskiy with some editorialization by me. Based on the discussion
not back-patched. A separate patch will exercise the hook in the sepgsql
extension.
Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com
Up to now, whatever you'd edited was put back into the query buffer
but not redisplayed, which is less than user-friendly. But we can
improve that just by printing the text along with a prompt, if we
enforce that the editing result ends with a newline (which it
typically would anyway). You then continue typing more lines if
you want, or you can type ";" or do \g or \r or another \e.
This is intentionally divorced from readline's processing,
for simplicity and so that it works the same with or without
readline enabled. We discussed possibly integrating things
more closely with readline; but that seems difficult, uncertainly
portable across different readline and libedit versions, and
of limited real benefit anyway. Let's try the simple way and
see if it's good enough.
Patch by me, thanks to Fabien Coelho and Laurenz Albe for review
Discussion: https://postgr.es/m/13192.1572318028@sss.pgh.pa.us
We already used this optimization if the plpgsql function is read-only.
But it seems okay to do it even in a read-write function, if the
expression contains only immutable functions/operators. There would
only be a change of behavior if an "immutable" called function depends
on seeing database updates made during the current plpgsql function.
That's enough of a violation of the promise of immutability that anyone
who complains won't have much of a case.
The benefits are significant --- for simple cases like
while i < 10000000
loop
i := i + 1;
end loop;
I see net performance improvements around 45%. Of course, real-world
cases won't get that much faster, but it ought to be noticeable.
At the very least, this removes much of the performance penalty that
used to exist for forgetting to mark a plpgsql function non-volatile.
Konstantin Knizhnik, reviewed by Pavel Stehule, cosmetic changes by me
Discussion: https://postgr.es/m/ed9da20e-01aa-d04b-d085-e6c16b14b9d7@postgrespro.ru
It seems pretty unacceptable to have no regression test coverage
for this aspect of the logical replication protocol, especially
given the bugs we've found in related code.
Discussion: https://postgr.es/m/16129-a0c0f48e71741e5f@postgresql.org
slot_modify_cstrings seriously abused the TupleTableSlot API by relying
on a slot's underlying data to stay valid across ExecClearTuple. Since
this abuse was also quite undocumented, it's little surprise that the
case got broken during the v12 slot rewrites. As reported in bug #16129
from Ondřej Jirman, this could lead to crashes or data corruption when
a logical replication subscriber processes a row update. Problems would
only arise if the subscriber's table contained columns of pass-by-ref
types that were not being copied from the publisher.
Fix by explicitly copying the datum/isnull arrays from the source slot
that the old row was in already. This ends up being about the same
thing that happened pre-v12, but hopefully in a less opaque and
fragile way.
We might've caught the problem sooner if there were any test cases
dealing with updates involving non-replicated or dropped columns.
Now there are.
Back-patch to v10 where this code came in. Even though the failure
does not manifest before v12, IMO this code is too fragile to leave
as-is. In any case we certainly want the additional test coverage.
Patch by me; thanks to Tomas Vondra for initial investigation.
Discussion: https://postgr.es/m/16129-a0c0f48e71741e5f@postgresql.org
Support has been removed as of 5ded4bd, but code related to the tutorial
still used it. Functions using version-1 are already present for some
time in the tutorial, and the documentation mentions them, so just
replace the old version with the new one.
Reported-by: Pavel Stehule
Analyzed-by: Euler Taveira
Author: Michael Paquier
Reviewed-by: Tom Lane, Pavel Stehule
Discussion: https://postgr.es/m/CAFj8pRCgC2uDzrw-vvanXu6Z3ofyviEOQPEpH6_aL4OCe7JRag@mail.gmail.com
While a self-referential view doesn't actually work, it's possible
to create one, and it turns out that this breaks some of the
information_schema views. Those views call relation_is_updatable(),
which neglected to consider the hazards of being recursive. In
older PG versions you get a "stack depth limit exceeded" error,
but since v10 it'd recurse to the point of stack overrun and crash,
because commit a4c35ea1c took out the expression_returns_set() call
that was incidentally checking the stack depth.
Since this function is only used by information_schema views, it
seems like it'd be better to return "not updatable" than suffer
an error. Hence, add tracking of what views we're examining,
in just the same way that the nearby fireRIRrules() code detects
self-referential views. I added a check_stack_depth() call too,
just to be defensive.
Per private report from Manuel Rigger. Back-patch to all
supported versions.
Oversight in commit e6d8069522. Since that commit changed the format of
XLOG_DBASE_DROP WAL record, XLOG_PAGE_MAGIC needs to be bumped.
Spotted by Michael Paquier
Previously DROP DATABASE generated as many XLOG_DBASE_DROP WAL records
as the number of tablespaces that the database to drop uses. This caused
the scans of shared_buffers as many times as the number of the tablespaces
during recovery because WAL replay of one XLOG_DBASE_DROP record needs
that full scan. This could make the recovery time longer especially
when shared_buffers is large.
This commit changes DROP DATABASE so that it generates only one
XLOG_DBASE_DROP record, and registers the information of all the tablespaces
into it. Then, WAL replay of XLOG_DBASE_DROP record needs full scan of
shared_buffers only once, and which may improve the recovery performance.
Author: Fujii Masao
Reviewed-by: Kirk Jamison, Simon Riggs
Discussion: https://postgr.es/m/CAHGQGwF8YwNH0ZaL+2wjZPkj+ji9UhC+Z4ScnG97WKtVY5L9iw@mail.gmail.com
This adds the statistics about transactions spilled to disk from
ReorderBuffer. Users can query the pg_stat_replication view to check
these stats.
Author: Tomas Vondra, with bug-fixes and minor changes by Dilip Kumar
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/688b0b7f-2f6c-d827-c27b-216a8e3ea700@2ndquadrant.com
Trying to use hypothetical indexes with BRIN currently fails when trying
to access a relation that does not exist when looking for the
statistics. With the current API, it is not possible to easily pass
a value for pages_per_range down to the hypothetical index, so this
makes use of the default value of BRIN_DEFAULT_PAGES_PER_RANGE, which
should be fine enough in most cases.
Being able to refine or enforce the hypothetical costs in more
optimistic ways would require more refactoring by filling in the
statistics when building IndexOptInfo in plancat.c. This would involve
ABI breakages around the costing routines, something not fit for stable
branches.
This is broken since 7e534ad, so backpatch down to v10.
Author: Julien Rouhaud, Heikki Linnakangas
Reviewed-by: Álvaro Herrera, Tom Lane, Michael Paquier
Discussion: https://postgr.es/m/CAOBaU_ZH0LKEA8VFCocr6Lpte1ab0b6FpvgS0y4way+RPSXfYg@mail.gmail.com
Backpatch-through: 10
Make patternsel_common() select the comparison operators to use with
hardwired logic that matches pattern_prefix()'s new logic, eliminating
its dependencies on particular index opfamilies.
This shouldn't change any behavior, as it's just replacing runtime
operator lookups with the same values hard-wired. But it makes these
closely-related functions look more alike, and saving some runtime
syscache lookups is worth something.
Actually, it's not quite true that this is zero behavioral change:
when estimating for a column of type "name", the comparison constant
will be kept as "text" not coerced to "name". But that's more correct
anyway, and it allows additional simplification of the coercion logic,
again syncing this more closely with pattern_prefix().
Per consideration of a report from Manuel Rigger.
Discussion: https://postgr.es/m/CA+u7OA7nnGYy8rY0vdTe811NuA+Frr9nbcBO9u2Z+JxqNaud+g@mail.gmail.com
Historically, the planner's LIKE/regex index optimizations were only
carried out for specific index opfamilies. That's never been a great
idea from the standpoint of extensibility, but it didn't matter so
much as long as we had no practical way to extend such behaviors anyway.
With the addition of planner support functions, and in view of ongoing
work to support additional table and index AMs, it seems like a good
time to relax this.
Hence, recast the decisions in match_pattern_prefix() so that rather
than decide which operators to generate by looking at what the index
opfamily contains, we decide which operators to generate a-priori
and then see if the opfamily supports them. This is much more
defensible from a semantic standpoint anyway, since we know the
semantics of the chosen operators precisely, and we only need to
assume that the opfamily correctly implements operators it claims
to support.
The existing "pattern" opfamilies put a crimp in this approach, since
we need to select the pattern operators if we want those to work.
So we still have to special-case those opfamilies. But that seems
all right, since in view of the addition of collations, the pattern
opfamilies seem like a legacy hack that nobody will be building on.
The only immediate effect of this change, so far as the core code is
concerned, is that anchored LIKE/regex patterns can be mapped onto
BRIN index searches, and exact-match patterns can be mapped onto hash
indexes, not only btree and spgist indexes as before. That's not a
terribly exciting result, but it does fix an omission mentioned in
the ancient comments here.
Note: no catversion bump, even though this touches pg_operator.dat,
because it's only adding OID macros not changing the contents of
postgres.bki.
Per consideration of a report from Manuel Rigger.
Discussion: https://postgr.es/m/CA+u7OA7nnGYy8rY0vdTe811NuA+Frr9nbcBO9u2Z+JxqNaud+g@mail.gmail.com
When ReadFile() encounters the end of a file while reading from
a synchronous handle with an offset provided via OVERLAPPED, it
reports an error instead of returning 0. By not handling that
(undocumented) result correctly, we caused some noisy LOG
messages about an unknown error code. Repair.
Back-patch to 12, where we started using pread()/ReadFile() with
an offset.
Reported-by: ZhenHua Cai, Amit Kapila
Diagnosed-by: Juan Jose Santamaria Flecha
Tested-by: Amit Kapila
Discussion: https://postgr.es/m/CAA4eK1LK3%2BWRtpz68TiRdpHwxxWm%3D%2Bt1BMf-G68hhQsAQ41PZg%40mail.gmail.com
Specifying '-f' will add the 'force' option to the DROP DATABASE command
sent to the server. This will try to terminate all existing connections
to the target database before dropping it.
Author: Pavel Stehule
Reviewed-by: Vignesh C and Amit Kapila
Discussion: https://postgr.es/m/CAP_rwwmLJJbn70vLOZFpxGw3XD7nLB_7+NKz46H5EOO2k5H7OQ@mail.gmail.com
The planner's optimization code for LIKE and regex operators could
error out with a complaint like "no = operator for opfamily NNN"
if someone created a binary-compatible index (for example, a
bpchar_ops index on a text column) on the LIKE's left argument.
This is a consequence of careless refactoring in commit 74dfe58a5.
The old code in match_special_index_operator only accepted specific
combinations of the pattern operator and the index opclass, thereby
indirectly guaranteeing that the opclass would have a comparison
operator with the same LHS input type as the pattern operator.
While moving the logic out to a planner support function, I simplified
that test in a way that no longer guarantees that. Really though we'd
like an altogether weaker dependency on the opclass, so rather than
put back exactly the old code, just allow lookup failure. I have in
mind now to rewrite this logic completely, but this is the minimum
change needed to fix the bug in v12.
Per report from Manuel Rigger. Back-patch to v12 where the mistake
came in.
Discussion: https://postgr.es/m/CA+u7OA7nnGYy8rY0vdTe811NuA+Frr9nbcBO9u2Z+JxqNaud+g@mail.gmail.com
By oversight 52ac6cd2d0 makes ginDeletePage() sets pd_prune_xid of page to be
deleted before entering the critical section. It appears that only versions 11
and later were affected by this oversight.
Backpatch-through: 11
We find GIN concurrency bugs from time to time. One of the problems here is
that concurrency of GIN isn't well-documented in README. So, it might be even
hard to distinguish design bugs from implementation bugs.
This commit revised concurrency section in GIN README providing more details.
Some examples are illustrated in ASCII art.
Also, this commit add the explanation of how is tuple layout in internal GIN
B-tree page different in comparison with nbtree.
Discussion: https://postgr.es/m/CAPpHfduXR_ywyaVN4%2BOYEGaw%3DcPLzWX6RxYLBncKw8de9vOkqw%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Peter Geoghegan
Backpatch-through: 9.4
Current GIN code appears to don't handle traversing to the deleted page via
downlink. This commit fixes that by stepping right from the delete page like
we do in nbtree.
This commit also fixes setting 'deleted' flag to the GIN pages. Now other page
flags are not erased once page is deleted. That helps to keep our assertions
true if we arrive deleted page via downlink.
Discussion: https://postgr.es/m/CAPpHfdvMvsw-NcE5bRS7R1BbvA4BxoDnVVjkXC5W0Czvy9LVrg%40mail.gmail.com
Author: Alexander Korotkov
Reviewed-by: Peter Geoghegan
Backpatch-through: 9.4
When ginDeletePage() is about to delete page it locks its left sibling to revise
the rightlink. So, it locks pages in right to left manner. Int he same time
ginStepRight() locks pages in left to right manner, and that could cause a
deadlock.
This commit makes ginScanToDelete() keep exclusive lock on left siblings of
currently investigated path. That elimites need to relock left sibling in
ginDeletePage(). Thus, deadlock with ginStepRight() can't happen anymore.
Reported-by: Chen Huajun
Discussion: https://postgr.es/m/5c332bd1.87b6.16d7c17aa98.Coremail.chjischj%40163.com
Author: Alexander Korotkov
Reviewed-by: Peter Geoghegan
Backpatch-through: 10
