- 18 Apr, 2018 3 commits
-
-
Tom Lane authored
Clean up error messages relating to mistakes in .dat files: make sure they provide the .dat file name and line number, not the place in the Perl script that's reporting the problem. Adopt more uniform message phrasing, too. Make genbki.pl spit up on unrecognized field names in the input hashes. Previously, it just silently ignored such fields, which could make a misspelled field name into a very hard-to-decipher problem. (This is in genbki.pl, *not* Catalog.pm, because we don't want reformat_dat_file.pl to complain about unrecognized fields. We'd rather it silently dropped them, to facilitate removing unwanted fields after a reorganization.)
-
Tom Lane authored
Commit 54eff531 did not account for the possibility that we'd have a transaction snapshot due to default_transaction_isolation being set high enough to require one. The transaction snapshot is enough to hold back our advertised xmin and thus risk deadlock anyway. The only way to get rid of that snap is to start a new transaction, so let's do that instead. Also throw in an assert checking that we really have gotten to a state where no xmin is being advertised. Back-patch to 9.4, like the previous commit. Discussion: https://postgr.es/m/CAMkU=1ztk3TpQdcUNbxq93pc80FrXUjpDWLGMeVBDx71GHNwZQ@mail.gmail.com
-
Heikki Linnakangas authored
Author: Michael Paquier Discussion: https://www.postgresql.org/message-id/20180411082020.GD19732%40paquier.xyz
-
- 17 Apr, 2018 8 commits
-
-
Tom Lane authored
Change things around so that proper quoting of values interpolated into the BKI data by initdb is the responsibility of initdb, not something we half-heartedly handle by putting double quotes into the raw BKI data. (Note: experimentation shows that it still doesn't work to put a double quote into the initial superuser username, but that's the fault of inadequate quoting while interpolating the name into SQL scripts; the BKI aspect of it works fine now.) Having done that, we can remove the special-case handling of values that look like "something" from genbki.pl, and instead teach it to escape double --- and single --- quotes properly. This removes the nowhere-documented need to treat those specially in the BKI source data; whatever you write will be passed through unchanged into the inserted data value, modulo Perl's rules about single-quoted strings. Add documentation explaining the (pre-existing) handling of backslashes in the BKI data. Per an earlier discussion with John Naylor. Discussion: https://postgr.es/m/CAJVSVGUNao=-Q2-vAN3PYcdF5tnL5JAHwGwzZGuYHtq+Mk_9ng@mail.gmail.com
-
Tom Lane authored
Formerly, Catalog.pm turned a C array type declaration in the catalog header files into a SQL type, e.g., 'foo[]'. Along the way, genbki.pl turned this into '_foo' for the purpose of type lookups, but wrote 'foo[]' to postgres.bki. During bootstrap, bootscanner.l had to have a special case rule to tokenize this, and then MapArrayTypeName() would turn 'foo[]' into '_foo' one more time. This seems unnecessarily complicated, especially since nobody cares that much about the readability of postgres.bki. Instead, make Catalog.pm convert the C declaration into '_foo' to start with, and preserve that representation of the type name throughout bootstrap data processing. Then rip out the special-case code in bootscanner.l and bootstrap.c. This changes postgres.bki to the extent that array fields are now declared like proconfig = _text , rather than proconfig = text[] , No documentation update, since the SGML docs didn't mention any of this in the first place, and it's all pretty transparent to writers of catalog header files anyway. John Naylor Discussion: https://postgr.es/m/CAJVSVGUNao=-Q2-vAN3PYcdF5tnL5JAHwGwzZGuYHtq+Mk_9ng@mail.gmail.com
-
Tom Lane authored
During the bootstrap data format conversion, it seemed important for verifiability's sake that the generated postgres.bki file stayed the same as before. That resulted in adding a bunch of ad-hoc rules about when to quote emitted data values, to match previous manual decisions that had often quoted values unnecessarily. Now that the conversion is complete, it seems fine to remove all those ad-hoc rules. The net actual effect on the current contents of postgres.bki is that some fields that had been quoted despite containing only digits or only "-" lose their unnecessary quotes. Also, now that genbki.pl will always quote values containing a backslash, there's no need for bootscanner.l to allow unquoted octal escapes; so simplify its production for "id" by removing that possibility. John Naylor, slightly modified by me Discussion: https://postgr.es/m/CAJVSVGUNao=-Q2-vAN3PYcdF5tnL5JAHwGwzZGuYHtq+Mk_9ng@mail.gmail.com
-
Heikki Linnakangas authored
Review of commit 1eb6d652: It's pointless to add padding to the GID fields, when the code that follows assumes that there is no alignment, and uses memcpy(). Remove the pointless padding. Update comments to note the new fields in the WAL records. Reviewed-by: Michael Paquier Discussion: https://www.postgresql.org/message-id/33b787bf-dc20-1161-54e9-3f3b607bf59d%40iki.fi
-
Alvaro Herrera authored
It turns out that after runtime partition pruning, Append's first_partial_plan does not accurately represent partial plans to run, if any of those got pruned. This could limit participation of workers in some partial subplans, if other subplans got pruned. Fix it by keeping an index of the first valid partial subplan in the state node, determined at execnode Init time. Author: David Rowley, with cosmetic changes by me. Discussion: https://postgr.es/m/CAKJS1f8o2Yd=rOP=Et3A0FWgF+gSAOkFSU6eNhnGzTPV7nN8sQ@mail.gmail.com
-
Heikki Linnakangas authored
Author: Michael Paquier Discussion: https://www.postgresql.org/message-id/20180411075223.GB19732%40paquier.xyz
-
Alvaro Herrera authored
coverage report indicated that mark_invalid_subplans_as_finished() and nearby code was not getting exercised by any tests. Add a new one which has execution-time Params rather than only external Params to fix this. In passing, David noticed that ab_q6 tests were not actually required to have a generic plan. The tests were testing exec Params not external Params, so there was no need for the PREPARE. Remove the PREPARE, making these plain queries. (The new queries are called from explain_parallel_append, which may be unnecessary since they don't actually have a Parallel Append node, just an Append. But it doesn't seem to hurt anything, either.) Author: David Rowley Discussion: https://postgr.es/m/CAKJS1f--hopb6JBSDY4wiXTS3ZcDp-wparXjTQ1nzNdBa04Fog@mail.gmail.com
-
Tatsuo Ishii authored
Also add regression test cases for detecting infinite recursion in locking view tests. Some document enhancements. Patch by Yugo Nagata.
-
- 16 Apr, 2018 4 commits
-
-
Alvaro Herrera authored
This reverts commit 4d0f6d3f ("Attempt to stabilize partition_prune test output (2)"), and attempts to stabilize the test by using string replacement to hide any loop count difference in parallel nodes. Discussion: https://postgr.es/m/4475.1523628300@sss.pgh.pa.us
-
Tom Lane authored
spg_text_leaf_consistent() supposed that it should compare only Min(querylen, entrylen) bytes of the two strings, and then deal with any excess bytes in one string or the other by assuming the longer string is greater if the prefixes are equal. Quite aside from the fact that that's just wrong in some locales (e.g., 'ch' is not less than 'd' in cs_CZ), it also risked passing incomplete multibyte characters to strcoll(), with ensuing bad results. Instead, just pass the full strings to varstr_cmp, and let it decide what to do about unequal-length strings. Fortunately, this error doesn't imply any index corruption, it's just that searches might return the wrong set of entries. Per report from Emre Hasegeli, though this is not his patch. Thanks to Peter Geoghegan for review and discussion. This code was born broken, so back-patch to all supported branches. In HEAD, I failed to resist the temptation to do a bit of cosmetic cleanup/pgindent'ing on 710d90da, too. Discussion: https://postgr.es/m/CAE2gYzzb6K51VnTq5i5p52z+j9p2duEa-K1T3RrC_GQEynAKEg@mail.gmail.com
-
Alvaro Herrera authored
Forgot to 'git add' the file after tweaking the test as submitted :-( Per buildfarm
-
Alvaro Herrera authored
We had an Assert() preventing whole-row expressions from being used in the SET clause of INSERT ON CONFLICT, but it seems unnecessary, given some tests, so remove it. Add a new test to exercise the case. Still at ExecInitPartitionInfo, we used map_partition_varattnos (which constructs an attribute map, then calls map_variable_attnos) using the same two relations many times in different expressions and with different parameters. Constructing the map over and over is a waste. To avoid this repeated work, construct the map once, and use map_variable_attnos() directly instead. Author: Amit Langote, per comments by me (Álvaro) Discussion: https://postgr.es/m/20180326142016.m4st5e34chrzrknk@alvherre.pgsql
-
- 15 Apr, 2018 9 commits
-
-
Tom Lane authored
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.)
-
Tom Lane authored
Coverity complained about the lack of a check on the return value in parse_jsonb_index_flags' last call of JsonbIteratorNext. Seems like a reasonable gripe to me, especially since the code is depending on that being WJB_DONE to not leak memory, so add a check. In passing, improve a couple other places where the result was being ignored, either by adding an assert or at least a cast to void. Also, don't spell "WJB_DONE" as "0". That's horrid coding style, and it wasn't consistent either.
-
Magnus Hagander authored
Teach both base backups and pg_verify_checksums that if a page is new, it does not have a checksum yet, so it shouldn't be verified. Noted by Tomas Vondra, review by David Steele.
-
Magnus Hagander authored
They were accidentally excluded when reverting the backend online checksum functionality, and since they weren't built the incorrect reference to a removed section also did not trigger a problem. Author: Christoph Berg
-
Magnus Hagander authored
Make it clear that a cluster has to be shut down cleanly before pg_verify_checksum can be run against it. Author: Michael Paquier Review: Daniel Gustafsson
-
Magnus Hagander authored
This option makes no sense when the cluster checksum state cannot be changed, and should have been removed in the revert. Author: Daniel Gustafsson Review: Michael Paquier
-
Tom Lane authored
In the wake of commit 50c6bb02, it's not necessary for ApplyRetrieveRule to have a forUpdatePushedDown parameter. By the time control gets here for any given view-referencing RTE, we should already have pushed down the effects of any FOR UPDATE/SHARE clauses affecting the view from outer query levels. Hence if we don't find a RowMarkClause at the current query level, that's sufficient proof that there is no outer one either. This in turn means we need no forUpdatePushedDown parameter for fireRIRrules. I wonder whether we oughtn't also revert commit cba2d271, since it now seems likely that that was band-aiding around the bad effects of doing FOR UPDATE pushdown and view expansion in the wrong order. However, in the absence of evidence that the current coding of markQueryForLocking is actually buggy (i.e. missing RTEs it ought to mark), it seems best to leave it alone. Discussion: https://postgr.es/m/24db7b8f-3de5-e25f-7ab9-d8848351d42c@gmail.com
-
Alvaro Herrera authored
This omission prevented partitioning header files from being installed. Per buildfarm member crake.
-
Alvaro Herrera authored
There's been a massive addition of partitioning code in PostgreSQL 11, with little oversight on its placement, resulting in a catalog/partition.c with poorly defined boundaries and responsibilities. This commit tries to set a couple of distinct modules to separate things a little bit. There are no code changes here, only code movement. There are three new files: src/backend/utils/cache/partcache.c src/include/partitioning/partdefs.h src/include/utils/partcache.h The previous arrangement of #including catalog/partition.h almost everywhere is no more. Authors: Amit Langote and Álvaro Herrera Discussion: https://postgr.es/m/98e8d509-790a-128c-be7f-e48a5b2d8d97@lab.ntt.co.jp https://postgr.es/m/11aa0c50-316b-18bb-722d-c23814f39059@lab.ntt.co.jp https://postgr.es/m/143ed9a4-6038-76d4-9a55-502035815e68@lab.ntt.co.jp https://postgr.es/m/20180413193503.nynq7bnmgh6vs5vm@alvherre.pgsql
-
- 14 Apr, 2018 5 commits
-
-
Tom Lane authored
I was dissatisfied with the code coverage report for expand_tuple() in the wake of commit 7c44c46d: while better than no coverage at all, it was still not exercising the core function of inserting out-of-line default values, nor was the HeapTuple-output path covered. So far as I can find, the only code path that reaches the latter at present is EvalPlanQual fetches for non-locked tables. Hence, extend eval-plan-qual.spec to test cases where out-of-line defaults must be inserted into a tuple fetched from a non-locked table. Discussion: https://postgr.es/m/87woxi24uw.fsf@ansel.ydns.eu
-
Tom Lane authored
SELECT FOR UPDATE on a view should require UPDATE (as well as SELECT) permissions on the view, and then the view's owner needs those same permissions against the relations it references, and so on all the way down to base tables. But ApplyRetrieveRule did things in the wrong order, resulting in failure to mark intermediate view levels as needing UPDATE permission. Thus for example, if user A creates a table T and an updatable view V1 on T, then grants only SELECT permissions on V1 to user B, B could create a second view V2 on V1 and then would be allowed to perform SELECT FOR UPDATE via V2 (since V1 wouldn't be checked for UPDATE permissions). To fix, just switch the order of expanding sub-views and marking referenced objects as needing UPDATE permission. I think additional simplifications are now possible, but that's distinct from the bug fix proper. This is certainly a security issue, but the consequences are pretty minor (just the ability to lock rows that shouldn't be lockable). Against that we have a small risk of breaking applications that are working as-desired, since nested views have behaved this way since such cases worked at all. On balance I'm inclined not to back-patch. Per report from Alexander Lakhin. Discussion: https://postgr.es/m/24db7b8f-3de5-e25f-7ab9-d8848351d42c@gmail.com
-
Tom Lane authored
MaxIndexTuplesPerPage ignores the fact that btree indexes sometimes store tuples with no data payload. But it also ignores the possibility of "special space" on index pages, which offsets that, so that the result isn't an underestimate. This all seems worth documenting, though. In passing, remove #define MinIndexTupleSize, which was added by commit 2c03216d but not used in that commit nor later ones. Comment text by me; issue noticed by Peter Geoghegan. Discussion: https://postgr.es/m/CAH2-WzkQmb54Kbx-YHXstRKXcNc+_87jwV3DRb54xcybLR7Oig@mail.gmail.com
-
Peter Eisentraut authored
As of 0c2c81b4, the replication parameter in libpq is no longer "deliberately undocumented".
-
Peter Eisentraut authored
We need to call expand_function_arguments() to expand named and default arguments. In PL/pgSQL, we also need to deal with named and default INOUT arguments when receiving the output values into variables. Author: Pavel Stehule <pavel.stehule@gmail.com>
-
- 13 Apr, 2018 5 commits
-
-
Andrew Dunstan authored
Commit 16828d5c forgot to check that it had a set of missing values before trying to retrieve a value from it. An additional query to add coverage for this code is added to the regression test. Per bug report from Andreas Seltenreich.
-
Tom Lane authored
In passing, throw an error if the AF count is too small, rather than just silently discarding extra affix entries. Note that the new regression test cases require installing the updated src/backend/tsearch/dicts files. Arthur Zakirov Discussion: https://postgr.es/m/20180413113447.GA32474@zakirov.localdomain
-
Tom Lane authored
We'd throw away the partial result anyway after parsing the error message. Throwing it away beforehand costs nothing and reduces the risk of out-of-memory failure. Also, at least in systems that behave like glibc/Linux, if the partial result was very large then the error PGresult would get allocated at high heap addresses, preventing the heap storage used by the partial result from being released to the OS until the error PGresult is freed. In psql >= 9.6, we hold onto the error PGresult until another error is received (for \errverbose), so that this behavior causes a seeming memory leak to persist for awhile, as in a recent complaint from Darafei Praliaskouski. This is a potential performance regression from older versions, justifying back-patching at least that far. But similar behavior may occur in other client applications, so it seems worth just back-patching to all supported branches. Discussion: https://postgr.es/m/CAC8Q8tJ=7cOkPePyAbJE_Pf691t8nDFhJp0KZxHvnq_uicfyVg@mail.gmail.com
-
Alvaro Herrera authored
This custom opclass was already in use in other tests -- defined independently in every such file. Move the definition to the earliest test that uses it, and keep it around so that later tests can reuse it. Use it in the tests for pruning of hash partitioning, and since this makes the second expected file unnecessary, put those tests back in partition_prune.sql whence they sprang. Author: Amit Langote Discussion: https://postgr.es/m/CA%2BTgmoZ0D5kJbt8eKXtvVdvTcGGWn6ehWCRSZbWytD-uzH92mQ%40mail.gmail.com
-
Alvaro Herrera authored
Environmental conditions might cause parallel workers to be scheduled in different ways in this test, destabilizing the EXPLAIN output. Disable use of workers in an attempt to make output stable. Author: David Rowley Diagnosed-by: Thomas Munro Discussion: https://postgr.es/m/CAKJS1f8j24tUX_nOwACiM=UO5jrMrDz8ca0xbG0vhVgfWph0ZA@mail.gmail.com
-
- 12 Apr, 2018 6 commits
-
-
Tom Lane authored
NISortAffixes() compared successive compound affixes incorrectly, thus possibly failing to merge identical affixes, or (less likely) merging ones that shouldn't be merged. The user-visible effects of this are unclear, to me anyway. Per bug #15150 from Alexander Lakhin. It's been broken for a long time, so back-patch to all supported branches. Arthur Zakirov Discussion: https://postgr.es/m/152353327780.31225.13445405496721177988@wrigleys.postgresql.org
-
Alvaro Herrera authored
I lowered the lock level for partitions being scanned from AccessExclusive to ShareLock in the course of 72cf7f31, but that was bogus, as pointed out by Robert Haas. Revert that bit. Doing this is possible, but requires more work. Discussion: https://postgr.es/m/CA+TgmobV7Nfmqv+TZXcdSsb9Bjc-OL-Anv6BNmCbfJVZLYPE4Q@mail.gmail.com
-
Alvaro Herrera authored
The intention of the test is not immediately obvious, so we need this much.
-
Tom Lane authored
We've made multiple attempts to stabilize the plans shown by commit 1bc0100d, with little success so far. The reason for the remaining instability seems to be that if a transaction (such as auto-analyze) is running concurrently with the test, then get_actual_variable_range may return a maximum value for "T 1"."C 1" that's far away from the actual max, as a result of our having transiently inserted such a value earlier in the test. Because we use a non-MVCC snapshot to fetch the value (for performance reasons), the presence of other transactions can cause that function to return entries that are actually dead. To fix, use a less extreme value in the earlier transient insertion, so that whether it is visible or not won't affect the selectivity estimate. The use of 9999 there seems to have been picked with the aid of a dartboard anyway, rather than having a specific reason. Discussion: https://postgr.es/m/16962.1523551784@sss.pgh.pa.us
-
Alvaro Herrera authored
We were using CurrentMemoryContext to put the partsupfunc fmgr_info into, which isn't right, because we want the PartitionKey as a whole to be in the isolated Relation->rd_partkeycxt context. This can cause a crash with user-defined support functions in the operator classes used by partitioning keys. (Maybe this can cause problems with core-supplied opclasses too, not sure.) This is demonstrably broken in Postgres 10, too, but the initial proposed fix runs afoul of a problem discussed back when 8a0596cb ("Get rid of copy_partition_key") reorganized that code: namely that it is possible to jump out of RelationBuildPartitionKey because of some error and leave a dangling memory context child of CacheMemoryContext. Also, while reviewing this I noticed that the removed-in-pg11 copy_partition_key was doing something wrong, unfixed in pg10, namely doing memcpy() on the FmgrInfo, which is bogus (should be doing fmgr_info_copy). Therefore, in branch pg10, the sane fix seems to be to backpatch both the aforementioned 8a0596cb and its followup be234322 ("Protect against hypothetical memory leaks in RelationGetPartitionKey"), so do that, then apply the fmgr_info memcxt bugfix on top. Add a test case exercising btree-based custom operator classes, which causes a crash prior to this fix. This is not a security problem, because in order to create an operator class you need superuser privileges anyway. Authors: Álvaro Herrera and Amit Langote Reported and diagnosed by: Amit Langote Discussion: https://postgr.es/m/3041e853-b1dd-a0c6-ff21-7cc5633bffd0@lab.ntt.co.jp
-
Tom Lane authored
We have to ensure that submake-generated-headers is finished before the topmost make run launches any child makes. Discussion: https://postgr.es/m/20180411235843.GG32449@paquier.xyz
-