1. 15 Dec, 2020 3 commits
    • Peter Eisentraut's avatar
      Clean up ancient test style · c06d6aa4
      Peter Eisentraut authored
      Many older tests where written in a style like
      
          SELECT '' AS two, i.* FROM INT2_TBL
      
      where the first column indicated the number of expected result rows.
      This has gotten increasingly out of date, as the test data fixtures
      have expanded, so a lot of these were wrong and misleading.  Moreover,
      this style isn't really necessary, since the psql output already shows
      the number of result rows.
      
      To clean this up, remove all those extra columns.
      
      Discussion: https://www.postgresql.org/message-id/flat/1a25312b-2686-380d-3c67-7a69094a999f%40enterprisedb.com
      c06d6aa4
    • Tom Lane's avatar
      Improve hash_create()'s API for some added robustness. · b3817f5f
      Tom Lane authored
      Invent a new flag bit HASH_STRINGS to specify C-string hashing, which
      was formerly the default; and add assertions insisting that exactly
      one of the bits HASH_STRINGS, HASH_BLOBS, and HASH_FUNCTION be set.
      This is in hopes of preventing recurrences of the type of oversight
      fixed in commit a1b8aa1e (i.e., mistakenly omitting HASH_BLOBS).
      
      Also, when HASH_STRINGS is specified, insist that the keysize be
      more than 8 bytes.  This is a heuristic, but it should catch
      accidental use of HASH_STRINGS for integer or pointer keys.
      (Nearly all existing use-cases set the keysize to NAMEDATALEN or
      more, so there's little reason to think this restriction should
      be problematic.)
      
      Tweak hash_create() to insist that the HASH_ELEM flag be set, and
      remove the defaults it had for keysize and entrysize.  Since those
      defaults were undocumented and basically useless, no callers
      omitted HASH_ELEM anyway.
      
      Also, remove memset's zeroing the HASHCTL parameter struct from
      those callers that had one.  This has never been really necessary,
      and while it wasn't a bad coding convention it was confusing that
      some callers did it and some did not.  We might as well save a few
      cycles by standardizing on "not".
      
      Also improve the documentation for hash_create().
      
      In passing, improve reinit.c's usage of a hash table by storing
      the key as a binary Oid rather than a string; and, since that's
      a temporary hash table, allocate it in CurrentMemoryContext for
      neatness.
      
      Discussion: https://postgr.es/m/590625.1607878171@sss.pgh.pa.us
      b3817f5f
    • Jeff Davis's avatar
      Revert "Cannot use WL_SOCKET_WRITEABLE without WL_SOCKET_READABLE." · a58db3aa
      Jeff Davis authored
      This reverts commit 3a9e64aa.
      
      Commit 4bad60e3 fixed the root of the problem that 3a9e64aa worked
      around.
      
      This enables proper pipelining of commands after terminating
      replication, eliminating an undocumented limitation.
      
      Discussion: https://postgr.es/m/3d57bc29-4459-578b-79cb-7641baf53c57%40iki.fi
      Backpatch-through: 9.5
      a58db3aa
  2. 14 Dec, 2020 2 commits
    • Michael Paquier's avatar
      Improve some code around cryptohash functions · 9b584953
      Michael Paquier authored
      This adjusts some code related to recent changes for cryptohash
      functions:
      - Add a variable in md5.h to track down the size of a computed result,
      moved from pgcrypto.  Note that pg_md5_hash() assumed a result of this
      size already.
      - Call explicit_bzero() on the hashed data when freeing the context for
      fallback implementations.  For MD5, particularly, it would be annoying
      to leave some non-zeroed data around.
      - Clean up some code related to recent changes of uuid-ossp.  .gitignore
      still included md5.c and a comment was incorrect.
      
      Discussion: https://postgr.es/m/X9HXKTgrvJvYO7Oh@paquier.xyz
      9b584953
    • Michael Paquier's avatar
      Add some checkpoint/restartpoint status to ps display · df9274ad
      Michael Paquier authored
      This is done for end-of-recovery and shutdown checkpoints/restartpoints
      (end-of-recovery restartpoints don't exist) rather than all types of
      checkpoints, in cases where it may not be possible to rely on
      pg_stat_activity to get a status from the startup or checkpointer
      processes.
      
      For example, at the end of a crash recovery, this is useful to know if a
      checkpoint is running in the startup process, while previously the ps
      display may only show some information about "recovering" something,
      that can be confusing while a checkpoint runs.
      
      Author: Justin Pryzby
      Reviewed-by: Nathan Bossart, Kirk Jamison, Fujii Masao, Michael Paquier
      Discussion: https://postgr.es/m/20200818225238.GP17022@telsasoft.com
      df9274ad
  3. 13 Dec, 2020 2 commits
  4. 12 Dec, 2020 2 commits
  5. 11 Dec, 2020 4 commits
  6. 10 Dec, 2020 2 commits
    • Michael Paquier's avatar
      Fix compilation of uuid-ossp · 525e60b7
      Michael Paquier authored
      This module had a dependency on pgcrypto's md5.c that got removed by
      b67b57a9.  Instead of the code from pgcrypto, this code can just use the
      new cryptohash routines for MD5 as a drop-in replacement, so let's just
      do this switch.  This has also the merit to simplify a bit the
      compilation of uuid-ossp.
      
      This requires --with-uuid to be reproduced, and I have used e2fs as a
      way to reproduce the failure, then test this commit.
      
      Per reports from buildfarm members longfin, florican and sifaka.
      
      Discussion: https://postgr.es/m/X9GToVd3QmWeNvj8@paquier.xyz
      525e60b7
    • Michael Paquier's avatar
      Refactor MD5 implementations according to new cryptohash infrastructure · b67b57a9
      Michael Paquier authored
      This commit heavily reorganizes the MD5 implementations that exist in
      the tree in various aspects.
      
      First, MD5 is added to the list of options available in cryptohash.c and
      cryptohash_openssl.c.  This means that if building with OpenSSL, EVP is
      used for MD5 instead of the fallback implementation that Postgres had
      for ages.  With the recent refactoring work for cryptohash functions,
      this change is straight-forward.  If not building with OpenSSL, a
      fallback implementation internal to src/common/ is used.
      
      Second, this reduces the number of MD5 implementations present in the
      tree from two to one, by moving the KAME implementation from pgcrypto to
      src/common/, and by removing the implementation that existed in
      src/common/.  KAME was already structured with an init/update/final set
      of routines by pgcrypto (see original pgcrypto/md5.h) for compatibility
      with OpenSSL, so moving it to src/common/ has proved to be a
      straight-forward move, requiring no actual manipulation of the internals
      of each routine.  Some benchmarking has not shown any performance gap
      between both implementations.
      
      Similarly to the fallback implementation used for SHA2, the fallback
      implementation of MD5 is moved to src/common/md5.c with an internal
      header called md5_int.h for the init, update and final routines.  This
      gets then consumed by cryptohash.c.
      
      The original routines used for MD5-hashed passwords are moved to a
      separate file called md5_common.c, also in src/common/, aimed at being
      shared between all MD5 implementations as utility routines to keep
      compatibility with any code relying on them.
      
      Like the SHA2 changes, this commit had its round of tests on both Linux
      and Windows, across all versions of OpenSSL supported on HEAD, with and
      even without OpenSSL.
      
      Author: Michael Paquier
      Reviewed-by: Daniel Gustafsson
      Discussion: https://postgr.es/m/20201106073434.GA4961@paquier.xyz
      b67b57a9
  7. 09 Dec, 2020 4 commits
  8. 08 Dec, 2020 12 commits
    • Tom Lane's avatar
      Teach contain_leaked_vars that assignment SubscriptingRefs are leaky. · 62ee7033
      Tom Lane authored
      array_get_element and array_get_slice qualify as leakproof, since
      they will silently return NULL for bogus subscripts.  But
      array_set_element and array_set_slice throw errors for such cases,
      making them clearly not leakproof.  contain_leaked_vars was evidently
      written with only the former case in mind, as it gave the wrong answer
      for assignment SubscriptingRefs (nee ArrayRefs).
      
      This would be a live security bug, were it not that assignment
      SubscriptingRefs can only occur in INSERT and UPDATE target lists,
      while we only care about leakproofness for qual expressions; so the
      wrong answer can't occur in practice.  Still, that's a rather shaky
      answer for a security-related question; and maybe in future somebody
      will want to ask about leakproofness of a tlist.  So it seems wise to
      fix and even back-patch this correction.
      
      (We would need some change here anyway for the upcoming
      generic-subscripting patch, since extensions might make different
      tradeoffs about whether to throw errors.  Commit 558d77f2 attempted
      to lay groundwork for that by asking check_functions_in_node whether a
      SubscriptingRef contains leaky functions; but that idea fails now that
      the implementation methods of a SubscriptingRef are not SQL-visible
      functions that could be marked leakproof or not.)
      
      Back-patch to 9.6.  While 9.5 has the same issue, the code's a bit
      different.  It seems quite unlikely that we'd introduce any actual bug
      in the short time 9.5 has left to live, so the work/risk/reward balance
      isn't attractive for changing 9.5.
      
      Discussion: https://postgr.es/m/3143742.1607368115@sss.pgh.pa.us
      62ee7033
    • Tom Lane's avatar
      Remove operator_precedence_warning. · a676386b
      Tom Lane authored
      This GUC was always intended as a temporary solution to help with
      finding 9.4-to-9.5 migration issues.  Now that all pre-9.5 branches
      are out of support, and 9.5 will be too before v14 is released,
      it seems like it's okay to drop it.  Doing so allows removal of
      several hundred lines of poorly-tested code in parse_expr.c,
      which have been a fertile source of bugs when people did use this.
      
      Discussion: https://postgr.es/m/2234320.1607117945@sss.pgh.pa.us
      a676386b
    • Dean Rasheed's avatar
      Improve estimation of ANDs under ORs using extended statistics. · 4f5760d4
      Dean Rasheed authored
      Formerly, extended statistics only handled clauses that were
      RestrictInfos. However, the restrictinfo machinery doesn't create
      sub-AND RestrictInfos for AND clauses underneath OR clauses.
      Therefore teach extended statistics to handle bare AND clauses,
      looking for compatible RestrictInfo clauses underneath them.
      
      Dean Rasheed, reviewed by Tomas Vondra.
      
      Discussion: https://postgr.es/m/CAEZATCW=J65GUFm50RcPv-iASnS2mTXQbr=CfBvWRVhFLJ_fWA@mail.gmail.com
      4f5760d4
    • Dean Rasheed's avatar
      Improve estimation of OR clauses using multiple extended statistics. · 88b0898f
      Dean Rasheed authored
      When estimating an OR clause using multiple extended statistics
      objects, treat the estimates for each set of clauses for each
      statistics object as independent of one another. The overlap estimates
      produced for each statistics object do not apply to clauses covered by
      other statistics objects.
      
      Dean Rasheed, reviewed by Tomas Vondra.
      
      Discussion: https://postgr.es/m/CAEZATCW=J65GUFm50RcPv-iASnS2mTXQbr=CfBvWRVhFLJ_fWA@mail.gmail.com
      88b0898f
    • Tom Lane's avatar
      Doc: clarify that CREATE TABLE discards redundant unique constraints. · f2a69b35
      Tom Lane authored
      The SQL standard says that redundant unique constraints are disallowed,
      but we long ago decided that throwing an error would be too
      user-unfriendly, so we just drop redundant ones.  The docs weren't very
      clear about that though, as this behavior was only explained for PRIMARY
      KEY vs UNIQUE, not UNIQUE vs UNIQUE.
      
      While here, I couldn't resist doing some copy-editing and markup-fixing
      on the adjacent text about INCLUDE options.
      
      Per bug #16767 from Matthias vd Meent.
      
      Discussion: https://postgr.es/m/16767-1714a2056ca516d0@postgresql.org
      f2a69b35
    • Tom Lane's avatar
      Doc: explain that the string types can't store \0 (ASCII NUL). · 9a264191
      Tom Lane authored
      This restriction was mentioned in connection with string literals,
      but it wasn't made clear that it's a general restriction not just
      a syntactic limitation in query strings.
      
      Per unsigned documentation comment.
      
      Discussion: https://postgr.es/m/160720552914.710.16625261471128631268@wrigleys.postgresql.org
      9a264191
    • Fujii Masao's avatar
      Speed up rechecking if relation needs to be vacuumed or analyze in autovacuum. · e2ac3fed
      Fujii Masao authored
      After autovacuum collects the relations to vacuum or analyze, it rechecks
      whether each relation still needs to be vacuumed or analyzed before actually
      doing that. Previously this recheck could be a significant overhead
      especially when there were a very large number of relations. This was
      because each recheck forced the statistics to be refreshed, and the refresh
      of the statistics for a very large number of relations could cause heavy
      overhead. There was the report that this issue caused autovacuum workers
      to have gotten “stuck” in a tight loop of table_recheck_autovac() that
      rechecks whether a relation needs to be vacuumed or analyzed.
      
      This commit speeds up the recheck by making autovacuum worker reuse
      the previously-read statistics for the recheck if possible. Then if that
      "stale" statistics says that a relation still needs to be vacuumed or analyzed,
      autovacuum refreshes the statistics and does the recheck again.
      
      The benchmark shows that the more relations exist and autovacuum workers
      are running concurrently, the more this change reduces the autovacuum
      execution time. For example, when there are 20,000 tables and 10 autovacuum
      workers are running, the benchmark showed that the change improved
      the performance of autovacuum more than three times. On the other hand,
      even when there are only 1000 tables and only a single autovacuum worker
      is running, the benchmark didn't show any big performance regression by
      the change.
      
      Firstly POC patch was proposed by Jim Nasby. As the result of discussion,
      we used Tatsuhito Kasahara's version of the patch using the approach
      suggested by Tom Lane.
      
      Reported-by: Jim Nasby
      Author: Tatsuhito Kasahara
      Reviewed-by: Masahiko Sawada, Fujii Masao
      Discussion: https://postgr.es/m/3FC6C2F2-8A47-44C0-B997-28830B5716D0@amazon.com
      e2ac3fed
    • Fujii Masao's avatar
      Bump catversion for pg_stat_wal changes. · 4e43ee88
      Fujii Masao authored
      Oversight in 01469241.
      
      Reported-by: Andres Freund
      Discussion: https://postgr.es/m/20201207185614.zzf63vggm5r4sozg@alap3.anarazel.de
      4e43ee88
    • Michael Paquier's avatar
      pgcrypto: Detect errors with EVP calls from OpenSSL · 28d1601a
      Michael Paquier authored
      The following routines are called within pgcrypto when handling digests
      but there were no checks for failures:
      - EVP_MD_CTX_size (can fail with -1 as of 3.0.0)
      - EVP_MD_CTX_block_size (can fail with -1 as of 3.0.0)
      - EVP_DigestInit_ex
      - EVP_DigestUpdate
      - EVP_DigestFinal_ex
      
      A set of elog(ERROR) is added by this commit to detect such failures,
      that should never happen except in the event of a processing failure
      internal to OpenSSL.
      
      Note that it would be possible to use ERR_reason_error_string() to get
      more context about such errors, but these refer mainly to the internals
      of OpenSSL, so it is not really obvious how useful that would be.  This
      is left out for simplicity.
      
      Per report from Coverity.  Thanks to Tom Lane for the discussion.
      
      Backpatch-through: 9.5
      28d1601a
    • Andres Freund's avatar
      jit: Correct parameter type for generated expression evaluation functions. · 5da871bf
      Andres Freund authored
      clang only uses the 'i1' type for scalar booleans, not for pointers to
      booleans (as the pointer might be pointing into a larger memory
      allocation). Therefore a pointer-to-bool needs to the "storage" boolean.
      
      There's no known case of wrong code generation due to this, but it seems quite
      possible that it could cause problems (see e.g. 72559438).
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20201207212142.wz5tnbk2jsaqzogb@alap3.anarazel.de
      Backpatch: 11-, where jit support was added
      5da871bf
    • Andres Freund's avatar
      jit: configure: Explicitly reference 'native' component. · 9543f086
      Andres Freund authored
      Until recently 'native' was implicitly included via 'orcjit', but a change
      included in LLVM 11 (not yet released) removed a number of such indirect
      component references.
      Reported-By: default avatarFabien COELHO <coelho@cri.ensmp.fr>
      Reported-By: default avatarAndres Freund <andres@anarazel.de>
      Reported-By: default avatarThomas Munro <thomas.munro@gmail.com>
      Author: Andres Freund <andres@anarazel.de>
      Discussion: https://postgr.es/m/20201201064949.mex6kvi2kygby3ni@alap3.anarazel.de
      Backpatch: 11-, where jit support was added
      9543f086
    • Michael Paquier's avatar
      Avoid using tuple from syscache for update of pg_database.datfrozenxid · 947789f1
      Michael Paquier authored
      pg_database.datfrozenxid gets updated using an in-place update at the
      end of vacuum or autovacuum.  Since 96cdeae0, as pg_database has a toast
      relation, it is possible for a pg_database tuple to have toast values
      if there is a large set of ACLs in place.  In such a case, the in-place
      update would fail because of the flattening of the toast values done for
      the catcache entry fetched.  Instead of using a copy from the catcache,
      this changes the logic to fetch the copy of the tuple by directly
      scanning pg_database.
      
      Per the lack of complaints on the matter, no backpatch is done.  Note
      that before 96cdeae0, attempting to insert such a tuple to pg_database
      would cause a "row is too big" error, so the end-of-vacuum problem was
      not reachable.
      
      Author: Ashwin Agrawal, Junfeng Yang
      Discussion: https://postgr.es/m/DM5PR0501MB38800D9E4605BCA72DD35557CCE10@DM5PR0501MB3880.namprd05.prod.outlook.com
      947789f1
  9. 07 Dec, 2020 4 commits
    • Tom Lane's avatar
      Add a couple of regression test cases related to array subscripting. · 0a665bbc
      Tom Lane authored
      Exercise some error cases that were never reached in the existing
      regression tests.  This is partly for code-coverage reasons, and
      partly to memorialize the current behavior in advance of planned
      changes for generic subscripting.
      
      Also, I noticed that type_sanity's check to verify that all standard
      types have array types was never extended when we added arrays for
      all system catalog rowtypes (f7f70d5e), nor when we added arrays
      over domain types (c12d570f).  So do that.  Also, since the query's
      expected output isn't empty, it seems like a good idea to add an
      ORDER BY to make sure the result stays stable.
      0a665bbc
    • Heikki Linnakangas's avatar
      Fix more race conditions in the newly-added pg_rewind test. · 6ba581cf
      Heikki Linnakangas authored
      pg_rewind looks at the control file to check what timeline a server is on.
      But promotion doesn't immediately write a checkpoint, it merely writes
      an end-of-recovery WAL record. If pg_rewind runs immediately after
      promotion, before the checkpoint has completed, it will think think that
      the server is still on the earlier timeline. We ran into this issue a long
      time ago already, see commit 484a848a.
      
      It's a bit bogus that pg_rewind doesn't determine the timeline correctly
      until the end-of-recovery checkpoint has completed. We probably should
      fix that. But for now work around it by waiting for the checkpoint
      to complete before running pg_rewind, like we did in commit 484a848a.
      
      In the passing, tidy up the new test a little bit. Rerder the INSERTs so
      that the comments make more sense, remove a spurious CHECKPOINT call after
      pg_rewind has already run, and add --debug option, so that if this fails
      again, we'll have more data.
      
      Per buildfarm failure at https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2020-12-06%2018%3A32%3A19&stg=pg_rewind-check.
      Backpatch to all supported versions.
      
      Discussion: https://www.postgresql.org/message-id/1713707e-e318-761c-d287-5b6a4aa807e8@iki.fi
      6ba581cf
    • Tom Lane's avatar
      pg_dump: Reorganize dumpBaseType() · 04732962
      Tom Lane authored
      Along the same lines as ed2c7f65 and daa9fe8a, reduce code duplication
      by having just one copy of the parts of the query that are the same
      across all server versions; and make the conditionals control the
      smallest possible amount of code.  This is in preparation for adding
      another dumpable field to pg_type.
      04732962
    • Michael Paquier's avatar
      Fix fd leak in pg_verifybackup · 51c38898
      Michael Paquier authored
      An error code path newly-introduced by 87ae9691 forgot to close a file
      descriptor when verifying a file's checksum.
      
      Per report from Coverity, via Tom Lane.
      51c38898
  10. 05 Dec, 2020 1 commit
  11. 04 Dec, 2020 4 commits
    • Heikki Linnakangas's avatar
      Fix race conditions in newly-added test. · 36a4ac20
      Heikki Linnakangas authored
      Buildfarm has been failing sporadically on the new test.  I was able to
      reproduce this by adding a random 0-10 s delay in the walreceiver, just
      before it connects to the primary. There's a race condition where node_3
      is promoted before it has fully caught up with node_1, leading to diverged
      timelines. When node_1 is later reconfigured as standby following node_3,
      it fails to catch up:
      
      LOG:  primary server contains no more WAL on requested timeline 1
      LOG:  new timeline 2 forked off current database system timeline 1 before current recovery point 0/30000A0
      
      That's the situation where you'd need to use pg_rewind, but in this case
      it happens already when we are just setting up the actual pg_rewind
      scenario we want to test, so change the test so that it waits until
      node_3 is connected and fully caught up before promoting it, so that you
      get a clean, controlled failover.
      
      Also rewrite some of the comments, for clarity. The existing comments
      detailed what each step in the test did, but didn't give a good overview
      of the situation the steps were trying to create.
      
      For reasons I don't understand, the test setup had to be written slightly
      differently in 9.6 and 9.5 than in later versions. The 9.5/9.6 version
      needed node 1 to be reinitialized from backup, whereas in later versions
      it could be shut down and reconfigured to be a standby. But even 9.5 should
      support "clean switchover", where primary makes sure that pending WAL is
      replicated to standby on shutdown. It would be nice to figure out what's
      going on there, but that's independent of pg_rewind and the scenario that
      this test tests.
      
      Discussion: https://www.postgresql.org/message-id/b0a3b95b-82d2-6089-6892-40570f8c5e60%40iki.fi
      36a4ac20
    • Peter Eisentraut's avatar
      Convert elog(LOG) calls to ereport() where appropriate · eb93f3a0
      Peter Eisentraut authored
      User-visible log messages should go through ereport(), so they are
      subject to translation.  Many remaining elog(LOG) calls are really
      debugging calls.
      Reviewed-by: default avatarAlvaro Herrera <alvherre@alvh.no-ip.org>
      Reviewed-by: default avatarMichael Paquier <michael@paquier.xyz>
      Reviewed-by: default avatarNoah Misch <noah@leadboat.com>
      Discussion: https://www.postgresql.org/message-id/flat/92d6f545-5102-65d8-3c87-489f71ea0a37%40enterprisedb.com
      eb93f3a0
    • Peter Eisentraut's avatar
      Remove unnecessary grammar symbols · a6964bc1
      Peter Eisentraut authored
      Instead of publication_name_list, we can use name_list.  We already
      refer to publications everywhere else by the 'name' or 'name_list'
      symbols, so this only improves consistency.
      
      Reviewed-by: https://www.postgresql.org/message-id/flat/3e3ccddb-41bd-ecd8-29fe-195e34d9886f%40enterprisedb.com
      Discussion: Tom Lane <tgl@sss.pgh.pa.us>
      a6964bc1
    • Amit Kapila's avatar
      Remove incorrect assertion in reorderbuffer.c. · 8ae4ef4f
      Amit Kapila authored
      We start recording changes in ReorderBufferTXN even before we reach
      SNAPBUILD_CONSISTENT state so that if the commit is encountered after
      reaching that we should be able to send the changes of the entire transaction.
      Now, while recording changes if the reorder buffer memory has exceeded
      logical_decoding_work_mem then we can start streaming if it is allowed and
      we haven't yet streamed that data. However, we must not allow streaming to
      start unless the snapshot has reached SNAPBUILD_CONSISTENT state.
      
      In passing, improve the comments atop ReorderBufferResetTXN to mention the
      case when we need to continue streaming after getting an error.
      
      Author: Amit Kapila
      Reviewed-by: Dilip Kumar
      Discussion: https://postgr.es/m/CAA4eK1KoOH0byboyYY40NBcC7Fe812trwTa+WY3jQF7WQWZbQg@mail.gmail.com
      8ae4ef4f