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
POSIX shm_open() can sleep for a long time and fail spuriously because
of contention on an internal lock file on Solaris (and presumably
illumos). Commit 389869af fixed the main problem with this, namely that
we could crash, but it's now clear that "posix" is not a good default.
Therefore, choose "sysv" at initdb time on Solaris and illumos. Other
choices are still available by editing the postgresql.conf file.
Back-patch only to 15, because contention is much less likely further
back, and it doesn't seem like a good idea to change this in released
branches. This should clear up the failures on build farm animal
margay.
Discussion: https://postgr.es/m/CA%2BhUKGKqKrCV5xKWfh9rnm%3Do%3DDwZLTLtnsj_XpUi9g5%3DV%2B9oyg%40mail.gmail.com
Previously, we trusted the OS not to report EEXIST unless we'd passed in
IPC_CREAT | IPC_EXCL or O_CREAT | O_EXCL, as appropriate. Solaris's
shm_open() can in fact do that, causing us to crash because we didn't
ereport and then we blithely assumed the mapping was successful.
Let's treat EEXIST just like any other error, unless we're actually
trying to create a new segment. This applies to shm_open(), where this
behavior has been seen, and also to the equivalent operations for our
sysv and mmap modes just on principle.
Based on the underlying reason for the error, namely contention on a
lock file managed by Solaris librt for each distinct name, this problem
is only likely to happen on 15 and later, because the new shared memory
stats system produces shm_open() calls for the same path from
potentially large numbers of backends concurrently during
authentication. Earlier releases only shared memory segments between a
small number of parallel workers under one Gather node. You could
probably hit it if you tried hard enough though, and we should have been
more defensive in the first place. Therefore, back-patch to all
supported releases.
Per build farm animal margay. This isn't the end of the story, though,
it just changes random crashes into random "File exists" errors; more
work needed for a green build farm.
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGKqKrCV5xKWfh9rnm%3Do%3DDwZLTLtnsj_XpUi9g5%3DV%2B9oyg%40mail.gmail.com
We have had a working and tunable autovacuum
for at least a decade now, so remove the recommendation to
manually vacuum tables at least every night.
Autovacuum is now also triggered by INSERTs, so we can also
remove the recommendation to run VACUUM (ANALYZE) after lots
of INSERTs or DELETEs.
Instead, suggest using autovacuum by moving the respective
paragraph up to where the importance of VACUUM is emphasized.
Author: Laurenz Albe <laurenz.albe@cybertec.at>
Reviewed-By: Magnus Hagander, Peter Geoghegan
Discussion: https://postgr.es/m/6f5e3da98fec14640f389d7b84c3b413833697f4.camel@cybertec.at
plpython_unicode_3.out was already removed a long time ago, so it
being listed here was very out of date.
plpython_types_3.out was removed with the Python 2 removal.
TransactionIdIsInProgress had a fast path to return 'false' if the
single-item CLOG cache said that the transaction was known to be
committed. However, that was wrong, because a transaction is first
marked as committed in the CLOG but doesn't become visible to others
until it has removed its XID from the proc array. That could lead to an
error:
ERROR: t_xmin is uncommitted in tuple to be updated
or for an UPDATE to go ahead without blocking, before the previous
UPDATE on the same row was made visible.
The window is usually very short, but synchronous replication makes it
much wider, because the wait for synchronous replica happens in that
window.
Another thing that makes it hard to hit is that it's hard to get such
a commit-in-progress transaction into the single item CLOG cache.
Normally, if you call TransactionIdIsInProgress on such a transaction,
it determines that the XID is in progress without checking the CLOG
and without populating the cache. One way to prime the cache is to
explicitly call pg_xact_status() on the XID. Another way is to use a
lot of subtransactions, so that the subxid cache in the proc array is
overflown, making TransactionIdIsInProgress rely on pg_subtrans and
CLOG checks.
This has been broken ever since it was introduced in 2008, but the race
condition is very hard to hit, especially without synchronous
replication. There were a couple of reports of the error starting from
summer 2021, but no one was able to find the root cause then.
TransactionIdIsKnownCompleted() is now unused. In 'master', remove it,
but I left it in place in backbranches in case it's used by extensions.
Also change pg_xact_status() to check TransactionIdIsInProgress().
Previously, it only checked the CLOG, and returned "committed" before
the transaction was actually made visible to other queries. Note that
this also means that you cannot use pg_xact_status() to reproduce the
bug anymore, even if the code wasn't fixed.
Report and analysis by Konstantin Knizhnik. Patch by Simon Riggs, with
the pg_xact_status() change added by me.
Author: Simon Riggs
Reviewed-by: Andres Freund
Discussion: https://www.postgresql.org/message-id/flat/4da7913d-398c-e2ad-d777-f752cf7f0bbb%40garret.ru
Previously, we encoded both NULL and the first byte at the base address
as 0. That confusion led to the assertion in commit e07d4ddc, which
failed when min_dynamic_shared_memory was used. Give them distinct
encodings, by switching to 1-based offsets for non-NULL pointers. Also
improve macro hygiene in passing (missing/misplaced parentheses), and
remove open-coded access to the raw offset value from freepage.c/h.
Although e07d4ddc was back-patched to 10, the only code that actually
makes use of relptr at the base address arrived in 84b1c63a, so no need
to back-patch further than 14 for now.
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/20220519193839.GT19626%40telsasoft.com
Commit 64919aaab made pull_up_simple_subquery set rte->subquery = NULL
after doing the deed, so that we don't waste cycles copying a
now-useless subquery tree around. This turns out to create a core dump
hazard in range_table_mutator, which supposes that that field is never
NULL. Apparently none of our own code invokes query_tree_mutator or
range_table_mutator on the top Query after subquery pullup; but it
wouldn't be surprising if outside code does, and anyway I'm working
on a v16 patch that will need it.
We can fix this cleanly by just getting rid of the special-case
handling of this field and treating it more like all the rest.
I think the special case might be left over from a time when
QTW_DONT_COPY_QUERY was the default behavior, but that was eons ago.
Thanks to Dean Rasheed for review.
Discussion: https://postgr.es/m/545569.1656107045@sss.pgh.pa.us
Since commit 6a2a70a02, we've used signalfd() to receive latch wakeups
when building with WAIT_USE_EPOLL (default for Linux and illumos), and
our traditional self-pipe when falling back to WAIT_USE_POLL (default
for other Unixes with neither epoll() nor kqueue()).
Unexplained hangs and kernel panics have been reported on illumos
systems, apparently linked to this use of signalfd(), leading illumos
users and build farm members to have to define WAIT_USE_POLL explicitly
as a work-around. A bug report exists at
https://www.illumos.org/issues/13700 but no fix is available yet.
Let's provide a way for illumos users to go back to self-pipes with
epoll(), like releases before 14, and choose that by default. No change
for Linux users. To help with development/debugging, macros
WAIT_USE_{EPOLL,POLL} and WAIT_USE_{SIGNALFD,SELF_PIPE} can be defined
explicitly to override the defaults.
Back-patch to 14, where we started using signalfd().
Reported-by: Japin Li <japinli@hotmail.com>
Reported-by: Olaf Bohlen <olbohlen@eenfach.de> (off-list)
Reviewed-by: Japin Li <japinli@hotmail.com>
Discussion: https://postgr.es/m/MEYP282MB1669C8D88F0997354C2313C1B6CA9%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Commit a117cebd638dd02e5c2e791c25e43745f233111b used the original userid
for ACL checks located directly in DefineIndex(), but it still adopted
the table owner userid for more ACL checks than intended. That broke
dump/reload of indexes that refer to an operator class, collation, or
exclusion operator in a schema other than "public" or "pg_catalog".
Back-patch to v10 (all supported versions), like the earlier commit.
Nathan Bossart and Noah Misch
Discussion: https://postgr.es/m/f8a4105f076544c180a87ef0c4822352@stmuk.bayern.de
This removes the need to drop unlogged relations in the src/test/regress
suite, like commit dec8ad367e46180f826d5b6dc820fbecba1b71d2 did.
Reviewed by Thomas Munro.
Discussion: https://postgr.es/m/39945.1650895508@sss.pgh.pa.us
The ssl test "IPv4 host with CIDR mask does not match" apparently has
a portability problem. Some operating systems don't reject the host
name specification "192.0.2.1/32" as an IP address, and that is then
later rejected when the SNI is set, which results in a different error
message that the test is supposed to verify.
The value of the test has been questioned in the discussion, and it
was suggested that removing it would be an acceptable fix, so that's
what this is doing.
Reported-by: Nazir Bilal Yavuz <byavuz81@gmail.com>
Bug: #17522
Discussion: https://www.postgresql.org/message-id/flat/17522-bfcd5c603b5f4daa%40postgresql.org
The test was not waiting for the subscriber's data synchronization to
happen after refreshing the publication on the subscriber side. This leads
subscriber's apply worker to skip applying the changes on the
corresponding relation which results in a test failure.
Reported-by: Hou Zhijie, as per buildfarm
Author: Hou Zhijie
Reviewed-by: Masahiko Sawada, Amit Kapila
Discussion: https://postgr.es/m/OS0PR01MB5716A69496A8E2F2E155DB8D94B59@OS0PR01MB5716.jpnprd01.prod.outlook.com
This patch documents that the initial data synchronization (tablesync) for
logical replication does not take into account the publication 'publish'
parameter when copying the existing table data.
Author: Peter Smith
Reviewed-by: Shi yu, Euler Taveira, Robert Haas, Amit Kapila
Discussion: https://postgr.es/m/CAHut+PtbfALjFpS2MkrvQ+wWQKByP7CNh9RtFta-r=BHEU3S3w@mail.gmail.com
It has been reported that PL/Tcl built on macOS with GCC >=11 crashes.
The reason is that there is a hash_search() function in the operating
system's libraries, and that ends up being called instead of the one
in postgres. This has something to do with how the linker resolves
references between the various possibilities it has been given, and
somehow something changed that it is now picking that one in this
configuration.
We found that removing the -lc from the link command line fixes this
problem. The -lc was introduced a long time ago in commit
e3909672f12e0ddf3e202b824fda068ad2195ef2, and we think the reasons
might be obsolete, so we decided that we'll try to just remove it and
see if any problems arise.
Discussion: https://www.postgresql.org/message-id/flat/a78c847a-4f79-9286-be99-e819e9e4139e%40enterprisedb.com
When rebuilding the relation mapping on subscribers, we were not releasing
the attribute mapping's memory which was no longer required.
The attribute mapping used in logical tuple conversion was refactored in
PG13 (by commit e1551f96e6) but we forgot to update the related code that
frees the attribute map.
Author: Hou Zhijie
Reviewed-by: Amit Langote, Amit Kapila, Shi yu
Backpatch-through: 10, where it was introduced
Discussion: https://postgr.es/m/OSZPR01MB6310F46CD425A967E4AEF736FDA49@OSZPR01MB6310.jpnprd01.prod.outlook.com
072132f0 used the attnum offset to access the raw_fields array when
checking that the attribute names of the header and of the relation
match, leading to incorrect results or even crashes if the attribute
numbers of a relation are changed, like on a dropped attribute. This
fixes the logic to use the correct attribute names for the header
matching requirements.
Also, this commit disallows HEADER MATCH in COPY TO as there is no
validation that can be done in this case.
The tests are expanded for HEADER MATCH with COPY FROM and dropped
columns, with cases where a relation has a dropped and re-added column,
as well as a reduced set of columns.
Author: Julien Rouhaud
Reviewed-by: Peter Eisentraut, Michael Paquier
Discussion: https://postgr.es/m/20220607154744.vvmitnqhyxrne5ms@jrouhaud
The macro is being applied to a TOAST pointer, not a varlena header.
Therefore the use of VARATT_IS_COMPRESSED() is wrong. We can check
VARATT_EXTERNAL_IS_COMPRESSED(), but then we don't need the length
check that follows.
Report and fix by Kyotaro Horiguchi.
Discussion: http://postgr.es/m/20220517.162719.1671558681467343711.horikyota.ntt@gmail.com
Second thoughts about 9cd43f6cb: given that we're staying bug-compatible
with the old behavior of using double not single quotes for extension
versions, we can simplify this completion code by pretending that
extension versions *are* identifiers, and not using VERBATIM. Then
_complete_from_query() will think that the query results are identifiers
in need of quoting, and we end up with the same behavior as before.
This doesn't work for Query_for_list_of_available_extension_versions_with_TO,
but let's just drop that: there is no other place where we handle
multi-keyword phrases that way, and it doesn't seem very desirable here
either. Handle completion of "UPDATE TO" in our more usual pattern.
Discussion: https://postgr.es/m/CAMkU=1yV+egSYrzWvbDY8VZ6bKEMrKbzxr-HTuiHi+wDgSUMgA@mail.gmail.com
We build the partition map entries on subscribers while applying the
changes for update/delete on partitions. The component relation in each
entry is closed after its use so we need to update it on successive use of
cache entries.
This problem was there since the original commit f1ac27bfda that
introduced this code but we didn't notice it till the recent commit
26b3455afa started to use the component relation of partition map cache
entry.
Reported-by: Tom Lane, as per buildfarm
Author: Amit Langote, Hou Zhijie
Reviewed-by: Amit Kapila, Shi Yu
Backpatch-through: 13, where it was introduced
Discussion: https://postgr.es/m/OSZPR01MB6310F46CD425A967E4AEF736FDA49@OSZPR01MB6310.jpnprd01.prod.outlook.com
In logical replication, we will check if the target table on the
subscriber is updatable by comparing the replica identity of the table on
the publisher with the table on the subscriber. When the target table is a
partitioned table, we only check its replica identity but not for the
partition tables. This leads to assertion failure while applying changes
for update/delete as we expect those to succeed only when the
corresponding partition table has a primary key or has a replica
identity defined.
Fix it by checking the replica identity of the partition table while
applying changes.
Reported-by: Shi Yu
Author: Shi Yu, Hou Zhijie
Reviewed-by: Amit Langote, Amit Kapila
Backpatch-through: 13, where it was introduced
Discussion: https://postgr.es/m/OSZPR01MB6310F46CD425A967E4AEF736FDA49@OSZPR01MB6310.jpnprd01.prod.outlook.com
In 02b8048ba I (tgl) got rid of the need for most tab-completion queries
to return pre-quoted identifiers. But I over-hastily removed the
quote_ident call from Query_for_list_of_available_extension_versions*
too; those still need it, because what is returned isn't an identifier
at all and will (almost?) always need quoting.
Arguably we should use quote_literal here instead. But quote_ident
works too and people may be used to that behavior, so stick with it.
In passing, fix inconsistent omission of schema-qualification in
Query_for_list_of_encodings. That's not a security issue per our
current guidelines, but it ought to be like the rest.
Jeff Janes
Discussion: https://postgr.es/m/CAMkU=1yV+egSYrzWvbDY8VZ6bKEMrKbzxr-HTuiHi+wDgSUMgA@mail.gmail.com
Three parameters have been using "int" rather than "integer" to describe
their type:
auth_delay.milliseconds
max_logical_replication_workers
pg_prewarm.autoprewarm_interval
This is inconsistent with any other integer GUCs listed in the docs
(148, as far as I can see).
Author: Peter Smith
Discussion: https://postgr.es/m/CAHut+Pv6X5T-veN2abUDUvBxZm+SSm-9otfi3LZPGyOc6u6hiA@mail.gmail.com
