aboutsummaryrefslogtreecommitdiff
path: root/contrib
Commit message (Collapse)AuthorAge
* In security-restricted operations, block enqueue of at-commit user code.Noah Misch2020-11-09
| | | | | | | | | | | | | | | | | | Specifically, this blocks DECLARE ... WITH HOLD and firing of deferred triggers within index expressions and materialized view queries. An attacker having permission to create non-temp objects in at least one schema could execute arbitrary SQL functions under the identity of the bootstrap superuser. One can work around the vulnerability by disabling autovacuum and not manually running ANALYZE, CLUSTER, REINDEX, CREATE INDEX, VACUUM FULL, or REFRESH MATERIALIZED VIEW. (Don't restore from pg_dump, since it runs some of those commands.) Plain VACUUM (without FULL) is safe, and all commands are fine when a trusted user owns the target object. Performance may degrade quickly under this workaround, however. Back-patch to 9.5 (all supported versions). Reviewed by Robert Haas. Reported by Etienne Stalmans. Security: CVE-2020-25695
* Fix potential memory leak in pgcryptoMichael Paquier2020-10-19
| | | | | | | | | | | | | | | | | | | | | When allocating a EVP context, it would have been possible to leak some memory allocated directly by OpenSSL, that PostgreSQL lost track of if the initialization of the context allocated failed. The cleanup can be done with EVP_MD_CTX_destroy(). Note that EVP APIs exist since OpenSSL 0.9.7 and we have in the tree equivalent implementations for older versions since ce9b75d (code removed with 9b7cd59a as of 10~). However, in 9.5 and 9.6, the existing code makes use of EVP_MD_CTX_destroy() and EVP_MD_CTX_create() without an equivalent implementation when building the tree with OpenSSL 0.9.6 or older, meaning that this code is in reality broken with such versions since it got introduced in e2838c5. As we have heard no complains about that, it does not seem worth bothering with in 9.5 and 9.6, so I have left that out for simplicity. Author: Michael Paquier Discussion: https://postgr.es/m/20201015072212.GC2305@paquier.xyz Backpatch-through: 9.5
* Add missing error check in pgcrypto/crypt-md5.c.Tom Lane2020-10-16
| | | | | | | | | | | | | | | In theory, the second px_find_digest call in px_crypt_md5 could fail even though the first one succeeded, since resource allocation is required. Don't skip testing for a failure. (If one did happen, the likely result would be a crash rather than clean recovery from an OOM failure.) The code's been like this all along, so back-patch to all supported branches. Daniel Gustafsson Discussion: https://postgr.es/m/AA8D6FE9-4AB2-41B4-98CB-AE64BA668C03@yesql.se
* Make contrib modules' installation scripts more secure.Tom Lane2020-08-10
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Hostile objects located within the installation-time search_path could capture references in an extension's installation or upgrade script. If the extension is being installed with superuser privileges, this opens the door to privilege escalation. While such hazards have existed all along, their urgency increases with the v13 "trusted extensions" feature, because that lets a non-superuser control the installation path for a superuser-privileged script. Therefore, make a number of changes to make such situations more secure: * Tweak the construction of the installation-time search_path to ensure that references to objects in pg_catalog can't be subverted; and explicitly add pg_temp to the end of the path to prevent attacks using temporary objects. * Disable check_function_bodies within installation/upgrade scripts, so that any security gaps in SQL-language or PL-language function bodies cannot create a risk of unwanted installation-time code execution. * Adjust lookup of type input/receive functions and join estimator functions to complain if there are multiple candidate functions. This prevents capture of references to functions whose signature is not the first one checked; and it's arguably more user-friendly anyway. * Modify various contrib upgrade scripts to ensure that catalog modification queries are executed with secure search paths. (These are in-place modifications with no extension version changes, since it is the update process itself that is at issue, not the end result.) Extensions that depend on other extensions cannot be made fully secure by these methods alone; therefore, revert the "trusted" marking that commit eb67623c9 applied to earthdistance and hstore_plperl, pending some better solution to that set of issues. Also add documentation around these issues, to help extension authors write secure installation scripts. Patch by me, following an observation by Andres Freund; thanks to Noah Misch for review. Security: CVE-2020-14350
* Fix corner case with 16kB-long decompression in pgcrypto, take 2Michael Paquier2020-07-27
| | | | | | | | | | | | | | | | | | | | | | | A compressed stream may end with an empty packet. In this case decompression finishes before reading the empty packet and the remaining stream packet causes a failure in reading the following data. This commit makes sure to consume such extra data, avoiding a failure when decompression the data. This corner case was reproducible easily with a data length of 16kB, and existed since e94dd6a. A cheap regression test is added to cover this case based on a random, incompressible string. The first attempt of this patch has allowed to find an older failure within the compression logic of pgcrypto, fixed by b9b6105. This involved SLES 15 with z390 where a custom flavor of libz gets used. Bonus thanks to Mark Wong for providing access to the specific environment. Reported-by: Frank Gagnepain Author: Kyotaro Horiguchi, Michael Paquier Reviewed-by: Tom Lane Discussion: https://postgr.es/m/16476-692ef7b84e5fb893@postgresql.org Backpatch-through: 9.5
* Fix ancient violation of zlib's API spec.Tom Lane2020-07-23
| | | | | | | | | | | | | | | | | | | | | | | | | | contrib/pgcrypto mishandled the case where deflate() does not consume all of the offered input on the first try. It reset the next_in pointer to the start of the input instead of leaving it alone, causing the wrong data to be fed to the next deflate() call. This has been broken since pgcrypto was committed. The reason for the lack of complaints seems to be that it's fairly hard to get stock zlib to not consume all the input, so long as the output buffer is big enough (which it normally would be in pgcrypto's usage; AFAICT the input is always going to be packetized into packets no larger than ZIP_OUT_BUF). However, IBM's zlibNX implementation for AIX evidently will do it in some cases. I did not add a test case for this, because I couldn't find one that would fail with stock zlib. When we put back the test case for bug #16476, that will cover the zlibNX situation well enough. While here, write deflate()'s second argument as Z_NO_FLUSH per its API spec, instead of hard-wiring the value zero. Per buildfarm results and subsequent investigation. Discussion: https://postgr.es/m/16476-692ef7b84e5fb893@postgresql.org
* Revert "Fix corner case with PGP decompression in pgcrypto"Michael Paquier2020-07-23
| | | | | | | | | | | | | | This reverts commit 9e10898, after finding out that buildfarm members running SLES 15 on z390 complain on the compression and decompression logic of the new test: pipistrelles, barbthroat and steamerduck. Those hosts are visibly using hardware-specific changes to improve zlib performance, requiring more investigation. Thanks to Tom Lane for the discussion. Discussion: https://postgr.es/m/20200722093749.GA2564@paquier.xyz Backpatch-through: 9.5
* Fix corner case with PGP decompression in pgcryptoMichael Paquier2020-07-22
| | | | | | | | | | | | | | | | | A compressed stream may end with an empty packet, and PGP decompression finished before reading this empty packet in the remaining stream. This caused a failure in pgcrypto, handling this case as corrupted data. This commit makes sure to consume such extra data, avoiding a failure when decompression the entire stream. This corner case was reproducible with a data length of 16kB, and existed since its introduction in e94dd6a. A cheap regression test is added to cover this case. Thanks to Jeff Janes for the extra investigation. Reported-by: Frank Gagnepain Author: Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/16476-692ef7b84e5fb893@postgresql.org Backpatch-through: 9.5
* Initialize dblink remoteConn struct in all casesJoe Conway2020-05-28
| | | | | | | | | | | Two of the members of rconn were left uninitialized. When dblink_open() is called without an outer transaction it handles the initialization for us, but with an outer transaction it does not. Arrange for initialization in all cases. Backpatch to all supported versions. Reported-by: Alexander Lakhin Discussion: https://www.postgresql.org/message-id/flat/9bd0744f-5f04-c778-c5b3-809efe9c30c7%40joeconway.com#c545909a41664991aca60c4d70a10ce7
* Get rid of trailing semicolons in C macro definitions.Tom Lane2020-05-01
| | | | | | | | | | | | | | | | | | | | | | | Writing a trailing semicolon in a macro is almost never the right thing, because you almost always want to write a semicolon after each macro call instead. (Even if there was some reason to prefer not to, pgindent would probably make a hash of code formatted that way; so within PG the rule should basically be "don't do it".) Thus, if we have a semi inside the macro, the compiler sees "something;;". Much of the time the extra empty statement is harmless, but it could lead to mysterious syntax errors at call sites. In perhaps an overabundance of neatnik-ism, let's run around and get rid of the excess semicolons whereever possible. The only thing worse than a mysterious syntax error is a mysterious syntax error that only happens in the back branches; therefore, backpatch these changes where relevant, which is most of them because most of these mistakes are old. (The lack of reported problems shows that this is largely a hypothetical issue, but still, it could bite us in some future patch.) John Naylor and Tom Lane Discussion: https://postgr.es/m/CACPNZCs0qWTqJ2QUSGJ07B7uvAvzMb-KbG2q+oo+J3tsWN5cqw@mail.gmail.com
* Fix cache reference leak in contrib/sepgsql.Tom Lane2020-04-16
| | | | | | | | | | | | | | fixup_whole_row_references() did the wrong thing with a dropped column, resulting in a commit-time warning about a cache reference leak. I (tgl) added a test case exercising this, but back-patched the test only as far as v10; the patch didn't apply cleanly to 9.6 and it didn't seem worth the trouble to adapt it. The bug is pretty old though, so apply the code change all the way back. Michael Luo, with cosmetic improvements by me Discussion: https://postgr.es/m/BYAPR08MB5606D1453D7F50E2AF4D2FD29AD80@BYAPR08MB5606.namprd08.prod.outlook.com
* Fix bogus CALLED_AS_TRIGGER() defenses.Tom Lane2020-04-03
| | | | | | | | | | | | | | | | | | | | | contrib/lo's lo_manage() thought it could use trigdata->tg_trigger->tgname in its error message about not being called as a trigger. That naturally led to a core dump. unique_key_recheck() figured it could Assert that fcinfo->context is a TriggerData node in advance of having checked that it's being called as a trigger. That's harmless in production builds, and perhaps not that easy to reach in any case, but it's logically wrong. The first of these per bug #16340 from William Crowell; the second from manual inspection of other CALLED_AS_TRIGGER call sites. Back-patch the lo.c change to all supported branches, the other to v10 where the thinko crept in. Discussion: https://postgr.es/m/16340-591c7449dc7c8c47@postgresql.org
* Back-patch addition of stack overflow and interrupt checks for lquery.Tom Lane2020-03-31
| | | | | | | | | | | | | Experimentation shows that it's not hard at all to drive the old implementation of "ltree ~ lquery" match to stack overflow, so throw in a check_stack_depth() call, as I just did in HEAD. I wasn't able to make it take a long time, because all the pathological cases I tried hit stack overflow first; but I bet there are some others that do take a long time, so add CHECK_FOR_INTERRUPTS() too. Discussion: https://postgr.es/m/CAP_rww=waX2Oo6q+MbMSiZ9ktdj6eaJj0cQzNu=Ry2cCDij5fw@mail.gmail.com
* Protect against overflow of ltree.numlevel and lquery.numlevel.Tom Lane2020-03-28
| | | | | | | | | | | | | | | | | | | | | | These uint16 fields could be overflowed by excessively long input, producing strange results. Complain for invalid input. Likewise check for out-of-range values of the repeat counts in lquery. (We don't try too hard on that one, notably not bothering to detect if atoi's result has overflowed.) Also detect length overflow in ltree_concat. In passing, be more consistent about whether "syntax error" messages include the type name. Also, clarify the documentation about what the size limit is. This has been broken for a long time, so back-patch to all supported branches. Nikita Glukhov, reviewed by Benjie Gillam and Tomas Vondra Discussion: https://postgr.es/m/CAP_rww=waX2Oo6q+MbMSiZ9ktdj6eaJj0cQzNu=Ry2cCDij5fw@mail.gmail.com
* Add missing errcode() in a few ereport calls.Amit Kapila2020-03-18
| | | | | | | | | | This will allow to specifying SQLSTATE error code for the errors in the missing places. Reported-by: Sawada Masahiko Author: Sawada Masahiko Backpatch-through: 9.5 Discussion: https://postgr.es/m/CA+fd4k6N8EjNvZpM8nme+y+05mz-SM8Z_BgkixzkA34R+ej0Kw@mail.gmail.com
* Avoid holding a directory FD open across assorted SRF calls.Tom Lane2020-03-16
| | | | | | | | | | | | | | | | | This extends the fixes made in commit 085b6b667 to other SRFs with the same bug, namely pg_logdir_ls(), pgrowlocks(), pg_timezone_names(), pg_ls_dir(), and pg_tablespace_databases(). Also adjust various comments and documentation to warn against expecting to clean up resources during a ValuePerCall SRF's final call. Back-patch to all supported branches, since these functions were all born broken. Justin Pryzby, with cosmetic tweaks by me Discussion: https://postgr.es/m/20200308173103.GC1357@telsasoft.com
* Stop demanding that top xact must be seen before subxact in decoding.Amit Kapila2020-02-19
| | | | | | | | | | | | | | | | | | | | | | | | | | | Manifested as ERROR: subtransaction logged without previous top-level txn record this check forbids legit behaviours like - First xl_xact_assignment record is beyond reading, i.e. earlier restart_lsn. - After restart_lsn there is some change of a subxact. - After that, there is second xl_xact_assignment (for another subxact) revealing the relationship between top and first subxact. Such a transaction won't be streamed anyway because we hadn't seen it in full. Saying for sure whether xact of some record encountered after the snapshot was deserialized can be streamed or not requires to know whether it wrote something before deserialization point --if yes, it hasn't been seen in full and can't be decoded. Snapshot doesn't have such info, so there is no easy way to relax the check. Reported-by: Hsu, John Diagnosed-by: Arseny Sher Author: Arseny Sher, Amit Kapila Reviewed-by: Amit Kapila, Dilip Kumar Backpatch-through: 9.5 Discussion: https://postgr.es/m/AB5978B2-1772-4FEE-A245-74C91704ECB0@amazon.com
* Disallow null category in crosstab_hashJoe Conway2019-12-23
| | | | | | | | | | | | | | | | | | | While building a hash map of categories in load_categories_hash, resulting category names have not thus far been checked to ensure they are not null. Prior to pg12 null category names worked to the extent that they did not crash on some platforms. This is because those system libraries have an snprintf which can deal with being passed a null pointer argument for a string. But even in those cases null categories did nothing useful. And on some platforms it crashed. As of pg12, our own version of snprintf gets called, and it does not deal with null pointer arguments at all, and crashes consistently. Fix that by disallowing null categories. They never worked usefully, and no one has ever asked for them to work previously. Back-patch to all supported branches. Reported-By: Ireneusz Pluta Discussion: https://postgr.es/m/16176-7489719b05e4303c@postgresql.org
* libpq should expose GSS-related parameters even when not implemented.Tom Lane2019-12-20
| | | | | | | | | | | | | | | | | | | | | | We realized years ago that it's better for libpq to accept all connection parameters syntactically, even if some are ignored or restricted due to lack of the feature in a particular build. However, that lesson from the SSL support was for some reason never applied to the GSSAPI support. This is causing various buildfarm members to have problems with a test case added by commit 6136e94dc, and it's just a bad idea from a user-experience standpoint anyway, so fix it. While at it, fix some places where parameter-related infrastructure was added with the aid of a dartboard, or perhaps with the aid of the anti-pattern "add new stuff at the end". It should be safe to rearrange the contents of struct pg_conn even in released branches, since that's private to libpq (and we'd have to move some fields in some builds to fix this, anyway). Back-patch to all supported branches. Discussion: https://postgr.es/m/11297.1576868677@sss.pgh.pa.us
* Ensure maxlen is at leat 1 in dict_intTomas Vondra2019-12-03
| | | | | | | | | | | | | | | | | | The dict_int text search dictionary template accepts maxlen parameter, which is then used to cap the length of input strings. The value was not properly checked, and the code simply does txt[d->maxlen] = '\0'; to insert a terminator, leading to segfaults with negative values. This commit simply rejects values less than 1. The issue was there since dct_int was introduced in 9.3, so backpatch all the way back to 9.4 which is the oldest supported version. Reported-by: cili Discussion: https://postgr.es/m/16144-a36a5bef7657047d@postgresql.org Backpatch-through: 9.4
* postgres_fdw: Fix error message for PREPARE TRANSACTION.Etsuro Fujita2019-11-08
| | | | | | | | | | | | | | | | | | | | Currently, postgres_fdw does not support preparing a remote transaction for two-phase commit even in the case where the remote transaction is read-only, but the old error message appeared to imply that that was not supported only if the remote transaction modified remote tables. Change the message so as to include the case where the remote transaction is read-only. Also fix a comment above the message. Also add a note about the lack of supporting PREPARE TRANSACTION to the postgres_fdw documentation. Reported-by: Gilles Darold Author: Gilles Darold and Etsuro Fujita Reviewed-by: Michael Paquier and Kyotaro Horiguchi Backpatch-through: 9.4 Discussion: https://postgr.es/m/08600ed3-3084-be70-65ba-279ab19618a5%40darold.net
* Fix intarray's GiST opclasses to not fail for empty arrays with <@.Tom Lane2019-08-06
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | contrib/intarray considers "arraycol <@ constant-array" to be indexable, but its GiST opclass code fails to reliably find index entries for empty array values (which of course should trivially match such queries). This is because the test condition to see whether we should descend through a non-leaf node is wrong. Unfortunately, empty array entries could be anywhere in the index, as these index opclasses are currently designed. So there's no way to fix this except by lobotomizing <@ indexscans to scan the whole index ... which is what this patch does. That's pretty unfortunate: the performance is now actually worse than a seqscan, in most cases. We'd be better off to remove <@ from the GiST opclasses entirely, and perhaps a future non-back-patchable patch will do so. In the meantime, applications whose performance is adversely impacted have a couple of options. They could switch to a GIN index, which doesn't have this bug, or they could replace "arraycol <@ constant-array" with "arraycol <@ constant-array AND arraycol && constant-array". That will provide about the same performance as before, and it will find all non-empty subsets of the given constant-array, which is all that could reliably be expected of the query before. While at it, add some more regression test cases to improve code coverage of contrib/intarray. In passing, adjust resize_intArrayType so that when it's returning an empty array, it uses construct_empty_array for that rather than cowboy hacking on the input array. While the hack produces an array that looks valid for most purposes, it isn't bitwise equal to empty arrays produced by other code paths, which could have subtle odd effects. I don't think this code path is performance-critical enough to justify such shortcuts. (Back-patch this part only as far as v11; before commit 01783ac36 we were not careful about this in other intarray code paths either.) Back-patch the <@ fixes to all supported versions, since this was broken from day one. Patch by me; thanks to Alexander Korotkov for review. Discussion: https://postgr.es/m/458.1565114141@sss.pgh.pa.us
* Fix handling of previous password hooks in passwordcheckMichael Paquier2019-08-01
| | | | | | | | | | | | | When piling up loading of modules using check_password_hook_type, loading passwordcheck would remove any trace of a previously-loaded hook. Unloading the module would also cause previous hooks to be entirely gone. Reported-by: Rafael Castro Author: Michael Paquier Reviewed-by: Daniel Gustafsson Discussion: https://postgr.es/m/15932-78f48f9ef166778c@postgresql.org Backpatch-through: 9.4
* Fix contrib/sepgsql test policy to work with latest SELinux releases.Tom Lane2019-07-25
| | | | | | | | | | | | | | As of Fedora 30, it seems that the system-provided macros for setting up user privileges in SELinux policies don't grant the ability to read /etc/passwd, as they formerly did. This restriction breaks psql (which tries to use getpwuid() to obtain the user name it's running under) and thereby the contrib/sepgsql regression test. Add explicit specifications that we need the right to read /etc/passwd. Mike Palmiotto, per a report from me. Back-patch to all supported branches. Discussion: https://postgr.es/m/23856.1563381159@sss.pgh.pa.us
* postgres_fdw: Account for triggers in non-direct remote UPDATE planning.Etsuro Fujita2019-06-13
| | | | | | | | | | | | | | | | | | Previously, in postgresPlanForeignModify, we planned an UPDATE operation on a foreign table so that we transmit only columns that were explicitly targets of the UPDATE, so as to avoid unnecessary data transmission, but if there were BEFORE ROW UPDATE triggers on the foreign table, those triggers might change values for non-target columns, in which case we would miss sending changed values for those columns. Prevent optimizing away transmitting all columns if there are BEFORE ROW UPDATE triggers on the foreign table. This is an oversight in commit 7cbe57c34 which added triggers on foreign tables, so apply the patch all the way back to 9.4 where that came in. Author: Shohei Mochizuki Reviewed-by: Amit Langote Discussion: https://postgr.es/m/201905270152.x4R1q3qi014550@toshiba.co.jp
* Fix copy-pasto in freeing memory on error in vacuumlo.Heikki Linnakangas2019-06-07
| | | | | | | | | | | | It's harmless to call PQfreemem() with a NULL argument, so the only consequence was that if allocating 'schema' failed, but allocating 'table' or 'field' succeeded, we would leak a bit of memory. That's highly unlikely to happen, so this is just academical, but let's get it right. Per bug #15838 from Timur Birsh. Backpatch back to 9.5, where the PQfreemem() calls were introduced. Discussion: https://www.postgresql.org/message-id/15838-3221652c72c5e69d@postgresql.org
* Fix potential assertion failure when reindexing a pg_class index.Andres Freund2019-04-29
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When reindexing individual indexes on pg_class it was possible to either trigger an assertion failure: TRAP: FailedAssertion("!(!ReindexIsProcessingIndex(((index)->rd_id))) That's because reindex_index() called SetReindexProcessing() - which enables an asserts ensuring no index insertions happen into the index - before calling RelationSetNewRelfilenode(). That not correct for indexes on pg_class, because RelationSetNewRelfilenode() updates the relevant pg_class row, which needs to update the indexes. The are two reasons this wasn't noticed earlier. Firstly the bug doesn't trigger when reindexing all of pg_class, as reindex_relation has code "hiding" all yet-to-be-reindexed indexes. Secondly, the bug only triggers when the the update to pg_class doesn't turn out to be a HOT update - otherwise there's no index insertion to trigger the bug. Most of the time there's enough space, making this bug hard to trigger. To fix, move RelationSetNewRelfilenode() to before the SetReindexProcessing() (and, together with some other code, to outside of the PG_TRY()). To make sure the error checking intended by SetReindexProcessing() is more robust, modify CatalogIndexInsert() to check ReindexIsProcessingIndex() even when the update is a HOT update. Also add a few regression tests for REINDEXing of system catalogs. The last two improvements would have prevented some of the issues fixed in 5c1560606dc4c from being introduced in the first place. Reported-By: Michael Paquier Diagnosed-By: Tom Lane and Andres Freund Author: Andres Freund Reviewed-By: Tom Lane Discussion: https://postgr.es/m/20190418011430.GA19133@paquier.xyz Backpatch: 9.4-, the bug is present in all branches
* Avoid double-free in vacuumlo error path.Tom Lane2019-03-24
| | | | | | | | | | | The code would do "PQclear(res)" twice if lo_unlink failed, evidently due to careless thinking about how far out a "break" would break. Remove the extra PQclear and adjust the loop logic so that we'll fall out of both levels of loop after an error, as was clearly the intent. Spotted by Coverity. I have no idea why it took this long to notice, since the bug has been there since commit 67ccbb080. Accordingly, back-patch to all supported branches.
* Fix volatile vs. pointer confusionPeter Eisentraut2019-03-15
| | | | | | | | | | | | | | Variables used after a longjmp() need to be declared volatile. In case of a pointer, it's the pointer itself that needs to be declared volatile, not the pointed-to value. So we need PyObject *volatile items; instead of volatile PyObject *items; /* wrong */ Discussion: https://www.postgresql.org/message-id/flat/f747368d-9e1a-c46a-ac76-3c27da32e8e4%402ndquadrant.com
* Relax overly strict assertionAlvaro Herrera2019-02-12
| | | | | | | | | | | | | | | Ever since its birth, ReorderBufferBuildTupleCidHash() has contained an assertion that a catalog tuple cannot change Cmax after acquiring one. But that's wrong: if a subtransaction executes DDL that affects that catalog tuple, and later aborts and another DDL affects the same tuple, it will change Cmax. Relax the assertion to merely verify that the Cmax remains valid and monotonically increasing, instead. Add a test that tickles the relevant code. Diagnosed by, and initial patch submitted by: Arseny Sher Co-authored-by: Arseny Sher Discussion: https://postgr.es/m/874l9p8hyw.fsf@ars-thinkpad
* Ensure that foreign scans with lateral refs are planned correctly.Tom Lane2019-02-07
| | | | | | | | | | | | | | | | | | | | | | | | | As reported in bug #15613 from Srinivasan S A, file_fdw and postgres_fdw neglected to mark plain baserel foreign paths as parameterized when the relation has lateral_relids. Other FDWs have surely copied this mistake, so rather than just patching those two modules, install a band-aid fix in create_foreignscan_path to rectify the mistake centrally. Although the band-aid is enough to fix the visible symptom, correct the calls in file_fdw and postgres_fdw anyway, so that they are valid examples for external FDWs. Also, since the band-aid isn't enough to make this work for parameterized foreign joins, throw an elog(ERROR) if such a case is passed to create_foreignscan_path. This shouldn't pose much of a problem for existing external FDWs, since it's likely they aren't trying to make such paths anyway (though some of them may need a defense against joins with lateral_relids, similar to the one this patch installs into postgres_fdw). Add some assertions in relnode.c to catch future occurrences of the same error --- in particular, as backstop against core-code mistakes like the one fixed by commit bdd9a99aa. Discussion: https://postgr.es/m/15613-092be1be9576c728@postgresql.org
* Fix misc typos in comments.Heikki Linnakangas2019-01-23
| | | | | | Spotted mostly by Fabien Coelho. Discussion: https://www.postgresql.org/message-id/alpine.DEB.2.21.1901230947050.16643@lancre
* Fix hstore hash function for empty hstores upgraded from 8.4.Andrew Gierth2018-11-24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Hstore data generated on pg 8.4 and pg_upgraded to current versions remains in its original on-disk format unless modified. The same goes for values generated by the addon hstore-new module on pre-9.0 versions. (The hstoreUpgrade function converts old values on the fly when read in, but the on-disk value is not modified by this.) Since old-format empty hstores (and hstore-new hstores) have representations compatible with the new format, hstoreUpgrade thought it could get away without modifying such values; but this breaks hstore_hash (and the new hstore_hash_extended) which assumes bit-perfect matching between semantically identical hstore values. Only one bit actually differs (the "new version" flag in the count field) but that of course is enough to break the hash. Fix by making hstoreUpgrade unconditionally convert all old values to new format. Backpatch all the way, even though this changes a hash value in some cases, because in those cases the hash value is already failing - for example, a hash join between old- and new-format empty hstores will be failing to match, or a hash index on an hstore column containing an old-format empty value will be failing to find the value since it will be searching for a hash derived from a new-format datum. (There are no known field reports of this happening, probably because hashing of hstores has only been useful in limited circumstances and there probably isn't much upgraded data being used this way.) Per concerns arising from discussion of commit eb6f29141be. Original bug is my fault. Discussion: https://postgr.es/m/60b1fd3b-7332-40f0-7e7f-f2f04f777747%402ndquadrant.com
* Avoid crashes in contrib/intarray gist__int_ops (bug #15518)Andrew Gierth2018-11-24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 1. Integer overflow in internal_size could result in memory corruption in decompression since a zero-length array would be allocated and then written to. This leads to crashes or corruption when traversing an index which has been populated with sufficiently sparse values. Fix by using int64 for computations and checking for overflow. 2. Integer overflow in g_int_compress could cause pessimal merge choices, resulting in unnecessarily large ranges (which would in turn trigger issue 1 above). Fix by using int64 again. 3. Even without overflow, array sizes could become large enough to cause unexplained memory allocation errors. Fix by capping the sizes to a safe limit and report actual errors pointing at gist__intbig_ops as needed. 4. Large inputs to the compression function always consist of large runs of consecutive integers, and the compression loop was processing these one at a time in an O(N^2) manner with a lot of overhead. The expected runtime of this function could easily exceed 6 months for a single call as a result. Fix by performing a linear-time first pass, which reduces the worst case to something on the order of seconds. Backpatch all the way, since this has been wrong forever. Per bug #15518 from report from irc user "dymk", analysis and patch by me. Discussion: https://postgr.es/m/15518-799e426c3b4f8358@postgresql.org
* Still further rethinking of build changes for macOS Mojave.Tom Lane2018-10-18
| | | | | | | | | | | | | | | | | | | | | | | To avoid the sorts of problems complained of by Jakob Egger, it'd be best if configure didn't emit any references to the sysroot path at all. In the case of PL/Tcl, we can do that just by keeping our hands off the TCL_INCLUDE_SPEC string altogether. In the case of PL/Perl, we need to substitute -iwithsysroot for -I in the compile commands, which is easily handled if we change to using a configure output variable that includes the switch not only the directory name. Since PL/Tcl and PL/Python already do it like that, this seems like good consistency cleanup anyway. Hence, this replaces the advice given to Perl-related extensions in commit 5e2217131; instead of writing "-I$(perl_archlibexp)/CORE", they should just write "$(perl_includespec)". (The old way continues to work, but not on recent macOS.) It's still the case that configure needs to be aware of the sysroot path internally, but that's cleaner than what we had before. As before, back-patch to all supported versions. Discussion: https://postgr.es/m/20840.1537850987@sss.pgh.pa.us
* Improve stability of recently-added regression test case.Tom Lane2018-10-16
| | | | | | | | | | | | | Commit b5febc1d1 added a contrib/btree_gist test case that has been observed to fail in the buildfarm as a result of background auto-analyze updating stats and changing the selected plan. Forestall that by forcibly analyzing in foreground, instead. The new plan choice is just as good for our purposes, since we really only care that an index-only plan does not get selected. Back-patch to 9.5, like the previous patch. Discussion: https://postgr.es/m/14643.1539629304@sss.pgh.pa.us
* Fix logical decoding error when system table w/ toast is repeatedly rewritten.Andres Freund2018-10-10
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Repeatedly rewriting a mapped catalog table with VACUUM FULL or CLUSTER could cause logical decoding to fail with: ERROR, "could not map filenode \"%s\" to relation OID" To trigger the problem the rewritten catalog had to have live tuples with toasted columns. The problem was triggered as during catalog table rewrites the heap_insert() check that prevents logical decoding information to be emitted for system catalogs, failed to treat the new heap's toast table as a system catalog (because the new heap is not recognized as a catalog table via RelationIsLogicallyLogged()). The relmapper, in contrast to the normal catalog contents, does not contain historical information. After a single rewrite of a mapped table the new relation is known to the relmapper, but if the table is rewritten twice before logical decoding occurs, the relfilenode cannot be mapped to a relation anymore. Which then leads us to error out. This only happens for toast tables, because the main table contents aren't re-inserted with heap_insert(). The fix is simple, add a new heap_insert() flag that prevents logical decoding information from being emitted, and accept during decoding that there might not be tuple data for toast tables. Unfortunately that does not fix pre-existing logical decoding errors. Doing so would require not throwing an error when a filenode cannot be mapped to a relation during decoding, and that seems too likely to hide bugs. If it's crucial to fix decoding for an existing slot, temporarily changing the ERROR in ReorderBufferCommit() to a WARNING appears to be the best fix. Author: Andres Freund Discussion: https://postgr.es/m/20180914021046.oi7dm4ra3ot2g2kt@alap3.anarazel.de Backpatch: 9.4-, where logical decoding was introduced
* Allow btree comparison functions to return INT_MIN.Tom Lane2018-10-05
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Historically we forbade datatype-specific comparison functions from returning INT_MIN, so that it would be safe to invert the sort order just by negating the comparison result. However, this was never really safe for comparison functions that directly return the result of memcmp(), strcmp(), etc, as POSIX doesn't place any such restriction on those library functions. Buildfarm results show that at least on recent Linux on s390x, memcmp() actually does return INT_MIN sometimes, causing sort failures. The agreed-on answer is to remove this restriction and fix relevant call sites to not make such an assumption; code such as "res = -res" should be replaced by "INVERT_COMPARE_RESULT(res)". The same is needed in a few places that just directly negated the result of memcmp or strcmp. To help find places having this problem, I've also added a compile option to nbtcompare.c that causes some of the commonly used comparators to return INT_MIN/INT_MAX instead of their usual -1/+1. It'd likely be a good idea to have at least one buildfarm member running with "-DSTRESS_SORT_INT_MIN". That's far from a complete test of course, but it should help to prevent fresh introductions of such bugs. This is a longstanding portability hazard, so back-patch to all supported branches. Discussion: https://postgr.es/m/20180928185215.ffoq2xrq5d3pafna@alap3.anarazel.de
* Make some fixes to allow building Postgres on macOS 10.14 ("Mojave").Tom Lane2018-09-25
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Apple's latest rearrangements of the system-supplied headers have broken building of PL/Perl and PL/Tcl. The only practical way to fix PL/Tcl is to start using the "-isysroot" compiler flag to point to SDK-supplied headers, as Apple expects. We must also start distinguishing where to find Perl's headers from where to find its shared library; but that seems like good cleanup anyway. Extensions that formerly did something like -I$(perl_archlibexp)/CORE should now do -I$(perl_includedir)/CORE instead. perl_archlibexp is still the place to look for libperl.so, though. If for some reason you don't like the default -isysroot setting, you can override that by setting PG_SYSROOT in configure's arguments. I don't currently think people would need to do so, unless maybe for cross-version build purposes. In addition, teach configure where to find tclConfig.sh. Our traditional method of searching $auto_path hasn't worked for the last couple of macOS releases, and it now seems clear that Apple's not going to change that. The workaround of manually specifying --with-tclconfig was annoying already, but Mojave's made it a lot more so because the sysroot path now has to be included as well. Let's just wire the knowledge into configure instead. To avoid breaking builds against non-default Tcl installations (e.g. MacPorts) wherein the $auto_path method probably still works, arrange to try the additional case only after all else has failed. Back-patch to all supported versions, since at least the buildfarm cares about that. The changes are set up to not do anything on macOS releases that are old enough to not have functional sysroot trees.
* Make contrib/unaccent's unaccent() function work when not in search path.Tom Lane2018-09-06
| | | | | | | | | | | | | | | | | | | | | | | | Since the fixes for CVE-2018-1058, we've advised people to schema-qualify function references in order to fix failures in code that executes under a minimal search_path setting. However, that's insufficient to make the single-argument form of unaccent() work, because it looks up the "unaccent" text search dictionary using the search path. The most expedient answer seems to be to remove the search_path dependency by making it look in the same schema that the unaccent() function itself is declared in. This will definitely work for the normal usage of this function with the unaccent dictionary provided by the extension. It's barely possible that there are people who were relying on the search-path-dependent behavior to select other dictionaries with the same name; but if there are any such people at all, they can still get that behavior by writing unaccent('unaccent', ...), or possibly unaccent('unaccent'::text::regdictionary, ...) if the lookup has to be postponed to runtime. Per complaint from Gunnlaugur Thor Briem. Back-patch to all supported branches. Discussion: https://postgr.es/m/CAPs+M8LCex6d=DeneofdsoJVijaG59m9V0ggbb3pOH7hZO4+cQ@mail.gmail.com
* Avoid using potentially-under-aligned page buffers.Tom Lane2018-09-01
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | There's a project policy against using plain "char buf[BLCKSZ]" local or static variables as page buffers; preferred style is to palloc or malloc each buffer to ensure it is MAXALIGN'd. However, that policy's been ignored in an increasing number of places. We've apparently got away with it so far, probably because (a) relatively few people use platforms on which misalignment causes core dumps and/or (b) the variables chance to be sufficiently aligned anyway. But this is not something to rely on. Moreover, even if we don't get a core dump, we might be paying a lot of cycles for misaligned accesses. To fix, invent new union types PGAlignedBlock and PGAlignedXLogBlock that the compiler must allocate with sufficient alignment, and use those in place of plain char arrays. I used these types even for variables where there's no risk of a misaligned access, since ensuring proper alignment should make kernel data transfers faster. I also changed some places where we had been palloc'ing short-lived buffers, for coding style uniformity and to save palloc/pfree overhead. Since this seems to be a live portability hazard (despite the lack of field reports), back-patch to all supported versions. Patch by me; thanks to Michael Paquier for review. Discussion: https://postgr.es/m/1535618100.1286.3.camel@credativ.de
* Enforce cube dimension limit in all cube construction functionsAlexander Korotkov2018-08-31
| | | | | | | | | | | | | | contrib/cube has a limit to 100 dimensions for cube datatype. However, it's not enforced everywhere, and one can actually construct cube with more than 100 dimensions having then trouble with dump/restore. This commit add checks for dimensions limit in all functions responsible for cube construction. Backpatch to all supported versions. Reported-by: Andrew Gierth Discussion: https://postgr.es/m/87va7uybt4.fsf%40news-spur.riddles.org.uk Author: Andrey Borodin with small additions by me Review: Tom Lane Backpatch-through: 9.3
* Split contrib/cube platform-depended checks into separate testAlexander Korotkov2018-08-31
| | | | | | | | | | | | We're currently maintaining two outputs for cube regression test. But that appears to be unsuitable, because these outputs are different in out few checks involving scientific notation. So, split checks involving scientific notation into separate test, making contrib/cube easier to maintain. Backpatch to all supported versions in order to make further backpatching easier. Discussion: https://postgr.es/m/CAPpHfdvJgWjxHsJTtT%2Bo1tz3OR8EFHcLQjhp-d3%2BUcmJLh-fQA%40mail.gmail.com Author: Alexander Korotkov Backpatch-through: 9.3
* Fix earthdistance test suite function name typo.Noah Misch2018-07-29
| | | | | | Affected test queries have been testing the wrong thing since their introduction in commit 4c1383efd132e4f532213c8a8cc63a455f55e344. Back-patch to 9.3 (all supported versions).
* Fix crash in contrib/ltree's lca() function for empty input array.Tom Lane2018-07-13
| | | | | | | | | | | | | | | | | | | | lca_inner() wasn't prepared for the possibility of getting no inputs. Fix that, and make some cosmetic improvements to the code while at it. Also, I thought the documentation of this function as returning the "longest common prefix" of the paths was entirely misleading; it really returns a path one shorter than the longest common prefix, for the typical definition of "prefix". Don't use that term in the docs, and adjust the examples to clarify what really happens. This has been broken since its beginning, so back-patch to all supported branches. Per report from Hailong Li. Thanks to Pierre Ducroquet for diagnosing and for the initial patch, though I whacked it around some and added test cases. Discussion: https://postgr.es/m/5b0d8e4f-f2a3-1305-d612-e00e35a7be66@qunar.com
* Prevent accidental linking of system-supplied copies of libpq.so etc.Tom Lane2018-07-09
| | | | | | | | | | | | | | | | | | | | | | | | | | | Back-patch commit dddfc4cb2, which broke LDFLAGS and related Makefile variables into two parts, one for within-build-tree library references and one for external libraries, to ensure that the order of -L flags has all of the former before all of the latter. This turns out to fix a problem recently noted on buildfarm member peripatus, that we attempted to incorporate code from libpgport.a into a shared library. That will fail on platforms that are sticky about putting non-PIC code into shared libraries. (It's quite surprising we hadn't seen such failures before, since the code in question has been like that for a long time.) I think that peripatus' problem could have been fixed with just a subset of this patch; but since the previous issue of accidentally linking to the wrong copy of a Postgres shlib seems likely to bite people in the field, let's just back-patch the whole change. Now that commit dddfc4cb2 has survived some beta testing, I'm less afraid to back-patch it than I was at the time. This also fixes undesired inclusion of "-DFRONTEND" in pg_config's CPPFLAGS output (in 9.6 and up) and undesired inclusion of "-L../../src/common" in its LDFLAGS output (in all supported branches). Back-patch to v10 and older branches; this is already in v11. Discussion: https://postgr.es/m/20180704234304.bq2dxispefl65odz@ler-imac.local
* Reduce cost of test_decoding's new oldest_xmin testAlvaro Herrera2018-07-05
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Change a whole-database VACUUM into doing just pg_attribute, which is the portion that verifies what we want it to do. The original formulation wastes a lot of CPU time, which leads the test to fail when runtime exceeds isolationtester timeout when it's super-slow, such as under CLOBBER_CACHE_ALWAYS. Per buildfarm member friarbird. It turns out that the previous shape of the test doesn't always detect the condition it is supposed to detect (on unpatched reorderbuffer code): the reason is that there is a good chance of encountering a xl_running_xacts record (logged every 15 seconds) before the checkpoint -- and because we advance the xmin when we receive that WAL record, and we *don't* advance the xmin twice consecutively without receiving a client message in between, that means the xmin is not advanced enough for the tuple to be pruned from pg_attribute by VACUUM. So the test would spuriously pass. The reason this test deficiency wasn't detected earlier is that HOT pruning removes the tuple anyway, even if vacuum leaves it in place, so the test correctly fails (detecting the coding mistake), but for the wrong reason. To fix this mess, run the s0_get_changes step twice before vacuum instead of once: this seems to cause the xmin to be advanced reliably, wreaking havoc with more certainty. Author: Arseny Sher Discussion: https://postgr.es/m/87h8lkuxoa.fsf@ars-thinkpad
* Fix "base" snapshot handling in logical decodingAlvaro Herrera2018-06-26
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Two closely related bugs are fixed. First, xmin of logical slots was advanced too early. During xl_running_xacts processing, xmin of the slot was set to the oldest running xid in the record, but that's wrong: actually, snapshots which will be used for not-yet-replayed transactions might consider older txns as running too, so we need to keep xmin back for them. The problem wasn't noticed earlier because DDL which allows to delete tuple (set xmax) while some another not-yet-committed transaction looks at it is pretty rare, if not unique: e.g. all forms of ALTER TABLE which change schema acquire ACCESS EXCLUSIVE lock conflicting with any inserts. The included test case (test_decoding's oldest_xmin) uses ALTER of a composite type, which doesn't have such interlocking. To deal with this, we must be able to quickly retrieve oldest xmin (oldest running xid among all assigned snapshots) from ReorderBuffer. To fix, add another list of ReorderBufferTXNs to the reorderbuffer, where transactions are sorted by base-snapshot-LSN. This is slightly different from the existing (sorted by first-LSN) list, because a transaction can have an earlier LSN but a later Xmin, if its first record does not obtain an xmin (eg. xl_xact_assignment). Note this new list doesn't fully replace the existing txn list: we still need that one to prevent WAL recycling. The second issue concerns SnapBuilder snapshots and subtransactions. SnapBuildDistributeNewCatalogSnapshot never assigned a snapshot to a transaction that is known to be a subtxn, which is good in the common case that the top-level transaction already has one (no point in doing so), but a bug otherwise. To fix, arrange to transfer the snapshot from the subtxn to its top-level txn as soon as the kinship gets known. test_decoding's snapshot_transfer verifies this. Also, fix a minor memory leak: refcount of toplevel's old base snapshot was not decremented when the snapshot is transferred from child. Liberally sprinkle code comments, and rewrite a few existing ones. This part is my (Álvaro's) contribution to this commit, as I had to write all those comments in order to understand the existing code and Arseny's patch. Reported-by: Arseny Sher <a.sher@postgrespro.ru> Diagnosed-by: Arseny Sher <a.sher@postgrespro.ru> Co-authored-by: Arseny Sher <a.sher@postgrespro.ru> Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org> Reviewed-by: Antonin Houska <ah@cybertec.at> Discussion: https://postgr.es/m/87lgdyz1wj.fsf@ars-thinkpad
* Fix contrib/hstore_plperl to look through scalar refs.Tom Lane2018-06-18
| | | | | | | | | | | | | | Bring this transform function into sync with the policy established by commit 3a382983d. Also, fix it to make sure that what it drills down to is indeed a hash, and not some other kind of Perl SV. Previously, the test cases added here provoked crashes. Because of the crash hazard, back-patch to 9.5 where this module was introduced. Discussion: https://postgr.es/m/28336.1528393969@sss.pgh.pa.us
* Fix potentially-unportable code in contrib/adminpack.Tom Lane2018-04-15
| | | | | | | | | | | | | | | Spelling access(2)'s second argument as "2" is just horrid. POSIX makes no promises as to the numeric values of W_OK and related macros. Even if it accidentally works as intended on every supported platform, it's still unreadable and inconsistent with adjacent code. In passing, don't spell "NULL" as "0" either. Yes, that's legal C; no, it's not project style. Back-patch, just in case the unportability is real and not theoretical. (Most likely, even if a platform had different bit assignments for access()'s modes, there'd not be an observable behavior difference here; but I'm being paranoid today.)
generated by cgit v1.2.3 (git 2.25.1) at 2025-08-07 02:56:33 +0000