aboutsummaryrefslogtreecommitdiff
path: root/src/backend
Commit message (Collapse)AuthorAge
* Fix rescanning of async-aware Append nodes.Etsuro Fujita2021-06-07
| | | | | | | | | | | | | | | | | | | | | In cases where run-time pruning isn't required, the synchronous and asynchronous subplans for an async-aware Append node determined using classify_matching_subplans() should be re-used when rescanning the node, but the previous code re-determined them using that function repeatedly each time when rescanning the node, leading to incorrect results in a normal build and an Assert failure in an Assert-enabled build as that function doesn't assume that it's called repeatedly in such cases. Fix the code as mentioned above. My oversight in commit 27e1f1456. While at it, initialize async-related pointers/variables to NULL/zero explicitly in ExecInitAppend() and ExecReScanAppend(), just to be sure. (The variables would have been set to zero before we get to the latter function, but let's do so.) Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/CAPmGK16Q4B2_KY%2BJH7rb7wQbw54AUprp7TMekGTd2T1B62yysQ%40mail.gmail.com
* Fix inconsistent equalfuncs.c behavior for FuncCall.funcformat.Tom Lane2021-06-06
| | | | | | | | | | | | | | | | | Other equalfuncs.c checks on CoercionForm fields use COMPARE_COERCIONFORM_FIELD (which makes them no-ops), but commit 40c24bfef neglected to make _equalFuncCall do likewise. Fix that. This is only strictly correct if FuncCall.funcformat has no semantic effect, instead just determining ruleutils.c display formatting. 40c24bfef added a couple of checks in parse analysis that could break that rule; but on closer inspection, they're redundant, so just take them out again. Per report from Noah Misch. Discussion: https://postgr.es/m/20210606063331.GC297923@rfd.leadboat.com
* Add transformed flag to nodes/*funcs.c for CREATE STATISTICSTomas Vondra2021-06-06
| | | | | | | | | | | Commit a4d75c86bf added a new flag, tracking if the statement was processed by transformStatsStmt(), but failed to add this flag to nodes/*funcs.c. Catversion bump, due to adding a flag to copy/equal/out functions. Reported-by: Noah Misch Discussion: https://postgr.es/m/ad7891d2-e90c-b446-9fe2-7419143847d7%40enterprisedb.com
* Standardize nodes/*funcs.c cosmetics for ForeignScan.resultRelation.Noah Misch2021-06-06
| | | | catversion bump due to readfuncs.c field order change.
* Clean up some questionable usages of DatumGet* macrosDavid Rowley2021-06-04
| | | | | | | | | | | | | | | | | | | | | This tidies up some questionable coding which made use of DatumGetPointer() for Datums being passed into functions where the parameter is expected to be a cstring. We saw no compiler warnings with the old code as the Pointer type used in DatumGetPointer() happens to be a char * rather than a void *. However, that's no excuse and we should be using the correct macro for the job. Here we also make use of OutputFunctionCall() rather than using FunctionCall1() directly to call the type's output function. OutputFunctionCall() is the standard way to do this. It casts the returned value to a cstring for us. In passing get rid of a duplicate call to strlen(). Most compilers will likely optimize away the 2nd call, but there may be some that won't. In any case, this just aligns the code to some other nearby code that already does this. Discussion: https://postgr.es/m/CAApHDvq1D=ehZ8hey8Hz67N+_Zth0GHO5wiVCfv1YcGPMXJq0A@mail.gmail.com
* Adjust locations which have an incorrect copyright yearDavid Rowley2021-06-04
| | | | | | | A few patches committed after ca3b37487 mistakenly forgot to make the copyright year 2021. Fix these. Discussion: https://postgr.es/m/CAApHDvqyLmd9P2oBQYJ=DbrV8QwyPRdmXtCTFYPE08h+ip0UJw@mail.gmail.com
* Fix incorrect permissions on pg_subscription.Tom Lane2021-06-03
| | | | | | | | | | | | | | | | | The documented intent is for all columns except subconninfo to be publicly readable. However, this has been overlooked twice. subsynccommit has never been readable since it was introduced, nor has the oid column (which is important for joining). Given the lack of previous complaints, it's not clear that it's worth doing anything about this in the back branches. But there's still time to fix it inexpensively for v14. Per report from Israel Barth (via Euler Taveira). Patch by Euler Taveira, possibly-vain comment updates by me. Discussion: https://postgr.es/m/b8f7c17c-0041-46b6-acfe-2d1f5a985ab4@www.fastmail.com
* Reduce risks of conflicts in internal queries of REFRESH MATVIEW CONCURRENTLYMichael Paquier2021-06-03
| | | | | | | | | | | | | | | | | | | | | | The internal SQL queries used by REFRESH MATERIALIZED VIEW CONCURRENTLY include some aliases for its diff and temporary relations with rather-generic names: diff, newdata, newdata2 and mv. Depending on the queries used for the materialized view, using CONCURRENTLY could lead to some internal failures if the matview query and those internal aliases conflict. Those names have been chosen in 841c29c8. This commit switches instead to a naming pattern which is less likely going to cause conflicts, based on an idea from Thomas Munro, by appending _$ to those aliases. This is not perfect as those new names could still conflict, but at least it has the advantage to keep the code readable and simple while reducing the likelihood of conflicts to be close to zero. Reported-by: Mathis Rudolf Author: Bharath Rupireddy Reviewed-by: Bernd Helmle, Thomas Munro, Michael Paquier Discussion: https://postgr.es/m/109c267a-10d2-3c53-b60e-720fcf44d9e8@credativ.de Backpatch-through: 9.6
* Standardize usages of appendStringInfo and appendPQExpBufferDavid Rowley2021-06-03
| | | | | | | | | | | | | | | | | | | Fix a few places that were using appendStringInfo() when they should have been using appendStringInfoString(). Also some cases of appendPQExpBuffer() that would have been better suited to use appendPQExpBufferChar(), and finally, some places that used appendPQExpBuffer() when appendPQExpBufferStr() would have suited better. There are no bugs are being fixed here. The aim is just to make the code use the most optimal function for the job. All the code being changed here is new to PG14. It makes sense to fix these before we branch for PG15. There are a few other places that we could fix, but those cases are older code so fixing those seems less worthwhile as it may cause unnecessary back-patching pain in the future. Author: Hou Zhijie Discussion: https://postgr.es/m/OS0PR01MB5716732158B1C4142C6FE375943D9@OS0PR01MB5716.jpnprd01.prod.outlook.com
* Re-allow custom GUC names that have more than two components.Tom Lane2021-06-02
| | | | | | | | | | Commit 3db826bd5 disallowed this case, but it turns out that some people are depending on it. Since the core grammar has allowed it since 3dc37cd8d, it seems like this code should fall in line. Per bug #17045 from Robert Sosinski. Discussion: https://postgr.es/m/17045-6a4a9f0d1513f72b@postgresql.org
* Revert most of 39b66a91bdTomas Vondra2021-06-03
| | | | | | | | | | Reverts most of commit 39b66a91bd, which was found to cause significant regression for REFRESH MATERIALIZED VIEW. This means only rows inserted by heap_multi_insert will benefit from the optimization, implemented in commit 7db0cd2145. Reported-by: Masahiko Sawada Discussion: https://postgr.es/m/CAD21AoA%3D%3Df2VSw3c-Cp_y%3DWLKHMKc1D6s7g3YWsCOvgaYPpJcg%40mail.gmail.com
* Fix planner's row-mark code for inheritance from a foreign table.Tom Lane2021-06-02
| | | | | | | | | | | | | | | | Commit 428b260f8 broke planning of cases where row marks are needed (SELECT FOR UPDATE, etc) and one of the query's tables is a foreign table that has regular table(s) as inheritance children. We got the reverse case right, but apparently were thinking that foreign tables couldn't be inheritance parents. Not so; so we need to be able to add a CTID junk column while adding a new child, not only a wholerow junk column. Back-patch to v12 where the faulty code came in. Amit Langote Discussion: https://postgr.es/m/CA+HiwqEmo3FV1LAQ4TVyS2h1WM=kMkZUmbNuZSCnfHvMcUcPeA@mail.gmail.com
* Reject SELECT ... GROUP BY GROUPING SETS (()) FOR UPDATE.Tom Lane2021-06-01
| | | | | | | | | | | | | | | | | This case should be disallowed, just as FOR UPDATE with a plain GROUP BY is disallowed; FOR UPDATE only makes sense when each row of the query result can be identified with a single table row. However, we missed teaching CheckSelectLocking() to check groupingSets as well as groupClause, so that it would allow degenerate grouping sets. That resulted in a bad plan and a null-pointer dereference in the executor. Looking around for other instances of the same bug, the only one I found was in examine_simple_variable(). That'd just lead to silly estimates, but it should be fixed too. Per private report from Yaoguang Chen. Back-patch to all supported branches.
* pgoutput: Fix memory leak due to RelationSyncEntry.map.Amit Kapila2021-06-01
| | | | | | | | | | | | | Release memory allocated when creating the tuple-conversion map and its component TupleDescs when its owning sync entry is invalidated. TupleDescs must also be freed when no map is deemed necessary, to begin with. Reported-by: Andres Freund Author: Amit Langote Reviewed-by: Takamichi Osumi, Amit Kapila Backpatch-through: 13, where it was introduced Discussion: https://postgr.es/m/MEYP282MB166933B1AB02B4FE56E82453B64D9@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM
* Fix RADIUS error reporting in hba file parsingPeter Eisentraut2021-05-31
| | | | | | | | | | | | The RADIUS-related checks in parse_hba_line() did not respect elevel and did not fill in *err_msg. Also, verify_option_list_length() pasted together error messages in an untranslatable way. To fix the latter, remove the function and do the error checking inline. It's a bit more verbose but only minimally longer, and it makes fixing the first two issues straightforward. Reviewed-by: Magnus Hagander <magnus@hagander.net> Discussion: https://www.postgresql.org/message-id/flat/8381e425-8c23-99b3-15ec-3115001db1b2%40enterprisedb.com
* Fix mis-planning of repeated application of a projection.Tom Lane2021-05-31
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | create_projection_plan contains a hidden assumption (here made explicit by an Assert) that a projection-capable Path will yield a projection-capable Plan. Unfortunately, that assumption is violated only a few lines away, by create_projection_plan itself. This means that two stacked ProjectionPaths can yield an outcome where we try to jam the upper path's tlist into a non-projection-capable child node, resulting in an invalid plan. There isn't any good reason to have stacked ProjectionPaths; indeed the whole concept is faulty, since the set of Vars/Aggs/etc needed by the upper one wouldn't necessarily be available in the output of the lower one, nor could the lower one create such values if they weren't available from its input. Hence, we can fix this by adjusting create_projection_path to strip any top-level ProjectionPath from the subpath it's given. (This amounts to saying "oh, we changed our minds about what we need to project here".) The test case added here only fails in v13 and HEAD; before that, we don't attempt to shove the Sort into the parallel part of the plan, for reasons that aren't entirely clear to me. However, all the directly-related code looks generally the same as far back as v11, where the hazard was introduced (by d7c19e62a). So I've got no faith that the same type of bug doesn't exist in v11 and v12, given the right test case. Hence, back-patch the code changes, but not the irrelevant test case, into those branches. Per report from Bas Poot. Discussion: https://postgr.es/m/534fca83789c4a378c7de379e9067d4f@politie.nl
* Improve some error wording with multirange type parsingMichael Paquier2021-05-31
| | | | | | | | | | | | Braces were referred in some error messages as only brackets (not curly brackets or curly braces), which can be confusing as other types of brackets could be used. While on it, add one test to check after the case of junk characters detected after a right brace. Author: Kyotaro Horiguchi Discussion: https://postgr.es/m/20210514.153153.1814935914483287479.horikyota.ntt@gmail.com
* Fix race condition when sharing tuple descriptors.Thomas Munro2021-05-29
| | | | | | | | | | | | Parallel query processes that called BlessTupleDesc() for identical tuple descriptors at the same moment could crash. There was code to handle that rare case, but it dereferenced a bogus DSA pointer. Repair. Back-patch to 11, where commit cc5f8136 added support for sharing tuple descriptors in parallel queries. Reported-by: Eric Thinnes <e.thinnes@gmx.de> Discussion: https://postgr.es/m/99aaa2eb-e194-bf07-c29a-1a76b4f2bcf9%40gmx.de
* Fix VACUUM VERBOSE's LP_DEAD item pages output.Peter Geoghegan2021-05-27
| | | | Oversight in commit 5100010e.
* Reduce the range of OIDs reserved for genbki.pl.Tom Lane2021-05-27
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit ab596105b increased FirstBootstrapObjectId from 12000 to 13000, but we've had some push-back about that. It's worrisome to reduce the daylight between there and FirstNormalObjectId, because the number of OIDs consumed during initdb for collation objects is hard to predict. We can improve the situation by abandoning the assumption that these OIDs must be globally unique. It should be sufficient for them to be unique per-catalog. (Any code that's unhappy about that is broken anyway, since no more than per-catalog uniqueness can be guaranteed once the OID counter wraps around.) With that change, the largest OID assigned during genbki.pl (starting from a base of 10000) is a bit under 11000. This allows reverting FirstBootstrapObjectId to 12000 with reasonable confidence that that will be sufficient for many years to come. We are not, at this time, abandoning the expectation that hand-assigned OIDs (below 10000) are globally unique. Someday that'll likely be necessary, but the need seems years away still. This is late for v14, but it seems worth doing it now so that downstream software doesn't have to deal with the consequences of a change in FirstBootstrapObjectId. In any case, we already bought into forcing an initdb for beta2, so another catversion bump won't hurt. Discussion: https://postgr.es/m/1665197.1622065382@sss.pgh.pa.us
* Rethink definition of pg_attribute.attcompression.Tom Lane2021-05-27
| | | | | | | | | | | | | | | | | | | | | | | Redefine '\0' (InvalidCompressionMethod) as meaning "if we need to compress, use the current setting of default_toast_compression". This allows '\0' to be a suitable default choice regardless of datatype, greatly simplifying code paths that initialize tupledescs and the like. It seems like a more user-friendly approach as well, because now the default compression choice doesn't migrate into table definitions, meaning that changing default_toast_compression is usually sufficient to flip an installation's behavior; one needn't tediously issue per-column ALTER SET COMPRESSION commands. Along the way, fix a few minor bugs and documentation issues with the per-column-compression feature. Adopt more robust APIs for SetIndexStorageProperties and GetAttributeCompression. Bump catversion because typical contents of attcompression will now be different. We could get away without doing that, but it seems better to ensure v14 installations all agree on this. (We already forced initdb for beta2, anyway.) Discussion: https://postgr.es/m/626613.1621787110@sss.pgh.pa.us
* Replace run-time error check with assertionPeter Eisentraut2021-05-27
| | | | | | | | | | The error message was checking that the structures returned from the parser matched expectations. That's something we usually use assertions for, not a full user-facing error message. So replace that with an assertion (hidden inside lfirst_node()). Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://www.postgresql.org/message-id/flat/452e9df8-ec89-e01b-b64a-8cc6ce830458%40enterprisedb.com
* doc: Fix description of some GUCs in docs and postgresql.conf.sampleMichael Paquier2021-05-27
| | | | | | | | | | | | | | | | | | | | | | The following parameters have been imprecise, or incorrect, about their description (PGC_POSTMASTER or PGC_SIGHUP): - autovacuum_work_mem (docs, as of 9.6~) - huge_page_size (docs, as of 14~) - max_logical_replication_workers (docs, as of 10~) - max_sync_workers_per_subscription (docs, as of 10~) - min_dynamic_shared_memory (docs, as of 14~) - recovery_init_sync_method (postgresql.conf.sample, as of 14~) - remove_temp_files_after_crash (docs, as of 14~) - restart_after_crash (docs, as of 9.6~) - ssl_min_protocol_version (docs, as of 12~) - ssl_max_protocol_version (docs, as of 12~) This commit adjusts the description of all these parameters to be more consistent with the practice used for the others. Revewed-by: Justin Pryzby Discussion: https://postgr.es/m/YK2ltuLpe+FbRXzA@paquier.xyz Backpatch-through: 9.6
* Fix assertion during streaming of multi-insert toast changes.Amit Kapila2021-05-27
| | | | | | | | | | | | | | | | | | | | While decoding the multi-insert WAL we can't clean the toast untill we get the last insert of that WAL record. Now if we stream the changes before we get the last change, the memory for toast chunks won't be released and we expect the txn to have streamed all changes after streaming. This restriction is mainly to ensure the correctness of streamed transactions and it doesn't seem worth uplifting such a restriction just to allow this case because anyway we will stream the transaction once such an insert is complete. Previously we were using two different flags (one for toast tuples and another for speculative inserts) to indicate partial changes. Now instead we replaced both of them with a single flag to indicate partial changes. Reported-by: Pavan Deolasee Author: Dilip Kumar Reviewed-by: Pavan Deolasee, Amit Kapila Discussion: https://postgr.es/m/CABOikdN-_858zojYN-2tNcHiVTw-nhxPwoQS4quExeweQfG1Ug@mail.gmail.com
* Fix typo in heapam.cMichael Paquier2021-05-26
| | | | | Author: Hou Zhijie Discussion: https://postgr.es/m/OS0PR01MB571612191738540B27A8DE5894249@OS0PR01MB5716.jpnprd01.prod.outlook.com
* Fix use of uninitialized variable in inline_function().Tom Lane2021-05-25
| | | | | | | | | | | | | | | | | | | Commit e717a9a18 introduced a code path that bypassed the call of get_expr_result_type, which is not good because we need its rettupdesc result to pass to check_sql_fn_retval. We'd failed to notice right away because the code path in which check_sql_fn_retval uses that argument is fairly hard to reach in this context. It's not impossible though, and in any case inline_function would have no business assuming that check_sql_fn_retval doesn't need that value. To fix, move get_expr_result_type out of the if-block, which in turn requires moving the construction of the dummy FuncExpr out of it. Per report from Ranier Vilela. (I'm bemused by the lack of any compiler complaints...) Discussion: https://postgr.es/m/CAEudQAqBqQpQ3HruWAGU_7WaMJ7tntpk0T8k_dVtNB46DqdBgw@mail.gmail.com
* postgresql.conf.sample: Make vertical spacing consistentPeter Eisentraut2021-05-25
|
* Fix memory leak when de-toasting compressed values in VACUUM FULL/CLUSTERMichael Paquier2021-05-25
| | | | | | | | | | | | | | | | | | | | | | VACUUM FULL and CLUSTER can be used to enforce the use of the existing compression method of a toastable column if a value currently stored is compressed with a method that does not match the column's defined method. The code in charge of decompressing and recompressing toast values at rewrite left around the detoasted values, causing an accumulation of memory allocated in TopTransactionContext. When processing large relations, this could cause the system to run out of memory. The detoasted values are not needed once their tuple is rewritten, and this commit ensures that the necessary cleanup happens. Issue introduced by bbe0a81d. The comments of the area are reordered a bit while on it. Reported-by: Andres Freund Analyzed-by: Andres Freund Author: Michael Paquier Reviewed-by: Dilip Kumar Discussion: https://postgr.es/m/20210521211929.pcehg6f23icwstdb@alap3.anarazel.de
* Improve docs and error messages for parallel vacuum.Amit Kapila2021-05-25
| | | | | | | | | | | | The error messages, docs, and one of the options were using 'parallel degree' to indicate parallelism used by vacuum command. We normally use 'parallel workers' at other places so change it for parallel vacuum accordingly. Author: Bharath Rupireddy Reviewed-by: Dilip Kumar, Amit Kapila Backpatch-through: 13 Discussion: https://postgr.es/m/CALj2ACWz=PYrrFXVsEKb9J1aiX4raA+UBe02hdRp_zqDkrWUiw@mail.gmail.com
* Disallow SSL renegotiationMichael Paquier2021-05-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | SSL renegotiation is already disabled as of 48d23c72, however this does not prevent the server to comply with a client willing to use renegotiation. In the last couple of years, renegotiation had its set of security issues and flaws (like the recent CVE-2021-3449), and it could be possible to crash the backend with a client attempting renegotiation. This commit takes one extra step by disabling renegotiation in the backend in the same way as SSL compression (f9264d15) or tickets (97d3a0b0). OpenSSL 1.1.0h has added an option named SSL_OP_NO_RENEGOTIATION able to achieve that. In older versions there is an option called SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS that was undocumented, and could be set within the SSL object created when the TLS connection opens, but I have decided not to use it, as it feels trickier to rely on, and it is not official. Note that this option is not usable in OpenSSL < 1.1.0h as the internal contents of the *SSL object are hidden to applications. SSL renegotiation concerns protocols up to TLSv1.2. Per original report from Robert Haas, with a patch based on a suggestion by Andres Freund. Author: Michael Paquier Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/YKZBXx7RhU74FlTE@paquier.xyz Backpatch-through: 9.6
* Fix setrefs.c code for Result Cache nodesDavid Rowley2021-05-25
| | | | | | | | | | | | | | Result Cache, added in 9eacee2e6 neglected to properly adjust the plan references in setrefs.c. This could lead to the following error during EXPLAIN: ERROR: cannot decompile join alias var in plan tree Fix that. Bug: 17030 Reported-by: Hans Buschmann Discussion: https://postgr.es/m/17030-5844aecae42fe223@postgresql.org
* Consider triggering VACUUM failsafe during scan.Peter Geoghegan2021-05-24
| | | | | | | | | | | | | | | | | | | | | The wraparound failsafe mechanism added by commit 1e55e7d1 handled the one-pass strategy case (i.e. the "table has no indexes" case) by adding a dedicated failsafe check. This made up for the fact that the usual one-pass checks inside lazy_vacuum_all_indexes() cannot ever be reached during a one-pass strategy VACUUM. This approach failed to account for two-pass VACUUMs that opt out of index vacuuming up-front. The INDEX_CLEANUP off case in the only case that works like that. Fix this by performing a failsafe check every 4GB during the first scan of the heap, regardless of the details of the VACUUM. This eliminates the special case, and will make the failsafe trigger more reliably. Author: Peter Geoghegan <pg@bowt.ie> Reported-By: Andres Freund <andres@anarazel.de> Reviewed-By: Masahiko Sawada <sawada.mshk@gmail.com> Discussion: https://postgr.es/m/20210424002921.pb3t7h6frupdqnkp@alap3.anarazel.de
* Add missing NULL check when building Result Cache pathsDavid Rowley2021-05-24
| | | | | | | | | | | | | | | Code added in 9e215378d to disable building of Result Cache paths when not all join conditions are part of the parameterization of a unique join failed to first check if the inner path's param_info was set before checking the param_info's ppi_clauses. Add a check for NULL values here and just bail on trying to build the path if param_info is NULL. lateral_vars are not considered when deciding if the join is unique, so we're not missing out on doing the optimization when there are lateral_vars and no param_info. Reported-by: Coverity, via Tom Lane Discussion: https://postgr.es/m/457998.1621779290@sss.pgh.pa.us
* Re-order pg_attribute columns to eliminate some padding space.Tom Lane2021-05-23
| | | | | | | | | | | | | | | | | | Now that attcompression is just a char, there's a lot of wasted padding space after it. Move it into the group of char-wide columns to save a net of 4 bytes per pg_attribute entry. While we're at it, swap the order of attstorage and attalign to make for a more logical grouping of these columns. Also re-order actions in related code to match the new field ordering. This patch also fixes one outright bug: equalTupleDescs() failed to compare attcompression. That could, for example, cause relcache reload to fail to adopt a new value following a change. Michael Paquier and Tom Lane, per a gripe from Andres Freund. Discussion: https://postgr.es/m/20210517204803.iyk5wwvwgtjcmc5w@alap3.anarazel.de
* Be more verbose when the postmaster unexpectedly quits.Tom Lane2021-05-23
| | | | | | | | | | | | | | | | | Emit a LOG message when the postmaster stops because of a failure in the startup process. There already is a similar message if we exit for that reason during PM_STARTUP phase, so it seems inconsistent that there was none if the startup process fails later on. Also emit a LOG message when the postmaster stops after a crash because restart_after_crash is disabled. This seems potentially helpful in case DBAs (or developers) forget that that's set. Also, it was the only remaining place where the postmaster would do an abnormal exit without any comment as to why. In passing, remove an unreachable call of ExitPostmaster(0). Discussion: https://postgr.es/m/194914.1621641288@sss.pgh.pa.us
* Fix access to no-longer-open relcache entry in logical-rep worker.Tom Lane2021-05-22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | If we redirected a replicated tuple operation into a partition child table, and then tried to fire AFTER triggers for that event, the relation cache entry for the child table was already closed. This has no visible ill effects as long as the entry is still there and still valid, but an unluckily-timed cache flush could result in a crash or other misbehavior. To fix, postpone the ExecCleanupTupleRouting call (which is what closes the child table) until after we've fired triggers. This requires a bit of refactoring so that the cleanup function can have access to the necessary state. In HEAD, I took the opportunity to simplify some of worker.c's function APIs based on use of the new ApplyExecutionData struct. However, it doesn't seem safe/practical to back-patch that aspect, at least not without a lot of analysis of possible interactions with a04daa97a. In passing, add an Assert to afterTriggerInvokeEvents to catch such cases. This seems worthwhile because we've grown a number of fairly unstructured ways of calling AfterTriggerEndQuery. Back-patch to v13, where worker.c grew the ability to deal with partitioned target tables. Discussion: https://postgr.es/m/3382681.1621381328@sss.pgh.pa.us
* Fix planner's use of Result Cache with unique joinsDavid Rowley2021-05-22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When the planner considered using a Result Cache node to cache results from the inner side of a Nested Loop Join, it failed to consider that the inner path's parameterization may not be the entire join condition. If the join was marked as inner_unique then we may accidentally put the cache in singlerow mode. This meant that entries would be marked as complete after caching the first row. That was wrong as if only part of the join condition was parameterized then the uniqueness of the unique join was not guaranteed at the Result Cache's level. The uniqueness is only guaranteed after Nested Loop applies the join filter. If subsequent rows were found, this would lead to: ERROR: cache entry already complete This could have been fixed by only putting the cache in singlerow mode if the entire join condition was parameterized. However, Nested Loop will only read its inner side so far as the first matching row when the join is unique, so that might mean we never get an opportunity to mark cache entries as complete. Since non-complete cache entries are useless for subsequent lookups, we just don't bother considering a Result Cache path in this case. In passing, remove the XXX comment that claimed the above ERROR might be better suited to be an Assert. After there being an actual case which triggered it, it seems better to keep it an ERROR. Reported-by: David Christensen Discussion: https://postgr.es/m/CAOxo6X+dy-V58iEPFgst8ahPKEU+38NZzUuc+a7wDBZd4TrHMQ@mail.gmail.com
* Disallow whole-row variables in GENERATED expressions.Tom Lane2021-05-21
| | | | | | | | | | | | | | | This was previously allowed, but I think that was just an oversight. It's a clear violation of the rule that a generated column cannot depend on itself or other generated columns. Moreover, because the code was relying on the assumption that no such cross-references exist, it was pretty easy to crash ALTER TABLE and perhaps other places. Even if you managed not to crash, you got quite unstable, implementation-dependent results. Per report from Vitaly Ustinov. Back-patch to v12 where GENERATED came in. Discussion: https://postgr.es/m/CAM_DEiWR2DPT6U4xb-Ehigozzd3n3G37ZB1+867zbsEVtYoJww@mail.gmail.com
* Fix usage of "tableoid" in GENERATED expressions.Tom Lane2021-05-21
| | | | | | | | | | | | | | | We consider this supported (though I've got my doubts that it's a good idea, because tableoid is not immutable). However, several code paths failed to fill the field in soon enough, causing such a GENERATED expression to see zero or the wrong value. This occurred when ALTER TABLE adds a new GENERATED column to a table with existing rows, and during regular INSERT or UPDATE on a foreign table with GENERATED columns. Noted during investigation of a report from Vitaly Ustinov. Back-patch to v12 where GENERATED came in. Discussion: https://postgr.es/m/CAM_DEiWR2DPT6U4xb-Ehigozzd3n3G37ZB1+867zbsEVtYoJww@mail.gmail.com
* Restore the portal-level snapshot after procedure COMMIT/ROLLBACK.Tom Lane2021-05-21
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | COMMIT/ROLLBACK necessarily destroys all snapshots within the session. The original implementation of intra-procedure transactions just cavalierly did that, ignoring the fact that this left us executing in a rather different environment than normal. In particular, it turns out that handling of toasted datums depends rather critically on there being an outer ActiveSnapshot: otherwise, when SPI or the core executor pop whatever snapshot they used and return, it's unsafe to dereference any toasted datums that may appear in the query result. It's possible to demonstrate "no known snapshots" and "missing chunk number N for toast value" errors as a result of this oversight. Historically this outer snapshot has been held by the Portal code, and that seems like a good plan to preserve. So add infrastructure to pquery.c to allow re-establishing the Portal-owned snapshot if it's not there anymore, and add enough bookkeeping support that we can tell whether it is or not. We can't, however, just re-establish the Portal snapshot as part of COMMIT/ROLLBACK. As in normal transaction start, acquiring the first snapshot should wait until after SET and LOCK commands. Hence, teach spi.c about doing this at the right time. (Note that this patch doesn't fix the problem for any PLs that try to run intra-procedure transactions without using SPI to execute SQL commands.) This makes SPI's no_snapshots parameter rather a misnomer, so in HEAD, rename that to allow_nonatomic. replication/logical/worker.c also needs some fixes, because it wasn't careful to hold a snapshot open around AFTER trigger execution. That code doesn't use a Portal, which I suspect someday we're gonna have to fix. But for now, just rearrange the order of operations. This includes back-patching the recent addition of finish_estate() to centralize the cleanup logic there. This also back-patches commit 2ecfeda3e into v13, to improve the test coverage for worker.c (it was that test that exposed that worker.c's snapshot management is wrong). Per bug #15990 from Andreas Wicht. Back-patch to v11 where intra-procedure COMMIT was added. Discussion: https://postgr.es/m/15990-eee2ac466b11293d@postgresql.org
* Fix deadlock for multiple replicating truncates of the same table.Amit Kapila2021-05-21
| | | | | | | | | | | | | | | | | | While applying the truncate change, the logical apply worker acquires RowExclusiveLock on the relation being truncated. This allowed truncate on the relation at a time by two apply workers which lead to a deadlock. The reason was that one of the workers after updating the pg_class tuple tries to acquire SHARE lock on the relation and started to wait for the second worker which has acquired RowExclusiveLock on the relation. And when the second worker tries to update the pg_class tuple, it starts to wait for the first worker which leads to a deadlock. Fix it by acquiring AccessExclusiveLock on the relation before applying the truncate change as we do for normal truncate operation. Author: Peter Smith, test case by Haiying Tang Reviewed-by: Dilip Kumar, Amit Kapila Backpatch-through: 11 Discussion: https://postgr.es/m/CAHut+PsNm43p0jM+idTvWwiGZPcP0hGrHMPK9TOAkc+a4UpUqw@mail.gmail.com
* Avoid detoasting failure after COMMIT inside a plpgsql FOR loop.Tom Lane2021-05-20
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | exec_for_query() normally tries to prefetch a few rows at a time from the query being iterated over, so as to reduce executor entry/exit overhead. Unfortunately this is unsafe if we have COMMIT or ROLLBACK within the loop, because there might be TOAST references in the data that we prefetched but haven't yet examined. Immediately after the COMMIT/ROLLBACK, we have no snapshots in the session, meaning that VACUUM is at liberty to remove recently-deleted TOAST rows. This was originally reported as a case triggering the "no known snapshots" error in init_toast_snapshot(), but even if you miss hitting that, you can get "missing toast chunk", as illustrated by the added isolation test case. To fix, just disable prefetching in non-atomic contexts. Maybe there will be performance complaints prompting us to work harder later, but it's not clear at the moment that this really costs much, and I doubt we'd want to back-patch any complicated fix. In passing, adjust that error message in init_toast_snapshot() to be a little clearer about the likely cause of the problem. Patch by me, based on earlier investigation by Konstantin Knizhnik. Per bug #15990 from Andreas Wicht. Back-patch to v11 where intra-procedure COMMIT was added. Discussion: https://postgr.es/m/15990-eee2ac466b11293d@postgresql.org
* Make standby promotion reset the recovery pause state to 'not paused'.Fujii Masao2021-05-19
| | | | | | | | | | | | | | If a promotion is triggered while recovery is paused, the paused state ends and promotion continues. But previously in that case pg_get_wal_replay_pause_state() returned 'paused' wrongly while a promotion was ongoing. This commit changes a standby promotion so that it marks the recovery pause state as 'not paused' when it's triggered, to fix the issue. Author: Fujii Masao Reviewed-by: Dilip Kumar, Kyotaro Horiguchi Discussion: https://postgr.es/m/f706876c-4894-0ba5-6f4d-79803eeea21b@oss.nttdata.com
* Fix issues in pg_stat_wal.Fujii Masao2021-05-19
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 1) Previously there were both pgstat_send_wal() and pgstat_report_wal() in order to send WAL activity to the stats collector. With the former being used by wal writer, the latter by most other processes. They were a bit redundant and so this commit merges them into pgstat_send_wal() to simplify the code. 2) Previously WAL global statistics counters were calculated and then compared with zero-filled buffer in order to determine whether any WAL activity has happened since the last submission. These calculation and comparison were not cheap. This was regularly exercised even in read-only workloads. This commit fixes the issue by making some WAL activity counters directly be checked to determine if there's WAL activity stats to send. 3) Previously pgstat_report_stat() did not check if there's WAL activity stats to send as part of the "Don't expend a clock check if nothing to do" check at the top. It's probably rare to have pending WAL stats without also passing one of the other conditions, but for safely this commit changes pgstat_report_stats() so that it checks also some WAL activity counters at the top. This commit also adds the comments about the design of WAL stats. Reported-by: Andres Freund Author: Masahiro Ikeda Reviewed-by: Kyotaro Horiguchi, Atsushi Torikoshi, Andres Freund, Fujii Masao Discussion: https://postgr.es/m/20210324232224.vrfiij2rxxwqqjjb@alap3.anarazel.de
* Fix typo and outdated information in README.barrierDavid Rowley2021-05-18
| | | | | | | | | README.barrier didn't seem to get the memo when atomics were added. Fix that. Author: Tatsuo Ishii, David Rowley Discussion: https://postgr.es/m/20210516.211133.2159010194908437625.t-ishii%40sraoss.co.jp Backpatch-through: 9.6, oldest supported release
* Translation updatesPeter Eisentraut2021-05-17
| | | | | Source-Git-URL: git://git.postgresql.org/git/pgtranslation/messages.git Source-Git-Hash: 9bbd9c3714d0c76daaa806588b1fbf744aa60496
* Unbreak EXEC_BACKEND buildAlvaro Herrera2021-05-15
| | | | Per buildfarm
* Allow compute_query_id to be set to 'auto' and make it defaultAlvaro Herrera2021-05-15
| | | | | | | | | | | | | | | | | Allowing only on/off meant that all either all existing configuration guides would become obsolete if we disabled it by default, or that we would have to accept a performance loss in the default config if we enabled it by default. By allowing 'auto' as a middle ground, the performance cost is only paid by those who enable pg_stat_statements and similar modules. I only edited the release notes to comment-out a paragraph that is now factually wrong; further edits are probably needed to describe the related change in more detail. Author: Julien Rouhaud <rjuju123@gmail.com> Reviewed-by: Justin Pryzby <pryzby@telsasoft.com> Discussion: https://postgr.es/m/20210513002623.eugftm4nk2lvvks3@nol
* Be more careful about barriers when releasing BackgroundWorkerSlots.Tom Lane2021-05-15
| | | | | | | | | | | | | | | | | | | | ForgetBackgroundWorker lacked any memory barrier at all, while BackgroundWorkerStateChange had one but unaccountably did additional manipulation of the slot after the barrier. AFAICS, the rule must be that the barrier is immediately before setting or clearing slot->in_use. It looks like back in 9.6 when ForgetBackgroundWorker was first written, there might have been some case for not needing a barrier there, but I'm not very convinced of that --- the fact that the load of bgw_notify_pid is in the caller doesn't seem to guarantee no memory ordering problem. So patch 9.6 too. It's likely that this doesn't fix any observable bug on Intel hardware, but machines with weaker memory ordering rules could have problems here. Discussion: https://postgr.es/m/4046084.1620244003@sss.pgh.pa.us
* Harden nbtree deduplication posting split code.Peter Geoghegan2021-05-14
| | | | | | | | | | | | | | | | | | Add a defensive "can't happen" error to code that handles nbtree posting list splits (promote an existing assertion). This avoids a segfault in the event of an insertion of a newitem that is somehow identical to an existing non-pivot tuple in the index. An nbtree index should never have two index tuples with identical TIDs. This scenario is not particular unlikely in the event of any kind of corruption that leaves the index in an inconsistent state relative to the heap relation that is indexed. There are two known reports of preventable hard crashes. Doing nothing seems unacceptable given the general expectation that nbtree will cope reasonably well with corrupt data. Discussion: https://postgr.es/m/CAH2-Wz=Jr_d-dOYEEmwz0-ifojVNWho01eAqewfQXgKfoe114w@mail.gmail.com Backpatch: 13-, where nbtree deduplication was introduced.
generated by cgit v1.2.3 (git 2.25.1) at 2025-08-03 22:42:46 +0000