1. 16 Oct, 2015 19 commits
    • Robert Haas's avatar
      Allow a parallel context to relaunch workers. · b0b0d84b
      Robert Haas authored
      This may allow some callers to avoid the overhead involved in tearing
      down a parallel context and then setting up a new one, which means
      releasing the DSM and then allocating and populating a new one.  I
      suspect we'll want to revise the Gather node to make use of this new
      capability, but even if not it may be useful elsewhere and requires
      very little additional code.
      b0b0d84b
    • Tom Lane's avatar
      Miscellaneous cleanup of regular-expression compiler. · afdfcd3f
      Tom Lane authored
      Revert our previous addition of "all" flags to copyins() and copyouts();
      they're no longer needed, and were never anything but an unsightly hack.
      
      Improve a couple of infelicities in the REG_DEBUG code for dumping
      the NFA data structure, including adding code to count the total
      number of states and arcs.
      
      Add a couple of missed error checks.
      
      Add some more documentation in the README file, and some regression tests
      illustrating cases that exceeded the state-count limit and/or took
      unreasonable amounts of time before this set of patches.
      
      Back-patch to all supported branches.
      afdfcd3f
    • Tom Lane's avatar
      Improve memory-usage accounting in regular-expression compiler. · 538b3b8b
      Tom Lane authored
      This code previously counted the number of NFA states it created, and
      complained if a limit was exceeded, so as to prevent bizarre regex patterns
      from consuming unreasonable time or memory.  That's fine as far as it went,
      but the code paid no attention to how many arcs linked those states.  Since
      regexes can be contrived that have O(N) states but will need O(N^2) arcs
      after fixempties() processing, it was still possible to blow out memory,
      and take a long time doing it too.  To fix, modify the bookkeeping to count
      space used by both states and arcs.
      
      I did not bother with including the "color map" in the accounting; it
      can only grow to a few megabytes, which is not a lot in comparison to
      what we're allowing for states+arcs (about 150MB on 64-bit machines
      or half that on 32-bit machines).
      
      Looking at some of the larger real-world regexes captured in the Tcl
      regression test suite suggests that the most that is likely to be needed
      for regexes found in the wild is under 10MB, so I believe that the current
      limit has enough headroom to make it okay to keep it as a hard-wired limit.
      
      In connection with this, redefine REG_ETOOBIG as meaning "regular
      expression is too complex"; the previous wording of "nfa has too many
      states" was already somewhat inapropos because of the error code's use
      for stack depth overrun, and it was not very user-friendly either.
      
      Back-patch to all supported branches.
      538b3b8b
    • Tom Lane's avatar
      Improve performance of pullback/pushfwd in regular-expression compiler. · 6a715366
      Tom Lane authored
      The previous coding would create a new intermediate state every time it
      wanted to interchange the ordering of two constraint arcs.  Certain regex
      features such as \Y can generate large numbers of parallel constraint arcs,
      and if we needed to reorder the results of that, we created unreasonable
      numbers of intermediate states.  To improve matters, keep a list of
      already-created intermediate states associated with the state currently
      being considered by the outer loop; we can re-use such states to place all
      the new arcs leading to the same destination or source.
      
      I also took the trouble to redefine push() and pull() to have a less risky
      API: they no longer delete any state or arc that the caller might possibly
      have a pointer to, except for the specifically-passed constraint arc.
      This reduces the risk of re-introducing the same type of error seen in
      the failed patch for CVE-2007-4772.
      
      Back-patch to all supported branches.
      6a715366
    • Tom Lane's avatar
      Improve performance of fixempties() pass in regular-expression compiler. · f5b7d103
      Tom Lane authored
      The previous coding took something like O(N^4) time to fully process a
      chain of N EMPTY arcs.  We can't really do much better than O(N^2) because
      we have to insert about that many arcs, but we can do lots better than
      what's there now.  The win comes partly from using mergeins() to amortize
      de-duplication of arcs across multiple source states, and partly from
      exploiting knowledge of the ordering of arcs for each state to avoid
      looking at arcs we don't need to consider during the scan.  We do have
      to be a bit careful of the possible reordering of arcs introduced by
      the sort-merge coding of the previous commit, but that's not hard to
      deal with.
      
      Back-patch to all supported branches.
      f5b7d103
    • Tom Lane's avatar
      Fix O(N^2) performance problems in regular-expression compiler. · 579840ca
      Tom Lane authored
      Change the singly-linked in-arc and out-arc lists to be doubly-linked,
      so that arc deletion is constant time rather than having worst-case time
      proportional to the number of other arcs on the connected states.
      
      Modify the bulk arc transfer operations copyins(), copyouts(), moveins(),
      moveouts() so that they use a sort-and-merge algorithm whenever there's
      more than a small number of arcs to be copied or moved.  The previous
      method is O(N^2) in the number of arcs involved, because it performs
      duplicate checking independently for each copied arc.  The new method may
      change the ordering of existing arcs for the destination state, but nothing
      really cares about that.
      
      Provide another bulk arc copying method mergeins(), which is unused as
      of this commit but is needed for the next one.  It basically is like
      copyins(), but the source arcs might not all come from the same state.
      
      Replace the O(N^2) bubble-sort algorithm used in carcsort() with a qsort()
      call.
      
      These changes greatly improve the performance of regex compilation for
      large or complex regexes, at the cost of extra space for arc storage during
      compilation.  The original tradeoff was probably fine when it was made, but
      now we care more about speed and less about memory consumption.
      
      Back-patch to all supported branches.
      579840ca
    • Tom Lane's avatar
      Fix regular-expression compiler to handle loops of constraint arcs. · 48789c5d
      Tom Lane authored
      It's possible to construct regular expressions that contain loops of
      constraint arcs (that is, ^ $ AHEAD BEHIND or LACON arcs).  There's no use
      in fully traversing such a loop at execution, since you'd just end up in
      the same NFA state without having consumed any input.  Worse, such a loop
      leads to infinite looping in the pullback/pushfwd stage of compilation,
      because we keep pushing or pulling the same constraints around the loop
      in a vain attempt to move them to the pre or post state.  Such looping was
      previously recognized in CVE-2007-4772; but the fix only handled the case
      of trivial single-state loops (that is, a constraint arc leading back to
      its source state) ... and not only that, it was incorrect even for that
      case, because it broke the admittedly-not-very-clearly-stated API contract
      of the pull() and push() subroutines.  The first two regression test cases
      added by this commit exhibit patterns that result in assertion failures
      because of that (though there seem to be no ill effects in non-assert
      builds).  The other new test cases exhibit multi-state constraint loops;
      in an unpatched build they will run until the NFA state-count limit is
      exceeded.
      
      To fix, remove the code added for CVE-2007-4772, and instead create a
      general-purpose constraint-loop-breaking phase of regex compilation that
      executes before we do pullback/pushfwd.  Since we never need to traverse
      a constraint loop fully, we can just break the loop at any chosen spot,
      if we add clone states that can replicate any sequence of arc transitions
      that would've traversed just part of the loop.
      
      Also add some commentary clarifying why we have to have all these
      machinations in the first place.
      
      This class of problems has been known for some time --- we had a report
      from Marc Mamin about two years ago, for example, and there are related
      complaints in the Tcl bug tracker.  I had discussed a fix of this kind
      off-list with Henry Spencer, but didn't get around to doing something
      about it until the issue was rediscovered by Greg Stark recently.
      
      Back-patch to all supported branches.
      48789c5d
    • Robert Haas's avatar
      Remove volatile qualifiers from proc.c and procarray.c · d53e3d5f
      Robert Haas authored
      Prior to commit 0709b7ee, access to
      variables within a spinlock-protected critical section had to be done
      through a volatile pointer, but that should no longer be necessary.
      
      Michael Paquier
      d53e3d5f
    • Robert Haas's avatar
      Remove volatile qualifiers from dynahash.c, shmem.c, and sinvaladt.c · 430008b5
      Robert Haas authored
      Prior to commit 0709b7ee, access to
      variables within a spinlock-protected critical section had to be done
      through a volatile pointer, but that should no longer be necessary.
      
      Thomas Munro
      430008b5
    • Robert Haas's avatar
      Remove cautions about using volatile from spin.h. · 78652a33
      Robert Haas authored
      Commit 0709b7ee obsoleted this comment
      but neglected to update it.
      
      Thomas Munro
      78652a33
    • Robert Haas's avatar
      Prohibit parallel query when the isolation level is serializable. · a53c06a1
      Robert Haas authored
      In order for this to be safe, the code which hands true serializability
      will need to taught that the SIRead locks taken by a parallel worker
      pertain to the same transaction as those taken by the parallel leader.
      Some further changes may be needed as well.  Until the necessary
      adaptations are made, don't generate parallel plans in serializable
      mode, and if a previously-generated parallel plan is used after
      serializable mode has been activated, run it serially.
      
      This fixes a bug in commit 7aea8e4f.
      a53c06a1
    • Robert Haas's avatar
      Rewrite interaction of parallel mode with parallel executor support. · bfc78d71
      Robert Haas authored
      In the previous coding, before returning from ExecutorRun, we'd shut
      down all parallel workers.  This was dead wrong if ExecutorRun was
      called with a non-zero tuple count; it had the effect of truncating
      the query output.  To fix, give ExecutePlan control over whether to
      enter parallel mode, and have it refuse to do so if the tuple count
      is non-zero.  Rewrite the Gather logic so that it can cope with being
      called outside parallel mode.
      
      Commit 7aea8e4f is largely to blame
      for this problem, though this patch modifies some subsequently-committed
      code which relied on the guarantees it purported to make.
      bfc78d71
    • Robert Haas's avatar
      Mark more functions parallel-restricted or parallel-unsafe. · 816e336f
      Robert Haas authored
      Commit 7aea8e4f was overoptimistic
      about the degree of safety associated with running various functions
      in parallel mode.  Functions that take a table name or OID as an
      argument are at least parallel-restricted, because the table might be
      temporary, and we currently don't allow parallel workers to touch
      temporary tables.  Functions that take a query as an argument are
      outright unsafe, because the query could be anything, including a
      parallel-unsafe query.
      
      Also, the queue of pending notifications is backend-private, so adding
      to it from a worker doesn't behave correctly.  We could fix this by
      transferring the worker's queue of pending notifications to the master
      during worker cleanup, but that seems like more trouble than it's
      worth for now.  In addition to adjusting the pg_proc.h markings, also
      add an explicit check for this in async.c.
      816e336f
    • Robert Haas's avatar
      Fix a problem with parallel workers being unable to restore role. · 82b37765
      Robert Haas authored
      check_role() tries to verify that the user has permission to become the
      requested role, but this is inappropriate in a parallel worker, which
      needs to exactly recreate the master's authorization settings.  So skip
      the check in that case.
      
      This fixes a bug in commit 924bcf4f.
      82b37765
    • Robert Haas's avatar
      Invalidate caches after cranking up a parallel worker transaction. · 6de6d96d
      Robert Haas authored
      Starting a parallel worker transaction changes our notion of which XIDs
      are in-progress or committed, and our notion of the current command
      counter ID.  Therefore, our view of these caches prior to starting
      this transaction may no longer valid.  Defend against that by clearing
      them.
      
      This fixes a bug in commit 924bcf4f.
      6de6d96d
    • Michael Meskes's avatar
    • Robert Haas's avatar
      Tighten up application of parallel mode checks. · 94b4f7e2
      Robert Haas authored
      Commit 924bcf4f failed to enforce
      parallel mode checks during the commit of a parallel worker, because
      we exited parallel mode prior to ending the transaction so that we
      could pop the active snapshot.  Re-establish parallel mode during
      parallel worker commit.  Without this, it's far too easy for unsafe
      actions during the pre-commit sequence to crash the server instead of
      hitting the error checks as intended.
      
      Just to be extra paranoid, adjust a couple of the sanity checks in
      xact.c to check not only IsInParallelMode() but also
      IsParallelWorker().
      94b4f7e2
    • Robert Haas's avatar
      Transfer current command counter ID to parallel workers. · 423ec087
      Robert Haas authored
      Commit 924bcf4f correctly forbade
      parallel workers to modify the command counter while in parallel mode,
      but it inexplicably neglected to actually transfer the current command
      counter from leader to workers.  This can result in the workers seeing
      a different set of tuples from the leader, which is bad.  Repair.
      423ec087
    • Robert Haas's avatar
      Don't send protocol messages to a shm_mq that no longer exists. · 2ad5c27b
      Robert Haas authored
      Commit 2bd9e412 introduced a mechanism
      for relaying protocol messages from a background worker to another
      backend via a shm_mq.  However, there was no provision for shutting
      down the communication channel.  Therefore, a protocol message sent
      late in the shutdown sequence, such as a DEBUG message resulting from
      cranking up log_min_messages, could crash the server.  To fix, install
      an on_dsm_detach callback that disables sending messages to the shm_mq
      when the associated DSM is detached.
      2ad5c27b
  2. 15 Oct, 2015 4 commits
    • Tom Lane's avatar
      Fix NULL handling in datum_to_jsonb(). · 3587cbc3
      Tom Lane authored
      The function failed to adhere to its specification that the "tcategory"
      argument should not be examined when the input value is NULL.  This
      resulted in a crash in some cases.  Per bug #13680 from Boyko Yordanov.
      
      In passing, re-pgindent some recent changes in jsonb.c, and fix a rather
      ungrammatical comment.
      
      Diagnosis and patch by Michael Paquier, cosmetic changes by me
      3587cbc3
    • Robert Haas's avatar
      Revert "Have dtrace depend on object files directly, not objfiles.txt" · 08fbad0a
      Robert Haas authored
      This reverts commit 73537828.  Per
      report from Tom Lane, this breaks parallel builds.
      08fbad0a
    • Robert Haas's avatar
      Allow FDWs to push down quals without breaking EvalPlanQual rechecks. · 5fc4c26d
      Robert Haas authored
      This fixes a long-standing bug which was discovered while investigating
      the interaction between the new join pushdown code and the EvalPlanQual
      machinery: if a ForeignScan appears on the inner side of a paramaterized
      nestloop, an EPQ recheck would re-return the original tuple even if
      it no longer satisfied the pushed-down quals due to changed parameter
      values.
      
      This fix adds a new member to ForeignScan and ForeignScanState and a
      new argument to make_foreignscan, and requires changes to FDWs which
      push down quals to populate that new argument with a list of quals they
      have chosen to push down.  Therefore, I'm only back-patching to 9.5,
      even though the bug is not new in 9.5.
      
      Etsuro Fujita, reviewed by me and by Kyotaro Horiguchi.
      5fc4c26d
    • Alvaro Herrera's avatar
      Fix bogus comments · 817588bc
      Alvaro Herrera authored
      Author: Amit Langote
      817588bc
  3. 13 Oct, 2015 4 commits
    • Bruce Momjian's avatar
      -- email subject limit ----------------------------------------- · aa7f9493
      Bruce Momjian authored
      -- gitweb summary limit --------------------------
      pg_upgrade:  reorder controldata checks to match program output
      
      Also improve comment for how float8_pass_by_value is used.
      
      Backpatch through 9.5
      aa7f9493
    • Robert Haas's avatar
      Have dtrace depend on object files directly, not objfiles.txt · 73537828
      Robert Haas authored
      Per Mark Johnston, this resolves a build error on FreeBSD related
      to the fact that dtrace is modifying the generated object files
      under the hood.  Consequently, without this, dtrace gets reinvoked
      at install time because the object files have been updated.  This
      is a pretty hacky fix, but it shouldn't hurt anything, and it's
      not clear that it's worth expending any more effort for a feature
      that not too many people are using.
      
      Patch by Mark Johnston.  This is arguably back-patchable as a bug
      fix to the build system, but I'm not certain enough of the
      consequences to try that.  Let's see what the buildfarm (and
      our packagers) think of this change on master first.
      73537828
    • Robert Haas's avatar
      Improve INSERT .. ON CONFLICT error message. · b8dd19af
      Robert Haas authored
      Peter Geoghegan, reviewed by me.
      b8dd19af
    • Tom Lane's avatar
      On Windows, ensure shared memory handle gets closed if not being used. · 869f693a
      Tom Lane authored
      Postmaster child processes that aren't supposed to be attached to shared
      memory were not bothering to close the shared memory mapping handle they
      inherit from the postmaster process.  That's mostly harmless, since the
      handle vanishes anyway when the child process exits -- but the syslogger
      process, if used, doesn't get killed and restarted during recovery from a
      backend crash.  That meant that Windows doesn't see the shared memory
      mapping as becoming free, so it doesn't delete it and the postmaster is
      unable to create a new one, resulting in failure to recover from crashes
      whenever logging_collector is turned on.
      
      Per report from Dmitry Vasilyev.  It's a bit astonishing that we'd not
      figured this out long ago, since it's been broken from the very beginnings
      of out native Windows support; probably some previously-unexplained trouble
      reports trace to this.
      
      A secondary problem is that on Cygwin (perhaps only in older versions?),
      exec() may not detach from the shared memory segment after all, in which
      case these child processes did remain attached to shared memory, posing
      the risk of an unexpected shared memory clobber if they went off the rails
      somehow.  That may be a long-gone bug, but we can deal with it now if it's
      still live, by detaching within the infrastructure introduced here to deal
      with closing the handle.
      
      Back-patch to all supported branches.
      
      Tom Lane and Amit Kapila
      869f693a
  4. 12 Oct, 2015 5 commits
    • Tom Lane's avatar
      Fix "pg_ctl start -w" to test child process status directly. · 6bcce258
      Tom Lane authored
      pg_ctl start with -w previously relied on a heuristic that the postmaster
      would surely always manage to create postmaster.pid within five seconds.
      Unfortunately, that fails much more often than we would like on some of the
      slower, more heavily loaded buildfarm members.
      
      We have known for quite some time that we could remove the need for that
      heuristic on Unix by using fork/exec instead of system() to launch the
      postmaster.  This allows us to know the exact PID of the postmaster, which
      allows near-certain verification that the postmaster.pid file is the one
      we want and not a leftover, and it also lets us use waitpid() to detect
      reliably whether the child postmaster has exited or not.
      
      What was blocking this change was not wanting to rewrite the Windows
      version of start_postmaster() to avoid use of CMD.EXE.  That's doable
      in theory but would require fooling about with stdout/stderr redirection,
      and getting the handling of quote-containing postmaster switches to
      stay the same might be rather ticklish.  However, we realized that
      we don't have to do that to fix the problem, because we can test
      whether the shell process has exited as a proxy for whether the
      postmaster is still alive.  That doesn't allow an exact check of the
      PID in postmaster.pid, but we're no worse off than before in that
      respect; and we do get to get rid of the heuristic about how long the
      postmaster might take to create postmaster.pid.
      
      On Unix, this change means that a second "pg_ctl start -w" immediately
      after another such command will now reliably fail, whereas previously
      it would succeed if done within two seconds of the earlier command.
      Since that's a saner behavior anyway, it's fine.  On Windows, the case can
      still succeed within the same time window, since pg_ctl can't tell that the
      earlier postmaster's postmaster.pid isn't the pidfile it is looking for.
      To ensure stable test results on Windows, we can insert a short sleep into
      the test script for pg_ctl, ensuring that the existing pidfile looks stale.
      This hack can be removed if we ever do rewrite start_postmaster(), but that
      no longer seems like a high-priority thing to do.
      
      Back-patch to all supported versions, both because the current behavior
      is buggy and because we must do that if we want the buildfarm failures
      to go away.
      
      Tom Lane and Michael Paquier
      6bcce258
    • Noah Misch's avatar
      Use JsonbIteratorToken consistently in automatic variable declarations. · 7732d49c
      Noah Misch authored
      Many functions stored JsonbIteratorToken values in variables of other
      integer types.  Also, standardize order relative to other declarations.
      Expect compilers to generate the same code before and after this change.
      7732d49c
    • Peter Eisentraut's avatar
      Fix whitespace · f20b2696
      Peter Eisentraut authored
      f20b2696
    • Noah Misch's avatar
      Avoid scan-build warning about uninitialized htonl() arguments. · dfa1cddc
      Noah Misch authored
      Josh Kupershmidt
      dfa1cddc
    • Noah Misch's avatar
      Make prove_installcheck remove the old log directory, if any. · 03a22f8b
      Noah Misch authored
      prove_check already has been doing this.  Back-patch to 9.4, like the
      commit that introduced this logging.
      03a22f8b
  5. 09 Oct, 2015 5 commits
    • Robert Haas's avatar
      Speed up text sorts where the same strings occur multiple times. · 0e57b4d8
      Robert Haas authored
      Cache strxfrm() blobs across calls made to the text SortSupport
      abbreviation routine.  This can speed up sorting if the same string
      needs to be abbreviated many times in a row.
      
      Also, cache the result of the previous strcoll() comparison, so that
      if we're asked to compare the same strings agin, we do need to call
      strcoll() again.
      
      Perhaps surprisingly, these optimizations don't seem to hurt even when
      they don't help.  memcmp() is really cheap compared to strcoll() or
      strxfrm().
      
      Peter Geoghegan, reviewed by me.
      0e57b4d8
    • Robert Haas's avatar
      Make abbreviated key comparisons for text a bit cheaper. · bfb54ff1
      Robert Haas authored
      If we do some byte-swapping while abbreviating, we can do comparisons
      using integer arithmetic rather than memcmp.
      
      Peter Geoghegan, reviewed and slightly revised by me.
      bfb54ff1
    • Robert Haas's avatar
      Remove set_latch_on_sigusr1 flag. · db0f6cad
      Robert Haas authored
      This flag has proven to be a recipe for bugs, and it doesn't seem like
      it can really buy anything in terms of performance.  So let's just
      *always* set the process latch when we receive SIGUSR1 instead of
      trying to do it only when needed.
      
      Per my recent proposal on pgsql-hackers.
      db0f6cad
    • Stephen Frost's avatar
      Handle append_rel_list in expand_security_qual · b7aac362
      Stephen Frost authored
      During expand_security_quals, we take the security barrier quals on an
      RTE and create a subquery which evaluates the quals.  During this, we
      have to replace any variables in the outer query which refer to the
      original RTE with references to the columns from the subquery.
      
      We need to also perform that replacement for any Vars in the
      append_rel_list.
      
      Only backpatching to 9.5 as we only go through this process in 9.4 for
      auto-updatable security barrier views, which UNION ALL queries aren't.
      
      Discovered by Haribabu Kommi
      
      Patch by Dean Rasheed
      b7aac362
    • Tom Lane's avatar
      Fix uninitialized-variable bug. · 94f5246c
      Tom Lane authored
      For some reason, neither of the compilers I usually use noticed the
      uninitialized-variable problem I introduced in commit 7e2a18a9.
      That's hardly a good enough excuse though.  Committing with brown paper bag
      on head.
      
      In addition to putting the operations in the right order, move the
      declaration of "now" inside the loop; there's no need for it to be
      outside, and that does wake up older gcc enough to notice any similar
      future problem.
      
      Back-patch to 9.4; earlier versions lack the time-to-SIGKILL stanza
      so there's no bug.
      94f5246c
  6. 08 Oct, 2015 3 commits
    • Robert Haas's avatar
      Fix typo in docs. · 41562b14
      Robert Haas authored
      Pallavi Sontakke
      41562b14
    • Robert Haas's avatar
      Add BSWAP64 macro. · c171818b
      Robert Haas authored
      This is like BSWAP32, but for 64-bit values.  Since we've got two of
      them now and they have use cases (like sortsupport) beyond CRCs, move
      the definitions to their own header file.
      
      Peter Geoghegan
      c171818b
    • Robert Haas's avatar
      Hyphenate variable-length for consistency. · 1e353198
      Robert Haas authored
      We hyphenate "fixed-length" earlier in the same sentence, and overall we
      more often use "variable-length" rather than "variable length".
      
      Nikolay Shaplov
      1e353198