1. 12 Feb, 2019 3 commits
    • Michael Paquier's avatar
      Clarify docs about limitations of constraint exclusion with partitions · ea05b221
      Michael Paquier authored
      The current wording can confuse the reader about constraint exclusion
      being available at query execution, but this only applies to partition
      pruning.
      
      Reported-by: Shouyu Luo
      Author: David Rowley
      Reviewed-by: Chapman Flack, Amit Langote
      Discussion: https://postgr.es/m/15629-2ef8b22e61f8333f@postgresql.org
      ea05b221
    • Tom Lane's avatar
      Allow extensions to generate lossy index conditions. · 74dfe58a
      Tom Lane authored
      For a long time, indxpath.c has had the ability to extract derived (lossy)
      index conditions from certain operators such as LIKE.  For just as long,
      it's been obvious that we really ought to make that capability available
      to extensions.  This commit finally accomplishes that, by adding another
      API for planner support functions that lets them create derived index
      conditions for their functions.  As proof of concept, the hardwired
      "special index operator" code formerly present in indxpath.c is pushed
      out to planner support functions attached to LIKE and other relevant
      operators.
      
      A weak spot in this design is that an extension needs to know OIDs for
      the operators, datatypes, and opfamilies involved in the transformation
      it wants to make.  The core-code prototypes use hard-wired OID references
      but extensions don't have that option for their own operators etc.  It's
      usually possible to look up the required info, but that may be slow and
      inconvenient.  However, improving that situation is a separate task.
      
      I want to do some additional refactorization around selfuncs.c, but
      that also seems like a separate task.
      
      Discussion: https://postgr.es/m/15193.1548028093@sss.pgh.pa.us
      74dfe58a
    • Michael Paquier's avatar
      Move max_wal_senders out of max_connections for connection slot handling · ea92368c
      Michael Paquier authored
      Since its introduction, max_wal_senders is counted as part of
      max_connections when it comes to define how many connection slots can be
      used for replication connections with a WAL sender context.  This can
      lead to confusion for some users, as it could be possible to block a
      base backup or replication from happening because other backend sessions
      are already taken for other purposes by an application, and
      superuser-only connection slots are not a correct solution to handle
      that case.
      
      This commit makes max_wal_senders independent of max_connections for its
      handling of PGPROC entries in ProcGlobal, meaning that connection slots
      for WAL senders are handled using their own free queue, like autovacuum
      workers and bgworkers.
      
      One compatibility issue that this change creates is that a standby now
      requires to have a value of max_wal_senders at least equal to its
      primary.  So, if a standby created enforces the value of
      max_wal_senders to be lower than that, then this could break failovers.
      Normally this should not be an issue though, as any settings of a
      standby are inherited from its primary as postgresql.conf gets normally
      copied as part of a base backup, so parameters would be consistent.
      
      Author: Alexander Kukushkin
      Reviewed-by: Kyotaro Horiguchi, Petr Jelínek, Masahiko Sawada, Oleksii
      Kliukin
      Discussion: https://postgr.es/m/CAFh8B=nBzHQeYAu0b8fjK-AF1X4+_p6GRtwG+cCgs6Vci2uRuQ@mail.gmail.com
      ea92368c
  2. 11 Feb, 2019 5 commits
    • Tom Lane's avatar
      Redesign the partition dependency mechanism. · 1d92a0c9
      Tom Lane authored
      The original setup for dependencies of partitioned objects had
      serious problems:
      
      1. It did not verify that a drop cascading to a partition-child object
      also cascaded to at least one of the object's partition parents.  Now,
      normally a child object would share all its dependencies with one or
      another parent (e.g. a child index's opclass dependencies would be shared
      with the parent index), so that this oversight is usually harmless.
      But if some dependency failed to fit this pattern, the child could be
      dropped while all its parents remain, creating a logically broken
      situation.  (It's easy to construct artificial cases that break it,
      such as attaching an unrelated extension dependency to the child object
      and then dropping the extension.  I'm not sure if any less-artificial
      cases exist.)
      
      2. Management of partition dependencies during ATTACH/DETACH PARTITION
      was complicated and buggy; for example, after detaching a partition
      table it was possible to create cases where a formerly-child index
      should be dropped and was not, because the correct set of dependencies
      had not been reconstructed.
      
      Less seriously, because multiple partition relationships were
      represented identically in pg_depend, there was an order-of-traversal
      dependency on which partition parent was cited in error messages.
      We also had some pre-existing order-of-traversal hazards for error
      messages related to internal and extension dependencies.  This is
      cosmetic to users but causes testing problems.
      
      To fix #1, add a check at the end of the partition tree traversal
      to ensure that at least one partition parent got deleted.  To fix #2,
      establish a new policy that partition dependencies are in addition to,
      not instead of, a child object's usual dependencies; in this way
      ATTACH/DETACH PARTITION need not cope with adding or removing the
      usual dependencies.
      
      To fix the cosmetic problem, distinguish between primary and secondary
      partition dependency entries in pg_depend, by giving them different
      deptypes.  (They behave identically except for having different
      priorities for being cited in error messages.)  This means that the
      former 'I' dependency type is replaced with new 'P' and 'S' types.
      
      This also fixes a longstanding bug that after handling an internal
      dependency by recursing to the owning object, findDependentObjects
      did not verify that the current target was now scheduled for deletion,
      and did not apply the current recursion level's objflags to it.
      Perhaps that should be back-patched; but in the back branches it
      would only matter if some concurrent transaction had removed the
      internal-linkage pg_depend entry before the recursive call found it,
      or the recursive call somehow failed to find it, both of which seem
      unlikely.
      
      Catversion bump because the contents of pg_depend change for
      partitioning relationships.
      
      Patch HEAD only.  It's annoying that we're not fixing #2 in v11,
      but there seems no practical way to do so given that the problem
      is exactly a poor choice of what entries to put in pg_depend.
      We can't really fix that while staying compatible with what's
      in pg_depend in existing v11 installations.
      
      Discussion: https://postgr.es/m/CAH2-Wzkypv1R+teZrr71U23J578NnTBt2X8+Y=Odr4pOdW1rXg@mail.gmail.com
      1d92a0c9
    • Alvaro Herrera's avatar
      Fix misleading PG_RE_THROW commentary · c603b392
      Alvaro Herrera authored
      The old verbiage indicated that PG_RE_THROW is optional, which is not
      really true.  This has confused many people, so it seems worth fixing.
      
      Discussion: https://postgr.es/m/20190206160958.GA22304@alvherre.pgsql
      c603b392
    • Peter Eisentraut's avatar
      256fc004
    • Peter Eisentraut's avatar
      Remove unused macro · 78b0cac7
      Peter Eisentraut authored
      Last use was removed in 2c66f992.
      78b0cac7
    • Tom Lane's avatar
      Fix indexable-row-comparison logic to account for covering indexes. · 6bdc3005
      Tom Lane authored
      indxpath.c needs a good deal more attention for covering indexes than
      it's gotten.  But so far as I can tell, the only really awful breakage
      is in expand_indexqual_rowcompare (nee adjust_rowcompare_for_index),
      which was only half fixed in c266ed31.  The other problems aren't
      bad enough to take the risk of a just-before-wrap fix.
      
      The problem here is that if the leading column of a row comparison
      matches an index (allowing this code to be reached), and some later
      column doesn't match the index, it'll nonetheless believe that that
      column matches the first included index column.  Typically that'll
      lead to an error like "operator M is not a member of opfamily N" as
      a result of fetching a garbage opfamily OID.  But with enough bad
      luck, maybe a broken plan would be generated.
      
      Discussion: https://postgr.es/m/25526.1549847928@sss.pgh.pa.us
      6bdc3005
  3. 10 Feb, 2019 4 commits
    • Tom Lane's avatar
      Add per-test-script runtime display to pg_regress. · 72d71e03
      Tom Lane authored
      It seems useful to have this information available, so that it's
      easier to tell when a test script is taking a disproportionate
      amount of time.
      
      Discussion: https://postgr.es/m/16646.1549770618@sss.pgh.pa.us
      72d71e03
    • Alvaro Herrera's avatar
      Fix trigger drop procedure · cb90de1a
      Alvaro Herrera authored
      After commit 123cc697a8eb, we remove redundant FK action triggers during
      partition ATTACH by merely deleting the catalog tuple, but that's wrong:
      it should use performDeletion() instead.  Repair, and make the comments
      more explicit.
      
      Per code review from Tom Lane.
      
      Discussion: https://postgr.es/m/18885.1549642539@sss.pgh.pa.us
      cb90de1a
    • Tom Lane's avatar
      Solve cross-version-upgrade testing problem induced by 1fb57af9. · 068503c7
      Tom Lane authored
      Renaming varchar_transform to varchar_support had a side effect
      I hadn't foreseen: the core regression tests leave around a
      transform object that relies on that function, so the name
      change breaks cross-version upgrade tests, because the name
      used in the older branches doesn't match.
      
      Since the dependency on varchar_transform was chosen with the
      aid of a dartboard anyway (it would surely not work as a
      language transform support function), fix by just choosing
      a different random builtin function with the right signature.
      Also add some comments explaining why this isn't horribly unsafe.
      
      I chose to make the same substitution in a couple of other
      copied-and-pasted test cases, for consistency, though those
      aren't directly contributing to the testing problem.
      
      Per buildfarm.  Back-patch, else it doesn't fix the problem.
      068503c7
    • Tom Lane's avatar
      Repair unsafe/unportable snprintf usage in pg_restore. · 4dbe1969
      Tom Lane authored
      warn_or_exit_horribly() was blithely passing a potentially-NULL
      string pointer to a %s format specifier.  That works (at least
      to the extent of not crashing) on some platforms, but not all,
      and since we switched to our own snprintf.c it doesn't work
      for us anywhere.
      
      Of the three string fields being handled this way here, I think
      that only "owner" is supposed to be nullable ... but considering
      that this is error-reporting code, it has very little business
      assuming anything, so put in defenses for all three.
      
      Per a crash observed on buildfarm member crake and then
      reproduced here.  Because of the portability aspect,
      back-patch to all supported versions.
      4dbe1969
  4. 09 Feb, 2019 9 commits
    • Tom Lane's avatar
      Build out the planner support function infrastructure. · a391ff3c
      Tom Lane authored
      Add support function requests for estimating the selectivity, cost,
      and number of result rows (if a SRF) of the target function.
      
      The lack of a way to estimate selectivity of a boolean-returning
      function in WHERE has been a recognized deficiency of the planner
      since Berkeley days.  This commit finally fixes it.
      
      In addition, non-constant estimates of cost and number of output
      rows are now possible.  We still fall back to looking at procost
      and prorows if the support function doesn't service the request,
      of course.
      
      To make concrete use of the possibility of estimating output rowcount
      for SRFs, this commit adds support functions for array_unnest(anyarray)
      and the integer variants of generate_series; the lack of plausible
      rowcount estimates for those, even when it's obvious to a human,
      has been a repeated subject of complaints.  Obviously, much more
      could now be done in this line, but I'm mostly just trying to get
      the infrastructure in place.
      
      Discussion: https://postgr.es/m/15193.1548028093@sss.pgh.pa.us
      a391ff3c
    • Tom Lane's avatar
      Create the infrastructure for planner support functions. · 1fb57af9
      Tom Lane authored
      Rename/repurpose pg_proc.protransform as "prosupport".  The idea is
      still that it names an internal function that provides knowledge to
      the planner about the behavior of the function it's attached to;
      but redesign the API specification so that it's not limited to doing
      just one thing, but can support an extensible set of requests.
      
      The original purpose of simplifying a function call is handled by
      the first request type to be invented, SupportRequestSimplify.
      Adjust all the existing transform functions to handle this API,
      and rename them fron "xxx_transform" to "xxx_support" to reflect
      the potential generalization of what they do.  (Since we never
      previously provided any way for extensions to add transform functions,
      this change doesn't create an API break for them.)
      
      Also add DDL and pg_dump support for attaching a support function to a
      user-defined function.  Unfortunately, DDL access has to be restricted
      to superusers, at least for now; but seeing that support functions
      will pretty much have to be written in C, that limitation is just
      theoretical.  (This support is untested in this patch, but a follow-on
      patch will add cases that exercise it.)
      
      Discussion: https://postgr.es/m/15193.1548028093@sss.pgh.pa.us
      1fb57af9
    • Tom Lane's avatar
      Refactor the representation of indexable clauses in IndexPaths. · 1a8d5afb
      Tom Lane authored
      In place of three separate but interrelated lists (indexclauses,
      indexquals, and indexqualcols), an IndexPath now has one list
      "indexclauses" of IndexClause nodes.  This holds basically the same
      information as before, but in a more useful format: in particular, there
      is now a clear connection between an indexclause (an original restriction
      clause from WHERE or JOIN/ON) and the indexquals (directly usable index
      conditions) derived from it.
      
      We also change the ground rules a bit by mandating that clause commutation,
      if needed, be done up-front so that what is stored in the indexquals list
      is always directly usable as an index condition.  This gets rid of repeated
      re-determination of which side of the clause is the indexkey during costing
      and plan generation, as well as repeated lookups of the commutator
      operator.  To minimize the added up-front cost, the typical case of
      commuting a plain OpExpr is handled by a new special-purpose function
      commute_restrictinfo().  For RowCompareExprs, generating the new clause
      properly commuted to begin with is not really any more complex than before,
      it's just different --- and we can save doing that work twice, as the
      pretty-klugy original implementation did.
      
      Tracking the connection between original and derived clauses lets us
      also track explicitly whether the derived clauses are an exact or lossy
      translation of the original.  This provides a cheap solution to getting
      rid of unnecessary rechecks of boolean index clauses, which previously
      seemed like it'd be more expensive than it was worth.
      
      Another pleasant (IMO) side-effect is that EXPLAIN now always shows
      index clauses with the indexkey on the left; this seems less confusing.
      
      This commit leaves expand_indexqual_conditions() and some related
      functions in a slightly messy state.  I didn't bother to change them
      any more than minimally necessary to work with the new data structure,
      because all that code is going to be refactored out of existence in
      a follow-on patch.
      
      Discussion: https://postgr.es/m/22182.1549124950@sss.pgh.pa.us
      1a8d5afb
    • Tom Lane's avatar
      Call set_rel_pathlist_hook before generate_gather_paths, not after. · 64015838
      Tom Lane authored
      The previous ordering of these steps satisfied the nominal requirement
      that set_rel_pathlist_hook could editorialize on the whole set of Paths
      constructed for a base relation.  In practice, though, trying to change
      the set of partial paths was impossible.  Adding one didn't work because
      (a) it was too late to be included in Gather paths made by the core code,
      and (b) calling add_partial_path after generate_gather_paths is unsafe,
      because it might try to delete a path it thinks is dominated, but that
      is already embedded in some Gather path(s).  Nor could the hook safely
      remove partial paths, for the same reason that they might already be
      embedded in Gathers.
      
      Better to call extensions first, let them add partial paths as desired,
      and then gather.  In v11 and up, we already doubled down on that ordering
      by postponing gathering even further for single-relation queries; so even
      if the hook wished to editorialize on Gather path construction, it could
      not.
      
      Report and patch by KaiGai Kohei.  Back-patch to 9.6 where Gather paths
      were added.
      
      Discussion: https://postgr.es/m/CAOP8fzahwpKJRTVVTqo2AE=mDTz_efVzV6Get_0=U3SO+-ha1A@mail.gmail.com
      64015838
    • Peter Eisentraut's avatar
      Use better comment marker in Autoconf input · 4446565d
      Peter Eisentraut authored
      The comment marker "#" is copied to the output, so it's only
      appropriate for comments that make sense in the shell output.  For
      comments about the Autoconf language, "dnl" should be used.
      4446565d
    • Andres Freund's avatar
      Reset, not recreate, execGrouping.c style hashtables. · 356687bd
      Andres Freund authored
      This uses the facility added in the preceding commit to fix
      performance issues caused by rebuilding the hashtable (with its
      comparator expression being the most expensive bit), after every
      reset. That's especially important when the comparator is JIT
      compiled.
      
      Bug: #15592 #15486
      Reported-By: Jakub Janeček, Dmitry Marakasov
      Author: Andres Freund
      Discussion:
          https://postgr.es/m/15486-05850f065da42931@postgresql.org
          https://postgr.es/m/20190114180423.ywhdg2iagzvh43we@alap3.anarazel.de
      Backpatch: 11, where I broke this in bf6c614a
      356687bd
    • Andres Freund's avatar
      Allow to reset execGrouping.c style tuple hashtables. · 317ffdfe
      Andres Freund authored
      This has the advantage that the comparator expression, the table's
      slot, etc do not have to be rebuilt. Additionally the simplehash.h
      hashtable within the tuple hashtable now keeps its previous size and
      doesn't need to be reallocated. That both reduces allocator overhead,
      and improves performance in cases where the input estimation was off
      by a significant factor.
      
      To avoid an API/ABI break, the new parameter is exposed via the new
      BuildTupleHashTableExt(), and BuildTupleHashTable() now is a wrapper
      around the former, that continues to allocate the table itself in the
      tablecxt.
      
      Using this fixes performance issues discovered in the two bugs
      referenced. This commit however has not converted the callers, that's
      done in a separate commit.
      
      Bug: #15592 #15486
      Reported-By: Jakub Janeček, Dmitry Marakasov
      Author: Andres Freund
      Discussion:
          https://postgr.es/m/15486-05850f065da42931@postgresql.org
          https://postgr.es/m/20190114180423.ywhdg2iagzvh43we@alap3.anarazel.de
      Backpatch: 11, this is a prerequisite for other fixes
      317ffdfe
    • Andres Freund's avatar
      simplehash: Add support for resetting a hashtable's contents. · 3b632a58
      Andres Freund authored
      A hashtable reset just reset the hashtable entries, but does not free
      memory.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20190114180423.ywhdg2iagzvh43we@alap3.anarazel.de
      Bug: #15592 #15486
      Backpatch: 11, this is a prerequisite for other fixes
      3b632a58
    • Andres Freund's avatar
      Plug leak in BuildTupleHashTable by creating ExprContext in correct context. · 5567d12c
      Andres Freund authored
      In bf6c614a I added a expr context to evaluate the grouping
      expression. Unfortunately the code I added initialized them while in
      the calling context, rather the table context.  Additionally, I used
      CreateExprContext() rather than CreateStandaloneExprContext(), which
      creates the econtext in the estate's query context.
      
      Fix that by using CreateStandaloneExprContext when in the table's
      tablecxt. As we rely on the memory being freed by a memory context
      reset that means that the econtext's shutdown callbacks aren't being
      called, but that seems ok as the expressions are tightly controlled
      due to ExecBuildGroupingEqual().
      
      Bug: #15592
      Reported-By: Dmitry Marakasov
      Author: Andres Freund
      Discussion: https://postgr.es/m/20190114222838.h6r3fuyxjxkykf6t@alap3.anarazel.de
      Backpatch: 11, where I broke this in bf6c614a
      5567d12c
  5. 08 Feb, 2019 4 commits
  6. 07 Feb, 2019 10 commits
  7. 06 Feb, 2019 5 commits
    • Peter Geoghegan's avatar
      Avoid amcheck inline compression false positives. · eba77534
      Peter Geoghegan authored
      The previous tacit assumption that index_form_tuple() hides differences
      in the TOAST state of its input datums was wrong.  Normalize input
      varlena datums by decompressing compressed values, and forming a new
      index tuple for fingerprinting using uncompressed inputs.  The final
      normalized representation may actually be compressed once again within
      index_form_tuple(), though that shouldn't matter.  When the original
      tuple is found to have no datums that are compressed inline, fingerprint
      the original tuple directly.
      
      Normalization avoids false positive reports of corruption in certain
      cases.  For example, the executor can apply toasting with some inline
      compression to an entire heap tuple because its input has a single
      external TOAST pointer.  Varlena datums for other attributes that are
      not particularly good candidates for inline compression can be
      compressed in the heap tuple in passing, without the representation of
      the same values in index tuples ever receiving concomitant inline
      compression.
      
      Add a test case to recreate the issue in a simpler though less realistic
      way: by exploiting differences in pg_attribute.attstorage between heap
      and index relations.
      
      This bug was discovered by me during testing of an upcoming set of nbtree
      enhancements.  It was also independently reported by Andreas Kunert, as
      bug #15597.  His test case was rather more realistic than the one I
      ended up using.
      
      Bug: #15597
      Discussion: https://postgr.es/m/CAH2-WznrVd9ie+TTJ45nDT+v2nUt6YJwQrT9SebCdQKtAvfPZw@mail.gmail.com
      Discussion: https://postgr.es/m/15597-294e5d3e7f01c407@postgresql.org
      Backpatch: 11-, where heapallindexed verification was introduced.
      eba77534
    • Peter Eisentraut's avatar
      Hide cascade messages in collate tests · 727921f4
      Peter Eisentraut authored
      These are not relevant to the tests and would just uselessly bloat
      patches.
      727921f4
    • Tom Lane's avatar
      Propagate lateral-reference information to indirect descendant relations. · bdd9a99a
      Tom Lane authored
      create_lateral_join_info() computes a bunch of information about lateral
      references between base relations, and then attempts to propagate those
      markings to appendrel children of the original base relations.  But the
      original coding neglected the possibility of indirect descendants
      (grandchildren etc).  During v11 development we noticed that this was
      wrong for partitioned-table cases, but failed to realize that it was just
      as wrong for any appendrel.  While the case can't arise for appendrels
      derived from traditional table inheritance (because we make a flat
      appendrel for that), nested appendrels can arise from nested UNION ALL
      subqueries.  Failure to mark the lower-level relations as having lateral
      references leads to confusion in add_paths_to_append_rel about whether
      unparameterized paths can be built.  It's not very clear whether that
      leads to any user-visible misbehavior; the lack of field reports suggests
      that it may cause nothing worse than minor cost misestimation.  Still,
      it's a bug, and it leads to failures of Asserts that I intend to add
      later.
      
      To fix, we need to propagate information from all appendrel parents,
      not just those that are RELOPT_BASERELs.  We can still do it in one
      pass, if we rely on the append_rel_list to be ordered with ancestor
      relationships before descendant ones; add assertions checking that.
      While fixing this, we can make a small performance improvement by
      traversing the append_rel_list just once instead of separately for
      each appendrel parent relation.
      
      Noted while investigating bug #15613, though this patch does not fix
      that (which is why I'm not committing the related Asserts yet).
      
      Discussion: https://postgr.es/m/3951.1549403812@sss.pgh.pa.us
      bdd9a99a
    • Andrew Dunstan's avatar
      Unify searchpath and do file logic in MSVC build scripts. · 592123ef
      Andrew Dunstan authored
      Commit f83419b7 failed to notice that mkvcbuild.pl and build.pl use
      different searchpath and do-file logic, breaking the latter, so it is
      adjusted to use the same logic as mkvcbuild.pl.
      592123ef
    • Andres Freund's avatar
      Fix heap_getattr() handling of fast defaults. · 171e0418
      Andres Freund authored
      Previously heap_getattr() returned NULL for attributes with a fast
      default value (c.f. 16828d5c), as it had no handling whatsoever
      for that case.
      
      A previous fix, 7636e5c6, attempted to fix issues caused by this
      oversight, but just expanding OLD tuples for triggers doesn't actually
      solve the underlying issue.
      
      One known consequence of this bug is that the check for HOT updates
      can return the wrong result, when a previously fast-default'ed column
      is set to NULL. Which in turn means that an index over a column with
      fast default'ed columns might be corrupt if the underlying column(s)
      allow NULLs.
      
      Fix by handling fast default columns in heap_getattr(), remove now
      superfluous expansion in GetTupleForTrigger().
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20190201162404.onngi77f26baem4g@alap3.anarazel.de
      Backpatch: 11, where fast defaults were introduced
      171e0418