1. 12 Sep, 2017 5 commits
  2. 11 Sep, 2017 7 commits
  3. 10 Sep, 2017 3 commits
    • Tom Lane's avatar
      Quick-hack fix for foreign key cascade vs triggers with transition tables. · 3c435952
      Tom Lane authored
      AFTER triggers using transition tables crashed if they were fired due
      to a foreign key ON CASCADE update.  This is because ExecEndModifyTable
      flushes the transition tables, on the assumption that any trigger that
      could need them was already fired during ExecutorFinish.  Normally
      that's true, because we don't allow transition-table-using triggers
      to be deferred.  However, foreign key CASCADE updates force any
      triggers on the referencing table to be deferred to the outer query
      level, by means of the EXEC_FLAG_SKIP_TRIGGERS flag.  I don't recall
      all the details of why it's like that and am pretty loath to redesign
      it right now.  Instead, just teach ExecEndModifyTable to skip destroying
      the TransitionCaptureState when that flag is set.  This will allow the
      transition table data to survive until end of the current subtransaction.
      
      This isn't a terribly satisfactory solution, because (1) we might be
      leaking the transition tables for much longer than really necessary,
      and (2) as things stand, an AFTER STATEMENT trigger will fire once per
      RI updating query, ie once per row updated or deleted in the referenced
      table.  I suspect that is not per SQL spec.  But redesigning this is a
      research project that we're certainly not going to get done for v10.
      So let's go with this hackish answer for now.
      
      In passing, tweak AfterTriggerSaveEvent to not save the transition_capture
      pointer into the event record for a deferrable trigger.  This is not
      necessary to fix the current bug, but it avoids letting dangling pointers
      to long-gone transition tables persist in the trigger event queue.  That's
      at least a safety feature.  It might also allow merging shared trigger
      states in more cases than before.
      
      I added a regression test that demonstrates the crash on unpatched code,
      and also exposes the behavior of firing the AFTER STATEMENT triggers
      once per row update.
      
      Per bug #14808 from Philippe Beaudoin.  Back-patch to v10.
      
      Discussion: https://postgr.es/m/20170909064853.25630.12825@wrigleys.postgresql.org
      3c435952
    • Tom Lane's avatar
      Add a test harness for the red-black tree code. · 610bbdd8
      Tom Lane authored
      This improves the regression tests' coverage of rbtree.c from pretty
      awful (because some of the functions aren't used yet) to basically 100%.
      
      Victor Drobny, reviewed by Aleksander Alekseev and myself
      
      Discussion: https://postgr.es/m/c9d61310e16e75f8acaf6cb1c48b7b77@postgrespro.ru
      610bbdd8
    • Tom Lane's avatar
      Remove pre-order and post-order traversal logic for red-black trees. · f80e782a
      Tom Lane authored
      This code isn't used, and there's no clear reason why anybody would ever
      want to use it.  These traversal mechanisms don't yield a visitation order
      that is semantically meaningful for any external purpose, nor are they
      any faster or simpler than the left-to-right or right-to-left traversals.
      (In fact, some rough testing suggests they are slower :-(.)  Moreover,
      these mechanisms are impossible to test in any arm's-length fashion; doing
      so requires knowledge of the red-black tree's internal implementation.
      Hence, let's just jettison them.
      
      Discussion: https://postgr.es/m/17735.1505003111@sss.pgh.pa.us
      f80e782a
  4. 09 Sep, 2017 2 commits
    • Peter Eisentraut's avatar
      pg_upgrade: Message style fixes · c824c7e2
      Peter Eisentraut authored
      c824c7e2
    • Tom Lane's avatar
      Fix failure-to-copy bug in commit 6f6b99d1. · fdf87ed4
      Tom Lane authored
      The previous coding of get_qual_for_list() was careful to copy everything
      it was using from the input data structure.  The new version missed
      making a copy of pass-by-ref datum values that it's inserting into Consts.
      This is not optional, however, as revealed by buildfarm failures on
      machines running -DRELCACHE_FORCE_RELEASE: we're copying from a relcache
      entry that could go away before the required lifespan of our output
      expression.  I'm pretty sure -DCLOBBER_CACHE_ALWAYS machines won't like
      this either, but none of them have reported in yet.
      fdf87ed4
  5. 08 Sep, 2017 14 commits
  6. 07 Sep, 2017 7 commits
    • Tom Lane's avatar
      Improve performance of get_actual_variable_range with recently-dead tuples. · 3ca930fc
      Tom Lane authored
      In commit fccebe42, we hacked get_actual_variable_range() to scan the
      index with SnapshotDirty, so that if there are many uncommitted tuples
      at the end of the index range, it wouldn't laboriously scan through all
      of them looking for a live value to return.  However, that didn't fix it
      for the case of many recently-dead tuples at the end of the index;
      SnapshotDirty recognizes those as committed dead and so we're back to
      the same problem.
      
      To improve the situation, invent a "SnapshotNonVacuumable" snapshot type
      and use that instead.  The reason this helps is that, if the snapshot
      rejects a given index entry, we know that the indexscan will mark that
      index entry as killed.  This means the next get_actual_variable_range()
      scan will proceed past that entry without visiting the heap, making the
      scan a lot faster.  We may end up accepting a recently-dead tuple as
      being the estimated extremal value, but that doesn't seem much worse than
      the compromise we made before to accept not-yet-committed extremal values.
      
      The cost of the scan is still proportional to the number of dead index
      entries at the end of the range, so in the interval after a mass delete
      but before VACUUM's cleaned up the mess, it's still possible for
      get_actual_variable_range() to take a noticeable amount of time, if you've
      got enough such dead entries.  But the constant factor is much much better
      than before, since all we need to do with each index entry is test its
      "killed" bit.
      
      We chose to back-patch commit fccebe42 at the time, but I'm hesitant to
      do so here, because this form of the problem seems to affect many fewer
      people.  Also, even when it happens, it's less bad than the case fixed
      by commit fccebe42 because we don't get the contention effects from
      expensive TransactionIdIsInProgress tests.
      
      Dmitriy Sarafannikov, reviewed by Andrey Borodin
      
      Discussion: https://postgr.es/m/05C72CF7-B5F6-4DB9-8A09-5AC897653113@yandex.ru
      3ca930fc
    • Tom Lane's avatar
      Improve documentation about behavior of multi-statement Query messages. · b9764994
      Tom Lane authored
      We've long done our best to sweep this topic under the rug, but in view
      of recent work it seems like it's time to explain it more precisely.
      Here's an initial cut at doing that.
      
      Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6BE40D@G01JPEXMBYT05
      b9764994
    • Peter Eisentraut's avatar
      Reduce excessive dereferencing of function pointers · 1356f78e
      Peter Eisentraut authored
      It is equivalent in ANSI C to write (*funcptr) () and funcptr().  These
      two styles have been applied inconsistently.  After discussion, we'll
      use the more verbose style for plain function pointer variables, to make
      it clear that it's a variable, and the shorter style when the function
      pointer is in a struct (s.func() or s->func()), because then it's clear
      that it's not a plain function name, and otherwise the excessive
      punctuation makes some of those invocations hard to read.
      
      Discussion: https://www.postgresql.org/message-id/f52c16db-14ed-757d-4b48-7ef360b1631d@2ndquadrant.com
      1356f78e
    • Robert Haas's avatar
      Even if some partitions are foreign, allow tuple routing. · 9d71323d
      Robert Haas authored
      This doesn't allow routing tuple to the foreign partitions themselves,
      but it permits tuples to be routed to regular partitions despite the
      presence of foreign partitions in the same inheritance hierarchy.
      
      Etsuro Fujita, reviewed by Amit Langote and by me.
      
      Discussion: http://postgr.es/m/bc3db4c1-1693-3b8a-559f-33ad2b50b7ad@lab.ntt.co.jp
      9d71323d
    • Tom Lane's avatar
      Fix handling of savepoint commands within multi-statement Query strings. · 6eb52da3
      Tom Lane authored
      Issuing a savepoint-related command in a Query message that contains
      multiple SQL statements led to a FATAL exit with a complaint about
      "unexpected state STARTED".  This is a shortcoming of commit 4f896dac,
      which attempted to prevent such misbehaviors in multi-statement strings;
      its quick hack of marking the individual statements as "not top-level"
      does the wrong thing in this case, and isn't a very accurate description
      of the situation anyway.
      
      To fix, let's introduce into xact.c an explicit model of what happens for
      multi-statement Query strings.  This is an "implicit transaction block
      in progress" state, which for many purposes works like the normal
      TBLOCK_INPROGRESS state --- in particular, IsTransactionBlock returns true,
      causing the desired result that PreventTransactionChain will throw error.
      But in case of error abort it works like TBLOCK_STARTED, allowing the
      transaction to be cancelled without need for an explicit ROLLBACK command.
      
      Commit 4f896dac is reverted in toto, so that we go back to treating the
      individual statements as "top level".  We could have left it as-is, but
      this allows sharpening the error message for PreventTransactionChain
      calls inside functions.
      
      Except for getting a normal error instead of a FATAL exit for savepoint
      commands, this patch should result in no user-visible behavioral change
      (other than that one error message rewording).  There are some things
      we might want to do in the line of changing the appearance or wording of
      error and warning messages around this behavior, which would be much
      simpler to do now that it's an explicitly modeled state.  But I haven't
      done them here.
      
      Although this fixes a long-standing bug, no backpatch.  The consequences
      of the bug don't seem severe enough to justify the risk that this commit
      itself creates some new issue.
      
      Patch by me, but it owes something to previous investigation by
      Takayuki Tsunakawa, who also reported the bug in the first place.
      Also thanks to Michael Paquier for reviewing.
      
      Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6BE40D@G01JPEXMBYT05
      6eb52da3
    • Tom Lane's avatar
      Further marginal hacking on generic atomic ops. · bfea9256
      Tom Lane authored
      In the generic atomic ops that rely on a loop around a CAS primitive,
      there's no need to force the initial read of the "old" value to be atomic.
      In the typically-rare case that we get a torn value, that simply means
      that the first CAS attempt will fail; but it will update "old" to the
      atomically-read value, so the next attempt has a chance of succeeding.
      It was already being done that way in pg_atomic_exchange_u64_impl(),
      but let's duplicate the approach in the rest.
      
      (Given the current coding of the pg_atomic_read functions, this change
      is a no-op anyway on popular platforms; it only makes a difference where
      pg_atomic_read_u64_impl() is implemented as a CAS.)
      
      In passing, also remove unnecessary take-a-pointer-and-dereference-it
      coding in the pg_atomic_read functions.  That seems to have been based
      on a misunderstanding of what the C standard requires.  What actually
      matters is that the pointer be declared as pointing to volatile, which
      it is.
      
      I don't believe this will change the assembly code at all on x86
      platforms (even ignoring the likelihood that these implementations
      get overridden by others); but it may help on less-mainstream CPUs.
      
      Discussion: https://postgr.es/m/13707.1504718238@sss.pgh.pa.us
      bfea9256
    • Simon Riggs's avatar
      Exclude special values in recovery_target_time · f06588a8
      Simon Riggs authored
      recovery_target_time accepts timestamp input, though
      does not allow use of special values, e.g. “today”.
      Report a useful error message for these cases.
      
      Reported-by: Piotr Stefaniak
      Author: Simon Riggs
      Discussion: https://postgr.es/m/CANP8+jJdKA+BkkYLWz9zAm16Y0s2ExBv0WfpAwXdTpPfWnA9Bg@mail.gmail.com
      f06588a8
  7. 06 Sep, 2017 2 commits