summaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2015-10-01Fix documentation error in commit 8703059c6b55c427100e00a09f66534b6ccbfaa1.Tom Lane
Etsuro Fujita spotted a thinko in the README commentary.
2015-09-30Improve LISTEN startup time when there are many unread notifications.Tom Lane
If some existing listener is far behind, incoming new listener sessions would start from that session's read pointer and then need to advance over many already-committed notification messages, which they have no interest in. This was expensive in itself and also thrashed the pg_notify SLRU buffers a lot more than necessary. We can improve matters considerably in typical scenarios, without much added cost, by starting from the furthest-ahead read pointer, not the furthest-behind one. We do have to consider only sessions in our own database when doing this, which requires an extra field in the data structure, but that's a pretty small cost. Back-patch to 9.0 where the current LISTEN/NOTIFY logic was introduced. Matt Newell, slightly adjusted by me
2015-09-29Fix plperl to handle non-ASCII error message texts correctly.Tom Lane
We were passing error message texts to croak() verbatim, which turns out not to work if the text contains non-ASCII characters; Perl mangles their encoding, as reported in bug #13638 from Michal Leinweber. To fix, convert the text into a UTF8-encoded SV first. It's hard to test this without risking failures in different database encodings; but we can follow the lead of plpython, which is already assuming that no-break space (U+00A0) has an equivalent in all encodings we care about running the regression tests in (cf commit 2dfa15de5). Back-patch to 9.1. The code is quite different in 9.0, and anyway it seems too risky to put something like this into 9.0's final minor release. Alex Hunsaker, with suggestions from Tim Bunce and Tom Lane
2015-09-28Fix compiler warning about unused function in non-readline case.Andrew Dunstan
Backpatch to all live branches to keep the code in sync.
2015-09-25Second try at fixing O(N^2) problem in foreign key references.Tom Lane
This replaces ill-fated commit 5ddc72887a012f6a8b85707ef27d85c274faf53d, which was reverted because it broke active uses of FK cache entries. In this patch, we still do nothing more to invalidatable cache entries than mark them as needing revalidation, so we won't break active uses. To keep down the overhead of InvalidateConstraintCacheCallBack(), keep a list of just the currently-valid cache entries. (The entries are large enough that some added space for list links doesn't seem like a big problem.) This would still be O(N^2) when there are many valid entries, though, so when the list gets too long, just force the "sinval reset" behavior to remove everything from the list. I set the threshold at 1000 entries, somewhat arbitrarily. Possibly that could be fine-tuned later. Another item for future study is whether it's worth adding reference counting so that we could safely remove invalidated entries. As-is, problem cases are likely to end up with large and mostly invalid FK caches. Like the previous attempt, backpatch to 9.3. Jan Wieck and Tom Lane
2015-09-25Further fix for psql's code for locale-aware formatting of numeric output.Tom Lane
(Third time's the charm, I hope.) Additional testing disclosed that this code could mangle already-localized output from the "money" datatype. We can't very easily skip applying it to "money" values, because the logic is tied to column right-justification and people expect "money" output to be right-justified. Short of decoupling that, we can fix it in what should be a safe enough way by testing to make sure the string doesn't contain any characters that would not be expected in plain numeric output.
2015-09-25Further fix for psql's code for locale-aware formatting of numeric output.Tom Lane
On closer inspection, those seemingly redundant atoi() calls were not so much inefficient as just plain wrong: the author of this code either had not read, or had not understood, the POSIX specification for localeconv(). The grouping field is *not* a textual digit string but separate integers encoded as chars. We'll follow the existing code as well as the backend's cash.c in only honoring the first group width, but let's at least honor it correctly. This doesn't actually result in any behavioral change in any of the locales I have installed on my Linux box, which may explain why nobody's complained; grouping width 3 is close enough to universal that it's barely worth considering other cases. Still, wrong is wrong, so back-patch.
2015-09-24Fix psql's code for locale-aware formatting of numeric output.Tom Lane
This code did the wrong thing entirely for numbers with an exponent but no decimal point (e.g., '1e6'), as reported by Jeff Janes in bug #13636. More generally, it made lots of unverified assumptions about what the input string could possibly look like. Rearrange so that it only fools with leading digits that it's directly verified are there, and an immediately adjacent decimal point. While at it, get rid of some useless inefficiencies, like converting the grouping count string to integer over and over (and over). This has been broken for a long time, so back-patch to all supported branches.
2015-09-24Lower *_freeze_max_age minimum values.Andres Freund
The old minimum values are rather large, making it time consuming to test related behaviour. Additionally the current limits, especially for multixacts, can be problematic in space-constrained systems. 10000000 multixacts can contain a lot of members. Since there's no good reason for the current limits, lower them a good bit. Setting them to 0 would be a bad idea, triggering endless vacuums, so still retain a limit. While at it fix autovacuum_multixact_freeze_max_age to refer to multixact.c instead of varsup.c. Reviewed-By: Robert Haas Discussion: CA+TgmoYmQPHcrc3GSs7vwvrbTkbcGD9Gik=OztbDGGrovkkEzQ@mail.gmail.com Backpatch: back to 9.0 (in parts)
2015-09-21Fix possible internal overflow in numeric multiplication.Tom Lane
mul_var() postpones propagating carries until it risks overflow in its internal digit array. However, the logic failed to account for the possibility of overflow in the carry propagation step, allowing wrong results to be generated in corner cases. We must slightly reduce the when-to-propagate-carries threshold to avoid that. Discovered and fixed by Dean Rasheed, with small adjustments by me. This has been wrong since commit d72f6c75038d8d37e64a29a04b911f728044d83b, so back-patch to all supported branches.
2015-09-20Restrict file mode creation mask during tmpfile().Noah Misch
Per Coverity. Back-patch to 9.0 (all supported versions). Michael Paquier, reviewed (in earlier versions) by Heikki Linnakangas.
2015-09-20Be more wary about partially-valid LOCALLOCK data in RemoveLocalLock().Tom Lane
RemoveLocalLock() must consider the possibility that LockAcquireExtended() failed to palloc the initial space for a locallock's lockOwners array. I had evidently meant to cope with this hazard when the code was originally written (commit 1785acebf2ed14fd66955e2d9a55d77a025f418d), but missed that the pfree needed to be protected with an if-test. Just to make sure things are left in a clean state, reset numLockOwners as well. Per low-memory testing by Andreas Seltenreich. Back-patch to all supported branches.
2015-09-19Let compiler handle size calculation of bool types.Michael Meskes
Back in the day this did not work, but modern compilers should handle it themselves.
2015-09-18Fix low-probability memory leak in regex execution.Tom Lane
After an internal failure in shortest() or longest() while pinning down the exact location of a match, find() forgot to free the DFA structure before returning. This is pretty unlikely to occur, since we just successfully ran the "search" variant of the DFA; but it could happen, and it would result in a session-lifespan memory leak since this code uses malloc() directly. Problem seems to have been aboriginal in Spencer's library, so back-patch all the way. In passing, correct a thinko in a comment I added awhile back about the meaning of the "ntree" field. I happened across these issues while comparing our code to Tcl's version of the library.
2015-09-15Revert "Fix an O(N^2) problem in foreign key references".Tom Lane
Commit 5ddc72887a012f6a8b85707ef27d85c274faf53d does not actually work because it will happily blow away ri_constraint_cache entries that are in active use in outer call levels. In any case, it's a very ugly, brute-force solution to the problem of limiting the cache size. Revert until it can be redesigned.
2015-09-11pg_dump, pg_upgrade: allow postgres/template1 tablespace movesBruce Momjian
Modify pg_dump to restore postgres/template1 databases to non-default tablespaces by switching out of the database to be moved, then switching back. Also, to fix potentially cases where the old/new tablespaces might not match, fix pg_upgrade to process new/old tablespaces separately in all cases. Report by Marti Raudsepp Patch by Marti Raudsepp, me Backpatch through 9.0
2015-09-11Fix an O(N^2) problem in foreign key references.Kevin Grittner
Commit 45ba424f improved foreign key lookups during bulk updates when the FK value does not change. When restoring a schema dump from a database with many (say 100,000) foreign keys, this cache would grow very big and every ALTER TABLE command was causing an InvalidateConstraintCacheCallBack(), which uses a sequential hash table scan. This could cause a severe performance regression in restoring a schema dump (including during pg_upgrade). The patch uses a heuristic method of detecting when the hash table should be destroyed and recreated. InvalidateConstraintCacheCallBack() adds the current size of the hash table to a counter. When that sum reaches 1,000,000, the hash table is flushed. This fixes the regression without noticeable harm to the bulk update use case. Jan Wieck Backpatch to 9.3 where the performance regression was introduced.
2015-09-10Fix setrefs.c comment properly.Tom Lane
The "typo" alleged in commit 1e460d4bd was actually a comment that was correct when written, but I missed updating it in commit b5282aa89. Use a slightly less specific (and hopefully more future-proof) description of what is collected. Back-patch to 9.2 where that commit appeared, and revert the comment to its then-entirely-correct state before that.
2015-09-10Fix typo in setrefs.cStephen Frost
We're adding OIDs, not TIDs, to invalItems. Pointed out by Etsuro Fujita. Back-patch to all supported branches.
2015-09-09Fix minor bug in regexp makesearch() function.Tom Lane
The list-wrangling here was done wrong, allowing the same state to get put into the list twice. The following loop then would clone it twice. The second clone would wind up with no inarcs, so that there was no observable misbehavior AFAICT, but a useless state in the finished NFA isn't an especially good thing.
2015-09-09Remove files signaling a standby promotion request at postmaster startupFujii Masao
This commit makes postmaster forcibly remove the files signaling a standby promotion request. Otherwise, the existence of those files can trigger a promotion too early, whether a user wants that or not. This removal of files is usually unnecessary because they can exist only during a few moments during a standby promotion. However there is a race condition: if pg_ctl promote is executed and creates the files during a promotion, the files can stay around even after the server is brought up to new master. Then, if new standby starts by using the backup taken from that master, the files can exist at the server startup and should be removed in order to avoid an unexpected promotion. Back-patch to 9.1 where promote signal file was introduced. Problem reported by Feike Steenbergen. Original patch by Michael Paquier, modified by me. Discussion: 20150528100705.4686.91426@wrigleys.postgresql.org
2015-09-08Lock all relations referred to in updatable viewsStephen Frost
Even views considered "simple" enough to be automatically updatable may have mulitple relations involved (eg: in a where clause). We need to make sure and lock those relations when rewriting the query. Back-patch to 9.3 where updatable views were added. Pointed out by Andres, patch thanks to Dean Rasheed.
2015-09-09Add gin_fuzzy_search_limit to postgresql.conf.sample.Fujii Masao
This was forgotten in 8a3631f (commit that originally added the parameter) and 0ca9907 (commit that added the documentation later that year). Back-patch to all supported versions.
2015-09-07Change type of DOW/DOY to UNITSGreg Stark
2015-09-07Make GIN's cleanup pending list process interruptableTeodor Sigaev
Cleanup process could be called by ordinary insert/update and could take a lot of time. Add vacuum_delay_point() to make this process interruptable. Under vacuum this call will also throttle a vacuum process to decrease system load, called from insert/update it will not throttle, and that reduces a latency. Backpatch for all supported branches. Jeff Janes <jeff.janes@gmail.com>
2015-09-06Move DTK_ISODOW DTK_DOW and DTK_DOY to be type UNITS rather thanGreg Stark
RESERV. RESERV is meant for tokens like "now" and having them in that category throws errors like these when used as an input date: stark=# SELECT 'doy'::timestamptz; ERROR: unexpected dtype 33 while parsing timestamptz "doy" LINE 1: SELECT 'doy'::timestamptz; ^ stark=# SELECT 'dow'::timestamptz; ERROR: unexpected dtype 32 while parsing timestamptz "dow" LINE 1: SELECT 'dow'::timestamptz; ^ Found by LLVM's Libfuzzer
2015-09-05Fix CreateTableSpace() so it will compile without HAVE_SYMLINK.Tom Lane
This has been broken since 9.3 (commit 82b1b213cad3a69c to be exact), which suggests that nobody is any longer using a Windows build system that doesn't provide a symlink emulation. Still, it's wrong on its own terms, so repair. YUriy Zhuravlev
2015-09-05Fix misc typos.Heikki Linnakangas
Oskari Saarenmaa. Backpatch to stable branches where applicable.
2015-09-04Fix subtransaction cleanup after an outer-subtransaction portal fails.Tom Lane
Formerly, we treated only portals created in the current subtransaction as having failed during subtransaction abort. However, if the error occurred while running a portal created in an outer subtransaction (ie, a cursor declared before the last savepoint), that has to be considered broken too. To allow reliable detection of which ones those are, add a bookkeeping field to struct Portal that tracks the innermost subtransaction in which each portal has actually been executed. (Without this, we'd end up failing portals containing functions that had called the subtransaction, thereby breaking plpgsql exception blocks completely.) In addition, when we fail an outer-subtransaction Portal, transfer its resources into the subtransaction's resource owner, so that they're released early in cleanup of the subxact. This fixes a problem reported by Jim Nasby in which a function executed in an outer-subtransaction cursor could cause an Assert failure or crash by referencing a relation created within the inner subtransaction. The proximate cause of the Assert failure is that AtEOSubXact_RelationCache assumed it could blow away a relcache entry without first checking that the entry had zero refcount. That was a bad idea on its own terms, so add such a check there, and to the similar coding in AtEOXact_RelationCache. This provides an independent safety measure in case there are still ways to provoke the situation despite the Portal-level changes. This has been broken since subtransactions were invented, so back-patch to all supported branches. Tom Lane and Michael Paquier
2015-08-31psql: print longtable as a possible \pset optionBruce Momjian
For some reason this message was not updated when the longtable option was added. Backpatch through 9.3
2015-08-29Fix s_lock.h PPC assembly code to be compatible with native AIX assembler.Tom Lane
On recent AIX it's necessary to configure gcc to use the native assembler (because the GNU assembler hasn't been updated to handle AIX 6+). This caused PG builds to fail with assembler syntax errors, because we'd try to compile s_lock.h's gcc asm fragment for PPC, and that assembly code relied on GNU-style local labels. We can't substitute normal labels because it would fail in any file containing more than one inlined use of tas(). Fortunately, that code is stable enough, and the PPC ISA is simple enough, that it doesn't seem like too much of a maintenance burden to just hand-code the branch offsets, removing the need for any labels. Note that the AIX assembler only accepts "$" for the location counter pseudo-symbol. The usual GNU convention is "."; but it appears that all versions of gas for PPC also accept "$", so in theory this patch will not break any other PPC platforms. This has been reported by a few people, but Steve Underwood gets the credit for being the first to pursue the problem far enough to understand why it was failing. Thanks also to Noah Misch for additional testing.
2015-08-21Avoid O(N^2) behavior when enlarging SPI tuple table in spi_printtup().Tom Lane
For no obvious reason, spi_printtup() was coded to enlarge the tuple pointer table by just 256 slots at a time, rather than doubling the size at each reallocation, as is our usual habit. For very large SPI results, this makes for O(N^2) time spent in repalloc(), which of course soon comes to dominate the runtime. Use the standard doubling approach instead. This is a longstanding performance bug, so back-patch to all active branches. Neil Conway
2015-08-21Fix plpython crash when returning string representation of a RECORD result.Tom Lane
PLyString_ToComposite() blithely overwrote proc->result.out.d, even though for a composite result type the other union variant proc->result.out.r is the one that should be valid. This could result in a crash if out.r had in fact been filled in (proc->result.is_rowtype == 1) and then somebody later attempted to use that data; as per bug #13579 from Paweł Michalak. Just to add insult to injury, it didn't work for RECORD results anyway, because record_in() would refuse the case. Fix by doing the I/O function lookup in a local PLyTypeInfo variable, as we were doing already in PLyObject_ToComposite(). This is not a great technique because any fn_extra data allocated by the input function will be leaked permanently (thanks to using TopMemoryContext as fn_mcxt). But that's a pre-existing issue that is much less serious than a crash, so leave it to be fixed separately. This bug would be a potential security issue, except that plpython is only available to superusers and the crash requires coding the function in a way that didn't work before today's patches. Add regression test cases covering all the supported methods of converting composite results. Back-patch to 9.1 where the faulty coding was introduced.
2015-08-21Allow record_in() and record_recv() to work for transient record types.Tom Lane
If we have the typmod that identifies a registered record type, there's no reason that record_in() should refuse to perform input conversion for it. Now, in direct SQL usage, record_in() will always be passed typmod = -1 with type OID RECORDOID, because no typmodin exists for type RECORD, so the case can't arise. However, some InputFunctionCall users such as PLs may be able to supply the right typmod, so we should allow this to support them. Note: the previous coding and comment here predate commit 59c016aa9f490b53. There has been no case since 8.1 in which the passed type OID wouldn't be valid; and if it weren't, this error message wouldn't be apropos anyway. Better to let lookup_rowtype_tupdesc complain about it. Back-patch to 9.1, as this is necessary for my upcoming plpython fix. I'm committing it separately just to make it a bit more visible in the commit history.
2015-08-18Fix a few bogus statement type names in plpgsql error messages.Tom Lane
plpgsql's error location context messages ("PL/pgSQL function fn-name line line-no at stmt-type") would misreport a CONTINUE statement as being an EXIT, and misreport a MOVE statement as being a FETCH. These are clear bugs that have been there a long time, so back-patch to all supported branches. In addition, in 9.5 and HEAD, change the description of EXECUTE from "EXECUTE statement" to just plain EXECUTE; there seems no good reason why this statement type should be described differently from others that have a well-defined head keyword. And distinguish GET STACKED DIAGNOSTICS from plain GET DIAGNOSTICS. These are a bit more of a judgment call, and also affect existing regression-test outputs, so I did not back-patch into stable branches. Pavel Stehule and Tom Lane
2015-08-15Don't use 'bool' as a struct member name in help_config.c.Andres Freund
Doing so doesn't work if bool is a macro rather than a typedef. Although c.h spends some effort to support configurations where bool is a preexisting macro, help_config.c has existed this way since 2003 (b700a6), and there have not been any reports of problems. Backpatch anyway since this is as riskless as it gets. Discussion: 20150812084351.GD8470@awork2.anarazel.de Backpatch: 9.0-master
2015-08-13Improve regression test case to avoid depending on system catalog stats.Tom Lane
In commit 95f4e59c32866716 I added a regression test case that examined the plan of a query on system catalogs. That isn't a terribly great idea because the catalogs tend to change from version to version, or even within a version if someone makes an unrelated regression-test change that populates the catalogs a bit differently. Usually I try to make planner test cases rely on test tables that have not changed since Berkeley days, but I got sloppy in this case because the submitted crasher example queried the catalogs and I didn't spend enough time on rewriting it. But it was a problem waiting to happen, as I was rudely reminded when I tried to port that patch into Salesforce's Postgres variant :-(. So spend a little more effort and rewrite the query to not use any system catalogs. I verified that this version still provokes the Assert if 95f4e59c32866716's code fix is reverted. I also removed the EXPLAIN output from the test, as it turns out that the assertion occurs while considering a plan that isn't the one ultimately selected anyway; so there's no value in risking any cross-platform variation in that printout. Back-patch to 9.2, like the previous patch.
2015-08-13Fix declaration of isarray variable.Michael Meskes
Found and fixed by Andres Freund.
2015-08-12Undo mistaken tightening in join_is_legal().Tom Lane
One of the changes I made in commit 8703059c6b55c427 turns out not to have been such a good idea: we still need the exception in join_is_legal() that allows a join if both inputs already overlap the RHS of the special join we're checking. Otherwise we can miss valid plans, and might indeed fail to find a plan at all, as in recent report from Andreas Seltenreich. That code was added way back in commit c17117649b9ae23d, but I failed to include a regression test case then; my bad. Put it back with a better explanation, and a test this time. The logic does end up a bit different than before though: I now believe it's appropriate to make this check first, thereby allowing such a case whether or not we'd consider the previous SJ(s) to commute with this one. (Presumably, we already decided they did; but it was confusing to have this consideration in the middle of the code that was handling the other case.) Back-patch to all active branches, like the previous patch.
2015-08-12This routine was calling ecpg_alloc to allocate to memory but did notMichael Meskes
actually check the returned pointer allocated, potentially NULL which could be the result of a malloc call. Issue noted by Coverity, fixed by Michael Paquier <michael@otacoo.com>
2015-08-12Fix some possible low-memory failures in regexp compilation.Tom Lane
newnfa() failed to set the regex error state when malloc() fails. Several places in regcomp.c failed to check for an error after calling subre(). Each of these mistakes could lead to null-pointer-dereference crashes in memory-starved backends. Report and patch by Andreas Seltenreich. Back-patch to all branches.
2015-08-10Fix privilege dumping from servers too old to have that type of privilege.Tom Lane
pg_dump produced fairly silly GRANT/REVOKE commands when dumping types from pre-9.2 servers, and when dumping functions or procedural languages from pre-7.3 servers. Those server versions lack the typacl, proacl, and/or lanacl columns respectively, and pg_dump substituted default values that were in fact incorrect. We ended up revoking all the owner's own privileges for the object while granting all privileges to PUBLIC. Of course the owner would then have those privileges again via PUBLIC, so long as she did not try to revoke PUBLIC's privileges; which may explain the lack of field reports. Nonetheless this is pretty silly behavior. The stakes were raised by my recent patch to make pg_dump dump shell types, because 9.2 and up pg_dump would proceed to emit bogus GRANT/REVOKE commands for a shell type if dumping from a pre-9.2 server; and the server will not accept GRANT/REVOKE commands for a shell type. (Perhaps it should, but that's a topic for another day.) So the resulting dump script wouldn't load without errors. The right thing to do is to act as though these objects have default privileges (null ACL entries), which causes pg_dump to print no GRANT/REVOKE commands at all for them. That fixes the silly results and also dodges the problem with shell types. In passing, modify getProcLangs() to be less creatively different about how to handle missing columns when dumping from older server versions. Every other data-acquisition function in pg_dump does that by substituting appropriate default values in the version-specific SQL commands, and I see no reason why this one should march to its own drummer. Its use of "SELECT *" was likewise not conformant with anyplace else, not to mention it's not considered good SQL style for production queries. Back-patch to all supported versions. Although 9.0 and 9.1 pg_dump don't have the issue with typacl, they are more likely than newer versions to be used to dump from ancient servers, so we ought to fix the proacl/lanacl issues all the way back.
2015-08-10Accept alternate spellings of __sparcv7 and __sparcv8.Tom Lane
Apparently some versions of gcc prefer __sparc_v7__ and __sparc_v8__. Per report from Waldemar Brodkorb.
2015-08-10Further mucking with PlaceHolderVar-related restrictions on join order.Tom Lane
Commit 85e5e222b1dd02f135a8c3bf387d0d6d88e669bd turns out not to have taken care of all cases of the partially-evaluatable-PlaceHolderVar problem found by Andreas Seltenreich's fuzz testing. I had set it up to check for risky PHVs only in the event that we were making a star-schema-based exception to the param_source_rels join ordering heuristic. However, it turns out that the problem can occur even in joins that satisfy the param_source_rels heuristic, in which case allow_star_schema_join() isn't consulted. Refactor so that we check for risky PHVs whenever the proposed join has any remaining parameterization. Back-patch to 9.2, like the previous patch (except for the regression test case, which only works back to 9.3 because it uses LATERAL). Note that this discovery implies that problems of this sort could've occurred in 9.2 and up even before the star-schema patch; though I've not tried to prove that experimentally.
2015-08-07Further adjustments to PlaceHolderVar removal.Tom Lane
A new test case from Andreas Seltenreich showed that we were still a bit confused about removing PlaceHolderVars during join removal. Specifically, remove_rel_from_query would remove a PHV that was used only underneath the removable join, even if the place where it's used was the join partner relation and not the join clause being deleted. This would lead to a "too late to create a new PlaceHolderInfo" error later on. We can defend against that by checking ph_eval_at to see if the PHV could possibly be getting used at some partner rel. Also improve some nearby LATERAL-related logic. I decided that the check on ph_lateral needed to take precedence over the check on ph_needed, in case there's a lateral reference underneath the join being considered. (That may be impossible, but I'm not convinced of it, and it's easy enough to defend against the case.) Also, I realized that remove_rel_from_query's logic for updating LateralJoinInfos is dead code, because we don't build those at all until after join removal. Back-patch to 9.3. Previous versions didn't have the LATERAL issues, of course, and they also didn't attempt to remove PlaceHolderInfos during join removal. (I'm starting to wonder if changing that was really such a great idea.)
2015-08-06Fix old oversight in join removal logic.Tom Lane
Commit 9e7e29c75ad441450f9b8287bd51c13521641e3b introduced an Assert that join removal didn't reduce the eval_at set of any PlaceHolderVar to empty. At first glance it looks like join_is_removable ensures that's true --- but actually, the loop in join_is_removable skips PlaceHolderVars that are not referenced above the join due to be removed. So, if we don't want any empty eval_at sets, the right thing to do is to delete any now-unreferenced PlaceHolderVars from the data structure entirely. Per fuzz testing by Andreas Seltenreich. Back-patch to 9.3 where the aforesaid Assert was added.
2015-08-06Fix eclass_useful_for_merging to give valid results for appendrel children.Tom Lane
Formerly, this function would always return "true" for an appendrel child relation, because it would think that the appendrel parent was a potential join target for the child. In principle that should only lead to some inefficiency in planning, but fuzz testing by Andreas Seltenreich disclosed that it could lead to "could not find pathkey item to sort" planner errors in odd corner cases. Specifically, we would think that all columns of a child table's multicolumn index were interesting pathkeys, causing us to generate a MergeAppend path that sorts by all the columns. However, if any of those columns weren't actually used above the level of the appendrel, they would not get added to that rel's targetlist, which would result in being unable to resolve the MergeAppend's sort keys against its targetlist during createplan.c. Backpatch to 9.3. In older versions, columns of an appendrel get added to its targetlist even if they're not mentioned above the scan level, so that the failure doesn't occur. It might be worth back-patching this fix to older versions anyway, but I'll refrain for the moment.
2015-08-06Further fixes for degenerate outer join clauses.Tom Lane
Further testing revealed that commit f69b4b9495269cc4 was still a few bricks shy of a load: minor tweaking of the previous test cases resulted in the same wrong-outer-join-order problem coming back. After study I concluded that my previous changes in make_outerjoininfo() were just accidentally masking the problem, and should be reverted in favor of forcing syntactic join order whenever an upper outer join's predicate doesn't mention a lower outer join's LHS. This still allows the chained-outer-joins style that is the normally optimizable case. I also tightened things up some more in join_is_legal(). It seems to me on review that what's really happening in the exception case where we ignore a mismatched special join is that we're allowing the proposed join to associate into the RHS of the outer join we're comparing it to. As such, we should *always* insist that the proposed join be a left join, which eliminates a bunch of rather dubious argumentation. The case where we weren't enforcing that was the one that was already known buggy anyway (it had a violatable Assert before the aforesaid commit) so it hardly deserves a lot of deference. Back-patch to all active branches, like the previous patch. The added regression test case failed in all branches back to 9.1, and I think it's only an unrelated change in costing calculations that kept 9.0 from choosing a broken plan.
2015-08-05Make real sure we don't reassociate joins into or out of SEMI/ANTI joins.Tom Lane
Per the discussion in optimizer/README, it's unsafe to reassociate anything into or out of the RHS of a SEMI or ANTI join. An example from Piotr Stefaniak showed that join_is_legal() wasn't sufficiently enforcing this rule, so lock it down a little harder. I couldn't find a reasonably simple example of the optimizer trying to do this, so no new regression test. (Piotr's example involved the random search in GEQO accidentally trying an invalid case and triggering a sanity check way downstream in clause selectivity estimation, which did not seem like a sequence of events that would be useful to memorialize in a regression test as-is.) Back-patch to all active branches.
2015-08-04Fix pg_dump to dump shell types.Tom Lane
Per discussion, it really ought to do this. The original choice to exclude shell types was probably made in the dark ages before we made it harder to accidentally create shell types; but that was in 7.3. Also, cause the standard regression tests to leave a shell type behind, for convenience in testing the case in pg_dump and pg_upgrade. Back-patch to all supported branches.