Skip to content
Snippets Groups Projects
  1. May 05, 2015
    • Tom Lane's avatar
    • Tom Lane's avatar
      Fix incorrect declaration of citext's regexp_matches() functions. · d4070d10
      Tom Lane authored
      These functions should return SETOF TEXT[], like the core functions they
      are wrappers for; but they were incorrectly declared as returning just
      TEXT[].  This mistake had two results: first, if there was no match you got
      a scalar null result, whereas what you should get is an empty set (zero
      rows).  Second, the 'g' flag was effectively ignored, since you would get
      only one result array even if there were multiple matches, as reported by
      Jeff Certain.
      
      While ignoring 'g' is a clear bug, the behavior for no matches might well
      have been thought to be the intended behavior by people who hadn't compared
      it carefully to the core regexp_matches() functions.  So we should tread
      carefully about introducing this change in the back branches.  Still, it
      clearly is a bug and so providing some fix is desirable.
      
      After discussion, the conclusion was to introduce the change in a 1.1
      version of the citext extension (as we would need to do anyway); 1.0 still
      contains the incorrect behavior.  1.1 is the default and only available
      version in HEAD, but it is optional in the back branches, where 1.0 remains
      the default version.  People wishing to adopt the fix in back branches will
      need to explicitly do ALTER EXTENSION citext UPDATE TO '1.1'.  (I also
      provided a downgrade script in the back branches, so people could go back
      to 1.0 if necessary.)
      
      This should be called out as an incompatible change in the 9.5 release
      notes, although we'll also document it in the next set of back-branch
      release notes.  The notes should mention that any views or rules that use
      citext's regexp_matches() functions will need to be dropped before
      upgrading to 1.1, and then recreated again afterwards.
      
      Back-patch to 9.1.  The bug goes all the way back to citext's introduction
      in 8.4, but pre-9.1 there is no extension mechanism with which to manage
      the change.  Given the lack of previous complaints it seems unnecessary to
      change this behavior in 9.0, anyway.
      d4070d10
    • Robert Haas's avatar
      Fix some problems with patch to fsync the data directory. · 53e1498c
      Robert Haas authored
      pg_win32_is_junction() was a typo for pgwin32_is_junction().  open()
      was used not only in a two-argument form, which breaks on Windows,
      but also where BasicOpenFile() should have been used.
      
      Per reports from Andrew Dunstan and David Rowley.
      53e1498c
  2. May 04, 2015
    • Robert Haas's avatar
      Recursively fsync() the data directory after a crash. · 2bc33971
      Robert Haas authored
      Otherwise, if there's another crash, some writes from after the first
      crash might make it to disk while writes from before the crash fail
      to make it to disk.  This could lead to data corruption.
      
      Back-patch to all supported versions.
      
      Abhijit Menon-Sen, reviewed by Andres Freund and slightly revised
      by me.
      2bc33971
  3. Apr 26, 2015
  4. Apr 25, 2015
    • Tom Lane's avatar
      Prevent improper reordering of antijoins vs. outer joins. · 950f80dd
      Tom Lane authored
      An outer join appearing within the RHS of an antijoin can't commute with
      the antijoin, but somehow I missed teaching make_outerjoininfo() about
      that.  In Teodor Sigaev's recent trouble report, this manifests as a
      "could not find RelOptInfo for given relids" error within eqjoinsel();
      but I think silently wrong query results are possible too, if the planner
      misorders the joins and doesn't happen to trigger any internal consistency
      checks.  It's broken as far back as we had antijoins, so back-patch to all
      supported branches.
      950f80dd
    • Noah Misch's avatar
      Build every ECPG library with -DFRONTEND. · 5eab0f13
      Noah Misch authored
      Each of the libraries incorporates src/port files, which often check
      FRONTEND.  Build systems disagreed on whether to build libpgtypes this
      way.  Only libecpg incorporates files that rely on it today.  Back-patch
      to 9.0 (all supported versions) to forestall surprises.
      5eab0f13
  5. Apr 24, 2015
    • Tom Lane's avatar
      Fix obsolete comment in set_rel_size(). · b743a8b3
      Tom Lane authored
      The cross-reference to set_append_rel_pathlist() was obsoleted by
      commit e2fa76d8, which split what
      had been set_rel_pathlist() and child routines into two sets of
      functions.  But I (tgl) evidently missed updating this comment.
      
      Back-patch to 9.2 to avoid unnecessary divergence among branches.
      
      Amit Langote
      b743a8b3
  6. Apr 23, 2015
    • Heikki Linnakangas's avatar
      Fix deadlock at startup, if max_prepared_transactions is too small. · d3f5d289
      Heikki Linnakangas authored
      When the startup process recovers transactions by scanning pg_twophase
      directory, it should clear MyLockedGxact after it's done processing each
      transaction. Like we do during normal operation, at PREPARE TRANSACTION.
      Otherwise, if the startup process exits due to an error, it will try to
      clear the locking_backend field of the last recovered transaction. That's
      usually harmless, but if the error happens in MarkAsPreparing, while
      holding TwoPhaseStateLock, the shmem-exit hook will try to acquire
      TwoPhaseStateLock again, and deadlock with itself.
      
      This fixes bug #13128 reported by Grant McAlister. The bug was introduced
      by commit bb38fb0d, so backpatch to all supported versions like that
      commit.
      d3f5d289
  7. Apr 14, 2015
    • Alvaro Herrera's avatar
      Fix typo in comment · 3d73a67f
      Alvaro Herrera authored
      SLRU_SEGMENTS_PER_PAGE -> SLRU_PAGES_PER_SEGMENT
      
      I introduced this ancient typo in subtrans.c and later propagated it to
      multixact.c.  I fixed the latter in f741300c, but only back to 9.3;
      backpatch to all supported branches for consistency.
      3d73a67f
  8. Apr 13, 2015
    • Heikki Linnakangas's avatar
      Don't archive bogus recycled or preallocated files after timeline switch. · cc2939f4
      Heikki Linnakangas authored
      After a timeline switch, we would leave behind recycled WAL segments that
      are in the future, but on the old timeline. After promotion, and after they
      become old enough to be recycled again, we would notice that they don't have
      a .ready or .done file, create a .ready file for them, and archive them.
      That's bogus, because the files contain garbage, recycled from an older
      timeline (or prealloced as zeros). We shouldn't archive such files.
      
      This could happen when we're following a timeline switch during replay, or
      when we switch to new timeline at end-of-recovery.
      
      To fix, whenever we switch to a new timeline, scan the data directory for
      WAL segments on the old timeline, but with a higher segment number, and
      remove them. Those don't belong to our timeline history, and are most
      likely bogus recycled or preallocated files. They could also be valid files
      that we streamed from the primary ahead of time, but in any case, they're
      not needed to recover to the new timeline.
      cc2939f4
  9. Apr 12, 2015
  10. Apr 09, 2015
  11. Apr 08, 2015
    • Alvaro Herrera's avatar
      Fix autovacuum launcher shutdown sequence · 37dc228e
      Alvaro Herrera authored
      It was previously possible to have the launcher re-execute its main loop
      before shutting down if some other signal was received or an error
      occurred after getting SIGTERM, as reported by Qingqing Zhou.
      
      While investigating, Tom Lane further noticed that if autovacuum had
      been disabled in the config file, it would misbehave by trying to start
      a new worker instead of bailing out immediately -- it would consider
      itself as invoked in emergency mode.
      
      Fix both problems by checking the shutdown flag in a few more places.
      These problems have existed since autovacuum was introduced, so
      backpatch all the way back.
      37dc228e
  12. Apr 07, 2015
    • Tom Lane's avatar
      Fix assorted inconsistent function declarations. · 6a560f5b
      Tom Lane authored
      While gcc doesn't complain if you declare a function "static" and then
      define it not-static, other compilers do; and in any case the code is
      highly misleading this way.  Add the missing "static" keywords to a
      couple of recent patches.  Per buildfarm member pademelon.
      6a560f5b
  13. Apr 06, 2015
  14. Apr 05, 2015
    • Tom Lane's avatar
      Fix incorrect matching of subexpressions in outer-join plan nodes. · b7d493bf
      Tom Lane authored
      Previously we would re-use input subexpressions in all expression trees
      attached to a Join plan node.  However, if it's an outer join and the
      subexpression appears in the nullable-side input, this is potentially
      incorrect for apparently-matching subexpressions that came from above
      the outer join (ie, targetlist and qpqual expressions), because the
      executor will treat the subexpression value as NULL when maybe it should
      not be.
      
      The case is fairly hard to hit because (a) you need a non-strict
      subexpression (else NULL is correct), and (b) we don't usually compute
      expressions in the outputs of non-toplevel plan nodes.  But we might do
      so if the expressions are sort keys for a mergejoin, for example.
      
      Probably in the long run we should make a more explicit distinction between
      Vars appearing above and below an outer join, but that will be a major
      planner redesign and not at all back-patchable.  For the moment, just hack
      set_join_references so that it will not match any non-Var expressions
      coming from nullable inputs to expressions that came from above the join.
      (This is somewhat overkill, in that a strict expression could still be
      matched, but it doesn't seem worth the effort to check that.)
      
      Per report from Qingqing Zhou.  The added regression test case is based
      on his example.
      
      This has been broken for a very long time, so back-patch to all active
      branches.
      b7d493bf
  15. Apr 03, 2015
    • Tom Lane's avatar
      Remove unnecessary variables in _hash_splitbucket(). · 0aff9d83
      Tom Lane authored
      Commit ed9cc2b5 made it unnecessary to pass
      start_nblkno to _hash_splitbucket(), and for that matter unnecessary to
      have the internal nblkno variable either.  My compiler didn't complain
      about that, but some did.  I also rearranged the use of oblkno a bit to
      make that case more parallel.
      
      Report and initial patch by Petr Jelinek, rearranged a bit by me.
      Back-patch to all branches, like the previous patch.
      0aff9d83
  16. Apr 02, 2015
    • Alvaro Herrera's avatar
      psql: fix \connect with URIs and conninfo strings · d4bacdcb
      Alvaro Herrera authored
      psql was already accepting conninfo strings as the first parameter in
      \connect, but the way it worked wasn't sane; some of the other
      parameters would get the previous connection's values, causing it to
      connect to a completely unexpected server or, more likely, not finding
      any server at all because of completely wrong combinations of
      parameters.
      
      Fix by explicitely checking for a conninfo-looking parameter in the
      dbname position; if one is found, use its complete specification rather
      than mix with the other arguments.  Also, change tab-completion to not
      try to complete conninfo/URI-looking "dbnames" and document that
      conninfos are accepted as first argument.
      
      There was a weak consensus to backpatch this, because while the behavior
      of using the dbname as a conninfo is nowhere documented for \connect, it
      is reasonable to expect that it works because it does work in many other
      contexts.  Therefore this is backpatched all the way back to 9.0.
      
      To implement this, routines previously private to libpq have been
      duplicated so that psql can decide what looks like a conninfo/URI
      string.  In back branches, just duplicate the same code all the way back
      to 9.2, where URIs where introduced; 9.0 and 9.1 have a simpler version.
      In master, the routines are moved to src/common and renamed.
      
      Author: David Fetter, Andrew Dunstan.  Some editorialization by me
      (probably earning a Gierth's "Sloppy" badge in the process.)
      Reviewers: Andrew Gierth, Erik Rijkers, Pavel Stěhule, Stephen Frost,
      Robert Haas, Andrew Dunstan.
      d4bacdcb
  17. Apr 01, 2015
  18. Mar 31, 2015
  19. Mar 30, 2015
    • Andrew Dunstan's avatar
      Run pg_upgrade and pg_resetxlog with restricted token on Windows · 94856631
      Andrew Dunstan authored
      As with initdb these programs need to run with a restricted token, and
      if they don't pg_upgrade will fail when run as a user with Adminstrator
      privileges.
      
      Backpatch to all live branches. On the development branch the code is
      reorganized so that the restricted token code is now in a single
      location. On the stable bramches a less invasive change is made by
      simply copying the relevant code to pg_upgrade.c and pg_resetxlog.c.
      
      Patches and bug report from Muhammad Asif Naeem, reviewed by Michael
      Paquier, slightly edited by me.
      94856631
    • Tom Lane's avatar
      Fix bogus concurrent use of _hash_getnewbuf() in bucket split code. · f155466f
      Tom Lane authored
      _hash_splitbucket() obtained the base page of the new bucket by calling
      _hash_getnewbuf(), but it held no exclusive lock that would prevent some
      other process from calling _hash_getnewbuf() at the same time.  This is
      contrary to _hash_getnewbuf()'s API spec and could in fact cause failures.
      In practice, we must only call that function while holding write lock on
      the hash index's metapage.
      
      An additional problem was that we'd already modified the metapage's bucket
      mapping data, meaning that failure to extend the index would leave us with
      a corrupt index.
      
      Fix both issues by moving the _hash_getnewbuf() call to just before we
      modify the metapage in _hash_expandtable().
      
      Unfortunately there's still a large problem here, which is that we could
      also incur ENOSPC while trying to get an overflow page for the new bucket.
      That would leave the index corrupt in a more subtle way, namely that some
      index tuples that should be in the new bucket might still be in the old
      one.  Fixing that seems substantially more difficult; even preallocating as
      many pages as we could possibly need wouldn't entirely guarantee that the
      bucket split would complete successfully.  So for today let's just deal
      with the base case.
      
      Per report from Antonin Houska.  Back-patch to all active branches.
      f155466f
  20. Mar 29, 2015
    • Tom Lane's avatar
      Add vacuum_delay_point call in compute_index_stats's per-sample-row loop. · d12afe11
      Tom Lane authored
      Slow functions in index expressions might cause this loop to take long
      enough to make it worth being cancellable.  Probably it would be enough
      to call CHECK_FOR_INTERRUPTS here, but for consistency with other
      per-sample-row loops in this file, let's use vacuum_delay_point.
      
      Report and patch by Jeff Janes.  Back-patch to all supported branches.
      d12afe11
  21. Mar 26, 2015
  22. Mar 24, 2015
    • Tom Lane's avatar
      Fix ExecOpenScanRelation to take a lock on a ROW_MARK_COPY relation. · 3fbfd5db
      Tom Lane authored
      ExecOpenScanRelation assumed that any relation listed in the ExecRowMark
      list has been locked by InitPlan; but this is not true if the rel's
      markType is ROW_MARK_COPY, which is possible if it's a foreign table.
      
      In most (possibly all) cases, failure to acquire a lock here isn't really
      problematic because the parser, planner, or plancache would have taken the
      appropriate lock already.  In principle though it might leave us vulnerable
      to working with a relation that we hold no lock on, and in any case if the
      executor isn't depending on previously-taken locks otherwise then it should
      not do so for ROW_MARK_COPY relations.
      
      Noted by Etsuro Fujita.  Back-patch to all active versions, since the
      inconsistency has been there a long time.  (It's almost certainly
      irrelevant in 9.0, since that predates foreign tables, but the code's
      still wrong on its own terms.)
      3fbfd5db
  23. Mar 16, 2015
    • Tom Lane's avatar
      Replace insertion sort in contrib/intarray with qsort(). · 8582ae7a
      Tom Lane authored
      It's all very well to claim that a simplistic sort is fast in easy
      cases, but O(N^2) in the worst case is not good ... especially if the
      worst case is as easy to hit as "descending order input".  Replace that
      bit with our standard qsort.
      
      Per bug #12866 from Maksym Boguk.  Back-patch to all active branches.
      8582ae7a
  24. Mar 14, 2015
    • Tom Lane's avatar
      Remove workaround for ancient incompatibility between readline and libedit. · 309ff2ad
      Tom Lane authored
      GNU readline defines the return value of write_history() as "zero if OK,
      else an errno code".  libedit's version of that function used to have a
      different definition (to wit, "-1 if error, else the number of lines
      written to the file").  We tried to work around that by checking whether
      errno had become nonzero, but this method has never been kosher according
      to the published API of either library.  It's reportedly completely broken
      in recent Ubuntu releases: psql bleats about "No such file or directory"
      when saving ~/.psql_history, even though the write worked fine.
      
      However, libedit has been following the readline definition since somewhere
      around 2006, so it seems all right to finally break compatibility with
      ancient libedit releases and trust that the return value is what readline
      specifies.  (I'm not sure when the various Linux distributions incorporated
      this fix, but I did find that OS X has been shipping fixed versions since
      10.5/Leopard.)
      
      If anyone is still using such an ancient libedit, they will find that psql
      complains it can't write ~/.psql_history at exit, even when the file was
      written correctly.  This is no worse than the behavior we're fixing for
      current releases.
      
      Back-patch to all supported branches.
      309ff2ad
    • Tatsuo Ishii's avatar
      Fix integer overflow in debug message of walreceiver · 4909cb59
      Tatsuo Ishii authored
      The message tries to tell the replication apply delay which fails if
      the first WAL record is not applied yet. Fix is, instead of telling
      overflowed minus numeric, showing "N/A" which indicates that the delay
      data is not yet available. Problem reported by me and patch by
      Fabrízio de Royes Mello.
      
      Back patched to 9.4, 9.3 and 9.2 stable branches (9.1 and 9.0 do not
      have the debug message).
      4909cb59
  25. Mar 12, 2015
    • Tom Lane's avatar
      Ensure tableoid reads correctly in EvalPlanQual-manufactured tuples. · 590fc5d9
      Tom Lane authored
      The ROW_MARK_COPY path in EvalPlanQualFetchRowMarks() was just setting
      tableoid to InvalidOid, I think on the assumption that the referenced
      RTE must be a subquery or other case without a meaningful OID.  However,
      foreign tables also use this code path, and they do have meaningful
      table OIDs; so failure to set the tuple field can lead to user-visible
      misbehavior.  Fix that by fetching the appropriate OID from the range
      table.
      
      There's still an issue about whether CTID can ever have a meaningful
      value in this case; at least with postgres_fdw foreign tables, it does.
      But that is a different problem that seems to require a significantly
      different patch --- it's debatable whether postgres_fdw really wants to
      use this code path at all.
      
      Simplified version of a patch by Etsuro Fujita, who also noted the
      problem to begin with.  The issue can be demonstrated in all versions
      having FDWs, so back-patch to 9.1.
      590fc5d9
  26. Mar 08, 2015
    • Tom Lane's avatar
      Fix documentation for libpq's PQfn(). · ae67e81e
      Tom Lane authored
      The SGML docs claimed that 1-byte integers could be sent or received with
      the "isint" options, but no such behavior has ever been implemented in
      pqGetInt() or pqPutInt().  The in-code documentation header for PQfn() was
      even less in tune with reality, and the code itself used parameter names
      matching neither the SGML docs nor its libpq-fe.h declaration.  Do a bit
      of additional wordsmithing on the SGML docs while at it.
      
      Since the business about 1-byte integers is a clear documentation bug,
      back-patch to all supported branches.
      ae67e81e
  27. Mar 06, 2015
  28. Mar 05, 2015
    • Alvaro Herrera's avatar
      Fix user mapping object description · e166e644
      Alvaro Herrera authored
      We were using "user mapping for user XYZ" as description for user mappings, but
      that's ambiguous because users can have mappings on multiple foreign
      servers; therefore change it to "for user XYZ on server UVW" instead.
      Object identities for user mappings are also updated in the same way, in
      branches 9.3 and above.
      
      The incomplete description string was introduced together with the whole
      SQL/MED infrastructure by commit cae565e5 of 8.4 era, so backpatch all
      the way back.
      e166e644
  29. Mar 02, 2015
    • Stephen Frost's avatar
      Fix pg_dump handling of extension config tables · d13bbfab
      Stephen Frost authored
      Since 9.1, we've provided extensions with a way to denote
      "configuration" tables- tables created by an extension which the user
      may modify.  By marking these as "configuration" tables, the extension
      is asking for the data in these tables to be pg_dump'd (tables which
      are not marked in this way are assumed to be entirely handled during
      CREATE EXTENSION and are not included at all in a pg_dump).
      
      Unfortunately, pg_dump neglected to consider foreign key relationships
      between extension configuration tables and therefore could end up
      trying to reload the data in an order which would cause FK violations.
      
      This patch teaches pg_dump about these dependencies, so that the data
      dumped out is done so in the best order possible.  Note that there's no
      way to handle circular dependencies, but those have yet to be seen in
      the wild.
      
      The release notes for this should include a caution to users that
      existing pg_dump-based backups may be invalid due to this issue.  The
      data is all there, but restoring from it will require extracting the
      data for the configuration tables and then loading them in the correct
      order by hand.
      
      Discussed initially back in bug #6738, more recently brought up by
      Gilles Darold, who provided an initial patch which was further reworked
      by Michael Paquier.  Further modifications and documentation updates
      by me.
      
      Back-patch to 9.1 where we added the concept of extension configuration
      tables.
      d13bbfab
  30. Mar 01, 2015
    • Noah Misch's avatar
      Unlink static libraries before rebuilding them. · c3b0baf9
      Noah Misch authored
      When the library already exists in the build directory, "ar" preserves
      members not named on its command line.  This mattered when, for example,
      a "configure" rerun dropped a file from $(LIBOBJS).  libpgport carried
      the obsolete member until "make clean".  Back-patch to 9.0 (all
      supported versions).
      c3b0baf9
  31. Feb 28, 2015
    • Tom Lane's avatar
      Fix planning of star-schema-style queries. · 6f419958
      Tom Lane authored
      Part of the intent of the parameterized-path mechanism was to handle
      star-schema queries efficiently, but some overly-restrictive search
      limiting logic added in commit e2fa76d8
      prevented such cases from working as desired.  Fix that and add a
      regression test about it.  Per gripe from Marc Cousin.
      
      This is arguably a bug rather than a new feature, so back-patch to 9.2
      where parameterized paths were introduced.
      6f419958
  32. Feb 26, 2015
    • Andres Freund's avatar
      Reconsider when to wait for WAL flushes/syncrep during commit. · d6707652
      Andres Freund authored
      Up to now RecordTransactionCommit() waited for WAL to be flushed (if
      synchronous_commit != off) and to be synchronously replicated (if
      enabled), even if a transaction did not have a xid assigned. The primary
      reason for that is that sequence's nextval() did not assign a xid, but
      are worthwhile to wait for on commit.
      
      This can be problematic because sometimes read only transactions do
      write WAL, e.g. HOT page prune records. That then could lead to read only
      transactions having to wait during commit. Not something people expect
      in a read only transaction.
      
      This lead to such strange symptoms as backends being seemingly stuck
      during connection establishment when all synchronous replicas are
      down. Especially annoying when said stuck connection is the standby
      trying to reconnect to allow syncrep again...
      
      This behavior also is involved in a rather complicated <= 9.4 bug where
      the transaction started by catchup interrupt processing waited for
      syncrep using latches, but didn't get the wakeup because it was already
      running inside the same overloaded signal handler. Fix the issue here
      doesn't properly solve that issue, merely papers over the problems. In
      9.5 catchup interrupts aren't processed out of signal handlers anymore.
      
      To fix all this, make nextval() acquire a top level xid, and only wait for
      transaction commit if a transaction both acquired a xid and emitted WAL
      records.  If only a xid has been assigned we don't uselessly want to
      wait just because of writes to temporary/unlogged tables; if only WAL
      has been written we don't want to wait just because of HOT prunes.
      
      The xid assignment in nextval() is unlikely to cause overhead in
      real-world workloads. For one it only happens SEQ_LOG_VALS/32 values
      anyway, for another only usage of nextval() without using the result in
      an insert or similar is affected.
      
      Discussion: 20150223165359.GF30784@awork2.anarazel.de,
          369698E947874884A77849D8FE3680C2@maumau,
          5CF4ABBA67674088B3941894E22A0D25@maumau
      
      Per complaint from maumau and Thom Brown
      
      Backpatch all the way back; 9.0 doesn't have syncrep, but it seems
      better to be consistent behavior across all maintained branches.
      d6707652
    • Noah Misch's avatar
      Free SQLSTATE and SQLERRM no earlier than other PL/pgSQL variables. · d7083cc5
      Noah Misch authored
      "RETURN SQLERRM" prompted plpgsql_exec_function() to read from freed
      memory.  Back-patch to 9.0 (all supported versions).  Little code ran
      between the premature free and the read, so non-assert builds are
      unlikely to witness user-visible consequences.
      d7083cc5
  33. Feb 25, 2015
    • Tom Lane's avatar
      Fix dumping of views that are just VALUES(...) but have column aliases. · be8801e9
      Tom Lane authored
      The "simple" path for printing VALUES clauses doesn't work if we need
      to attach nondefault column aliases, because there's noplace to do that
      in the minimal VALUES() syntax.  So modify get_simple_values_rte() to
      detect nondefault aliases and treat that as a non-simple case.  This
      further exposes that the "non-simple" path never actually worked;
      it didn't produce valid syntax.  Fix that too.  Per bug #12789 from
      Curtis McEnroe, and analysis by Andrew Gierth.
      
      Back-patch to all supported branches.  Before 9.3, this also requires
      back-patching the part of commit 092d7ded
      that created get_simple_values_rte() to begin with; inserting the extra
      test into the old factorization of that logic would've been too messy.
      be8801e9
  34. Feb 23, 2015
    • Andres Freund's avatar
      Guard against spurious signals in LockBufferForCleanup. · c76e6dd7
      Andres Freund authored
      When LockBufferForCleanup() has to wait for getting a cleanup lock on a
      buffer it does so by setting a flag in the buffer header and then wait
      for other backends to signal it using ProcWaitForSignal().
      Unfortunately LockBufferForCleanup() missed that ProcWaitForSignal() can
      return for other reasons than the signal it is hoping for. If such a
      spurious signal arrives the wait flags on the buffer header will still
      be set. That then triggers "ERROR: multiple backends attempting to wait
      for pincount 1".
      
      The fix is simple, unset the flag if still set when retrying. That
      implies an additional spinlock acquisition/release, but that's unlikely
      to matter given the cost of waiting for a cleanup lock.  Alternatively
      it'd have been possible to move responsibility for maintaining the
      relevant flag to the waiter all together, but that might have had
      negative consequences due to possible floods of signals. Besides being
      more invasive.
      
      This looks to be a very longstanding bug. The relevant code in
      LockBufferForCleanup() hasn't changed materially since its introduction
      and ProcWaitForSignal() was documented to return for unrelated reasons
      since 8.2.  The master only patch series removing ImmediateInterruptOK
      made it much easier to hit though, as ProcSendSignal/ProcWaitForSignal
      now uses a latch shared with other tasks.
      
      Per discussion with Kevin Grittner, Tom Lane and me.
      
      Backpatch to all supported branches.
      
      Discussion: 11553.1423805224@sss.pgh.pa.us
      c76e6dd7
Loading