1. 15 Apr, 2021 8 commits
    • Tom Lane's avatar
      Undo decision to allow pg_proc.prosrc to be NULL. · 1111b266
      Tom Lane authored
      Commit e717a9a1 changed the longstanding rule that prosrc is NOT NULL
      because when a SQL-language function is written in SQL-standard style,
      we don't currently have anything useful to put there.  This seems a poor
      decision though, as it could easily have negative impacts on external
      PLs (opening them to crashes they didn't use to have, for instance).
      SQL-function-related code can just as easily test "is prosqlbody not
      null" as "is prosrc null", so there's no real gain there either.
      Hence, revert the NOT NULL marking removal and adjust related logic.
      
      For now, we just put an empty string into prosrc for SQL-standard
      functions.  Maybe we'll have a better idea later, although the
      history of things like pg_attrdef.adsrc suggests that it's not
      easy to maintain a string equivalent of a node tree.
      
      This also adds an assertion that queryDesc->sourceText != NULL
      to standard_ExecutorStart.  We'd been silently relying on that
      for awhile, so let's make it less silent.
      
      Also fix some overlooked documentation and test cases.
      
      Discussion: https://postgr.es/m/2197698.1617984583@sss.pgh.pa.us
      1111b266
    • Tom Lane's avatar
      Stabilize recently-added information_schema test queries. · 3157cbe9
      Tom Lane authored
      These queries could show unexpected entries if the core system,
      or concurrently-running test scripts, created any functions that
      would appear in the information_schema views.  Restrict them
      to showing functions belonging to this test's schema, as the
      far-older nearby test case does.
      
      Per experimentation with conversion of some built-in functions
      to SQL-function-body style.
      3157cbe9
    • Peter Eisentraut's avatar
      Revert "psql: Show all query results by default" · fae65629
      Peter Eisentraut authored
      This reverts commit 3a513067.
      
      Per discussion, this patch had too many issues to resolve at this
      point of the development cycle.  We'll try again in the future.
      
      Discussion: https://www.postgresql.org/message-id/flat/alpine.DEB.2.21.1904132231510.8961@lancre
      fae65629
    • Fujii Masao's avatar
      doc: Add missing COMPRESSION into CREATE TABLE synopsis. · e2e2efca
      Fujii Masao authored
      Commit bbe0a81d introduced "INCLUDING COMPRESSION" option
      in CREATE TABLE command, but forgot to mention it in the
      CREATE TABLE syntax synopsis.
      
      Author: Fujii Masao
      Reviewed-by: Michael Paquier
      Discussion: https://postgr.es/m/54d30e66-dbd6-5485-aaf6-a291ed55919d@oss.nttdata.com
      e2e2efca
    • Michael Paquier's avatar
      doc: Simplify example of HISTFILE for psql · 1840d9f4
      Michael Paquier authored
      e4c76191 has added a space to the example used for HISTFILE in the docs
      of psql before the variable DBNAME, as a workaround because variables
      were not parsed the same way back then.  This behavior has changed in
      9.2, causing the example in the psql docs to result in the same history
      file created with or without a space added before the DBNAME variable.
      
      Let's just remove this space in the example, to reduce any confusion, as
      the point of it is to prove that a per-database history file is easy to
      set up, and that's easier to read this way.
      
      Per discussion with Tom Lane.
      
      Reported-by: Ludovic Kuty
      Discussion: https://postgr.es/m/161830067409.691.16198363670687811485@wrigleys.postgresql.org
      1840d9f4
    • Peter Eisentraut's avatar
    • Peter Eisentraut's avatar
      amcheck: Use correct format placeholder for TOAST chunk numbers · 59da8d9e
      Peter Eisentraut authored
      Several of these were already fixed in passing in
      9acaf1a6, but one was remaining
      inconsistent.
      59da8d9e
    • Michael Paquier's avatar
      Tweak behavior of pg_dump --extension with configuration tables · 344487e2
      Michael Paquier authored
      6568cef2, that introduced the option, had an inconsistent behavior when
      it comes to configuration tables set up by pg_extension_config_dump, as
      the data of all configuration tables would included in a dump even for
      extensions not listed by a set of --extension switches.
      
      The contents dumped changed depending on the schema where an extension
      was installed when an extension was not listed.  For example, an
      extension installed under the public schema would have its configuration
      data not dumped even when not listed with --extension, which was
      inconsistent with the case of an extension installed on a non-public
      schema, where the configuration would be dumped.
      
      Per discussion with Noah, we have settled down to the simple rule of
      dumping configuration data of an extension if it is listed in
      --extension (default is unchanged and backward-compatible, to dump
      everything on sight if there are no extensions directly listed).  This
      avoids some weird cases where the dumps depended on a --schema for one.
      
      More tests are added to cover the gap, where we cross-check more
      behaviors depending on --schema when an extension is not listed.
      
      Reported-by: Noah Misch
      Reviewed-by: Noah Misch
      Discussion: https://postgr.es/m/20210404220802.GA728316@rfd.leadboat.com
      344487e2
  2. 14 Apr, 2021 6 commits
  3. 13 Apr, 2021 12 commits
    • Tomas Vondra's avatar
      Initialize t_self and t_tableOid in statext_expressions_load · 20661c15
      Tomas Vondra authored
      The function is building a fake heap tuple, but left some of the header
      fields (tid and table OID) uninitialized. Per Coverity report.
      
      Reported-by: Ranier Vilela
      Discussion: https://postgr.es/m/CAEudQApj6h8tZ0-eP5Af5PKc5NG1YUc7=SdN_99YoHS51fKa0Q@mail.gmail.com
      20661c15
    • Peter Geoghegan's avatar
      Don't truncate heap when VACUUM's failsafe is in effect. · 60f1f09f
      Peter Geoghegan authored
      It seems like a good idea to bypass heap truncation when the wraparound
      failsafe mechanism (which was added in commit 1e55e7d1) is in effect.
      
      Deliberately don't bypass heap truncation in the INDEX_CLEANUP=off case,
      even though it is similar to the failsafe case.  There is already a
      separate reloption (and related VACUUM parameter) for that.
      Reported-By: default avatarMasahiko Sawada <sawada.mshk@gmail.com>
      Discussion: https://postgr.es/m/CAD21AoDWRh6oTN5T8wa+cpZUVpHXET8BJ8Da7WHVHpwkPP6KLg@mail.gmail.com
      60f1f09f
    • Tom Lane's avatar
      Allow table-qualified variable names in ON CONFLICT ... WHERE. · 6c0373ab
      Tom Lane authored
      Previously you could only use unqualified variable names here.
      While that's not a functional deficiency, since only the target
      table can be referenced, it's a surprising inconsistency with the
      rules for partial-index predicates, on which this syntax is
      supposedly modeled.
      
      The fix for that is no harder than passing addToRelNameSpace = true
      to addNSItemToQuery.  However, it's really pretty bogus for
      transformOnConflictArbiter and transformOnConflictClause to be
      messing with the namespace item for the target table at all.
      It's not theirs to manage, it results in duplicative creations of
      namespace items, and transformOnConflictClause wasn't even doing
      it quite correctly (that coding resulted in two nsitems for the
      target table, since it hadn't cleaned out the existing one).
      Hence, make transformInsertStmt responsible for setting up the
      target nsitem once for both these clauses and RETURNING.
      
      Also, arrange for ON CONFLICT ... UPDATE's "excluded" pseudo-relation
      to be added to the rangetable before we run transformOnConflictArbiter.
      This produces a more helpful HINT if someone writes "excluded.col"
      in the arbiter expression.
      
      Per bug #16958 from Lukas Eder.  Although I agree this is a bug,
      the consequences are hardly severe, so no back-patch.
      
      Discussion: https://postgr.es/m/16958-963f638020de271c@postgresql.org
      6c0373ab
    • Robert Haas's avatar
      docs: Update TOAST storage docs for configurable compression. · e8c435a8
      Robert Haas authored
      Mention that there are multiple TOAST compression methods and that the
      compression method used is stored in a TOAST pointer along with the
      other information that was stored there previously. Add a reference to
      the documentation for default_toast_compression, where the supported
      methods are listed, instead of duplicating that here.
      
      I haven't tried to preserve the text claiming that pglz is "fairly
      simple and very fast." I have no view on the veracity of the former
      claim, but LZ4 seems to be faster (and to compress better) so it
      seems better not to muddy the waters by talking about compression
      speed as a strong point of PGLZ.
      
      Patch by me, reviewed by Justin Pryzby.
      
      Discussion: http://postgr.es/m/CA+Tgmoaw_YBwQhOS_hhEPPwFhfAnu+VCLs18EfGr9gQw1z4H-w@mail.gmail.com
      e8c435a8
    • Tom Lane's avatar
      Fix some inappropriately-disallowed uses of ALTER ROLE/DATABASE SET. · 69d5ca48
      Tom Lane authored
      Most GUC check hooks that inspect database state have special checks
      that prevent them from throwing hard errors for state-dependent issues
      when source == PGC_S_TEST.  This allows, for example,
      "ALTER DATABASE d SET default_text_search_config = foo" when the "foo"
      configuration hasn't been created yet.  Without this, we have problems
      during dump/reload or pg_upgrade, because pg_dump has no idea about
      possible dependencies of GUC values and can't ensure a safe restore
      ordering.
      
      However, check_role() and check_session_authorization() hadn't gotten
      the memo about that, and would throw hard errors anyway.  It's not
      entirely clear what is the use-case for "ALTER ROLE x SET role = y",
      but we've now heard two independent complaints about that bollixing
      an upgrade, so apparently some people are doing it.
      
      Hence, fix these two functions to act more like other check hooks
      with similar needs.  (But I did not change their insistence on
      being inside a transaction, as it's still not apparent that setting
      either GUC from the configuration file would be wise.)
      
      Also fix check_temp_buffers, which had a different form of the disease
      of making state-dependent checks without any exception for PGC_S_TEST.
      A cursory survey of other GUC check hooks did not find any more issues
      of this ilk.  (There are a lot of interdependencies among
      PGC_POSTMASTER and PGC_SIGHUP GUCs, which may be a bad idea, but
      they're not relevant to the immediate concern because they can't be
      set via ALTER ROLE/DATABASE.)
      
      Per reports from Charlie Hornsby and Nathan Bossart.  Back-patch
      to all supported branches.
      
      Discussion: https://postgr.es/m/HE1P189MB0523B31598B0C772C908088DB7709@HE1P189MB0523.EURP189.PROD.OUTLOOK.COM
      Discussion: https://postgr.es/m/20160711223641.1426.86096@wrigleys.postgresql.org
      69d5ca48
    • Tom Lane's avatar
      Redesign the caching done by get_cached_rowtype(). · c2db458c
      Tom Lane authored
      Previously, get_cached_rowtype() cached a pointer to a reference-counted
      tuple descriptor from the typcache, relying on the ExprContextCallback
      mechanism to release the tupdesc refcount when the expression tree
      using the tupdesc was destroyed.  This worked fine when it was designed,
      but the introduction of within-DO-block COMMITs broke it.  The refcount
      is logged in a transaction-lifespan resource owner, but plpgsql won't
      destroy simple expressions made within the DO block (before its first
      commit) until the DO block is exited.  That results in a warning about
      a leaked tupdesc refcount when the COMMIT destroys the original resource
      owner, and then an error about the active resource owner not holding a
      matching refcount when the expression is destroyed.
      
      To fix, get rid of the need to have a shutdown callback at all, by
      instead caching a pointer to the relevant typcache entry.  Those
      survive for the life of the backend, so we needn't worry about the
      pointer becoming stale.  (For registered RECORD types, we can still
      cache a pointer to the tupdesc, knowing that it won't change for the
      life of the backend.)  This mechanism has been in use in plpgsql
      and expandedrecord.c since commit 4b93f579, and seems to work well.
      
      This change requires modifying the ExprEvalStep structs used by the
      relevant expression step types, which is slightly worrisome for
      back-patching.  However, there seems no good reason for extensions
      to be familiar with the details of these particular sub-structs.
      
      Per report from Rohit Bhogate.  Back-patch to v11 where within-DO-block
      COMMITs became a thing.
      
      Discussion: https://postgr.es/m/CAAV6ZkQRCVBh8qAY+SZiHnz+U+FqAGBBDaDTjF2yiKa2nJSLKg@mail.gmail.com
      c2db458c
    • Tom Lane's avatar
      Avoid improbable PANIC during heap_update. · 34f581c3
      Tom Lane authored
      heap_update needs to clear any existing "all visible" flag on
      the old tuple's page (and on the new page too, if different).
      Per coding rules, to do this it must acquire pin on the appropriate
      visibility-map page while not holding exclusive buffer lock;
      which creates a race condition since someone else could set the
      flag whenever we're not holding the buffer lock.  The code is
      supposed to handle that by re-checking the flag after acquiring
      buffer lock and retrying if it became set.  However, one code
      path through heap_update itself, as well as one in its subroutine
      RelationGetBufferForTuple, failed to do this.  The end result,
      in the unlikely event that a concurrent VACUUM did set the flag
      while we're transiently not holding lock, is a non-recurring
      "PANIC: wrong buffer passed to visibilitymap_clear" failure.
      
      This has been seen a few times in the buildfarm since recent VACUUM
      changes that added code paths that could set the all-visible flag
      while holding only exclusive buffer lock.  Previously, the flag
      was (usually?) set only after doing LockBufferForCleanup, which
      would insist on buffer pin count zero, thus preventing the flag
      from becoming set partway through heap_update.  However, it's
      clear that it's heap_update not VACUUM that's at fault here.
      
      What's less clear is whether there is any hazard from these bugs
      in released branches.  heap_update is certainly violating API
      expectations, but if there is no code path that can set all-visible
      without a cleanup lock then it's only a latent bug.  That's not
      100% certain though, besides which we should worry about extensions
      or future back-patch fixes that could introduce such code paths.
      
      I chose to back-patch to v12.  Fixing RelationGetBufferForTuple
      before that would require also back-patching portions of older
      fixes (notably 0d1fe9f7), which is more code churn than seems
      prudent to fix a hypothetical issue.
      
      Discussion: https://postgr.es/m/2247102.1618008027@sss.pgh.pa.us
      34f581c3
    • Fujii Masao's avatar
      5fe83ada
    • Noah Misch's avatar
      Use "-I." in directories holding Bison parsers, for Oracle compilers. · 455dbc01
      Noah Misch authored
      With the Oracle Developer Studio 12.6 compiler, #line directives alter
      the current source file location for purposes of #include "..."
      directives.  Hence, a VPATH build failed with 'cannot find include file:
      "specscanner.c"'.  With two exceptions, parser-containing directories
      already add "-I. -I$(srcdir)"; eliminate the exceptions.  Back-patch to
      9.6 (all supported versions).
      455dbc01
    • Noah Misch's avatar
      Port regress-python3-mangle.mk to Solaris "sed". · c3556f6f
      Noah Misch authored
      It doesn't support "\(foo\)*" like a POSIX "sed" implementation does;
      see the Autoconf manual.  Back-patch to 9.6 (all supported versions).
      c3556f6f
    • Thomas Munro's avatar
      Fix potential SSI hazard in heap_update(). · b1df6b69
      Thomas Munro authored
      Commit 6f38d4da failed to heed a warning about the stability of the
      value pointed to by "otid".  The caller is allowed to pass in a pointer to
      newtup->t_self, which will be updated during the execution of the
      function.  Instead, the SSI check should use the value we copy into
      oldtup.t_self near the top of the function.
      
      Not a live bug, because newtup->t_self doesn't really get updated until
      a bit later, but it was confusing and broke the rule established by the
      comment.
      
      Back-patch to 13.
      Reported-by: default avatarTom Lane <tgl@sss.pgh.pa.us>
      Discussion: https://postgr.es/m/2689164.1618160085%40sss.pgh.pa.us
      b1df6b69
    • Michael Paquier's avatar
      Remove duplicated --no-sync switches in new tests of test_pg_dump · 885a8764
      Michael Paquier authored
      These got introduced in 6568cef2.
      
      Reported-by: Noah Misch
      Discussion: https://postgr.es/m/20210404220802.GA728316@rfd.leadboat.com
      885a8764
  4. 12 Apr, 2021 9 commits
  5. 11 Apr, 2021 5 commits
    • Tom Lane's avatar
      Silence some Coverity warnings and improve code consistency. · 6277435a
      Tom Lane authored
      Coverity complained about possible overflow in expressions like
      	intresult = tm->tm_sec * 1000000 + fsec;
      on the grounds that the multiplication would happen in 32-bit
      arithmetic before widening to the int64 result.  I think these
      are all false positives because of the limited possible range of
      tm_sec; but nonetheless it seems silly to spell it like that when
      nearby lines have the identical computation written with a 64-bit
      constant.
      
      ... or more accurately, with an LL constant, which is not project
      style.  Make all of these use INT64CONST(), as we do elsewhere.
      
      This is all new code from a2da77cd, so no need for back-patch.
      6277435a
    • Tom Lane's avatar
      Add macro PGWARNING, and make PGERROR available on all platforms. · d7cff12c
      Tom Lane authored
      We'd previously noted the need for coping with Windows headers
      that provide some other definition of macro "ERROR" than elog.h
      does.  It turns out that R also wants to define ERROR, and
      WARNING too.  PL/R has been working around this in a hacky way
      that broke when we recently changed the numeric value of ERROR.
      To let them have a more future-proof solution, provide an
      alternate macro PGWARNING for WARNING, and make PGERROR visible
      always, not only when #ifdef WIN32.
      
      Discussion: https://postgr.es/m/CADK3HHK6iMChd1yoOqssxBn5Z14Zar8Ztr3G-N_fuG7F8YTP3w@mail.gmail.com
      d7cff12c
    • Tom Lane's avatar
      Fix uninitialized variable from commit a4d75c86. · 9cb92334
      Tom Lane authored
      The path for *exprs != NIL would misbehave, and likely crash,
      since pull_varattnos expects its last argument to be valid
      at call.
      
      Found by Coverity --- we have no coverage of this path in
      the regression tests.
      9cb92334
    • Fujii Masao's avatar
      Avoid unnecessary table open/close in TRUNCATE command. · 81a23dd8
      Fujii Masao authored
      ExecuteTruncate() filters out the duplicate tables specified
      in the TRUNCATE command, for example in the case where "TRUNCATE foo, foo"
      is executed. Such duplicate tables obviously don't need to be opened
      and closed because they are skipped. But previously it always opened
      the tables before checking whether they were duplicated ones or not,
      and then closed them if they were. That is, the duplicated tables were
      opened and closed unnecessarily.
      
      This commit changes ExecuteTruncate() so that it opens the table
      after it confirms that table is not duplicated one, which leads to
      avoid unnecessary table open/close.
      
      Do not back-patch because such unnecessary table open/close is not
      a bug though it exists in older versions.
      
      Author: Bharath Rupireddy
      Reviewed-by: Amul Sul, Fujii Masao
      Discussion: https://postgr.es/m/CALj2ACUdBO_sXJTa08OZ0YT0qk7F_gAmRa9hT4dxRcgPS4nsZA@mail.gmail.com
      81a23dd8
    • Fujii Masao's avatar
      Remove COMMIT_TS_SETTS record. · 08aa89b3
      Fujii Masao authored
      Commit 438fc4a3 prevented the WAL replay from writing
      COMMIT_TS_SETTS record. By this change there is no code that
      generates COMMIT_TS_SETTS record in PostgreSQL core.
      Also we can think that there are no extensions using the record
      because we've not received so far any complaints about the issue
      that commit 438fc4a3 fixed. Therefore this commit removes
      COMMIT_TS_SETTS record and its related code. Even without
      this record, the timestamp required for commit timestamp feature
      can be acquired from the COMMIT record.
      
      Bump WAL page magic.
      Reported-by: default avatarlx zou <zoulx1982@163.com>
      Author: Fujii Masao
      Reviewed-by: Alvaro Herrera
      Discussion: https://postgr.es/m/16931-620d0f2fdc6108f1@postgresql.org
      08aa89b3