1. 17 Oct, 2018 8 commits
    • Tom Lane's avatar
      Improve some comments related to executor result relations. · 26cb8203
      Tom Lane authored
      es_leaf_result_relations doesn't exist; perhaps this was an old name
      for es_tuple_routing_result_relations, or maybe this comment has gone
      unmaintained through multiple rounds of whacking the code around.
      
      Related comment in execnodes.h was both obsolete and ungrammatical.
      26cb8203
    • Tom Lane's avatar
      48d818ed
    • Tom Lane's avatar
      Fix minor bug in isolationtester. · 9958b2b2
      Tom Lane authored
      If the lock wait query failed, isolationtester would report the
      PQerrorMessage from some other connection, meaning there would be
      no message or an unrelated one.  This seems like a pretty unlikely
      occurrence, but if it did happen, this bug could make it really
      difficult/confusing to figure out what happened.  That seems to
      justify patching all the way back.
      
      In passing, clean up another place where the "wrong" conn was used
      for an error report.  That one's not actually buggy because it's
      a different alias for the same connection, but it's still confusing
      to the reader.
      9958b2b2
    • Peter Eisentraut's avatar
      Fix crash in multi-insert COPY · a7a1b445
      Peter Eisentraut authored
      A bug introduced in 0d5f05cd
      considered the *previous* partition's triggers when deciding whether
      multi-insert can be used.  Rearrange the code so that the current
      partition is considered.
      
      Author: Ashutosh Sharma <ashu.coek88@gmail.com>
      a7a1b445
    • Tom Lane's avatar
      Minor additional improvements for ecpglib/prepare.c. · d8cc1616
      Tom Lane authored
      Avoid allocating never-used entries in stmtCacheEntries[], other than the
      intentionally-unused zero'th entry.  Tie the array size directly to the
      bucket count and size, rather than having undocumented dependencies between
      three magic constants.  Fix the hash calculation to be platform-independent
      --- notably, it was sensitive to the signed'ness of "char" before, not to
      mention having an unnecessary hard-wired dependency on the existence and
      size of type "long long".  (The lack of complaints says it's been a long
      time since anybody tried to build PG on a compiler without "long long",
      and certainly with the requirement for C99 this isn't a live bug anymore.
      But it's still not per project coding style.)  Fix ecpg_auto_prepare's
      new-cache-entry path so that it increments the exec count for the new
      cache entry not the dummy zero'th entry.
      
      The last of those is an actual bug, though one of little consequence;
      the rest is mostly future-proofing and neatnik-ism.  Doesn't seem
      necessary to back-patch.
      d8cc1616
    • Tom Lane's avatar
      Improve tzparse's handling of TZDEFRULES ("posixrules") zone data. · e7eb07f7
      Tom Lane authored
      In the IANA timezone code, tzparse() always tries to load the zone
      file named by TZDEFRULES ("posixrules").  Previously, we'd hacked
      that logic to skip the load in the "lastditch" code path, which we use
      only to initialize the default "GMT" zone during GUC initialization.
      That's critical for a couple of reasons: since we do not support leap
      seconds, we *must not* allow "GMT" to have leap seconds, and since this
      case runs before the GUC subsystem is fully alive, we'd really rather
      not take the risk of pg_open_tzfile throwing any errors.
      
      However, that still left the code reading TZDEFRULES on every other
      call, something we'd noticed to the extent of having added code to cache
      the result so it was only done once per process not a lot of times.
      Andres Freund complained about the static data space used up for the
      cache; but as long as the logic was like this, there was no point in
      trying to get rid of that space.
      
      We can improve matters by looking a bit more closely at what the IANA
      code actually needs the TZDEFRULES data for.  One thing it does is
      that if "posixrules" is a leap-second-aware zone, the leap-second
      behavior will be absorbed into every POSIX-style zone specification.
      However, that's a behavior we'd really prefer to do without, since
      for our purposes the end effect is to render every POSIX-style zone
      name unsupported.  Otherwise, the TZDEFRULES data is used only if
      the POSIX zone name specifies DST but doesn't include a transition
      date rule (e.g., "EST5EDT" rather than "EST5EDT,M3.2.0,M11.1.0").
      That is a minority case for our purposes --- in particular, it
      never happens when tzload() invokes tzparse() to interpret a
      transition date rule string found in a tzdata zone file.
      
      Hence, if we legislate that we're going to ignore leap-second data
      from "posixrules", we can postpone the TZDEFRULES load into the path
      where we actually need to substitute for a missing date rule string.
      That means it will never happen at all in common scenarios, making it
      reasonable to dynamically allocate the cache space when it does happen.
      Even when the data is already loaded, this saves some cycles in the
      common code path since we avoid a memcpy of 23KB or so.  And, IMO at
      least, this is a less ugly hack on the IANA logic than what we had
      before, since it's not messing with the lastditch-vs-regular code paths.
      
      Back-patch to all supported branches, not so much because this is a
      critical change as that I want to keep all our copies of the IANA
      timezone code in sync.
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      e7eb07f7
    • Tom Lane's avatar
      Avoid statically allocating statement cache in ecpglib/prepare.c. · e15aae82
      Tom Lane authored
      This removes a megabyte of storage that isn't used at all in ecpglib's
      default operating mode --- you have to enable auto-prepare to get any
      use out of it.  Seems well worth the trouble to allocate on demand.
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      e15aae82
    • Tom Lane's avatar
      Formatting cleanup in ecpglib/prepare.c. · 92dff341
      Tom Lane authored
      Looking at this code made my head hurt.  Format the comments more
      like the way it's done elsewhere, break a few overly long lines.
      No actual code changes in this commit.
      92dff341
  2. 16 Oct, 2018 13 commits
    • Andres Freund's avatar
      Reorder FmgrBuiltin members, saving 25% in size. · 28d750c0
      Andres Freund authored
      That's worth it, as fmgr_builtins is frequently accessed, and as
      fmgr_builtins is one of the biggest constant variables in a backend.
      
      On most 64bit systems this will change the size of the struct from
      32byte to 24bytes. While that could make indexing into the array
      marginally more expensive, the higher cache hit ratio is worth more,
      especially because these days fmgr_builtins isn't searched with a
      binary search anymore (c.f. 212e6f34).
      
      Discussion: https://postgr.es/m/20181016201145.aa2dfeq54rhqzron@alap3.anarazel.de
      28d750c0
    • Tom Lane's avatar
      Back off using -isysroot on Darwin. · 68fc227d
      Tom Lane authored
      Rethink the solution applied in commit 5e221713 to get PL/Tcl to
      build on macOS Mojave.  I feared that adding -isysroot globally might
      have undesirable consequences, and sure enough Jakob Egger reported
      one: it complicates building extensions with a different Xcode version
      than was used for the core server.  (I find that a risky proposition
      in general, but apparently it works most of the time, so we shouldn't
      break it if we don't have to.)
      
      We'd already adopted the solution for PL/Perl of inserting the sysroot
      path directly into the -I switches used to find Perl's headers, and we
      can do the same thing for PL/Tcl by changing the -iwithsysroot switch
      that Apple's tclConfig.sh reports.  This restricts the risks to PL/Perl
      and PL/Tcl themselves and directly-dependent extensions, which is a lot
      more pleasing in general than a global -isysroot switch.
      
      Along the way, tighten the test to see if we need to inject the sysroot
      path into $perl_includedir, as I'd speculated about upthread but not
      gotten round to doing.
      
      As before, back-patch to all supported versions.
      
      Discussion: https://postgr.es/m/20840.1537850987@sss.pgh.pa.us
      68fc227d
    • Andres Freund's avatar
      Mark constantly allocated dest receiver as const. · 93ca02e0
      Andres Freund authored
      This allows the compiler / linker to mark affected pages as read-only.
      
      Doing so requires casting constness away, as CreateDestReceiver()
      returns both constant and non-constant dest receivers. That's fine
      though, as any modification of the statically allocated receivers
      would already have been a bug (and would now be caught on some
      platforms).
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      93ca02e0
    • Andres Freund's avatar
      Add macro to cast away const without allowing changes to underlying type. · d1211c63
      Andres Freund authored
      The new unconsitify(underlying_type, var) macro allows to cast
      constness away from a variable, but doesn't allow changing the
      underlying type.  Enforcement of the latter currently only works for
      gcc like compilers.
      
      Please note IT IS NOT SAFE to cast constness away if the variable will ever
      be modified (it would be undefined behaviour). Doing so anyway can cause
      compiler misoptimizations or runtime crashes (modifying readonly memory).
      It is only safe to use when the the variable will not be modified, but API
      design or language restrictions prevent you from declaring that
      (e.g. because a function returns both const and non-const variables).
      
      This'll be used in an upcoming change, but seems like it's independent
      infrastructure.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      d1211c63
    • Tom Lane's avatar
      Be smarter about age-counter overflow in formatting.c caches. · 2c300c68
      Tom Lane authored
      The previous code here simply threw away whatever it knew about cache
      entry ages whenever a counter overflow occurred.  Since the counter
      is int width and will be bumped once per format function execution,
      overflows are not really so rare as to not be worth thinking about.
      Instead, let's deal with the situation by halving all the age values,
      essentially rescaling the age metric.  In that way, we retain a
      pretty accurate (if not quite perfect) idea of which entries are oldest.
      2c300c68
    • Tom Lane's avatar
      Avoid rare race condition in privileges.sql regression test. · f7a953c2
      Tom Lane authored
      We created a temp table, then switched to a new session, leaving
      the old session to clean up its temp objects in background.  If that
      took long enough, the eventual attempt to drop the user that owns
      the temp table could fail, as exhibited today by sidewinder.
      Fix by dropping the temp table explicitly when we're done with it.
      
      It's been like this for quite some time, so back-patch to all
      supported branches.
      
      Report: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder&dt=2018-10-16%2014%3A45%3A00
      f7a953c2
    • Tom Lane's avatar
      Avoid statically allocating formatting.c's format string caches. · fd85e9f7
      Tom Lane authored
      This eliminates circa 120KB of static data from Postgres' memory
      footprint.  In some usage patterns that space will get allocated
      anyway, but in many processes it never will be allocated.
      
      We can improve matters further by allocating only as many cache
      entries as we actually use, rather than allocating the whole array
      on first use.  However, to avoid wasting lots of space due to
      palloc's habit of rounding requests up to power-of-2 sizes, tweak
      the maximum cacheable format string length to make the struct sizes
      be powers of 2 or just less.  The sizes I chose make the maximums
      a little bit less than they were before, but I doubt it matters much.
      
      While at it, rearrange struct FormatNode to avoid wasting quite so
      much padding space.  This change actually halves the size of that
      struct on 64-bit machines.
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      fd85e9f7
    • Andres Freund's avatar
      Correct constness of system attributes in heap.c & prerequisites. · 02a30a09
      Andres Freund authored
      This allows the compiler / linker to mark affected pages as read-only.
      
      There's a fair number of pre-requisite changes, to allow the const
      properly be propagated. Most of consts were already required for
      correctness anyway, just not represented on the type-level.  Arguably
      we could be more aggressive in using consts in related code, but..
      
      This requires using a few of the types underlying typedefs that
      removes pointers (e.g. const NameData *) as declaring the typedefed
      type constant doesn't have the same meaning (it makes the variable
      const, not what it points to).
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      02a30a09
    • Tom Lane's avatar
      Make PostgresNode.pm's poll_query_until() more chatty about failures. · c015ccb3
      Tom Lane authored
      Reporting only the stderr is unhelpful when the problem is that the
      server output we're getting doesn't match what was expected.  So we
      should report the query output too; and just for good measure, let's
      print the query we used and the output we expected.
      
      Back-patch to 9.5 where poll_query_until was introduced.
      
      Discussion: https://postgr.es/m/17913.1539634756@sss.pgh.pa.us
      c015ccb3
    • Tom Lane's avatar
      Improve stability of recently-added regression test case. · 17d6a8fb
      Tom Lane authored
      Commit b5febc1d added a contrib/btree_gist test case that has been
      observed to fail in the buildfarm as a result of background auto-analyze
      updating stats and changing the selected plan.  Forestall that by
      forcibly analyzing in foreground, instead.  The new plan choice is
      just as good for our purposes, since we really only care that an
      index-only plan does not get selected.
      
      Back-patch to 9.5, like the previous patch.
      
      Discussion: https://postgr.es/m/14643.1539629304@sss.pgh.pa.us
      17d6a8fb
    • Tom Lane's avatar
      Avoid statically allocating gmtsub()'s timezone workspace. · 3dfef0c5
      Tom Lane authored
      localtime.c's "struct state" is a rather large object, ~23KB.  We were
      statically allocating one for gmtsub() to use to represent the GMT
      timezone, even though that function is not at all heavily used and is
      never reached in most backends.  Let's malloc it on-demand, instead.
      
      This does pose the question of how to handle a malloc failure, but
      there's already a well-defined error report convention here, ie
      set errno and return NULL.
      
      We have but one caller of pg_gmtime in HEAD, and two in back branches,
      neither of which were troubling to check for error.  Make them do so.
      The possible errors are sufficiently unlikely (out-of-range timestamp,
      and now malloc failure) that I think elog() is adequate.
      
      Back-patch to all supported branches to keep our copies of the IANA
      timezone code in sync.  This particular change is in a stanza that
      already differs from upstream, so it's a wash for maintenance purposes
      --- but only as long as we keep the branches the same.
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      3dfef0c5
    • Andres Freund's avatar
      Correct constness of a few variables. · 62649bad
      Andres Freund authored
      This allows the compiler / linker to mark affected pages as read-only.
      
      There's other cases, but they're a bit more invasive, and should go
      through some review. These are easy.
      
      They were found with
      objdump -j .data -t src/backend/postgres|awk '{print $4, $5, $6}'|sort -r|less
      
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya@alap3.anarazel.de
      62649bad
    • Andres Freund's avatar
      Move TupleTableSlots boolean member into one flag variable. · c5257345
      Andres Freund authored
      There's several reasons for this change:
      1) It reduces the total size of TupleTableSlot / reduces alignment
         padding, making the commonly accessed members fit into a single
         cacheline (but we currently do not force proper alignment, so
         that's not yet guaranteed to be helpful)
      2) Combining the booleans into a flag allows to combine read/writes
         from memory.
      3) With the upcoming slot abstraction changes, it allows to have core
         and extended flags, in a memory efficient way.
      
      Author: Ashutosh Bapat and Andres Freund
      Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
      c5257345
  3. 15 Oct, 2018 6 commits
    • Andres Freund's avatar
      Move generic slot support functions from heaptuple.c into execTuples.c. · 9d906f11
      Andres Freund authored
      heaptuple.c was never a particular good fit for slot_getattr(),
      slot_getsomeattrs() and slot_getmissingattrs(), but in upcoming
      changes slots will be made more abstract (allowing slots that contain
      different types of tuples), making it clearly the wrong place.
      
      Note that slot_deform_tuple() remains in it's current place, as it
      clearly deals with a HeapTuple.  getmissingattrs() also remains, but
      it's less clear that that's correct - but execTuples.c wouldn't be the
      right place.
      
      Author: Ashutosh Bapat.
      Discussion: https://postgr.es/m/20180220224318.gw4oe5jadhpmcdnm@alap3.anarazel.de
      9d906f11
    • Thomas Munro's avatar
      Move the replication lag tracker into heap memory. · e73ca79f
      Thomas Munro authored
      Andres Freund complained about the 128KB of .bss occupied by LagTracker.
      It's only needed in the walsender process, so allocate it in heap
      memory there.
      
      Author: Thomas Munro
      Discussion: https://postgr.es/m/20181015200754.7y7zfuzsoux2c4ya%40alap3.anarazel.de
      e73ca79f
    • Tom Lane's avatar
      Check for stack overrun in standard_ProcessUtility(). · d48da369
      Tom Lane authored
      ProcessUtility can recurse, and indeed can be driven to infinite
      recursion, so it ought to have a check_stack_depth() call.  This
      covers the reported bug (portal trying to execute itself) and a bunch
      of other cases that could perhaps arise somewhere.
      
      Per bug #15428 from Malthe Borch.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/15428-b3c2915ec470b033@postgresql.org
      d48da369
    • Peter Eisentraut's avatar
      pgbench: Report errors during run better · 5b75a4f8
      Peter Eisentraut authored
      When an error occurs during a benchmark run, exit with a nonzero exit
      code and write a message at the end.  Previously, it would just print
      the error message when it happened but then proceed to print the run
      summary normally and exit with status 0.  To still allow
      distinguishing setup from run-time errors, we use exit status 2 for
      the new state, whereas existing errors during pgbench initialization
      use exit status 1.
      Reviewed-by: default avatarFabien COELHO <coelho@cri.ensmp.fr>
      5b75a4f8
    • Peter Eisentraut's avatar
      Make spelling of "acknowledgment" consistent · 35584fd0
      Peter Eisentraut authored
      I used the preferred U.S. spelling, as we do in other cases.
      35584fd0
    • Peter Eisentraut's avatar
      Fixes for "Glyph not available" warnings from FOP · 9274c577
      Peter Eisentraut authored
      With the PostgreSQL 11 release notes acknowledgments list, FOP reported
      
      WARNING: Glyph "?" (0x144, nacute) not available in font "Times-Roman".
      WARNING: Glyph "?" (0x15e, Scedilla) not available in font "Times-Roman".
      WARNING: Glyph "?" (0x15f, scedilla) not available in font "Times-Roman".
      WARNING: Glyph "?" (0x131, dotlessi) not available in font "Times-Roman".
      
      This is because we have some new contributors whose names use letters
      that we haven't used before, and apparently FOP can't handle them out
      of the box.
      
      For now, just fix this by "unaccenting" those names.  In the future,
      maybe this can be fixed better with a different font configuration.
      
      There is also another warning
      
      WARNING: Glyph "?" (0x3c0, pi) not available in font "Times-Roman".
      
      but that existed in previous releases and is not touched here.
      9274c577
  4. 14 Oct, 2018 7 commits
    • Alexander Korotkov's avatar
      Add missed tag in bloom.sgml · 981b64f8
      Alexander Korotkov authored
      Backpatch commits don't contain this error.
      981b64f8
    • Alexander Korotkov's avatar
      contrib/bloom documentation improvement · 98afb839
      Alexander Korotkov authored
      This commit documents rounding of "length" parameter and absence of support
      for unique indexes and NULLs searching.  Backpatch to 9.6 where contrib/bloom
      was introduced.
      
      Discussion: https://postgr.es/m/CAF4Au4wPQQ7EHVSnzcLjsbY3oLSzVk6UemZLD1Sbmwysy3R61g%40mail.gmail.com
      Author: Oleg Bartunov with minor editorialization by me
      Backpatch-through: 9.6
      98afb839
    • Tom Lane's avatar
      Make some subquery-using test cases a bit more robust. · b403ea43
      Tom Lane authored
      These test cases could be adversely affected by an upcoming change
      to allow pullup of FROM-less subqueries.  Tweak them to ensure that
      they'll continue to test what they did before.
      
      Discussion: https://postgr.es/m/5395.1539275668@sss.pgh.pa.us
      b403ea43
    • Tom Lane's avatar
      Use PlaceHolderVars within the quals of a FULL JOIN. · 7d4a10e2
      Tom Lane authored
      This prevents failures in cases where we pull up a constant or var-free
      expression from a subquery and put it into a full join's qual.  That can
      result in not recognizing the qual as containing a mergejoin-able or
      hashjoin-able condition.  A PHV prevents the problem because it is still
      recognized as belonging to the side of the join the subquery is in.
      
      I'm not very sure about the net effect of this change on plan quality.
      In "typical" cases where the join keys are Vars, nothing changes.
      In an affected case, the PHV-wrapped expression is less likely to be seen
      as equal to PHV-less instances below the join, but more likely to be seen
      as equal to similar expressions above the join, so it may end up being a
      wash.  In the one existing case where there's any visible change in a
      regression-test plan, it amounts to referencing a lower computation of a
      COALESCE result instead of recomputing it, which seems like a win.
      
      Given my uncertainty about that and the lack of field complaints,
      no back-patch, even though this is a very ancient problem.
      
      Discussion: https://postgr.es/m/32090.1539378124@sss.pgh.pa.us
      7d4a10e2
    • Tom Lane's avatar
      Clean up/tighten up coercibility checks in opr_sanity regression test. · e9f42d52
      Tom Lane authored
      With the removal of the old abstime type, there are no longer any cases
      in this test where we need to use the weaker castcontext-ignoring form
      of binary coercibility check.  (The other major source of such headaches,
      apparently-incompatible hash functions, is now hashvalidate()'s problem
      not this test script's problem.)  Hence, just use binary_coercible()
      everywhere, and remove the comments explaining why we don't do so ---
      which were broken anyway by cda6a8d0.
      
      I left physically_coercible() in place but renamed it to better
      match what it's actually testing, and added some comments.
      
      Also, in test queries that have an assumption about the maximum number
      of function arguments they need to handle, add a clause to make them fail
      if someday there's a relevant function with more arguments.  Otherwise
      we're likely not to notice that we need to extend the queries.
      
      Discussion: https://postgr.es/m/27637.1539388060@sss.pgh.pa.us
      e9f42d52
    • Michael Paquier's avatar
      Avoid duplicate XIDs at recovery when building initial snapshot · 1df21ddb
      Michael Paquier authored
      On a primary, sets of XLOG_RUNNING_XACTS records are generated on a
      periodic basis to allow recovery to build the initial state of
      transactions for a hot standby.  The set of transaction IDs is created
      by scanning all the entries in ProcArray.  However it happens that its
      logic never counted on the fact that two-phase transactions finishing to
      prepare can put ProcArray in a state where there are two entries with
      the same transaction ID, one for the initial transaction which gets
      cleared when prepare finishes, and a second, dummy, entry to track that
      the transaction is still running after prepare finishes.  This way
      ensures a continuous presence of the transaction so as callers of for
      example TransactionIdIsInProgress() are always able to see it as alive.
      
      So, if a XLOG_RUNNING_XACTS takes a standby snapshot while a two-phase
      transaction finishes to prepare, the record can finish with duplicated
      XIDs, which is a state expected by design.  If this record gets applied
      on a standby to initial its recovery state, then it would simply fail,
      so the odds of facing this failure are very low in practice.  It would
      be tempting to change the generation of XLOG_RUNNING_XACTS so as
      duplicates are removed on the source, but this requires to hold on
      ProcArrayLock for longer and this would impact all workloads,
      particularly those using heavily two-phase transactions.
      
      XLOG_RUNNING_XACTS is also actually used only to initialize the standby
      state at recovery, so instead the solution is taken to discard
      duplicates when applying the initial snapshot.
      
      Diagnosed-by: Konstantin Knizhnik
      Author: Michael Paquier
      Discussion: https://postgr.es/m/0c96b653-4696-d4b4-6b5d-78143175d113@postgrespro.ru
      Backpatch-through: 9.3
      1df21ddb
    • Tom Lane's avatar
      8f850f2c
  5. 13 Oct, 2018 3 commits
  6. 12 Oct, 2018 3 commits