On Linux, we call posix_fallocate() on shm_open()'d memory to avoid
later potential SIGBUS (see commit 899bd785).
Based on field reports of systems stuck in an EINTR retry loop there,
there, we made it possible to break out of that loop via slightly odd
coding where the CHECK_FOR_INTERRUPTS() call was somewhat removed from
the loop (see commit 422952ee).
On further reflection, that was not a great choice for at least two
reasons:
1. If interrupts were held, the CHECK_FOR_INTERRUPTS() would do nothing
and the EINTR error would be surfaced to the user.
2. If EINTR was reported but neither QueryCancelPending nor
ProcDiePending was set, then we'd dutifully retry, but with a bit more
understanding of how posix_fallocate() works, it's now clear that you
can get into a loop that never terminates. posix_fallocate() is not a
function that can do some of the job and tell you about progress if it's
interrupted, it has to undo what it's done so far and report EINTR, and
if signals keep arriving faster than it can complete (cf recovery
conflict signals), you're stuck.
Therefore, for now, we'll simply block most signals to guarantee
progress. SIGQUIT is not blocked (see InitPostmasterChild()), because
its expected handler doesn't return, and unblockable signals like
SIGCONT are not expected to arrive at a high rate. For good measure,
we'll include the ftruncate() call in the blocked region, and add a
retry loop.
Back-patch to all supported releases.
Reported-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reported-by: Nicola Contu <nicola.contu@gmail.com>
Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220701154105.jjfutmngoedgiad3%40alvherre.pgsql
Commit 054325c5ee created a memory leak in PQsendQueryInternal in case
an error occurs while sending the message. Repair.
Backpatch to 14, like that commit. Reported by Coverity.
When checking for interleaved partitions, we mark the partition as
interleaved when;
1. we find an earlier partition index when looping over the
sorted-by-Datum indexes[] array, or;
2. we find that the NULL partition allows some non-NULL Datum value.
In the code, as it was written in db632fbca we'll continue to check for
case 2 when we've already marked the partition as interleaved for case 1.
Here we make it so we don't bother marking the partition as interleaved
for case 2 when it's already been marked due to case 1.
Really all this saves is a useless call to bms_add_member(), but since
this code is new to PG15, it seems worth fixing it now to save anyone the
trouble of complaining at some time in the future. We have the
opportunity to improve this now before PG15 is out. This might ease some
future back-patching pain.
Per report and patch by Zhihong Yu. However, I slightly revised the
comments and altered the bms_add_member() code to match in both locations.
We already know that index is equal to boundinfo->null_index from the if
condition.
Author: Zhihong Yu
Discussion: https://postgr.es/m/CALNJ-vQbZR0pYxz9zQ5bqXVcwtGgNgVupeEpNT65HZ+yWZnc4g@mail.gmail.com
Backpatch-through: 15, same as db632fbca.
There are a few things that we could do a little better within
get_cheapest_group_keys_order():
1. We should be using list_free() rather than pfree() on a List.
2. We should use for_each_from() instead of manually coding a for loop to
skip the first n elements of a List
3. list_truncate(list_copy(...), n) is not a great way to copy the first n
elements of a list. Let's invent list_copy_head() for that. That way we
don't need to copy the entire list just to truncate it directly
afterwards.
4. We can simplify finding the cheapest cost by setting the cheapest cost
variable to DBL_MAX. That allows us to skip special-casing the initial
iteration of the loop.
Author: David Rowley
Discussion: https://postgr.es/m/CAApHDvrGyL3ft8waEkncG9y5HDMu5TFFJB1paoTC8zi9YK97Nw@mail.gmail.com
Backpatch-through: 15, where get_cheapest_group_keys_order was added.
Justin Pryzby reported that some scenarios could cause gathering
of extended statistics to spend many seconds in an un-cancelable
qsort() operation. To fix, invent qsort_interruptible(), which is
just like qsort_arg() except that it will also do CHECK_FOR_INTERRUPTS
every so often. This bloats the backend by a couple of kB, which
seems like a good investment. (We considered just enabling
CHECK_FOR_INTERRUPTS in the existing qsort and qsort_arg functions,
but there are some callers for which that'd demonstrably be unsafe.
Opt-in seems like a better way.)
For now, just apply qsort_interruptible() in statistics collection.
There's probably more places where it could be useful, but we can
always change other call sites as we find problems.
Back-patch to v14. Before that we didn't have extended stats on
expressions, so that the problem was less severe. Also, this patch
depends on the sort_template infrastructure introduced in v14.
Tom Lane and Justin Pryzby
Discussion: https://postgr.es/m/20220509000108.GQ28830@telsasoft.com
dshash.c previously maintained flags to be able to assert that you
didn't hold any partition lock. These flags could get out of sync with
reality in error scenarios.
Get rid of all that, and make assertions about the locks themselves
instead. Since LWLockHeldByMe() loops internally, we don't want to put
that inside another loop over all partition locks. Introduce a new
debugging-only interface LWLockAnyHeldByMe() to avoid that.
This problem was noted by Tom and Andres while reviewing changes to
support the new shared memory stats system, and later showed up in
reality while working on commit 389869af.
Back-patch to 11, where dshash.c arrived.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Reviewed-by: Zhihong Yu <zyu@yugabyte.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220311012712.botrpsikaufzteyt@alap3.anarazel.de
Discussion: https://postgr.es/m/CA%2BhUKGJ31Wce6HJ7xnVTKWjFUWQZPBngxfJVx4q0E98pDr3kAw%40mail.gmail.com
This addresses two issues in the tests of test_oat_hooks:
- The role regress_test_user was being left behind, preventing the test
to succeed on repeated runs. It makes sense to leave some objects
behind to have more coverage for pg_upgrade (as does test_pg_dump), but
the role dropped here does not own any objects so there is no reason to
keep it.
- GRANT SET ON PARAMETER is issued, creating an entry in
pg_parameter_acl without cleaning up the entry created. This causes
an overlap with unsafe_tests as both use work_mem, making the latter
fail. This commit adds an extra REVOKE SET ON PARAMETER to clean the
contents of pg_parameter_acl, switching to maintenance_work_mem rather
than work_mem to avoid an overlap between both tests.
The tests of test_oat_hooks cannot use installcheck yet as these are
proving to be unstable with caching and the namespace search hooks, so
the issues fixed here cannot be reached yet, but they would be once the
hook issue is addressed and installcheck is allowed again in
test_oat_hooks.
Discussion: https://postgr.es/m/YrpVkADAY0knF6vM@paquier.xyz
Backpatch-through: 15
When you hit ^C, the terminal driver in Unix-like systems echoes "^C" as
well as sending an interrupt signal (depending on stty settings). At
least libedit (but maybe also libreadline) is then confused about the
current cursor location, and corrupts the display if you try to scroll
back. Fix, by moving to a new line before the next prompt is displayed.
Back-patch to all supported released.
Author: Pavel Stehule <pavel.stehule@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/3278793.1626198638%40sss.pgh.pa.us
Commit 9a974cbcba did this for user
tables, but pg_upgrade treats pg_largeobject as a user table, and so
needs the same treatment. Without this fix, if you rewrite the
pg_largeobject table and then perform an upgrade with pg_upgrade, the
table will apparently be empty on the new cluster, while all of your
objects will end up with an orphaned file.
With this fix, instead of the old cluster's pg_largeobject files ending
up orphaned, the original files fro the new cluster do. That's mostly
harmless because we expect the table to be empty, but we might want
to arrange to remove the as part of the upgrade. Since we're still
debating the best way of doing that, I (rhaas) have decided to postpone
dealing with that problem and get the basic fix committed.
Justin Pryzby, reviewed by Shruthi Gowda and by me.
Discussion: http://postgr.es/m/20220701231413.GI13040@telsasoft.com
These are documented to be the allowed types for the RETURNING clause,
but the restriction was not being enforced, which caused a segfault if
another type was specified. Add some testing for this.
Per report from a.kozhemyakin
Backpatch to release 15.
When locking a specific named relation for a FOR [KEY] UPDATE/SHARE
clause, transformLockingClause() finds the relation to lock by
scanning the rangetable for an RTE with a matching eref->aliasname.
However, it failed to account for the visibility rules of a join RTE.
If a join RTE doesn't have a user-supplied alias, it will have a
generated eref->aliasname of "unnamed_join" that is not visible as a
relation name in the parse namespace. Such an RTE needs to be skipped,
otherwise it might be found in preference to a regular base relation
with a user-supplied alias of "unnamed_join", preventing it from being
locked.
In addition, if a join RTE doesn't have a user-supplied alias, but
does have a join_using_alias, then the RTE needs to be matched using
that alias rather than the generated eref->aliasname, otherwise a
misleading "relation not found" error will be reported rather than a
"join cannot be locked" error.
Backpatch all the way, except for the second part which only goes back
to 14, where JOIN USING aliases were added.
Dean Rasheed, reviewed by Tom Lane.
Discussion: https://postgr.es/m/CAEZATCUY_KOBnqxbTSPf=7fz9HWPnZ5Xgb9SwYzZ8rFXe7nb=w@mail.gmail.com
By convention, the tab-complete subscription parameters are listed in the
COMPLETE_WITH lists in alphabetical order, but when the "disable_on_error"
parameter was introduced this was not done.
This patch just tidies that up.
Reported-by: Peter Smith
Author: Peter Smith
Reviewed-by: Euler Taveira, Takamichi Osumi
Backpatch-through: 15, where it was introduced
Discussion: https://postgr.es/m/CAHut+PucvKZgg_eJzUW--iL6DXHg1Jwj6F09tQziE3kUF67uLg@mail.gmail.com
That comment might have been true at some point during development, but
definitely isn't anymore.
Reported-By: Melanie Plageman <melanieplageman@gmail.com>
Backpatch: 15-
We hadn't noticed this because it's dead code: there is no
situation where we read raw parse trees from text format.
So maybe the right fix is to remove the function altogether,
but I'll forbear for now; it's not the only dead code in
readfuncs.c, I think.
Noted while comparing existing code to the results of
Peter's auto-generation script.
40af10b57 changed things so we make use of a generation memory context for
storing tuples to be sorted by tuplesort.c. That change does not play
nicely with the changes made in 9f03ca915 (back in 2014). That commit
changed things so that index_form_tuple() is called while switched into
the tuplestore's tuplecontext. In order to fetch the tuple from the index,
index_form_tuple() must do various memory allocations which are unrelated
to the storage of the final returned tuple. Although all of these
allocations are pfree'd, the fact that we now use a generation context
means that the memory for these pfree'd allocations won't be used again by
any other allocation due to generation.c's lack of freelists. This could
result in sorts used for building indexes exceeding maintenance_work_mem
by a very large amount.
Here we fix it so we no longer allocate anything apart from the tuple
itself into the generation context by adding a new version of
index_form_tuple named index_form_tuple_context, which can be called to
specify the MemoryContext to allocate the tuple into.
Discussion: https://postgr.es/m/CAApHDvrHQkiFRHiGiAS-LMOvJN-eK-s762=tVzBz8ZqUea-a_A@mail.gmail.com
Backpatch-through: 15, where 40af10b57 was added.
There's no reason anymore to only drop subscription stats if associated with a
slot, now that stats drops are transactional. Additionally, the comment
referring to autovacuum cleaning up stats was clearly outdated.
Author: Masahiko Sawada <sawada.mshk@gmail.com>
Discussion: https://postgr.es/m/CAD21AoAwiby3HeJE7vJe16Gr75RFfJ640dyHqvsiUhyKJTXPtw@mail.gmail.com
Backpatch: 15-
50e17ad28 increased the size of ExprEvalStep from 64 bytes up to 88 bytes.
Lots of effort was spent during the development of the current expression
evaluation code to make an instance of this struct as small as possible.
Making this struct larger than needed reduces CPU cache efficiency during
expression evaluation which causes noticeable slowdowns during query
execution.
In order to reduce the size of the struct, here we remove the fn_addr
field. The values from this field can be obtained via fcinfo, just with
some extra pointer dereferencing. The extra indirection does not seem to
cause any noticeable slowdowns.
Various other fields have been moved into the ScalarArrayOpExprHashTable
struct. These fields are only used when the ScalarArrayOpExprHashTable
pointer has already been dereferenced, so no additional pointer
dereferences occur for these. Here we also make hash_fcinfo_data the last
field in ScalarArrayOpExprHashTable so that we can avoid a further pointer
dereference to get the FunctionCallInfoBaseData. This also saves a call to
palloc().
50e17ad28 was added in 14, but it's too late to adjust the size of the
ExprEvalStep in that version, so here we just backpatch to 15, which is
currently in beta.
Author: Andres Freund, David Rowley
Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
Backpatch-through: 15
Previously the timer was enabled whenever there were any pending stats after
executing a statement, just to then be disabled again when not idle
anymore. That lead to an increase in GetCurrentTimestamp() calls from within
timeout.c compared to 14.
To avoid that increase, leave the timer enabled until stats are reported,
rather than until idle. The timer is only disabled once the pending stats have
been reported.
For me this fixes the increase in GetCurrentTimestamp() calls, there now are
fewer calls in 15 than in 14, in the previously slowed down workload.
While at it, also update assertion in pgstat_report_stat() to be more precise.
Author: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
Backpatch: 15-
The new expression step types increased the size of ExprEvalStep by ~4 for all
types of expression steps, slowing down expression evaluation noticeably. Move
them out of line.
There's other issues with these expression steps, but addressing them is
largely independent of this aspect.
Author: Andres Freund <andres@anarazel.de>
Reviewed-By: Andrew Dunstan <andrew@dunslane.net>
Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
Backpatch: 15-
This reverts most of 91c0570a79, f28bf667f6, fe0972ee5e, afdeff1052. The
only thing left is the retry loop in 019_replslot_limit.pl that avoids
spurious failures by retrying a couple times.
We haven't seen any hard evidence that this is caused by anything but slow
process shutdown. We did not find any cases where walsenders did not vanish
after waiting for longer. Therefore there's no reason for this debugging code
to remain.
Discussion: https://postgr.es/m/20220530190155.47wr3x2prdwyciah@alap3.anarazel.de
Backpatch: 15-
When we changed some built-in functions to use anycompatiblearray
instead of anyarray, we created a dump/restore hazard for user-defined
operators and aggregates relying on those functions: the user objects
have to be modified to change their signatures similarly. This causes
pg_upgrade to fail partway through if the source installation contains
such objects. We generally try to have pg_upgrade detect such hazards
and fail before it does anything exciting, so add logic to detect
this case too.
Back-patch to v14 where the change was made.
Justin Pryzby, reviewed by Andrey Borodin
Discussion: https://postgr.es/m/3383880.QJadu78ljV@vejsadalnx
Noted while comparing existing code to the output of the proposed
patch to automate creation of these functions. Some of the changes
are just cosmetic, but others represent real bugs. I've not
attempted to analyze the user-visible impact.
Back-patch to v15 where this code came in.
Discussion: https://postgr.es/m/1794155.1656984188@sss.pgh.pa.us
We were going into IDLE state too soon when executing queries via
PQsendQuery in pipeline mode, causing several scenarios to misbehave in
different ways -- most notably, as reported by Daniele Varrazzo, that a
warning message is produced by libpq:
message type 0x33 arrived from server while idle
But it is also possible, if queries are sent and results consumed not in
lockstep, for the expected mediating NULL result values from PQgetResult
to be lost (a problem which has not been reported, but which is more
serious).
Fix this by introducing two new concepts: one is a command queue element
PGQUERY_CLOSE to tell libpq to wait for the CloseComplete server
response to the Close message that is sent by PQsendQuery. Because the
application is not expecting any PGresult from this, the mechanism to
consume it is a bit hackish.
The other concept, authored by Horiguchi-san, is a PGASYNC_PIPELINE_IDLE
state for libpq's state machine to differentiate "really idle" from
merely "the idle state that occurs in between reading results from the
server for elements in the pipeline". This makes libpq not go fully
IDLE when the libpq command queue contains entries; in normal cases, we
only go IDLE once at the end of the pipeline, when the server response
to the final SYNC message is received. (However, there are corner cases
it doesn't fix, such as terminating the query sequence by
PQsendFlushRequest instead of PQpipelineSync; this sort of scenario is
what requires PGQUERY_CLOSE bit above.)
This last bit helps make the libpq state machine clearer; in particular
we can get rid of an ugly hack in pqParseInput3 to avoid considering
IDLE as such when the command queue contains entries.
A new test mode is added to libpq_pipeline.c to tickle some related
problematic cases.
Reported-by: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Co-authored-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CA+mi_8bvD0_CW3sumgwPvWdNzXY32itoG_16tDYRu_1S2gV2iw@mail.gmail.com
Several buildfarm members are failing the pg_upgrade test in
REL_15_STABLE, though the identical test is fine in HEAD.
On thorntail it's possible to see that the problem is an
overlength socket path name, and I bet the same is true
on the others.
The normally-started postmasters used in the test are already
set up with short socket directory paths, but we neglected to
tell pg_upgrade itself to do likewise when starting child
postmasters, and indeed it seems to be explicitly selecting
the test directory instead.
Back-patch to v15 where the current test script was introduced.
(The previous script might have the same issue, because I don't
see any use of -s/--socketdir in it either; but we've had no
complaints, so leave it alone for now.)
Discussion: https://postgr.es/m/1410025.1656890531@sss.pgh.pa.us
None of the other bison parsers contains this directive, and it gives
rise to some unfortunate and impenetrable messages, so just remove it.
Backpatch to release 12, where it was introduced.
Per gripe from Erik Rijkers
Discussion: https://postgr.es/m/ba069ce2-a98f-dc70-dc17-2ccf2a9bf7c7@xs4all.nl
After commit 662dbe2c8, psql tab completion didn't conveniently
support the case of "ALTER EXTENSION foo UPDATE". It'd always
add "TO", which is fine if you want to specify a target version
but not if you don't ... and surely the latter is the much more
common case.
To fix, remove "TO" from the initially offered completion; you now
need to press TAB one additional time to get that. We won't try to
duplicate the old behavior of attempting initial completion on the
target version along with TO. It's too squirrelly to get the quoting
right, and this is such an infrequent usage that it doesn't seem worth
expending a lot of effort and special code on.
Noted by Noah Misch. Back-patch to v15.
Discussion: https://postgr.es/m/20220703083217.GB2476530@rfd.leadboat.com
Per buildfarm member prairiedog, this platform rejects uninitialized
global variables in shared libraries. Back-patch to v10, like the
addition of the variable.
Reviewed by Tom Lane.
Discussion: https://postgr.es/m/20220703030619.GB2378460@rfd.leadboat.com
ecpglib has been calling it once per SQL query and once per EXEC SQL GET
DESCRIPTOR. Instead, if newlocale() has not succeeded before, call it
while establishing a connection. This mitigates three problems:
- If newlocale() failed in EXEC SQL GET DESCRIPTOR, the command silently
proceeded without the intended locale change.
- On AIX, each newlocale()+freelocale() cycle leaked memory.
- newlocale() CPU usage may have been nontrivial.
Fail the connection attempt if newlocale() fails. Rearrange
ecpg_do_prologue() to validate the connection before its uselocale().
The sort of program that may regress is one running in an environment
where newlocale() fails. If that program establishes connections
without running SQL statements, it will stop working in response to this
change. I'm betting against the importance of such an ECPG use case.
Most SQL execution (any using ECPGdo()) has long required newlocale()
success, so there's little a connection could do without newlocale().
Back-patch to v10 (all supported versions).
Reviewed by Tom Lane. Reported by Guillaume Lelarge.
Discussion: https://postgr.es/m/20220101074055.GA54621@rfd.leadboat.com
John Naylor