- 15 Apr, 2021 9 commits
-
-
Tom Lane authored
This reverts commit b3ee4c50. We don't need it in the wake of the preceding commit, which added an upstream check that the querystring isn't null. Discussion: https://postgr.es/m/2197698.1617984583@sss.pgh.pa.us
-
Tom Lane authored
Commit e717a9a1 changed the longstanding rule that prosrc is NOT NULL because when a SQL-language function is written in SQL-standard style, we don't currently have anything useful to put there. This seems a poor decision though, as it could easily have negative impacts on external PLs (opening them to crashes they didn't use to have, for instance). SQL-function-related code can just as easily test "is prosqlbody not null" as "is prosrc null", so there's no real gain there either. Hence, revert the NOT NULL marking removal and adjust related logic. For now, we just put an empty string into prosrc for SQL-standard functions. Maybe we'll have a better idea later, although the history of things like pg_attrdef.adsrc suggests that it's not easy to maintain a string equivalent of a node tree. This also adds an assertion that queryDesc->sourceText != NULL to standard_ExecutorStart. We'd been silently relying on that for awhile, so let's make it less silent. Also fix some overlooked documentation and test cases. Discussion: https://postgr.es/m/2197698.1617984583@sss.pgh.pa.us
-
Tom Lane authored
These queries could show unexpected entries if the core system, or concurrently-running test scripts, created any functions that would appear in the information_schema views. Restrict them to showing functions belonging to this test's schema, as the far-older nearby test case does. Per experimentation with conversion of some built-in functions to SQL-function-body style.
-
Peter Eisentraut authored
This reverts commit 3a513067. Per discussion, this patch had too many issues to resolve at this point of the development cycle. We'll try again in the future. Discussion: https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904132231510.8961@lancre
-
Fujii Masao authored
Commit bbe0a81d introduced "INCLUDING COMPRESSION" option in CREATE TABLE command, but forgot to mention it in the CREATE TABLE syntax synopsis. Author: Fujii Masao Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/54d30e66-dbd6-5485-aaf6-a291ed55919d@oss.nttdata.com
-
Michael Paquier authored
e4c76191 has added a space to the example used for HISTFILE in the docs of psql before the variable DBNAME, as a workaround because variables were not parsed the same way back then. This behavior has changed in 9.2, causing the example in the psql docs to result in the same history file created with or without a space added before the DBNAME variable. Let's just remove this space in the example, to reduce any confusion, as the point of it is to prove that a per-database history file is easy to set up, and that's easier to read this way. Per discussion with Tom Lane. Reported-by: Ludovic Kuty Discussion: https://postgr.es/m/161830067409.691.16198363670687811485@wrigleys.postgresql.org
-
Peter Eisentraut authored
-
Peter Eisentraut authored
Several of these were already fixed in passing in 9acaf1a6, but one was remaining inconsistent.
-
Michael Paquier authored
6568cef2, that introduced the option, had an inconsistent behavior when it comes to configuration tables set up by pg_extension_config_dump, as the data of all configuration tables would included in a dump even for extensions not listed by a set of --extension switches. The contents dumped changed depending on the schema where an extension was installed when an extension was not listed. For example, an extension installed under the public schema would have its configuration data not dumped even when not listed with --extension, which was inconsistent with the case of an extension installed on a non-public schema, where the configuration would be dumped. Per discussion with Noah, we have settled down to the simple rule of dumping configuration data of an extension if it is listed in --extension (default is unchanged and backward-compatible, to dump everything on sight if there are no extensions directly listed). This avoids some weird cases where the dumps depended on a --schema for one. More tests are added to cover the gap, where we cross-check more behaviors depending on --schema when an extension is not listed. Reported-by: Noah Misch Reviewed-by: Noah Misch Discussion: https://postgr.es/m/20210404220802.GA728316@rfd.leadboat.com
-
- 14 Apr, 2021 6 commits
-
-
Tom Lane authored
That field went away in commit edca44b1, but it seems that commit 45be99f8 re-introduced some comments mentioning it. Noted by James Coleman, though this isn't exactly his proposed new wording. Also thanks to Justin Pryzby for software archaeology. Discussion: https://postgr.es/m/CAAaqYe8fxZjq3na+XkNx4C78gDqykH-7dbnzygm9Qa9nuDTePg@mail.gmail.com
-
Robert Haas authored
We don't need to mention the attribute number in these messages, because there's a dedicated column for that, but we should mention the toast value ID, because that's really useful for any follow-up troubleshooting the user wants to do. This also rewords some of the messages to hopefully read a little better. Also, use VARATT_EXTERNAL_GET_POINTER in case we're accessing a TOAST pointer that isn't aligned on a platform that's fussy about alignment, so that we don't crash while corruption-checking the user's data. Mark Dilger, reviewed by me. Discussion: http://postgr.es/m/7D3B9BF6-50D0-4C30-8506-1C1851C7F96F@enterprisedb.com
-
Peter Eisentraut authored
-
Michael Paquier authored
This GUC has always been classified as a planner option since its introduction in 7c944bd9, and was listed in postgresql.conf.sample. As this parameter exists for testing purposes, move it to the section dedicated to developer parameters and hence remove it from postgresql.conf.sample. This will avoid any temptation to play with it on production servers for users that should never really have to touch this parameter. The general description used for developer options is reworded a bit, to take into account the inclusion of force_parallel_mode, per a suggestion from Tom Lane. Per discussion between Tom Lane, Bruce Momjian, Justin Pryzby, Bharath Rupireddy and me. Author: Justin Pryzby, Tom Lane Discussion: https://postgr.es/m/20210403152402.GA8049@momjian.us
-
Michael Paquier authored
The tests introduced in 32a9c0bd for connections broken and re-established rely on pg_terminate_backend() for their logic. When these were introduced, this function simply sent a signal to a backend without waiting for the operation to complete, and the tests repeatedly looked at pg_stat_activity to check if the operation was completed or not. Since aaf04325, it is possible to define a timeout to make pg_terminate_backend() wait for a certain duration, so make use of it, with a timeout reasonably large enough (3min) to give enough room for the tests to pass even on slow machines. Some measurements show that the tests of postgres_fdw are much faster with this change. For example, on my laptop, they now take 4s instead of 6s. Author: Bharath Rupireddy Discussion: https://postgr.es/m/CALj2ACXGY_EfGrMTjKjHy2zi-u1u9rdeioU_fro0T6Jo8t56KQ@mail.gmail.com
-
Amit Kapila authored
This will make it consistent with the other usage of slotname in the code. In the passing, change pgstat_report_replslot signature to use a structure rather than multiple parameters. Reported-by: Andres Freund Author: Vignesh C Reviewed-by: Sawada Masahiko, Amit Kapila Discussion: https://postgr.es/m/20210319185247.ldebgpdaxsowiflw@alap3.anarazel.de
-
- 13 Apr, 2021 12 commits
-
-
Tomas Vondra authored
The function is building a fake heap tuple, but left some of the header fields (tid and table OID) uninitialized. Per Coverity report. Reported-by: Ranier Vilela Discussion: https://postgr.es/m/CAEudQApj6h8tZ0-eP5Af5PKc5NG1YUc7=SdN_99YoHS51fKa0Q@mail.gmail.com
-
Peter Geoghegan authored
It seems like a good idea to bypass heap truncation when the wraparound failsafe mechanism (which was added in commit 1e55e7d1) is in effect. Deliberately don't bypass heap truncation in the INDEX_CLEANUP=off case, even though it is similar to the failsafe case. There is already a separate reloption (and related VACUUM parameter) for that. Reported-By: Masahiko Sawada <sawada.mshk@gmail.com> Discussion: https://postgr.es/m/CAD21AoDWRh6oTN5T8wa+cpZUVpHXET8BJ8Da7WHVHpwkPP6KLg@mail.gmail.com
-
Tom Lane authored
Previously you could only use unqualified variable names here. While that's not a functional deficiency, since only the target table can be referenced, it's a surprising inconsistency with the rules for partial-index predicates, on which this syntax is supposedly modeled. The fix for that is no harder than passing addToRelNameSpace = true to addNSItemToQuery. However, it's really pretty bogus for transformOnConflictArbiter and transformOnConflictClause to be messing with the namespace item for the target table at all. It's not theirs to manage, it results in duplicative creations of namespace items, and transformOnConflictClause wasn't even doing it quite correctly (that coding resulted in two nsitems for the target table, since it hadn't cleaned out the existing one). Hence, make transformInsertStmt responsible for setting up the target nsitem once for both these clauses and RETURNING. Also, arrange for ON CONFLICT ... UPDATE's "excluded" pseudo-relation to be added to the rangetable before we run transformOnConflictArbiter. This produces a more helpful HINT if someone writes "excluded.col" in the arbiter expression. Per bug #16958 from Lukas Eder. Although I agree this is a bug, the consequences are hardly severe, so no back-patch. Discussion: https://postgr.es/m/16958-963f638020de271c@postgresql.org
-
Robert Haas authored
Mention that there are multiple TOAST compression methods and that the compression method used is stored in a TOAST pointer along with the other information that was stored there previously. Add a reference to the documentation for default_toast_compression, where the supported methods are listed, instead of duplicating that here. I haven't tried to preserve the text claiming that pglz is "fairly simple and very fast." I have no view on the veracity of the former claim, but LZ4 seems to be faster (and to compress better) so it seems better not to muddy the waters by talking about compression speed as a strong point of PGLZ. Patch by me, reviewed by Justin Pryzby. Discussion: http://postgr.es/m/CA+Tgmoaw_YBwQhOS_hhEPPwFhfAnu+VCLs18EfGr9gQw1z4H-w@mail.gmail.com
-
Tom Lane authored
Most GUC check hooks that inspect database state have special checks that prevent them from throwing hard errors for state-dependent issues when source == PGC_S_TEST. This allows, for example, "ALTER DATABASE d SET default_text_search_config = foo" when the "foo" configuration hasn't been created yet. Without this, we have problems during dump/reload or pg_upgrade, because pg_dump has no idea about possible dependencies of GUC values and can't ensure a safe restore ordering. However, check_role() and check_session_authorization() hadn't gotten the memo about that, and would throw hard errors anyway. It's not entirely clear what is the use-case for "ALTER ROLE x SET role = y", but we've now heard two independent complaints about that bollixing an upgrade, so apparently some people are doing it. Hence, fix these two functions to act more like other check hooks with similar needs. (But I did not change their insistence on being inside a transaction, as it's still not apparent that setting either GUC from the configuration file would be wise.) Also fix check_temp_buffers, which had a different form of the disease of making state-dependent checks without any exception for PGC_S_TEST. A cursory survey of other GUC check hooks did not find any more issues of this ilk. (There are a lot of interdependencies among PGC_POSTMASTER and PGC_SIGHUP GUCs, which may be a bad idea, but they're not relevant to the immediate concern because they can't be set via ALTER ROLE/DATABASE.) Per reports from Charlie Hornsby and Nathan Bossart. Back-patch to all supported branches. Discussion: https://postgr.es/m/HE1P189MB0523B31598B0C772C908088DB7709@HE1P189MB0523.EURP189.PROD.OUTLOOK.COM Discussion: https://postgr.es/m/20160711223641.1426.86096@wrigleys.postgresql.org
-
Tom Lane authored
Previously, get_cached_rowtype() cached a pointer to a reference-counted tuple descriptor from the typcache, relying on the ExprContextCallback mechanism to release the tupdesc refcount when the expression tree using the tupdesc was destroyed. This worked fine when it was designed, but the introduction of within-DO-block COMMITs broke it. The refcount is logged in a transaction-lifespan resource owner, but plpgsql won't destroy simple expressions made within the DO block (before its first commit) until the DO block is exited. That results in a warning about a leaked tupdesc refcount when the COMMIT destroys the original resource owner, and then an error about the active resource owner not holding a matching refcount when the expression is destroyed. To fix, get rid of the need to have a shutdown callback at all, by instead caching a pointer to the relevant typcache entry. Those survive for the life of the backend, so we needn't worry about the pointer becoming stale. (For registered RECORD types, we can still cache a pointer to the tupdesc, knowing that it won't change for the life of the backend.) This mechanism has been in use in plpgsql and expandedrecord.c since commit 4b93f579, and seems to work well. This change requires modifying the ExprEvalStep structs used by the relevant expression step types, which is slightly worrisome for back-patching. However, there seems no good reason for extensions to be familiar with the details of these particular sub-structs. Per report from Rohit Bhogate. Back-patch to v11 where within-DO-block COMMITs became a thing. Discussion: https://postgr.es/m/CAAV6ZkQRCVBh8qAY+SZiHnz+U+FqAGBBDaDTjF2yiKa2nJSLKg@mail.gmail.com
-
Tom Lane authored
heap_update needs to clear any existing "all visible" flag on the old tuple's page (and on the new page too, if different). Per coding rules, to do this it must acquire pin on the appropriate visibility-map page while not holding exclusive buffer lock; which creates a race condition since someone else could set the flag whenever we're not holding the buffer lock. The code is supposed to handle that by re-checking the flag after acquiring buffer lock and retrying if it became set. However, one code path through heap_update itself, as well as one in its subroutine RelationGetBufferForTuple, failed to do this. The end result, in the unlikely event that a concurrent VACUUM did set the flag while we're transiently not holding lock, is a non-recurring "PANIC: wrong buffer passed to visibilitymap_clear" failure. This has been seen a few times in the buildfarm since recent VACUUM changes that added code paths that could set the all-visible flag while holding only exclusive buffer lock. Previously, the flag was (usually?) set only after doing LockBufferForCleanup, which would insist on buffer pin count zero, thus preventing the flag from becoming set partway through heap_update. However, it's clear that it's heap_update not VACUUM that's at fault here. What's less clear is whether there is any hazard from these bugs in released branches. heap_update is certainly violating API expectations, but if there is no code path that can set all-visible without a cleanup lock then it's only a latent bug. That's not 100% certain though, besides which we should worry about extensions or future back-patch fixes that could introduce such code paths. I chose to back-patch to v12. Fixing RelationGetBufferForTuple before that would require also back-patching portions of older fixes (notably 0d1fe9f7), which is more code churn than seems prudent to fix a hypothetical issue. Discussion: https://postgr.es/m/2247102.1618008027@sss.pgh.pa.us
-
Fujii Masao authored
Introduced in commit 0aa8a01d. Author: Peter Smith Discussion: https://postgr.es/m/CAHut+Ps8rkVHKWyjg09Fb1PaVG5-EhoFTFJ9OZMF4HPzDSXfew@mail.gmail.com
-
Noah Misch authored
With the Oracle Developer Studio 12.6 compiler, #line directives alter the current source file location for purposes of #include "..." directives. Hence, a VPATH build failed with 'cannot find include file: "specscanner.c"'. With two exceptions, parser-containing directories already add "-I. -I$(srcdir)"; eliminate the exceptions. Back-patch to 9.6 (all supported versions).
-
Noah Misch authored
It doesn't support "\(foo\)*" like a POSIX "sed" implementation does; see the Autoconf manual. Back-patch to 9.6 (all supported versions).
-
Thomas Munro authored
Commit 6f38d4da failed to heed a warning about the stability of the value pointed to by "otid". The caller is allowed to pass in a pointer to newtup->t_self, which will be updated during the execution of the function. Instead, the SSI check should use the value we copy into oldtup.t_self near the top of the function. Not a live bug, because newtup->t_self doesn't really get updated until a bit later, but it was confusing and broke the rule established by the comment. Back-patch to 13. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/2689164.1618160085%40sss.pgh.pa.us
-
Michael Paquier authored
These got introduced in 6568cef2. Reported-by: Noah Misch Discussion: https://postgr.es/m/20210404220802.GA728316@rfd.leadboat.com
-
- 12 Apr, 2021 9 commits
-
-
Tom Lane authored
collate.icu.utf8.sql was exercising the recording of a collation dependency for an enum comparison expression, but such an expression should never have had any collation dependency in the first place. After I fixed that in commit c402b02b, the test started failing. We don't need to test that scenario anymore, so just remove the now-useless test steps. (This test case is new in HEAD, so no need to back-patch.) Discussion: https://postgr.es/m/3044030.1618261159@sss.pgh.pa.us Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com
-
Tom Lane authored
There are hacks in parse_coerce.c to push down a requested coercion to below any CollateExpr that may appear. However, we did that even if the requested data type is non-collatable, leading to an invalid expression tree in which CollateExpr is applied to a non-collatable type. The fix is just to drop the CollateExpr altogether, reasoning that it's useless. This bug is ten years old, dating to the original addition of COLLATE support. The lack of field complaints suggests that there aren't a lot of user-visible consequences. We noticed the problem because it would trigger an assertion in DefineVirtualRelation if the invalid structure appears as an output column of a view; however, in a non-assert build, you don't see a crash just a (subtly incorrect) complaint about applying collation to a non-collatable type. I found that by putting the incorrect structure further down in a view, I could make a view definition that would fail dump/reload, per the added regression test case. But CollateExpr doesn't do anything at run-time, so this likely doesn't lead to any really exciting consequences. Per report from Yulin Pei. Back-patch to all supported branches. Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com
-
Peter Eisentraut authored
This could write wrong output into the cluster deletion script if a database OID exceeds the signed 32-bit range.
-
Peter Eisentraut authored
-
Peter Eisentraut authored
broken by 37d2ff38
-
Fujii Masao authored
Commit 8ff1c946 extended TRUNCATE command so that it can also truncate foreign tables. But it forgot to support tab-complete for TRUNCATE on foreign tables. That is, previously tab-complete for TRUNCATE displayed only the names of regular tables. This commit improves tab-complete for TRUNCATE so that it displays also the names of foreign tables. Author: Fujii Masao Reviewed-by: Bharath Rupireddy Discussion: https://postgr.es/m/551ed8c1-f531-818b-664a-2cecdab99cd8@oss.nttdata.com
-
Michael Paquier authored
This GUC has already been classified as LOGGING_WHAT, but its location in postgresql.conf.sample and the documentation did not reflect that, so fix those inconsistencies. Author: Justin Pryzby Discussion: https://postgr.es/m/20210404012546.GK6592@telsasoft.com
-
Amit Kapila authored
Updated documentation for new messages added for streaming of in-progress transactions, as well as changes made to the existing messages. It also updates the information of protocol versions supported for logical replication. Author: Ajin Cherian Reviewed-by: Amit Kapila, Peter Smith, Euler Taveira Discussion: https://postgr.es/m/CAFPTHDYHN9m=MZZct-B=BYg_TETvv+kXvL9RD2DpaBS5pGxGYg@mail.gmail.com
-
Michael Paquier authored
Using Roman numbers (via "RM" or "rm") for a conversion to calculate a number of months has never considered the case of negative numbers, where a conversion could easily cause out-of-bound memory accesses. The conversions in themselves were not completely consistent either, as specifying 12 would result in NULL, but it should mean XII. This commit reworks the conversion calculation to have a more consistent behavior: - If the number of months and years is 0, return NULL. - If the number of months is positive, return the exact month number. - If the number of months is negative, do a backward calculation, with -1 meaning December, -2 November, etc. Reported-by: Theodor Arsenij Larionov-Trichkin Author: Julien Rouhaud Discussion: https://postgr.es/m/16953-f255a18f8c51f1d5@postgresql.org backpatch-through: 9.6
-
- 11 Apr, 2021 4 commits
-
-
Tom Lane authored
Coverity complained about possible overflow in expressions like intresult = tm->tm_sec * 1000000 + fsec; on the grounds that the multiplication would happen in 32-bit arithmetic before widening to the int64 result. I think these are all false positives because of the limited possible range of tm_sec; but nonetheless it seems silly to spell it like that when nearby lines have the identical computation written with a 64-bit constant. ... or more accurately, with an LL constant, which is not project style. Make all of these use INT64CONST(), as we do elsewhere. This is all new code from a2da77cd, so no need for back-patch.
-
Tom Lane authored
We'd previously noted the need for coping with Windows headers that provide some other definition of macro "ERROR" than elog.h does. It turns out that R also wants to define ERROR, and WARNING too. PL/R has been working around this in a hacky way that broke when we recently changed the numeric value of ERROR. To let them have a more future-proof solution, provide an alternate macro PGWARNING for WARNING, and make PGERROR visible always, not only when #ifdef WIN32. Discussion: https://postgr.es/m/CADK3HHK6iMChd1yoOqssxBn5Z14Zar8Ztr3G-N_fuG7F8YTP3w@mail.gmail.com
-
Tom Lane authored
The path for *exprs != NIL would misbehave, and likely crash, since pull_varattnos expects its last argument to be valid at call. Found by Coverity --- we have no coverage of this path in the regression tests.
-
Fujii Masao authored
ExecuteTruncate() filters out the duplicate tables specified in the TRUNCATE command, for example in the case where "TRUNCATE foo, foo" is executed. Such duplicate tables obviously don't need to be opened and closed because they are skipped. But previously it always opened the tables before checking whether they were duplicated ones or not, and then closed them if they were. That is, the duplicated tables were opened and closed unnecessarily. This commit changes ExecuteTruncate() so that it opens the table after it confirms that table is not duplicated one, which leads to avoid unnecessary table open/close. Do not back-patch because such unnecessary table open/close is not a bug though it exists in older versions. Author: Bharath Rupireddy Reviewed-by: Amul Sul, Fujii Masao Discussion: https://postgr.es/m/CALj2ACUdBO_sXJTa08OZ0YT0qk7F_gAmRa9hT4dxRcgPS4nsZA@mail.gmail.com
-