1. 14 Jan, 2017 5 commits
    • Tom Lane's avatar
      Change representation of statement lists, and add statement location info. · ab1f0c82
      Tom Lane authored
      This patch makes several changes that improve the consistency of
      representation of lists of statements.  It's always been the case
      that the output of parse analysis is a list of Query nodes, whatever
      the types of the individual statements in the list.  This patch brings
      similar consistency to the outputs of raw parsing and planning steps:
      
      * The output of raw parsing is now always a list of RawStmt nodes;
      the statement-type-dependent nodes are one level down from that.
      
      * The output of pg_plan_queries() is now always a list of PlannedStmt
      nodes, even for utility statements.  In the case of a utility statement,
      "planning" just consists of wrapping a CMD_UTILITY PlannedStmt around
      the utility node.  This list representation is now used in Portal and
      CachedPlan plan lists, replacing the former convention of intermixing
      PlannedStmts with bare utility-statement nodes.
      
      Now, every list of statements has a consistent head-node type depending
      on how far along it is in processing.  This allows changing many places
      that formerly used generic "Node *" pointers to use a more specific
      pointer type, thus reducing the number of IsA() tests and casts needed,
      as well as improving code clarity.
      
      Also, the post-parse-analysis representation of DECLARE CURSOR is changed
      so that it looks more like EXPLAIN, PREPARE, etc.  That is, the contained
      SELECT remains a child of the DeclareCursorStmt rather than getting flipped
      around to be the other way.  It's now true for both Query and PlannedStmt
      that utilityStmt is non-null if and only if commandType is CMD_UTILITY.
      That allows simplifying a lot of places that were testing both fields.
      (I think some of those were just defensive programming, but in many places,
      it was actually necessary to avoid confusing DECLARE CURSOR with SELECT.)
      
      Because PlannedStmt carries a canSetTag field, we're also able to get rid
      of some ad-hoc rules about how to reconstruct canSetTag for a bare utility
      statement; specifically, the assumption that a utility is canSetTag if and
      only if it's the only one in its list.  While I see no near-term need for
      relaxing that restriction, it's nice to get rid of the ad-hocery.
      
      The API of ProcessUtility() is changed so that what it's passed is the
      wrapper PlannedStmt not just the bare utility statement.  This will affect
      all users of ProcessUtility_hook, but the changes are pretty trivial; see
      the affected contrib modules for examples of the minimum change needed.
      (Most compilers should give pointer-type-mismatch warnings for uncorrected
      code.)
      
      There's also a change in the API of ExplainOneQuery_hook, to pass through
      cursorOptions instead of expecting hook functions to know what to pick.
      This is needed because of the DECLARE CURSOR changes, but really should
      have been done in 9.6; it's unlikely that any extant hook functions
      know about using CURSOR_OPT_PARALLEL_OK.
      
      Finally, teach gram.y to save statement boundary locations in RawStmt
      nodes, and pass those through to Query and PlannedStmt nodes.  This allows
      more intelligent handling of cases where a source query string contains
      multiple statements.  This patch doesn't actually do anything with the
      information, but a follow-on patch will.  (Passing this information through
      cleanly is the true motivation for these changes; while I think this is all
      good cleanup, it's unlikely we'd have bothered without this end goal.)
      
      catversion bump because addition of location fields to struct Query
      affects stored rules.
      
      This patch is by me, but it owes a good deal to Fabien Coelho who did
      a lot of preliminary work on the problem, and also reviewed the patch.
      
      Discussion: https://postgr.es/m/alpine.DEB.2.20.1612200926310.29821@lancre
      ab1f0c82
    • Tom Lane's avatar
      Throw suitable error for COPY TO STDOUT/FROM STDIN in a SQL function. · 75abb955
      Tom Lane authored
      A client copy can't work inside a function because the FE/BE wire protocol
      doesn't support nesting of a COPY operation within query results.  (Maybe
      it could, but the protocol spec doesn't suggest that clients should support
      this, and libpq for one certainly doesn't.)
      
      In most PLs, this prohibition is enforced by spi.c, but SQL functions don't
      use SPI.  A comparison of _SPI_execute_plan() and init_execution_state()
      shows that rejecting client COPY is the only discrepancy in what they
      allow, so there's no other similar bugs.
      
      This is an astonishingly ancient oversight, so back-patch to all supported
      branches.
      
      Report: https://postgr.es/m/BY2PR05MB2309EABA3DEFA0143F50F0D593780@BY2PR05MB2309.namprd05.prod.outlook.com
      75abb955
    • Magnus Hagander's avatar
      Change default values for backup and replication parameters · f6d6d292
      Magnus Hagander authored
      This changes the default values of the following parameters:
      
      wal_level = replica
      max_wal_senders = 10
      max_replication_slots = 10
      
      in order to make it possible to make a backup and set up simple
      replication on the default settings, without requiring a system restart.
      
      Discussion: https://postgr.es/m/CABUevEy4PR_EAvZEzsbF5s+V0eEvw7shJ2t-AUwbHOjT+yRb3A@mail.gmail.com
      
      Reviewed by Peter Eisentraut. Benchmark help from Tomas Vondra.
      f6d6d292
    • Peter Eisentraut's avatar
      pg_ctl: Change default to wait for all actions · 05cd12ed
      Peter Eisentraut authored
      The different actions in pg_ctl had different defaults for -w and -W,
      mostly for historical reasons.  Most users will want the -w behavior, so
      make that the default.
      
      Remove the -w option in most example and test code, so avoid confusion
      and reduce verbosity.  pg_upgrade is not touched, so it can continue to
      work with older installations.
      Reviewed-by: default avatarBeena Emerson <memissemerson@gmail.com>
      Reviewed-by: default avatarRyan Murphy <ryanfmurphy@gmail.com>
      05cd12ed
    • Peter Eisentraut's avatar
      Updates to reflect that pg_ctl stop -m fast is the default · e574f15d
      Peter Eisentraut authored
      Various example and test code used -m fast explicitly, but since it's
      the default, this can be omitted now or should be replaced by a better
      example.
      
      pg_upgrade is not touched, so it can continue to operate with older
      installations.
      e574f15d
  2. 13 Jan, 2017 5 commits
    • Tom Lane's avatar
      Fix some more regression test row-order-instability issues. · 5ad966ab
      Tom Lane authored
      Commit 0563a3a8 just introduced another instance of the same unsafe
      testing methodology that appeared in 2ac3ef7a, which I corrected in
      257d8157.  Robert/Amit, please stop doing that.
      
      Also look through the rest of f0e44751's test cases, and correct some
      other queries with underdetermined ordering of results from the system
      catalogs.  These haven't failed in the buildfarm yet, but I don't
      have any confidence in that staying true.
      
      Per multiple buildfarm members.
      5ad966ab
    • Tom Lane's avatar
      In PL/Tcl tests, don't choke if optional error fields are missing. · 5b29e6b6
      Tom Lane authored
      This fixes a portability issue introduced by commit 961bed02: with a
      compiler that doesn't support PG_FUNCNAME_MACRO, the "funcname" field of
      errorCode won't be provided, leading to a failure of the unset command.
      I added -nocomplain to the unset commands for filename and lineno too, just
      in case, though I know of no platform that wouldn't populate those fields.
      (BTW, -nocomplain is new in Tcl 8.4, but fortunately we dropped support
      for pre-8.4 Tcl some time ago.)
      
      Per buildfarm member pademelon.
      5b29e6b6
    • Peter Eisentraut's avatar
      pg_upgrade: Fix for changed pg_ctl default stop mode · 7f5b043d
      Peter Eisentraut authored
      In 9.5, the default pg_ctl stop mode was changed from "smart" to "fast".
      pg_upgrade still thought the default mode was "smart" and only specified
      the mode when "fast" was asked for.  This results in using "fast" all
      the time.  It's not clear what the effect in practice is, but fix it
      nonetheless to restore the previous behavior.
      7f5b043d
    • Robert Haas's avatar
      Fix a bug in how we generate partition constraints. · 0563a3a8
      Robert Haas authored
      Move the code for doing parent attnos to child attnos mapping for Vars
      in partition constraint expressions to a separate function
      map_partition_varattnos() and call it from the appropriate places.
      Doing it in get_qual_from_partbound(), as is now, would produce wrong
      result in certain multi-level partitioning cases, because it only
      considers the current pair of parent-child relations.  In certain
      multi-level partitioning cases, attnums for the same key attribute(s)
      might differ between various levels causing the same attribute to be
      numbered differently in different instances of the Var corresponding
      to a given attribute.
      
      With this commit, in generate_partition_qual(), we first generate the
      the whole partition constraint (considering all levels of partitioning)
      and then do the mapping, so that Vars in the final expression are
      numbered according the leaf relation (to which it is supposed to apply).
      
      Amit Langote, reviewed by me.
      0563a3a8
    • Robert Haas's avatar
      Fix cardinality estimates for parallel joins. · 0c2070ce
      Robert Haas authored
      For a partial path, the cardinality estimate needs to reflect the
      number of rows we think each worker will see, rather than the total
      number of rows; otherwise, costing will go wrong.  The previous coding
      got this completely wrong for parallel joins.
      
      Unfortunately, this change may destabilize plans for users of 9.6 who
      have enabled parallel query, but since 9.6 is still fairly new I'm
      hoping expectations won't be too settled yet.  Also, this is really a
      brown-paper-bag bug, so leaving it unfixed for the entire lifetime of
      9.6 seems unwise.
      
      Related reports (whose import I initially failed to recognize) by
      Tomas Vondra and Tom Lane.
      
      Discussion: http://postgr.es/m/CA+TgmoaDxZ5z5Kw_oCQoymNxNoVaTCXzPaODcOuao=CzK8dMZw@mail.gmail.com
      0c2070ce
  3. 12 Jan, 2017 4 commits
  4. 11 Jan, 2017 3 commits
    • Stephen Frost's avatar
      pg_restore: Don't allow non-positive number of jobs · e72059f3
      Stephen Frost authored
      pg_restore will currently accept invalid values for the number of
      parallel jobs to run (eg: -1), unlike pg_dump which does check that the
      value provided is reasonable.
      
      Worse, '-1' is actually a valid, independent, parameter (as an alias for
      --single-transaction), leading to potentially completely unexpected
      results from a command line such as:
      
        -> pg_restore -j -1
      
      Where a user would get neither parallel jobs nor a single-transaction.
      
      Add in validity checking of the parallel jobs option, as we already have
      in pg_dump, before we try to open up the archive.  Also move the check
      that we haven't been asked to run more parallel jobs than possible on
      Windows to the same place, so we do all the option validity checking
      before opening the archive.
      
      Back-patch all the way, though for 9.2 we're adding the Windows-specific
      check against MAXIMUM_WAIT_OBJECTS as that check wasn't back-patched
      originally.
      
      Discussion: https://www.postgresql.org/message-id/20170110044815.GC18360%40tamriel.snowman.net
      e72059f3
    • Magnus Hagander's avatar
      Fix some typos in comments · 268f9e3d
      Magnus Hagander authored
      Masahiko Sawada
      268f9e3d
    • Bruce Momjian's avatar
      pg_xlogdump: document --path behavior · 73f8d733
      Bruce Momjian authored
      The previous --path documentation and --help output were wrong in both
      its meaning and the defaults.
      
      Reviewed-by: Michael Paquier
      
      Backpatch-through: 9.6
      73f8d733
  5. 10 Jan, 2017 4 commits
    • Stephen Frost's avatar
      pg_dump: Strict names with no matching schema · abfd0095
      Stephen Frost authored
      When using pg_dump --strict-names and a schema pattern which doesn't
      match any schemas (eg: --schema='nonexistant*'), we were incorrectly
      throwing an error claiming no tables were found when, really, there
      were no schemas found:
      
        -> pg_dump --strict-names --schema='nonexistant*'
        pg_dump: no matching tables were found for pattern "nonexistant*"
      
      Fix that by changing the error message to say 'schemas' instead, since
      that is what we are actually complaining about.
      
      Noticed while testing pg_dump error cases.
      
      Back-patch to 9.6 where --strict-names and this error message were
      introduced.
      abfd0095
    • Alvaro Herrera's avatar
      Fix overflow check in StringInfo; add missing casts · 42f50cb8
      Alvaro Herrera authored
      A few thinkos I introduced in fa2fa995.  Also, amend a similarly
      broken comment.
      
      Report by Daniel Vérité.
      Authors: Daniel Vérité, Álvaro Herrera
      Discussion: https://postgr.es/m/1706e85e-60d2-494e-8a64-9af1e1b2186e@manitou-mail.org
      42f50cb8
    • Robert Haas's avatar
      Improve coding in _hash_addovflpage. · e8984374
      Robert Haas authored
      Instead of relying on the page contents to know whether we have
      advanced from the primary bucket page to an overflow page, track
      that explicitly.
      
      Amit Kapila, per a complaint by me.
      e8984374
    • Stephen Frost's avatar
      Fix invalid-parallel-jobs error message · 2ef6fe9c
      Stephen Frost authored
      Including the program name twice is not helpful:
      
      -> pg_dump -j -1
      pg_dump: pg_dump: invalid number of parallel jobs
      
      Correct by removing the progname from the exit_horribly() call used when
      validating the number of parallel jobs.
      
      Noticed while testing various pg_dump error cases.
      
      Back-patch to 9.3 where parallel pg_dump was added.
      2ef6fe9c
  6. 09 Jan, 2017 5 commits
    • Tom Lane's avatar
      Fix error handling in pltcl_returnnext. · 8c572294
      Tom Lane authored
      We can't throw elog(ERROR) out of a Tcl command procedure; we have
      to catch the error and return TCL_ERROR to the Tcl interpreter.
      pltcl_returnnext failed to meet this requirement, so that errors
      detected by pltcl_build_tuple_result or other functions called here
      led to longjmp'ing out of the Tcl interpreter and thereby leaving it
      in a bad state.  Use the existing subtransaction support to prevent
      that.  Oversight in commit 26abb50c, found more or less accidentally
      by the buildfarm thanks to the tests added in 961bed02.
      
      Report: https://postgr.es/m/30647.1483989734@sss.pgh.pa.us
      8c572294
    • Alvaro Herrera's avatar
      Fix ALTER TABLE / SET TYPE for irregular inheritance · 3957b58b
      Alvaro Herrera authored
      If inherited tables don't have exactly the same schema, the USING clause
      in an ALTER TABLE / SET DATA TYPE misbehaves when applied to the
      children tables since commit 9550e834.  Starting with that commit,
      the attribute numbers in the USING expression are fixed during parse
      analysis.  This can lead to bogus errors being reported during
      execution, such as:
         ERROR:  attribute 2 has wrong type
         DETAIL:  Table has type smallint, but query expects integer.
      
      Since it wouldn't do to revert to the original coding, we now apply a
      transformation to map the attribute numbers to the correct ones for each
      child.
      
      Reported by Justin Pryzby
      Analysis by Tom Lane; patch by me.
      Discussion: https://postgr.es/m/20170102225618.GA10071@telsasoft.com
      3957b58b
    • Alvaro Herrera's avatar
      BRIN revmap pages are not standard pages ... · 7403561c
      Alvaro Herrera authored
      ... and therefore we ought not to tell XLogRegisterBuffer the opposite,
      when writing XLog for a brin update that moves the index tuple to a
      different page.  Otherwise, xlog insertion would try to "compress the
      hole" when producing a full-page image for it; but since we don't update
      pd_lower/upper, the hole covers the whole page.  On WAL replay, the
      revmap page becomes empty and so the entire portion of the index is
      useless and needs to be recomputed.
      
      This is low-probability: a BRIN update only moves an index tuple to a
      different page when the summary tuple is larger than the existing one,
      which doesn't happen with fixed-width datatypes.  Also, the revmap
      page must be first after a checkpoint.
      
      Report and patch: Kuntal Ghosh
      Bug is alleged to have detected by a WAL-consistency-checking tool.
      Discussion: https://postgr.es/m/CAGz5QCJ=00UQjScSEFbV=0qO5ShTZB9WWz_Fm7+Wd83zPs9Geg@mail.gmail.com
      
      I posted a test case demonstrating the problem, but I'm refraining from
      adding it to the test suite; if the WAL consistency tool makes it in,
      that will be a better way to catch this from regressing.  (We should
      definitely have someting that causes not-same-page updates, though.)
      7403561c
    • Tom Lane's avatar
      Expand the regression tests for PL/Tcl. · 961bed02
      Tom Lane authored
      This raises the test coverage (by line count) in pltcl.c from about 70%
      to 86%.
      
      Karl Lehenbauer and Jim Nasby
      
      Discussion: https://postgr.es/m/92a1670d-21b6-8f03-9c13-e4fb2207ab7b@BlueTreble.com
      961bed02
    • Magnus Hagander's avatar
      Use an enum instead of two bools to indicate wal inclusion in base backups · 534b6f3e
      Magnus Hagander authored
      This makes the code easier to read as it becomes more explicit what the
      different allowed combinations really are.
      
      Suggested by Michael Paquier
      534b6f3e
  7. 07 Jan, 2017 2 commits
    • Tom Lane's avatar
      Get rid of ParseState.p_value_substitute; use a columnref hook instead. · 7c3abe3c
      Tom Lane authored
      I noticed that p_value_substitute, which is a single-purpose kluge I added
      in 2002 (commit b0422b21), could be replaced by having domainAddConstraint
      install a parser hook that looks for the name "value".  The parser hook
      code only dates back to 2009, so it's not surprising that we had to kluge
      this in 2002, but we can do it more cleanly now.
      7c3abe3c
    • Tom Lane's avatar
      Improve documentation of struct ParseState. · 3c40594e
      Tom Lane authored
      I got annoyed about how some fields of ParseState were documented in the
      struct's block comment and some weren't; not all of the latter are trivial.
      Fix that.  Also reorder a couple of fields that seem to have been placed
      rather randomly, or maybe with an idea of avoiding padding space; but there
      are never so many ParseStates in existence at one time that we ought to
      value pad space over readability.
      3c40594e
  8. 06 Jan, 2017 5 commits
    • Stephen Frost's avatar
      Add basic pg_dumpall/pg_restore TAP tests · 9b815a8f
      Stephen Frost authored
      For reasons unknown, pg_dumpall and pg_restore managed to escape the
      basic set of TAP tests that were added for pg_dump in 6bd356c3, so
      let's get them added now.  A few minor adjustments are also made to the
      dump/restore tests to improve code coverage for pg_restore/pg_dumpall.
      9b815a8f
    • Tom Lane's avatar
      Merge two copies of tuple-building code in pltcl.c. · de5fed0d
      Tom Lane authored
      Make pltcl_trigger_handler() construct modified tuples using
      pltcl_build_tuple_result(), rather than its own copy of essentially
      the same logic.  This results in slightly different message wording for
      the error cases, and in one case a different SQLSTATE, but it seems
      unlikely that any existing applications are depending on any of those
      details.
      
      While at it, fix a typo in commit 26abb50c: pltcl_build_tuple_result was
      applying encoding conversion in the wrong direction.  That would be a
      back-patchable bug fix, except the code hasn't shipped yet.
      
      Jim Nasby, reviewed by me
      
      Discussion: https://postgr.es/m/d2c6425a-d9e0-f034-f774-4a872c234d89@BlueTreble.com
      de5fed0d
    • Stephen Frost's avatar
      Protect against NULL-dereference in pg_dump · d74ecbc8
      Stephen Frost authored
      findTableByOid() is allowed to return NULL and we should therefore be
      checking for that case.  getOwnedSeqs() and dumpSequence() shouldn't
      ever actually see this happen, but given odd circumstances it might and
      commit f9e439b1 probably shouldn't have removed that check.
      
      Pointed out by Coverity.  Initial patch from Michael Paquier.
      
      Back-patch to 9.6, where that commit had removed the check.
      d74ecbc8
    • Tom Lane's avatar
      Invalidate cached plans on FDW option changes. · c52d37c8
      Tom Lane authored
      This fixes problems where a plan must change but fails to do so,
      as seen in a bug report from Rajkumar Raghuwanshi.
      
      For ALTER FOREIGN TABLE OPTIONS, do this through the standard method of
      forcing a relcache flush on the table.  For ALTER FOREIGN DATA WRAPPER
      and ALTER SERVER, just flush the whole plan cache on any change in
      pg_foreign_data_wrapper or pg_foreign_server.  That matches the way
      we handle some other low-probability cases such as opclass changes, and
      it's unclear that the case arises often enough to be worth working harder.
      Besides, that gives a patch that is simple enough to back-patch with
      confidence.
      
      Back-patch to 9.3.  In principle we could apply the code change to 9.2 as
      well, but (a) we lack postgres_fdw to test it with, (b) it's doubtful that
      anyone is doing anything exciting enough with FDWs that far back to need
      this desperately, and (c) the patch doesn't apply cleanly.
      
      Patch originally by Amit Langote, reviewed by Etsuro Fujita and Ashutosh
      Bapat, who each contributed substantial changes as well.
      
      Discussion: https://postgr.es/m/CAKcux6m5cA6rRPTKkqVdJ-R=KKDfe35Q_ZuUqxDSV_4hwga=og@mail.gmail.com
      c52d37c8
    • Robert Haas's avatar
      Repair commit b81b5a96. · 0355e6f3
      Robert Haas authored
      This commit purported to use a variable hash seed for Partial
      HashAggregate, but actually did the opposite - it made us use a
      variable seed for any HashAggregate that is NOT partial.  Woops.
      0355e6f3
  9. 05 Jan, 2017 5 commits
    • Robert Haas's avatar
      Fix possible leak of semaphore count. · e5b7451e
      Robert Haas authored
      Commit 4aec4989 reorganized the order
      of operations here so that we no longer increment the number of "extra
      waits" before locking the semaphore, but it did not change the
      starting value of extraWaits from 0 to -1 to compensate.  In the worst
      case, this could leak a semaphore count, but that seems to be unlikely
      in practice.
      
      Discussion: http://postgr.es/m/CAA4eK1JyVqXiMba+-a589Rk0pyHsyKkGxeumVKjU6Y74hdrVLQ@mail.gmail.com
      
      Amit Kapila, per an off-list report by Dilip Kumar.  Reviewed by me.
      e5b7451e
    • Peter Eisentraut's avatar
      Use 'use strict' in all Perl programs · 933b4664
      Peter Eisentraut authored
      933b4664
    • Robert Haas's avatar
      Fix possible crash reading pg_stat_activity. · 175ff659
      Robert Haas authored
      With the old code, a backend that read pg_stat_activity without ever
      having executed a parallel query might see a backend in the midst of
      executing one waiting on a DSA LWLock, resulting in a crash.  The
      solution is for backends to register the tranche at startup time, not
      the first time a parallel query is executed.
      
      Report by Andreas Seltenreich.  Patch by me, reviewed by Thomas Munro.
      175ff659
    • Tom Lane's avatar
      Fix handling of empty arrays in array_fill(). · 82f8107b
      Tom Lane authored
      array_fill(..., array[0]) produced an empty array, which is probably
      what users expect, but it was a one-dimensional zero-length array
      which is not our standard representation of empty arrays.  Also, for
      no very good reason, it rejected empty input arrays; that case should
      be allowed and produce an empty output array.
      
      In passing, remove the restriction that the input array(s) have lower
      bound 1.  That seems rather pointless, and it would have needed extra
      complexity to make the check deal with empty input arrays.
      
      Per bug #14487 from Andrew Gierth.  It's been broken all along, so
      back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/20170105152156.10135.64195@wrigleys.postgresql.org
      82f8107b
    • Simon Riggs's avatar
      Fix format for TAP test docs · 2e44f379
      Simon Riggs authored
      Small number of fixes to perl docs for TAP tests.
      Plus two comments that use "xlog" rather than WAL
      
      Michael Paquier
      2e44f379
  10. 04 Jan, 2017 2 commits