1. 18 May, 2019 3 commits
  2. 17 May, 2019 3 commits
    • Tom Lane's avatar
      Restructure creation of run-time pruning steps. · 6630ccad
      Tom Lane authored
      Previously, gen_partprune_steps() always built executor pruning steps
      using all suitable clauses, including those containing PARAM_EXEC
      Params.  This meant that the pruning steps were only completely safe
      for executor run-time (scan start) pruning.  To prune at executor
      startup, we had to ignore the steps involving exec Params.  But this
      doesn't really work in general, since there may be logic changes
      needed as well --- for example, pruning according to the last operator's
      btree strategy is the wrong thing if we're not applying that operator.
      The rules embodied in gen_partprune_steps() and its minions are
      sufficiently complicated that tracking their incremental effects in
      other logic seems quite impractical.
      
      Short of a complete redesign, the only safe fix seems to be to run
      gen_partprune_steps() twice, once to create executor startup pruning
      steps and then again for run-time pruning steps.  We can save a few
      cycles however by noting during the first scan whether we rejected
      any clauses because they involved exec Params --- if not, we don't
      need to do the second scan.
      
      In support of this, refactor the internal APIs in partprune.c to make
      more use of passing information in the GeneratePruningStepsContext
      struct, rather than as separate arguments.
      
      This is, I hope, the last piece of our response to a bug report from
      Alan Jackson.  Back-patch to v11 where this code came in.
      
      Discussion: https://postgr.es/m/FAD28A83-AC73-489E-A058-2681FA31D648@tvsquared.com
      6630ccad
    • Bruce Momjian's avatar
    • Michael Paquier's avatar
      Fix regression test outputs · 6ba500ca
      Michael Paquier authored
      75445c15 has caused various failures in tests across the tree after
      updating some error messages, so fix the newly-expected output.
      
      Author: Michael Paquier
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/8332.1558048838@sss.pgh.pa.us
      6ba500ca
  3. 16 May, 2019 5 commits
    • Michael Paquier's avatar
      41998f90
    • Alvaro Herrera's avatar
    • Peter Geoghegan's avatar
      Remove extra nbtree half-dead internal page check. · 3f58cc6d
      Peter Geoghegan authored
      It's not safe for nbtree VACUUM to attempt to delete a target page whose
      right sibling is already half-dead, since that would fail the
      cross-check when VACUUM attempts to re-find a downlink to the right
      sibling in the parent page.  Logic to prevent this from happening was
      added by commit 8da31837, which addressed a bug in the overhaul of
      page deletion that went into PostgreSQL 9.4 (commit efada2b8).
      VACUUM was made to check the right sibling page, and back off when it
      happened to be half-dead already.
      
      However, it is only truly necessary to do the right sibling check on the
      leaf level, since that transitively determines if the deletion target's
      parent's right sibling page is itself undergoing deletion.  Remove the
      internal page level check, and add a comment explaining why the leaf
      level check alone suffices.
      
      The extra check is also unnecessary due to the fact that internal pages
      that are marked half-dead are generally considered corrupt.  Commit
      efada2b8 established the principle that there should never be
      half-dead internal pages (internal pages pending deletion are possible,
      but that status is never directly represented in the internal page).
      VACUUM will complain about corruption when it encounters half-dead
      internal pages, so VACUUM is bound to raise an error one way or another
      when an nbtree index has a half-dead internal page (contrib/amcheck will
      also report that the page is corrupt).
      
      It's possible that a pg_upgrade'd 9.3 database will still have half-dead
      internal pages, so it may seem like there is an argument for leaving the
      check in place to reliably get a cleaner error message that advises the
      user to REINDEX.  However, leaf pages are also deleted in the first
      phase of deletion prior to PostgreSQL 9.4, so I believe we won't even
      attempt to re-find the parent page anyway (we won't have the fully
      deleted leaf page as the right sibling of our target page, so we won't
      even try to find a downlink for it).
      
      Discussion: https://postgr.es/m/CAH2-Wzm_ntmqJjWLRyKzimFmFvk+BnVAvUpaA4s1h9Ja58woaQ@mail.gmail.com
      3f58cc6d
    • Tom Lane's avatar
      Fix bogus logic for combining range-partitioned columns during pruning. · 3922f106
      Tom Lane authored
      gen_prune_steps_from_opexps's notion of how to do this was overly
      complicated and underly correct.
      
      Per discussion of a report from Alan Jackson (though this fixes only one
      aspect of that problem).  Back-patch to v11 where this code came in.
      
      Amit Langote
      
      Discussion: https://postgr.es/m/FAD28A83-AC73-489E-A058-2681FA31D648@tvsquared.com
      3922f106
    • Tom Lane's avatar
      Fix partition pruning to treat stable comparison operators properly. · 4b1fcb43
      Tom Lane authored
      Cross-type comparison operators in a btree or hash opclass might be
      only stable not immutable (this is true of timestamp vs. timestamptz
      for example).  partprune.c ignored this possibility and would perform
      plan-time pruning with them anyway, possibly leading to wrong answers
      if the environment changed between planning and execution.
      
      To fix, teach gen_partprune_steps() to do things differently when
      creating plan-time pruning steps vs. run-time pruning steps.
      analyze_partkey_exprs() also needs an extra check, which is rather
      annoying but now is not the time to restructure things enough to
      avoid that.
      
      While at it, simplify the logic for the plan-time case a little
      by insisting that the comparison value be a Const and nothing else.
      This relies on the assumption that eval_const_expressions will have
      reduced any immutable expression to a Const; which is not quite
      100% true, but certainly any case that comes up often enough to be
      interesting should have simplification logic there.
      
      Also improve a bunch of inadequate/obsolete/wrong comments.
      
      Per discussion of a report from Alan Jackson (though this fixes only one
      aspect of that problem).  Back-patch to v11 where this code came in.
      
      David Rowley, with some further hacking by me
      
      Discussion: https://postgr.es/m/FAD28A83-AC73-489E-A058-2681FA31D648@tvsquared.com
      4b1fcb43
  4. 15 May, 2019 4 commits
    • Peter Geoghegan's avatar
      Remove obsolete nbtree insertion comment. · 489e431b
      Peter Geoghegan authored
      Remove a Berkeley-era comment above _bt_insertonpg() that admonishes the
      reader to grok Lehman and Yao's paper before making any changes.  This
      made a certain amount of sense back when _bt_insertonpg() was
      responsible for most of the things that are now spread across
      _bt_insertonpg(), _bt_findinsertloc(), _bt_insert_parent(), and
      _bt_split(), but it doesn't work like that anymore.
      
      I believe that this comment alludes to the need to "couple" or "crab"
      buffer locks as we ascend the tree as page splits cascade upwards.  The
      nbtree README already explains this in detail, which seems sufficient.
      Besides, the changes to page splits made by commit 40dae7ec altered
      the exact details of how buffer locks are retained during splits; Lehman
      and Yao's original algorithm seems to release the lock on the left child
      page/buffer slightly earlier than _bt_insertonpg()/_bt_insert_parent()
      can.
      489e431b
    • Tom Lane's avatar
      Remove no-longer-used typedef. · 8a0f0ad5
      Tom Lane authored
      struct ClonedConstraint is no longer needed, so delete it.
      
      Discussion: https://postgr.es/m/18102.1557947143@sss.pgh.pa.us
      8a0f0ad5
    • Peter Geoghegan's avatar
      Reverse order of newitem nbtree candidate splits. · 7505da2f
      Peter Geoghegan authored
      Commit fab25024, which taught nbtree to choose candidate split points
      more carefully, had _bt_findsplitloc() record all possible split points
      in an initial pass over a page that is about to be split.  The order
      that candidate split points were processed and stored in was assumed to
      match the offset number order of split points on an imaginary version of
      the page that contains the same items as the original, but also fits
      newitem (the item that provoked the split precisely because it didn't
      fit).
      
      However, the order of split points in the final array was not quite what
      was expected: the split point that makes newitem the firstright item
      came after the split point that makes newitem the lastleft item -- not
      before.  As a result, _bt_findsplitloc() could get confused about the
      leftmost and rightmost tuples among all possible split points recorded
      for the page.  This seems to have no appreciable impact on the quality
      of the final split point chosen by _bt_findsplitloc(), but it's still
      wrong.
      
      To fix, switch the order in which newitem candidate splits are recorded
      in.  This also makes it possible to describe candidate split points in
      terms of which pair of adjoining tuples enclose the split point within
      _bt_findsplitloc(), making it clearer why it's generally safe for
      _bt_split() to expect lastleft and firstright tuples.
      7505da2f
    • Bruce Momjian's avatar
      docs: properly indent PG 12 release notes · a429164e
      Bruce Momjian authored
      a429164e
  5. 14 May, 2019 20 commits
  6. 13 May, 2019 5 commits
    • Peter Geoghegan's avatar
      Standardize ItemIdData terminology. · ae7291ac
      Peter Geoghegan authored
      The term "item pointer" should not be used to refer to ItemIdData
      variables, since that is needlessly ambiguous.  Only
      ItemPointerData/ItemPointer variables should be called item pointers.
      
      To fix, establish the convention that ItemIdData variables should always
      be referred to either as "item identifiers" or "line pointers".  The
      term "item identifier" already predominates in docs and translatable
      messages, and so should be the preferred alternative there.
      
      Discussion: https://postgr.es/m/CAH2-Wz=c=MZQjUzde3o9+2PLAPuHTpVZPPdYxN=E4ndQ2--8ew@mail.gmail.com
      ae7291ac
    • Peter Geoghegan's avatar
      Doc: Refer to line pointers as item identifiers. · 08ca9d7f
      Peter Geoghegan authored
      An upcoming HEAD-only patch will standardize the terminology around
      ItemIdData variables/line pointers, ending the practice of referring to
      them as "item pointers".  Make the "Database Page Layout" docs
      consistent with the new policy.  The term "item identifier" is already
      used in the same section, so stick with that.
      
      Discussion: https://postgr.es/m/CAH2-Wz=c=MZQjUzde3o9+2PLAPuHTpVZPPdYxN=E4ndQ2--8ew@mail.gmail.com
      Backpatch: All supported branches.
      08ca9d7f
    • Tom Lane's avatar
      Fix logical replication's ideas about which type OIDs are built-in. · 32ebb351
      Tom Lane authored
      Only hand-assigned type OIDs should be presumed to match across different
      PG servers; those assigned during genbki.pl or during initdb are likely
      to change due to addition or removal of unrelated objects.
      
      This means that the cutoff should be FirstGenbkiObjectId (in HEAD)
      or FirstBootstrapObjectId (before that), not FirstNormalObjectId.
      Compare postgres_fdw's is_builtin() test.
      
      It's likely that this error has no observable consequence in a
      normally-functioning system, since ATM the only affected type OIDs are
      system catalog rowtypes and information_schema types, which would not
      typically be interesting for logical replication.  But you could
      probably break it if you tried hard, so back-patch.
      
      Discussion: https://postgr.es/m/15150.1557257111@sss.pgh.pa.us
      32ebb351
    • Tom Lane's avatar
      Improve commentary about hack in is_publishable_class(). · e34ee993
      Tom Lane authored
      The FirstNormalObjectId test here is a kluge that needs to go away,
      but the only substitute we can think of is to add a column to pg_class,
      which will take more work than can be handled right now.  Add some
      commentary in the meanwhile.
      
      Discussion: https://postgr.es/m/15150.1557257111@sss.pgh.pa.us
      e34ee993
    • Peter Geoghegan's avatar
      Don't leave behind junk nbtree pages during split. · 9b42e713
      Peter Geoghegan authored
      Commit 8fa30f90 reduced the elevel of a number of "can't happen"
      _bt_split() errors from PANIC to ERROR.  At the same time, the new right
      page buffer for the split could continue to be acquired well before the
      critical section.  This was possible because it was relatively
      straightforward to make sure that _bt_split() could not throw an error,
      with a few specific exceptions.  The exceptional cases were safe because
      they involved specific, well understood errors, making it possible to
      consistently zero the right page before actually raising an error using
      elog().  There was no danger of leaving around a junk page, provided
      _bt_split() stuck to this coding rule.
      
      Commit 8224de4f, which introduced INCLUDE indexes, added code to make
      _bt_split() truncate away non-key attributes.  This happened at a point
      that broke the rule around zeroing the right page in _bt_split().  If
      truncation failed (perhaps due to palloc() failure), that would result
      in an errant right page buffer with junk contents.  This could confuse
      VACUUM when it attempted to delete the page, and should be avoided on
      general principle.
      
      To fix, reorganize _bt_split() so that truncation occurs before the new
      right page buffer is even acquired.  A junk page/buffer will not be left
      behind if _bt_nonkey_truncate()/_bt_truncate() raise an error.
      
      Discussion: https://postgr.es/m/CAH2-WzkcWT_-NH7EeL=Az4efg0KCV+wArygW8zKB=+HoP=VWMw@mail.gmail.com
      Backpatch: 11-, where INCLUDE indexes were introduced.
      9b42e713