1. 11 Jan, 2019 3 commits
  2. 10 Jan, 2019 10 commits
  3. 09 Jan, 2019 2 commits
    • Tom Lane's avatar
      Reduce the size of the fmgr_builtin_oid_index[] array. · 8ff5f824
      Tom Lane authored
      This index array was originally defined to have 10000 entries (ranging
      up to FirstGenbkiObjectId), but we really only need entries up to the
      last existing builtin function OID, currently 6121.  That saves close
      to 8K of never-accessed space in the server executable, at the small
      price of one more fetch in fmgr_isbuiltin().
      
      We could reduce the array size still further by renumbering a few of
      the highest-numbered builtin functions; but there's a small risk of
      breaking clients that have chosen to hardwire those function OIDs,
      so it's not clear if it'd be worth the trouble.  (We should, however,
      discourage future patches from choosing function OIDs above 6K as long
      as there's still lots of space below that.)
      
      Discussion: https://postgr.es/m/12359.1547063064@sss.pgh.pa.us
      8ff5f824
    • Tom Lane's avatar
      Update docs & tests to reflect that unassigned OLD/NEW are now NULL. · 59029b6f
      Tom Lane authored
      For a long time, plpgsql has allowed trigger functions to parse
      references to OLD and NEW even if the current trigger event type didn't
      assign a value to one or the other variable; but actually executing such
      a reference would fail.  The v11 changes to use "expanded records" for
      DTYPE_REC variables changed the behavior so that the unassigned variable
      now reads as a null composite value.  While this behavioral change was
      more or less unintentional, it seems that leaving it like this is better
      than adding code and complexity to be bug-compatible with the old way.
      The change doesn't break any code that worked before, and it eliminates
      a gotcha that often required extra code to work around.
      
      Hence, update the docs to say that these variables are "null" not
      "unassigned" when not relevant to the event type.  And add a regression
      test covering the behavior, so that we'll notice if we ever break it
      again.
      
      Per report from Kristjan Tammekivi.
      
      Discussion: https://postgr.es/m/CAABK7uL-uC9ZxKBXzo_68pKt7cECfNRv+c35CXZpjq6jCAzYYA@mail.gmail.com
      59029b6f
  4. 08 Jan, 2019 3 commits
  5. 07 Jan, 2019 4 commits
  6. 06 Jan, 2019 1 commit
    • Tom Lane's avatar
      Replace the data structure used for keyword lookup. · afb0d071
      Tom Lane authored
      Previously, ScanKeywordLookup was passed an array of string pointers.
      This had some performance deficiencies: the strings themselves might
      be scattered all over the place depending on the compiler (and some
      quick checking shows that at least with gcc-on-Linux, they indeed
      weren't reliably close together).  That led to very cache-unfriendly
      behavior as the binary search touched strings in many different pages.
      Also, depending on the platform, the string pointers might need to
      be adjusted at program start, so that they couldn't be simple constant
      data.  And the ScanKeyword struct had been designed with an eye to
      32-bit machines originally; on 64-bit it requires 16 bytes per
      keyword, making it even more cache-unfriendly.
      
      Redesign so that the keyword strings themselves are allocated
      consecutively (as part of one big char-string constant), thereby
      eliminating the touch-lots-of-unrelated-pages syndrome.  And get
      rid of the ScanKeyword array in favor of three separate arrays:
      uint16 offsets into the keyword array, uint16 token codes, and
      uint8 keyword categories.  That reduces the overhead per keyword
      to 5 bytes instead of 16 (even less in programs that only need
      one of the token codes and categories); moreover, the binary search
      only touches the offsets array, further reducing its cache footprint.
      This also lets us put the token codes somewhere else than the
      keyword strings are, which avoids some unpleasant build dependencies.
      
      While we're at it, wrap the data used by ScanKeywordLookup into
      a struct that can be treated as an opaque type by most callers.
      That doesn't change things much right now, but it will make it
      less painful to switch to a hash-based lookup method, as is being
      discussed in the mailing list thread.
      
      Most of the change here is associated with adding a generator
      script that can build the new data structure from the same
      list-of-PG_KEYWORD header representation we used before.
      The PG_KEYWORD lists that plpgsql and ecpg used to embed in
      their scanner .c files have to be moved into headers, and the
      Makefiles have to be taught to invoke the generator script.
      This work is also necessary if we're to consider hash-based lookup,
      since the generator script is what would be responsible for
      constructing a hash table.
      
      Aside from saving a few kilobytes in each program that includes
      the keyword table, this seems to speed up raw parsing (flex+bison)
      by a few percent.  So it's worth doing even as it stands, though
      we think we can gain even more with a follow-on patch to switch
      to hash-based lookup.
      
      John Naylor, with further hacking by me
      
      Discussion: https://postgr.es/m/CAJVSVGXdFVU2sgym89XPL=Lv1zOS5=EHHQ8XWNzFL=mTXkKMLw@mail.gmail.com
      afb0d071
  7. 05 Jan, 2019 1 commit
    • Tom Lane's avatar
      Fix program build rule in src/bin/scripts/Makefile. · c5c7fa26
      Tom Lane authored
      Commit 69ae9dcb added a globally-visible "%: %.o" rule, but we failed
      to notice that src/bin/scripts/Makefile already had such a rule.
      Apparently, the later occurrence of the same rule wins in nearly all
      versions of gmake ... but not in the one used by buildfarm member jacana.
      jacana is evidently using the global rule, which says to link "$<",
      ie just the first dependency.  But the scripts makefile needs to
      link "$^", ie all the dependencies listed for the target.
      
      There is, fortunately, no good reason not to use "$^" in the global
      version of the rule, so we can just do that and get rid of the local
      version.
      c5c7fa26
  8. 04 Jan, 2019 6 commits
  9. 03 Jan, 2019 2 commits
    • Tom Lane's avatar
      Use symbolic references for pg_language OIDs in the bootstrap data. · 814c9019
      Tom Lane authored
      This patch teaches genbki.pl to replace pg_language names by OIDs
      in much the same way as it already does for pg_am names etc, and
      converts pg_proc.dat to use such symbolic references in the prolang
      column.
      
      Aside from getting rid of a few more magic numbers in the initial
      catalog data, this means that Gen_fmgrtab.pl no longer needs to read
      pg_language.dat, since it doesn't have to know the OID of the "internal"
      language; now it's just looking for the string "internal".
      
      No need for a catversion bump, since the contents of postgres.bki
      don't actually change at all.
      
      John Naylor
      
      Discussion: https://postgr.es/m/CAJVSVGWtUqxpfAaxS88vEGvi+jKzWZb2EStu5io-UPc4p9rSJg@mail.gmail.com
      814c9019
    • Tom Lane's avatar
      Improve ANALYZE's handling of concurrent-update scenarios. · 7170268e
      Tom Lane authored
      This patch changes the rule for whether or not a tuple seen by ANALYZE
      should be included in its sample.
      
      When we last touched this logic, in commit 51e1445f, we weren't
      thinking very hard about tuples being UPDATEd by a long-running
      concurrent transaction.  In such a case, we might see the pre-image as
      either LIVE or DELETE_IN_PROGRESS depending on timing; and we might see
      the post-image not at all, or as INSERT_IN_PROGRESS.  Since the existing
      code will not sample either DELETE_IN_PROGRESS or INSERT_IN_PROGRESS
      tuples, this leads to concurrently-updated rows being omitted from the
      sample entirely.  That's not very helpful, and it's especially the wrong
      thing if the concurrent transaction ends up rolling back.
      
      The right thing seems to be to sample DELETE_IN_PROGRESS rows just as if
      they were live.  This makes the "sample it" and "count it" decisions the
      same, which seems good for consistency.  It's clearly the right thing
      if the concurrent transaction ends up rolling back; in effect, we are
      sampling as though IN_PROGRESS transactions haven't happened yet.
      Also, this combination of choices ensures maximum robustness against
      the different combinations of whether and in which state we might see the
      pre- and post-images of an update.
      
      It's slightly annoying that we end up recording immediately-out-of-date
      stats in the case where the transaction does commit, but on the other
      hand the stats are fine for columns that didn't change in the update.
      And the alternative of sampling INSERT_IN_PROGRESS rows instead seems
      like a bad idea, because then the sampling would be inconsistent with
      the way rows are counted for the stats report.
      
      Per report from Mark Chambers; thanks to Jeff Janes for diagnosing
      what was happening.  Back-patch to all supported versions.
      
      Discussion: https://postgr.es/m/CAFh58O_Myr6G3tcH3gcGrF-=OExB08PJdWZcSBcEcovaiPsrHA@mail.gmail.com
      7170268e
  10. 02 Jan, 2019 5 commits
    • Tom Lane's avatar
      Don't believe MinMaxExpr is leakproof without checking. · 68a13f28
      Tom Lane authored
      MinMaxExpr invokes the btree comparison function for its input datatype,
      so it's only leakproof if that function is.  Many such functions are
      indeed leakproof, but others are not, and we should not just assume that
      they are.  Hence, adjust contain_leaked_vars to verify the leakproofness
      of the referenced function explicitly.
      
      I didn't add a regression test because it would need to depend on
      some particular comparison function being leaky, and that's a moving
      target, per discussion.
      
      This has been wrong all along, so back-patch to supported branches.
      
      Discussion: https://postgr.es/m/31042.1546194242@sss.pgh.pa.us
      68a13f28
    • Peter Eisentraut's avatar
    • Tom Lane's avatar
      Ensure link commands list *.o files before LDFLAGS. · 69ae9dcb
      Tom Lane authored
      It's important for link commands to list *.o input files before -l
      switches for libraries, as library code may not get pulled into the link
      unless referenced by an earlier command-line entry.  This is certainly
      necessary for static libraries (.a style).  Apparently on some platforms
      it is also necessary for shared libraries, as reported by Donald Dong.
      
      We often put -l switches for within-tree libraries into LDFLAGS, meaning
      that link commands that list *.o files after LDFLAGS are hazardous.
      Most of our link commands got this right, but a few did not.  In
      particular, places that relied on gmake's default implicit link rule
      failed, because that puts LDFLAGS first.  Fix that by overriding the
      built-in rule with our own.  The implicit link rules in
      src/makefiles/Makefile.* for single-.o-file shared libraries mostly
      got this wrong too, so fix them.  I also changed the link rules for the
      backend and a couple of other places for consistency, even though they
      are not (currently) at risk because they aren't adding any -l switches
      to LDFLAGS.
      
      Arguably, the real problem here is that we're abusing LDFLAGS by
      putting -l switches in it and we should stop doing that.  But changing
      that would be quite invasive, so I'm not eager to do so.
      
      Perhaps this is a candidate for back-patching, but so far it seems
      that problems can only be exhibited in test code we don't normally
      build, and at least some of the problems are new in HEAD anyway.
      So I'll refrain for now.
      
      Donald Dong and Tom Lane
      
      Discussion: https://postgr.es/m/CAKABAquXn-BF-vBeRZxhzvPyfMqgGuc74p8BmQZyCFDpyROBJQ@mail.gmail.com
      69ae9dcb
    • Bruce Momjian's avatar
      Update copyright for 2019 · 97c39498
      Bruce Momjian authored
      Backpatch-through: certain files through 9.4
      97c39498
    • Peter Eisentraut's avatar
      Convert unaccent tests to UTF-8 · b6f3649b
      Peter Eisentraut authored
      This makes it easier to add new tests that are specific to Unicode
      features.  The files were previously in KOI8-R.
      
      Discussion: https://www.postgresql.org/message-id/8506.1545111362@sss.pgh.pa.us
      b6f3649b
  11. 01 Jan, 2019 2 commits
    • Michael Paquier's avatar
      Remove configure switch --disable-strong-random · 1707a0d2
      Michael Paquier authored
      This removes a portion of infrastructure introduced by fe0a0b59 to allow
      compilation of Postgres in environments where no strong random source is
      available, meaning that there is no linking to OpenSSL and no
      /dev/urandom (Windows having its own CryptoAPI).  No systems shipped
      this century lack /dev/urandom, and the buildfarm is actually not
      testing this switch at all, so just remove it.  This simplifies
      particularly some backend code which included a fallback implementation
      using shared memory, and removes a set of alternate regression output
      files from pgcrypto.
      
      Author: Michael Paquier
      Reviewed-by: Tom Lane
      Discussion: https://postgr.es/m/20181230063219.GG608@paquier.xyz
      1707a0d2
    • Michael Paquier's avatar
      Fix generation of padding message before encrypting Elgamal in pgcrypto · d880b208
      Michael Paquier authored
      fe0a0b59, which has added a stronger random source in Postgres, has
      introduced a thinko when creating a padding message which gets encrypted
      for Elgamal.  The padding message cannot have zeros, which are replaced
      by random bytes.  However if pg_strong_random() failed, the message
      would finish by being considered in correct shape for encryption with
      zeros.
      
      Author: Tom Lane
      Reviewed-by: Michael Paquier
      Discussion: https://postgr.es/m/20186.1546188423@sss.pgh.pa.us
      Backpatch-through: 10
      d880b208
  12. 31 Dec, 2018 1 commit
    • Michael Paquier's avatar
      Improve comments and logs in do_pg_stop/start_backup · 8d3b389e
      Michael Paquier authored
      The function name pg_stop_backup() has been included for ages in some
      log messages when stopping the backup, which is confusing for base
      backups taken with the replication protocol because this function is
      never called.  Some other comments and messages in this area are
      improved while on it.
      
      The new wording is based on input and suggestions from several people,
      all listed below.
      
      Author: Michael Paquier
      Reviewed-by: Peter Eisentraut, Álvaro Herrera, Tom Lane
      Discussion: https://postgr.es/m/20181221040510.GA12599@paquier.xyz
      8d3b389e