- 29 Jan, 2018 7 commits
-
-
Andres Freund authored
It's a common task to evaluate a qual and reset the corresponding expression context. Currently that requires storing the result of the qual eval, resetting the context, and then reacting on the result. As that's awkward several places only reset the context next time through a node. That's not great, so introduce a helper that evaluates and resets. It's a bit ugly that it currently uses MemoryContextReset() instead of ResetExprContext(), but that seems easier than reordering all of executor.h. Author: Andres Freund Discussion: https://postgr.es/m/20180109222544.f7loxrunqh3xjl5f@alap3.anarazel.de
-
Tom Lane authored
There's never any value in giving a fully specified cache key to SearchCatCacheList: you might as well call SearchCatCache instead, since there could be only one match. So the maximum useful number of key arguments is one less than the supported number of key columns. We might as well remove the useless extra argument and save some few bytes per call site, as well as a cycle or so per call. I believe the reason it was coded like this is that originally, callers had to write out all the dummy arguments in each call, and so it seemed less confusing if SearchCatCache and SearchCatCacheList took the same number of key arguments. But since commit e26c539e, callers only write their live arguments explicitly, making that a non-factor; and there's surely been enough time for third-party modules to adapt to that coding style. So this is only an ABI break not an API break for callers. Per discussion with Oliver Ford, this might also make it less confusing how to use SearchCatCacheList correctly. Discussion: https://postgr.es/m/27788.1517069693@sss.pgh.pa.us
-
Andres Freund authored
ExecPushExprSlots didn't initialize ExprEvalStep's resvalue/resnull steps as it didn't use them. That caused wrong valgrind warnings for an upcoming patch, so zero-intialize. Also zero-initialize all scratch ExprEvalStep's allocated on the stack, to avoid issues with similar future omissions of non-critial data.
-
Peter Eisentraut authored
Clarify that the restriction against reg* types only applies to table columns using these types, not to the type appearing in any other way, for example as a function argument.
-
Andres Freund authored
In cases where simplehash tables where filled with either a lot of conflicting hash-values, or values that hash to consecutive values (i.e. build "chains") the growth heuristics in d4c62a6b could trigger rather explosively. To fix that, address some of the reasons (see previous commit) of why the growth heuristics where needed, and only allow growth when the table isn't too empty. While that means there's a few cases of bad input that can be slower, that seems a lot better than running very quickly out of memory. Author: Tomas Vondra and Andres Freund, with additional input by Thomas Munro, Tom Lane Todd A. Cook Reported-By: Todd A. Cook, Tomas Vondra, Thomas Munro Discussion: https://postgr.es/m/20171127185700.1470.20362@wrigleys.postgresql.org Backpatch: 10, where simplehash was introduced
-
Andres Freund authored
The changes in b81b5a96 did not fully address the issue, because the bit-mixing of the IV into the final hash-key didn't prevent clustering in the input-data survive in the output data. This didn't cause a lot of problems because of the additional growth conditions added d4c62a6b. But as we want to rein those in due to explosive growth in some edges, this needs to be fixed. Author: Andres Freund Discussion: https://postgr.es/m/20171127185700.1470.20362@wrigleys.postgresql.org Backpatch: 10, where simplehash was introduced
-
Tom Lane authored
When a password is needed, cases such as psql -d "postgresql://alice@localhost/testdb" -U bob would incorrectly prompt for "Password for user bob: ", when actually the connection will be attempted with username alice. The priority order of which name to use isn't that important here, but the misleading prompt is. When we are prompting for a password after initial connection failure, we can fix this reliably by looking at PQuser(conn) to see how libpq interpreted the connection arguments. But when we're doing a forced password prompt because of a -W switch, we can't use that solution. Fortunately, because the main use of -W is for noninteractive situations, it's less critical to produce a helpful prompt in such cases. I made the startup prompt for -W just say "Password: " all the time, rather than expending extra code on trying to identify which username to use. In the case of a \c command (after -W has been given), there's already logic in do_connect that determines whether the "dbname" is a connstring or URI, so we can avoid lobotomizing the prompt except in cases that are actually dubious. (We could do similarly in startup.c if anyone complains, but for now it seems not worthwhile, especially since that would still be only a partial solution.) Per bug #15025 from Akos Vandra. Although this is arguably a bug fix, it doesn't seem worth back-patching. The case where it matters seems like a very corner-case usage, and someone might complain that we'd changed the behavior of -W in a minor release. Discussion: https://postgr.es/m/20180123130013.7407.24749@wrigleys.postgresql.org
-
- 28 Jan, 2018 2 commits
-
-
Tom Lane authored
create_plan_recurse lacked any stack depth check. This is not per our normal coding rules, but I'd supposed it was safe because earlier planner processing is more complex and presumably should eat more stack. But bug #15033 from Andrew Grossman shows this isn't true, at least not for queries having the form of a many-thousand-way INTERSECT stack. Further testing showed that recurse_set_operations is also capable of being crashed in this way, since it likewise will recurse to the bottom of a parsetree before calling any support functions that might themselves contain any stack checks. However, its stack consumption is only perhaps a third of create_plan_recurse's. It's possible that this particular problem with create_plan_recurse can only manifest in 9.6 and later, since before that we didn't build a Path tree for set operations. But having seen this example, I now have no faith in the proposition that create_plan_recurse doesn't need a stack check, so back-patch to all supported branches. Discussion: https://postgr.es/m/20180127050845.28812.58244@wrigleys.postgresql.org
-
Bruce Momjian authored
Discussion: https://postgr.es/m/5A69AA50.2060600@lab.ntt.co.jp Author: Etsuro Fujita
-
- 27 Jan, 2018 3 commits
-
-
Tom Lane authored
DST law changes in Brazil, Sao Tome and Principe. Historical corrections for Bolivia, Japan, and South Sudan. The "US/Pacific-New" zone has been removed (it was only a link to America/Los_Angeles anyway).
-
Tom Lane authored
Commit 09529a70 changed nodeIndexscan.c and nodeIndexonlyscan.c to postpone initialization of the indexscan proper until the first tuple fetch. It overlooked the question of mark/restore behavior, which means that if some caller attempts to mark the scan before the first tuple fetch, you get a null pointer dereference. The only existing user of mark/restore is nodeMergejoin.c, which (somewhat accidentally) will never attempt to set a mark before the first inner tuple unless the inner child node is a Material node. Hence the case can't arise normally, so it seems sufficient to document the assumption at both ends. However, during an EvalPlanQual recheck, ExecScanFetch doesn't call IndexNext but just returns the jammed-in test tuple. Therefore, if we're doing a recheck in a plan tree with a mergejoin with inner indexscan, it's possible to reach ExecIndexMarkPos with iss_ScanDesc still null, as reported by Guo Xiang Tan in bug #15032. Really, when there's a test tuple supplied during an EPQ recheck, touching the index at all is the wrong thing: rather, the behavior of mark/restore ought to amount to saving and restoring the es_epqScanDone flag. We can avoid finding a place to actually save the flag, for the moment, because given the assumption that no caller will set a mark before fetching a tuple, es_epqScanDone must always be set by the time we try to mark. So the actual behavior change required is just to not reach the index access if a test tuple is supplied. The set of plan node types that need to consider this issue are those that support EPQ test tuples (i.e., call ExecScan()) and also support mark/restore; which is to say, IndexScan, IndexOnlyScan, and perhaps CustomScan. It's tempting to try to fix the problem in one place by teaching ExecMarkPos() itself about EPQ; but ExecMarkPos supports some plan types that aren't Scans, and also it seems risky to make assumptions about what a CustomScan wants to do here. Also, the most likely future change here is to decide that we do need to support marks placed before the first tuple, which would require additional work in IndexScan and IndexOnlyScan in any case. Hence, fix the EPQ issue in nodeIndexscan.c and nodeIndexonlyscan.c, accepting the small amount of code duplicated thereby, and leave it to CustomScan providers to fix this bug if they have it. Back-patch to v10 where commit 09529a70 came in. In earlier branches, the index_markpos() call is a waste of cycles when EPQ is active, but no more than that, so it doesn't seem appropriate to back-patch further. Discussion: https://postgr.es/m/20180126074932.3098.97815@wrigleys.postgresql.org
-
Magnus Hagander authored
Author: Daniel Gustafsson <daniel@yesql.se>
-
- 26 Jan, 2018 7 commits
-
-
Tom Lane authored
We have a lot of code in which option names, which from the user's viewpoint are logically keywords, are passed through the grammar as plain identifiers, and then matched to string literals during command execution. This approach avoids making words into lexer keywords unnecessarily. Some places matched these strings using plain strcmp, some using pg_strcasecmp. But the latter should be unnecessary since identifiers would have been downcased on their way through the parser. Aside from any efficiency concerns (probably not a big factor), the lack of consistency in this area creates a hazard of subtle bugs due to different places coming to different conclusions about whether two option names are the same or different. Hence, standardize on using strcmp() to match any option names that are expected to have been fed through the parser. This does create a user-visible behavioral change, which is that while formerly all of these would work: alter table foo set (fillfactor = 50); alter table foo set (FillFactor = 50); alter table foo set ("fillfactor" = 50); alter table foo set ("FillFactor" = 50); now the last case will fail because that double-quoted identifier is different from the others. However, none of our documentation says that you can use a quoted identifier in such contexts at all, and we should discourage doing so since it would break if we ever decide to parse such constructs as true lexer keywords rather than poor man's substitutes. So this shouldn't create a significant compatibility issue for users. Daniel Gustafsson, reviewed by Michael Paquier, small changes by me Discussion: https://postgr.es/m/29405B24-564E-476B-98C0-677A29805B84@yesql.se
-
Robert Haas authored
This is preparatory refactoring to prepare the way for partition-wise aggregate, which will reuse the new subroutines for child grouping rels. It also does not seem like a bad idea on general principle, as the function was getting pretty long. Jeevan Chalke. The larger patch series of which this patch is a part was reviewed and tested by Antonin Houska, Rajkumar Raghuwanshi, Ashutosh Bapat, David Rowley, Dilip Kumar, Konstantin Knizhnik, Pascal Legrand, and me. Some cosmetic changes by me. Discussion: http://postgr.es/m/CAM2+6=V64_xhstVHie0Rz=KPEQnLJMZt_e314P0jaT_oJ9MR8A@mail.gmail.com
-
Tom Lane authored
This clause was superseded by SQL-standard syntax back in 7.3. We've kept it around for backwards-compatibility purposes ever since; but 15 years seems like long enough for that, especially seeing that there are undocumented weirdnesses in how it interacts with the SQL-standard syntax for specifying the same options. Michael Paquier, per an observation by Daniel Gustafsson; some small cosmetic adjustments to nearby code by me. Discussion: https://postgr.es/m/20180115022748.GB1724@paquier.xyz
-
Robert Haas authored
This can cause it to produce incorrect output. Report and patch by Masahiko Sawada. Discussion: http://postgr.es/m/CAD21AoBc5Asx7pXdUWu6NqU_g=Ysn95EGL9SMeYhLLduYoO_OA@mail.gmail.com
-
Peter Eisentraut authored
The existing "connection authorized" server log messages used OpenSSL API calls directly, even though similar abstracted API calls exist. Change to use the latter instead. Change the function prototype for the functions that return the TLS version and the cipher to return const char * directly instead of copying into a buffer. That makes them slightly easier to use. Add bits= to the message. psql shows that, so we might as well show the same information on the client and server. Reviewed-by: Daniel Gustafsson <daniel@yesql.se> Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
-
Peter Eisentraut authored
As the comment there stated, these were needed for old-style user-defined functions, but since we removed support for those, we don't need this anymore. Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
-
Bruce Momjian authored
Reported-by: Masahiko Sawada Discussion: https://postgr.es/m/CAD21AoBgnHy2YKAUuB6iVG4ibvLYepHr+RDRkr1arqWwc1AHCw@mail.gmail.com Author: Masahiko Sawada
-
- 25 Jan, 2018 8 commits
-
-
Tom Lane authored
We have switches already to suppress other subsidiary object properties, such as ACLs, security labels, ownership, and tablespaces, so just on the grounds of symmetry we should allow suppressing comments as well. Also, commit 0d4e6ed3 added a positive reason to have this feature, i.e. to allow obtaining the old behavior of selective pg_restore should anyone desire that. Recent commits have removed the cases where pg_dump emitted comments on built-in objects that the restoring user might not have privileges to comment on, so the original primary motivation for this feature is gone, but it still seems at least somewhat useful in its own right. Robins Tharakan, reviewed by Fabrízio Mello Discussion: https://postgr.es/m/CAEP4nAx22Z4ch74oJGzr5RyyjcyUSbpiFLyeYXX8pehfou92ug@mail.gmail.com
-
Tom Lane authored
Per buildfarm.
-
Tom Lane authored
Ensure that CREATE DATABASE and related commands are issued when, and only when, --create is specified. Previously there were scenarios where using selective-dump switches would prevent --create from having any effect. For example, it would fail to do anything in pg_restore if the archive file had been made by a selective dump, because there would be no TOC entry for the database. Since we don't issue \connect either if we don't issue CREATE DATABASE, this could result in unexpectedly restoring objects into the wrong database. Also fix pg_restore's selective restore logic so that when an object is selected to be restored, we also restore its ACL, comment, and security label if any. Previously there was no way to get the latter properties except through tedious mucking about with a -L file. If, for some reason, you don't want these properties, you can match the old behavior by adding --no-acl etc. While at it, try to make _tocEntryRequired() a little better organized and better documented. Discussion: https://postgr.es/m/32668.1516848577@sss.pgh.pa.us
-
Alvaro Herrera authored
get_relation_info() was too optimistic about opening indexes in partitioned tables, which would raise errors when any queries were planned on such tables. Fix by ignoring any indexes of the partitioned kind. CLUSTER (and ALTER TABLE CLUSTER ON) had a similar problem. Fix by disallowing these commands in partitioned tables. Fallout from 8b08f7d4.
-
Tom Lane authored
We had some pretty ad-hoc handling of the public schema and the plpgsql extension, which are both presumed to exist in template0 but might be modified or deleted in the database being dumped. Up to now, by default pg_dump would emit a CREATE EXTENSION IF NOT EXISTS command as well as a COMMENT command for plpgsql. The usefulness of the former is questionable, and the latter caused annoying errors in non-superuser dump/restore scenarios. Let's instead install a rule that built-in extensions (identified by having low-numbered OIDs) are not to be dumped. We were doing it that way already in binary-upgrade mode, so this just makes regular mode behave the same. It remains true that if someone has installed a non-default ACL on the plpgsql language, that will get dumped thanks to the pg_init_privs mechanism. This is more consistent with the handling of built-in objects of other kinds. Also, change the very ad-hoc mechanism that was used to avoid dumping creation and comment commands for the public schema. Instead of hardwiring a test in _printTocEntry(), make use of the DUMP_COMPONENT_ infrastructure to mark that schema up-front about what we want to do with it. This has the visible effect that the public schema won't be mentioned in the output at all, except for updating its ACL if it has a non-default ACL. Previously, while it was normally not mentioned, --clean mode would drop and recreate it, again causing headaches for non-superuser usage. This change likewise makes the public schema less special and more like other built-in objects. If plpgsql, or the public schema, has been removed entirely in the source DB, that situation won't be reproduced in the destination ... but that was true before. Discussion: https://postgr.es/m/29048.1516812451@sss.pgh.pa.us
-
Peter Eisentraut authored
Previously, the docs implied that only Linux and Windows could use huge pages. That's not quite true: it's just that we only know how to request them explicitly on those OSes. Be more explicit about what huge_pages really does and mention that some OSes may use huge pages automatically. Author: Thomas Munro and Catalin Iacob Reviewed-By: Justin Pryzby, Peter Eisentraut Discussion: https://postgr.es/m/CAEepm=3qzR-hfjepymohuC4XO5phxoSoipOjm6BEhnJHjNR+jg@mail.gmail.com
-
Peter Eisentraut authored
These were introduced in 4cbb6463, but after further analysis and testing, they should not be necessary and probably weren't the part of that commit that fixed anything. Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
-
Peter Eisentraut authored
Connection strings can have items with spaces in them, wrapped in quotes. The tests however ran a SELECT '$connstr' upon connection which broke on the embedded quotes. Use dollar quotes on the connstr to protect against this. This was hit during the development of the macOS Secure Transport patch, but is independent of it. Author: Daniel Gustafsson <daniel@yesql.se>
-
- 24 Jan, 2018 5 commits
-
-
Robert Haas authored
Report by buildfarm member skink and Tom Lane. Analysis by me. Patch by Amit Khandekar. Discussion: http://postgr.es/m/CAJ3gD9fVA1iXQYhfqHP5n_TEd4U9=V8TL_cc-oKRnRmxgdvJrQ@mail.gmail.com
-
Bruce Momjian authored
This was done to match the surrounding indentation. Text added in PG 10. Backpatch-through: 10
-
Peter Eisentraut authored
record_image_eq was covered a bit by the materialized view code that it is meant to support, but record_image_cmp was not tested at all. While we're here, add more tests to record_eq and record_cmp as well, for symmetry. Reviewed-by: Michael Paquier <michael.paquier@gmail.com>
-
Bruce Momjian authored
Document likely use of RegisterDynamicBackgroundWorker by another background worker. Reported-by: Chapman Flack Discussion: https://postgr.es/m/CAB7nPqTdi=J9HH8PPPiEOohebdd+xkgbbhdY7=VbGnZ3CkZXxA@mail.gmail.com Author: Chapman Flack
-
Tom Lane authored
Avoid compiler warnings on MSVC (which doesn't want to see both __forceinline and inline) and ancient GCC (which doesn't have __attribute__((always_inline))). Don't force inline-ing when building at -O0, as the programmer is probably hoping for exact source-to-object-line correspondence in that case. (For the moment this only works for GCC; maybe we can extend it later.) Make pg_attribute_always_inline be syntactically a drop-in replacement for inline, rather than an additional wart. And improve the comments. Thomas Munro and Michail Nikolaev, small tweaks by me Discussion: https://postgr.es/m/32278.1514863068@sss.pgh.pa.us Discussion: https://postgr.es/m/CANtu0oiYp74brgntKOxgg1FK5+t8uQ05guSiFU6FYz_5KUhr6Q@mail.gmail.com
-
- 23 Jan, 2018 8 commits
-
-
Bruce Momjian authored
Reported-by: Mark Wood Bug: 14912 Discussion: https://postgr.es/m/20171116171735.1474.30450@wrigleys.postgresql.org Author: David G. Johnston Backpatch-through: 10
-
Tom Lane authored
If we're inside a lateral subquery, there may be no unparameterized paths for a particular child relation of an appendrel, in which case we *must* be able to create similarly-parameterized paths for each other child relation, else the planner will fail with "could not devise a query plan for the given query". This means that there are situations where we'd better be able to reparameterize at least one path for each child. This calls into question the assumption in reparameterize_path() that it can just punt if it feels like it. However, the only case that is known broken right now is where the child is itself an appendrel so that all its paths are AppendPaths. (I think possibly I disregarded that in the original coding on the theory that nested appendrels would get folded together --- but that only happens *after* reparameterize_path(), so it's not excused from handling a child AppendPath.) Given that this code's been like this since 9.3 when LATERAL was introduced, it seems likely we'd have heard of other cases by now if there were a larger problem. Per report from Elvis Pranskevichus. Back-patch to 9.3. Discussion: https://postgr.es/m/5981018.zdth1YWmNy@hammer.magicstack.net
-
Alvaro Herrera authored
autovacuum.c no longer needs dsa.h, since commit 31ae1638. Author: Masahiko Sawada Discussion: https://postgr.es/m/CAD21AoCWvYyXrvdANSHWWWEWJH5TeAWAkJ_2gqrHhukG+OBo1g@mail.gmail.com
-
Tom Lane authored
Overlooked in commit f13ea95f. Noted by Nick Barnes. Discussion: https://postgr.es/m/20180123093723.7407.3386@wrigleys.postgresql.org
-
Peter Eisentraut authored
Author: Fabien COELHO <coelho@cri.ensmp.fr>
-
Robert Haas authored
Since 9.6, heavyweight locking is not an abstract and unhandled concern of the parallel machinery, but rather something to which we have a specific approach.
-
Robert Haas authored
Commit 28724fd9 fixed things so that if a background worker fails to start due to fork() failure or because it is terminated before startup succeeds, BGWH_STOPPED will be reported. However, that only helps if the code that uses the background worker machinery notices the change in status, and the code in parallel.c did not. To fix that, do two things. First, make sure that when a worker exits, it triggers the leader to read from error queues. That way, if a worker which has attached to an error queue exits uncleanly, the leader is sure to throw some error, either the contents of the ErrorResponse sent by the worker, or "lost connection to parallel worker" if it exited without sending one. To cover the case where the worker never starts up in the first place or exits before attaching to the error queue, the ParallelContext now keeps track of which workers have sent at least one message via the error queue. A worker which sends no messages by the time the parallel operation finishes will be checked to see whether it exited before attaching to the error queue; if so, a new error message, "parallel worker failed to initialize", will be reported. If not, we'll continue to wait until it either starts up and exits cleanly, starts up and exits uncleanly, or fails to start, and then take the appropriate action. Patch by me, reviewed by Amit Kapila. Discussion: http://postgr.es/m/CA+TgmoYnBgXgdTu6wk5YPdWhmgabYc9nY_pFLq=tB=FSLYkD8Q@mail.gmail.com
-
Tom Lane authored
The folly of not doing this was exposed by the buildfarm: in some cases, the GUC settings applied through ALTER DATABASE SET may be essential to interpreting the reloaded data correctly. Another argument why we can't really get away with the scheme proposed in commit b3f84012 is that it cannot work for parallel restore: even if the parent process manages to hang onto the previous GUC state, worker processes would see the state post-ALTER-DATABASE. (Perhaps we could have dodged that bullet by delaying DATABASE PROPERTIES restoration to the end of the run, but that does nothing for the data semantics problem.) This leaves us with no solution for the default_transaction_read_only issue that commit 4bd371f6 intended to work around, other than "you gotta remove such settings before dumping/upgrading". However, in view of the fact that parallel restore broke that hack years ago and no one has noticed, it's fair to question how many people care. I'm unexcited about adding a large dollop of new complexity to handle that corner case. This would be a one-liner fix, except it turns out that ReconnectToServer tries to optimize away "redundant" reconnections. While that may have been valuable when coded, a quick survey of current callers shows that there are no cases where that's actually useful, so just remove that check. While at it, remove the function's useless return value. Discussion: https://postgr.es/m/12453.1516655001@sss.pgh.pa.us
-