1. 14 Apr, 2022 2 commits
    • Robert Haas's avatar
      Rethink the delay-checkpoint-end mechanism in the back-branches. · 10520f43
      Robert Haas authored
      The back-patch of commit bbace569 had
      the unfortunate effect of changing the layout of PGPROC in the
      back-branches, which could break extensions. This happened because it
      changed the delayChkpt from type bool to type int. So, change it back,
      and add a new bool delayChkptEnd field instead. The new field should
      fall within what used to be padding space within the struct, and so
      hopefully won't cause any extensions to break.
      
      Per report from Markus Wanner and discussion with Tom Lane and others.
      
      Patch originally by me, somewhat revised by Markus Wanner per a
      suggestion from Michael Paquier. A very similar patch was developed
      by Kyotaro Horiguchi, but I failed to see the email in which that was
      posted before writing one of my own.
      
      Discussion: http://postgr.es/m/CA+Tgmoao-kUD9c5nG5sub3F7tbo39+cdr8jKaOVEs_1aBWcJ3Q@mail.gmail.com
      Discussion: http://postgr.es/m/20220406.164521.17171257901083417.horikyota.ntt@gmail.com
      10520f43
    • Michael Paquier's avatar
      pageinspect: Fix handling of all-zero pages · df6bbe73
      Michael Paquier authored
      Getting from get_raw_page() an all-zero page is considered as a valid
      case by the buffer manager and it can happen for example when finding a
      corrupted page with zero_damaged_pages enabled (using zero_damaged_pages
      to look at corrupted pages happens), or after a crash when a relation
      file is extended before any WAL for its new data is generated (before a
      vacuum or autovacuum job comes in to do some cleanup).
      
      However, all the functions of pageinspect, as of the index AMs (except
      hash that has its own idea of new pages), heap, the FSM or the page
      header have never worked with all-zero pages, causing various crashes
      when going through the page internals.
      
      This commit changes all the pageinspect functions to be compliant with
      all-zero pages, where the choice is made to return NULL or no rows for
      SRFs when finding a new page.  get_raw_page() still works the same way,
      returning a batch of zeros in the bytea of the page retrieved.  A hard
      error could be used but NULL, while more invasive, is useful when
      scanning relation files in full to get a batch of results for a single
      relation in one query.  Tests are added for all the code paths
      impacted.
      
      Reported-by: Daria Lepikhova
      Author: Michael Paquier
      Discussion: https://postgr.es/m/561e187b-3549-c8d5-03f5-525c14e65bd0@postgrespro.ru
      Backpatch-through: 10
      df6bbe73
  2. 13 Apr, 2022 2 commits
    • Tom Lane's avatar
      Prevent access to no-longer-pinned buffer in heapam_tuple_lock(). · c590e514
      Tom Lane authored
      heap_fetch() used to have a "keep_buf" parameter that told it to return
      ownership of the buffer pin to the caller after finding that the
      requested tuple TID exists but is invisible to the specified snapshot.
      This was thoughtlessly removed in commit 5db6df0c, which broke
      heapam_tuple_lock() (formerly EvalPlanQualFetch) because that function
      needs to do more accesses to the tuple even if it's invisible.  The net
      effect is that we would continue to touch the page for a microsecond or
      two after releasing pin on the buffer.  Usually no harm would result;
      but if a different session decided to defragment the page concurrently,
      we could see garbage data and mistakenly conclude that there's no newer
      tuple version to chain up to.  (It's hard to say whether this has
      happened in the field.  The bug was actually found thanks to a later
      change that allowed valgrind to detect accesses to non-pinned buffers.)
      
      The most reasonable way to fix this is to reintroduce keep_buf,
      although I made it behave slightly differently: buffer ownership
      is passed back only if there is a valid tuple at the requested TID.
      In HEAD, we can just add the parameter back to heap_fetch().
      To avoid an API break in the back branches, introduce an additional
      function heap_fetch_extended() in those branches.
      
      In HEAD there is an additional, less obvious API change: tuple->t_data
      will be set to NULL in all cases where buffer ownership is not returned,
      in particular when the tuple exists but fails the time qual (and
      !keep_buf).  This is to defend against any other callers attempting to
      access non-pinned buffers.  We concluded that making that change in back
      branches would be more likely to introduce problems than cure any.
      
      In passing, remove a comment about heap_fetch that was obsoleted by
      9a8ee1dc.
      
      Per bug #17462 from Daniil Anisimov.  Back-patch to v12 where the bug
      was introduced.
      
      Discussion: https://postgr.es/m/17462-9c98a0f00df9bd36@postgresql.org
      c590e514
    • David Rowley's avatar
      Docs: wording improvement for compute_query_id = regress · ea669b80
      David Rowley authored
      It's more accurate to say that the query identifier is not shown when
      compute_query_id = regress rather than to say it is hidden.
      
      This change (ebf6c5249) appeared in v14, so it makes sense to backpatch
      this small adjustment to keep the documents consistent between v14 and
      master.
      
      Author: Justin Pryzby
      Discussion: https://postgr.es/m/20220411020336.GB26620@telsasoft.com
      Backpatch-through: 14, where compute_query_id = regress was added
      ea669b80
  3. 12 Apr, 2022 3 commits
  4. 06 Apr, 2022 2 commits
  5. 05 Apr, 2022 1 commit
  6. 02 Apr, 2022 2 commits
  7. 01 Apr, 2022 1 commit
  8. 31 Mar, 2022 3 commits
  9. 30 Mar, 2022 1 commit
  10. 29 Mar, 2022 1 commit
  11. 28 Mar, 2022 3 commits
  12. 27 Mar, 2022 2 commits
    • Tom Lane's avatar
      Fix breakage of get_ps_display() in the PS_USE_NONE case. · 3f7a59c5
      Tom Lane authored
      Commit 8c6d30f2 caused this function to fail to set *displen
      in the PS_USE_NONE code path.  If the variable's previous value
      had been negative, that'd lead to a memory clobber at some call
      sites.  We'd managed not to notice due to very thin test coverage
      of such configurations, but this appears to explain buildfarm member
      lorikeet's recent struggles.
      
      Credit to Andrew Dunstan for spotting the problem.  Back-patch
      to v13 where the bug was introduced.
      
      Discussion: https://postgr.es/m/136102.1648320427@sss.pgh.pa.us
      3f7a59c5
    • Michael Paquier's avatar
      pageinspect: Add more sanity checks to prevent out-of-bound reads · 27d38444
      Michael Paquier authored
      A couple of code paths use the special area on the page passed by the
      function caller, expecting to find some data in it.  However, feeding
      an incorrect page can lead to out-of-bound reads when trying to access
      the page special area (like a heap page that has no special area,
      leading PageGetSpecialPointer() to grab a pointer outside the allocated
      page).
      
      The functions used for hash and btree indexes have some protection
      already against that, while some other functions using a relation OID
      as argument would make sure that the access method involved is correct,
      but functions taking in input a raw page without knowing the relation
      the page is attached to would run into problems.
      
      This commit improves the set of checks used in the code paths of BRIN,
      btree (including one check if a leaf page is found with a non-zero
      level), GIN and GiST to verify that the page given in input has a
      special area size that fits with each access method, which is done
      though PageGetSpecialSize(), becore calling PageGetSpecialPointer().
      
      The scope of the checks done is limited to work with pages that one
      would pass after getting a block with get_raw_page(), as it is possible
      to craft byteas that could bypass existing code paths.  Having too many
      checks would also impact the usability of pageinspect, as the existing
      code is very useful to look at the content details in a corrupted page,
      so the focus is really to avoid out-of-bound reads as this is never a
      good thing even with functions whose execution is limited to
      superusers.
      
      The safest approach could be to rework the functions so as these fetch a
      block using a relation OID and a block number, but there are also cases
      where using a raw page is useful.
      
      Tests are added to cover all the code paths that needed such checks, and
      an error message for hash indexes is reworded to fit better with what
      this commit adds.
      
      Reported-By: Alexander Lakhin
      Author: Julien Rouhaud, Michael Paquier
      Discussion: https://postgr.es/m/16527-ef7606186f0610a1@postgresql.org
      Discussion: https://postgr.es/m/561e187b-3549-c8d5-03f5-525c14e65bd0@postgrespro.ru
      Backpatch-through: 10
      27d38444
  13. 26 Mar, 2022 1 commit
    • Tom Lane's avatar
      Suppress compiler warning in relptr_store(). · 0144c9c7
      Tom Lane authored
      clang 13 with -Wextra warns that "performing pointer subtraction with
      a null pointer has undefined behavior" in the places where freepage.c
      tries to set a relptr variable to constant NULL.  This appears to be
      a compiler bug, but it's unlikely to get fixed instantly.  Fortunately,
      we can work around it by introducing an inline support function, which
      seems like a good change anyway because it removes the macro's existing
      double-evaluation hazard.
      
      Backpatch to v10 where this code was introduced.
      
      Patch by me, based on an idea of Andres Freund's.
      
      Discussion: https://postgr.es/m/48826.1648310694@sss.pgh.pa.us
      0144c9c7
  14. 25 Mar, 2022 2 commits
    • Tom Lane's avatar
      Harden TAP tests that intentionally corrupt page checksums. · 579cef5f
      Tom Lane authored
      The previous method for doing that was to write zeroes into a
      predetermined set of page locations.  However, there's a roughly
      1-in-64K chance that the existing checksum will match by chance,
      and yesterday several buildfarm animals started to reproducibly
      see that, resulting in test failures because no checksum mismatch
      was reported.
      
      Since the checksum includes the page LSN, test success depends on
      the length of the installation's WAL history, which is affected by
      (at least) the initial catalog contents, the set of locales installed
      on the system, and the length of the pathname of the test directory.
      Sooner or later we were going to hit a chance match, and today is
      that day.
      
      Harden these tests by specifically inverting the checksum field and
      leaving all else alone, thereby guaranteeing that the checksum is
      incorrect.
      
      In passing, fix places that were using seek() to set up for syswrite(),
      a combination that the Perl docs very explicitly warn against.  We've
      probably escaped problems because no regular buffered I/O is done on
      these filehandles; but if it ever breaks, we wouldn't deserve or get
      much sympathy.
      
      Although we've only seen problems in HEAD, now that we recognize the
      environmental dependencies it seems like it might be just a matter
      of time until someone manages to hit this in back-branch testing.
      Hence, back-patch to v11 where we started doing this kind of test.
      
      Discussion: https://postgr.es/m/3192026.1648185780@sss.pgh.pa.us
      579cef5f
    • Alvaro Herrera's avatar
      Fix replay of create database records on standby · ffd28516
      Alvaro Herrera authored
      Crash recovery on standby may encounter missing directories when
      replaying create database WAL records.  Prior to this patch, the standby
      would fail to recover in such a case.  However, the directories could be
      legitimately missing.  Consider a sequence of WAL records as follows:
      
          CREATE DATABASE
          DROP DATABASE
          DROP TABLESPACE
      
      If, after replaying the last WAL record and removing the tablespace
      directory, the standby crashes and has to replay the create database
      record again, the crash recovery must be able to move on.
      
      This patch adds a mechanism similar to invalid-page tracking, to keep a
      tally of missing directories during crash recovery.  If all the missing
      directory references are matched with corresponding drop records at the
      end of crash recovery, the standby can safely continue following the
      primary.
      
      Backpatch to 13, at least for now.  The bug is older, but fixing it in
      older branches requires more careful study of the interactions with
      commit e6d80695, which appeared in 13.
      
      A new TAP test file is added to verify the condition.  However, because
      it depends on commit d6d317dbf615, it can only be added to branch
      master.  I (Álvaro) manually verified that the code behaves as expected
      in branch 14.  It's a bit nervous-making to leave the code uncovered by
      tests in older branches, but leaving the bug unfixed is even worse.
      Also, the main reason this fix took so long is precisely that we
      couldn't agree on a good strategy to approach testing for the bug, so
      perhaps this is the best we can do.
      Diagnosed-by: default avatarPaul Guo <paulguo@gmail.com>
      Author: Paul Guo <paulguo@gmail.com>
      Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
      Author: Asim R Praveen <apraveen@pivotal.io>
      Discussion: https://postgr.es/m/CAEET0ZGx9AvioViLf7nbR_8tH9-=27DN5xWJ2P9-ROH16e4JUA@mail.gmail.com
      ffd28516
  15. 24 Mar, 2022 1 commit
  16. 23 Mar, 2022 6 commits
  17. 22 Mar, 2022 2 commits
    • Andres Freund's avatar
      Add missing dependency of pg_dumpall to WIN32RES. · 2d608c96
      Andres Freund authored
      When cross-building to windows, or building with mingw on windows, the build
      could fail with
        x86_64-w64-mingw32-gcc: error: win32ver.o: No such file or director
      because pg_dumpall didn't depend on WIN32RES, but it's recipe references
      it. The build nevertheless succeeded most of the time, due to
      pg_dump/pg_restore having the required dependency, causing win32ver.o to be
      built.
      Reported-By: default avatarThomas Munro <thomas.munro@gmail.com>
      Discussion: https://postgr.es/m/CA+hUKGJeekpUPWW6yCVdf9=oBAcCp86RrBivo4Y4cwazAzGPng@mail.gmail.com
      Backpatch: 10-, omission present on all live branches
      2d608c96
    • Michael Paquier's avatar
      Fix failures in SSL tests caused by out-of-tree keys and certificates · fdb1be49
      Michael Paquier authored
      This issue is environment-sensitive, where the SSL tests could fail in
      various way by feeding on defaults provided by sslcert, sslkey,
      sslrootkey, sslrootcert, sslcrl and sslcrldir coming from a local setup,
      as of ~/.postgresql/ by default.  Horiguchi-san has reported two
      failures, but more advanced testing from me (aka inclusion of garbage
      SSL configuration in ~/.postgresql/ for all the configuration
      parameters) has showed dozens of failures that can be triggered in the
      whole test suite.
      
      History has showed that we are not good when it comes to address such
      issues, fixing them locally like in dd877998, and such problems keep
      appearing.  This commit strengthens the entire test suite to put an end
      to this set of problems by embedding invalid default values in all the
      connection strings used in the tests.  The invalid values are prefixed
      in each connection string, relying on the follow-up values passed in the
      connection string to enforce any invalid value previously set.  Note
      that two tests related to CRLs are required to fail with certain pre-set
      configurations, but we can rely on enforcing an empty value instead
      after the invalid set of values.
      
      Reported-by: Kyotaro Horiguchi
      Reviewed-by: Andrew Dunstan, Daniel Gustafsson, Kyotaro Horiguchi
      Discussion: https://postgr.es/m/20220316.163658.1122740600489097632.horikyota.ntt@gmail.com
      backpatch-through: 10
      fdb1be49
  18. 21 Mar, 2022 2 commits
    • Tom Lane's avatar
      Fix assorted missing logic for GroupingFunc nodes. · 48b6035f
      Tom Lane authored
      The planner needs to treat GroupingFunc like Aggref for many purposes,
      in particular with respect to processing of the argument expressions,
      which are not to be evaluated at runtime.  A few places hadn't gotten
      that memo, notably including subselect.c's processing of outer-level
      aggregates.  This resulted in assertion failures or wrong plans for
      cases in which a GROUPING() construct references an outer aggregation
      level.
      
      Also fix missing special cases for GroupingFunc in cost_qual_eval
      (resulting in wrong cost estimates for GROUPING(), although it's
      not clear that that would affect plan shapes in practice) and in
      ruleutils.c (resulting in excess parentheses in pretty-print mode).
      
      Per bug #17088 from Yaoguang Chen.  Back-patch to all supported
      branches.
      
      Richard Guo, Tom Lane
      
      Discussion: https://postgr.es/m/17088-e33882b387de7f5c@postgresql.org
      48b6035f
    • Tom Lane's avatar
      Fix risk of deadlock failure while dropping a partitioned index. · 05ccf974
      Tom Lane authored
      DROP INDEX needs to lock the index's table before the index itself,
      else it will deadlock against ordinary queries that acquire the
      relation locks in that order.  This is correctly mechanized for
      plain indexes by RangeVarCallbackForDropRelation; but in the case of
      a partitioned index, we neglected to lock the child tables in advance
      of locking the child indexes.  We can fix that by traversing the
      inheritance tree and acquiring the needed locks in RemoveRelations,
      after we have acquired our locks on the parent partitioned table and
      index.
      
      While at it, do some refactoring to eliminate confusion between
      the actual and expected relkind in RangeVarCallbackForDropRelation.
      We can save a couple of syscache lookups too, by having that function
      pass back info that RemoveRelations will need.
      
      Back-patch to v11 where partitioned indexes were added.
      
      Jimmy Yih, Gaurab Dey, Tom Lane
      
      Discussion: https://postgr.es/m/BYAPR05MB645402330042E17D91A70C12BD5F9@BYAPR05MB6454.namprd05.prod.outlook.com
      05ccf974
  19. 20 Mar, 2022 1 commit
  20. 19 Mar, 2022 1 commit
  21. 18 Mar, 2022 1 commit