This is a continuation of the work done in ac59a90bef. The
*GetDatum() macros for output should match with what the SQL functions
use as DatumGet*() in input.
Aleksander has spotted some of the areas patched here, for pageinspect.
I have spotted the rest while digging into the state of the tree.
There is no behavior change after this commit, since all the affected
values are small enough that the signed bit is never used.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/afLsqRjVqKK8hhKk@paquier.xyz
When fixing up the targetlist and qpqual of an outer join, we must
account for the effects of the outer join. Vars and PHVs appearing
there are logically above the join, so they should have nullingrels
equal to the input Vars/PHVs' nullingrels plus the bit added by the
outer join.
Determining the effects of the outer join can be tricky when the join
has been commuted with another one per outer join identity 3. In this
case, the Vars/PHVs in the join's targetlist and qpqual should have
the same nullingrels that they would if the two joins had been done in
syntactic order. Unfortunately, in setrefs.c, we don't have enough
information to identify what that should be, so we have to use
superset nullingrels matches instead of exact ones.
However, we can tighten the check somewhat. Currently, we check
whether the jointype is JOIN_INNER and use NRM_SUPERSET if it is not.
We can improve this by checking whether the Join node has non-empty
ojrelids and using NRM_SUPERSET only in that case. This allows us to
perform exact matches in more situations.
To support this, we record the outer-join relids in Join plan nodes.
This information can also improve EXPLAIN (RANGE_TABLE) output by
showing which outer-join relids are completed by each Join plan node.
We may discover additional uses for this information in the future.
Author: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs482_DFHzQ079ZPp6c8UvmFdz3Jj+4K8tVRu9g2Bw34NPA@mail.gmail.com
Commit 28972b6fc should have done this, but didn't.
While at it, remove an extra blank line in fetch_remote_statistics()
introduced by that commit.
Reported-by: Chao Li <lic@highgo.com>
Co-authored-by: Chao Li <lic@highgo.com>
Co-authored-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/6ED81190-B398-44C9-A1E9-8EFE4ED183AF%40gmail.com
Backpatch-through: 19
We forgot to use the PG_MODULE_MAGIC_EXT in some newly added modules:
pg_plan_advice, pg_stash_advice and the pgrepack output plugin and
instead used the older PG_MODULE_MAGIC macro.
Author: Andreas Karlsson <andreas@proxel.se>
Discussion: http://postgr.es/m/ad7b910c-d145-4120-994d-2e55c456aa75@proxel.se
Backpatch-through: 19
"prev_tuple" was overwritten with a new tuple coming from
CopyIndexTuple() on each loop, leaking memory for every tuple processed
on entry tree pages. The function uses a dedicated memory context, but
this could leave unused large areas of memory while processing a large
GIN index, the larger the worse.
Oversight in 14ffaece0f.
Author: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CALdSSPjTS6TYe5=5NfMUBYZyQu5cn=ABL6K5_OZjzGWqnwXeBw@mail.gmail.com
Backpatch-through: 18
There's no need to create and free a temporary copy of the input,
since str_tolower() is already able to cope with not-certainly-
nul-terminated input. (Before v18, copying was needed because
this code used lowerstr(), but now we can do without.)
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19525-b0be8e4eb7dbaf07@postgresql.org
gbt_var_node_cp_len() contained logic to ensure that its choice of
a common prefix length didn't truncate away part of a multibyte
character. However, that was really dead code, because we have not
allowed truncation of text-string data types since ef770cbb6, and
it seems unlikely that that behavior could ever get resurrected.
The code is still reachable via gbt_var_penalty, but for that
usage it hardly matters if we break in the middle of a multibyte
character: we're just calculating a small correction factor that
is arguably bunkum anyway in non-C locales.
Hence, delete said code. That actually removes all need for
gbtree_vinfo.eml, which allows const-ification of the gbtree_vinfo
structs in which we were changing it, which removes one headache
for future attempts to thread-ify the backend.
(Curiously, all this infrastructure was itself added by ef770cbb6.
Not sure why Teodor didn't see the contradiction.)
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
Truncating an internal node's upper bound can cause it to compare
less than some values that in fact are included in the represented
leaf page. So we need a hack to make sure it looks large enough
to include all values that could be on the page. But there's no
equivalent issue for the lower bound. The fact that the code did
fuzzy comparisons for the lower bound too seems to be the result of
fuzzy thinking. Or maybe there was a desire to not assume too much
about what the datatype's comparison rule is; but we've already
fully bought into the premise that internal keys compare like bytea.
Hence, remove the useless check against the key's lower bound in
gbt_var_node_pf_match. The comparable check in gbt_var_penalty may
also be useless, but I'm not quite sure. In any case that seems
negligible from a performance standpoint, so I left it alone.
Also, in the strategy cases in gbt_var_consistent that only
require comparisons to the lower bound, there's no need to call
gbt_var_node_pf_match at all. Refactor that logic by inventing
macros lower_is_below_query and upper_is_above_query to directly
express what we need to test. I also took this opportunity to flip
all the tests around to be "indexkey OP query" rather than mostly
being the reverse: IMO this makes the code less confusing since the
tests now match the names of the strategies.
Also, in the name of consistency, make gbt_num_consistent look
like that too. There's no functional change there, but this
should be more readable going forward.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
For some odd reason we pass the strategy number to gbt_num_consistent
as "const StrategyNumber *strategy". There's no reason for that:
it almost certainly costs more at both callers and callee to pass a
pointer than to pass a small integer value. And it's inconsistent
with gbt_var_consistent(), so fix it.
gbt_var_consistent() had its own infelicity, which was not marking
the input "key" value const. Fix that too while we're here.
This is primarily cosmetic, so I see no need to backpatch.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
gbt_var_consistent() handled the <> (BtreeGistNotEqual) strategy without
distinguishing leaf from internal pages, unlike every other strategy.
In particular, it tried to apply the datatype-specific f_eq method,
which is completely wrong since internal keys might not have the same
representation as leaf keys. This led to OOB reads and potentially
crashes, and most likely to wrong query results as well.
On leaf pages we can apply the inverse of what the Equal strategy does.
On internal pages, use a correct implementation of what the previous
code intended: we can descend if the query value equals both bounds,
*so long as the bounds aren't truncated*. With truncated bounds we
don't quite know the range of what's below, so we must always descend.
Adjust the code in gbt_num_consistent() to look similar, too. This
fixes a performance buglet in that there's no need to do two comparisons
on a leaf entry, but the main point is just to keep code consistency.
Reported-by: 王跃林 <violin0613@tju.edu.cn>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
Backpatch-through: 14
There are a number of rather subtle points about the behavior of
this code, which its original authors did not deign to document.
Try to improve that. In particular, explain how internal and leaf
keys can differ and what the restrictions are on that.
This work arose from trying to fix some bugs, and in the process
I believe I've identified some more, but this patch does not attempt
to fix anything, only document it. I did make a few purely cosmetic
code changes, such as removing dead (and confusing!) initializations
of variables and choosing more appropriate types for some pointers.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
If we're dealing with leaf entries, the function to call is bitcmp
not byteacmp. Using byteacmp didn't lead to any obvious failure,
but it did result in sorting the entries in a way not matching the
datatype's actual sort order. Hence the constructed index would be
less efficient than one would expect, and in particular worse than
what you got before this code was added in v18 (by commit e4309f73f).
We might want to recommend that users reindex btree_gist indexes
on bit/varbit columns.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/AH*AvQCYKhQGVvPWi1GiU4oY.8.1781609375063.Hmail.3020001251@tju.edu.cn
Backpatch-through: 18
A foreign scan can target a single relation while still reaching the
fs_relids branch of pgpa_build_scan() -- for example, when postgres_fdw
pushes an aggregate down over one foreign table. In that case, no
advice should be emitted.
Author: Mahendra Singh Thalor <mahi6run@gmail.com>
Co-authored-by: Robert Haas <rhaas@postgresql.org>
Discussion: http://postgr.es/m/CAKYtNAofuAJBz6++SeikpCb=Y=MO1QgEuZNJ+KZOP2johF1r4Q@mail.gmail.com
The float4 and float8 btree_gist opclasses compared keys with raw C
operators (==, <, >). IEEE 754 makes every comparison involving NaN
false, so GiST disagreed with the regular float comparison operators
and with the btree opclass, which uses float[4|8]_cmp_internal()
(so that all NaNs are equal and NaN sorts after every non-NaN value).
In addition, the penalty and distance functions were not careful
about NaNs, and the penalty functions could also misbehave for IEEE
infinities. Wrong answers from the penalty functions would probably
do no more than make the index non-optimal, but the distance mistakes
were visible from SQL.
To fix, make the comparison functions rely on the same NaN-aware
comparison functions the core code uses, and rewrite the penalty
and distance functions to follow the rules that NaNs are equal
but maximally far away from non-NaNs. The penalty_num() code was
formerly shared between integral and float cases, but I chose to make
two copies so that the integral cases are not saddled with the extra
logic for NaNs and infinities/overflows. I also rewrote it as static
inline functions instead of an unreadable and uncommented macro.
The float penalty functions were previously unreached by the
regression tests, so add new test cases to exercise them.
There's no on-disk format change, but users who have NaN entries
in a btree_gist index would be well advised to reindex it.
Bug: #19501
Bug: #19524
Reported-by: Man Zeng <zengman@halodbtech.com>
Reported-by: Yuelin Wang <3020001251@tju.edu.cn>
Author: Bill Kim <billkimjh@gmail.com>
Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19501-3bff3bbc97f1e7c9@postgresql.org
Discussion: https://postgr.es/m/19524-9559d302c8455664@postgresql.org
Discussion: https://postgr.es/m/CAMQXxcgbtD2LXfX0tpgvOizxP-XxrCHV2ZDy4By_TZnJMsxXWQ@mail.gmail.com
Backpatch-through: 14
If the user doesn't specify a name for an index, it's generated
based on the names chosen for the index columns (which the user
has no direct control over). For index columns that are just
columns of the base relation, the index column name is the same as
the base column name; but for index columns that are expressions,
it's less clear what to do. Up to now, what we have done is
equivalent to the heuristics used to choose SELECT output column
names, except that we fall back to "expr" not "?column?" in the
numerous cases that FigureColname doesn't know what to do with.
This is not tremendously helpful. More, it frequently leads to
collisions of generated index names, which we can handle but
only at the cost of user confusion; also there's some risk of
concurrent index creations trying to use the same name.
Let's try to do better.
Messing with the FigureColname heuristics would have a very
large blast radius, since that affects the column headings
that applications see. That doesn't seem wise, but fortunately
SQL queries are seldom directly concerned with index names.
So we should be able to change the index-name generation rules
as long as we decouple them from FigureColname.
The method used in this patch is to dig through the expression,
extract the names of Vars, the string representations of Consts,
and the names of functions, and run those together with underscores
between. Other expression node types are ignored but descended
through. We could work harder by handling more node types, but
it seems like this is likely to be sufficient to arrive at unique
index names in many cases.
Notably, this rule ignores the names of operators, for example
both "a + b" and "a * b" will be rendered as "a_b". This choice
was made to reduce the probability of having to double-quote
the index name.
I've also chosen to strip Const representations down to only
alphanumeric characters (plus non-ASCII characters, which our
parser treats as alphabetic anyway). So for example "x + 1.0"
would be represented as "x_10". This likewise avoids possible
quoting problems. I also considered limiting how many characters
we'd take from each Const, but didn't do that here.
We might tweak these rules some more after we get some experience
with this patch. It's being committed at the start of a
development cycle to provide as much time as possible to gather
feedback.
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/876799.1757987810@sss.pgh.pa.us
Discussion: https://postgr.es/m/18959-f63b53b864bb1417@postgresql.org
In fe_memutils.h, we have various allocation functions beginning with
either pg_ or p. The pg_ functions have a matching pg_free() for
freeing memory, while the p functions use pfree(). In some cases, we
were allocating memory with one set of functions while using the wrong
deallocation functions. This creates a tiny bit of mental overhead
when reading code. Matching up allocation and deallocation functions
makes it easier to analyze memory handling in a code path.
Author: Tristan Partin <tristan@partin.io>
Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com>
Discussion: https://www.postgresql.org/message-id/flat/DIBZE2B6SVF2.28R3EQTYJSWIG@partin.io
This changes the argtypes argument of SPI_prepare(),
SPI_prepare_cursor(), SPI_cursor_open_with_args(), and
SPI_execute_with_args() from Oid *argtypes to const Oid *argtypes.
The underlying functions were already receptive to that, so this
doesn't require any significant changes beyond the function signatures
and some internal variables.
Commit 28972b6fc3 recently introduced a case where a const had to be
cast away before calling these functions. This is fixed here.
In passing, make a very similar const addition to SPI_modifytuple().
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/86b5162f-c472-40fa-997b-0450dece1dec%40eisentraut.org
This is required before the creation of a new branch. pgindent is
clean, as well as is reformat-dat-files.
perltidy version is v20230309, as documented in pgindent's README.
PL/Python and its hstore and jsonb transforms build SQL values from
Python containers by calling Python C API functions that can return
NULL, and in several places the result was used without first checking
it.
On the sequence side, PySequence_GetItem() is used when converting a
returned sequence into a SQL array or composite value, when reading
the argument list passed to plpy.execute() or plpy.cursor(), and when
reading the list of type names given to plpy.prepare(). On the
mapping side, the hstore and jsonb transforms call PyMapping_Size()
and PyMapping_Items() and then index the result with PyList_GetItem()
and PyTuple_GetItem().
All of these return NULL (or -1), with a Python exception set, for a
broken object: for example one whose __getitem__() or items() raises,
or which reports a length that disagrees with what it actually yields.
The unchecked result was then dereferenced, crashing the backend.
Fix this by checking the result of each call and reporting a regular
error if it failed, so that the underlying Python exception is
surfaced instead of taking down the session.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAMbWs49BKM9wP6m8bCXEpHwQKp7usvOGV6Jf=J7FYr_BCpxLqg@mail.gmail.com
Backpatch-through: 14
We should defer validating FDW usage until after analysis. We have to
guard against not just the topmost table, but also individual child
partitions. Added the check to CheckValidResultRel, because it is
called after looking up child partitions (accounting for pruning), but
before the FDW can run a DirectModify update, which would bypass
per-tuple executor work.
Author: jian he <jian.universality@gmail.com>
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Paul A. Jungwirth <pj@illuminatedcomputing.com>
Discussion: https://www.postgresql.org/message-id/flat/CA%2BrenyUte0_UJsJiDJQi82oaBsMJn%3Dcct0Wn%3DvOqXtuDn%3DYYJA%40mail.gmail.com
The read stream callback apw_read_stream_next_block() advances p->pos
through the block_info array. When processing the last block, it
increments p->pos to prewarm_stop_idx before returning. The callback
itself is safe because it checks bounds before accessing the array.
However, the caller assigned blk from block_info[i] at the end of the
loop body, before the loop condition was re-evaluated. When i equaled
prewarm_stop_idx, this accessed memory beyond the allocated DSM segment,
causing a segfault.
Restructure the loop to check bounds at the top and assign blk at the
beginning of the loop body, where it is always safe. This avoids the
need for an explicit bounds check at the end.
Backpatch to 18, where the bug was introduced by commit 6acab8bdbc.
Author: Matheus Alcantara <mths.dev@pm.me>
Reported-by: Glauber Batista <glauberrbatista@gmail.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Backpatch-through: 18
Discussion: https://www.postgresql.org/message-id/CAO%2B_mTQgQyTYwDh%3DU8iTnsDmOGyWsZJjUV31SmEYwmw6_xY6Bw%40mail.gmail.com
This is similar to d3bba04154, batching all the reports of this type
received since the last batch. This covers typos and inconsistencies
for the most part.
The user-visible documentation change impacts only HEAD.
ExecInitModifyTable() rebuilds the per-result-relation lists after
dropping result relations removed by initial runtime pruning. The
re-indexing was done for withCheckOptionLists, returningLists,
updateColnosLists, mergeActionLists and mergeJoinConditions, but
fdwPrivLists and fdwDirectModifyPlans were missed. As a result, a
kept foreign result relation could be handed the wrong fdw_private,
or ri_usesFdwDirectModify could be set from the wrong plan index,
leading to wrong behavior or a crash in BeginForeignModify() and in
the direct-modify path.
show_modifytable_info() had the same problem: it indexed the
plan-ordered node->fdwPrivLists with the post-pruning executor
position, so once initial pruning removed a result relation it
could read a different relation's fdw_private (often a NIL entry),
producing wrong EXPLAIN output or a crash.
Fix by re-indexing fdwPrivLists and fdwDirectModifyPlans alongside
the other lists, saving the re-indexed private lists in
ModifyTableState.mt_fdwPrivLists and reading from there in both
nodeModifyTable.c and explain.c.
Reported-by: Chi Zhang <798604270@qq.com>
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Author: Rafia Sabih <rafia.pghackers@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/19484-a3cb82c8cde3c8fa%40postgresql.org
Backpatch-through: 18
Raising an error within a function using an OSSLCipher object led
to a complaint from ResourceOwnerForget and then a double-free crash,
because ResOwnerReleaseOSSLCipher forgot to unhook the OSSLCipher
object from its owner. (The sibling logic for OSSLDigest objects got
this right, as did every other ReleaseResource function AFAICS.)
Oversight in cd694f60d.
Bug: #19527
Reported-by: Yuelin Wang <3020001251@tju.edu.cn>
Author: Yuelin Wang <3020001251@tju.edu.cn>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19527-6e7686960c6dce78@postgresql.org
Backpatch-through: 17
Add CHECK_FOR_INTERRUPTS() to the while loop in plperl_to_hstore()
that dereferences chains of Perl references, so that a circular
reference (e.g. $x = \$x) can be cancelled by the user instead of
spinning indefinitely. (We looked at detecting such circular
references, but it seems more trouble than it's worth.)
This is a follow-up to da82fbb8f, which fixed the same issue in
SV_to_JsonbValue() in jsonb_plperl.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TPbjkzUk4qJ5dHvDNEz0hBuFue3A-XWz_=897z+BC+z8A@mail.gmail.com
Backpatch-through: 14
Add check_stack_depth() to Jsonb_to_SV, SV_to_JsonbValue,
PLyObject_FromJsonbContainer, and PLyObject_ToJsonbValue. Without
this, deeply nested JSONB values can crash the backend with SIGSEGV
instead of raising a proper error.
Also add CHECK_FOR_INTERRUPTS() to the while loop in SV_to_JsonbValue
that dereferences chains of Perl references, so that a circular
reference (e.g. $x = \$x) can be cancelled by the user instead of
spinning indefinitely. (We looked at detecting such circular
references, but it seems more trouble than it's worth.)
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CAJ7c6TPbjkzUk4qJ5dHvDNEz0hBuFue3A-XWz_=897z+BC+z8A@mail.gmail.com
Backpatch-through: 14
The expression (len_diff * 10 * (an + 1)) used as the return value of
ltree_compare() is computed at int32 width. With LTREE_MAX_LEVELS =
65535, the product can exceed INT32_MAX once an ltree has more than
~14,653 levels, which causes the result to wrap and invert its sign.
That corrupts btree ordering as well as the "magnitude" consumed by
ltree_penalty() for GiST page splits.
To fix, split ltree_compare() into two functions. The new
ltree_compare_distance() function returns a float, which won't
overflow. It's used by the ltree_penalty() caller. All the other
callers only care about the sign of the return value, i.e. which of
the arguments is greater, so change ltree_compare() to not multiply
the result with (10 * (an + 1)), which avoids the overflow for those
callers.
Existing btree or GiST indexes on ltree columns containing values with
more than ~14,653 levels may be corrupt and should be REINDEXed.
Add a regression test based on the reporter's PoC.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reported-by: 王跃林 <violin0613@tju.edu.cn>
Discussion: https://www.postgresql.org/message-id/AI6AnABgKW93Qbx1jVzi84r9.8.1781322625756.Hmail.3020001251%40tju.edu.cn
Backpatch-through: 14
Since the remote column names of a foreign table could be longer than
NAMEDATALEN, remattrmap_cmp(), which compares such column names, should
have used strcmp(), not strncmp() with n=NAMEDATALEN.
Author: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Etsuro Fujita <etsuro.fujita@gmail.com>
Discussion: https://postgr.es/m/81D981EB-ECC1-495D-8EAC-5CFB67B2CF77%40gmail.com
bt_normalize_tuple() uses VARSIZE() to get the size of varlena, even though
it's not yet known, that it has a 4-byte header. Fix this by replacing a
accessor with a universal VARSIZE_ANY().
Backpatch to all supported versions.
Reported-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/7ckc7oka4bvafkf5bwlqs6ygrhlsbhz25ppozfch7zbuxcx3rf%40e4pr4oqenalc
Author: Andrey Borodin <x4mmm@yandex-team.ru>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
Backpatch-through: 14
When amcheck validates that a B-Tree metapage's allequalimage flag
matches _bt_allequalimage(), it could fail to report corruption
unless one of the index key columns used interval_ops. As a result,
pg_amcheck could silently miss this corruption on other opclasses,
incorrectly reporting the index as valid.
The mistake was that bt_index_check_callback() kept ereport(ERROR)
inside the loop that scans key attributes for INTERVAL_BTREE_FAM_OID,
even though that loop is only needed to decide whether to add
the interval-specific hint. This commit moves ereport() out of the loop
so allequalimage mismatches are always reported, while still emitting
the hint for affected interval indexes.
Back-patch to v18, where d70b17636d introduced this regression
while moving the check into bt_index_check_callback().
Author: Chao Li <lic@highgo.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/011ACC9C-CB87-4160-ACE7-4ED57AB86E15@gmail.com
Backpatch-through: 18
When printing the upper boundary of a seg interval, seg_out() decided
whether to emit the certainty indicator ('<', '>' or '~') by testing the
upper indicator (u_ext) for '<' and '>', but mistakenly tested the lower
indicator (l_ext) for '~'. This is a copy-and-paste slip from the
symmetric code that prints the lower boundary a few lines above.
The consequences for valid input were:
* A '~' on the upper boundary was dropped on output, e.g.
'1.5 .. ~2.5'::seg printed as '1.5 .. 2.5'.
* When the lower boundary carried '~' but the upper boundary had no
indicator, the wrong test matched and sprintf(p, "%c", seg->u_ext)
wrote a NUL byte (u_ext == '\0'), which truncated the result string
and silently lost the entire upper boundary, e.g.
'~6.5 .. 8.5'::seg printed as '~6.5 .. '.
Certainty indicators are documented to be preserved on output (they are
ignored by the operators, but kept as comments), so this broke the
input/output round-trip for the affected values.
The bug has existed since seg was added. It went unnoticed because the
existing regression tests only exercised certainty indicators on
single-point segs, which are printed by a different branch of seg_out().
Add tests that place indicators on both boundaries of an interval.
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://www.postgresql.org/message-id/CAON2xHPYeRRCEVAv8XfE18KsEsEHCiYcJ5fOsoxFuMEfpxF1=g@mail.gmail.com
Backpatch-through: 14
pgxmlNodeSetToText() passed nodeTab[i]->doc to xmlNodeDump() without
checking the node type, which could cause a crash as a
XML_NAMESPACE_DECL maps to a xmlNs struct. The passed-in code would
then be dereferenced in xmlNodeDump().
This commit switches the code to render XML_NAMESPACE_DECL nodes with
xmlXPathCastNodeToString(), like xpath_table(). Some tests are added,
written by me.
Author: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20260611031436.5afde3cb@andrnote
Backpatch-through: 14
Commit 257c8231bf changed pg_buffercache_pages() to materialize its output
directly into a tuplestore. As a result, the function ended up trusting
a caller-supplied RECORD descriptors. That could lead to crashes
if the supplied row definition did not match the actual returned values,
for example by passing bool Datums to tuplestore_putvalues() with
an incompatible descriptor.
Fix this by constructing the correct tuple descriptor for
pg_buffercache_pages() and assigning it to
rsinfo->setDesc after InitMaterializedSRF(). This restores the executor's
tupledesc_match() verification, so incompatible caller-supplied
row definitions are rejected with an error, as before commit 257c8231bf.
Bug: #19508
Reported-by: Nikita Kalinin <n.kalinin@postgrespro.ru>
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Ashutosh Sharma <ashu.coek88@gmail.com>
Discussion: https://postgr.es/m/19508-e5f188183279219b@postgresql.org
Nothing is to be gained from using pgrepack outside of REPACK
(CONCURRENTLY), and it leads to assertion failures in assertion-enabled
builds, and to crashes due to bogus memory lifetime in production
builds. Reject attempts to do that with a clean error report.
Clean up the nearby code a tad while at it. The only functional changes
in that are that the output_writer_private context is allocated and
partially filled by the pgrepack output plugin; and that ->relid therein
is now always present (rather than only in assertion-enabled builds).
Other than that it's just minor code rearrangement and added comments.
Author: Álvaro Herrera <alvherre@kurilemu.de>
Reported-by: Nikita Kalinin <n.kalinin@postgrespro.ru>
Suggested-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Antonin Houska <ah@cybertec.at> (older version)
Reviewed-by: Srinath Reddy Sadipiralla <srinath2133@gmail.com> (older version)
Discussion: https://postgr.es/m/19500-38a02529a69353a5@postgresql.org
heap_force_common() declared a boolean array indexed with an
OffsetNumber for a size of MaxHeapTuplesPerPage. OffsetNumbers are
1-based, so an input TID whose offset number equals MaxHeapTuplesPerPage
wrote one byte past the end of the stack array, crashing the server.
Like heapam_handler.c, this commit changes the array so as it uses a
0-based index, substracting one from the OffsetNumbers.
Reported-by: Wang Yuelin <violin0613@tju.edu.cn>
Reviewed-by: Ashutosh Sharma <ashu.coek88@gmail.com>
Discussion: https://postgr.es/m/20260604002256.40f1fd544@smtp.qiye.163.com
Backpatch-through: 14
Presently, refint stores plans in a per-backend cache to avoid
re-preparing in each call. This has a few problems. For one,
check_foreign_key() embeds the new key values in its cascade-UPDATE
queries, so a cached plan reuses the values from preparation.
Also, the cache is never invalidated, so it can return stale
entries that cause other problems. There may very well be more
bugs lurking.
We could spend a lot of time trying to address all these problems,
but this module is primarily intended as sample code, and by all
indications, it sees minimal use. Furthermore, there is a growing
consensus for removing refint in v20. However, since we'll need to
support it on the back-branches for a while longer, it probably
still makes sense to fix some of the more egregious bugs.
Therefore, let's just remove refint's plan cache entirely. That
means we'll re-prepare on every call, but that seems quite unlikely
to bother anyone. On v17 and older versions, the regression test
for triggers fails after this change, so I've borrowed pieces of
commit 8cfbdf8f4d to fix it.
Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: https://postgr.es/m/CAJTYsWXU%2BfhuzrEd_bnrxyGH3%2Bny8QRQC2QHf3ws6s9iki3c2Q%40mail.gmail.com
Backpatch-through: 14
xslt_process() parses the stylesheet text into an xmlDoc before passing it
to xsltParseStylesheetDoc(). On success, the returned stylesheet owns
that document and frees it through xsltFreeStylesheet(), calling
xmlFreeDoc() at its end. On failure, libxslt leaves the caller
responsible for the xmlDoc. In xml2, this would cause the memory
allocated for the xmlDoc to pile up across calls of xslt_process() when
failing to create a sheet.
While on it, I have double-checked the code of xml2 with libxslt, and it
seems that we are in the clear now, hopefully.. This leak exists for a
long time. 732061150b has made the fix introduced in this commit
easier to implement.
Author: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Discussion: https://postgr.es/m/20260605024642.5a1b6518@andrnote
Several objects were allocated by libxml2 and never released in some
error or even success paths, leading to some memory leaks that would
stack across SQL calls:
- In pgxml_xpath(), the result of xmlXPathCompiledEval() could leak.
This now uses a TRY/CATCH block to ensure a correct cleanup of a
workspace on failure.
- In xpath_table() missed some objects not freed on failure. Some
xmlFree() calls were missing for the results copied after a success.
- In pgxmlNodeSetToText(), xmlXPathCastNodeToString() allocates a result
that the caller is responsible for freeing. It was not freed.
Most of the work of this commit stands on top of 732061150b, that has
refactored xml2 to make the handling of such leaks easier. The leaks
fixed here are more ancient than the commit mentioned above, and we have
never bothered doing something about them in older stable branches.
Author: Andrey Chernyy <andrey.cherny@tantorlabs.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20260601010124.5edf9a20@andrnote
OpenSSL 4.0.0 changed some parameters and returnvalues to const, so
we need to update our declarations and subsequently cast away const-
ness from a few callsites to make libpq build without warnings. This
is tested with OpenSSL 1.1.1 through 4.0.0 as well as with LibreSSL.
No functional change is introduced, this commit only allows postgres
to be compiled against OpenSSL 4.0.0 without warnings.
There is also an errormessage change in OpenSSL 4.0.0 which needed
to be covered by our testharness.
This will be backpatched to all supported branches since they are
all equally likely to be built against OpenSSL 4.0.0 as it becomes
available in distributions. Backpatching will be done once it has
been in master for a few days without issues.
Author: Daniel Gustafsson <daniel@yesql.se>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/066B07BB-85FA-487C-BE8C-40F791CFC3C4@yesql.se
Backpatch-through: 14
Generic tags allow sublists, e.g. MERGE_JOIN((x y)), but simple
tags do not, e.g. SEQ_SCAN(x) is valid but SEQ_SCAN((x)) is not.
DO_NOT_SCAN was intended to be simple tag, but was accidentally
implemented as a generic one. This could result in assertion
failures. Repair.
Reported-by: Nikita Kalinin <n.kalinin@postgrespro.ru>
Analyzed-by: Tender Wang <tndrwang@gmail.com>
Analyzed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Discussion: http://postgr.es/m/19493-5878eac7a2525c23@postgresql.org
Otherwise, the server will crash. This can only happen if the dump
file has been corrupted or manually altered, but we should still
defend against it.
Reported-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Co-authored-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Co-authored-by: Robert Haas <rhaas@postgresql.org>
Discussion: http://postgr.es/m/CAJTYsWWYhcEx1YqC=B331-Df9EpD8MxzwswWL0okz9LLCUUpBA@mail.gmail.com
The coverage report shows that some error cases were not being tested;
add test cases for them.
While at it, move some recently added ones to the test_decoding suite:
the preventative check added in 43649b6a53 now causes servers with
wal_level=minimal to error out earlier than before.
Author: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Baji Shaik <baji.pgdev@gmail.com>
Discussion: https://postgr.es/m/ahiwD29RNfVT4tjQ@alvherre.pgsql
Previously a continuation line in the call to libpqsrv_connect_params_start()
in postgres_fdw/connection.c was indented with spaces, violating
the PostgreSQL project's whitespace rules for C files and triggering
git show --check.
This commit reformats the code to avoid that layout and use indentation
consistent with project style. No functional change.
Reported-by: Peter Eisentraut <peter@eisentraut.org>
Author: Chao Li <lic@highgo.com>
Author: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/2620a664-7b44-48b2-9aa8-5f0131702d9c@eisentraut.org
The use_scram_passthrough option in postgres_fdw and dblink accepts
only boolean values. However, unlike other boolean options such as
keep_connections, its value was not previously validated.
As a result, commands such as
"CREATE SERVER ... OPTIONS (use_scram_passthrough 'invalid')"
could succeed unexpectedly.
This commit updates postgres_fdw and dblink to validate that
use_scram_passthrough is assigned a valid boolean value, and throw an
error for invalid input.
Backpatch to v18, where use_scram_passthrough was introduced.
Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Reviewed-by: Matheus Alcantara <matheusssilv97@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwF+-k-Ehsu5W94ZP7GxS3wiBd+mi0PfGTdJ_i2Yr0zR3g@mail.gmail.com
Backpatch-through: 18
Given a WHERE clause like "int[] @@ query_int" or "query_int ~~ int[]"
where the query_int side is a table column having statistics,
_int_matchsel() exited without remembering to free the statistics
tuple. This would typically lead to warnings about cache refcount
leakage, like
WARNING: resource was not closed: cache pg_statistic (73), tuple 42/12 has count 1
It's been wrong since this code was added, in commit c6fbe6d6f.
Bug: #19492
Reported-by: Man Zeng <zengman@halodbtech.com>
Author: Man Zeng <zengman@halodbtech.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19492-ddcd0e22399ef85a@postgresql.org
Backpatch-through: 14
Previously, dblink accepted the use_scram_passthrough option on
foreign-data wrappers via ALTER FOREIGN DATA WRAPPER dblink_fdw
OPTIONS, even though the setting had no effect there.
use_scram_passthrough should be only meaningful for foreign servers
and user mappings, so this commit updates dblink to accept the option
only in those contexts.
Backpatch to v18, where use_scram_passthrough was introduced.
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEJ8rZjmbOvCicyr4vbuLio082bNTde0WNoSWaWr9wVcg@mail.gmail.com
Backpatch-through: 18
Commit 97f6fc10ff changed postgres_fdw so that user-mapping settings
override foreign server settings for use_scram_passthrough. This commit
applies the same behavior to dblink.
Backpatch to v18, where use_scram_passthrough was introduced.
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEJ8rZjmbOvCicyr4vbuLio082bNTde0WNoSWaWr9wVcg@mail.gmail.com
Backpatch-through: 18
Previously, when use_scram_passthrough was specified on both a foreign server
and a user mapping, the server-level setting took precedence over the
user-mapping setting. This was inconsistent with the usual semantics of
postgres_fdw options, where foreign server options provide shared defaults
and user mapping options override them on a per-user basis.
This commit updates postgres_fdw so that the user-mapping setting takes
precedence when use_scram_passthrough is specified in both places. This
matches the behavior of other connection options such as sslcert and sslkey.
Backpatch to v18, where use_scram_passthrough was introduced. In v18,
this only affects limited configurations that specify conflicting values
at both the foreign server and user-mapping levels. In such cases, users
would naturally expect the user-mapping setting to override the server-level
setting, so changing the behavior should be minimally disruptive.
Also keeping v18 as the only branch with different semantics for
use_scram_passthrough would be unnecessarily confusing, so backpatch
this fix to v18.
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/CAHGQGwEJ8rZjmbOvCicyr4vbuLio082bNTde0WNoSWaWr9wVcg@mail.gmail.com
Backpatch-through: 18
