1. 17 Mar, 2014 1 commit
    • Tom Lane's avatar
      Fix advertised dispsize for libpq's sslmode connection parameter. · f4051e36
      Tom Lane authored
      "8" was correct back when "disable" was the longest allowed value, but
      since "verify-full" was added, it should be "12".  Given the lack of
      complaints, I wouldn't be surprised if nobody is actually using these
      values ... but still, if they're in the API, they should be right.
      
      Noticed while pursuing a different problem.  It's been wrong for quite
      a long time, so back-patch to all supported branches.
      f4051e36
  2. 16 Mar, 2014 1 commit
    • Magnus Hagander's avatar
      Cleanups from the remove-native-krb5 patch · 0294023a
      Magnus Hagander authored
      krb_srvname is actually not available anymore as a parameter server-side, since
      with gssapi we accept all principals in our keytab. It's still used in libpq for
      client side specification.
      
      In passing remove declaration of krb_server_hostname, where all the functionality
      was already removed.
      
      Noted by Stephen Frost, though a different solution than his suggestion
      0294023a
  3. 15 Mar, 2014 2 commits
  4. 14 Mar, 2014 2 commits
    • Heikki Linnakangas's avatar
      Fix race condition in B-tree page deletion. · efada2b8
      Heikki Linnakangas authored
      In short, we don't allow a page to be deleted if it's the rightmost child
      of its parent, but that situation can change after we check for it.
      
      Problem
      -------
      
      We check that the page to be deleted is not the rightmost child of its
      parent, and then lock its left sibling, the page itself, its right sibling,
      and the parent, in that order. However, if the parent page is split after
      the check but before acquiring the locks, the target page might become the
      rightmost child, if the split happens at the right place. That leads to an
      error in vacuum (I reproduced this by setting a breakpoint in debugger):
      
      ERROR:  failed to delete rightmost child 41 of block 3 in index "foo_pkey"
      
      We currently re-check that the page is still the rightmost child, and throw
      the above error if it's not. We could easily just give up rather than throw
      an error, but that approach doesn't scale to half-dead pages. To recap,
      although we don't normally allow deleting the rightmost child, if the page
      is the *only* child of its parent, we delete the child page and mark the
      parent page as half-dead in one atomic operation. But before we do that, we
      check that the parent can later be deleted, by checking that it in turn is
      not the rightmost child of the grandparent (potentially recursing all the
      way up to the root). But the same situation can arise there - the
      grandparent can be split while we're not holding the locks. We end up with
      a half-dead page that we cannot delete.
      
      To make things worse, the keyspace of the deleted page has already been
      transferred to its right sibling. As the README points out, the keyspace at
      the grandparent level is "out-of-whack" until the half-dead page is deleted,
      and if enough tuples with keys in the transferred keyspace are inserted, the
      page might get split and a downlink might be inserted into the grandparent
      that is out-of-order. That might not cause any serious problem if it's
      transient (as the README ponders), but is surely bad if it stays that way.
      
      Solution
      --------
      
      This patch changes the page deletion algorithm to avoid that problem. After
      checking that the topmost page in the chain of to-be-deleted pages is not
      the rightmost child of its parent, and then deleting the pages from bottom
      up, unlink the pages from top to bottom. This way, the intermediate stages
      are similar to the intermediate stages in page splitting, and there is no
      transient stage where the keyspace is "out-of-whack". The topmost page in
      the to-be-deleted chain doesn't have a downlink pointing to it, like a page
      split before the downlink has been inserted.
      
      This also allows us to get rid of the cleanup step after WAL recovery, if we
      crash during page deletion. The deletion will be continued at next VACUUM,
      but the tree is consistent for searches and insertions at every step.
      
      This bug is old, all supported versions are affected, but this patch is too
      big to back-patch (and changes the WAL record formats of related records).
      We have not heard any reports of the bug from users, so clearly it's not
      easy to bump into. Maybe backpatch later, after this has had some field
      testing.
      
      Reviewed by Kevin Grittner and Peter Geoghegan.
      efada2b8
    • Tom Lane's avatar
      Prevent interrupts while reporting non-ERROR elog messages. · 6c461cb9
      Tom Lane authored
      This should eliminate the risk of recursive entry to syslog(3), which
      appears to be the cause of the hang reported in bug #9551 from James
      Morton.
      
      Arguably, the real problem here is auth.c's willingness to turn on
      ImmediateInterruptOK while executing fairly wide swaths of backend code.
      We may well need to work at narrowing the code ranges in which the
      authentication_timeout interrupt is enabled.  For the moment, though,
      this is a cheap and reasonably noninvasive fix for a field-reported
      failure; the other approach would be complex and not necessarily
      bug-free itself.
      
      Back-patch to all supported branches.
      6c461cb9
  5. 13 Mar, 2014 5 commits
    • Tom Lane's avatar
      Allow psql to print COPY command status in more cases. · f70a78bc
      Tom Lane authored
      Previously, psql would print the "COPY nnn" command status only for COPY
      commands executed server-side.  Now it will print that for frontend copies
      too (including \copy).  However, we continue to suppress the command status
      for COPY TO STDOUT, since in that case the copy data has been routed to the
      same place that the command status would go, and there is a risk of the
      status line being mistaken for another line of COPY data.  Doing that would
      break existing scripts, and it doesn't seem worth the benefit --- this case
      seems fairly analogous to SELECT, for which we also suppress the command
      status.
      
      Kumar Rajeev Rastogi, with substantial review by Amit Khandekar
      f70a78bc
    • Tom Lane's avatar
      Avoid transaction-commit race condition while receiving a NOTIFY message. · 7bae0284
      Tom Lane authored
      Use TransactionIdIsInProgress, then TransactionIdDidCommit, to distinguish
      whether a NOTIFY message's originating transaction is in progress,
      committed, or aborted.  The previous coding could accept a message from a
      transaction that was still in-progress according to the PGPROC array;
      if the client were fast enough at starting a new transaction, it might fail
      to see table rows added/updated by the message-sending transaction.  Which
      of course would usually be the point of receiving the message.  We noted
      this type of race condition long ago in tqual.c, but async.c overlooked it.
      
      The race condition probably cannot occur unless there are multiple NOTIFY
      senders in action, since an individual backend doesn't send NOTIFY signals
      until well after it's done committing.  But if two senders commit in close
      succession, it's certainly possible that we could see the second sender's
      message within the race condition window while responding to the signal
      from the first one.
      
      Per bug #9557 from Marko Tiikkaja.  This patch is slightly more invasive
      than what he proposed, since it removes the now-redundant
      TransactionIdDidAbort call.
      
      Back-patch to 9.0, where the current NOTIFY implementation was introduced.
      7bae0284
    • Heikki Linnakangas's avatar
      Fix a couple of typos in docs. · 16ff08b7
      Heikki Linnakangas authored
      Thom Brown
      16ff08b7
    • Bruce Momjian's avatar
      C comments: remove odd blank lines after #ifdef WIN32 lines · 242c2737
      Bruce Momjian authored
      A few more
      242c2737
    • Bruce Momjian's avatar
  6. 12 Mar, 2014 8 commits
  7. 10 Mar, 2014 4 commits
    • Tom Lane's avatar
      Fix tracking of psql script line numbers during \copy from another place. · e85a5ffb
      Tom Lane authored
      Commit 08146775 changed do_copy() to
      temporarily scribble on pset.cur_cmd_source.  That was a mighty ugly bit of
      code in any case, but in particular it broke handleCopyIn's ability to tell
      whether it was reading from the current script source file (in which case
      pset.lineno should be incremented for each line of COPY data), or from
      someplace else (in which case it shouldn't).  The former case still worked,
      the latter not so much.  The visible effect was that line numbers reported
      for errors in a script file would be wrong if there were an earlier \copy
      that was reading anything other than inline-in-the-script-file data.
      
      To fix, introduce another pset field that holds the file do_copy wants the
      COPY code to use.  This is a little bit ugly, but less so than passing the
      file down explicitly through several layers that aren't COPY-specific.
      
      Extracted from a larger patch by Kumar Rajeev Rastogi; that patch also
      changes printing of COPY command tags, which is not a bug fix and shouldn't
      get back-patched.  This particular idea was from a suggestion by Amit
      Khandekar, if I'm reading the thread correctly.
      
      Back-patch to 9.2 where the faulty code was introduced.
      e85a5ffb
    • Robert Haas's avatar
      Allow dynamic shared memory segments to be kept until shutdown. · 8722017b
      Robert Haas authored
      Amit Kapila, reviewed by Kyotaro Horiguchi, with some further
      changes by me.
      8722017b
    • Robert Haas's avatar
      Allow logical decoding via the walsender interface. · 5a991ef8
      Robert Haas authored
      In order for this to work, walsenders need the optional ability to
      connect to a database, so the "replication" keyword now allows true
      or false, for backward-compatibility, and the new value "database"
      (which causes the "dbname" parameter to be respected).
      
      walsender needs to loop not only when idle but also when sending
      decoded data to the user and when waiting for more xlog data to decode.
      This means that there are now three separate loops inside walsender.c;
      although some refactoring has been done here, this is still a bit ugly.
      
      Andres Freund, with contributions from Álvaro Herrera, and further
      review by me.
      5a991ef8
    • Robert Haas's avatar
      Teach on_exit_reset() to discard pending cleanups for dsm. · cb9a0c79
      Robert Haas authored
      If a postmaster child invokes fork() and then calls on_exit_reset, that
      should be sufficient to let it exit() without breaking anything, but
      dynamic shared memory broke that by not updating on_exit_reset() to
      discard callbacks registered with dynamic shared memory segments.
      
      Per investigation of a complaint from Tom Lane.
      cb9a0c79
  8. 09 Mar, 2014 1 commit
  9. 08 Mar, 2014 7 commits
  10. 07 Mar, 2014 8 commits
    • Tom Lane's avatar
      Remove unportable use of anonymous unions from reorderbuffer.h. · ea177a3b
      Tom Lane authored
      In b89e1510 I had assumed it was ok to use anonymous unions as
      struct members, but while a longstanding extension in many compilers,
      it's only been standardized in C11.
      
      To fix, remove one of the anonymous unions which tried to hide some
      implementation specific enum values and give the other a name. The
      latter unfortunately requires changes in output plugins, but since the
      feature has only been added a few days ago...
      
      Andres Freund
      ea177a3b
    • Tom Lane's avatar
      Fix contrib/postgres_fdw to handle multiple join conditions properly. · 83204e10
      Tom Lane authored
      The previous coding supposed that it could consider just a single join
      condition in any one parameterized path for the foreign table.  But in
      reality, the parameterized-path machinery forces all join clauses that are
      "movable to" the foreign table to be evaluated at that node; including
      clauses that we might not consider safe to send across.  Such cases would
      result in an Assert failure in an assert-enabled build, and otherwise in
      sending an unsafe clause to the foreign server, which might result in
      errors or silently-wrong answers.  A lesser problem was that the
      cost/rowcount estimates generated for the parameterized path failed to
      account for any additional join quals that get assigned to the scan.
      
      To fix, rewrite postgresGetForeignPaths so that it correctly collects all
      the movable quals for any one outer relation when generating parameterized
      paths; we'll now generate just one path per outer relation not one per join
      qual.  Also fix bogus assumptions in postgresGetForeignPlan and
      estimate_path_cost_size that only safe-to-send join quals will be
      presented.
      
      Based on complaint from Etsuro Fujita that the path costs were being
      miscalculated, though this is significantly different from his proposed
      patch.
      83204e10
    • Bruce Momjian's avatar
      release notes: add item missed in 9.2.5 release · 4ea2e2d4
      Bruce Momjian authored
      Item is "Prevent errors in WAL replay due to references to uninitialized
      empty pages".
      
      Report and text by Andres Freund
      
      Backpatch through 9.2.
      4ea2e2d4
    • Bruce Momjian's avatar
      fix ReplicationSlotsCountDBSlots for dropping unrelated databases · 91d9de97
      Bruce Momjian authored
      YAMAMOTO Takashi
      91d9de97
    • Heikki Linnakangas's avatar
      Fix dangling smgr_owner pointer when a fake relcache entry is freed. · 55566c9a
      Heikki Linnakangas authored
      A fake relcache entry can "own" a SmgrRelation object, like a regular
      relcache entry. But when it was free'd, the owner field in SmgrRelation
      was not cleared, so it was left pointing to free'd memory.
      
      Amazingly this apparently hasn't caused crashes in practice, or we would've
      heard about it earlier. Andres found this with Valgrind.
      
      Report and fix by Andres Freund, with minor modifications by me. Backpatch
      to all supported versions.
      55566c9a
    • Heikki Linnakangas's avatar
      Avoid memcpy() with same source and destination address. · ad7b48ea
      Heikki Linnakangas authored
      The behavior of that is undefined, although unlikely to lead to problems in
      practice.
      
      Found by running regression tests with Valgrind.
      ad7b48ea
    • Heikki Linnakangas's avatar
      Fix name of syslog_ident GUC in docs. · 2b8483d6
      Heikki Linnakangas authored
      Michael Paquier
      2b8483d6
    • Tom Lane's avatar
      Avoid getting more than AccessShareLock when deparsing a query. · 7c318749
      Tom Lane authored
      In make_ruledef and get_query_def, we have long used AcquireRewriteLocks
      to ensure that the querytree we are about to deparse is up-to-date and
      the schemas of the underlying relations aren't changing.  Howwever, that
      function thinks the query is about to be executed, so it acquires locks
      that are stronger than necessary for the purpose of deparsing.  Thus for
      example, if pg_dump asks to deparse a rule that includes "INSERT INTO t",
      we'd acquire RowExclusiveLock on t.  That results in interference with
      concurrent transactions that might for example ask for ShareLock on t.
      Since pg_dump is documented as being purely read-only, this is unexpected.
      (Worse, it used to actually be read-only; this behavior dates back only
      to 8.1, cf commit ba420024.)
      
      Fix this by adding a parameter to AcquireRewriteLocks to tell it whether
      we want the "real" execution locks or only AccessShareLock.
      
      Report, diagnosis, and patch by Dean Rasheed.  Back-patch to all supported
      branches.
      7c318749
  11. 06 Mar, 2014 1 commit
    • Heikki Linnakangas's avatar
      isdigit() needs an unsigned char argument. · a0c2fa9b
      Heikki Linnakangas authored
      Per the C standard, the routine should be passed an int, with a value that's
      representable as an unsigned char or EOF. Passing a signed char is wrong,
      because a negative value is not representable as an unsigned char.
      Unfortunately no compiler warns about that.
      a0c2fa9b