| Age | Commit message (Collapse) | Author |
|---|
|
An array element equal to INT_MAX gave this code indigestion,
causing an infinite loop that surely ended in SIGSEGV. We fixed
some nearby problems awhile ago (cf 757c5182f) but missed this.
Report and diagnosis by Alexander Lakhin (bug #18273); patch by me
Discussion: https://postgr.es/m/18273-9a832d1da122600c@postgresql.org
|
|
This function reads directly a page from a relation, relying on
index_open() to open the index to read from. Unfortunately, this would
crash when using partitioned indexes, as these can be opened with
index_open() but they have no physical pages.
Alexander has fixed the module, while I have written the test.
Author: Alexander Lakhin, Michael Paquier
Discussion: https://postgr.es/m/18246-f4d9ff7cb3af77e6@postgresql.org
Backpatch-through: 12
|
|
As coded, the function relied on index_open() when opening an index
relation, allowing partitioned indexes to be processed by
pgstathashindex(). This was leading to a "could not open file" error
because partitioned indexes have no physical files, or to a crash with
an assertion failure (like on HEAD).
This issue is fixed by applying the same checks as the other stat
functions for indexes, with a lookup at both RELKIND_INDEX and the index
AM expected.
Author: Alexander Lakhin
Discussion: https://postgr.es/m/18246-f4d9ff7cb3af77e6@postgresql.org
Backpatch-through: 12
|
|
If an error is thrown after calling CreateWaitEventSet(), the memory
of a WaitEventSet is free'd as it's allocated in the short-lived
memory context, but the file descriptor (on epoll- or kqueue-based
systems) or handles (on Windows) that it contains are leaked.
Use PG_TRY-FINALLY to ensure it gets freed. (On master, I will apply a
better fix, using ResourceOwners to track the WaitEventSet, but that's
not backpatchable.)
The added test doesn't check for leaking resources, so it passed even
before this commit. But at least it covers the code path.
In the passing, fix misleading comment on what the 'nevents' argument
to WaitEventSetWait means.
Report by Alexander Lakhin, analysis and suggestion for the fix by Tom
Lane. Fixes bug #17828. Backpatch to v14 where async execution was
introduced, but master gets a different fix.
Discussion: https://www.postgresql.org/message-id/17828-122da8cba23236be@postgresql.org
Discussion: https://www.postgresql.org/message-id/472235.1678387869@sss.pgh.pa.us
|
|
4b8266415 added a precheck to pgrowlocks() to ensure the given object's
pg_class.relam is HEAP_TABLE_AM_OID, however, that check was put before
another check which was checking if the given object was a partitioned
table. Since the pg_class.relam is always InvalidOid for partitioned
tables, if pgrowlocks() was called passing a partitioned table, then the
"only heap AM is supported" error would be raised instead of the intended
error about the given object being a partitioned table.
Here we simply move the pg_class.relam check to after the check that
verifies that we are in fact working with a normal (non-partitioned)
table.
Reported-by: jian he
Discussion: https://postgr.es/m/CACJufxFaSp_WguFCf0X98951zFVX+dXFnF1mxAb-G3g1HiHOow@mail.gmail.com
Backpatch-through: 12, where 4b8266415 was introduced.
|
|
pgstatindex failed with ERRCODE_DATA_CORRUPTED, of the "can't-happen"
class XX. The other functions succeeded on an empty index; they might
have malfunctioned if the failed index build left torn I/O or other
complex state. Report an ERROR in statistics functions pgstatindex,
pgstatginindex, pgstathashindex, and pgstattuple. Report DEBUG1 and
skip all index I/O in maintenance functions brin_desummarize_range,
brin_summarize_new_values, brin_summarize_range, and
gin_clean_pending_list. Back-patch to v11 (all supported versions).
Discussion: https://postgr.es/m/20231001195309.a3@google.com
|
|
This prevents false-positive reports about "the first child of leftmost
target page is not leftmost of its level", "block %u is not leftmost"
and "left link/right link pair". They appeared if amcheck ran before
VACUUM cleaned things, after a cluster exited recovery between the
first-stage and second-stage WAL records of a deletion. Back-patch to
v11 (all supported versions).
Reviewed by Peter Geoghegan.
Discussion: https://postgr.es/m/20231005025232.c7.nmisch@google.com
|
|
Formerly, the value computed by leftmostvalue_interval() was a long
way short of the minimum possible interval value. As a result, an
index scan on a GIN index on an interval column with < or <= operators
would miss large negative interval values.
Fix by setting all fields of the leftmost interval to their minimum
values, ensuring that the result is less than any other possible
interval. Since this only affects index searches, no index rebuild is
necessary.
Back-patch to all supported branches.
Dean Rasheed, reviewed by Heikki Linnakangas.
Discussion: https://postgr.es/m/CAEZATCV80%2BgOfF8ehNUUfaKBZgZMDfCfL-g1HhWGb6kC3rpDfw%40mail.gmail.com
|
|
Under interval_ops, some equal values are distinguishable. One such
pair is '24:00:00' and '1 day'. With that being so, btequalimage()
breaches the documented contract for the "equalimage" btree support
function. This can cause incorrect results from index-only scans.
Users should REINDEX any btree indexes having interval-type columns.
After updating, pg_amcheck will report an error for almost all such
indexes. This fix makes interval_ops simply omit the support function,
like numeric_ops does. Back-pack to v13, where btequalimage() first
appeared. In back branches, for the benefit of old catalog content,
btequalimage() code will return false for type "interval". Going
forward, back-branch initdb will include the catalog change.
Reviewed by Peter Geoghegan.
Discussion: https://postgr.es/m/20231011013317.22.nmisch@google.com
|
|
As coded, the module's Makefile would fail to set a value for PYTHON as
it checked if the variable is defined. When compiling without
--with-python, PYTHON is defined and set to an empty value, so the
existing check is not able to do its work.
This commit switches the rule to check if the value is empty rather than
defined, allowing the generation of unaccent.rules even if --with-python
is not used as long as "python" exists. BISON and FLEX do the same in
pgxs.mk, for instance.
Thinko in f85a485f89e2.
Author: Japin Li
Discussion: https://postgr.es/m/MEYP282MB1669F86C0DC7B4DC48489CB0B6C3A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Backpatch-through: 13
|
|
Yet another bug in the ilk of commits a7ee7c851 and 741b88435. In
741b88435, we took care to clear the memorized location of the
downlink when we split the parent page, because splitting the parent
page can move the downlink. But we missed that even *updating* a tuple
on the parent can move it, because updating a tuple on a gist page is
implemented as a delete+insert, so the updated tuple gets moved to the
end of the page.
This commit fixes the bug in two different ways (belt and suspenders):
1. Clear the downlink when we update a tuple on the parent page, even
if it's not split. This the same approach as in commits a7ee7c851
and 741b88435.
I also noticed that gistFindCorrectParent did not clear the
'downlinkoffnum' when it stepped to the right sibling. Fix that
too, as it seems like a clear bug even though I haven't been able
to find a test case to hit that.
2. Change gistFindCorrectParent so that it treats 'downlinkoffnum'
merely as a hint. It now always first checks if the downlink is
still at that location, and if not, it scans the page like before.
That's more robust if there are still more cases where we fail to
clear 'downlinkoffnum' that we haven't yet uncovered. With this,
it's no longer necessary to meticulously clear 'downlinkoffnum',
so this makes the previous fixes unnecessary, but I didn't revert
them because it still seems nice to clear it when we know that the
downlink has moved.
Also add the test case using the same test data that Alexander
posted. I tried to reduce it to a smaller test, and I also tried to
reproduce this with different test data, but I was not able to, so
let's just include what we have.
Backpatch to v12, like the previous fixes.
Reported-by: Alexander Lakhin
Discussion: https://www.postgresql.org/message-id/18129-caca016eaf0c3702@postgresql.org
|
|
Commit e4106b252 should have updated this test, but did not; back-patch
to all supported branches.
Reviewed by Richard Guo.
Discussion: http://postgr.es/m/CAPmGK15nR0NXLSCKQAcqbZbTzrzd5MozowWnTnGfPkayndF43Q%40mail.gmail.com
|
|
Commit e7cb7ee14, which introduced the infrastructure for FDWs and
custom scan providers to replace joins with scans, failed to add support
handling of pseudoconstant quals assigned to replaced joins in
createplan.c, leading to an incorrect plan without a gating Result node
when postgres_fdw replaced a join with such a qual.
To fix, we could add the support by 1) modifying the ForeignPath and
CustomPath structs to store the list of RestrictInfo nodes to apply to
the join, as in JoinPaths, if they represent foreign and custom scans
replacing a join with a scan, and by 2) modifying create_scan_plan() in
createplan.c to use that list in that case, instead of the
baserestrictinfo list, to get pseudoconstant quals assigned to the join;
but #1 would cause an ABI break. So fix by modifying the infrastructure
to just disallow replacing joins with such quals.
Back-patch to all supported branches.
Reported by Nishant Sharma. Patch by me, reviewed by Nishant Sharma and
Richard Guo.
Discussion: https://postgr.es/m/CADrsxdbcN1vejBaf8a%2BQhrZY5PXL-04mCd4GDu6qm6FigDZd6Q%40mail.gmail.com
|
|
Some of the test_decoding test output was extremely wide, because it
deals with massive toasted values, and the aligned mode causes psql to
produce 200kB of whitespace and dashes. Change to unaligned mode
temporarily to avoid that behavior.
Backpatch to 14, where it applies cleanly.
Discussion: https://postgr.es/m/20230405103953.sxleixp3uz5lazst@alvherre.pgsql
|
|
GiST compress functions (like all GiST opclass functions) are
supposed to be called in short-lived memory contexts, so that
minor memory leaks in them are not of concern, and indeed
explicit pfree's are likely slightly counterproductive.
But this one in g_intbig_compress() is more than
slightly counterproductive, because it's guarded by
"if (in != DatumGetArrayTypeP(entry->key))" which means
that if this test succeeds, we've detoasted the datum twice.
(And to add insult to injury, the extra detoast result is
leaked.) Let's just drop the whole stanza, relying on the
GiST temporary context mechanism to clean up in good time.
The analogous bit in g_int_compress() is
if (r != (ArrayType *) DatumGetPointer(entry->key))
pfree(r);
which doesn't have the gratuitous-detoast problem so
I left it alone. Perhaps there is a case for removing
unnecessary pfree's more widely, but I'm not sure if it's
worth the code churn.
The potential extra decompress seems expensive enough to
justify calling this a (minor) performance bug and
back-patching.
Konstantin Knizhnik, Matthias van de Meent, Tom Lane
Discussion: https://postgr.es/m/CAEze2Wi86=DxErfvf+SCB2UKmU2amKOF60BKuJOX=w-RojRn0A@mail.gmail.com
|
|
The test inserted 70k rows into a foreign table, in order to verify
correct behavior with more than 65535 parameters, and was added in
response to a bug report.
However, this is rather expensive, especially when running the tests
under valgrind, CLOBBER_CACHE_ALWAYS etc. It doesn't seem worth it to
keep running the test, so remove it from all branches (14+).
Backpatch-through: 14
Discussion: https://postgr.es/m/2131017.1623451468@sss.pgh.pa.us
|
|
This should have been part of dc73db6, but it got lost in the mix.
Oversight in 6b4d23f.
Author: Japin Li
Discussion: https://postgr.es/m/MEYP282MB1669FC91C764E277821936D3B624A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Backpatch-through: 14
|
|
Oversight in 6b4d23f.
Author: Japin Li, Richard Guo
Discussion: https://postgr.es/m/MEYP282MB1669FC91C764E277821936D3B624A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
Backpatch-through: 14
|
|
Author: Japin Li
Discussion: https://www.postgresql.org/message-id/MEYP282MB16696317B5DA7D0D92306149B627A@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
|
|
As gist__int_ops stands in intarray, it is possible to store GiST
entries for leaf pages that can cause corruptions when decompressed.
Leaf nodes are stored as decompressed all the time by the compression
method, and the decompression method should map with that, retrieving
the contents of the page without doing any decompression. However, the
code authorized the insertion of leaf page data with a higher number of
array items than what can be supported, generating a NOTICE message to
inform about this matter (199 for a 8k page, for reference). When
calling the decompression method, a decompression would be attempted on
this leaf node item but the contents should be retrieved as they are.
The NOTICE message generated when dealing with the compression of a leaf
page and too many elements in the input array for gist__int_ops has been
introduced by 08ee64e, removing the marker stored in the array to track
if this is actually a leaf node. However, it also missed the fact that
the decompression path should do nothing for a leaf page. Hence, as the
code stand, a too-large array would be stored as uncompressed but the
decompression path would attempt a decompression rather that retrieving
the contents as they are.
This leads to various problems. First, even if 08ee64e tried to address
that, it is possible to do out-of-bound chunk writes with a large input
array, with the backend informing about that with WARNINGs. On
decompression, retrieving the stored leaf data would lead to incorrect
memory reads, leading to crashes or even worse.
Perhaps somebody would be interested in expanding the number of array
items that can be handled in a leaf page for this operator in the
future, which would require revisiting the choice done in 08ee64e, but
based on the lack of reports about this problem since 2005 it does not
look so. For now, this commit prevents the insertion of data for leaf
pages when using more array items that the code can handle on
decompression, switching the NOTICE message to an ERROR. If one wishes
to use more array items, gist__intbig_ops is an optional choice.
While on it, use ERRCODE_PROGRAM_LIMIT_EXCEEDED as error code when a
limit is reached, because that's what the module is facing in such
cases.
Author: Ankit Kumar Pandey, Alexander Lakhin
Reviewed-by: Richard Guo, Michael Paquier
Discussion: https://postgr.es/m/796b65c3-57b7-bddf-b0d5-a8afafb8b627@gmail.com
Discussion: https://postgr.es/m/17888-f72930e6b5ce8c14@postgresql.org
Backpatch-through: 11
|
|
isspace() can be locale-sensitive depending on the platform, causing
hstore to consider as whitespaces characters it should not see as such.
For example, U+0105, being decoded as 0xC4 0x85 in UTF-8, would be
discarded from the input given.
This problem is similar to 9ae2661, though it was missed that hstore
can also manipulate non-ASCII inputs, so replace the existing isspace()
calls with scanner_isspace().
This problem exists for a long time, so backpatch all the way down.
Author: Evan Jones
Discussion: https://postgr.es/m/CA+HWA9awUW0+RV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig@mail.gmail.com
Backpatch-through: 11
|
|
Non-leaf pages of GiST indexes contain key attributes, leaf pages
contain both key and non-key attributes, and gist_page_items() ignored
the handling of non-key attributes. This caused a few problems when
using gist_page_items() on a GiST index with INCLUDE:
- On a non-leaf page, the function would crash.
- On a leaf page, the function would work, but miss to display all the
values for included attributes.
This commit fixes gist_page_items() to handle such cases in a more
appropriate way, and now displays the values of key and non-key
attributes for each item separately in a style consistent with what
ruleutils.c would generate for the attribute list, depending on the page
type dealt with. In a way similar to how a record is displayed, values
would be double-quoted for key or non-key attributes if required.
ruleutils.c did not provide a routine able to control if non-key
attributes should be displayed, so an extended() routine for index
definitions is added to work around the leaf and non-leaf page
differences.
While on it, this commit fixes a third problem related to the amount of
data reported for key attributes. The code originally relied on
BuildIndexValueDescription() (used for error reports on constraints)
that would not print all the data stored in the index but the index
opclass's input type, so this limited the amount of information
available. This switch makes gist_page_items() much cheaper as there is
no need to run ACL checks for each item printed, which is not an issue
anyway as superuser rights are required to execute the functions of
pageinspect. Opclasses whose data cannot be displayed can rely on
gist_page_items_bytea().
The documentation of this function was slightly incorrect for the
output results generated on HEAD and v15, so adjust it on these
branches.
Author: Alexander Lakhin, Michael Paquier
Discussion: https://postgr.es/m/17884-cb8c326522977acb@postgresql.org
Backpatch-through: 14
|
|
fuzzystrmatch's difference() function assumes that _soundex()
always initializes its output buffer fully. This was not so for
the case of a string containing no alphabetic characters, resulting
in unstable output and Valgrind complaints.
Fix by using memset() to fill the whole buffer in the early-exit
case. Also make some cosmetic improvements (I didn't care for the
random switches between "instr[0]" and "*instr" notation).
Report and diagnosis by Alexander Lakhin (bug #17935).
Back-patch to all supported branches.
Discussion: https://postgr.es/m/17935-b99316aa79c18513@postgresql.org
|
|
Security: CVE-2023-2454
|
|
The two methods don't cooperate, so set_config_option("search_path",
...) has been ineffective under non-empty overrideStack. This defect
enabled an attacker having database-level CREATE privilege to execute
arbitrary code as the bootstrap superuser. While that particular attack
requires v13+ for the trusted extension attribute, other attacks are
feasible in all supported versions.
Standardize on the combination of NewGUCNestLevel() and
set_config_option("search_path", ...). It is newer than
PushOverrideSearchPath(), more-prevalent, and has no known
disadvantages. The "override" mechanism remains for now, for
compatibility with out-of-tree code. Users should update such code,
which likely suffers from the same sort of vulnerability closed here.
Back-patch to v11 (all supported versions).
Alexander Lakhin. Reported by Alexander Lakhin.
Security: CVE-2023-2454
|
|
Python 3 changed the behavior of PyMapping_Check(), breaking the
test in plpython_to_hstore() that verifies whether a function result
to be transformed is acceptable. A backwards-compatible fix is to
first verify that the object doesn't pass PySequence_Check().
Perhaps accidentally, our other uses of PyMapping_Check() already
follow uses of PySequence_Check(), so that no other bugs were
created by this change.
Per bug #17908 from Alexander Lakhin. Back-patch to all supported
branches.
Dmitry Dolgov and Tom Lane
Discussion: https://postgr.es/m/17908-3f19a125d56a11d6@postgresql.org
|
|
The leak would show up when using batch inserts with foreign tables
included in a partition tree, as the slots used in the batch were not
reset once processed. In order to fix this problem, some
ExecClearTuple() are added to clean up the slots used once a batch is
filled and processed, mapping with the number of slots currently in use
as tracked by the counter ri_NumSlots.
This buffer refcount leak has been introduced in b676ac4 with the
addition of the executor facility to improve bulk inserts for FDWs, so
backpatch down to 14.
Alexander has provided the patch (slightly modified by me). The test
for postgres_fdw comes from me, based on the test case that the author
has sent in the report.
Author: Alexander Pyhalov
Discussion: https://postgr.es/m/b035780a740efd38dc30790c76927255@postgrespro.ru
Backpatch-through: 14
|
|
Unaligned siglen could lead to an unaligned access to subsequent key fields.
Backpatch to 13, where opclass options were introduced.
Reported-by: Alexander Lakhin
Bug: 17847
Discussion: https://postgr.es/m/17847-171232970bea406b%40postgresql.org
Reviewed-by: Tom Lane, Pavel Borisov, Alexander Lakhin
Backpatch-through: 13
|
|
Commit e88754a1965c0f40a723e6e46d670cacda9e19bd caused that case
to be reported as corruption, but Peter Geoghegan pointed out that
it can legitimately happen in the case of a speculative insertion
that aborts, so we'd better not flag it as corruption after all.
Back-patch to v14, like the commit that introduced the issue.
Discussion: http://postgr.es/m/CAH2-WzmEabzcPTxSY-NXKH6Qt3FkAPYHGQSe2PtvGgj17ZQkCw@mail.gmail.com
|
|
In such cases, get_xid_status() doesn't set its output parameter (the
third argument), so we shouldn't fall through to code which will test
the value of that parameter. There are five existing calls to
get_xid_status(), three of which seem to already handle this case
properly. This commit tries to fix the other two.
If we're checking xmin and find that it is invalid (i.e. 0) just
report that as corruption, similar to what's already done in the
three cases that seem correct. If we're checking xmax and find
that's invalid, that's fine: it just means that the tuple hasn't
been updated or deleted.
Thanks to Andres Freund and valgrind for finding this problem, and
also to Andres for having a look at the patch. This bug seems to go
all the way back to where verify_heapam was first introduced, but
wasn't detected until recently, possibly because of the new test cases
added for update chain verification. Back-patch to v14, where this
code showed up.
Discussion: http://postgr.es/m/CA+TgmoZAYzQZqyUparXy_ks3OEOfLD9-bEXt8N-2tS1qghX9gQ@mail.gmail.com
|
|
64bit xids can't represent xids before epoch 0 (see also be504a3e974). When
FullTransactionIdFromXidAndCtx() was passed such an xid, it'd create a 64bit
xid far into the future. Noticed while adding assertions in the course of
investigating be504a3e974, as amcheck's test create such xids.
To fix the issue, just return FirstNormalFullTransactionId in this case. A
freshly initdb'd cluster already has a newer horizon. The most minimal version
of this would make the messages for some detected corruptions differently
inaccurate. To make those cases accurate, switch
FullTransactionIdFromXidAndCtx() to use the 32bit modulo difference between
xid and nextxid to compute the 64bit xid, yielding sensible "in the future" /
"in the past" answers.
Reviewed-by: Mark Dilger <mark.dilger@enterprisedb.com>
Discussion: https://postgr.es/m/20230108002923.cyoser3ttmt63bfn@awork3.anarazel.de
Backpatch: 14-, where heapam verification was introduced
|
|
The initialization order in update_cached_xid_range() was wrong, calling
FullTransactionIdFromXidAndCtx() before setting
->next_xid. FullTransactionIdFromXidAndCtx() uses ->next_xid.
In most situations this will not cause visible issues, because the next call
to update_cached_xid_range() will use a less wrong ->next_xid. It's rare that
xids advance fast enough for this to be a problem.
Found while adding more asserts to the 64bit xid infrastructure.
Reviewed-by: Mark Dilger <mark.dilger@enterprisedb.com>
Discussion: https://postgr.es/m/20230108002923.cyoser3ttmt63bfn@awork3.anarazel.de
Backpatch: 14-, where heapam verification was introduced
|
|
If the regex compiler can see that a regex is unsatisfiable
(for example, '$foo') then it may emit an NFA having no arcs.
pg_trgm's packGraph function did the wrong thing in this case;
it would access off the end of a work array, and with bad luck
could produce a corrupted output data structure causing more
problems later. This could end with wrong answers or crashes
in queries using a pg_trgm GIN or GiST index with such a regex.
Fix by not trying to de-duplicate if there aren't at least 2 arcs.
Per bug #17830 from Alexander Lakhin. Back-patch to all supported
branches.
Discussion: https://postgr.es/m/17830-57ff5f89bdb02b09@postgresql.org
|
|
Attempting to use this function with a raw page not coming from a GiST
index would cause a crash, as it was missing the same sanity checks as
gist_page_items_bytea(). This slightly refactors the code so as all the
basic validation checks for GiST pages are done in a single routine,
in the same fashion as the pageinspect functions for hash and BRIN.
This fixes an issue similar to 076f4d9. A test is added to stress for
this case. While on it, I have added a similar test for
brin_page_items() with a combination make of a valid GiST index and a
raw btree page. This one was already protected, but it was not tested.
Reported-by: Egor Chindyaskin
Author: Dmitry Koval
Discussion: https://postgr.es/m/17815-fc4a2d3b74705703@postgresql.org
Backpatch-through: 14
|
|
postgres_fdw will close its remote session if an sinval cache reset
occurs, since it's possible that that means some FDW parameters
changed. We had two tests that were trying to ensure that the
session remains alive by setting debug_discard_caches = 0; but
that's not sufficient. Even though the tests seem stable enough
in the buildfarm, they flap a lot under CI.
In the first test, which is checking the ability to recover from
a lost connection, we can stabilize the results by just not
caring whether pg_terminate_backend() finds a victim backend.
If a reset did happen, there won't be a session to terminate
anymore, but the test can proceed anyway. (Arguably, we are
then not testing the unintentional-disconnect case, but as long
as that scenario is exercised in most runs I think it's fine;
testing the reset-driven case is of value too.)
In the second test, which is trying to verify the application_name
displayed in pg_stat_activity by a remote session, we had a race
condition in that the remote session might go away before we can
fetch its pg_stat_activity entry. We can close that race and make
the test more certainly test what it intends to by arranging things
so that the remote session itself fetches its pg_stat_activity entry
(based on PID rather than a somewhat-circular assumption about the
application name).
Both tests now demonstrably pass under debug_discard_caches = 1,
so we can remove that hack.
Back-patch into relevant back branches.
Discussion: https://postgr.es/m/20230226194340.u44bkfgyz64c67i6@awork3.anarazel.de
|
|
We were identifying the updatable generated columns of inheritance
children by transposing the calculation made for their parent.
However, there's nothing that says a traditional-inheritance child
can't have generated columns that aren't there in its parent, or that
have different dependencies than are in the parent's expression.
(At present it seems that we don't enforce that for partitioning
either, which is likely wrong to some degree or other; but the case
clearly needs to be handled with traditional inheritance.)
Hence, drop the very-klugy-anyway "extraUpdatedCols" RTE field
in favor of identifying which generated columns depend on updated
columns during executor startup. In HEAD we can remove
extraUpdatedCols altogether; in back branches, it's still there but
always empty. Another difference between the HEAD and back-branch
versions of this patch is that in HEAD we can add the new bitmap field
to ResultRelInfo, but that would cause an ABI break in back branches.
Like 4b3e37993, add a List field at the end of struct EState instead.
Back-patch to v13. The bogus calculation is also being made in v12,
but it doesn't have the same visible effect because we don't use it
to decide which generated columns to recalculate; as a consequence of
which the patch doesn't apply easily. I think that there might still
be a demonstrable bug associated with trigger firing conditions, but
that's such a weird corner-case usage that I'm content to leave it
unfixed in v12.
Amit Langote and Tom Lane
Discussion: https://postgr.es/m/CA+HiwqFshLKNvQUd1DgwJ-7tsTp=dwv7KZqXC4j2wYBV1aCDUA@mail.gmail.com
Discussion: https://postgr.es/m/2793383.1672944799@sss.pgh.pa.us
|
|
seg stores the number of significant digits in an input number
in a "char" field. If char is signed, and the input is more than
127 digits long, the count can read out as negative causing
seg_out() to print garbage (or, if you're really unlucky,
even crash).
To fix, clamp the digit count to be not more than FLT_DIG.
(In theory this loses some information about what the original
input was, but it doesn't seem like useful information; it would
not survive dump/restore in any case.)
Also, in case there are stored values of the seg type containing
bad data, add a clamp in seg_out's restore() subroutine.
Per bug #17725 from Robins Tharakan. It's been like this
forever, so back-patch to all supported branches.
Discussion: https://postgr.es/m/17725-0a09313b67fbe86e@postgresql.org
|
|
Commit b663a4136, which allowed FDWs to INSERT rows in bulk, added to
nodeModifyTable.c code to flush pending inserts to the foreign-table
result relation(s) before completing processing of the ModifyTable node,
but the code failed to take into account the case where the INSERT query
has modifying CTEs, leading to incorrect results.
Also, that commit failed to flush pending inserts before firing BEFORE
ROW triggers so that rows are visible to such triggers.
In that commit we scanned through EState's
es_tuple_routing_result_relations or es_opened_result_relations list to
find the foreign-table result relations to which pending inserts are
flushed, but that would be inefficient in some cases. So to fix, 1) add
a List member to EState to record the insert-pending result relations,
and 2) modify nodeModifyTable.c so that it adds the foreign-table result
relation to the list in ExecInsert() if appropriate, and flushes pending
inserts properly using the list where needed.
While here, fix a copy-and-pasteo in a comment in ExecBatchInsert(),
which was added by that commit.
Back-patch to v14 where that commit appeared.
Discussion: https://postgr.es/m/CAPmGK16qutyCmyJJzgQOhfBq%3DNoGDqTB6O0QBZTihrbqre%2BoxA%40mail.gmail.com
|
|
This reverts commit 5cda142bb9d2bd7e7ed1c22ae89afe58abfa8d7b
(in v14 only).
It turns out that that fails under force_parallel_mode = regress,
because pageinspect's disk-access functions are marked parallel
safe, which they are not if you try to use them on a temp table.
The cost of fixing that pre-v15 seems to exceed the value of
making this test case fully stable, so we will just leave things
as-is in v14.
|
|
pageinspect has occasionally failed on slow buildfarm members,
with symptoms indicating that the expected effects of VACUUM
FREEZE didn't happen. This is presumably because a background
transaction such as auto-analyze was holding back global xmin.
We can work around that by using a temp table in the test.
Since commit a7212be8b, that will use an up-to-date cutoff xmin
regardless of other processes. And pageinspect itself shouldn't
really care whether the table is temp.
Back-patch to v14. There would be no point in older branches
without back-patching a7212be8b, which seems like more trouble
than the problem is worth.
Discussion: https://postgr.es/m/2892135.1668976646@sss.pgh.pa.us
|
|
This is a back-patch of the v15-era commit f10f0ae42 into older
supported branches. The idea is to design out bugs in which an
ill-timed relcache flush clears rel->rd_smgr partway through
some code sequence that wasn't expecting that. We had another
report today of a corner case that reliably crashes v14 under
debug_discard_caches (nee CLOBBER_CACHE_ALWAYS), and therefore
would crash once in a blue moon in the field. We're unlikely
to get rid of all such code paths unless we adopt the more
rigorous coding rules instituted by f10f0ae42. Therefore,
even though this is a bit invasive, it's time to back-patch.
Some comfort can be taken in the fact that f10f0ae42 has been
in v15 for 16 months without problems.
I left the RelationOpenSmgr macro present in the back branches,
even though no core code should use it anymore, in order to not break
third-party extensions in minor releases. Such extensions might opt
to start using RelationGetSmgr instead, to reduce their code
differential between v15 and earlier branches. This carries a hazard
of failing to compile against headers from existing minor releases.
However, once compiled the extension should work fine even with such
releases, because RelationGetSmgr is a "static inline" function so
it creates no link-time dependency. So depending on distribution
practices, that might be an OK tradeoff.
Per report from Spyridon Dimitrios Agathos. Original patch
by Amul Sul.
Discussion: https://postgr.es/m/CAFM5RaqdgyusQvmWkyPYaWMwoK5gigdtW-7HcgHgOeAw7mqJ_Q@mail.gmail.com
Discussion: https://postgr.es/m/CANiYTQsU7yMFpQYnv=BrcRVqK_3U3mtAzAsJCaqtzsDHfsUbdQ@mail.gmail.com
|
|
When executing a utility statement, we must fetch everything
we need out of the PlannedStmt data structure before calling
standard_ProcessUtility. In certain cases (possibly only ROLLBACK
in extended query protocol), that data structure will get freed
during command execution. The situation is probably often harmless
in production builds, but in debug builds we intentionally overwrite
the freed memory with garbage, leading to picking up garbage values
of statement location and length, typically causing an assertion
failure later in pg_stat_statements. In non-debug builds, if
something did go wrong it would likely lead to storing garbage
for the query string.
Report and fix by zhaoqigui (with cosmetic adjustments by me).
It's an old problem, so back-patch to all supported versions.
Discussion: https://postgr.es/m/17663-a344fd0675f92128@postgresql.org
Discussion: https://postgr.es/m/1667307420050.56657@hundsun.com
|
|
decoding.
This problem has been introduced by commit 272248a0c1 where we started
assigning the subtransactions to the top-level transaction when we mark
both the top-level transaction and its subtransactions as containing
catalog changes. After we assign subtransactions to the top-level
transaction, we were not allowed to execute any invalidations associated
with it when we decide to skip the transaction.
The reason to assign the subtransactions to the top-level transaction was
to avoid the assertion failure in AssertTXNLsnOrder() as they have the
same LSN when we sometimes start accumulating transaction changes for
partial transactions after the restart. Now that with commit 64ff0fe4e8,
we skip this assertion check until we reach the LSN at which we start
decoding the contents of the transaction, so, there is no reason for such
an assignment anymore.
The assignment change was introduced in 15 and prior versions but this bug
doesn't exist in branches prior to 14 since we don't add invalidation
messages to subtransactions. We decided to backpatch through 11 for
consistency but not for 10 since its final release is near.
Reported-by: Kuroda Hayato
Author: Masahiko Sawada
Reviewed-by: Amit Kapila
Backpatch-through: 11
Discussion: https://postgr.es/m/TYAPR01MB58660803BCAA7849C8584AA4F57E9%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Discussion: https://postgr.es/m/a89b46b6-0239-2fd5-71a9-b19b1f7a7145%40enterprisedb.com
|
|
When the logical decoding restarts from NEW_CID, since there is no
association between the top transaction and its subtransaction, both are
created as top transactions and have the same LSN. This caused the
assertion failure in AssertTXNLsnOrder().
This patch skips the assertion check until we reach the LSN at which we
start decoding the contents of the transaction, specifically
start_decoding_at LSN in SnapBuild. This is okay because we don't
guarantee to make the association between top transaction and
subtransaction until we try to decode the actual contents of transaction.
The ordering of the records prior to the start_decoding_at LSN should have
been checked before the restart.
The other assertion failure is due to the reason that we forgot to track
that we have considered top-level transaction id in the list of catalog
changing transactions that were committed when one of its subtransactions
is marked as containing catalog change.
Reported-by: Tomas Vondra, Osumi Takamichi
Author: Masahiko Sawada, Kuroda Hayato
Reviewed-by: Amit Kapila, Dilip Kumar, Kuroda Hayato, Kyotaro Horiguchi, Masahiko Sawada
Backpatch-through: 10
Discussion: https://postgr.es/m/a89b46b6-0239-2fd5-71a9-b19b1f7a7145%40enterprisedb.com
Discussion: https://postgr.es/m/TYCPR01MB83733C6CEAE47D0280814D5AED7A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
|
|
The tlist of the EvalPlanQual outer plan for a ForeignScan node is
adjusted to produce a tuple whose descriptor matches the scan tuple slot
for the ForeignScan node. But in the case where the outer plan contains
an extra Sort node, if the new tlist contained columns required only for
evaluating PlaceHolderVars or columns required only for evaluating local
conditions, this would cause setrefs.c to fail with the error.
The cause of this is that when creating the outer plan by injecting the
Sort node into an alternative local join plan that could emit such extra
columns as well, we fail to arrange for the outer plan to propagate them
up through the Sort node, causing setrefs.c to fail to match up them in
the new tlist to what is available from the outer plan. Repair.
Per report from Alexander Pyhalov.
Richard Guo and Etsuro Fujita, reviewed by Alexander Pyhalov and Tom Lane.
Backpatch to all supported versions.
Discussion: http://postgr.es/m/cfb17bf6dfdf876467bd5ef533852d18%40postgrespro.ru
|
|
When using the BSD UUID functions, contrib/uuid-ossp expects
uuid_create() to produce a version-1 UUID. FreeBSD still does so,
but in recent NetBSD releases that function produces a version-4
(random) UUID instead. That's not acceptable for our purposes:
if the user wanted v4 she would have asked for v4, not v1.
Hence, check the version digit and complain if it's not '1'.
Also drop the documentation's claim that the NetBSD implementation
is usable. It might be, depending on which OS version you're using,
but we're not going to get into that kind of detail.
(Maybe someday we should ditch all these external libraries
and just write our own UUID code, but today is not that day.)
Nazir Bilal Yavuz, with cosmetic adjustments and docs by me.
Backpatch to all supported versions.
Discussion: https://postgr.es/m/3848059.1661038772@sss.pgh.pa.us
Discussion: https://postgr.es/m/17358-89806e7420797025@postgresql.org
|
|
Previously, we relied on HEAP2_NEW_CID records and XACT_INVALIDATION
records to know if the transaction has modified the catalog, and that
information is not serialized to snapshot. Therefore, after the restart,
if the logical decoding decodes only the commit record of the transaction
that has actually modified a catalog, we will miss adding its XID to the
snapshot. Thus, we will end up looking at catalogs with the wrong
snapshot.
To fix this problem, this changes the snapshot builder so that it
remembers the last-running-xacts list of the decoded RUNNING_XACTS record
after restoring the previously serialized snapshot. Then, we mark the
transaction as containing catalog changes if it's in the list of initial
running transactions and its commit record has XACT_XINFO_HAS_INVALS. To
avoid ABI breakage, we store the array of the initial running transactions
in the static variables InitialRunningXacts and NInitialRunningXacts,
instead of storing those in SnapBuild or ReorderBuffer.
This approach has a false positive; we could end up adding the transaction
that didn't change catalog to the snapshot since we cannot distinguish
whether the transaction has catalog changes only by checking the COMMIT
record. It doesn't have the information on which (sub) transaction has
catalog changes, and XACT_XINFO_HAS_INVALS doesn't necessarily indicate
that the transaction has catalog change. But that won't be a problem since
we use snapshot built during decoding only to read system catalogs.
On the master branch, we took a more future-proof approach by writing
catalog modifying transactions to the serialized snapshot which avoids the
above false positive. But we cannot backpatch it because of a change in
the SnapBuild.
Reported-by: Mike Oh
Author: Masahiko Sawada
Reviewed-by: Amit Kapila, Shi yu, Takamichi Osumi, Kyotaro Horiguchi, Bertrand Drouvot, Ahsan Hadi
Backpatch-through: 10
Discussion: https://postgr.es/m/81D0D8B0-E7C4-4999-B616-1E5004DBDCD2%40amazon.com
|
|
When inserting a view referencing a foreign table that has WITH CHECK
OPTION constraints, in single-insert mode postgres_fdw retrieves the
data that was actually inserted on the remote side so that the WITH
CHECK OPTION constraints are enforced with the data locally, but in
batch-insert mode it cannot currently retrieve the data (except for the
row first inserted through the view), resulting in enforcing the WITH
CHECK OPTION constraints with the data passed from the core (except for
the first-inserted row), which led to incorrect results when inserting
into a view referencing a foreign table in which a remote BEFORE ROW
INSERT trigger changes the rows inserted through the view so that they
violate the view's WITH CHECK OPTION constraint. Also, the query
inserting into the view caused an assertion failure in assert-enabled
builds.
Fix these by disabling batch insertion when inserting into such a view.
Back-patch to v14 where batch insertion was added.
Discussion: https://postgr.es/m/CAPmGK17LpbTZs4m4a_6THP54UBeK9fHvX8aVVA%2BC6yEZDZwQcg%40mail.gmail.com
|
|
We've heard a couple of reports of people having trouble with
multi-gigabyte-sized query-texts files. It occurred to me that on
32-bit platforms, there could be an issue with integer overflow
of calculations associated with the total query text size.
Address that with several changes:
1. Limit pg_stat_statements.max to INT_MAX / 2 not INT_MAX.
The hashtable code will bound it to that anyway unless "long"
is 64 bits. We still need overflow guards on its use, but
this helps.
2. Add a check to prevent extending the query-texts file to
more than MaxAllocHugeSize. If it got that big, qtext_load_file
would certainly fail, so there's not much point in allowing it.
Without this, we'd need to consider whether extent, query_offset,
and related variables shouldn't be off_t not size_t.
3. Adjust the comparisons in need_gc_qtexts() to be done in 64-bit
arithmetic on all platforms. It appears possible that under duress
those multiplications could overflow 32 bits, yielding a false
conclusion that we need to garbage-collect the texts file, which
could lead to repeatedly garbage-collecting after every hash table
insertion.
Per report from Bruno da Silva. I'm not convinced that these
issues fully explain his problem; there may be some other bug that's
contributing to the query-texts file becoming so large in the first
place. But it did get that big, so #2 is a reasonable defense,
and #3 could explain the reported performance difficulties.
(See also commit 8bbe4cbd9, which addressed some related bugs.
The second Discussion: link is the thread that led up to that.)
This issue is old, and is primarily a problem for old platforms,
so back-patch.
Discussion: https://postgr.es/m/CAB+Nuk93fL1Q9eLOCotvLP07g7RAv4vbdrkm0cVQohDVMpAb9A@mail.gmail.com
Discussion: https://postgr.es/m/5601D354.5000703@BlueTreble.com
|
|
When postgres_fdw begins an asynchronous data fetch, it submits FETCH query
by using PQsendQuery(). If PQsendQuery() fails and returns 0, postgres_fdw
should report an error. But, previously, postgres_fdw reported an error
only when the return value is less than 0, though PQsendQuery() never return
the values other than 0 and 1. Therefore postgres_fdw could not handle
the failure to send FETCH query in an asynchronous data fetch.
This commit fixes postgres_fdw so that it reports an error
when PQsendQuery() returns 0.
Back-patch to v14 where asynchronous execution was supported in postgres_fdw.
Author: Fujii Masao
Reviewed-by: Japin Li, Tom Lane
Discussion: https://postgr.es/m/b187a7cf-d4e3-5a32-4d01-8383677797f3@oss.nttdata.com
|
