1. 20 Oct, 2017 4 commits
    • Peter Eisentraut's avatar
      Convert SGML IDs to lower case · 1ff01b39
      Peter Eisentraut authored
      IDs in SGML are case insensitive, and we have accumulated a mix of upper
      and lower case IDs, including different variants of the same ID.  In
      XML, these will be case sensitive, so we need to fix up those
      differences.  Going to all lower case seems most straightforward, and
      the current build process already makes all anchors and lower case
      anyway during the SGML->XML conversion, so this doesn't create any
      difference in the output right now.  A future XML-only build process
      would, however, maintain any mixed case ID spellings in the output, so
      that is another reason to clean this up beforehand.
      
      Author: Alexander Lakhin <exclusion@gmail.com>
      1ff01b39
    • Tom Lane's avatar
      Fix typcache's failure to treat ranges as container types. · 36ea99c8
      Tom Lane authored
      Like the similar logic for arrays and records, it's necessary to examine
      the range's subtype to decide whether the range type can support hashing.
      We can omit checking the subtype for btree-defined operations, though,
      since range subtypes are required to have those operations.  (Possibly
      that simplification for btree cases led us to overlook that it does
      not apply for hash cases.)
      
      This is only an issue if the subtype lacks hash support, which is not
      true of any built-in range type, but it's easy to demonstrate a problem
      with a range type over, eg, money: you can get a "could not identify
      a hash function" failure when the planner is misled into thinking that
      hash join or aggregation would work.
      
      This was born broken, so back-patch to all supported branches.
      36ea99c8
    • Tom Lane's avatar
      Fix misimplementation of typcache logic for extended hashing. · a8f1efc8
      Tom Lane authored
      The previous coding would report that an array type supports extended
      hashing if its element type supports regular hashing.  This bug is
      only latent at the moment, since AFAICS there is not yet any code
      that depends on checking presence of extended-hashing support to make
      any decisions.  (And in any case it wouldn't matter unless the element
      type has only regular hashing, which isn't true of any core data type.)
      But that doesn't make it less broken.  Extend the
      cache_array_element_properties infrastructure to check this properly.
      a8f1efc8
    • Robert Haas's avatar
      pg_stat_statements: Add a comment about the dangers of padding bytes. · 2959213b
      Robert Haas authored
      Inspired by a patch from Julien Rouhaud, but I reworded it.
      
      Discussion: http://postgr.es/m/CAOBaU_a8AH8=ypfqgHnDYu06ts+jWTUgh=VgCxA3yNV-K10j9w@mail.gmail.com
      2959213b
  2. 19 Oct, 2017 6 commits
  3. 18 Oct, 2017 2 commits
  4. 17 Oct, 2017 2 commits
  5. 16 Oct, 2017 6 commits
    • Tom Lane's avatar
      Fix incorrect handling of CTEs and ENRs as DML target relations. · 7421f4b8
      Tom Lane authored
      setTargetTable threw an error if the proposed target RangeVar's relname
      matched any visible CTE or ENR.  This breaks backwards compatibility in
      the CTE case, since pre-v10 we never looked for a CTE here at all, so that
      CTE names did not mask regular tables.  It does seem like a good idea to
      throw an error for the ENR case, though, thus causing ENRs to mask tables
      for this purpose; ENRs are new in v10 so we're not breaking existing code,
      and we may someday want to allow them to be the targets of DML.
      
      To fix that, replace use of getRTEForSpecialRelationTypes, which was
      overkill anyway, with use of scanNameSpaceForENR.
      
      A second problem was that the check neglected to verify null schemaname,
      so that a CTE or ENR could incorrectly be thought to match a qualified
      RangeVar.  That happened because getRTEForSpecialRelationTypes relied
      on its caller to have checked for null schemaname.  Even though the one
      remaining caller got it right, this is obviously bug-prone, so move
      the check inside getRTEForSpecialRelationTypes.
      
      Also, revert commit 18ce3a4a's extremely poorly thought out decision to
      add a NULL return case to parserOpenTable --- without either documenting
      that or adjusting any of the callers to check for it.  The current bug
      seems to have arisen in part due to working around that bad idea.
      
      In passing, remove the one-line shim functions transformCTEReference and
      transformENRReference --- they don't seem to be adding any clarity or
      functionality.
      
      Per report from Hugo Mercier (via Julien Rouhaud).  Back-patch to v10
      where the bug was introduced.
      
      Thomas Munro, with minor editing by me
      
      Discussion: https://postgr.es/m/CAOBaU_YdPVH+PTtiKSSLOiiW3mVDYsnNUekK+XPbHXiP=wrFLA@mail.gmail.com
      7421f4b8
    • Peter Eisentraut's avatar
      Exclude flex-generated code from coverage testing · 42116736
      Peter Eisentraut authored
      Flex generates a lot of functions that are not actually used.  In order
      to avoid coverage figures being ruined by that, mark up the part of the
      .l files where the generated code appears by lcov exclusion markers.
      That way, lcov will typically only reported on coverage for the .l file,
      which is under our control, but not for the .c file.
      Reviewed-by: default avatarMichael Paquier <michael.paquier@gmail.com>
      42116736
    • Tom Lane's avatar
      Treat aggregate direct arguments as per-agg data not per-trans data. · cf5ba7c3
      Tom Lane authored
      There is no reason to insist that direct arguments must match before
      we can merge transition states of two aggregate calls.  They're only
      used during the finalfn call, so we can treat them as like the finalfn
      itself.  This allows, eg, merging of
      
      select
        percentile_cont(0.25) within group (order by a),
        percentile_disc(0.5) within group (order by a)
      from ...
      
      This didn't matter (and could not have been tested) before we allowed
      state merging of OSAs otherwise.
      
      Discussion: https://postgr.es/m/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com
      cf5ba7c3
    • Tom Lane's avatar
      Allow the built-in ordered-set aggregates to share transition state. · be0ebb65
      Tom Lane authored
      The built-in OSAs all share the same transition function, so they can
      share transition state as long as the final functions cooperate to not
      do the sort step more than once.  To avoid running the tuplesort object
      in randomAccess mode unnecessarily, add a bit of infrastructure to
      nodeAgg.c to let the aggregate functions find out whether the transition
      state is actually being shared or not.
      
      This doesn't work for the hypothetical aggregates, since those inject
      a hypothetical row that isn't traceable to the shared input state.
      So they remain marked aggfinalmodify = 'w'.
      
      Discussion: https://postgr.es/m/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com
      be0ebb65
    • Tom Lane's avatar
      Repair breakage of aggregate FILTER option. · c3dfe0fe
      Tom Lane authored
      An aggregate's input expression(s) are not supposed to be evaluated
      at all for a row where its FILTER test fails ... but commit 8ed3f11b
      overlooked that requirement.  Reshuffle so that aggregates having a
      filter clause evaluate their arguments separately from those without.
      This still gets the benefit of doing only one ExecProject in the
      common case of multiple Aggrefs, none of which have filters.
      
      While at it, arrange for filter clauses to be included in the common
      ExecProject evaluation, thus perhaps buying a little bit even when
      there are filters.
      
      Back-patch to v10 where the bug was introduced.
      
      Discussion: https://postgr.es/m/30065.1508161354@sss.pgh.pa.us
      c3dfe0fe
    • Alvaro Herrera's avatar
      Rework DefineIndex relkind check · 60a1d96e
      Alvaro Herrera authored
      Simplify coding using a switch rather than nested if tests.
      
      Author: Álvaro
      Reviewed-by: Robert Haas, Amit Langote, Michaël Paquier
      Discussion: https://postgr.es/m/20171013163820.pai7djcaxrntaxtn@alvherre.pgsql
      60a1d96e
  6. 15 Oct, 2017 2 commits
    • Tom Lane's avatar
      Restore nodeAgg.c's ability to check for improperly-nested aggregates. · 5fc438fb
      Tom Lane authored
      While poking around in the aggregate logic, I noticed that commit
      8ed3f11b broke the logic in nodeAgg.c that purports to detect nested
      aggregates, by moving initialization of regular aggregate argument
      expressions out of the code segment that checks for that.
      
      You could argue that this check is unnecessary, but it's not much code
      so I'm inclined to keep it as a backstop against parser and planner
      bugs.  However, there's certainly zero value in checking only some of
      the subexpressions.
      
      We can make the check complete again, and as a bonus make it a good
      deal more bulletproof against future mistakes of the same ilk, by
      moving it out to the outermost level of ExecInitAgg.  This means we
      need to check only once per Agg node not once per aggregate, which
      also seems like a good thing --- if the check does find something
      wrong, it's not urgent that we report it before the plan node
      initialization finishes.
      
      Since this requires remembering the original length of the aggs list,
      I deleted a long-obsolete stanza that changed numaggs from 0 to 1.
      That's so old it predates our decision that palloc(0) is a valid
      operation, in (digs...) 2004, see commit 24a1e20f.
      
      In passing improve a few comments.
      
      Back-patch to v10, just in case.
      5fc438fb
    • Peter Eisentraut's avatar
      doc: Postgres -> PostgreSQL · d8794fd7
      Peter Eisentraut authored
      d8794fd7
  7. 14 Oct, 2017 3 commits
    • Tom Lane's avatar
      gcc's support for __attribute__((noinline)) hasn't been around forever. · 82aff8d3
      Tom Lane authored
      Buildfarm member gaur says it wasn't there in 2.95.3.  Guess that 3.0
      and later have it.
      82aff8d3
    • Tom Lane's avatar
      Explicitly track whether aggregate final functions modify transition state. · 4de2d4fb
      Tom Lane authored
      Up to now, there's been hard-wired assumptions that normal aggregates'
      final functions never modify their transition states, while ordered-set
      aggregates' final functions always do.  This has always been a bit
      limiting, and in particular it's getting in the way of improving the
      built-in ordered-set aggregates to allow merging of transition states.
      Therefore, let's introduce catalog and CREATE AGGREGATE infrastructure
      that lets the finalfn's behavior be declared explicitly.
      
      There are now three possibilities for the finalfn behavior: it's purely
      read-only, it trashes the transition state irrecoverably, or it changes
      the state in such a way that no more transfn calls are possible but the
      state can still be passed to other, compatible finalfns.  There are no
      examples of this third case today, but we'll shortly make the built-in
      OSAs act like that.
      
      This change allows user-defined aggregates to explicitly disclaim support
      for use as window functions, and/or to prevent transition state merging,
      if their implementations cannot handle that.  While it was previously
      possible to handle the window case with a run-time error check, there was
      not any way to prevent transition state merging, which in retrospect is
      something commit 804163bc should have provided for.  But better late
      than never.
      
      In passing, split out pg_aggregate.c's extern function declarations into
      a new header file pg_aggregate_fn.h, similarly to what we've done for
      some other catalog headers, so that pg_aggregate.h itself can be safe
      for frontend files to include.  This lets pg_dump use the symbolic
      names for relevant constants.
      
      Discussion: https://postgr.es/m/4834.1507849699@sss.pgh.pa.us
      4de2d4fb
    • Peter Eisentraut's avatar
      Reinstate genhtml --prefix option for non-vpath builds · 5f340cb3
      Peter Eisentraut authored
      In c3d9a660, the genhtml --prefix option
      was removed to get slightly better behavior for vpath builds.  genhtml
      would then automatically pick a suitable prefix.  However, for non-vpath
      builds, this makes the coverage output dependent on the length of the
      path where the source code happens to be, leading to confusingly
      arbitrary results.  So put the --prefix option back for non-vpath
      builds.
      5f340cb3
  8. 13 Oct, 2017 10 commits
    • Joe Conway's avatar
      Add missing options to pg_regress help() output · b81eba6a
      Joe Conway authored
      A few command line options accepted by pg_regress were not being output
      by help(), including --help itself. Add that one, as well as --version
      and --bindir, and the corresponding short options for the first two.
      
      We could consider this for backpatching, but it did not seem worthwhile
      and no one else advocated for it, so apply only to master for now.
      
      Author: Joe Conway
      Reviewed-By: Tom Lane
      Discussion: https://postgr.es/m/dd519469-06d7-2662-83ef-c926f6c4f0f1%40joeconway.com
      b81eba6a
    • Andres Freund's avatar
      Improve sys/catcache performance. · 141fd1b6
      Andres Freund authored
      The following are the individual improvements:
      1) Avoidance of FunctionCallInfo based function calls, replaced by
         more efficient functions with a native C argument interface.
      2) Don't extract columns from a cache entry's tuple whenever matching
         entries - instead store them as a Datum array. This also allows to
         get rid of having to build dummy tuples for negative & list
         entries, and of a hack for dealing with cstring vs. text weirdness.
      3) Reorder members of catcache.h struct, so imortant entries are more
         likely to be on one cacheline.
      4) Allowing the compiler to specialize critical SearchCatCache for a
         specific number of attributes allows to unroll loops and avoid
         other nkeys dependant initialization.
      5) Only initializing the ScanKey when necessary, i.e. catcache misses,
         greatly reduces cache unnecessary cpu cache misses.
      6) Split of the cache-miss case from the hash lookup, reducing stack
         allocations etc in the common case.
      7) CatCTup and their corresponding heaptuple are allocated in one
         piece.
      
      This results in making cache lookups themselves roughly three times as
      fast - full-system benchmarks obviously improve less than that.
      
      I've also evaluated further techniques:
      - replace open coded hash with simplehash - the list walk right now
        shows up in profiles. Unfortunately it's not easy to do so safely as
        an entry's memory location can change at various times, which
        doesn't work well with the refcounting and cache invalidation.
      - Cacheline-aligning CatCTup entries - helps some with performance,
        but the win isn't big and the code for it is ugly, because the
        tuples have to be freed as well.
      - add more proper functions, rather than macros for
        SearchSysCacheCopyN etc., but right now they don't show up in
        profiles.
      
      The reason the macro wrapper for syscache.c/h have to be changed,
      rather than just catcache, is that doing otherwise would require
      exposing the SysCache array to the outside.  That might be a good idea
      anyway, but it's for another day.
      
      Author: Andres Freund
      Reviewed-By: Robert Haas
      Discussion: https://postgr.es/m/20170914061207.zxotvyopetm7lrrp@alap3.anarazel.de
      141fd1b6
    • Andres Freund's avatar
      Add pg_noinline macro to c.h. · a0247e7a
      Andres Freund authored
      Forcing a function not to be inlined can be useful if it's the
      slow-path of a performance critical function, or should be visible in
      profiles to allow for proper cost attribution.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20170914061207.zxotvyopetm7lrrp@alap3.anarazel.de
      a0247e7a
    • Andres Freund's avatar
      Force "restrict" not to be used when compiling with xlc. · d133982d
      Andres Freund authored
      Per buildfarm animal Hornet and followup manual testing by Noah Misch,
      it appears xlc miscompiles code using "restrict" in at least some
      cases. Allow disabling restrict usage with FORCE_DISABLE_RESTRICT=yes
      in template files, and do so for aix/xlc.
      
      Author: Andres Freund and Tom Lane
      Discussion: https://postgr.es/m/1820.1507918762@sss.pgh.pa.us
      d133982d
    • Robert Haas's avatar
      Fix possible crash with Parallel Bitmap Heap Scan. · 6393613b
      Robert Haas authored
      If a Parallel Bitmap Heap scan's chain of leftmost descendents
      includes a BitmapOr whose first child is a BitmapAnd, the prior coding
      would mistakenly create a non-shared TIDBitmap and then try to perform
      shared iteration.
      
      Report by Tomas Vondra.  Patch by Dilip Kumar.
      
      Discussion: http://postgr.es/m/50e89684-8ad9-dead-8767-c9545bafd3b6@2ndquadrant.com
      6393613b
    • Tom Lane's avatar
      Improve implementation of CRE-stack-flattening in map_variable_attnos(). · 73937119
      Tom Lane authored
      I (tgl) objected to the obscure implementation introduced in commit
      1c497fa7.  This one seems a bit less action-at-a-distance-y, at the
      price of repeating a few lines of code.
      
      Improve the comments about what the function is doing, too.
      
      Amit Khandekar, whacked around a bit more by me
      
      Discussion: https://postgr.es/m/CAJ3gD9egYTyHUH0nTMxm8-1m3RvdqEbaTyGC-CUNtYf7tKNDaQ@mail.gmail.com
      73937119
    • Tom Lane's avatar
      Rely on sizeof(typename) rather than sizeof(variable) in pqformat.h. · 5229db6c
      Tom Lane authored
      In each of the pq_writeintN functions, the three uses of sizeof() should
      surely all be consistent.  I started out to make them all sizeof(ni),
      but on reflection let's make them sizeof(typename) instead.  That's more
      like our usual style elsewhere, and it's just barely possible that the
      failures buildfarm member hornet has shown since 4c119fbc went in are
      caused by the compiler getting confused about sizeof() a parameter that
      it's optimizing away.
      
      In passing, improve a couple of comments.
      
      Discussion: https://postgr.es/m/E1e2RML-0002do-Lc@gemulon.postgresql.org
      5229db6c
    • Peter Eisentraut's avatar
      Attempt to fix LDAP build · 7d1b8e75
      Peter Eisentraut authored
      Apparently, an older spelling of LDAP_OPT_DIAGNOSTIC_MESSAGE is
      LDAP_OPT_ERROR_STRING, so fall back to that one.
      7d1b8e75
    • Peter Eisentraut's avatar
      Log diagnostic messages if errors occur during LDAP auth. · cf1238cd
      Peter Eisentraut authored
      Diagnostic messages seem likely to help users diagnose root
      causes more easily, so let's report them as errdetail.
      
      Author: Thomas Munro
      Reviewed-By: Ashutosh Bapat, Christoph Berg, Alvaro Herrera, Peter Eisentraut
      Discussion: https://postgr.es/m/CAEepm=2_dA-SYpFdmNVwvKsEBXOUj=K4ooKovHmvj6jnMdt8dw@mail.gmail.com
      cf1238cd
    • Peter Eisentraut's avatar
      Improve LDAP cleanup code in error paths. · 1feff99f
      Peter Eisentraut authored
      After calling ldap_unbind_s() we probably shouldn't try to use the LDAP
      connection again to call ldap_get_option(), even if it failed.  The OpenLDAP
      man page for ldap_unbind[_s] says "Once it is called, the connection to the
      LDAP server is closed, and the ld structure is invalid."  Otherwise, as a
      general rule we should probably call ldap_unbind() before returning in all
      paths to avoid leaking resources.  It is unlikely there is any practical
      leak problem since failure to authenticate currently results in the backend
      exiting soon afterwards.
      
      Author: Thomas Munro
      Reviewed-By: Alvaro Herrera, Peter Eisentraut
      Discussion: https://postgr.es/m/20170914141205.eup4kxzlkagtmfac%40alvherre.pgsql
      1feff99f
  9. 12 Oct, 2017 5 commits