1. 05 Oct, 2019 5 commits
  2. 04 Oct, 2019 10 commits
    • Andres Freund's avatar
      Add isolation tests for the combination of EPQ and triggers. · c8841199
      Andres Freund authored
      As evidenced by bug #16036 this area is woefully under-tested. Add
      fairly extensive tests for the combination.
      
      Backpatch back to 9.6 - before that isolationtester was not capable
      enough. While we don't backpatch tests all the time, future fixes to
      trigger.c would potentially look different enough in 12+ from the
      earlier branches that introducing bugs during backpatching is more
      likely than normal. Also, it's just a crucial and undertested area of
      the code.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/16036-28184c90d952fb7f@postgresql.org
      Backpatch: 9.6-, the earliest these tests work
      c8841199
    • Andres Freund's avatar
      Fix crash caused by EPQ happening with a before update trigger present. · d986d4e8
      Andres Freund authored
      When ExecBRUpdateTriggers()'s GetTupleForTrigger() follows an EPQ
      chain the former needs to run the result tuple through the junkfilter
      again, and update the slot containing the new version of the tuple to
      contain that new version. The input tuple may already be in the
      junkfilter's output slot, which used to be OK - we don't need the
      previous version anymore. Unfortunately ff11e7f4 started to use
      ExecCopySlot() to update newslot, and ExecCopySlot() doesn't support
      copying a slot into itself, leading to a slot in a corrupt
      state, which then can cause crashes or other symptoms.
      
      Fix this by skipping the ExecCopySlot() when copying into itself.
      
      While we could have easily made ExecCopySlot() handle that case, it
      seems better to add an assert forbidding doing so instead. As the goal
      of copying might be to make the contents of one slot independent from
      another, it seems failure prone to handle doing so silently.
      
      A follow-up commit will add tests for the obviously under-covered
      combination of EPQ and triggers. Done as a separate commit as it might
      make sense to backpatch them further than this bug.
      
      Also remove confusion with confusing variable names for slots in
      ExecBRDeleteTriggers() and ExecBRUpdateTriggers().
      
      Bug: #16036
      Reported-By: Антон Власов
      Author: Andres Freund
      Discussion: https://postgr.es/m/16036-28184c90d952fb7f@postgresql.org
      Backpatch: 12-, where ff11e7f4 was merged
      d986d4e8
    • Andres Freund's avatar
      Use a fd opened for read/write when syncing slots during startup, take 2. · a586cc4b
      Andres Freund authored
      Cribbing from dfbaed45:
          Some operating systems, including the reporter's windows, return EBADFD
          or similar when fsync() is invoked on a O_RDONLY file descriptor.
          Unfortunately RestoreSlotFromDisk() does exactly that; which causes
          failures after restarts in at least some scenarios.
      
          If you hit the bug the error message will be something like
          ERROR: could not fsync file "pg_replslot/$name/state": Bad file descriptor
      
          Simply use O_RDWR instead of O_RDONLY when opening the relevant file
          descriptor to fix the bug.
      
      Unfortunately this fix was undone in 82a5649f. Re-apply, and add a
      comment.
      
      Bug: 16039
      Reported-By: Hans Buschmann
      Author: Andres Freund
      Discussion: https://postgr.es/m/16039-196fc97cc05e141c@postgresql.org
      Backpatch: 12-, as 82a5649f
      a586cc4b
    • Andrew Dunstan's avatar
      Handle spaces in OpenSSL install location for MSVC · ad7595b8
      Andrew Dunstan authored
      First, make sure that the .exe name is quoted when trying to get the
      version number. Also, don't quote the lib name for using in the project
      files if it's already been quoted. This second change applies to all
      libraries, not just OpenSSL.
      
      This has clearly been broken forever, so backpatch to all live branches.
      ad7595b8
    • Robert Haas's avatar
      Rename some toasting functions based on whether they are heap-specific. · 2e8b6bfa
      Robert Haas authored
      The old names for the attribute-detoasting functions names included
      the word "heap," which seems outdated now that the heap is only one of
      potentially many table access methods.
      
      On the other hand, toast_insert_or_update and toast_delete are
      heap-specific, so rename them by adding "heap_" as a prefix.
      
      Not all of the work of making the TOAST system fully accessible to AMs
      other than the heap is done yet, but there seems to be little harm in
      getting this renaming out of the way now. Commit
      8b94dab0 already divided up the
      functions among various files partially according to whether it was
      intended that they should be heap-specific or AM-agnostic, so this is
      just clarifying the division contemplated by that commit.
      
      Patch by me, reviewed and tested by Prabhat Sabu, Thomas Munro,
      Andres Freund, and Álvaro Herrera.
      
      Discussion: http://postgr.es/m/CA+TgmoZv-=2iWM4jcw5ZhJeL18HF96+W1yJeYrnGMYdkFFnEpQ@mail.gmail.com
      2e8b6bfa
    • Tom Lane's avatar
      Fix bitshiftright()'s zero-padding some more. · 61aa9f54
      Tom Lane authored
      Commit 5ac0d936 failed to entirely fix bitshiftright's habit of
      leaving one-bits in the pad space that should be all zeroes,
      because in a moment of sheer brain fade I'd concluded that only
      the code path used for not-a-multiple-of-8 shift distances needed
      to be fixed.  Of course, a multiple-of-8 shift distance can also
      cause the problem, so we need to forcibly zero the extra bits
      in both cases.
      
      Per bug #16037 from Alexander Lakhin.  As before, back-patch to all
      supported branches.
      
      Discussion: https://postgr.es/m/16037-1d1ebca564db54f4@postgresql.org
      61aa9f54
    • Tomas Vondra's avatar
      Use Size instead of int64 to track allocated memory · f2369bc6
      Tomas Vondra authored
      Commit 5dd7fc15 added block-level memory accounting, but used int64 variable to
      track the amount of allocated memory. That is incorrect, because we have Size for
      exactly these purposes, but it was mostly harmless until c477f3e4 which changed
      how we handle with repalloc() when downsizing the chunk. Previously we've ignored
      these cases and just kept using the original chunk, but now we need to update the
      accounting, and the code was doing this:
      
          context->mem_allocated += blksize - oldblksize;
      
      Both blksize and oldblksize are Size (so unsigned) which means the subtraction
      underflows, producing a very high positive value. On 64-bit platforms (where Size
      has the same size as mem_alllocated) this happens to work because the result wraps
      to the right value, but on (some) 32-bit platforms this fails.
      
      This fixes two things - it changes mem_allocated (and related variables) to Size,
      and it splits the update to two separate steps, to prevent any underflows.
      
      Discussion: https://www.postgresql.org/message-id/15151.1570163761%40sss.pgh.pa.us
      f2369bc6
    • Robert Haas's avatar
      Remove AtSubStart_Notify. · 967e276e
      Robert Haas authored
      Allocate notify-related state lazily instead. This makes trivial
      subtransactions noticeably faster.
      
      Patch by me, reviewed and tested by Dilip Kumar, Kyotaro Horiguchi,
      and Jeevan Ladhe.
      
      Discussion: https://postgr.es/m/CA+TgmobE1J22S1eC-6N-je9LgrcwZypkwp+zH6JXo9mc=4Nk3A@mail.gmail.com
      967e276e
    • Michael Paquier's avatar
      Fix issues in pg_rewind with --no-ensure-shutdown/--write-recovery-conf · 6837632b
      Michael Paquier authored
      This fixes two issues with recent features added in pg_rewind:
      - --dry-run should do nothing on the target directory, but 927474ce
      forgot to consider that for --write-recovery-conf.
      - --no-ensure-shutdown was not actually working.  There is no test
      coverage for this option yet, but a subsequent patch will add that.
      
      Author: Alexey Kondratov
      Discussion: https://postgr.es/m/7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru
      6837632b
    • Michael Paquier's avatar
      Fix --dry-run mode of pg_rewind · 6f3823b0
      Michael Paquier authored
      Even if --dry-run mode was specified, the control file was getting
      updated, preventing follow-up runs of pg_rewind to work properly on the
      target data folder.  The origin of the problem came from the refactoring
      done by ce6afc68.
      
      Author: Alexey Kondratov
      Discussion: https://postgr.es/m/7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru
      Backpatch-through: 12
      6f3823b0
  3. 03 Oct, 2019 4 commits
    • Tom Lane's avatar
      Avoid unnecessary out-of-memory errors during encoding conversion. · 8e10405c
      Tom Lane authored
      Encoding conversion uses the very simplistic rule that the output
      can't be more than 4X longer than the input, and palloc's a buffer
      of that size.  This results in failure to convert any string longer
      than 1/4 GB, which is becoming an annoying limitation.
      
      As a band-aid to improve matters, allow the allocated output buffer
      size to exceed 1GB.  We still insist that the final result fit into
      MaxAllocSize (1GB), though.  Perhaps it'd be safe to relax that
      restriction, but it'd require close analysis of all callers, which
      is daunting (not least because external modules might call these
      functions).  For the moment, this should allow a 2X to 4X improvement
      in the longest string we can convert, which is a useful gain in
      return for quite a simple patch.
      
      Also, once we have successfully converted a long string, repalloc
      the output down to the actual string length, returning the excess
      to the malloc pool.  This seems worth doing since we can usually
      expect to give back several MB if we take this path at all.
      
      This still leaves much to be desired, most notably that the assumption
      that MAX_CONVERSION_GROWTH == 4 is very fragile, and yet we have no
      guard code verifying that the output buffer isn't overrun.  Fixing
      that would require significant changes in the encoding conversion
      APIs, so it'll have to wait for some other day.
      
      The present patch seems safely back-patchable, so patch all supported
      branches.
      
      Alvaro Herrera and Tom Lane
      
      Discussion: https://postgr.es/m/20190816181418.GA898@alvherre.pgsql
      Discussion: https://postgr.es/m/3614.1569359690@sss.pgh.pa.us
      8e10405c
    • Tom Lane's avatar
      Allow repalloc() to give back space when a large chunk is downsized. · c477f3e4
      Tom Lane authored
      Up to now, if you resized a large (>8K) palloc chunk down to a smaller
      size, aset.c made no attempt to return any space to the malloc pool.
      That's unpleasant if a really large allocation is resized to a
      significantly smaller size.  I think no such cases existed when this
      code was designed, and I'm not sure whether they're common even yet,
      but an upcoming fix to encoding conversion will certainly create such
      cases.  Therefore, fix AllocSetRealloc so that it gives realloc()
      a chance to do something with the block.  This doesn't noticeably
      increase complexity, we mostly just have to change the order in which
      the cases are considered.
      
      Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/20190816181418.GA898@alvherre.pgsql
      Discussion: https://postgr.es/m/3614.1569359690@sss.pgh.pa.us
      c477f3e4
    • Andrew Gierth's avatar
      Selectively include window frames in expression walks/mutates. · b7a1c553
      Andrew Gierth authored
      query_tree_walker and query_tree_mutator were skipping the
      windowClause of the query, without regard for the fact that the
      startOffset and endOffset in a WindowClause node are expression trees
      that need to be processed. This was an oversight in commit ec4be2ee
      from 2010 which added the expression fields; the main symptom is that
      function parameters in window frame clauses don't work in inlined
      functions.
      
      Fix (as conservatively as possible since this needs to not break
      existing out-of-tree callers) and add tests.
      
      Backpatch all the way, since this has been broken since 9.0.
      
      Per report from Alastair McKinley; fix by me with kibitzing and review
      from Tom Lane.
      
      Discussion: https://postgr.es/m/DB6PR0202MB2904E7FDDA9D81504D1E8C68E3800@DB6PR0202MB2904.eurprd02.prod.outlook.com
      b7a1c553
    • Amit Kapila's avatar
      pgbench: add --partitions and --partition-method options. · b1c1aa53
      Amit Kapila authored
      These new options allow users to partition the pgbench_accounts table by
      specifying the number of partitions and partitioning method.  The values
      allowed for partitioning method are range and hash.
      
      This feature allows users to measure the overhead of partitioning if any.
      
      Author: Fabien COELHO
      Reviewed-by: Amit Kapila, Amit Langote, Dilip Kumar, Asif Rehman, and
      Alvaro Herrera
      Discussion: https://postgr.es/m/alpine.DEB.2.21.1907230826190.7008@lancre
      b1c1aa53
  4. 02 Oct, 2019 2 commits
  5. 01 Oct, 2019 7 commits
    • Tomas Vondra's avatar
      Blind attempt to fix pglz_maximum_compressed_size · 540f3168
      Tomas Vondra authored
      Commit 11a078cf triggered failures on big-endian machines, and the
      only plausible place for an issue seems to be that TOAST_COMPRESS_SIZE
      calls VARSIZE instead of VARSIZE_ANY. So try fixing that blindly.
      
      Discussion: https://www.postgresql.org/message-id/20191001131803.j6uin7nho7t6vxzy%40development
      540f3168
    • Tomas Vondra's avatar
      Mark two variables in in aset.c with PG_USED_FOR_ASSERTS_ONLY · fa2fe04b
      Tomas Vondra authored
      This fixes two compiler warnings about unused variables in non-assert builds,
      introduced by 5dd7fc15.
      fa2fe04b
    • Tomas Vondra's avatar
      Optimize partial TOAST decompression · 11a078cf
      Tomas Vondra authored
      Commit 4d0e994e added support for partial TOAST decompression, so the
      decompression is interrupted after producing the requested prefix. For
      prefix and slices near the beginning of the entry, this may saves a lot
      of decompression work.
      
      That however only deals with decompression - the whole compressed entry
      was still fetched and re-assembled, even though the compression used
      only a small fraction of it. This commit improves that by computing how
      much compressed data may be needed to decompress the requested prefix,
      and then fetches only the necessary part.
      
      We always need to fetch a bit more compressed data than the requested
      (uncompressed) prefix, because the prefix may not be compressible at all
      and pglz itself adds a bit of overhead. That means this optimization is
      most effective when the requested prefix is much smaller than the whole
      compressed entry.
      
      Author: Binguo Bao
      Reviewed-by: Andrey Borodin, Tomas Vondra, Paul Ramsey
      Discussion: https://www.postgresql.org/message-id/flat/CAL-OGkthU9Gs7TZchf5OWaL-Gsi=hXqufTxKv9qpNG73d5na_g@mail.gmail.com
      11a078cf
    • Michael Paquier's avatar
      Fix test_session_hooks with parallel workers · 002962dc
      Michael Paquier authored
      Several buildfarm machines have been complaining about the new module
      test_session_hooks to be unstable, like crake and thorntail.  The issue
      was that the module was trying to log some start and end session
      activity for parallel workers, which makes little sense as they don't
      support DML, so just prevent this pattern to happen in the module.
      
      This could be reproduced by enforcing force_parallel_mode=regress, which
      is the value used by some of the buildfarm members.
      
      Discussion: https://postgr.es/m/20191001045246.GF2781@paquier.xyz
      002962dc
    • Michael Paquier's avatar
      Add hooks for session start and session end, take two · e788bd92
      Michael Paquier authored
      These hooks can be used in loadable modules.  A simple test module is
      included.
      
      The first attempt was done with cd8ce3a2 but we lacked handling for
      NO_INSTALLCHECK in the MSVC scripts (problem solved afterwards by
      431f1599) so the buildfarm got angry.  This also fixes a couple of
      issues noticed upon review compared to the first attempt, so the code
      has slightly changed, resulting in a more simple test module.
      
      Author: Fabrízio de Royes Mello, Yugo Nagata
      Reviewed-by: Andrew Dunstan, Michael Paquier, Aleksandr Parfenov
      Discussion: https://postgr.es/m/20170720204733.40f2b7eb.nagata@sraoss.co.jp
      Discussion: https://postgr.es/m/20190823042602.GB5275@paquier.xyz
      e788bd92
    • Michael Paquier's avatar
      Fix confusing error caused by connection parameter channel_binding · 41a6de41
      Michael Paquier authored
      When using a client compiled without channel binding support (linking to
      OpenSSL 1.0.1 or older) to connect to a server which supports channel
      binding (linking to OpenSSL 1.0.2 or newer), libpq would generate a
      confusing error message with channel_binding=require for an SSL
      connection, where the server sends back SCRAM-SHA-256-PLUS:
      "channel binding is required, but server did not offer an authentication
      method that supports channel binding."
      
      This is confusing because the server did send a SASL mechanism able to
      support channel binding, but libpq was not able to detect that
      properly.
      
      The situation can be summarized as followed for the case described in
      the previous paragraph for the SASL mechanisms used with the various
      modes of channel_binding:
      1) Client supports channel binding.
      1-1) channel_binding = disable => OK, with SCRAM-SHA-256.
      1-2) channel_binding = prefer => OK, with SCRAM-SHA-256-PLUS.
      1-3) channel_binding = require => OK, with SCRAM-SHA-256-PLUS.
      2) Client does not support channel binding.
      2-1) channel_binding = disable => OK, with SCRAM-SHA-256.
      2-2) channel_binding = prefer => OK, with SCRAM-SHA-256.
      2-3) channel_binding = require => failure with new error message,
      instead of the confusing one.
      This commit updates case 2-3 to generate a better error message.  Note
      that the SSL TAP tests are not impacted as it is not possible to test
      with mixed versions of OpenSSL for the backend and libpq.
      
      Reported-by: Tom Lane
      Author: Michael Paquier
      Reviewed-by: Jeff Davis, Tom Lane
      Discussion: https://postgr.es/m/24857.1569775891@sss.pgh.pa.us
      41a6de41
    • Tomas Vondra's avatar
      Add transparent block-level memory accounting · 5dd7fc15
      Tomas Vondra authored
      Adds accounting of memory allocated in a memory context. Compared to
      various ad hoc solutions, the main advantage is that the accounting is
      transparent and does not require direct control over allocations (this
      matters for use cases where the allocations happen in user code, like
      for example aggregate states allocated in a transition functions).
      
      To reduce overhead, the accounting happens at the block level (not for
      individual chunks) and only the context immediately owning the block is
      updated. When inquiring about amount of memory allocated in a context,
      we have to recursively walk all children contexts.
      
      This "lazy" accounting works well for cases with relatively small number
      of contexts in the relevant subtree and/or with infrequent inquiries.
      
      Author: Jeff Davis
      Reivewed-by: Tomas Vondra, Melanie Plageman, Soumyadeep Chakraborty
      Discussion: https://www.postgresql.org/message-id/flat/027a129b8525601c6a680d27ce3a7172dab61aab.camel@j-davis.com
      5dd7fc15
  6. 30 Sep, 2019 11 commits
  7. 29 Sep, 2019 1 commit
    • Andres Freund's avatar
      jit: Re-allow JIT compilation of execGrouping.c hashtable comparisons. · ac88807f
      Andres Freund authored
      In the course of 5567d12c, 356687bd and 317ffdfe, I changed
      BuildTupleHashTable[Ext]'s call to ExecBuildGroupingEqual to not pass
      in the parent node, but NULL. Which in turn prevents the tuple
      equality comparator from being JIT compiled.  While that fixes
      bug #15486, it is not actually necessary after all of the above commits,
      as we don't re-build the comparator when using the new
      BuildTupleHashTableExt() interface (as the content of the hashtable
      are reset, but the TupleHashTable itself is not).
      
      Therefore re-allow jit compilation for callers that use
      BuildTupleHashTableExt with a separate context for "metadata" and
      content.
      
      As in the previous commit, there's ongoing work to make this easier to
      test to prevent such regressions in the future, but that
      infrastructure is not going to be backpatchable.
      
      The performance impact of not JIT compiling hashtable equality
      comparators can be substantial e.g. for aggregation queries that
      aggregate a lot of input rows to few output rows (when there are a lot
      of output groups, there will be fewer comparisons).
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20190927072053.njf6prdl3vb7y7qb@alap3.anarazel.de
      Backpatch: 11, just as 5567d12c
      ac88807f