1. 24 Jul, 2019 1 commit
    • Michael Paquier's avatar
      Improve stability of TAP test for synchronous replication · 7d81bdc8
      Michael Paquier authored
      Slow buildfarm machines have run into issues with this TAP test caused
      by a race condition related to the startup of a set of standbys, where
      it is possible to finish with an unexpected order in the WAL sender
      array of the primary.
      
      This closes the race condition by making sure that any standby started
      is registered into the WAL sender array of the primary before starting
      the next one based on lookups of pg_stat_replication.
      
      Backpatch down to 9.6 where the test has been introduced.
      
      Author: Michael Paquier
      Reviewed-by: Álvaro Herrera, Noah Misch
      Discussion: https://postgr.es/m/20190617055145.GB18917@paquier.xyz
      Backpatch-through: 9.6
      7d81bdc8
  2. 23 Jul, 2019 5 commits
  3. 22 Jul, 2019 7 commits
  4. 21 Jul, 2019 4 commits
    • David Rowley's avatar
      Adjust overly strict Assert · e1a0f6a9
      David Rowley authored
      3373c715 changed how we determine EquivalenceClasses for relations and
      added an Assert to ensure all relations mentioned in each EC's ec_relids
      was a RELOPT_BASEREL.  However, the join removal code may remove a LEFT
      JOIN and since it does not clean up EC members belonging to the removed
      relations it can leave RELOPT_DEADREL rels in ec_relids.
      
      Fix this by adjusting the Assert to allow RELOPT_DEADREL rels too.
      
      Reported-by: sqlsmith via Andreas Seltenreich
      Discussion: https://postgr.es/m/87y30r8sls.fsf@ansel.ydns.eu
      e1a0f6a9
    • Tom Lane's avatar
      Remove no-longer-helpful reliance on fixed-size local array. · 330cafdf
      Tom Lane authored
      Coverity complained about this code, apparently because it uses a local
      array of size FUNC_MAX_ARGS without a guard that the input argument list
      is no longer than that.  (Not sure why it complained today, since this
      code's been the same for a long time; possibly it re-analyzed everything
      the List API change touched?)
      
      Rather than add a guard, though, let's just get rid of the local array
      altogether.  It was only there to avoid list_nth() calls, and those are
      no longer expensive.
      330cafdf
    • Michael Paquier's avatar
      Fix compilation warning of pg_basebackup with MinGW · 90317ab7
      Michael Paquier authored
      Several buildfarm members have been complaining about that with gcc,
      like jacana.  Weirdly enough, Visual Studio's compilers do not find this
      issue.
      
      Author: Michael Paquier
      Reviewed-by: Andrew Dunstan
      Discussion: https://postgr.es/m/20190719050830.GK1859@paquier.xyz
      90317ab7
    • David Rowley's avatar
      Speed up finding EquivalenceClasses for a given set of rels · 3373c715
      David Rowley authored
      Previously in order to determine which ECs a relation had members in, we
      had to loop over all ECs stored in PlannerInfo's eq_classes and check if
      ec_relids mentioned the relation.  For the most part, this was fine, as
      generally, unless queries were fairly complex, the overhead of performing
      the lookup would have not been that significant.  However, when queries
      contained large numbers of joins and ECs, the overhead to find the set of
      classes matching a given set of relations could become a significant
      portion of the overall planning effort.
      
      Here we allow a much more efficient method to access the ECs which match a
      given relation or set of relations.  A new Bitmapset field in RelOptInfo
      now exists to store the indexes into PlannerInfo's eq_classes list which
      each relation is mentioned in.  This allows very fast lookups to find all
      ECs belonging to a single relation.  When we need to lookup ECs belonging
      to a given pair of relations, we can simply bitwise-AND the Bitmapsets from
      each relation and use the result to perform the lookup.
      
      We also take the opportunity to write a new implementation of
      generate_join_implied_equalities which makes use of the new indexes.
      generate_join_implied_equalities_for_ecs must remain as is as it can be
      given a custom list of ECs, which we can't easily determine the indexes of.
      
      This was originally intended to fix the performance penalty of looking up
      foreign keys matching a join condition which was introduced by 100340e2.
      However, we're speeding up much more than just that here.
      
      Author: David Rowley, Tom Lane
      Reviewed-by: Tom Lane, Tomas Vondra
      Discussion: https://postgr.es/m/6970.1545327857@sss.pgh.pa.us
      3373c715
  5. 20 Jul, 2019 3 commits
    • Peter Geoghegan's avatar
      Don't rely on estimates for amcheck Bloom filters. · 894af78f
      Peter Geoghegan authored
      Solely relying on a relation's reltuples/relpages estimate to size the
      Bloom filters used by amcheck verification makes verification less
      effective when the estimates are very stale.  In extreme cases,
      verification options that use Bloom filters internally could be totally
      ineffective, without users receiving any clear indication that certain
      types of corruption might easily be missed.
      
      To fix, use RelationGetNumberOfBlocks() instead of relpages to size the
      downlink block Bloom filter.  Use the same RelationGetNumberOfBlocks()
      value to derive a minimum size for the heapallindexed Bloom filter,
      rather than completely trusting reltuples.  Verification will still be
      reasonably effective when the projected/estimated number of Bloom filter
      elements is at least 1/5 of the final number of elements, which is
      assured by the new sizing logic.
      
      Reported-By: Alexander Korotkov
      Discussion: https://postgr.es/m/CAH2-Wzk0ke2J42KrNYBKu0Xovjy-sU5ub7PWjgpbsKdAQcL4OA@mail.gmail.com
      Backpatch: 11-, where downlink/heapallindexed verification were added.
      894af78f
    • Tomas Vondra's avatar
      Use column collation for extended statistics · a63378a0
      Tomas Vondra authored
      The current extended statistics code was a bit confused which collation
      to use.  When building the statistics, the collations defined as default
      for the data types were used (since commit 5e092800).  The MCV code was
      however using the column collations for MCV serialization, and then
      DEFAULT_COLLATION_OID when computing estimates. So overall the code was
      using all three possible options, inconsistently.
      
      This uses the column colation everywhere - this makes it consistent with
      what 5e092800 did for regular stats.  We however do not track the
      collations in a catalog, because we can derive them from column-level
      information.  This may need to change in the future, e.g. after allowing
      statistics on expressions.
      
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      Backpatch-to: 12
      a63378a0
    • Tomas Vondra's avatar
      Rework examine_opclause_expression to use varonleft · e38a55ba
      Tomas Vondra authored
      The examine_opclause_expression function needs to return information on
      which side of the operator we found the Var, but the variable was called
      "isgt" which is rather misleading (it assumes the operator is either
      less-than or greater-than, but it may be equality or something else).
      Other places in the planner use a variable called "varonleft" for this
      purpose, so just adopt the same convention here.
      
      The code also assumed we don't care about this flag for equality, as
      (Var = Const) and (Const = Var) should be the same thing. But that does
      not work for cross-type operators, in which case we need to pass the
      parameters to the procedure in the right order. So just use the same
      code for all types of expressions.
      
      This means we don't need to care about the selectivity estimation
      function anymore, at least not in this code. We should only get the
      supported cases here (thanks to statext_is_compatible_clause).
      
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      Backpatch-to: 12
      e38a55ba
  6. 19 Jul, 2019 5 commits
  7. 18 Jul, 2019 10 commits
    • Michael Paquier's avatar
      Fix typo in mvdistinct.c · 70a33b21
      Michael Paquier authored
      Noticed while browsing the code.
      70a33b21
    • Jeff Davis's avatar
      Fix daterange canonicalization for +/- infinity. · e6feef57
      Jeff Davis authored
      The values 'infinity' and '-infinity' are a part of the DATE type
      itself, so a bound of the date 'infinity' is not the same as an
      unbounded/infinite range. However, it is still wrong to try to
      canonicalize such values, because adding or subtracting one has no
      effect. Fix by treating 'infinity' and '-infinity' the same as
      unbounded ranges for the purposes of canonicalization (but not other
      purposes).
      
      Backpatch to all versions because it is inconsistent with the
      documented behavior. Note that this could be an incompatibility for
      applications relying on the behavior contrary to the documentation.
      
      Author: Laurenz Albe
      Reviewed-by: Thomas Munro
      Discussion: https://postgr.es/m/77f24ea19ab802bc9bc60ddbb8977ee2d646aec1.camel%40cybertec.at
      Backpatch-through: 9.4
      e6feef57
    • Peter Geoghegan's avatar
      Fix nbtree metapage cache upgrade bug. · d004147e
      Peter Geoghegan authored
      Commit 857f9c36, which taught nbtree VACUUM to avoid unnecessary
      index scans, bumped the nbtree version number from 2 to 3, while adding
      the ability for nbtree indexes to be upgraded on-the-fly.  Various
      assertions that assumed that an nbtree index was always on version 2 had
      to be changed to accept any supported version (version 2 or 3 on
      Postgres 11).
      
      However, a few assertions were missed in the initial commit, all of
      which were in code paths that cache a local copy of the metapage
      metadata, where the index had been expected to be on the current version
      (no longer version 2) as a generic sanity check.  Rather than simply
      update the assertions, follow-up commit 0a64b451 intentionally made
      the metapage caching code update the per-backend cached metadata version
      without changing the on-disk version at the same time.  This could even
      happen when the planner needed to determine the height of a B-Tree for
      costing purposes.  The assertions only fail on Postgres v12 when
      upgrading from v10, because they were adjusted to use the authoritative
      shared memory metapage by v12's commit dd299df8.
      
      To fix, remove the cache-only upgrade mechanism entirely, and update the
      assertions themselves to accept any supported version (go back to using
      the cached version in v12).  The fix is almost a full revert of commit
      0a64b451 on the v11 branch.
      
      VACUUM only considers the authoritative metapage, and never bothers with
      a locally cached version, whereas everywhere else isn't interested in
      the metapage fields that were added by commit 857f9c36.  It seems
      unlikely that this bug has affected any user on v11.
      
      Reported-By: Christoph Berg
      Bug: #15896
      Discussion: https://postgr.es/m/15896-5b25e260fdb0b081%40postgresql.org
      Backpatch: 11-, where VACUUM was taught to avoid unnecessary index scans.
      d004147e
    • Tom Lane's avatar
      Further adjust SPITupleTable to provide a public row-count field. · bc8393cf
      Tom Lane authored
      Now that commit fec0778c drew a clear line between public and private
      fields in SPITupleTable, it seems pretty silly that the count of valid
      tuples isn't on the public side of that line.  The reason why not was
      that there wasn't such a count.  For reasons lost in the mists of time,
      spi.c preferred to keep a count of remaining free entries in the array.
      But that seems pretty pointless: it's unlike the way we handle similar
      code everywhere else, and it involves extra subtractions that surely
      outweigh having to do a comparison rather than test-for-zero to check
      for array-full.
      
      Hence, rearrange so that this code does the expansible array logic
      the same as everywhere else, with a count of valid entries alongside
      the allocated array length.  And document the count as public.
      
      I looked for core-code callers where it would make sense to start
      relying on tuptable->numvals rather than the separate SPI_processed
      variable.  Right now there don't seem to be places where it'd be
      a win to do so without more code restructuring than I care to
      undertake today.  In principle, though, having SPITupleTables be
      fully self-contained should be helpful down the line.
      
      Discussion: https://postgr.es/m/16852.1563395722@sss.pgh.pa.us
      bc8393cf
    • Tomas Vondra's avatar
      Simplify bitmap updates in multivariate MCV code · 7d24f6a4
      Tomas Vondra authored
      When evaluating clauses on a multivariate MCV list, we build a bitmap
      tracking how the clauses match each item of the MCV list.  When updating
      the bitmap we need to consider the current value (tracking how the item
      matches preceding clauses), match for the current clause and whether the
      clauses are connected by AND or OR.
      
      Until now the logic was copied on every place updating the bitmap, which
      was not quite readable.  So just move it to a separate function and call
      it where needed.
      
      Backpatch to 12, where the code was introduced. While not a bugfix, this
      should make maintenance and future backpatches easier.
      
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      7d24f6a4
    • Tomas Vondra's avatar
      Fix handling of NULLs in MCV items and constants · e4deae73
      Tomas Vondra authored
      There were two issues in how the extended statistics handled NULL values
      in opclauses. Firstly, the code was oblivious to the possibility that
      Const may be NULL (constisnull=true) in which case the constvalue is
      undefined. We need to treat this as a mismatch, and not call the proc.
      
      Secondly, the MCV item itself may contain NULL values too - the code
      already did check that, and updated the match bitmap accordingly, but
      failed to ensure we won't call the operator procedure anyway. It did
      work for AND-clauses, because in that case false in the bitmap stops
      evaluation of further clauses. But for OR-clauses ir was not easy to
      get incorrect estimates or even trigger a crash.
      
      This fixes both issues by extending the existing check so that it looks
      at constisnull too, and making sure it skips calling the procedure.
      
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      e4deae73
    • Tomas Vondra's avatar
      Fix handling of opclauses in extended statistics · e8b6ae21
      Tomas Vondra authored
      We expect opclauses to have exactly one Var and one Const, but the code
      was checking the Const by calling is_pseudo_constant_clause() which is
      incorrect - we need a proper constant.
      
      Fixed by using plain IsA(x,Const) to check type of the node. We need to
      do these checks in two places, so move it into a separate function that
      can be called in both places.
      
      Reported by Andreas Seltenreich, based on crash reported by sqlsmith.
      
      Backpatch to v12, where this code was introduced.
      
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      Backpatch-to: 12
      e8b6ae21
    • Tomas Vondra's avatar
      Remove unnecessary TYPECACHE_GT_OPR lookup · a4303a07
      Tomas Vondra authored
      The TYPECACHE_GT_OPR is not needed (it used to be in older version of
      the MCV code), but the compiler failed to detect this as the result was
      used in a fmgr_info() call, populating a FmgrInfo entry.
      
      Backpatch to v12, where this code was introduced.
      
      Discussion: https://postgr.es/m/8736jdhbhc.fsf%40ansel.ydns.eu
      Backpatch-to: 12
      a4303a07
    • Andres Freund's avatar
      21039555
    • Michael Paquier's avatar
      Simplify description of --data-checksums in documentation of initdb · 1c1602b8
      Michael Paquier authored
      The documentation mentioned that data checksums cannot be changed after
      initialization, which is not true as pg_checksums can do that with its
      --enable option introduced in v12.  This simply removes the sentence
      telling so.
      
      Reported-by: Basil Bourque
      Author: Michael Paquier
      Reviewed-by: Daniel Gustafsson
      Discussion: https://postgr.es/m/15909-e9d74271f1647472@postgresql.org
      Backpatch-through: 12
      1c1602b8
  8. 17 Jul, 2019 5 commits