1. 09 Sep, 2019 5 commits
    • Tom Lane's avatar
      Be more careful about port selection in src/test/ldap/. · 3146f525
      Tom Lane authored
      Don't just assume that the next port is free; it might not be, or
      if we're really unlucky it might even be out of the TCP range.
      Do it honestly with two get_free_port() calls instead.
      
      This is surely a pretty low-probability problem, but I think it
      explains a buildfarm failure seen today, so let's fix it.
      
      Back-patch to v11 where this script was added.
      
      Discussion: https://postgr.es/m/25124.1568052346@sss.pgh.pa.us
      3146f525
    • Andrew Dunstan's avatar
      Prevent msys2 conversion of "cmd /c" switch to a file path · 73ff3a0a
      Andrew Dunstan authored
      Modern versions of msys2 have changed the treatment of "cmd /c" so that
      the runtime will try to convert the switch to a native file path. This
      patch adds a setting to inhibit that behaviour.
      
      Discussion: https://postgr.es/m/3227042f-cfcc-745a-57dd-fb8c471f8ddf@2ndQuadrant.com
      
      Backpatch to all live branches.
      73ff3a0a
    • Andres Freund's avatar
      Reorder EPQ work, to fix rowmark related bugs and improve efficiency. · 27cc7cd2
      Andres Freund authored
      In ad0bda5d I changed the EvalPlanQual machinery to store
      substitution tuples in slot, instead of using plain HeapTuples. The
      main motivation for that was that using HeapTuples will be inefficient
      for future tableams.  But it turns out that that conversion was buggy
      for non-locking rowmarks - the wrong tuple descriptor was used to
      create the slot.
      
      As a secondary issue 5db6df0c changed ExecLockRows() to begin EPQ
      earlier, to allow to fetch the locked rows directly into the EPQ
      slots, instead of having to copy tuples around. Unfortunately, as Tom
      complained, that forces some expensive initialization to happen
      earlier.
      
      As a third issue, the test coverage for EPQ was clearly insufficient.
      
      Fixing the first issue is unfortunately not trivial: Non-locked row
      marks were fetched at the start of EPQ, and we don't have the type
      information for the rowmarks available at that point. While we could
      change that, it's not easy. It might be worthwhile to change that at
      some point, but to fix this bug, it seems better to delay fetching
      non-locking rowmarks when they're actually needed, rather than
      eagerly. They're referenced at most once, and in cases where EPQ
      fails, might never be referenced. Fetching them when needed also
      increases locality a bit.
      
      To be able to fetch rowmarks during execution, rather than
      initialization, we need to be able to access the active EPQState, as
      that contains necessary data. To do so move EPQ related data from
      EState to EPQState, and, only for EStates creates as part of EPQ,
      reference the associated EPQState from EState.
      
      To fix the second issue, change EPQ initialization to allow use of
      EvalPlanQualSlot() to be used before EvalPlanQualBegin() (but
      obviously still requiring EvalPlanQualInit() to have been done).
      
      As these changes made struct EState harder to understand, e.g. by
      adding multiple EStates, significantly reorder the members, and add a
      lot more comments.
      
      Also add a few more EPQ tests, including one that fails for the first
      issue above. More is needed.
      
      Reported-By: yi huang
      Author: Andres Freund
      Reviewed-By: Tom Lane
      Discussion:
          https://postgr.es/m/CAHU7rYZo_C4ULsAx_LAj8az9zqgrD8WDd4hTegDTMM1LMqrBsg@mail.gmail.com
          https://postgr.es/m/24530.1562686693@sss.pgh.pa.us
      Backpatch: 12-, where the EPQ changes were introduced
      27cc7cd2
    • Alexander Korotkov's avatar
      Fix handling of non-key columns get_index_column_opclass() · 7e041603
      Alexander Korotkov authored
      f2e40380 introduces support of non-key attributes in GiST indexes.  Then if
      get_index_column_opclass() is asked by gistproperty() to get an opclass of
      non-key column, it returns garbage past oidvector value.  This commit fixes
      that by making get_index_column_opclass() return InvalidOid in this case.
      
      Discussion: https://postgr.es/m/20190902231948.GA5343%40alvherre.pgsql
      Author: Nikita Glukhov, Alexander Korotkov
      Backpatch-through: 12
      7e041603
    • Peter Eisentraut's avatar
      Improve new AND CHAIN tests · 89b160c3
      Peter Eisentraut authored
      Tweak the tests so that we're not just testing the default setting of
      transaction_read_only.
      Reported-by: default avatarfn ln <emuser20140816@gmail.com>
      89b160c3
  2. 08 Sep, 2019 6 commits
  3. 07 Sep, 2019 2 commits
    • Tom Lane's avatar
      Avoid using INFO elevel for what are fundamentally debug messages. · db438318
      Tom Lane authored
      Commit 6f6b99d1 stuck an INFO message into the fast path for
      checking partition constraints, for no very good reason except
      that it made it easy for the regression tests to verify that
      that path was taken.  Assorted later patches did likewise,
      increasing the unsuppressable-chatter level from ALTER TABLE
      even more.  This isn't good for the user experience, so let's
      drop these messages down to DEBUG1 where they belong.  So as
      not to have a loss of test coverage, create a TAP test that
      runs the relevant queries with client_min_messages = DEBUG1
      and greps for the expected messages.
      
      This testing method is a bit brute-force --- in particular,
      it duplicates the execution of a fair amount of the core
      create_table and alter_table tests.  We experimented with
      other solutions, but running any significant amount of
      standard testing with client_min_messages = DEBUG1 seems
      to have a lot of output-stability pitfalls, cf commits
      bbb96c37 and 5655565c.  Possibly at some point we'll look
      into whether we can reduce the amount of test duplication.
      
      Backpatch into v12, because some of these messages are new
      in v12 and we don't really want to ship it that way.
      
      Sergei Kornilov
      
      Discussion: https://postgr.es/m/81911511895540@web58j.yandex.ru
      Discussion: https://postgr.es/m/4859321552643736@myt5-02b80404fd9e.qloud-c.yandex.net
      db438318
    • Tom Lane's avatar
      Fix issues around strictness of SIMILAR TO. · ca70bdae
      Tom Lane authored
      As a result of some long-ago quick hacks, the SIMILAR TO operator
      and the corresponding flavor of substring() interpreted "ESCAPE NULL"
      as selecting the default escape character '\'.  This is both
      surprising and not per spec: the standard is clear that these
      functions should return NULL for NULL input.
      
      Additionally, because of inconsistency of the strictness markings
      of 3-argument substring() and similar_escape(), the planner could not
      inline the SQL definition of substring(), resulting in a substantial
      performance penalty compared to the underlying POSIX substring()
      function.
      
      The simplest fix for this would be to change the strictness marking
      of similar_escape(), but if we do that we risk breaking existing views
      that depend on that function.  Hence, leave similar_escape() as-is
      as a compatibility function, and instead invent a new function
      similar_to_escape() that comes in two strict variants.
      
      There are a couple of other behaviors in this area that are also
      not per spec, but they are documented and seem generally at least
      as sane as the spec's definition, so leave them alone.  But improve
      the documentation to describe them fully.
      
      Patch by me; thanks to Álvaro Herrera and Andrew Gierth for review
      and discussion.
      
      Discussion: https://postgr.es/m/14047.1557708214@sss.pgh.pa.us
      ca70bdae
  4. 06 Sep, 2019 10 commits
  5. 05 Sep, 2019 3 commits
    • Tom Lane's avatar
      Use data directory inode number, not port, to select SysV resource keys. · 7de19fbc
      Tom Lane authored
      This approach provides a much tighter binding between a data directory
      and the associated SysV shared memory block (and SysV or named-POSIX
      semaphores, if we're using those).  Key collisions are still possible,
      but only between data directories stored on different filesystems,
      so the situation should be negligible in practice.  More importantly,
      restarting the postmaster with a different port number no longer
      risks failing to identify a relevant shared memory block, even when
      postmaster.pid has been removed.  A standalone backend is likewise
      much more certain to detect conflicting leftover backends.
      
      (In the longer term, we might now think about deprecating the port as
      a cluster-wide value, so that one postmaster could support sockets
      with varying port numbers.  But that's for another day.)
      
      The hazards fixed here apply only on Unix systems; our Windows code
      paths already use identifiers derived from the data directory path
      name rather than the port.
      
      src/test/recovery/t/017_shm.pl, which intends to test key-collision
      cases, has been substantially rewritten since it can no longer use
      two postmasters with identical port numbers to trigger the case.
      Instead, use Perl's IPC::SharedMem module to create a conflicting
      shmem segment directly.  The test script will be skipped if that
      module is not available.  (This means that some older buildfarm
      members won't run it, but I don't think that that results in any
      meaningful coverage loss.)
      
      Patch by me; thanks to Noah Misch and Peter Eisentraut for discussion
      and review.
      
      Discussion: https://postgr.es/m/16908.1557521200@sss.pgh.pa.us
      7de19fbc
    • Robert Haas's avatar
      Split tuptoaster.c into three separate files. · 8b94dab0
      Robert Haas authored
      detoast.c/h contain functions required to detoast a datum, partially
      or completely, plus a few other utility functions for examining the
      size of toasted datums.
      
      toast_internals.c/h contain functions that are used internally to the
      TOAST subsystem but which (mostly) do not need to be accessed from
      outside.
      
      heaptoast.c/h contains code that is intrinsically specific to the
      heap AM, either because it operates on HeapTuples or is based on the
      layout of a heap page.
      
      detoast.c and toast_internals.c are placed in
      src/backend/access/common rather than src/backend/access/heap.  At
      present, both files still have dependencies on the heap, but that will
      be improved in a future commit.
      
      Patch by me, reviewed and tested by Prabhat Sabu, Thomas Munro,
      Andres Freund, and Álvaro Herrera.
      
      Discussion: http://postgr.es/m/CA+TgmoZv-=2iWM4jcw5ZhJeL18HF96+W1yJeYrnGMYdkFFnEpQ@mail.gmail.com
      8b94dab0
    • Peter Eisentraut's avatar
      Use explicit_bzero · 74a308cf
      Peter Eisentraut authored
      Use the explicit_bzero() function in places where it is important that
      security information such as passwords is cleared from memory.  There
      might be other places where it could be useful; this is just an
      initial collection.
      
      For platforms that don't have explicit_bzero(), provide various
      fallback implementations.  (explicit_bzero() itself isn't standard,
      but as Linux/glibc, FreeBSD, and OpenBSD have it, it's the most common
      spelling, so it makes sense to make that the invocation point.)
      
      Discussion: https://www.postgresql.org/message-id/flat/42d26bde-5d5b-c90d-87ae-6cab875f73be%402ndquadrant.com
      74a308cf
  6. 04 Sep, 2019 2 commits
    • Michael Paquier's avatar
      Fix thinko when ending progress report for a backend · ae060a52
      Michael Paquier authored
      The logic ending progress reporting for a backend entry introduced by
      b6fb6471 causes callers of pgstat_progress_end_command() to do some extra
      work when track_activities is enabled as the process fields are reset in
      the backend entry even if no command were started for reporting.
      
      This resets the fields only if a command is registered for progress
      reporting, and only if track_activities is enabled.
      
      Author: Masahiho Sawada
      Discussion: https://postgr.es/m/CAD21AoCry_vJ0E-m5oxJXGL3pnos-xYGCzF95rK5Bbi3Uf-rpA@mail.gmail.com
      Backpatch-through: 9.6
      ae060a52
    • Michael Paquier's avatar
      Delay fsyncs of pg_basebackup until the end of backup · 522baf14
      Michael Paquier authored
      Since the addition of fsync requests in bc34223b to make base backup data
      consistent on disk once pg_basebackup finishes, each tablespace tar file
      is individually flushed once completed, with an additional flush of the
      parent directory when the base backup finishes.  While holding a
      connection to the server, a fsync request taking a long time may cause a
      failure of the base backup, which is annoying for any integration.  A
      recent example of breakage can involve tcp_user_timeout, but
      wal_sender_timeout can cause similar problems.
      
      While reviewing the code, there was a second issue causing too many
      fsync requests to be done for the same WAL data.  As recursive fsyncs
      are done at the end of the backup for both the plain and tar formats
      from the base target directory where everything is written, it is fine
      to disable fsyncs when fetching or streaming WAL.
      
      Reported-by: Ryohei Takahashi
      Author: Michael Paquier
      Reviewed-by: Ryohei Takahashi
      Discussion: https://postgr.es/m/OSBPR01MB4550DAE2F8C9502894A45AAB82BE0@OSBPR01MB4550.jpnprd01.prod.outlook.com
      Backpatch-through: 10
      522baf14
  7. 03 Sep, 2019 8 commits
  8. 02 Sep, 2019 4 commits
    • Tom Lane's avatar
      Avoid touching replica identity index in ExtractReplicaIdentity(). · f63a5ead
      Tom Lane authored
      In what seems like a fit of misplaced optimization,
      ExtractReplicaIdentity() accessed the relation's replica-identity
      index without taking any lock on it.  Usually, the surrounding query
      already holds some lock so this is safe enough ... but in the case
      of a previously-planned delete, there might be no existing lock.
      Given a suitable test case, this is exposed in v12 and HEAD by an
      assertion added by commit b04aeb0a.
      
      The whole thing's rather poorly thought out anyway; rather than
      looking directly at the index, we should use the index-attributes
      bitmap that's held by the parent table's relcache entry, as the
      caller functions do.  This is more consistent and likely a bit
      faster, since it avoids a cache lookup.  Hence, change to doing it
      that way.
      
      While at it, rather than blithely assuming that the identity
      columns are non-null (with catastrophic results if that's wrong),
      add assertion checks that they aren't null.  Possibly those should
      be actual test-and-elog, but I'll leave it like this for now.
      
      In principle, this is a bug that's been there since this code was
      introduced (in 9.4).  In practice, the risk seems quite low, since
      we do have a lock on the index's parent table, so concurrent
      changes to the index's catalog entries seem unlikely.  Given the
      precedent that commit 9c703c16 wasn't back-patched, I won't risk
      back-patching this further than v12.
      
      Per report from Hadi Moshayedi.
      
      Discussion: https://postgr.es/m/CAK=1=Wrek44Ese1V7LjKiQS-Nd-5LgLi_5_CskGbpggKEf3tKQ@mail.gmail.com
      f63a5ead
    • Tom Lane's avatar
      Handle corner cases correctly in psql's reconnection logic. · aef36238
      Tom Lane authored
      After an unexpected connection loss and successful reconnection,
      psql neglected to resynchronize its internal state about the server,
      such as server version.  Ordinarily we'd be reconnecting to the same
      server and so this isn't really necessary, but there are scenarios
      where we do need to update --- one example is where we have a list
      of possible connection targets and they're not all alike.
      
      Define "resynchronize" as including connection_warnings(), so that
      this case acts the same as \connect.  This seems useful; for example,
      if the server version did change, the user might wish to know that.
      An attuned user might also notice that the new connection isn't
      SSL-encrypted, for example, though this approach isn't especially
      in-your-face about such changes.  Although this part is a behavioral
      change, it only affects interactive sessions, so it should not break
      any applications.
      
      Also, in do_connect, make sure that we desynchronize correctly when
      abandoning an old connection in non-interactive mode.
      
      These problems evidently are the result of people patching only one
      of the two places where psql deals with connection changes, so insert
      some cross-referencing comments in hopes of forestalling future bugs
      of the same ilk.
      
      Lastly, in Windows builds, issue codepage mismatch warnings only at
      startup, not during reconnections.  psql's codepage can't change
      during a reconnect, so complaining about it again seems like useless
      noise.
      
      Peter Billen and Tom Lane.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/CAMTXbE8e6U=EBQfNSe01Ej17CBStGiudMAGSOPaw-ALxM-5jXg@mail.gmail.com
      aef36238
    • Alvaro Herrera's avatar
      Add POD documentation to TestLib.pm · 6fcc40b1
      Alvaro Herrera authored
      This module was pretty much undocumented.  Fix that.
      
      Inspired by a preliminary patch sent by Ramanarayana, heavily updated by
      Andrew Dunstan, and reviewed by Michael Paquier.
      
      Discussion: https://postgr.es/m/CAF6A77G_WJTwBV9SBxCnQfZB09hm1p1O3stZ6eE5QiYd=X84Jg@mail.gmail.com
      6fcc40b1
    • Michael Paquier's avatar
      Add overflow-safe math inline functions for unsigned integers · 7dedfd22
      Michael Paquier authored
      Similarly to the signed versions added in 4d6ad312, this adds a set of
      inline functions for overflow checks with unsigned integers, including
      uint16, uint32 and uint64.  This relies on compiler built-in overflow
      checks by default if available.  The behavior of unsigned integers is
      well-defined so the fallback implementations checks are simple for
      additions and subtractions.  Multiplications avoid division-based checks
      which are expensive if possible, still this can happen for uint64 if
      128-bit integers are not available.
      
      While on it, the code in common/int.h is reorganized to avoid too many
      duplicated comments.  The new macros will be used in a follow-up patch.
      
      All thanks to Andres Freund for the input provided.
      
      Author: Fabien Coelho, Michael Paquier
      Discussion: https://postgr.es/m/20190830073423.GB2354@paquier.xyz
      7dedfd22