1. 13 Aug, 2018 6 commits
    • Peter Eisentraut's avatar
      Remove obsolete comment · 3ebdd21b
      Peter Eisentraut authored
      The sequence name is no longer stored in the sequence relation, since
      1753b1b0.
      3ebdd21b
    • Tom Lane's avatar
      Fix libpq's implementation of per-host connection timeouts. · 1e6e98f7
      Tom Lane authored
      Commit 5f374fe7 attempted to turn the connect_timeout from an overall
      maximum time limit into a per-host limit, but it didn't do a great job of
      that.  The timer would only get restarted if we actually detected timeout
      within connectDBComplete(), not if we changed our attention to a new host
      for some other reason.  In that case the old timeout continued to run,
      possibly causing a premature timeout failure for the new host.
      
      Fix that, and also tweak the logic so that if we do get a timeout,
      we advance to the next available IP address, not to the next host name.
      There doesn't seem to be a good reason to assume that all the IP
      addresses supplied for a given host name will necessarily fail the
      same way as the current one.  Moreover, this conforms better to the
      admittedly-vague documentation statement that the timeout is "per
      connection attempt".  I changed that to "per host name or IP address"
      to be clearer.  (Note that reconnections to the same server, such as for
      switching protocol version or SSL status, don't get their own separate
      timeout; that was true before and remains so.)
      
      Also clarify documentation about the interpretation of connect_timeout
      values less than 2.
      
      This seems like a bug, so back-patch to v10 where this logic came in.
      
      Tom Lane, reviewed by Fabien Coelho
      
      Discussion: https://postgr.es/m/5735.1533828184@sss.pgh.pa.us
      1e6e98f7
    • Michael Paquier's avatar
      Make autovacuum more aggressive to remove orphaned temp tables · 246a6c8f
      Michael Paquier authored
      Commit dafa0848, added in 10, made the removal of temporary orphaned
      tables more aggressive.  This commit makes an extra step into the
      aggressiveness by adding a flag in each backend's MyProc which tracks
      down any temporary namespace currently in use.  The flag is set when the
      namespace gets created and can be reset if the temporary namespace has
      been created in a transaction or sub-transaction which is aborted.  The
      flag value assignment is assumed to be atomic, so this can be done in a
      lock-less fashion like other flags already present in PGPROC like
      databaseId or backendId, still the fact that the temporary namespace and
      table created are still locked until the transaction creating those
      commits acts as a barrier for other backends.
      
      This new flag gets used by autovacuum to discard more aggressively
      orphaned tables by additionally checking for the database a backend is
      connected to as well as its temporary namespace in-use, removing
      orphaned temporary relations even if a backend reuses the same slot as
      one which created temporary relations in a past session.
      
      The base idea of this patch comes from Robert Haas, has been written in
      its first version by Tsunakawa Takayuki, then heavily reviewed by me.
      
      Author: Tsunakawa Takayuki
      Reviewed-by: Michael Paquier, Kyotaro Horiguchi, Andres Freund
      Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F8A4DC6@G01JPEXMBYT05
      Backpatch: 11-, as PGPROC gains a new flag and we don't want silent ABI
      breakages on already released versions.
      246a6c8f
    • Amit Kapila's avatar
      Adjust comment atop ExecShutdownNode. · 4f9a97e4
      Amit Kapila authored
      After commits a315b967 and b805b63ac2, part of the comment atop
      ExecShutdownNode is redundant.  Adjust it.
      
      Author: Amit Kapila
      Backpatch-through: 10 where both the mentioned commits are present.
      Discussion: https://postgr.es/m/86137f17-1dfb-42f9-7421-82fd786b04a1@anayrat.info
      4f9a97e4
    • Amit Kapila's avatar
      Prohibit shutting down resources if there is a possibility of back up. · 2cd0acfd
      Amit Kapila authored
      Currently, we release the asynchronous resources as soon as it is evident
      that no more rows will be needed e.g. when a Limit is filled.  This can be
      problematic especially for custom and foreign scans where we can scan
      backward.  Fix that by disallowing the shutting down of resources in such
      cases.
      
      Reported-by: Robert Haas
      Analysed-by: Robert Haas and Amit Kapila
      Author: Amit Kapila
      Reviewed-by: Robert Haas
      Backpatch-through: 9.6 where this code was introduced
      Discussion: https://postgr.es/m/86137f17-1dfb-42f9-7421-82fd786b04a1@anayrat.info
      2cd0acfd
    • Andrew Gierth's avatar
      Avoid query-lifetime memory leaks in XMLTABLE (bug #15321) · 07172d5a
      Andrew Gierth authored
      Multiple calls to XMLTABLE in a query (e.g. laterally applying it to a
      table with an xml column, an important use-case) were leaking large
      amounts of memory into the per-query context, blowing up memory usage.
      
      Repair by reorganizing memory context usage in nodeTableFuncscan; use
      the usual per-tuple context for row-by-row evaluations instead of
      perValueCxt, and use the explicitly created context -- renamed from
      perValueCxt to perTableCxt -- for arguments and state for each
      individual table-generation operation.
      
      Backpatch to PG10 where this code was introduced.
      
      Original report by IRC user begriffs; analysis and patch by me.
      Reviewed by Tom Lane and Pavel Stehule.
      
      Discussion: https://postgr.es/m/153394403528.10284.7530399040974170549@wrigleys.postgresql.org
      07172d5a
  2. 12 Aug, 2018 2 commits
    • Tom Lane's avatar
      Revert "Distinguish printf-like functions that support %m from those that don't." · 46b5e7c4
      Tom Lane authored
      This reverts commit 3a60c8ff.  Buildfarm
      results show that that caused a whole bunch of new warnings on platforms
      where gcc believes the local printf to be non-POSIX-compliant.  This
      problem outweighs the hypothetical-anyway possibility of getting warnings
      for misuse of %m.  We could use gnu_printf archetype when we've substituted
      src/port/snprintf.c, but that brings us right back to the problem of not
      getting warnings for %m.
      
      A possible answer is to attack it in the other direction by insisting
      that %m support be included in printf's feature set, but that will take
      more investigation.  In the meantime, revert the previous change, and
      update the comment for PGAC_C_PRINTF_ARCHETYPE to more fully explain
      what's going on.
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      46b5e7c4
    • Tom Lane's avatar
      Fix bogus loop logic in 013_crash_restart test's pump_until subroutine. · d11eae09
      Tom Lane authored
      The pump_nb() step might've already received the desired data, so we must
      check for that at the top of the loop not the bottom.  Otherwise, the
      call to pump() will sit with nothing to do until the timeout elapses.
      pump_until then falls out with apparent success ... but the timeout has
      been used up, causing the next call of pump_until to report a timeout
      failure.  I believe this explains the intermittent timeout failures
      we've seen in the buildfarm ever since this test went in.  I was able
      to reproduce the problem on gaur semi-repeatably, and this appears to
      fix it.
      
      In passing, remove a duplicate assignment, fix one stdin-assignment to
      look like the rest, and document the test's dependency on test_decoding.
      d11eae09
  3. 11 Aug, 2018 3 commits
    • Tom Lane's avatar
      Fix wrong order of operations in inheritance_planner. · 4a2994f0
      Tom Lane authored
      When considering a partitioning parent rel, we should stop processing that
      subroot as soon as we've done adjust_appendrel_attrs and any securityQuals
      updates.  The rest of this is unnecessary, and indeed adding duplicate
      subquery RTEs to the subroot is *wrong*.  As the code stood, the children
      of that partition ended up with two sets of copied subquery RTEs, confusing
      matters greatly.  Even more hilarity ensued if all of the children got
      excluded by constraint exclusion, so that the extra RTEs didn't make it
      back into the parent rtable.
      
      Per fuzz testing by Andreas Seltenreich.  Back-patch to v11 where this
      got broken (by commit 0a480502, it looks like).
      
      Discussion: https://postgr.es/m/87va8g7vq0.fsf@ansel.ydns.eu
      4a2994f0
    • Tom Lane's avatar
      Produce compiler errors if errno is referenced inside elog/ereport calls. · a2a8acd1
      Tom Lane authored
      It's often unsafe to reference errno within an elog/ereport call, because
      there are a lot of sub-functions involved and they might not all preserve
      errno.  (This is why we support the %m format spec: it works off a value
      of errno captured before we execute any potentially-unsafe functions in
      the arguments.)  Therefore, we have a project policy not to use errno
      there.
      
      This patch adds a hack to cause an (admittedly obscure) compiler error
      for such unsafe usages.  With the current code, the error will only be seen
      on Linux, macOS, and FreeBSD, but that should certainly be enough to catch
      mistakes in the buildfarm if they somehow get missed earlier.
      
      In addition, fix some places in src/common/exec.c that trip the error.
      I think these places are actually all safe, but it's simple enough to
      avoid the error by capturing errno manually, and doing so is good
      future-proofing in case these call sites get any more complicated.
      
      Thomas Munro (exec.c fixes by me)
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      a2a8acd1
    • Tom Lane's avatar
      Distinguish printf-like functions that support %m from those that don't. · 3a60c8ff
      Tom Lane authored
      The elog/ereport family of functions certainly support the %m format spec,
      because they implement it "by hand".  But elsewhere we have printf wrappers
      that might or might not allow it depending on whether the platform's printf
      does.  (Most non-glibc versions don't, and notably, src/port/snprintf.c
      doesn't.)  Hence, rather than using the gnu_printf format archetype
      interchangeably for all these functions, use it only for elog/ereport.
      This will allow us to get compiler warnings for mistakes like the ones
      fixed in commit a13b47a5, at least on platforms where printf doesn't
      take %m and gcc is correctly configured to know it.  (Unfortunately,
      that won't happen on Linux, nor on macOS according to my testing.
      It remains to be seen what the buildfarm's gcc-on-Windows animals will
      think of this, but we may well have to rely on less-popular platforms
      to warn us about unportable code of this kind.)
      
      Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us
      3a60c8ff
  4. 10 Aug, 2018 7 commits
  5. 09 Aug, 2018 6 commits
  6. 08 Aug, 2018 4 commits
    • Peter Geoghegan's avatar
      Doc: Correct description of amcheck example query. · 313cbdc7
      Peter Geoghegan authored
      The amcheck documentation incorrectly claimed that its example query
      verifies every catalog index in the database.  In fact, the query only
      verifies the 10 largest indexes (as determined by pg_class.relpages).
      Adjust the description accordingly.
      
      Backpatch: 10-, where contrib/amcheck was introduced.
      313cbdc7
    • Tom Lane's avatar
      Remove unwanted "garbage cleanup" logic in Makefiles. · 1eee8d49
      Tom Lane authored
      GNUmakefile.in defined a macro "garbage" that seems to have been meant
      as a suitable target for automatic "rm -rf" treatment, but it isn't
      actually used anywhere (and indeed never was, AFAICT).
      
      Moreover, we have concluded that the Makefiles shouldn't take it upon
      themselves to remove files that aren't expected by-products of building,
      so that doing anything like that would be against project policy anyway.
      Hence, just remove the macro.
      
      Grepping around finds another violation of that policy in ecpg/preproc,
      so clean that up too.
      
      Daniel Gustafsson (ecpg change by me)
      
      Discussion: https://postgr.es/m/AFBEF63E-E19D-4EBB-9F08-4617CDC751ED@yesql.se
      1eee8d49
    • Heikki Linnakangas's avatar
      Don't run atexit callbacks in quickdie signal handlers. · 8e19a826
      Heikki Linnakangas authored
      exit() is not async-signal safe. Even if the libc implementation is, 3rd
      party libraries might have installed unsafe atexit() callbacks. After
      receiving SIGQUIT, we really just want to exit as quickly as possible, so
      we don't really want to run the atexit() callbacks anyway.
      
      The original report by Jimmy Yih was a self-deadlock in startup_die().
      However, this patch doesn't address that scenario; the signal handling
      while waiting for the startup packet is more complicated. But at least this
      alleviates similar problems in the SIGQUIT handlers, like that reported
      by Asim R P later in the same thread.
      
      Backpatch to 9.3 (all supported versions).
      
      Discussion: https://www.postgresql.org/message-id/CAOMx_OAuRUHiAuCg2YgicZLzPVv5d9_H4KrL_OFsFP%3DVPekigA%40mail.gmail.com
      8e19a826
    • Tom Lane's avatar
      Match RelOptInfos by relids not pointer equality. · 11e22e48
      Tom Lane authored
      Commit 1c2cb274 added some code that tried to detect whether two
      RelOptInfos were the "same" rel by pointer comparison; but it turns
      out that inheritance_planner breaks that, through its shenanigans
      with copying some relations forward into new subproblems.  Compare
      relid sets instead.  Add a regression test case to exercise this
      area.
      
      Problem reported by Rushabh Lathia; diagnosis and fix by Amit Langote,
      modified a bit by me.
      
      Discussion: https://postgr.es/m/CAGPqQf3anJGj65bqAQ9edDr8gF7qig6_avRgwMT9MsZ19COUPw@mail.gmail.com
      11e22e48
  7. 07 Aug, 2018 4 commits
    • Tom Lane's avatar
      Don't record FDW user mappings as members of extensions. · 9b7c56d6
      Tom Lane authored
      CreateUserMapping has a recordDependencyOnCurrentExtension call that's
      been there since extensions were introduced (very possibly my fault).
      However, there's no support anywhere else for user mappings as members
      of extensions, nor are they listed as a possible member object type in
      the documentation.  Nor does it really seem like a good idea for user
      mappings to belong to extensions when roles don't.  Hence, remove the
      bogus call.
      
      (As we saw in bug #15310, the lack of any pg_dump support for this case
      ensures that any such membership record would silently disappear during
      pg_upgrade.  So there's probably no need for us to do anything else
      about cleaning up after this mistake.)
      
      Discussion: https://postgr.es/m/27952.1533667213@sss.pgh.pa.us
      9b7c56d6
    • Tom Lane's avatar
      Fix incorrect initialization of BackendActivityBuffer. · 41db9739
      Tom Lane authored
      Since commit c8e8b5a6, this has been zeroed out using the wrong length.
      In practice the length would always be too small, leading to not zeroing
      the whole buffer rather than clobbering additional memory; and that's
      pretty harmless, both because shmem would likely start out as zeroes
      and because we'd reinitialize any given entry before use.  Still,
      it's bogus, so fix it.
      
      Reported by Petru-Florin Mihancea (bug #15312)
      
      Discussion: https://postgr.es/m/153363913073.1303.6518849192351268091@wrigleys.postgresql.org
      41db9739
    • Tom Lane's avatar
      Fix pg_upgrade to handle event triggers in extensions correctly. · 03838b80
      Tom Lane authored
      pg_dump with --binary-upgrade must emit ALTER EXTENSION ADD commands
      for all objects that are members of extensions.  It forgot to do so for
      event triggers, as per bug #15310 from Nick Barnes.  Back-patch to 9.3
      where event triggers were introduced.
      
      Haribabu Kommi
      
      Discussion: https://postgr.es/m/153360083872.1395.4593932457718151600@wrigleys.postgresql.org
      03838b80
    • Tom Lane's avatar
      Ensure pg_dump_sort.c sorts null vs non-null namespace consistently. · 5b5ed475
      Tom Lane authored
      The original coding here (which is, I believe, my fault) supposed that
      it didn't need to concern itself with the possibility that one object
      of a given type-priority has a namespace while another doesn't.  But
      that's not reliably true anymore, if it ever was; and if it does happen
      then it's possible that DOTypeNameCompare returns self-inconsistent
      comparison results.  That leads to unspecified behavior in qsort()
      and a resultant weird output order from pg_dump.
      
      This should end up being only a cosmetic problem, because any ordering
      constraints that actually matter should be enforced by the later
      dependency-based sort.  Still, it's a bug, so back-patch.
      
      Report and fix by Jacob Champion, though I editorialized on his
      patch to the extent of making NULL sort after non-NULL, for consistency
      with our usual sorting definitions.
      
      Discussion: https://postgr.es/m/CABAq_6Hw+V-Kj7PNfD5tgOaWT_-qaYkc+SRmJkPLeUjYXLdxwQ@mail.gmail.com
      5b5ed475
  8. 06 Aug, 2018 2 commits
    • Tom Lane's avatar
      Last-minute updates for release notes. · e0ee9305
      Tom Lane authored
      Security: CVE-2018-10915, CVE-2018-10925
      e0ee9305
    • Tom Lane's avatar
      Fix failure to reset libpq's state fully between connection attempts. · d1c6a14b
      Tom Lane authored
      The logic in PQconnectPoll() did not take care to ensure that all of
      a PGconn's internal state variables were reset before trying a new
      connection attempt.  If we got far enough in the connection sequence
      to have changed any of these variables, and then decided to try a new
      server address or server name, the new connection might be completed
      with some state that really only applied to the failed connection.
      
      While this has assorted bad consequences, the only one that is clearly
      a security issue is that password_needed didn't get reset, so that
      if the first server asked for a password and the second didn't,
      PQconnectionUsedPassword() would return an incorrect result.  This
      could be leveraged by unprivileged users of dblink or postgres_fdw
      to allow them to use server-side login credentials that they should
      not be able to use.
      
      Other notable problems include the possibility of forcing a v2-protocol
      connection to a server capable of supporting v3, or overriding
      "sslmode=prefer" to cause a non-encrypted connection to a server that
      would have accepted an encrypted one.  Those are certainly bugs but
      it's harder to paint them as security problems in themselves.  However,
      forcing a v2-protocol connection could result in libpq having a wrong
      idea of the server's standard_conforming_strings setting, which opens
      the door to SQL-injection attacks.  The extent to which that's actually
      a problem, given the prerequisite that the attacker needs control of
      the client's connection parameters, is unclear.
      
      These problems have existed for a long time, but became more easily
      exploitable in v10, both because it introduced easy ways to force libpq
      to abandon a connection attempt at a late stage and then try another one
      (rather than just giving up), and because it provided an easy way to
      specify multiple target hosts.
      
      Fix by rearranging PQconnectPoll's state machine to provide centralized
      places to reset state properly when moving to a new target host or when
      dropping and retrying a connection to the same host.
      
      Tom Lane, reviewed by Noah Misch.  Our thanks to Andrew Krasichkov
      for finding and reporting the problem.
      
      Security: CVE-2018-10915
      d1c6a14b
  9. 05 Aug, 2018 5 commits
    • Tom Lane's avatar
      aa291a4c
    • Tom Lane's avatar
      Doc: fix incorrectly stated argument list for pgcrypto's hmac() function. · a3274e0d
      Tom Lane authored
      The bytea variant takes (bytea, bytea, text).
      Per unsigned report.
      
      Discussion: https://postgr.es/m/153344327294.1404.654155870612982042@wrigleys.postgresql.org
      a3274e0d
    • Heikki Linnakangas's avatar
      Remove now unused check for HAVE_X509_GET_SIGNATURE_NID in test. · 6b9eb503
      Heikki Linnakangas authored
      I removed the code that used this in the previous commit.
      
      Spotted by Michael Paquier.
      6b9eb503
    • Heikki Linnakangas's avatar
      Remove support for tls-unique channel binding. · 77291139
      Heikki Linnakangas authored
      There are some problems with the tls-unique channel binding type. It's not
      supported by all SSL libraries, and strictly speaking it's not defined for
      TLS 1.3 at all, even though at least in OpenSSL, the functions used for it
      still seem to work with TLS 1.3 connections. And since we had no
      mechanism to negotiate what channel binding type to use, there would be
      awkward interoperability issues if a server only supported some channel
      binding types. tls-server-end-point seems feasible to support with any SSL
      library, so let's just stick to that.
      
      This removes the scram_channel_binding libpq option altogether, since there
      is now only one supported channel binding type.
      
      This also removes all the channel binding tests from the SSL test suite.
      They were really just testing the scram_channel_binding option, which
      is now gone. Channel binding is used if both client and server support it,
      so it is used in the existing tests. It would be good to have some tests
      specifically for channel binding, to make sure it really is used, and the
      different combinations of a client and a server that support or doesn't
      support it. The current set of settings we have make it hard to write such
      tests, but I did test those things manually, by disabling
      HAVE_BE_TLS_GET_CERTIFICATE_HASH and/or
      HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH.
      
      I also removed the SCRAM_CHANNEL_BINDING_TLS_END_POINT constant. This is a
      matter of taste, but IMO it's more readable to just use the
      "tls-server-end-point" string.
      
      Refactor the checks on whether the SSL library supports the functions
      needed for tls-server-end-point channel binding. Now the server won't
      advertise, and the client won't choose, the SCRAM-SHA-256-PLUS variant, if
      compiled with an OpenSSL version too old to support it.
      
      In the passing, add some sanity checks to check that the chosen SASL
      mechanism, SCRAM-SHA-256 or SCRAM-SHA-256-PLUS, matches whether the SCRAM
      exchange used channel binding or not. For example, if the client selects
      the non-channel-binding variant SCRAM-SHA-256, but in the SCRAM message
      uses channel binding anyway. It's harmless from a security point of view,
      I believe, and I'm not sure if there are some other conditions that would
      cause the connection to fail, but it seems better to be strict about these
      things and check explicitly.
      
      Discussion: https://www.postgresql.org/message-id/ec787074-2305-c6f4-86aa-6902f98485a4%40iki.fi
      77291139
    • Tom Lane's avatar
      Update version 11 release notes. · 7a46068f
      Tom Lane authored
      Remove description of commit 1944cdc9, which has now been back-patched
      so it's not relevant to v11 any longer.  Add descriptions of other
      recent commits that seemed worth mentioning.
      
      I marked the update as stopping at 2018-07-30, because it's unclear
      whether d06eebce5 will be allowed to stay in v11, and I didn't feel like
      putting effort into writing a description of it yet.  If it does stay,
      I think it will deserve mention in the Source Code section.
      7a46068f
  10. 04 Aug, 2018 1 commit
    • Tom Lane's avatar
      Fix INSERT ON CONFLICT UPDATE through a view that isn't just SELECT *. · b8a1247a
      Tom Lane authored
      When expanding an updatable view that is an INSERT's target, the rewriter
      failed to rewrite Vars in the ON CONFLICT UPDATE clause.  This accidentally
      worked if the view was just "SELECT * FROM ...", as the transformation
      would be a no-op in that case.  With more complicated view targetlists,
      this omission would often lead to "attribute ... has the wrong type" errors
      or even crashes, as reported by Mario De Frutos Dieguez.
      
      Fix by adding code to rewriteTargetView to fix up the data structure
      correctly.  The easiest way to update the exclRelTlist list is to rebuild
      it from scratch looking at the new target relation, so factor the code
      for that out of transformOnConflictClause to make it sharable.
      
      In passing, avoid duplicate permissions checks against the EXCLUDED
      pseudo-relation, and prevent useless view expansion of that relation's
      dummy RTE.  The latter is only known to happen (after this patch) in cases
      where the query would fail later due to not having any INSTEAD OF triggers
      for the view.  But by exactly that token, it would create an unintended
      and very poorly tested state of the query data structure, so it seems like
      a good idea to prevent it from happening at all.
      
      This has been broken since ON CONFLICT was introduced, so back-patch
      to 9.5.
      
      Dean Rasheed, based on an earlier patch by Amit Langote;
      comment-kibitzing and back-patching by me
      
      Discussion: https://postgr.es/m/CAFYwGJ0xfzy8jaK80hVN2eUWr6huce0RU8AgU04MGD00igqkTg@mail.gmail.com
      b8a1247a