1. 14 Sep, 2018 5 commits
    • Tom Lane's avatar
      Improve parallel scheduling logic in pg_dump/pg_restore. · 548e5097
      Tom Lane authored
      Previously, the way this worked was that a parallel pg_dump would
      re-order the TABLE_DATA items in the dump's TOC into decreasing size
      order, and separately re-order (some of) the INDEX items into decreasing
      size order.  Then pg_dump would dump the items in that order.  Later,
      parallel pg_restore just followed the TOC order.  This method had lots
      of deficiencies:
      
      * TOC ordering randomly differed between parallel and non-parallel
      dumps, and was hard to predict in the former case, causing problems
      for building stable pg_dump test cases.
      
      * Parallel restore only followed a well-chosen order if the dump had
      been done in parallel; in particular, this never happened for restore
      from custom-format dumps.
      
      * The best order for restore isn't necessarily the same as for dump,
      and it's not really static either because of locking considerations.
      
      * TABLE_DATA and INDEX items aren't the only things that might take a lot
      of work during restore.  Scheduling was particularly stupid for the BLOBS
      item, which might require lots of work during dump as well as restore,
      but was left to the end in either case.
      
      This patch removes the logic that changed the TOC order, fixing the
      test instability problem.  Instead, we sort the parallelizable items
      just before processing them during a parallel dump.  Independently
      of that, parallel restore prioritizes the ready-to-execute tasks
      based on the size of the underlying table.  In the case of dependent
      tasks such as index, constraint, or foreign key creation, the largest
      relevant table is used as the metric for estimating the task length.
      (This is pretty crude, but it should be enough to avoid the case we
      want to avoid, which is ending the run with just a few large tasks
      such that we can't make use of all N workers.)
      
      Patch by me, responding to a complaint from Peter Eisentraut,
      who also reviewed the patch.
      
      Discussion: https://postgr.es/m/5137fe12-d0a2-4971-61b6-eb4e7e8875f8@2ndquadrant.com
      548e5097
    • Alvaro Herrera's avatar
      Fix ALTER/TYPE on columns referenced by FKs in partitioned tables · 20bef2c3
      Alvaro Herrera authored
      When ALTER TABLE ... SET DATA TYPE affects a column referenced by
      constraints and indexes, it drop those constraints and indexes and
      recreates them afterwards, so that the definitions match the new data
      type.  The original code did this by dropping one object at a time
      (commit 077db40f of May 2004), which worked fine because the
      dependencies between the objects were pretty straightforward, and
      ordering the objects in a specific way was enough to make this work.
      However, when there are foreign key constraints in partitioned tables,
      the dependencies are no longer so straightforward, and we were getting
      errors when attempted:
        ERROR:  cache lookup failed for constraint 16398
      
      This can be fixed by doing all the drops in one pass instead, using
      performMultipleDeletions (introduced by df18c51f of Aug 2006).  With
      this change we can also remove the code to carefully order the list of
      objects to be deleted.
      Reported-by: default avatarRajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>
      Reviewed-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Discussion: https://postgr.es/m/CAKcux6nWS_m+s=1Udk_U9B+QY7pA-Ac58qR5BdUfOyrwnWHDew@mail.gmail.com
      20bef2c3
    • Andrew Gierth's avatar
      Order active window clauses for greater reuse of Sort nodes. · 728202b6
      Andrew Gierth authored
      By sorting the active window list lexicographically by the sort clause
      list but putting longer clauses before shorter prefixes, we generate
      more chances to elide Sort nodes when building the path.
      
      Author: Daniel Gustafsson (with some editorialization by me)
      Reviewed-by: Alexander Kuzmenkov, Masahiko Sawada, Tom Lane
      Discussion: https://postgr.es/m/124A7F69-84CD-435B-BA0E-2695BE21E5C2%40yesql.se
      728202b6
    • Amit Kapila's avatar
      Don't allow LIMIT/OFFSET clause within sub-selects to be pushed to workers. · 75f9c4ca
      Amit Kapila authored
      Allowing sub-select containing LIMIT/OFFSET in workers can lead to
      inconsistent results at the top-level as there is no guarantee that the
      row order will be fully deterministic.  The fix is to prohibit pushing
      LIMIT/OFFSET within sub-selects to workers.
      
      Reported-by: Andrew Fletcher
      Bug: 15324
      Author: Amit Kapila
      Reviewed-by: Dilip Kumar
      Backpatch-through: 9.6
      Discussion: https://postgr.es/m/153417684333.10284.11356259990921828616@wrigleys.postgresql.org
      75f9c4ca
    • Michael Paquier's avatar
      Allow concurrent-safe open() and fopen() in frontend code for Windows · 0ba06e0b
      Michael Paquier authored
      PostgreSQL uses a custom wrapper for open() and fopen() which is
      concurrent-safe, allowing multiple processes to open and work on the
      same file.  This has a couple of advantages:
      - pg_test_fsync does not handle O_DSYNC correctly otherwise, leading to
      false claims that disks are unsafe.
      - TAP tests can run into race conditions when a postmaster and pg_ctl
      open postmaster.pid, fixing some random failures in the buildfam.
      
      pg_upgrade is one frontend tool using workarounds to bypass file locking
      issues with the log files it generates, however the interactions with
      pg_ctl are proving to be tedious to get rid of, so this is left for
      later.
      
      Author: Laurenz Albe
      Reviewed-by: Michael Paquier, Kuntal Ghosh
      Discussion: https://postgr.es/m/1527846213.2475.31.camel@cybertec.at
      Discussion: https://postgr.es/m/16922.1520722108@sss.pgh.pa.us
      0ba06e0b
  2. 13 Sep, 2018 7 commits
    • Michael Paquier's avatar
      Improve autovacuum logging for aggressive and anti-wraparound runs · 28a8fa98
      Michael Paquier authored
      A log message was being generated when log_min_duration is reached for
      autovacuum on a given relation to indicate if it was an aggressive run,
      and missed the point of mentioning if it is doing an anti-wrapround
      run.  The log message generated is improved so as one, both or no extra
      details are added depending on the option set.
      
      Author: Sergei Kornilov
      Reviewed-by: Masahiko Sawada, Michael Paquier
      Discussion: https://postgr.es/m/11587951532155118@sas1-19a94364928d.qloud-c.yandex.net
      28a8fa98
    • Peter Eisentraut's avatar
      Message style improvements · f48fa2bc
      Peter Eisentraut authored
      Fix one untranslatable string concatenation in pg_rewind.
      
      Fix one message in pg_verify_checksums to use a style use elsewhere
      and avoid plural issues.
      
      Fix one gratuitous abbreviation in psql.
      f48fa2bc
    • Andres Freund's avatar
      Detect LLVM 7 without specifying binaries explicitly. · 240d40db
      Andres Freund authored
      Before this commit LLVM 7 was supported, but only if one explicitly
      provided LLVM_CONFIG= and CLANG= paths.  As LLVM 7 is the first
      version that includes our upstreamed debugging and profiling features,
      and as debian is planning to default to 7 due to wider architecture
      support, it seems good to support auto-detecting that version.
      
      Author: Christoph Berg
      Discussion: https://postgr.es/m/20180912124517.GD24584@msg.df7cb.de
      Backpatch: 11, where LLVM was introduced
      240d40db
    • Tom Lane's avatar
      Attempt to identify system timezone by reading /etc/localtime symlink. · 23bd3cec
      Tom Lane authored
      On many modern platforms, /etc/localtime is a symlink to a file within the
      IANA database.  Reading the symlink lets us find out the name of the system
      timezone directly, without going through the brute-force search embodied in
      scan_available_timezones().  This shortens the runtime of initdb by some
      tens of ms, which is helpful for the buildfarm, and it also allows us to
      reliably select the same zone name the system was actually configured for,
      rather than possibly choosing one of IANA's many zone aliases.  (For
      example, in a system configured for "Asia/Tokyo", the brute-force search
      would not choose that name but its alias "Japan", on the grounds of the
      latter string being shorter.  More surprisingly, "Navajo" is preferred
      to either "America/Denver" or "US/Mountain", as seen in an old complaint
      from Josh Berkus.)
      
      If /etc/localtime doesn't exist, or isn't a symlink, or we can't make
      sense of its contents, or the contents match a zone we know but that
      zone doesn't match the observed behavior of localtime(), fall back to
      the brute-force search.
      
      Also, tweak initdb so that it prints the zone name it selected.
      
      In passing, replace the last few references to the "Olson" database in
      code comments with "IANA", as that's been our preferred term since
      commit b2cbced9.
      
      Patch by me, per a suggestion from Robert Haas; review by Michael Paquier
      
      Discussion: https://postgr.es/m/7408.1525812528@sss.pgh.pa.us
      23bd3cec
    • Amit Kapila's avatar
      Attach FPI to the first record after full_page_writes is turned on. · bc153c94
      Amit Kapila authored
      XLogInsert fails to attach a required FPI to the first record after
      full_page_writes is turned on by the last checkpoint.  This bug got
      introduced in 9.5 due to code rearrangement in commits 2c03216d and
      2076db2a.  Fix it by ensuring that XLogInsertRecord performs a
      recomputation when the given record is generated with FPW as off but
      found that the flag has been turned on while actually inserting the
      record.
      
      Reported-by: Kyotaro Horiguchi
      Author: Kyotaro Horiguchi
      Reviewed-by: Amit Kapila
      Backpatch-through: 9.5 where this problem was introduced
      Discussion: https://postgr.es/m/20180420.151043.74298611.horiguchi.kyotaro@lab.ntt.co.jp
      bc153c94
    • Michael Paquier's avatar
      Simplify static function in extension.c · 514a731d
      Michael Paquier authored
      An extra argument for the filename defining the extension script
      location was present, aimed at being used for error reporting, but has
      never been used.  This was around since extensions have been added in
      d9572c4e.
      
      Author: Yugo Nagata
      Reviewed-by: Tatsuo Ishii
      Discussion: https://postgr.es/m/20180907180504.1ff19e1675bb44a67e9c7ab1@sraoss.co.jp
      514a731d
    • Peter Eisentraut's avatar
      Simplify index tuple descriptor initialization · e5f1bb92
      Peter Eisentraut authored
      We have two code paths for initializing the tuple descriptor for a new
      index: For a normal index, we copy the tuple descriptor from the table
      and reset a number of fields that are not applicable to indexes.  For an
      expression index, we make a blank tuple descriptor and fill in the
      needed fields based on the provided expressions.  As pg_attribute has
      grown over time, the number of fields that we need to reset in the first
      case is now bigger than the number of fields we actually want to copy,
      so it's sensible to do it the other way around: Make a blank descriptor
      and copy just the fields we need.  This also allows more code sharing
      between the two branches, and it avoids having to touch this code for
      almost every unrelated change to the pg_attribute structure.
      Reviewed-by: default avatarArthur Zakirov <a.zakirov@postgrespro.ru>
      e5f1bb92
  3. 12 Sep, 2018 3 commits
    • Tom Lane's avatar
      Minor fixes for psql tab completion. · 7046d302
      Tom Lane authored
      * Include partitioned tables in what's offered after ANALYZE.
      
      * Include toast_tuple_target in what's offered after ALTER TABLE ... SET|RESET.
      
      * Include HASH in what's offered after PARTITION BY.
      
      This is extracted from a larger patch; these bits seem like
      uncontroversial bug fixes for v11 features, so back-patch them into v11.
      
      Justin Pryzby
      
      Discussion: https://postgr.es/m/20180529000623.GA21896@telsasoft.com
      7046d302
    • Andrew Gierth's avatar
      Repair bug in regexp split performance improvements. · b7f6bcbf
      Andrew Gierth authored
      Commit c8ea87e4 introduced a temporary conversion buffer for
      substrings extracted during regexp splits. Unfortunately the code that
      sized it was failing to ignore the effects of ignored degenerate
      regexp matches, so for regexp_split_* calls it could under-size the
      buffer in such cases.
      
      Fix, and add some regression test cases (though those will only catch
      the bug if run in a multibyte encoding).
      
      Backpatch to 9.3 as the faulty code was.
      
      Thanks to the PostGIS project, Regina Obe and Paul Ramsey for the
      report (via IRC) and assistance in analysis. Patch by me.
      b7f6bcbf
    • Peter Eisentraut's avatar
      ecpg: Change --version output to common style · ba37349c
      Peter Eisentraut authored
      When we removed the ecpg-specific versions, we also removed the
      "(PostgreSQL)" from the --version output, which we show in other
      programs.
      Reported-by: default avatarIoseph Kim <pgsql-kr@postgresql.kr>
      ba37349c
  4. 11 Sep, 2018 6 commits
    • Tom Lane's avatar
      Add PQresultMemorySize function to report allocated size of a PGresult. · 2970afa6
      Tom Lane authored
      This number can be useful for application memory management, and the
      overhead to track it seems pretty trivial.
      
      Lars Kanis, reviewed by Pavel Stehule, some mods by me
      
      Discussion: https://postgr.es/m/fa16a288-9685-14f2-97c8-b8ac84365a4f@greiz-reinsdorf.de
      2970afa6
    • Michael Paquier's avatar
      Parse more strictly integer parameters from connection strings in libpq · e7a22179
      Michael Paquier authored
      The following parameters have been parsed in lossy ways when specified
      in a connection string processed by libpq:
      - connect_timeout
      - keepalives
      - keepalives_count
      - keepalives_idle
      - keepalives_interval
      - port
      
      Overflowing values or the presence of incorrect characters were not
      properly checked, leading to libpq trying to use such values and fail
      with unhelpful error messages.  This commit hardens the parsing of those
      parameters so as it is possible to find easily incorrect values.
      
      Author: Fabien Coelho
      Reviewed-by: Peter Eisentraut, Michael Paquier
      Discussion: https://postgr.es/m/alpine.DEB.2.21.1808171206180.20841@lancre
      e7a22179
    • Bruce Momjian's avatar
      doc: adjust PG 11 release notes · 0d45cd96
      Bruce Momjian authored
      Fixes for channel binding, SQL procedures, and pg_trgm.
      
      Backpatch-through: 11
      0d45cd96
    • Tom Lane's avatar
      Remove ruleutils.c's special case for BIT [VARYING] literals. · fedc97cd
      Tom Lane authored
      Up to now, get_const_expr() insisted on prefixing BIT and VARBIT
      literals with 'B'.  That's not really necessary, because we always
      append explicit-cast syntax to identify the constant's type.
      Moreover, it's subtly wrong for VARBIT, because the parser will
      interpret B'...' as '...'::"bit"; see make_const() which explicitly
      assigns type BITOID for a T_BitString literal.  So what had been
      a simple VARBIT literal is reconstructed as ('...'::"bit")::varbit,
      which is not the same thing, at least not before constant folding.
      This results in odd differences after dump/restore, as complained
      of by the patch submitter, and it could result in actual failures in
      partitioning or inheritance DDL operations (see commit 542320c2,
      which repaired similar misbehaviors for some other data types).
      
      Fixing it is pretty easy: just remove the special case and let the
      default code path handle these types.  We could have kept the special
      case for BIT only, but there seems little point in that.
      
      Like the previous patch, I judge that back-patching this into stable
      branches wouldn't be a good idea.  However, it seems not quite too
      late for v11, so let's fix it there.
      
      Paul Guo, reviewed by Davy Machado and John Naylor, minor adjustments
      by me
      
      Discussion: https://postgr.es/m/CABQrizdTra=2JEqA6+Ms1D1k1Kqw+aiBBhC9TreuZRX2JzxLAA@mail.gmail.com
      fedc97cd
    • Andrew Gierth's avatar
      Repair double-free in SP-GIST rescan (bug #15378) · 500d4979
      Andrew Gierth authored
      spgrescan would first reset traversalCxt, and then traverse a
      potentially non-empty stack containing pointers to traversalValues
      which had been allocated in those contexts, freeing them a second
      time. This bug originates in commit ccd6eb49 where traversalValue was
      introduced.
      
      Repair by traversing the stack before the context reset; this isn't
      ideal, since it means doing retail pfree in a context that's about to
      be reset, but the freeing of a stack entry is also done in other
      places in the code during the scan so it's not worth trying to
      refactor it further. Regression test added.
      
      Backpatch to 9.6 where the problem was introduced.
      
      Per bug #15378; analysis and patch by me, originally from a report on
      IRC by user velix; see also PostGIS ticket #4174; review by Alexander
      Korotkov.
      
      Discussion: https://postgr.es/m/153663176628.23136.11901365223750051490@wrigleys.postgresql.org
      500d4979
    • Tom Lane's avatar
      Use -Bsymbolic for shared libraries on HP-UX and Solaris. · 4fa3741d
      Tom Lane authored
      These platforms are also subject to the mis-linking problem addressed
      in commit e3d77ea6.  It's not clear whether we could solve it with
      a solution equivalent to GNU ld's version scripts, but -Bsymbolic
      appears to fix it, so let's use that.
      
      Like the previous commit, back-patch as far as v10.
      
      Discussion: https://postgr.es/m/153626613985.23143.4743626885618266803@wrigleys.postgresql.org
      4fa3741d
  5. 10 Sep, 2018 1 commit
  6. 09 Sep, 2018 6 commits
    • Tom Lane's avatar
      Prevent mis-linking of src/port and src/common functions on *BSD. · e3d77ea6
      Tom Lane authored
      On ELF-based platforms (and maybe others?) it's possible for a shared
      library, when dynamically loaded into the backend, to call the backend
      versions of src/port and src/common functions rather than the frontend
      versions that are actually linked into the shlib.  This is the cause
      of bug #15367 from Jeremy Evans, and is likely to lead to more problems
      in future; it's accidental that we've failed to notice any bad effects
      up to now.
      
      The recommended way to fix this on ELF-based platforms is to use a
      linker "version script" that makes the shlib's versions of the functions
      local.  (Apparently, -Bsymbolic would fix it as well, but with other
      side effects that we don't want.)  Doing so has the additional benefit
      that we can make sure the shlib only exposes the symbols that are meant
      to be part of its API, and not ones that are just for cross-file
      references within the shlib.  So we'd already been using a version
      script for libpq on popular platforms, but it's now apparent that it's
      necessary for correctness on every ELF-based platform.
      
      Hence, add appropriate logic to the openbsd, freebsd, and netbsd stanzas
      of Makefile.shlib; this is just a copy-and-paste from the linux stanza.
      There may be additional work to do if commit ed0cdf0e reveals that the
      problem exists elsewhere, but this is all that is known to be needed
      right now.
      
      Back-patch to v10 where SCRAM support came in.  The problem is ancient,
      but analysis suggests that there were no really severe consequences
      in older branches.  Hence, I won't take the risk of such a large change
      in the build process for older branches.
      
      In passing, remove a rather opaque comment about -Bsymbolic; I don't
      think it's very on-point about why we don't use that, if indeed that's
      what it's talking about at all.
      
      Patch by me; thanks to Andrew Gierth for helping to diagnose the problem,
      and for additional testing.
      
      Discussion: https://postgr.es/m/153626613985.23143.4743626885618266803@wrigleys.postgresql.org
      e3d77ea6
    • Alexander Korotkov's avatar
      Improve behavior of to_timestamp()/to_date() functions · cf984672
      Alexander Korotkov authored
      to_timestamp()/to_date() functions were introduced mainly for Oracle
      compatibility, and became very popular among PostgreSQL users.  However, some
      behavior of to_timestamp()/to_date() functions are both incompatible with Oracle
      and confusing for our users.  This behavior is related to handling of spaces and
      separators in non FX (fixed format) mode.  This commit reworks this behavior
      making less confusing, better documented and more compatible with Oracle.
      
      Nevertheless, there are still following incompatibilities with Oracle.
      1) We don't insist that there are no format string patterns unmatched to
         input string.
      2) In FX mode we don't insist space and separators in format string to exactly
         match input string.
      3) When format string patterns are divided by mix of spaces and separators, we
         don't distinguish them, while Oracle takes into account only last group of
         spaces/separators.
      
      Discussion: https://postgr.es/m/1873520224.1784572.1465833145330.JavaMail.yahoo%40mail.yahoo.com
      Author: Artur Zakirov, Alexander Korotkov, Liudmila Mantrova
      Review: Amul Sul, Robert Haas, Tom Lane, Dmitry Dolgov, David G. Johnston
      cf984672
    • Alexander Korotkov's avatar
      Fix past pd_upper write in ginRedoRecompress() · 5f08accd
      Alexander Korotkov authored
      ginRedoRecompress() replays actions over compressed segments of posting list
      in-place.  However, it might lead to write past pg_upper, because intermediate
      state during playing the changes can take more space than both original state
      and final state.  This commit fixes that by refuse from in-place modification.
      Instead page tail is copied once modification is started, and then it's used
      as the source of original segments.  Backpatch to 9.4 where posting list
      compression was introduced.
      
      Reported-by: Sivasubramanian Ramasubramanian
      Discussion: https://postgr.es/m/1536091151804.6588%40amazon.com
      Author: Alexander Korotkov based on patch from and ideas by Sivasubramanian Ramasubramanian
      Review: Sivasubramanian Ramasubramanian
      Backpatch-through: 9.4
      5f08accd
    • Tom Lane's avatar
      Work around stdbool problem in dfmgr.c. · ff47d4bf
      Tom Lane authored
      Commit 842cb9fa refactored things so that dfmgr.c includes <dlfcn.h>,
      which before that had only been directly included in platform-specific
      stub files.  It turns out that on macOS, <dlfcn.h> includes <stdbool.h>,
      and that causes problems on platforms where _Bool is not char-sized ...
      which happens to include the PPC versions of macOS.  Work around it
      much as we have in plperl.h, by #undef'ing bool after including the
      problematic file, but only if we're not using stdbool-style booleans.
      
      Discussion: https://postgr.es/m/E1fxqjl-0003YS-NS@gemulon.postgresql.org
      ff47d4bf
    • Tom Lane's avatar
      Install a check for mis-linking of src/port and src/common functions. · ed0cdf0e
      Tom Lane authored
      On ELF-based platforms (and maybe others?) it's possible for a shared
      library, when dynamically loaded into the backend, to call the backend
      versions of src/port and src/common functions rather than the frontend
      versions that are actually linked into the shlib.  This is definitely
      not what we want, because the frontend versions often behave slightly
      differently.  Up to now it's been "slight" enough that nobody noticed;
      but with the addition of SCRAM support functions in src/common, we're
      observing crashes due to the difference between palloc and malloc
      memory allocation rules, as reported in bug #15367 from Jeremy Evans.
      
      The purpose of this patch is to create a direct test for this type of
      mis-linking, so that we know whether any given platform requires extra
      measures to prevent using the wrong functions.  If the test fails, it
      will lead to connection failures in the contrib/postgres_fdw regression
      test.  At the moment, *BSD platforms using ELF format are known to have
      the problem and can be expected to fail; but we need to know whether
      anything else does, and we need a reliable ongoing check for future
      platforms.
      
      Actually fixing the problem will be the subject of later commit(s).
      
      Discussion: https://postgr.es/m/153626613985.23143.4743626885618266803@wrigleys.postgresql.org
      ed0cdf0e
    • Noah Misch's avatar
      Allow ENOENT in check_mode_recursive(). · c85ad9cc
      Noah Misch authored
      Buildfarm member tern failed src/bin/pg_ctl/t/001_start_stop.pl when a
      check_mode_recursive() call overlapped a server's startup-time deletion
      of pg_stat/global.stat.  Just warn.  Also, include errno in the message.
      Back-patch to v11, where check_mode_recursive() first appeared.
      c85ad9cc
  7. 08 Sep, 2018 4 commits
    • Noah Misch's avatar
      Fix logical subscriber wait in test. · 076a3c21
      Noah Misch authored
      Buildfarm members sungazer and tern revealed this deficit.  Back-patch
      to v10, like commit 4f10e7ea, which
      introduced the test.
      076a3c21
    • Tom Lane's avatar
      Minor cleanup/future-proofing for pg_saslprep(). · f47f3148
      Tom Lane authored
      Ensure that pg_saslprep() initializes its output argument to NULL in
      all failure paths, and then remove the redundant initialization that
      some (not all) of its callers did.  This does not fix any live bug,
      but it reduces the odds of future bugs of omission.
      
      Also add a comment about why the existing failure-path coding is
      adequate.
      
      Back-patch so as to keep the function's API consistent across branches,
      again to forestall future bug introduction.
      
      Patch by me, reviewed by Michael Paquier
      
      Discussion: https://postgr.es/m/16558.1536407783@sss.pgh.pa.us
      f47f3148
    • Michael Paquier's avatar
      Remove duplicated words split across lines in comments · 9226a3b8
      Michael Paquier authored
      This has been detected using some interesting tricks with sed, and the
      method used is mentioned in details in the discussion below.
      
      Author: Justin Pryzby
      Discussion: https://postgr.es/m/20180908013109.GB15350@telsasoft.com
      9226a3b8
    • Tom Lane's avatar
      Save/restore SPI's global variables in SPI_connect() and SPI_finish(). · 361844fe
      Tom Lane authored
      This patch removes two sources of interference between nominally
      independent functions when one SPI-using function calls another,
      perhaps without knowing that it does so.
      
      Chapman Flack pointed out that xml.c's query_to_xml_internal() expects
      SPI_tuptable and SPI_processed to stay valid across datatype output
      function calls; but it's possible that such a call could involve
      re-entrant use of SPI.  It seems likely that there are similar hazards
      elsewhere, if not in the core code then in third-party SPI users.
      Previously SPI_finish() reset SPI's API globals to zeroes/nulls, which
      would typically make for a crash in such a situation.  Restoring them
      to the values they had at SPI_connect() seems like a considerably more
      useful behavior, and it still meets the design goal of not leaving any
      dangling pointers to tuple tables of the function being exited.
      
      Also, cause SPI_connect() to reset these variables to zeroes/nulls after
      saving them.  This prevents interference in the opposite direction: it's
      possible that a SPI-using function that's only ever been tested standalone
      contains assumptions that these variables start out as zeroes.  That was
      the case as long as you were the outermost SPI user, but not so much for
      an inner user.  Now it's consistent.
      
      Report and fix suggestion by Chapman Flack, actual patch by me.
      Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/9fa25bef-2e4f-1c32-22a4-3ad0723c4a17@anastigmatix.net
      361844fe
  8. 07 Sep, 2018 6 commits
    • Tom Lane's avatar
      Limit depth of forced recursion for CLOBBER_CACHE_RECURSIVELY. · f510412d
      Tom Lane authored
      It's somewhat surprising that we got away with this before.  (Actually,
      since nobody tests this routinely AFAIK, it might've been broken for
      awhile.  But it's definitely broken in the wake of commit f868a814.)
      It seems sufficient to limit the forced recursion to a small number
      of levels.
      
      Back-patch to all supported branches, like the preceding patch.
      
      Discussion: https://postgr.es/m/12259.1532117714@sss.pgh.pa.us
      f510412d
    • Tom Lane's avatar
      Fix longstanding recursion hazard in sinval message processing. · f868a814
      Tom Lane authored
      LockRelationOid and sibling routines supposed that, if our session already
      holds the lock they were asked to acquire, they could skip calling
      AcceptInvalidationMessages on the grounds that we must have already read
      any remote sinval messages issued against the relation being locked.
      This is normally true, but there's a critical special case where it's not:
      processing inside AcceptInvalidationMessages might attempt to access system
      relations, resulting in a recursive call to acquire a relation lock.
      
      Hence, if the outer call had acquired that same system catalog lock, we'd
      fall through, despite the possibility that there's an as-yet-unread sinval
      message for that system catalog.  This could, for example, result in
      failure to access a system catalog or index that had just been processed
      by VACUUM FULL.  This is the explanation for buildfarm failures we've been
      seeing intermittently for the past three months.  The bug is far older
      than that, but commits a54e1f15 et al added a new recursion case within
      AcceptInvalidationMessages that is apparently easier to hit than any
      previous case.
      
      To fix this, we must not skip calling AcceptInvalidationMessages until
      we have *finished* a call to it since acquiring a relation lock, not
      merely acquired the lock.  (There's already adequate logic inside
      AcceptInvalidationMessages to deal with being called recursively.)
      Fortunately, we can implement that at trivial cost, by adding a flag
      to LOCALLOCK hashtable entries that tracks whether we know we have
      completed such a call.
      
      There is an API hazard added by this patch for external callers of
      LockAcquire: if anything is testing for LOCKACQUIRE_ALREADY_HELD,
      it might be fooled by the new return code LOCKACQUIRE_ALREADY_CLEAR
      into thinking the lock wasn't already held.  This should be a fail-soft
      condition, though, unless something very bizarre is being done in
      response to the test.
      
      Also, I added an additional output argument to LockAcquireExtended,
      assuming that that probably isn't called by any outside code given
      the very limited usefulness of its additional functionality.
      
      Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/12259.1532117714@sss.pgh.pa.us
      f868a814
    • Michael Paquier's avatar
      Improve handling of corrupted two-phase state files at recovery · 8582b4d0
      Michael Paquier authored
      When a corrupted two-phase state file is found by WAL replay, be it for
      crash recovery or archive recovery, then the file is simply skipped and
      a WARNING is logged to the user, causing the transaction to be silently
      lost.  Facing an on-disk WAL file which is corrupted is as likely to
      happen as what is stored in WAL records, but WAL records are already
      able to fail hard if there is a CRC mismatch.  On-disk two-phase state
      files, on the contrary, are simply ignored if corrupted.  Note that when
      restoring the initial two-phase data state at recovery, files newer than
      the horizon XID are discarded hence no files present in pg_twophase/
      should be torned and have been made durable by a previous checkpoint, so
      recovery should never see any corrupted two-phase state file by design.
      
      The situation got better since 978b2f65 which has added two-phase state
      information directly in WAL instead of using on-disk files, so the risk
      is limited to two-phase transactions which live across at least one
      checkpoint for long periods.  Backups having legit two-phase state files
      on-disk could also lose silently transactions when restored if things
      get corrupted.
      
      This behavior exists since two-phase commit has been introduced, no
      back-patch is done for now per the lack of complaints about this
      problem.
      
      Author: Michael Paquier
      Discussion: https://postgr.es/m/20180709050309.GM1467@paquier.xyz
      8582b4d0
    • Andrew Gierth's avatar
      Refactor installation of extension headers. · 7b6b167f
      Andrew Gierth authored
      Commit be54b377 failed on gmake 3.80 due to a chained conditional,
      which on closer examination could be removed entirely with some
      refactoring elsewhere for a net simplification and more robustness
      against empty expansions. Along the way, add some more comments.
      
      Also make explicit in the documentation and comments that built
      headers are not removed by 'make clean', since we don't typically want
      that for headers generated by a separate ./configure step, and it's
      much easier to add your own 'distclean' rule or use EXTRA_CLEAN than
      to try and override a deletion rule in pgxs.mk.
      
      Per buildfarm member prariedog and comments by Michael Paquier, though
      all the actual changes are my fault.
      7b6b167f
    • Peter Eisentraut's avatar
      libpq: Change "options" dispchar to normal · 1fea1e32
      Peter Eisentraut authored
      libpq connection options as returned by PQconndefaults() have a
      "dispchar" field that determines (among other things) whether an option
      is a "debug" option, which shouldn't be shown by default to clients.
      postgres_fdw makes use of that to control which connection options to
      accept from a foreign server configuration.
      
      Curiously, the "options" option, which allows passing configuration
      settings to the backend server, was listed as a debug option, which
      prevented it from being used by postgres_fdw.  Maybe it was once meant
      for debugging, but it's clearly in general use nowadays.
      
      So change the dispchar for it to be the normal non-debug case.  Also
      remove the "debug" reference from its label field.
      Reported-by: default avatarShinoda, Noriyoshi <noriyoshi.shinoda@hpe.com>
      1fea1e32
    • Peter Eisentraut's avatar
      Use C99 designated initializers for some structs · 98afa68d
      Peter Eisentraut authored
      These are just a few particularly egregious cases that were hard to read
      and write, and error prone because of many similar adjacent types.
      
      Discussion: https://www.postgresql.org/message-id/flat/4c9f01be-9245-2148-b569-61a8562ef190%402ndquadrant.com
      98afa68d
  9. 06 Sep, 2018 2 commits
    • Tom Lane's avatar
      Fix inconsistent argument naming. · 75f78553
      Tom Lane authored
      Typo in commit 842cb9fa.
      75f78553
    • Tom Lane's avatar
      Make contrib/unaccent's unaccent() function work when not in search path. · a5322ca1
      Tom Lane authored
      Since the fixes for CVE-2018-1058, we've advised people to schema-qualify
      function references in order to fix failures in code that executes under
      a minimal search_path setting.  However, that's insufficient to make the
      single-argument form of unaccent() work, because it looks up the "unaccent"
      text search dictionary using the search path.
      
      The most expedient answer seems to be to remove the search_path dependency
      by making it look in the same schema that the unaccent() function itself
      is declared in.  This will definitely work for the normal usage of this
      function with the unaccent dictionary provided by the extension.
      It's barely possible that there are people who were relying on the
      search-path-dependent behavior to select other dictionaries with the same
      name; but if there are any such people at all, they can still get that
      behavior by writing unaccent('unaccent', ...), or possibly
      unaccent('unaccent'::text::regdictionary, ...) if the lookup has to be
      postponed to runtime.
      
      Per complaint from Gunnlaugur Thor Briem.  Back-patch to all supported
      branches.
      
      Discussion: https://postgr.es/m/CAPs+M8LCex6d=DeneofdsoJVijaG59m9V0ggbb3pOH7hZO4+cQ@mail.gmail.com
      a5322ca1