1. 16 Apr, 2021 10 commits
  2. 15 Apr, 2021 10 commits
  3. 14 Apr, 2021 6 commits
  4. 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
  5. 12 Apr, 2021 2 commits
    • Tom Lane's avatar
      Remove no-longer-relevant test case. · cf002008
      Tom Lane authored
      collate.icu.utf8.sql was exercising the recording of a collation
      dependency for an enum comparison expression, but such an expression
      should never have had any collation dependency in the first place.
      After I fixed that in commit c402b02b, the test started failing.
      We don't need to test that scenario anymore, so just remove the
      now-useless test steps.
      
      (This test case is new in HEAD, so no need to back-patch.)
      
      Discussion: https://postgr.es/m/3044030.1618261159@sss.pgh.pa.us
      Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com
      cf002008
    • Tom Lane's avatar
      Fix old bug with coercing the result of a COLLATE expression. · c402b02b
      Tom Lane authored
      There are hacks in parse_coerce.c to push down a requested coercion
      to below any CollateExpr that may appear.  However, we did that even
      if the requested data type is non-collatable, leading to an invalid
      expression tree in which CollateExpr is applied to a non-collatable
      type.  The fix is just to drop the CollateExpr altogether, reasoning
      that it's useless.
      
      This bug is ten years old, dating to the original addition of
      COLLATE support.  The lack of field complaints suggests that there
      aren't a lot of user-visible consequences.  We noticed the problem
      because it would trigger an assertion in DefineVirtualRelation if
      the invalid structure appears as an output column of a view; however,
      in a non-assert build, you don't see a crash just a (subtly incorrect)
      complaint about applying collation to a non-collatable type.  I found
      that by putting the incorrect structure further down in a view, I could
      make a view definition that would fail dump/reload, per the added
      regression test case.  But CollateExpr doesn't do anything at run-time,
      so this likely doesn't lead to any really exciting consequences.
      
      Per report from Yulin Pei.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com
      c402b02b