Drop inv index - #99
Open
reshke wants to merge 3302 commits into
Open
Conversation
The unknown-type literals in the COLUMNS clause of a GRAPH_TABLE are now resolved to the appropriate types. Without that, this could cause various failures. Author: Satya Narlapuram <satyanarlapuram@gmail.com> Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Reviewed-by: Junwang Zhao <zhjwpku@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/CAHg%2BQDcyKNWyzDoKMxiZNjv7C-wAxs8y0ZoNkOV137Y%2Bnk3UXg%40mail.gmail.com
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 e4309f7). 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
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
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
We were skipping a bunch of things that are mostly unnecessary for REPACK. However, one thing that seems would be better to pass closer to truth, is the updatedCols bitmapset in the range table entry for the repacked table. Cons up an RTE and install it into the EState. This only has an effect on btree indexes, because certain operations are optimized in the case of unchanged columns; and even then, correctnesss is not being compromised. The values we pass after this commit are not fully trustworthy either, because we simply say "all columns were updated" for all insert/updates, regardless of whether their values were actually modified or not. However, this way we err to the side of caution rather than to the opposite direction as we were originally doing. This could be refined in the future, but there's a trade-off: determining whether the column was in fact updated could be expensive. Author: Antonin Houska <ah@cybertec.at> Reviewed-by: Ewan Young <kdbase.hack@gmail.com> Backpatch-through: 19 Discussion: https://postgr.es/m/18222.1782126731@localhost
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
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
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 ef770cb, 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 ef770cb. 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
getObjectDescription() currently constructs property graph-related object descriptions incrementally with appendStringInfo(). This effectively fixes the word order in English, which makes the messages difficult to translate naturally into languages such as Japanese. Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/20260528.121622.1662808269492494574.horikyota.ntt%40gmail.com
ON SELECT rules must be named "_RETURN", while other kinds of rules must not be; this ancient restriction is depended on by various client code. We successfully enforced this convention in most places, but ALTER RULE allowed renaming a non-SELECT rule to "_RETURN". Notably, that would break dump/restore, since the eventual CREATE RULE command would reject the name. While at it, remove DefineQueryRewrite's hack to substitute "_RETURN" for the convention that was used before 7.3. We dropped other server-side code that supported restoring pre-7.3 dumps some time ago (notably in e58a599 and nearby commits), but this bit was missed. Bug: #19543 Reported-by: Adam Pickering <adamkpickering@gmail.com> Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/19543-461228e77f3b32fc@postgresql.org Backpatch-through: 14
AlterPropGraph() cleans up pg_propgraph_property entries that are orphaned by dropping an element or by dropping properties associated with an element. But it did not clean up pg_propgraph_property entries that are orphaned by dropping labels associated with an element. Fix this missing case. Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Author: zengman <zengman@halodbtech.com> Discussion: https://www.postgresql.org/message-id/flat/tencent_76F6ACA2364EAA1E5DBD7A47%40qq.com
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
psql decided whether to use the pager in expanded output without accounting for possible wrapping of column values. This could allow it to not use the pager in cases where it should do so. To fix, move the IsPagerNeeded decision in print_aligned_vertical() down until after the wrapped data width is known. Then, if we're in wrapped mode, prepare a width_wrap array specifying that width (which, in vertical mode, is the same for all columns). This is fixing an omission in 27da1a7, so back-patch to v19 where that came in. Author: Chao Li <li.evan.chao@gmail.com> Reviewed-by: Erik Wienhold <ewie@ewie.name> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/A44110E7-6A03-4C67-95AD-527192A6C768@gmail.com 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 14ffaec. 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
pgstat_register_kind() did not validate that required callbacks are set, which could lead to NULL pointer dereferences when trying to register a stats kind. This adds a couple of checks: - Fox fixed-sized kinds, init_shmem_cb, reset_all_cb, and snapshot_cb are required. - For variable-sized kinds, flush_pending_cb is called when there is pending data, pending_size being required. These issues should be easy to notice for someone developing an extension that relies on the custom pgstats APIs. No backpatch is done as it is mainly a life improvement. Author: Sami Imseih <samimseih@gmail.com> Discussion: https://postgr.es/m/CAA5RZ0uNoe=xT7QsU1K0mMRg-QAwPtupPWZ2J3weM2PjVL2tiA@mail.gmail.com
When io_min_workers is set strictly higher than io_max_workers, the minimum has no effect since the pool will never grow past io_max_workers. Previously this was silently accepted, which could be confusing for users expecting at least io_min_workers workers to always be running. In order to avoid noise in the server logs, the following restrictions are in place: - The only process printing the WARNING is the IO worker with ID 0, on startup and reload, which is we know the only process always running when using IO workers. - At reload, the message shows only if one of the bounds has changed. Note that this commit reuses a log message updated by 7905416. Author: Baji Shaik <baji.pgdev@gmail.com> Reviewed-by: Tristan Partin <tristan@partin.io> Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://postgr.es/m/CA+fm-RO_O7-XThg2qjj=ir35x9nOFbZYu07gttqAbM5T88QB4Q@mail.gmail.com
The planner has two optimizations that move a qual clause across a grouping boundary: subquery_planner transfers HAVING clauses to WHERE so they can be evaluated before aggregation, and qual_is_pushdown_safe pushes outer restriction clauses into a subquery past its DISTINCT, DISTINCT ON, window PARTITION BY, or set-operation grouping layer. Both produce wrong results when the moved clause's equivalence relation disagrees with the grouping's, since the clause then filters rows the grouping would have merged. The disagreement has two forms. A type may belong to multiple btree opfamilies whose equality operators disagree (e.g. record_ops vs record_image_ops); or the grouping may use a nondeterministic collation, where comparing the column under a different collation, or wrapping it in a function or operator, can distinguish values the collation considers equal. Because we cannot prove an arbitrary expression preserves that equality, a grouping column with a nondeterministic collation is safe to push only as a direct operand of a comparison under its own collation. Fix both call sites through a shared walker parameterized by a callback that maps each Var to the grouping equality operator for its column (or InvalidOid for non-grouping Vars). For HAVING, the callback recovers the SortGroupClause's eqop via the GROUP Var's varattno, which requires running before flatten_group_exprs while havingQual still contains GROUP Vars. For subquery pushdown, the callback recovers the eqop from subquery->distinctClause, a window's partitionClause, or any grouping node in the SetOperationStmt tree. The walker fires only when there is an equivalence boundary to cross, gated by either the existing UNSAFE_NOTIN_DISTINCTON_CLAUSE and UNSAFE_NOTIN_PARTITIONBY_CLAUSE flags or by a recursive check for any grouping node in the set-op tree. Back-patch to v18 only. The HAVING half relies on the RTE_GROUP mechanism introduced in v18 (commit 247dea8), which is what lets us identify grouping expressions via GROUP Vars on pre-flatten havingQual. Pre-v18 branches lack that machinery, so a back-patch there would need a different approach. Given the absence of field reports of these bugs on back branches, the risk of carrying a different fix on stable branches is not justified. Author: Richard Guo <guofenglinux@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: Florin Irion <irionr@gmail.com> Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Tender Wang <tndrwang@gmail.com> Reviewed-by: Chengpeng Yan <chengpeng_yan@outlook.com> Discussion: https://postgr.es/m/CAMbWs4-QLZpn3UVOpeG2fOxxhdnkDNMZ_3Zcm3dqJwRAphz68g@mail.gmail.com Backpatch-through: 18
With virtual generated columns there is no column to assign to, and we shouldn't assign directly to stored generated columns either. (Once we have PERIODs, we will allow a stored generated column here, but we will assign to its start/end inputs.) We can't do this in parse analysis, because views haven't yet been rewritten, so they mask generated columns. Author: Paul A. Jungwirth <pj@illuminatedcomputing.com> Discussion: https://www.postgresql.org/message-id/agOOykf2HV26yVfU%40nathan
Commit 2f094e7 added a mention of SECURITY LABEL ON PROPERTY GRAPH to the SECURITY LABEL reference page, and it added support to psql tab completion. However, security labels on property graphs are not actually supported (per SecLabelSupportsObjectType()). The syntax does work, but that is just a result of how gram.y is factored. We don't document or tab-complete the syntax of SECURITY LABEL for other object types that are not actually supported, so it was inconsistent to do this for property graphs. Thus, remove this. Reported-by: Noah Misch <noah@leadboat.com> Discussion: https://www.postgresql.org/message-id/flat/20260704221210.08.noahmisch%40microsoft.com
Author: Jelte Fennema-Nio <postgres@jeltef.nl> Discussion: https://www.postgresql.org/message-id/DJPAH0WPJV3K.1PYZ8P0QXZVMX@jeltef.nl
In commit ec8719c, I added switch statements with all expected shift counts to vector8_shift_{left,right} because vshlq_n_u32() and vshrq_n_u32() require integer literals. But we can use vshlq_u32() instead for both cases, which does not require an integer literal, thereby avoiding the need for the switch statements. This compiles to the same machine code on newer versions of popular compilers. Reviewed-by: John Naylor <johncnaylorls@gmail.com> Discussion: https://postgr.es/m/akWxkA-mszMm57cV%40nathan
Commit f3b0897 fixed some related problems, but overlooked this one. That commit first appeared in PostgreSQL 11, so back-patch to all supported branches. Backpatch-through: 14 Discussion: http://postgr.es/m/CA+TgmobsvQw3F+KRYT83=N3teh8D2t-oPR=U06QDZJE3viCJRg@mail.gmail.com Reviewed-by: Tender Wang <tndrwang@gmail.com> Reviewed-by: Ewan Young <kdbase.hack@gmail.com>
Commit 85b7efa introduced support for LIKE with non-deterministic collations. By moving some conditionals around, it accidentally broke the optimization for converting a LIKE or regex exact-match pattern to an equality indexqual when the index collation doesn't match the expression collation. That should be allowed if the expression collation is deterministic. This patch re-introduces the optimization for that common case. One important beneficiary of this optimization is the "\d tablename" command in psql. Without this fix that will do a seqscan on pg_class instead of an index point lookup. Reported-by: Andres Freund <andres@anarazel.de> Author: Jelte Fennema-Nio <postgres@jeltef.nl> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/DHBQIZX8SZVI.ZX614ZMFL645@jeltef.nl Backpatch-through: 18
This macro is supposed to work like ereport(). But when 59c2f03 adjusted ereport() to be more MSVC-friendly, it missed updating this copy of the logic. Discussion: https://postgr.es/m/754534.1783264708@sss.pgh.pa.us Backpatch-through: 19
Commit 85b7efa added support for LIKE with nondeterministic collations, but it included a bug in the de-escaping logic for literal pattern substrings. That unconditionally skipped all backslashes, but when it encounters '\\' it should emit the second backslash as a de-escaped character. That led to acting as though the escaped backslash was not there. Bug: #19474 Reported-by: Bowen Shi <zxwsbg12138@gmail.com> Author: Nitin Motiani <nitinmotiani@google.com> Reviewed-by: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Ewan Young <kdbase.hack@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/19474-5b86a95f3d9a7ecb@postgresql.org Discussion: https://postgr.es/m/CAH5HC94yU+K8Gcdy12M5BS8gwD_SXLSHzc9k5tNk7JDnpBiFMA@mail.gmail.com Backpatch-through: 18
The loop in MatchText() processed a leading '\' without regard to nondeterministic locales, which is problematic if what the '\' precedes is an ordinary character that should be subject to nondeterministic matching. We'd insist on a literal match for it, which is not right and is not like what happens with a '\' that follows some ordinary characters. Worse, we'd then advance the text and pattern pointers by one byte, so that if the escaped character is multibyte the next loop iteration would take the nondeterministic code path starting at a point within the character. That could very possibly cause pg_strncoll() to misbehave. The fix is quite simple: move the stanza that handles '\' down past the one that handles nondeterminism. The stanzas for '%' and '_' are fine where they are, but the '\' stanza is only correct for deterministic matching. The logic for nondeterministic cases is already prepared to do the right things with a '\'. While here, I replaced tests of "locale && !locale->deterministic" with a boolean local variable, reasoning that those are in the hot loop paths so saving a branch and indirect fetch is worth the trouble. I also improved a number of related comments. Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/391592.1783187986@sss.pgh.pa.us Backpatch-through: 18
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
transformJsonBehavior() coerced an ON EMPTY / ON ERROR DEFAULT
expression only when its type differed from the RETURNING type's OID.
When the base type matched but the RETURNING type carried a type
modifier (e.g. numeric(4,1) or varchar(3)), the coercion that enforces
the typmod was skipped, so the DEFAULT value could violate the
declared type:
SELECT JSON_VALUE(jsonb '{}', '$.a'
RETURNING numeric(4,1) DEFAULT 99999.999 ON EMPTY);
returned 99999.999, which 99999.999::numeric(4,1) would reject; the
value could even be stored into a numeric(4,1) column, as later
coercions trust its already-correct type label.
Fix by also coercing when the RETURNING type has a typmod, except for
a NULL constant. coerce_to_target_type() is a no-op when the typmod
already matches. The matching-OID short-circuit dates to 74c9669.
Reported-by: Ewan Young <kdbase.hack@gmail.com>
Author: Ewan Young <kdbase.hack@gmail.com>
Discussion: https://postgr.es/m/CAON2xHPO9f4cAmyGn1mQ=VqoS7wN5rz4yOiqudxX78zninZpCw@mail.gmail.com
Backpatch-through: 17
When GROUP BY ALL was added in commit ef38a4d, the SQL standard working draft was silent on what to do with window functions. This has now been fixed in the SQL standard working draft. Update the documentation and code comments about that. Also make the documentation more specific that we are only talking about aggregate functions referring to the same query level, which is another thing that has been made more precise in the SQL standard working draft since. The PostgreSQL implementation was already doing the right thing for both aspects, so no functionality changes. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://www.postgresql.org/message-id/flat/CAHM0NXjz0kDwtzoe-fnHAqPB1qA8_VJN0XAmCgUZ%2BiPnvP5LbA%40mail.gmail.com
Commit 28972b6 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
Commit 181b618 failed to do anything useful with a whole-row Var, deeming it "fishy". But it is legal to put such a Var into an expression index column, so let's expand it as the name of the table. Another problem reachable via that one is that we could generate an empty index column name, which isn't really legal although by chance nothing complained about it. It's not clear whether any other such cases remain, but as cheap insurance let's use "expr" if the tree walk fails to generate any text. Reported-by: Chauhan Dhruv <chauhandhruv351@gmail.com> Author: Chauhan Dhruv <chauhandhruv351@gmail.com> Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CANWwWcp_DCJjq8pomeqp6W=fbygvzXXQO028VDJ9_6sLPjQnVA@mail.gmail.com
index_create_copy is used to create copy definitions of existing indexes. Currently, it passes 0 as constr_flags to index_create(), which results in the copied index to always be created as immediate (indimmediate set to true). For deferrable unique constraints, it means that the transient index used during the phase 2 of REINDEX CONCURRENTLY forces immediate constraint checks on concurrent inserts, which can cause unexpected constraint violations based on the definition of the parent table, inconsistently set in the copied index. To fix this without violating the contract of constr_flags (which should only be used when creating constraints) and without relaxing the strict assertion in index_create(), this introduces a new index creation flag: INDEX_CREATE_DEFERRABLE. If set, a copied index's indimmediate is set to false, meaning that unique constraints are not enforced immediately on insertion, but at transaction commit time. An isolation test for REINDEX CONCURRENTLY is added, based on an injection point waiting after phase 1 of the operation, where an index copy has been built and is able to accept DMLs for its validation in phase 2. The test is tentatively backpatched down to v17. INJECTION_POINT() is outside a transaction context, which should be fine on HEAD since 8daeaa9 but I suspect may cause issues in v19 and older branches due to the wait facility depending on condition variables and a DSM setup, but let's see what the buildfarm tells. Author: Nitin Motiani <nitinmotiani@google.com> Discussion: https://postgr.es/m/CAH5HC97JmjPpgiQOqW9xm8qXhNiu7zZ1Qh+FfhEESJuDv69kuQ@mail.gmail.com Backpatch-through: 14
The mapped user name is built upon the OS user name of the environment where the test is run. Depending on the characters used in the OS user name, CREATE ROLE may not get parsed (the author has mentioned hyphens as one case), causing a failure of the test. Let's use double-quotes around the mapped user name, which should be a solution good enough for the environments where this test tends to run. The buildfarm issued no complaint over the years. Oversight in 3c4e26a, so backpatch down to v19. Perhaps 3c4e26a and this commit should be backpatched further down, but let's leave that for another day, if it proves necessary. Author: Yugo Nagata <nagata@sraoss.co.jp> Discussion: https://postgr.es/m/20260727133857.fbd23d43d422f10f376a8bee@sraoss.co.jp Backpatch-through: 19
UPDATE/DELETE ... FOR PORTION OF inserts leftover rows for the untouched parts of the original row. These hidden inserts should not affect the command tag or ROW_COUNT, so they call ExecInsert() with canSetTag set to false. However, ExecInsert() still processed the RETURNING list whenever the target ResultRelInfo had ri_projectReturning set. That caused RETURNING expressions to be evaluated for leftover rows even though their results were discarded. As a result, expressions with side effects and information-leaking functions could be executed on the leftover rows, in addition to the visibly updated or deleted row. Fix by having ExecInsert() skip RETURNING processing when it is handling an internal FOR PORTION OF leftover insert. Use both the presence of a FOR PORTION OF clause and mtstate->operation == CMD_INSERT for this check, so that the auxiliary INSERT of a cross-partition UPDATE with a FOR PORTION OF clause still processes RETURNING normally. Back-patch to v19, where support for FOR PORTION OF was added. Author: Chao Li <lic@highgo.com> Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com> Reviewed-by: Paul A Jungwirth <pj@illuminatedcomputing.com> Discussion: https://postgr.es/m/07C125E5-F6ED-460C-A394-E6503DAE18FB@gmail.com Backpatch-through: 19
Commit fd83c83 turned the recursive posting-tree cleanup in ginVacuumPostingTreeLeaves() into an iterative sweep that follows the tree's leaf pages via their rightlinks. The recursive version called vacuum_delay_point() while processing the tree, but that call was removed and never re-added to the new loop. As that commit only set out to fix a deadlock, the removal appears to have been unintentional. Consequently the leaf-page sweep of a single posting tree runs with no vacuum_delay_point(), and therefore no CHECK_FOR_INTERRUPTS(). A posting tree stores all the TIDs for one indexed key, so for a frequently occurring key it can span a large number of leaf pages. While such a tree is being vacuumed the operation ignores vacuum_cost_delay and does not respond to query cancellation or statement_timeout; an autovacuum worker likewise cannot be interrupted mid-sweep when another backend requests a conflicting lock. Restore the call, placed after the current page has been unlocked and released so that no buffer content lock is held across a potential delay (cf. 21c27af). The sibling loops in ginbulkdelete() and ginvacuumcleanup() already call vacuum_delay_point() once per page. Author: Paul Kim <mok03127@gmail.com> Co-authored-by: Alexander Korotkov <aekorotkov@gmail.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru> Reviewed-by: solai v <solai.cdac@gmail.com> Discussion: https://postgr.es/m/178447127453.110.12276981925360691905%40mail.gmail.com Backpatch-through: 14
pg_get_publication_tables() collects the OIDs of the published tables on its first call, without locking them, and then reopens each table later, once per result row, to compute its column list and fetch its row filter. The reopen used table_open(), which errors out with "could not open relation with OID" if the table has been dropped in the meantime. This could happen for any published table without an explicit column list, which is every table in FOR ALL TABLES and FOR TABLES IN SCHEMA publications, but also FOR TABLE entries without a column list. The failure is common in environments where many tables are created and dropped while publication tables are being queried, e.g. by table synchronization on a subscriber. Fix by opening every table with try_table_open(), which returns NULL if the relation no longer exists, and skipping the table in that case. Concurrently dropped tables are thus simply absent from the result set, which is the expected point-in-time behavior. As a side effect, tables with an explicit column list, which were previously returned without being opened, are now also locked with AccessShareLock, so the function can block behind concurrent DDL on such tables where it previously did not. Backpatch to v16, where we added the table_open() call in pg_get_publication_tables(). Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com> Reviewed-by: shveta malik <shveta.malik@gmail.com> Reviewed-by: Ajin Cherian <itsajin@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://www.postgresql.org/message-id/CALj2ACVYYooWH-5tJ6cPKkU%2BmutVxwb_z4S%2BqAi-zdrFqxXE2Q%40mail.gmail.com Backpatch-through: 16
A two-phase transaction that is assigned an XID but produces no change to be decoded -- for example, one that only acquires row locks via SELECT ... FOR SHARE -- has no base snapshot in the reorder buffer. ReorderBufferReplay() already skips such a transaction at PREPARE time and never invokes the begin_prepare/change/prepare callbacks for it, but ReorderBufferFinishPrepared() still called the commit_prepared (or rollback_prepared) callback. As a result a spurious COMMIT/ROLLBACK PREPARED was sent to the output plugin with no preceding PREPARE. For the built-in subscriber this breaks replication (the apply worker fails to find the prepared transaction), and test_decoding could even crash. Fix this by detecting an empty transaction (base_snapshot == NULL) in ReorderBufferFinishPrepared() and cleaning it up without invoking the commit/rollback prepared callbacks, mirroring the existing empty transaction handling in ReorderBufferReplay(). On v18 and newer versions, commit 072ee84 changed ReorderBufferPrepare() to send the prepare whenever it had not already been sent, which also fires for empty transactions and emits a spurious PREPARE. On those branches ReorderBufferPrepare() is therefore additionally guarded with base_snapshot != NULL. This guard and the Assert(!rbtxn_sent_prepare()) added in ReorderBufferFinishPrepared(), are not necessary on v17 and older versions: there ReorderBufferPrepare() only sends a prepare for concurrently-aborted transactions (which never applies to an empty transaction) and the RBTXN_SENT_PREPARE flag does not exist. Back-patch to v14, where decoding of two-phase transactions was introduced. Bug: #19556 Reported-by: Alexander Kozhemyakin <a.kozhemyakin@postgrespro.ru> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://postgr.es/m/19556-daa6d7ea65054d48@postgresql.org Backpatch-through: 14
The file_copy strategy check in createdb() runs during option validation, before the transaction has an XID and before the pg_database row exists, so the datachecksumsworker launcher can start in that window and see neither the new database nor the transaction creating it. It then raw-copies a template that was not processed yet, and those files stay unchecksummed, failing verification from then on. Recheck the state in CreateDatabaseUsingFileCopy(): the XID is assigned by then, so a launcher starting after this point waits for the transaction and finds the new database, and the copy errors out instead. Add an injection point before the catalog insert to test the window. Backpatch to v19 where online checksums were introduced. Author: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAN4CZFPEBsz8JeY4ixQ1V4ZL_xOY6pJaZS8ZLGH7R+wF--pEtg@mail.gmail.com Backpatch-through: 19
Enable errors out early with a hint when an invalid database exists, since the worker cannot connect to it and its files stay on disk. A worker that started but failed gets the same dropped-database heuristic as one that failed to start, so a concurrent drop during processing no longer aborts the whole run. The existence check locks the database first, otherwise a DROP DATABASE ... WITH (FORCE) which killed the worker is still only halfway done and the database looks like it is there to stay. Backpatch to v19 where online checksums were introduced. Author: Zsolt Parragi <zsolt.parragi@percona.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAN4CZFOGdqxtZ5-6gb4apqmvoH=Z+TNH8RKJ3mVtoR1HirKQWg@mail.gmail.com Backpatch-through: 19
find_nonnullable_rels and find_nonnullable_vars mistakenly treated a ScalarArrayOpExpr that could return FALSE as strict, but that's okay only at top level of a qual expression; further down, we've got to insist on a guaranteed-NULL result. The result was that we could draw mistaken conclusions about whether outer joins can be simplified, if the decision hinged on a non-top-level ScalarArrayOpExpr with a potentially-empty array argument. I believe this error dates to commit 72a070a, which taught find_nonnullable_rels to descend into non-top-level parts of qual expressions. is_strict_saop (added earlier by 72153c0) already had enough intelligence to do the case correctly, but it wasn't passed the proper flag, ie "top_level" needs to be passed for "falseOK". e006a24 copied that mistake into find_nonnullable_vars. Later, over-eager refactoring in commit 2f153dd broke contain_nonstrict_functions' handling of ScalarArrayOpExpr by treating it as though it were no different from an OpExpr. It is, because we must also prove the array is non-empty before concluding that the expression is strict. This could result in misclassifying an expression as strict when it is not, leading to assorted planning mistakes such as inlining a SQL function that shouldn't be inlined. We can almost fix this by just re-adding the previous handling of ScalarArrayOpExpr in that function, but doing only that would lead to also calling check_functions_in_node() and thus redundantly checking the operator's strictness. Avoid that by turning the if-series into an else-if chain, as it arguably should have been all along. The reason these errors have escaped detection for decades is that they are exposed only in arcane corner cases. ScalarArrayOpExpr with an empty array isn't typical usage, and even when that's possible several other conditions apply before the planner can reach a mistaken conclusion. While it's possible to build test cases demonstrating these mistakes, I (tgl) judged them too indirect and special-purpose to justify consuming regression test cycles forevermore. Author: Ayush Tiwari <ayushtiwari.slg01@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CAJTYsWV3vqRJmST-gv1NsXEef-zOnjVJpYS910aBaiuMij4nFg@mail.gmail.com Discussion: https://postgr.es/m/CAJTYsWWcLGmz0f8_QPP_Liq-fc7-geiFSCdqoq3XGeRHPPsWeA@mail.gmail.com Backpatch-through: 14
This commit replaces two calls of strcpy() and one call of strncpy() to use strlcpy(), which are patterns that static analyzers (mostly LLMs, it seems) have been complaining regarding buffer overflow risks. The existing calls are safe, here are more details for each one of them: - MarkAsPreparingGuts()'s strcpy() was guarded by MarkAsPreparing(). - PrepareRedoAdd()'s strcpy() is safe because the record-level CRC check prevents corrupted data from reaching it unless intentionally crafted. The replay code also assumes that the GID is within the allowed bounds, as WAL records are trusted. - Similarly, ParsePrepareRecord() stores its GID in a buffer bounded by GIDSIZE while trusting the length provided by the record. As a result, these changes are purely cosmetic. They adopt a more defensive coding style and should also silence some of the static analysis reports received recently. Author: Matt Suiche <matt@tolmo.com> Discussion: https://postgr.es/m/CAGf6Lfx2kbQfcEnCi99V2i65JSWD6ij_E29F+UkY=TyMUyeG6A@mail.gmail.com
While collecting the sequences to synchronize, the sequence sync worker opened each INIT sequence with RowExclusiveLock and held it until the transaction committed. With many such sequences, this could exhaust the shared lock table and fail with "out of shared memory". The worker only reads each sequence's identity (namespace and name) here and needs it to stay stable while read, for which AccessShareLock is enough, as it conflicts with the AccessExclusiveLock taken by DROP, RENAME, and SET SCHEMA. Take that lock instead and release it as soon as the identity is read. The later synchronization re-opens each sequence, so it does not rely on the lock being retained. Reported-by: Noah Misch <noah@leadboat.com> Author: vignesh C <vignesh21@gmail.com> Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Backpatch-through: 19, where it was introduced Discussion: https://postgr.es/m/20260710045217.f0.noahmisch@microsoft.com
TerminateBackgroundWorkersForDatabase() uses BackendPidGetProc() and, until now, accessed fields of the returned PGPROC after releasing ProcArrayLock, including its database OID. If the PGPROC slot is recycled during this window, the database OID being checked may belong to a different backend, causing an unrelated background worker to be terminated. Triggering this bug requires a very narrow race: the background worker identified by BackendPidGetProc() must exit, its PGPROC slot must be released and reused, and only then must TerminateBackgroundWorkersForDatabase() examine the database OID. TerminateBackgroundWorkersForDatabase() holds BackgroundWorkerLock, preventing parallel workers and dynamically registered workers (such as those created by worker_spi) from reusing the slot. As far as I know, the only plausible scenario is a static background worker that exits and is restarted quickly enough to reuse the same PGPROC slot within the race window. In practice, this race is extremely unlikely, still reachable in theory. Oversight in f1e251b. Author: Chao Li <li.evan.chao@gmail.com> Reviewed-by: Aya Iwata <iwata.aya@fujitsu.com> Reviewed-by: Haibo Yan <tristan.yim@gmail.com> Discussion: https://postgr.es/m/78E81763-EA1D-4788-9741-4092BCB997A5@gmail.com Backpatch-through: 19
The getdatabaseencoding function was added in bf00bbb in 1998 but was never documented. While mostly used in tests, there is no reason not to document it as this function isn't going anywhere and is already used in extensions. Author: Ian Barwick <barwick@gmail.com> Reviewed-by: Thom Brown <thom@linux.com> Reviewed-by: surya poondla <suryapoondla4@gmail.com> Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Discussion: https://postgr.es/m/CAB8KJ=ij+pznQGub=DkyJuKL=tC=Q=07qSahTyw7TLb0DdNJsg@mail.gmail.com
A cascading standby could fail to reconnect to its upstream standby with "requested starting point ... is ahead of the WAL flush position" after falling back to archive recovery. This happened because archive recovery processes whole segment files, so after replaying a segment the cascade's next read position lands at the start of the following segment, which is ahead of the upstream's flush position reported by GetStandbyFlushRecPtr() (still inside the just-replayed segment). Fix by having the walreceiver check the upstream's current WAL flush position via IDENTIFY_SYSTEM before issuing START_REPLICATION. IDENTIFY_SYSTEM already returns this position (as xlogpos), but walrcv_identify_system() previously discarded it; now we have a use for it. If the requested start point exceeds the upstream's flush position on the same timeline, the walreceiver waits for wal_retrieve_retry_interval and retries. The wait is limited to gaps of at most one WAL segment, which is the expected case from the segment-granularity of archive recovery. Larger gaps indicate the upstream is genuinely behind, so START_REPLICATION is allowed to proceed (and fail) normally, letting the startup process fall back to other WAL sources. The first wait is logged at LOG level; subsequent waits are demoted to DEBUG1 to avoid log noise. The walreceiver honors wal_receiver_timeout during the wait, so it will exit if the upstream doesn't catch up in time. To preserve ABI compatibility on back branches, the flush position from IDENTIFY_SYSTEM is communicated via a new global variable (WalRcvIdentifySystemLsn) rather than changing the signature of walrcv_identify_system(). The bug was introduced in Postgres 9.3 by commit abfd192, which added a flush-position check in StartReplication() that rejects requests ahead of the upstream server's WAL flush position. Author: Marco Nenciarini <marco.nenciarini@enterprisedb.com> Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com> Backpatch-through: 14 Discussion: https://postgr.es/m/CA+nrD2cTuTkkX5WXVZengTYYZbAO6zV8K+Tri-R0fbLFuoyMBA@mail.gmail.com
Two of the permutations in backwards-scan-concurrent-splits rely on their VACUUM step deleting the leaf pages that the waiting backwards scan will have to recover from. VACUUM can only do that when it's able to remove the index tuples whose heap tuples the concurrent session just deleted. An autovacuum worker holding a snapshot holds back the removable cutoff, which leaves the pages non-empty, and so undeleted, causing the test to fail spuriously. To fix, wait for the removable cutoff to advance past the deletions before the scan acquires its snapshot. This is much like commit 1c64d2f, which dealt with the same hazard in nbtree_half_dead_pages by adding the wait_prunable() helper that we reuse here. Oversight in commit e395fbd. Author: Peter Geoghegan <pg@bowt.ie> Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/b61d9944-d7a3-45f3-b69a-f18c8bfbbbd0@gmail.com
The comment claimed that a parallel vacuum worker has only the PROC_IN_VACUUM flag because parallel vacuum is not supported for autovacuum, but commit 1ff3180 allowed autovacuum to use parallel vacuum workers. The assertion itself still holds: the leader, whether a backend running VACUUM or an autovacuum worker, sets PROC_IN_VACUUM before taking its snapshot, and a parallel worker inherits the flag when importing the leader's snapshot. The leader's other flags don't reach the worker, since the snapshot import copies only the PROC_XMIN_FLAGS bits and PROC_IS_AUTOVACUUM is never set on parallel workers, which run as regular background workers. Reword the comment to explain that. Oversight in commit 1ff3180. Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com> Reviewed-by: Chao Li <li.evan.chao@gmail.com> Discussion: https://postgr.es/m/CALj2ACVwQ4WABqq8Lnf+VZEJ45jcTFhyFLFr_ctfS4=QLL-r5w@mail.gmail.com Backpatch-through: 19
Restructuring the tags makes the output consistent and doesn't require added spaces. Reported-by: Peter Smith Author: Peter Smith Discussion: https://postgr.es/m/CAHut+Pu8JahGm76CMdpzH350pHJedA4R2b8JmOim3+m3yxft3Q@mail.gmail.com Backpatch-through: 19
If the TLS init hook is defined in conjunction with ssl_sni we issue a warning to help the user re-configure the cluster. To avoid drowning the log in warnings, we log only once instead of once per host. This removes the static variable tracking the warning to aid future multithreading efforts. Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Tristan Partin <tristan@partin.io> Reviewed-by: Andreas Karlsson <andreas@proxel.se> Reviewed-by: Yilin Zhang <jiezhilove@126.com> Discussion: https://postgr.es/m/68B9881D-DAA8-467D-A251-C96E98E57BA0@yesql.se
The SSLv23_method() function has been an alias for TLS_method since 2015 (OpenSSL commit 32ec41539b5b) so we should use the appropriate name to avoid confusion. Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Tristan Partin <tristan@partin.io> Reviewed-by: Andreas Karlsson <andreas@proxel.se> Reviewed-by: Yilin Zhang <jiezhilove@126.com> Discussion: https://postgr.es/m/68B9881D-DAA8-467D-A251-C96E98E57BA0@yesql.se
X509_NAME_get_text_by_NID was deprecated in OpenSSL 4.0.0, and could be removed in a future version of OpenSSL. The replacement APIs are available in all versions of OpenSSL and LibreSSL that we support so we can easily change to make the code future proof. The reason for the deprecation is that X509_NAME_get_text_by_NID can only grab the first entry in a list, and doesn't handle multibyte strings well. The fix is to get the index of the name entry with X509_NAME_get_index_by_NID and use X509_NAME_get_entry to extract the data. Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Tristan Partin <tristan@partin.io> Reviewed-by: Andreas Karlsson <andreas@proxel.se> Reviewed-by: Yilin Zhang <jiezhilove@126.com> Discussion: https://postgr.es/m/68B9881D-DAA8-467D-A251-C96E98E57BA0@yesql.se
Our test for if the underlying TLS library supported a specific version tested against the TLSX_Y_VERSION set of macros. These are however always defined, regardless of if the library was built without support for the specific protocol version. Fix by using the feature test macros OPENSSL_NO_TLSX_Y which are intended for this usecase. The previous coding held no risk of protocol downgrade against the underlying library, a library not supporting the protocol version selected would simply error out as the feature isn't available. This can be easily verified using a modern version of LibreSSL, which in version 3.8 disabled TLS1 and 1.1 by default. Once we bump our minimum supported version of LibreSSL to 3.8+ we can add a test for this. Author: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Tristan Partin <tristan@partin.io> Reviewed-by: Andreas Karlsson <andreas@proxel.se> Reviewed-by: Yilin Zhang <jiezhilove@126.com> Discussion: https://postgr.es/m/68B9881D-DAA8-467D-A251-C96E98E57BA0@yesql.se
Commits ca326e9 and 1f8c504 widened the byte-count arguments of smgr__md__read__done and smgr__md__write__done to "long long int". SystemTap's dtrace(1) fails on that specific spelling and falls back with a warning (misreported near the previous probe). Use ssize_t and size_t instead, matching the md.c call sites. Revise probes.d's note about which types are usable as probe arguments: recommend using system-supplied type names (macOS dtrace rejects names like PostgreSQL's uint64), and call out "long long int" as a known SystemTap failure case rather than a wider failure mode. Also, update the monitoring.sgml entries for these probes, which were missed by the prior commits. Reported-by: Laurenz Albe <laurenz.albe@cybertec.at> Author: Andrey Rachitskiy <pl0h0yp1@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/697d3c88568442cd637d8453d129f1bb14bbd2a8.camel@cybertec.at
pg_dump in --binary-upgrade mode emits "SUBSCRIPTION TABLE" TOC entries to preserve pg_subscription_rel state across pg_upgrade. When such a dump was restored with --no-subscriptions, _tocEntryRequired() skipped the "SUBSCRIPTION" entry but not the associated "SUBSCRIPTION TABLE" entries, so the restore would try to apply subscription-relation state for a subscription that was never created. Skip "SUBSCRIPTION TABLE" entries as well when no_subscriptions is set. This can happen when pg_subscription_rel has entries, the dump is taken with --binary-upgrade, and it is restored with --no-subscriptions. Reported-by: Hayato Kuroda <kuroda.hayato@fujitsu.com> Author: Hayato Kuroda <kuroda.hayato@fujitsu.com> Reviewed-by: Shlok Kyal <shlok.kyal.oss@gmail.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Backpatch-through: 17, where it was introduced Discussion: https://postgr.es/m/OS9PR01MB121493DA4C1A7748B11A646D8F5C02@OS9PR01MB12149.jpnprd01.prod.outlook.com
The new test for enabling data checksums with concurrent CREATE
DATABASE calls use the same injection points as a previous test
but accidentally missed detaching the injection point first.
Fix by detaching the injection point in the PG_TEST_EXTRA SKIP
block to make it can be reused. Pointed out by buildfarm member
porpoise which failed with:
die: error running SQL: 'psql:<stdin>:1:
ERROR: injection point "datachecksumsworker-fake-temptable-wait"
already defined'
Backpatch to v19 where online checksums were introduced.
Author: Daniel Gustafsson <daniel@yesql.se>
Reported-by: Buildfarm member porpoise
Reviewed-by: Jonathan Gonzalez V. <jonathan.abdiel@gmail.com>
Discussion: https://postgr.es/m/28CF6FD9-E1C4-4C04-8270-E3305AC46171@yesql.se
Backpatch-through: 19
Index builds update pg_class.reltuples for the table. In parallel GIN builds, workers track the number of processed rows, and report it to the leader, who then updates the pg_class with a total. However, gin_parallel_build_main failed to initialize the bs_reltuples field, leaving it set to whatever happens to be on the stack (which may be bogus values like Infinity or NaN, or just impossibly high values). If such values get reported to the leader and stored in pg_class, that can have serious consequences. The pg_class.reltuples field is used to decide when a table is due for autovacuum or autoanalyze, and if it happens to be set to a bogus value, that may never happen. The field is also used by the optimizer when calculating costs. Fixed by initializing bs_reltuples together with the rest of the build state. The bs_numtuples was initialized later, but it seems cleaner to just initialize all the fields at once. After a bogus value gets persisted in pg_class, affected systems are unlikely to self-heal. That would require an ANALYZE, but preventing that is one of the consequences. We have considered forcing autoanalyze in these cases, but there's not a good way to reliably identify bogus values (except for a small minority like Infitiny/NaN). A manual ANALYZE on (possibly) affected tables is the only solution. Backpatch to 18, where parallel GIN builds were introduced. Reported-by: Jan Nidzwetzki <jan@planetscale.com> Discussion: https://postgr.es/m/518BA772-8026-412A-AA8F-A7FE4C6B3717@planetscale.com Backpatch-through: 18
When restoring relation stats, pg_restore_relation_stats() rejected calls with (reltuples < -1.0). But that is insufficient - Infinity and NaN values both pass that check, and get stored in pg_class verbatim. This can have various undesirable consequences. Fixed by rejecting non-finite reltuple values, in the same non-fatal way as for the existing checks (emit WARNING and skip the update). Adds a regression test to stats_import for these non-finite values, and to check the -1.0 special value is still accepted. Backpatch to 18, where pg_restore_relation_stats() was introduced. Patch by Jan Nidzwetzki, minor commit message tweaks by me. Author: Jan Nidzwetzki <jan@planetscale.com> Discussion: https://postgr.es/m/518BA772-8026-412A-AA8F-A7FE4C6B3717@planetscale.com Backpatch-through: 18
This patch adds new builtin function
pg_drop_invalid_indexes, which helps with removal of invalid
indexes that can appear after failed CREATE INDEX CONCURRENTLY
operations or etc. Previously required manual queries against
pg_index, which was error-prone.
Example of usage:
postgres=# \d+ sas
Table "public.sas"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+----------+-------------+--------------+-------------
id | integer | | | | plain | | |
t | text | | | | extended | | |
Indexes:
"idx1" btree (id) INVALID
Access method: heap
postgres=# select * from pg_drop_invalid_indexes('sas');
-[ RECORD 1 ]-----
index_name | idx1
index_oid | 16842
postgres=# \d+ sas
Table "public.sas"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
--------+---------+-----------+----------+---------+----------+-------------+--------------+-------------
id | integer | | | | plain | | |
t | text | | | | extended | | |
Access method: heap
Co-authored-by: Kirill Reshke <reshkekirill@gmail.com>
Signed-off-by: Roman Khapov <r.khapov@ya.ru>
Fix for empty invalid index list
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.