1. 29 Nov, 2018 4 commits
    • Michael Paquier's avatar
      Add support for NO_INSTALLCHECK in MSVC scripts · 431f1599
      Michael Paquier authored
      When fetching a list of tests for a given extension in contrib/ or
      src/test/modules/, NO_INSTALLCHECK now gets checked first.  If present,
      an empty list of tests is returned to let the caller know that tests
      for this module need to be bypassed.
      
      This actually fixes a set of issues with MSVC with modules using
      REGRESS_OPTS, as an incorrect parsing caused the launched command
      to eat the first test listed.  The actual effect on the tree is that
      several modules listed a single test, so regressions have been running
      with no actual tests.  pg_stat_statements, test_rls_hooks and commit_ts
      were impacted by that.  Some other modules like test_decoding (or
      snapshot_too_old) don't use yet PGXS rules, but their makefiles will
      soon be refactored with an upcoming patch.
      
      Author: Michael Paquier
      Reviewed-by: Andrew Dunstan
      Discussion: https://postgr.es/m/20181126054302.GI1776@paquier.xyz
      431f1599
    • Thomas Munro's avatar
      2ac180c2
    • Michael Paquier's avatar
      Add missing NO_INSTALLCHECK in commit_ts and test_rls_hooks · d79fb5d2
      Michael Paquier authored
      This bypasses installcheck if specified, which makes sense for those
      modules as they require non-default configuration, something which
      typical users don't have.  Those have been missing from the start, still
      no back-patch is done.
      
      This will be used by an upcoming patch for MSVC scripts adding support
      for NO_INSTALLCHECK as installcheck is the default mode for contrib and
      modules for performance reasons in the buildfarm.
      
      Author: Michael Paquier
      Reviewed-by: Andrew Dunstan
      Discussion: https://postgr.es/m/20181126054302.GI1776@paquier.xyz
      d79fb5d2
    • Michael Paquier's avatar
      Fix handling of synchronous replication for stopping WAL senders · 4c703369
      Michael Paquier authored
      This fixes an oversight from c6c33343 which forgot that if a subset of
      WAL senders are stopping and in a sync state, other WAL senders could
      still be waiting for a WAL position to be synced while committing a
      transaction.  However the subset of stopping senders would not release
      waiters, potentially breaking synchronous replication guarantees.  This
      commit makes sure that even WAL senders stopping are able to release
      waiters and are tracked properly.
      
      On 9.4, this can also trigger an assertion failure when setting for
      example max_wal_senders to 1 where a WAL sender is not able to find
      itself as in synchronous state when the instance stops.
      
      Reported-by: Paul Guo
      Author: Paul Guo, Michael Paquier
      Discussion: https://postgr.es/m/CAEET0ZEv8VFqT3C-cQm6byOB4r4VYWcef1J21dOX-gcVhCSpmA@mail.gmail.com
      Backpatch-through: 9.4
      4c703369
  2. 28 Nov, 2018 5 commits
    • Peter Geoghegan's avatar
      Have BufFileSize() ereport() on FileSize() failure. · 1a990b20
      Peter Geoghegan authored
      Move the responsibility for checking for and reporting a failure from
      the only current BufFileSize() caller, logtape.c, to BufFileSize()
      itself.  Code within buffile.c is generally responsible for interfacing
      with fd.c to report irrecoverable failures.  This seems like a
      convention that's worth sticking to.
      
      Reorganizing things this way makes it easy to make the error message
      raised in the event of BufFileSize() failure descriptive of the
      underlying problem.  We're now clear on the distinction between
      temporary file name and BufFile name, and can show errno, confident that
      its value actually relates to the error being reported.  In passing, an
      existing, similar buffile.c ereport() + errcode_for_file_access() site
      is changed to follow the same conventions.
      
      The API of the function BufFileSize() is changed by this commit, despite
      already being in a stable release (Postgres 11).  This seems acceptable,
      since the BufFileSize() ABI was changed by commit aa551830, which
      hasn't made it into a point release yet.  Besides, it's difficult to
      imagine a third party BufFileSize() caller not just raising an error
      anyway, since BufFile state should be considered corrupt when
      BufFileSize() fails.
      
      Per complaint from Tom Lane.
      
      Discussion: https://postgr.es/m/26974.1540826748@sss.pgh.pa.us
      Backpatch: 11-, where shared BufFiles were introduced.
      1a990b20
    • Peter Eisentraut's avatar
      Only allow one recovery target setting · f2cbffc7
      Peter Eisentraut authored
      The previous recovery.conf regime accepted multiple recovery_target*
      settings and used the last one.  This does not translate well to the
      general GUC system.  Specifically, under EXEC_BACKEND, the settings
      are written out not in any particular order, so the order in which
      they were originally set is not available to new processes.
      
      Rather than redesign the GUC system, it was decided to abandon the old
      behavior and only allow one recovery target setting.  A second setting
      will cause an error.  However, it is allowed to set the same parameter
      multiple times or unset a parameter and set a different one.
      
      Discussion: https://www.postgresql.org/message-id/flat/27802171543235530%40iva2-6ec8f0a6115e.qloud-c.yandex.net#701a59c837ad0bf8c244344aaf3ef5a4
      f2cbffc7
    • Bruce Momjian's avatar
      C comment: remove extra '*' · eae9143d
      Bruce Momjian authored
      Reported-by: Etsuro Fujita
      
      Discussion: https://postgr.es/m/5BFE34DE.1080404@lab.ntt.co.jp
      
      Author: Etsuro Fujita
      
      Backpatch-through: 10
      eae9143d
    • Thomas Munro's avatar
      Don't set PAM_RHOST for Unix sockets. · 0f9cdd7d
      Thomas Munro authored
      Since commit 2f1d2b7a we have set PAM_RHOST to "[local]" for Unix
      sockets.  This caused Linux PAM's libaudit integration to make DNS
      requests for that name.  It's not exactly clear what value PAM_RHOST
      should have in that case, but it seems clear that we shouldn't set it
      to an unresolvable name, so don't do that.
      
      Back-patch to 9.6.  Bug #15520.
      
      Author: Thomas Munro
      Reviewed-by: Peter Eisentraut
      Reported-by: Albert Schabhuetl
      Discussion: https://postgr.es/m/15520-4c266f986998e1c5%40postgresql.org
      0f9cdd7d
    • Tomas Vondra's avatar
      Do not decode TOAST data for table rewrites · f69c959d
      Tomas Vondra authored
      During table rewrites (VACUUM FULL and CLUSTER), the main heap is logged
      using XLOG / FPI records, and thus (correctly) ignored in decoding.
      But the associated TOAST table is WAL-logged as plain INSERT records,
      and so was logically decoded and passed to reorder buffer.
      
      That has severe consequences with TOAST tables of non-trivial size.
      Firstly, reorder buffer has to keep all those changes, possibly spilling
      them to a file, incurring I/O costs and disk space.
      
      Secondly, ReoderBufferCommit() was stashing all those TOAST chunks into
      a hash table, which got discarded only after processing the row from the
      main heap.  But as the main heap is not decoded for rewrites, this never
      happened, so all the TOAST data accumulated in memory, resulting either
      in excessive memory consumption or OOM.
      
      The fix is simple, as commit e9edc1ba already introduced infrastructure
      (namely HEAP_INSERT_NO_LOGICAL flag) to skip logical decoding of TOAST
      tables, but it only applied it to system tables.  So simply use it for
      all TOAST data in raw_heap_insert().
      
      That would however solve only the memory consumption issue - the TOAST
      changes would still be decoded and added to the reorder buffer, and
      spilled to disk (although without TOAST tuple data, so much smaller).
      But we can solve that by tweaking DecodeInsert() to just ignore such
      INSERT records altogether, using XLH_INSERT_CONTAINS_NEW_TUPLE flag,
      instead of skipping them later in ReorderBufferCommit().
      
      Review: Masahiko Sawada
      Discussion: https://www.postgresql.org/message-id/flat/1a17c643-e9af-3dba-486b-fbe31bc1823a%402ndquadrant.com
      Backpatch: 9.4-, where logical decoding was introduced
      f69c959d
  3. 27 Nov, 2018 8 commits
  4. 26 Nov, 2018 14 commits
    • Andres Freund's avatar
      Fix typo introduced in 578b2297. · 54bb22f6
      Andres Freund authored
      Author: Andreas Karlsson
      Discussion: https://postgr.es/m/0917c86f-e906-27c0-740e-abc581480823@proxel.se
      54bb22f6
    • Andres Freund's avatar
      Fix pg_upgrade for oid removal. · 12a53c73
      Andres Freund authored
      pg_upgrade previously copied pg_largeobject_metadata over from the old
      cluster. That doesn't work, because the table has oids before
      578b2297. I missed that.
      
      As most pieces of metadata for large objects already were dumped as
      DDL (except for comments overwritten by pg_upgrade, due to the copy of
      pg_largeobject_metadata) it seems reasonable to just also dump grants
      for large objects.  If we ever consider this a relevant performance
      problem, we'd need to fix the rest of the already emitted DDL
      too.
      
      There's still an open discussion about whether we'll want to force a
      specific ordering for the dumped objects, as currently
      pg_largeobjects_metadata potentially has a different ordering
      before/after pg_upgrade, which can make automated testing a bit
      harder.
      
      Reported-By: Andrew Dunstan
      Author: Andres Freund
      Discussion: https://postgr.es/m/91a8a980-41bc-412b-fba2-2ba71a141c2b@2ndQuadrant.com
      12a53c73
    • Tom Lane's avatar
      Fix translation of special characters in psql's LaTeX output modes. · 70d7e507
      Tom Lane authored
      latex_escaped_print() mistranslated \ and failed to provide any translation
      for # ^ and ~, all of which would typically lead to LaTeX document syntax
      errors.  In addition it didn't translate < > and |, which would typically
      render as unexpected characters.
      
      To some extent this represents shortcomings in ancient versions of LaTeX,
      which if memory serves had no easy way to render these control characters
      as ASCII text.  But that's been fixed for, um, decades.  In any case there
      is no value in emitting guaranteed-to-fail output for these characters.
      
      Noted while fooling with test cases added by commit 9a98984f.  Back-patch
      the code change to all supported versions.
      70d7e507
    • Tom Lane's avatar
      Avoid locale-dependent output in numericlocale check. · 95dcb8fc
      Tom Lane authored
      I'd forgotten that in the buildfarm, parts of the regression tests
      may run with psql exposed to a non-default LC_NUMERIC setting.
      Hence we can't assume that C locale prevails, nor is there any
      accessible way to force the setting for this single test step.
      Lobotomize the test case added by commit 9a98984f so that it covers as
      much as we can of print.c without having any locale-varying output.
      95dcb8fc
    • Alvaro Herrera's avatar
      Fix sample output for hash_metapage_info query · 67ed3b9d
      Alvaro Herrera authored
      One output column was duplicated.  Couldn't resist fixing the version
      number while at it.
      
      Reported-by: Gianni Ciolli
      67ed3b9d
    • Tom Lane's avatar
      Add CSV table output mode in psql. · aa2ba50c
      Tom Lane authored
      "\pset format csv", or --csv, selects comma-separated values table format.
      This is compliant with RFC 4180, except that we aren't too picky about
      whether the record separator is LF or CRLF; also, the user may choose a
      field separator other than comma.
      
      This output format is directly compatible with the server's COPY CSV
      format, and will also be useful as input to other programs.  It's
      considerably safer for that purpose than the old recommendation to
      use "unaligned" format, since the latter couldn't handle data
      containing the field separator character.
      
      Daniel Vérité, reviewed by Fabien Coelho and David Fetter, some
      tweaking by me
      
      Discussion: https://postgr.es/m/a8de371e-006f-4f92-ab72-2bbe3ee78f03@manitou-mail.org
      aa2ba50c
    • Tom Lane's avatar
      Improve regression test coverage for psql output formats. · 9a98984f
      Tom Lane authored
      As penance for the "\pset format latex" silliness, add some regression
      test coverage for the off-the-beaten-path output formats, which formerly
      had exactly no coverage, except for some poorly-thought-out (unreadable,
      repetitive, and incomplete) tests for asciidoc format.
      
      I make no claims for the behavior exposed here actually being correct;
      these test cases are just designed to ensure full code coverage in
      fe_utils/print.c.  This brings the line coverage for that file up
      from ~60% to ~93%.
      9a98984f
    • Tom Lane's avatar
      Fix breakage of "\pset format latex". · a7eece4f
      Tom Lane authored
      Commit eaf746a5 unintentionally made psql's "latex" output format
      inaccessible, since not only "latex" but all abbreviations of it
      were considered ambiguous against "latex-longtable".  Let's go
      back to the longstanding behavior that all shortened versions
      mean "latex", and you have to write at least "latex-" to get
      "latex-longtable".  This leaves the only difference from pre-v12
      behavior being that "\pset format a" is considered ambiguous.
      
      The fact that the regression tests didn't expose this is pretty bad,
      but fixing it is material for a separate commit.
      
      Discussion: https://postgr.es/m/cb7e1caf-3ea6-450d-af28-f524903a030c@manitou-mail.org
      a7eece4f
    • Alvaro Herrera's avatar
      Clarify that cross-row constraints are unsupported · 36d442a2
      Alvaro Herrera authored
      Maybe we'll implement them later, or maybe not, but let's make the statu
      quo clear for now.
      
      Author: Lætitia Avrot, Patrick Francelle
      Reviewers: too many to list
      Discussion: https://postgr.es/m/CAB_COdhUuzNFOJfc7SNNso5rOuVA3ui93KMVunEM8Yih+K5A6A@mail.gmail.com
      36d442a2
    • Michael Paquier's avatar
      Revert "Fix typo in documentation of toast storage" · 664f01b6
      Michael Paquier authored
      This reverts commit 058ef3a1, per complains from Magnus Hagander and Vik
      Fearing.
      664f01b6
    • Michael Paquier's avatar
    • Michael Paquier's avatar
      Revert all new recent changes to add PGXS options for TAP and isolation · 1d7dd186
      Michael Paquier authored
      A set of failures in buildfarm machines are proving that this is not
      quite ready yet because of another set of issues:
      - MSVC scripts assume that REGRESS_OPTS can only use top_builddir.  Some
      test suites actually finish by using top_srcdir, like pg_stat_statements
      which cause the regression tests to never run.
      - Trying to enforce top_builddir does not work either when using VPATH
      as this is not recognized properly.
      - TAP tests of bloom are unstable on various platforms, causing various
      failures.
      1d7dd186
    • Michael Paquier's avatar
      Fix regression test handling of test_decoding with MSVC · 3955cae0
      Michael Paquier authored
      The set of scripts in charge of running the regression tests for MSVC
      run currently under the assumption that only $(top_builddir) can used in
      option values defined in REGRESS_OPTS, and those options need to have a
      specific format as well to be correctly parsed, so fix the Makefile
      values so as those are correctly set.
      
      Per complains from buildfarm member dory and whelk, with some extra
      testing done on my side with MSVC to check this patch.
      3955cae0
    • Michael Paquier's avatar
      Disable temporarily TAP tests for contrib/bloom/ · b0b1f418
      Michael Paquier authored
      The recent commit 03faa4a8 has enabled those tests, however several
      buildfarm members are complaining about their stability on Windows and
      macOS.  This will keep the buildfarm green, while investigating the
      root problem.
      
      Discussion: https://postgr.es/m/20181126003351.GE1776@paquier.xyz
      b0b1f418
  5. 25 Nov, 2018 3 commits
    • Michael Paquier's avatar
      Add PGXS options to control TAP and isolation tests · 03faa4a8
      Michael Paquier authored
      The following options are added for extensions:
      - TAP_TESTS, to allow an extention to run TAP tests which are the ones
      present in t/*.pl.  A subset of tests can always be run with the
      existing PROVE_TESTS for developers.
      - ISOLATION, to define a list of isolation tests.
      - ISOLATION_OPTS, to pass custom options to isolation_tester.
      
      A couple of custom Makefile targets have been accumulated across the
      tree to cover the lack of facility in PGXS for a couple of releases when
      using those test suites, which are all now replaced with the new flags,
      without reducing the test coverage.  This also fixes an issue with
      contrib/bloom/, which had a custom target to trigger its TAP tests of
      its own not part of the main check runs.
      
      Author: Michael Paquier
      Reviewed-by: Adam Berlin, Álvaro Herrera, Tom Lane, Nikolay Shaplov,
      Arthur Zakirov
      Discussion: https://postgr.es/m/20180906014849.GG2726@paquier.xyz
      03faa4a8
    • Peter Eisentraut's avatar
      Integrate recovery.conf into postgresql.conf · 2dedf4d9
      Peter Eisentraut authored
      recovery.conf settings are now set in postgresql.conf (or other GUC
      sources).  Currently, all the affected settings are PGC_POSTMASTER;
      this could be refined in the future case by case.
      
      Recovery is now initiated by a file recovery.signal.  Standby mode is
      initiated by a file standby.signal.  The standby_mode setting is
      gone.  If a recovery.conf file is found, an error is issued.
      
      The trigger_file setting has been renamed to promote_trigger_file as
      part of the move.
      
      The documentation chapter "Recovery Configuration" has been integrated
      into "Server Configuration".
      
      pg_basebackup -R now appends settings to postgresql.auto.conf and
      creates a standby.signal file.
      
      Author: Fujii Masao <masao.fujii@gmail.com>
      Author: Simon Riggs <simon@2ndquadrant.com>
      Author: Abhijit Menon-Sen <ams@2ndquadrant.com>
      Author: Sergei Kornilov <sk@zsrv.org>
      Discussion: https://www.postgresql.org/message-id/flat/607741529606767@web3g.yandex.ru/
      2dedf4d9
    • Thomas Munro's avatar
      Fix assertion failure for SSL connections. · ab69ea9f
      Thomas Munro authored
      Commit cfdf4dc4 added an assertion that every WaitLatch() or similar
      handles postmaster death.  One place did not, but was missed in
      review and testing due to the need for an SSL connection.  Fix, by
      asking for WL_EXIT_ON_PM_DEATH.
      
      Reported-by: Christoph Berg
      Discussion: https://postgr.es/m/20181124143845.GA15039%40msg.df7cb.de
      ab69ea9f
  6. 24 Nov, 2018 4 commits
    • Andrew Gierth's avatar
      Fix hstore hash function for empty hstores upgraded from 8.4. · d5890f49
      Andrew Gierth authored
      Hstore data generated on pg 8.4 and pg_upgraded to current versions
      remains in its original on-disk format unless modified. The same goes
      for values generated by the addon hstore-new module on pre-9.0
      versions. (The hstoreUpgrade function converts old values on the fly
      when read in, but the on-disk value is not modified by this.)
      
      Since old-format empty hstores (and hstore-new hstores) have
      representations compatible with the new format, hstoreUpgrade thought
      it could get away without modifying such values; but this breaks
      hstore_hash (and the new hstore_hash_extended) which assumes
      bit-perfect matching between semantically identical hstore values.
      Only one bit actually differs (the "new version" flag in the count
      field) but that of course is enough to break the hash.
      
      Fix by making hstoreUpgrade unconditionally convert all old values to
      new format.
      
      Backpatch all the way, even though this changes a hash value in some
      cases, because in those cases the hash value is already failing - for
      example, a hash join between old- and new-format empty hstores will be
      failing to match, or a hash index on an hstore column containing an
      old-format empty value will be failing to find the value since it will
      be searching for a hash derived from a new-format datum. (There are no
      known field reports of this happening, probably because hashing of
      hstores has only been useful in limited circumstances and there
      probably isn't much upgraded data being used this way.)
      
      Per concerns arising from discussion of commit eb6f2914. Original
      bug is my fault.
      
      Discussion: https://postgr.es/m/60b1fd3b-7332-40f0-7e7f-f2f04f777747%402ndquadrant.com
      d5890f49
    • Andrew Gierth's avatar
      Avoid crashes in contrib/intarray gist__int_ops (bug #15518) · 757c5182
      Andrew Gierth authored
      1. Integer overflow in internal_size could result in memory corruption
      in decompression since a zero-length array would be allocated and then
      written to. This leads to crashes or corruption when traversing an
      index which has been populated with sufficiently sparse values. Fix by
      using int64 for computations and checking for overflow.
      
      2. Integer overflow in g_int_compress could cause pessimal merge
      choices, resulting in unnecessarily large ranges (which would in turn
      trigger issue 1 above). Fix by using int64 again.
      
      3. Even without overflow, array sizes could become large enough to
      cause unexplained memory allocation errors. Fix by capping the sizes
      to a safe limit and report actual errors pointing at gist__intbig_ops
      as needed.
      
      4. Large inputs to the compression function always consist of large
      runs of consecutive integers, and the compression loop was processing
      these one at a time in an O(N^2) manner with a lot of overhead. The
      expected runtime of this function could easily exceed 6 months for a
      single call as a result. Fix by performing a linear-time first pass,
      which reduces the worst case to something on the order of seconds.
      
      Backpatch all the way, since this has been wrong forever.
      
      Per bug #15518 from report from irc user "dymk", analysis and patch by
      me.
      
      Discussion: https://postgr.es/m/15518-799e426c3b4f8358@postgresql.org
      757c5182
    • Tom Lane's avatar
      Adjust new test case for more portability. · 452b637d
      Tom Lane authored
      Early returns from the buildfarm say that most critters are good with
      commit cbdb8b4c, but gaur gives unexpected results with the test case
      involving a float8 that's one-ULP-less-than-2^63.  It appears that that
      platform's version of rint() rounds that value up to 2^63 instead of
      leaving it be.  This is possibly a bug, and it's also possible that no
      other platform anybody is using anywhere behaves likewise.  Still, the
      point of the test is not to insist that everybody's rint() behaves exactly
      the same.  Let's use two-ULPs-less-than-2^63 instead, which I've tested
      to act the same on gaur as on more modern hardware.
      
      (This is, more or less, exactly the portability issue I'd feared might
      arise...)
      
      Discussion: https://postgr.es/m/15519-4fc785b483201ff1@postgresql.org
      452b637d
    • Tom Lane's avatar
      Fix float-to-integer coercions to handle edge cases correctly. · cbdb8b4c
      Tom Lane authored
      ftoi4 and its sibling coercion functions did their overflow checks in
      a way that looked superficially plausible, but actually depended on an
      assumption that the MIN and MAX comparison constants can be represented
      exactly in the float4 or float8 domain.  That fails in ftoi4, ftoi8,
      and dtoi8, resulting in a possibility that values near the MAX limit will
      be wrongly converted (to negative values) when they need to be rejected.
      
      Also, because we compared before rounding off the fractional part,
      the other three functions threw errors for values that really ought
      to get rounded to the min or max integer value.
      
      Fix by doing rint() first (requiring an assumption that it handles
      NaN and Inf correctly; but dtoi8 and ftoi8 were assuming that already),
      and by comparing to values that should coerce to float exactly, namely
      INTxx_MIN and -INTxx_MIN.  Also remove some random cosmetic discrepancies
      between these six functions.
      
      Per bug #15519 from Victor Petrovykh.  This should get back-patched,
      but first let's see what the buildfarm thinks of it --- I'm not too
      sure about portability of some of the regression test cases.
      
      Patch by me; thanks to Andrew Gierth for analysis and discussion.
      
      Discussion: https://postgr.es/m/15519-4fc785b483201ff1@postgresql.org
      cbdb8b4c
  7. 23 Nov, 2018 2 commits