1. 24 Apr, 2020 8 commits
    • Tom Lane's avatar
      Improve placement of "display name" comment in win32_tzmap[] entries. · bd8c5cee
      Tom Lane authored
      Sticking this comment at the end of the last line was a bad idea: it's
      not particularly readable, and it tempts pgindent to mess with line
      breaks within the comment, which in turn reveals that win32tzlist.pl's
      clean_displayname() does the wrong thing to clean up such line breaks.
      While that's not hard to fix, there's basically no excuse for this
      arrangement to begin with, especially since it makes the table layout
      needlessly vary across back branches with different pgindent rules.
      Let's just put the comment inside the braces, instead.
      
      This commit just moves and reformats the comments, and updates
      win32tzlist.pl to match; there's no actual data change.
      
      Per odd-looking results from Juan José Santamaría Flecha.
      Back-patch, since the point is to make win32_tzmap[] look the
      same in all supported branches again.
      
      Discussion: https://postgr.es/m/5752.1587740484@sss.pgh.pa.us
      bd8c5cee
    • Tom Lane's avatar
      Doc: update section 9.13 for new function table layout. · f8d3e2ab
      Tom Lane authored
      This includes the usual amount of editorial cleanup, such as
      correcting wrong or less-helpful-than-they-could-be examples.
      
      I moved the two tsvector-updating triggers into "9.28 Trigger
      Functions", which seems like a better home for them.  (I believe
      that section didn't exist when this text was originally written.)
      f8d3e2ab
    • Bruce Momjian's avatar
      git_changelog: use modern format for rel branch names in example · 395a9a12
      Bruce Momjian authored
      e.g., REL_12_STABLE
      395a9a12
    • Robert Haas's avatar
      Try to avoid compiler warnings in optimized builds. · 05021a2c
      Robert Haas authored
      Per report from Andres Freund, who also says that this fix
      works for him.
      
      Discussion: http://postgr.es/m/20200405193118.alprgmozhxcfabkw@alap3.anarazel.de
      05021a2c
    • Tom Lane's avatar
      Repair performance regression in information_schema.triggers view. · baf17ad9
      Tom Lane authored
      Commit 32ff2691 introduced use of rank() into the triggers view to
      calculate the spec-mandated action_order column.  As written, this
      prevents query constraints on the table-name column from being pushed
      below the window aggregate step.  That's bad for performance of this
      typical usage pattern, since the view now has to be evaluated for all
      tables not just the one(s) the user wants to see.  It's also the cause
      of some recent buildfarm failures, in which trying to evaluate the view
      rows for triggers in process of being dropped resulted in "cache lookup
      failed for function NNN" errors.  Those rows aren't of interest to the
      test script doing the query, but the filter that would eliminate them
      is being applied too late.  None of this happened before the rank()
      call was there, so it's a regression compared to v10 and before.
      
      We can improve matters by changing the rank() call so that instead of
      partitioning by OIDs, it partitions by nspname and relname, casting
      those to sql_identifier so that they match the respective view output
      columns exactly.  The planner has enough intelligence to know that
      constraints on partitioning columns are safe to push down, so this
      eliminates the performance problem and the regression test failure
      risk.  We could make the other partitioning columns match view outputs
      as well, but it'd be more complicated and the performance benefits
      are questionable.
      
      Side note: as this stands, the planner will push down constraints on
      event_object_table and trigger_schema, but not on event_object_schema,
      because it checks for ressortgroupref matches not expression
      equivalence.  That might be worth improving someday, but it's not
      necessary to fix the immediate concern.
      
      Back-patch to v11 where the rank() call was added.  Ordinarily we'd not
      change information_schema in released branches, but the test failure has
      been seen in v12 and presumably could happen in v11 as well, so we need
      to do this to keep the buildfarm happy.  The change is harmless so far
      as users are concerned.  Some might wish to apply it to existing
      installations if performance of this type of query is of concern,
      but those who don't are no worse off.
      
      I bumped catversion in HEAD as a pro forma matter (there's no
      catalog incompatibility that would really require a re-initdb).
      Obviously that can't be done in the back branches.
      
      Discussion: https://postgr.es/m/5891.1587594470@sss.pgh.pa.us
      baf17ad9
    • Tom Lane's avatar
      Update time zone data files to tzdata release 2020a. · 4cac3a49
      Tom Lane authored
      DST law changes in Morocco and the Canadian Yukon.
      Historical corrections for Shanghai.
      
      The America/Godthab zone is renamed to America/Nuuk to reflect
      current English usage; however, the old name remains available as a
      compatibility link.
      4cac3a49
    • Peter Eisentraut's avatar
      3a896157
    • Michael Paquier's avatar
      Remove some unstable parts from new TAP test for archive status check · f9c1b8db
      Michael Paquier authored
      The test is proving to have timing issues when looking at archive status
      files on standbys after crash recovery, while other parts of the test
      rely on pg_stat_archiver as a wait point to make sure that a given state
      of the archiving is reached.  The coverage is not heavily impacted by
      the removal those extra tests.
      
      Per reports from several buildfarm animals, like crake, piculet,
      culicidae and francolin.
      
      Discussion: https://postgr.es/m/20200424005929.GK33034@paquier.xyz
      Backpatch-through: 9.5
      f9c1b8db
  2. 23 Apr, 2020 9 commits
    • Michael Paquier's avatar
      Fix handling of WAL segments ready to be archived during crash recovery · 4e87c483
      Michael Paquier authored
      78ea8b5d has fixed an issue related to the recycling of WAL segments on
      standbys depending on archive_mode.  However, it has introduced a
      regression with the handling of WAL segments ready to be archived during
      crash recovery, causing those files to be recycled without getting
      archived.
      
      This commit fixes the regression by tracking in shared memory if a live
      cluster is either in crash recovery or archive recovery as the handling
      of WAL segments ready to be archived is different in both cases (those
      WAL segments should not be removed during crash recovery), and by using
      this new shared memory state to decide if a segment can be recycled or
      not.  Previously, it was not possible to know if a cluster was in crash
      recovery or archive recovery as the shared state was able to track only
      if recovery was happening or not, leading to the problem.
      
      A set of TAP tests is added to close the gap here, making sure that WAL
      segments ready to be archived are correctly handled when a cluster is in
      archive or crash recovery with archive_mode set to "on" or "always", for
      both standby and primary.
      
      Reported-by: Benoît Lobréau
      Author: Jehan-Guillaume de Rorthais
      Reviewed-by: Kyotaro Horiguchi, Fujii Masao, Michael Paquier
      Discussion: https://postgr.es/m/20200331172229.40ee00dc@firost
      Backpatch-through: 9.5
      4e87c483
    • Tom Lane's avatar
      Remove ACLDEBUG #define and associated code. · 3436c5e2
      Tom Lane authored
      In the footsteps of aaf069aa, remove ACLDEBUG, which was the only
      other remaining undocumented symbol in pg_config_manual.h.  The fact
      that nobody had bothered to document it in seventeen years is a good
      clue to its usefulness.  In practice, none of the tracing logic it
      enabled would be of any value without additional effort.
      
      Discussion: https://postgr.es/m/6631.1587565046@sss.pgh.pa.us
      3436c5e2
    • Tom Lane's avatar
      Remove useless (and broken) logging logic in memory context functions. · ee88ef55
      Tom Lane authored
      Nobody really uses this stuff, especially not since we created
      valgrind-based infrastructure that does the same thing better.
      It is thus unsurprising that the generation.c and slab.c versions
      were actually broken.  Rather than fix 'em, let's just remove 'em.
      
      Alexander Lakhin
      
      Discussion: https://postgr.es/m/8936216c-3492-3f6e-634b-d638fddc5f91@gmail.com
      ee88ef55
    • Tom Lane's avatar
      Doc: update section 9.12 for new function table layout. · 5b0aa112
      Tom Lane authored
      Also rearrange that page a bit for more consistency and less
      duplication.
      
      In passing, fix erroneous examples of the results of abbrev(cidr)
      in datatype.sgml, and do a bit of copy-editing there.
      5b0aa112
    • Robert Haas's avatar
      Also rename 'struct manifest_info'. · ab7646ff
      Robert Haas authored
      The previous commit renamed the struct's typedef, but not the struct
      name itself.
      ab7646ff
    • Robert Haas's avatar
      Rename exposed identifiers to say "backup manifest". · 3989dbdf
      Robert Haas authored
      Function names declared "extern" now use BackupManifest in the name
      rather than just Manifest, and data types use backup_manifest
      rather than just manifest.
      
      Per note from Michael Paquier.
      
      Discussion: http://postgr.es/m/20200418125713.GG350229@paquier.xyz
      3989dbdf
    • Andres Freund's avatar
      Fix transient memory leak for SRFs in FROM. · 299298bc
      Andres Freund authored
      In a9c35cf8 I changed ExecMakeTableFunctionResult() to dynamically
      allocate the FunctionCallInfo used to call the SRF. Unfortunately I
      did not account for the fact that the surrounding memory context has
      query lifetime, leading to a leak till the end of the query.
      
      In most cases the leak is fairly inconsequential, but if the
      FunctionScan is done many times in the query, the leak can add
      up. This happens e.g. if the function scan is on the inner side of a
      nested loop, due to a lateral join.
      EXPLAIN SELECT sum(f) FROM generate_series(1, 100000000) g(i), generate_series(i, i+1) f;
      quickly shows the leak.
      
      Instead of explicitly freeing the FunctionCallInfo it seems better to
      make sure all the per-set temporary state in
      ExecMakeTableFunctionResult() is cleaned up wholesale. Currently
      that's probably just the FunctionCallInfo allocation, but since
      there's some initialization work, and since there's already an
      appropriate context, this seems like a more robust approach.
      
      Bug: #16112
      Reported-By: Ben Cornett
      Author: Andres Freund
      Reviewed-By: Tom Lane
      Discussion: https://postgr.es/m/16112-4448bbf55a404189%40postgresql.org
      Backpatch: 12, a9c35cf8
      299298bc
    • Fujii Masao's avatar
      Fix option related issues in pg_verifybackup. · 0a89e93b
      Fujii Masao authored
      This commit does:
      
      - get rid of the garbage code for unused --print-parse-wal option.
      - add help message for --quiet option into usage().
      - fix typo of option name in help message.
      
      Author: Fujii Masao
      Reviewed-by: Robert Haas
      Discussion: https://postgr.es/m/ff4710f7-2331-4f6b-012e-d76da3275e91@oss.nttdata.com
      0a89e93b
    • Tom Lane's avatar
      Doc: improve description of geometric multiplication/division. · 1cc34640
      Tom Lane authored
      David Johnston reminded me that the per-point calculations being done
      by these operators are equivalent to complex multiplication/division.
      (Once I would've recognized that immediately, but it's been too long
      since I did any of that sort of math.)
      
      Also put in a footnote mentioning that "rotation" of a box doesn't do
      what you might expect, as I'd griped about in the referenced thread.
      
      Discussion: https://postgr.es/m/158110996889.1089.4224139874633222837@wrigleys.postgresql.org
      1cc34640
  3. 22 Apr, 2020 7 commits
  4. 21 Apr, 2020 13 commits
    • Michael Paquier's avatar
      Fix single-record reads to use restore_command if available in pg_rewind · cd123234
      Michael Paquier authored
      readOneRecord() is used now when looking for a checkpoint record to
      check if the target server is an ancestor of the source across multiple
      timelines, and using a restore_command if available improves the
      stability of the operation.  This part was missed in a7e8ece4.
      
      Reported-by: Kyotaro Horiguchi
      Discussion: https://postgr.es/m/20200421.150830.1410714948345179794.horikyota.ntt@gmail.com
      cd123234
    • Alvaro Herrera's avatar
      psql \d: Display table where trigger is defined, if inherited · c33869cc
      Alvaro Herrera authored
      It's important to know that a trigger is cloned from a parent table,
      because of the behavior that the trigger is dropped on detach.  Make
      psql's \d display it.
      
      We'd like to backpatch, but lack of the pg_trigger.tgparentid column
      makes it more difficult.  Punt for now.  If somebody wants to volunteer
      an implementation that reads pg_depend on older versions, that can
      probably be backpatched.
      
      Authors: Justin Pryzby, Amit Langote, Álvaro Herrera
      Discussion: https://postgr.es/m/20200419002206.GM26953@telsasoft.com
      c33869cc
    • Michael Paquier's avatar
      Fix memory leak in libpq when using sslmode=verify-full · 27dbe1a1
      Michael Paquier authored
      Checking if Subject Alternative Names (SANs) from a certificate match
      with the hostname connected to leaked memory after each lookup done.
      
      This is broken since acd08d76 that added support for SANs in SSL
      certificates, so backpatch down to 9.5.
      
      Author: Roman Peshkurov
      Reviewed-by: Hamid Akhtar, Michael Paquier, David Steele
      Discussion: https://postgr.es/m/CALLDf-pZ-E3mjxd5=bnHsDu9zHEOnpgPgdnO84E2RuwMCjjyPw@mail.gmail.com
      Backpatch-through: 9.5
      27dbe1a1
    • Alvaro Herrera's avatar
      Document partitiong tables ancillary object handling some more · 8803506c
      Alvaro Herrera authored
      Add a couple of lines to make it explicit that indexes, constraints,
      triggers are added, removed, or left alone.
      
      Backpatch to pg11.
      
      Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
      Reviewed-by: default avatarJustin Pryzby <pryzby@telsasoft.com>
      Discussion: https://postgr.es/m/20200421162038.GA18628@alvherre.pgsql
      8803506c
    • Tom Lane's avatar
      Fix possible crash during FATAL exit from reindexing. · d12bdba7
      Tom Lane authored
      index.c supposed that it could just use a PG_TRY block to clean up the
      state associated with an active REINDEX operation.  However, that code
      doesn't run if we do a FATAL exit --- for example, due to a SIGTERM
      shutdown signal --- while the REINDEX is happening.  And that state does
      get consulted during catalog accesses, which makes it problematic if we
      do any catalog accesses during shutdown --- for example, to clean up any
      temp tables created in the session.
      
      If this combination of circumstances occurred, we could find ourselves
      trying to access already-freed memory.  In debug builds that'd fairly
      reliably cause an assertion failure.  In production we might often
      get away with it, but with some bad luck it could cause a core dump.
      
      Another possible bad outcome is an erroneous conclusion that an
      index-to-be-accessed is being reindexed; but it looks like that would
      be unlikely to have any consequences worse than failing to drop temp
      tables right away.  (They'd still get dropped by the next session that
      uses that temp schema.)
      
      To fix, get rid of the use of PG_TRY here, and instead hook into
      the transaction abort mechanisms to clean up reindex state.
      
      Per bug #16378 from Alexander Lakhin.  This has been wrong for a
      very long time, so back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/16378-7a70ca41b3ec2009@postgresql.org
      d12bdba7
    • Tom Lane's avatar
      Fix minor violations of FunctionCallInvoke usage protocol. · 5836d326
      Tom Lane authored
      Working on commit 1c455078 led me to check through FunctionCallInvoke
      call sites to see if every one was being honest about (a) making sure
      that fcinfo.isnull is initially false, and (b) checking its state after
      the call.  Sure enough, I found some violations.
      
      The main one is that finalize_partialaggregate re-used serialfn_fcinfo
      without resetting isnull, even though it clearly intends to cater for
      serialfns that return NULL.  There would only be an issue with a
      non-strict serialfn, since it's unlikely that a serialfn would return
      NULL for non-null input.  We have no non-strict serialfns in core, and
      there may be none in the wild either, which would account for the lack
      of complaints.  Still, it's clearly wrong, so back-patch that fix to
      9.6 where finalize_partialaggregate was introduced.
      
      Also, arrayfuncs.c and rowtypes.c contained various callers that were
      not bothering to check for result nulls.  While what's being called is
      a comparison or hash function that probably *shouldn't* return null,
      that's a lousy excuse for not having any check at all.  There are
      existing places that just Assert(!fcinfo->isnull) in comparable
      situations, so I added that to the places that were calling btree
      comparison or hash support functions.  In the places calling
      boolean-returning equality functions, it's quite cheap to have them
      treat isnull as FALSE, so make those places do that.  Also remove some
      "locfcinfo->isnull = false" assignments that are unnecessary given the
      assumption that no previous call returned null.  These changes seem like
      mostly neatnik-ism or debugging support, so I didn't back-patch.
      5836d326
    • Alvaro Herrera's avatar
      Fix detaching partitions with cloned row triggers · afccd76f
      Alvaro Herrera authored
      When a partition is detached, any triggers that had been cloned from its
      parent were not properly disentangled from its parent triggers.
      This resulted in triggers that could not be dropped because they
      depended on the trigger in the trigger in the no-longer-parent table:
        ALTER TABLE t DETACH PARTITION t1;
        DROP TRIGGER trig ON t1;
          ERROR:  cannot drop trigger trig on table t1 because trigger trig on table t requires it
          HINT:  You can drop trigger trig on table t instead.
      
      Moreover the table can no longer be re-attached to its parent, because
      the trigger name is already taken:
        ALTER TABLE t ATTACH PARTITION t1 FOR VALUES FROM (1)TO(2);
          ERROR:  trigger "trig" for relation "t1" already exists
      
      The former is a bug introduced in commit 86f57594.  (The latter is
      not necessarily a bug, but it makes the bug more uncomfortable.)
      
      To avoid the complexity that would be needed to tell whether the trigger
      has a local definition that has to be merged with the one coming from
      the parent table, establish the behavior that the trigger is removed
      when the table is detached.
      
      Backpatch to pg11.
      
      Author: Justin Pryzby <pryzby@telsasoft.com>
      Reviewed-by: default avatarAmit Langote <amitlangote09@gmail.com>
      Reviewed-by: default avatarÁlvaro Herrera <alvherre@alvh.no-ip.org>
      Discussion: https://www.postgresql.org/message-id/flat/20200408152412.GZ2228@telsasoft.com
      afccd76f
    • Peter Geoghegan's avatar
      Consider outliers in split interval calculation. · 1542e16f
      Peter Geoghegan authored
      Commit 0d861bbb, which introduced deduplication to nbtree, added some
      logic to take large posting list tuples into account when choosing a
      split point.  We subtract firstright posting list overhead from the
      projected new high key size when calculating leftfree/rightfree values
      for an affected candidate split point.  Posting list tuples aren't
      special to nbtsplitloc.c, but taking them into account like this makes a
      huge difference in practice.  Posting list tuples are frequently tuple
      size outliers.
      
      However, commit 0d861bbb missed a closely related issue: split interval
      itself is calculated based on the assumption that tuples on the page
      being split are roughly equisized.  That assumption was acceptable back
      when commit fab25024 taught the logic for choosing a split point about
      suffix truncation, but it's pretty questionable now that very large
      tuple sizes are common.  This oversight led to unbalanced page splits in
      low cardinality multi-column indexes when deduplication was used: page
      splits that don't give sufficient weight to how unbalanced the split is
      when the interval happens to include some large posting list tuples (and
      when most other tuples on the page are not so large).
      
      Nail this down by calculating an initial split interval in a way that's
      attuned to the actual cost that we want to keep under control (not a
      fuzzy proxy for the cost): apply a leftfree + rightfree evenness test to
      each candidate split point that actually gets included in the split
      interval (for the default strategy).  This replaces logic that used a
      percentage of all legal split points for the page as the basis of the
      initial split interval.
      
      Discussion: https://postgr.es/m/CAH2-WznJt5aT2uUB2Bs+JBLdwe0XTX67+xeLFcaNvCKxO=QBVQ@mail.gmail.com
      1542e16f
    • Tom Lane's avatar
      Allow matchingsel() to be used with operators that might return NULL. · 1c455078
      Tom Lane authored
      Although selfuncs.c will never call a target operator with null inputs,
      some functions might return null anyway.  The existing coding will fail
      if that happens (since FunctionCall2Coll will punt), which seems
      undesirable given that matchingsel() has such a broad range of potential
      applicability --- in fact, we already have a problem because we apply it
      to jsonb_path_exists_opr, which can return null.  Hence, rejigger the
      underlying functions mcv_selectivity and histogram_selectivity to cope,
      treating a null result as false.
      
      While we are at it, we can move the InitFunctionCallInfoData overhead
      out of the inner loops, which isn't a huge number of cycles but might
      save something considering we are likely calling functions as cheap
      as int4eq().  Plus, the number of loop cycles to be expected is much
      more than it was when this code was written, since typical settings
      of default_statistics_target are higher.
      
      In view of that consideration, let's apply the same change to
      var_eq_const, eqjoinsel_inner, and eqjoinsel_semi.  We do not expect
      equality functions to ever return null for non-null inputs (and
      certainly that code has been that way a long time without complaints),
      but the cycle savings seem attractive, especially in the eqjoinsel loops
      where there's potentially an O(N^2) savings.
      
      Similar code exists in ineq_histogram_selectivity and
      get_variable_range, but I forebore from changing those for now.
      The performance argument for changing ineq_histogram_selectivity
      is really weak anyway, since that will only iterate log2(N) times.
      
      Nikita Glukhov and Tom Lane
      
      Discussion: https://postgr.es/m/9d3b0959-95d6-c37e-2c0b-287bcfe5c705@postgrespro.ru
      1c455078
    • Tom Lane's avatar
      Clean up cpluspluscheck violation. · 9d25e1aa
      Tom Lane authored
      "operator" is a reserved word in C++, so per project conventions,
      don't use it as an identifier in header files.
      
      My oversight in commit a8081860.
      9d25e1aa
    • Tom Lane's avatar
      Fix duplicate typedef from commit 0d8c9c12. · 2117c3cb
      Tom Lane authored
      Older gcc versions don't like duplicate typedefs, so get rid of
      that in favor of doing it like we do it elsewhere, ie just use
      a "struct" declaration when trying to avoid importing a whole
      header file.
      
      Also, there seems no reason to include stringinfo.h here at all,
      so get rid of that addition too.
      
      Discussion: https://postgr.es/m/27239.1587415696@sss.pgh.pa.us
      2117c3cb
    • Fujii Masao's avatar
      Mention pg_promote() as a method to trigger promotion in documentation. · 67f82e96
      Fujii Masao authored
      Previously in the "Standby Server Operation" section, pg_ctl promote and
      protmote_trigger_file were documented as a method to trigger standby
      promotion, but pg_promote() function not.
      
      This commit also adds parentheses into <function>pg_promote</function>
      in some docs to make it clearer that a function is being referred to.
      
      Author: Masahiro Ikeda
      Reviewed-by: Michael Paquier, Laurenz Albe, Tom Lane, Fujii Masao
      Discussion: https://postgr.es/m/de0068417a9f4046bac693cbcc00bdc9@oss.nttdata.com
      67f82e96
    • Bruce Momjian's avatar
      doc: change SGML markup "figure" to "example" · f192312d
      Bruce Momjian authored
      Reported-by: Jürgen Purtz
      
      Discussion: https://postgr.es/m/709d7809-d7f4-8175-47f3-4d131341bba8@purtz.de
      
      Author: Jürgen Purtz
      
      Backpatch-through: 9.5
      f192312d
  5. 20 Apr, 2020 3 commits