XLogRecPtr was compared with InvalidTransactionId instead ofInvalidXLogRecPtr. As both are defined to the same value this doesn'tcause any actual problems, but it's still wrong.Backpatch: 9.4-master, bug was introduced in 9.4

Commit2834855 added a not-very-carefully-thought-out isolation testto check a BRIN index bug fix.  The test depended on the availabilityof the pageinspect contrib module, which meant it did not work inseveral common testing scenarios such as "make check-world".  It's notclear whether we want a core test depending on a contrib module likethat, but in any case, failing to deal with the possibility that themodule isn't present in the installation-under-test is not acceptable.Remove that test pending some better solution.

Commit85e5e22 turns out not to have takencare of all cases of the partially-evaluatable-PlaceHolderVar problem foundby Andreas Seltenreich's fuzz testing.  I had set it up to check for riskyPHVs only in the event that we were making a star-schema-based exception tothe param_source_rels join ordering heuristic.  However, it turns out thatthe problem can occur even in joins that satisfy the param_source_relsheuristic, in which case allow_star_schema_join() isn't consulted.Refactor so that we check for risky PHVs whenever the proposed join hasany remaining parameterization.Back-patch to 9.2, like the previous patch (except for the regression testcase, which only works back to 9.3 because it uses LATERAL).Note that this discovery implies that problems of this sort could'veoccurred in 9.2 and up even before the star-schema patch; though I've nottried to prove that experimentally.

Apparently some versions of gcc prefer __sparc_v7__ and __sparc_v8__.Per report from Waldemar Brodkorb.

…lege.pg_dump produced fairly silly GRANT/REVOKE commands when dumping types frompre-9.2 servers, and when dumping functions or procedural languages frompre-7.3 servers.  Those server versions lack the typacl, proacl, and/orlanacl columns respectively, and pg_dump substituted default values thatwere in fact incorrect.  We ended up revoking all the owner's ownprivileges for the object while granting all privileges to PUBLIC.Of course the owner would then have those privileges again via PUBLIC, solong as she did not try to revoke PUBLIC's privileges; which may explainthe 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/REVOKEcommands for a shell type if dumping from a pre-9.2 server; and the serverwill not accept GRANT/REVOKE commands for a shell type.  (Perhaps itshould, but that's a topic for another day.)  So the resulting dump scriptwouldn't load without errors.The right thing to do is to act as though these objects have defaultprivileges (null ACL entries), which causes pg_dump to print noGRANT/REVOKE commands at all for them.  That fixes the silly resultsand also dodges the problem with shell types.In passing, modify getProcLangs() to be less creatively different abouthow to handle missing columns when dumping from older server versions.Every other data-acquisition function in pg_dump does that by substitutingappropriate default values in the version-specific SQL commands, and I seeno reason why this one should march to its own drummer.  Its use of"SELECT *" was likewise not conformant with anyplace else, not to mentionit's not considered good SQL style for production queries.Back-patch to all supported versions.  Although 9.0 and 9.1 pg_dump don'thave the issue with typacl, they are more likely than newer versions to beused to dump from ancient servers, so we ought to fix the proacl/lanaclissues all the way back.

Fix a bunch of typos, and remove two superflous includes.Author: Gurjeet SinghDiscussion: CABwTF4Wh_dBCzTU=49pFXR6coR4NW1ynb+vBqT+Po=7fuq5iCw@mail.gmail.comBackpatch: 9.4

newnfa() failed to set the regex error state when malloc() fails.Several places in regcomp.c failed to check for an error after callingsubre().  Each of these mistakes could lead to null-pointer-dereferencecrashes in memory-starved backends.Report and patch by Andreas Seltenreich.  Back-patch to all branches.

In4b4b680 I passed a buffer index number (starting from 0) instead ofa proper Buffer id (which start from 1 for shared buffers) in twoplaces.This wasn't noticed so far as one of those locations isn't compiled atall (PrintPinnedBufs) and the other one (InvalidBuffer) requires aunlikely, but possible, set of circumstances to trigger a symptom.To reduce the likelihood of such incidents a bit also convert existingopen coded mappings from buffer descriptors to buffer ids withBufferDescriptorGetBuffer().Author: Qingqing ZhouReported-By: Qingqing ZhouDiscussion: CAJjS0u2ai9ooUisKtkV8cuVUtEkMTsbK8c7juNAjv8K11zeCQg@mail.gmail.comBackpatch: 9.5 where the private ref count infrastructure was introduced

…til.c.Inff27db5 I missed that PQresultErrorField() may return NULL ifthere's no sqlstate associated with an error.Spotted-By: CoverityReported-By: Michael PaquierDiscussion: CAB7nPqQ3o10SY6NVdU4pjq85GQTN5tbbkq2gnNUh2fBNU3rKyQ@mail.gmail.comBackpatch: 9.5, likeff27db5

In some corner cases, it is possible for the BRIN index relation to beextended by brin_getinsertbuffer but the new page not be usedimmediately for anything by its callers; when this happens, the page isinitialized and the FSM is updated (by brin_getinsertbuffer) with theinfo about that page, but these actions are not WAL-logged.  A laterindex insert/update can use the page, but since the page is alreadyinitialized, the initialization itself is not WAL-logged then either.Replay of this sequence of events causes recovery to fail altogether.There is a related corner case within brin_getinsertbuffer itself, inwhich we extend the relation to put a new index tuple there, but laterfind out that we cannot do so, and do not return the buffer; the pageobtained from extension is not even initialized.  The resulting page islost forever.To fix, shuffle the code so that initialization is not theresponsibility of brin_getinsertbuffer anymore, in normal cases;instead, the initialization is done by its callers (brin_doinsert andbrin_doupdate) once they're certain that the page is going to be used.When either those functions determine that the new page cannot be used,before bailing out they initialize the page as an empty regular page,enter it in FSM and WAL-log all this.  This way, the page is usable forfuture index insertions, and WAL replay doesn't find trying to inserttuples in pages whose initialization didn't make it to the WAL.  Thesame strategy is used in brin_getinsertbuffer when it cannot return thenew page.Additionally, add a new step to vacuuming so that all pages of the indexare scanned; whenever an uninitialized page is found, it is initializedas empty and WAL-logged.  This closes the hole that the relation isextended but the system crashes before anything is WAL-logged about it.We also take this opportunity to update the FSM, in case it has gottenout of date.Thanks to Heikki Linnakangas for finding the problem that kicked someadditional analysis of BRIN page assignment code.Backpatch to 9.5, where BRIN was introduced.Discussion:https://www.postgresql.org/message-id/20150723204810.GY5596@postgresql.org

One of the changes I made in commit8703059 turns out not to havebeen such a good idea: we still need the exception in join_is_legal() thatallows a join if both inputs already overlap the RHS of the special joinwe're checking.  Otherwise we can miss valid plans, and might indeed failto find a plan at all, as in recent report from Andreas Seltenreich.That code was added way back in commitc171176, but I failed toinclude a regression test case then; my bad.  Put it back with a betterexplanation, and a test this time.  The logic does end up a bit differentthan before though: I now believe it's appropriate to make this checkfirst, thereby allowing such a case whether or not we'd consider theprevious SJ(s) to commute with this one.  (Presumably, we already decidedthey did; but it was confusing to have this consideration in the middleof the code that was handling the other case.)Back-patch to all active branches, like the previous patch.

As complained by clang, reported by Andres Freund.  Brown paper bag buginccc4c07.Add some comments, too.Backpatch to 9.5, like that one.

Found and fixed by Andres Freund.

This function was using the single-value-per-call mechanism, but thecode relied on a relcache entry that wasn't kept open across calls.This manifested as weird errors in buildfarm during the short time thatthe "brin-1" isolation test lived.Backpatch to 9.5, where it was introduced.

In commit95f4e59 I added a regression test case that examinedthe plan of a query on system catalogs.  That isn't a terribly great ideabecause the catalogs tend to change from version to version, or evenwithin a version if someone makes an unrelated regression-test change thatpopulates the catalogs a bit differently.  Usually I try to make plannertest cases rely on test tables that have not changed since Berkeley days,but I got sloppy in this case because the submitted crasher example queriedthe catalogs and I didn't spend enough time on rewriting it.  But it was aproblem waiting to happen, as I was rudely reminded when I tried to portthat patch into Salesforce's Postgres variant :-(.  So spend a little moreeffort and rewrite the query to not use any system catalogs.  I verifiedthat this version still provokes the Assert if95f4e59's code fixis reverted.I also removed the EXPLAIN output from the test, as it turns out that theassertion occurs while considering a plan that isn't the one ultimatelyselected anyway; so there's no value in risking any cross-platformvariation in that printout.Back-patch to 9.2, like the previous patch.

This time, instead of using a core isolation test, put it on its owntest module; this way it can require the pageinspect module to bepresent before running.The module's Makefile is loosely modeled after test_decoding's, so thatit's easy to add further tests for either pg_regress or isolationtesterlater.Backpatch to 9.5.

The build script is not able to parse the Makefile, so remove it.

Commit49c817e replaced with a harderror the dubious pg_do_encoding_conversion() behavior when outside atransaction.  Reintroduce the historic soft failure locally withinpgwin32_message_to_UTF16().  This fixes errors when writing messages inless-common encodings to the Windows event log or console.  Back-patchto 9.4, where the aforementioned commit first appeared.Per bug #13427 from Dmitri Bourlatchkov.

This fixes presentation of non-ASCII messages to the Windows event logand console in rare cases involving Korean locale.  Processes like thepostmaster and checkpointer, but not processes attached to databases,were affected.  Back-patch to 9.4, where MessageEncoding was introduced.The problem exists in all supported versions, but this change has noeffect in the absence of the code recognizing PG_UHC MessageEncoding.Noticed while investigating bug #13427 from Dmitri Bourlatchkov.

Mistakenly relreplident was stored as a bool. That works today as c.htypedefs bool to a char, but isn't very future proof.Discussion: 20150812084351.GD8470@awork2.anarazel.deBackpatch: 9.4 where replica identity was introduced.

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 isa preexisting macro, help_config.c has existed this way since2003 (b700a6), and there have not been any reports ofproblems. Backpatch anyway since this is as riskless as it gets.Discussion: 20150812084351.GD8470@awork2.anarazel.deBackpatch: 9.0-master

Sincea179232 (vacuumdb: enable parallel mode) -1 has been assignedto a boolean. That can, justifiedly, trigger compiler warnings. There'salso no need for ternary logic, result was only ever set to 0 or -1. Sodon't.Discussion: 20150812084351.GD8470@awork2.anarazel.deBackpatch: 9.5

It was a bool, even though it should be CEOUC_WAIT_MODE. That's unlikelyto have a negative effect with the current definition of bool (char),but it's definitely wrong.Discussion: 20150812084351.GD8470@awork2.anarazel.deBackpatch: 9.5, where ON CONFLICT was merged

DO blocks use private simple_eval_estates to avoid intra-transaction memoryleakage, cf commitc7b849a.  I had forgotten about that while writingcommit0fc94a5, but it means that expression execution trees createdwithin a DO block disappear immediately on exiting the DO block, and hencecan't safely be linked into plpgsql's session-wide cast hash table.To fix, give a DO block a private cast hash table to go with its privatesimple_eval_estate.  This is less efficient than one could wish, sinceDO blocks can no longer share any cast lookup work with other plpgsqlexecution, but it shouldn't be too bad; in any case it's no worse thanwhat happened in DO blocks before commit0fc94a5.Per bug #13571 from Feike Steenbergen.  Preliminary analysis byOleksandr Shulgin.

The table-rewriting forms of ALTER TABLE are MVCC-unsafe, in much the sameway as TRUNCATE, because they replace all rows of the table with newly-maderows with a new xmin.  (Ideally, concurrent transactions with old snapshotswould continue to see the old table contents, but the data is not thereanymore --- and if it were there, it would be inconsistent with the table'supdated rowtype, so there would be serious implementation problems to fix.)This was nowhere documented though, and the problem was only documented forTRUNCATE in a note in the TRUNCATE reference page.  Create a new "Caveats"section in the MVCC chapter that can be home to this and other limitationson serializable consistency.In passing, fix a mistaken statement that VACUUM and CLUSTER would reclaimspace occupied by a dropped column.  They don't reconstruct existing tuplesso they couldn't do that.Back-patch to all supported branches.

This behavior wasn't documented, but it should be because it's user-visiblein triggers and other functions executed on the remote server.Per question from Adam Fuchs.Back-patch to 9.3 where postgres_fdw was added.

With optimizations enabled at least one compiler, clang 3.7, optimizedaway the crc intrinsics knowing that the result went on unused and hasno side effects. That can trigger errors in code generation when theintrinsic is used, as we chose to use the intrinsics without anyadditional compiler flag. Return the computed value to prevent that.With some more pedantic warning flags (-Wold-style-definition) theconfigure test failed to recognize the existence of _mm_crc32_u*intrinsics due to an independent warning in the test because the testturned on -Werror, but that's not actually needed here.Discussion: 20150814092039.GH4955@awork2.anarazel.deBackpatch: 9.5, where the use of crc intrinsics was integrated.

If the user has typed GRANT EXECUTE, the correct completion is "ON",not "PROCEDURE".Daniel Verite

plpgsql's error location context messages ("PL/pgSQL function fn-name lineline-no at stmt-type") would misreport a CONTINUE statement as being anEXIT, and misreport a MOVE statement as being a FETCH.  These are clearbugs that have been there a long time, so back-patch to all supportedbranches.In addition, in 9.5 and HEAD, change the description of EXECUTE from"EXECUTE statement" to just plain EXECUTE; there seems no good reason whythis statement type should be described differently from others that havea well-defined head keyword.  And distinguish GET STACKED DIAGNOSTICS fromplain GET DIAGNOSTICS.  These are a bit more of a judgment call, and alsoaffect existing regression-test outputs, so I did not back-patch intostable branches.Pavel Stehule and Tom Lane

with suggestion from Michael Paquier

The shm_mq mechanism was intended to optionally notice when the processon the other end of the queue fails to attach to the queue.  It doesthis by allowing the user to pass a BackgroundWorkerHandle; if thebackground worker in question is launched and dies without attachingto the queue, then we know it never will.  This logic works OK inblocking mode, but when called with nowait = true we fail to noticethat this has happened due to an asymmetry in the logic.  Repair.Reported off-list by Rushabh Lathia.  Patch by me.

This way, we produce a better error message if someone tries to dosomething like ALTER INDEX .. ALTER COLUMN .. SET STORAGE.Amit Langote

If the counterparty writes some data into the queue and then detaches,it's wrong to return SHM_MQ_DETACHED right away.  If we do that, wefail to read whatever was written.

Bernd Helmle complained that CreateReplicationSlot() was assigning thesame value to the same variable twice, so we could remove one of them.Code inspection reveals that we can actually remove both assignments:according to the author the assignment was there for beauty of thestrlen line only, and another possible fix to that is to put the strlenin its own line, so do that.To be consistent within the file, refactor all duplicated strlen()calls, which is what we do elsewhere in the backend anyway.  Inbasebackup.c, snprintf already returns the right length; no need forstrlen afterwards.Backpatch to 9.4, where replication slots were introduced, to keep codeidentical.  Some of this is older, but the patch doesn't apply cleanlyand it's only of cosmetic value anyway.Discussion:http://www.postgresql.org/message-id/BE2FD71DEA35A2287EA5F018@eje.credativ.lan

Further tweak commit_ts.c so that on a standby the state is completelyconsistent with what that in the master, rather than behavingdifferently in the cases that the settings differ.  Now in standby andmaster the module should always be active or inactive in lockstep.Author: Petr Jelínek, with some further tweaks by Álvaro Herrera.Backpatch to 9.5, where commit timestamps were introduced.Discussion:http://www.postgresql.org/message-id/5622BF9D.2010409@2ndquadrant.com

A bug in the original free space computation made it possible toreturn a page which wasn't actually able to fit the item.  Since theinsertion code isn't prepared to deal with PageAddItem failing, a PANICresulted ("failed to add BRIN tuple [to new page]").  Add a macro toencapsulate the correct computation, and use it inbrin_getinsertbuffer's callers before calling that routine, to raise anearly error.I became aware of the possiblity of a problem in this area while workingonccc4c07.  There's no archived discussion about it, but it'seasy to reproduce a problem in the unpatched code with something likeCREATE TABLE t (a text);CREATE INDEX ti ON t USING brin (a) WITH (pages_per_range=1);for length in `seq 8000 8196`dopsql -f - <<EOFTRUNCATE TABLE t;INSERT INTO t VALUES ('z'), (repeat('a', $length));EOFdoneBackpatch to 9.5, where BRIN was introduced.

Backpatch to 9.5 -- this should have been part ofb0b7be6, but wedidn't have38b03ca either at the time.Author: Emre HasegeliRevised by: Ian BarwickDiscussion:http://www.postgresql.org/message-id/CAE2gYzyB39Q9up_-TO6FKhH44pcAM1x6n_Cuj15qKoLoFihUVg@mail.gmail.comhttp://www.postgresql.org/message-id/562DA711.3020305@2ndquadrant.com

Per red wall in buildfarm

Mistake introduced by commit3bf3ab8.Etsuro Fujita

Amit Langote, per Etsuro Fujita

Message style, plurals, quoting, spelling, consistency with similarmessages

Show how this can be used in practice to make queries simpler and moreflexible.  Also, draw an explicit contrast to the existence operator,which doesn't work that way.Peter Geoghegan and Tom Lane

Mistake introduced by commit5bd91e3.Hari Babu

On insert the CheckForSerializableConflictIn() test was performedbefore the page(s) which were going to be modified had been locked(with an exclusive buffer content lock).  If another processacquired a relation SIReadLock on the heap and scanned to a page onwhich an insert was going to occur before the page was so locked,a rw-conflict would be missed, which could allow a serializationanomaly to be missed.  The window between the check and the pagelock was small, so the bug was generally not noticed unless therewas high concurrency with multiple processes inserting into thesame table.This was reported by Peter Bailis as bug #11732, by Sean Chittendenas bug #13667, and by others.The race condition was eliminated in heap_insert() by moving thecheck down below the acquisition of the buffer lock, which had beenthe very next statement.  Because of the loop locking and unlockingmultiple buffers in heap_multi_insert() a check was added after allinserts were completed.  The check before the start of the insertswas left because it might avoid a large amount of work to detect aserialization anomaly before performing the all of the inserts andthe related WAL logging.While investigating this bug, other SSI bugs which were even harderto hit in practice were noticed and fixed, an unnecessary check(covered by another check, so redundant) was removed fromheap_update(), and comments were improved.Back-patch to all supported branches.Kevin Grittner and Thomas Munro

Backpatch to 9.3, where it was initially omitted.Craig Ringer, with minor adjustment by Kevin Grittner

Commita1480ec purported to fix theproblems with commitb2ccb5f, but itdidn't completely fix them.  The problem is that the checks wereperformed in the wrong order, leading to a race condition.  If thesender attached, sent a message, and detached after the receivercalled shm_mq_get_sender and before the receiver calledshm_mq_counterparty_gone, we'd incorrectly return SHM_MQ_DETACHEDbefore all messages were read.  Repair by reversing the order ofoperations, and add a long comment explaining why this new logic is(hopefully) correct.

Fix some brain fade in commita2dabf0: erroneous variable namesin docs, rearrangements that made sentences less clear not more so,undocumented and poorly-chosen-anyway API behaviors of subroutines,bad grammar in error messages, copy-and-paste faults.Albe Laurenz and Tom Lane

Standard-conforming literals have been the default for long enough thatit no longer seems necessary to go out of our way to tell people to writeregex escapes illegibly.

Peter Geoghegan