1. 24 Nov, 2019 7 commits
    • Andrew Dunstan's avatar
      Use native methods to open input in TestLib::slurp_file on Windows. · 114541d5
      Andrew Dunstan authored
      It is hoped that this will avoid some errors that we have seen before,
      but lately with greater frequency, in buildfarm animals.
      
      For now apply only to master. If this proves effective it can be
      backpatched.
      
      Discussion: https://postgr.es/m/13900.1572839580@sss.pgh.pa.us
      
      Author: Juan José Santamaría Flecha
      114541d5
    • Tom Lane's avatar
      Doc: improve discussion of race conditions involved in LISTEN. · d3aa114a
      Tom Lane authored
      The user docs didn't really explain how to use LISTEN safely,
      so clarify that.  Also clean up some fuzzy-headed explanations
      in comments.  No code changes.
      
      Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com
      d3aa114a
    • Tom Lane's avatar
      Avoid assertion failure with LISTEN in a serializable transaction. · 6b802cfc
      Tom Lane authored
      If LISTEN is the only action in a serializable-mode transaction,
      and the session was not previously listening, and the notify queue
      is not empty, predicate.c reported an assertion failure.  That
      happened because we'd acquire the transaction's initial snapshot
      during PreCommit_Notify, which was called *after* predicate.c
      expects any such snapshot to have been established.
      
      To fix, just swap the order of the PreCommit_Notify and
      PreCommit_CheckForSerializationFailure calls during CommitTransaction.
      This will imply holding the notify-insertion lock slightly longer,
      but the difference could only be meaningful in serializable mode,
      which is an expensive option anyway.
      
      It appears that this is just an assertion failure, with no
      consequences in non-assert builds.  A snapshot used only to scan
      the notify queue could not have been involved in any serialization
      conflicts, so there would be nothing for
      PreCommit_CheckForSerializationFailure to do except assign it a
      prepareSeqNo and set the SXACT_FLAG_PREPARED flag.  And given no
      conflicts, neither of those omissions affect the behavior of
      ReleasePredicateLocks.  This admittedly once-over-lightly analysis
      is backed up by the lack of field reports of trouble.
      
      Per report from Mark Dilger.  The bug is old, so back-patch to all
      supported branches; but the new test case only goes back to 9.6,
      for lack of adequate isolationtester infrastructure before that.
      
      Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com
      Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
      6b802cfc
    • Thomas Munro's avatar
      doc: Fix whitespace in syntax. · 1974853d
      Thomas Munro authored
      Back-patch to 10.
      
      Author: Andreas Karlsson
      Discussion: https://postgr.es/m/043acae2-a369-b7fa-be48-1933aa2e82d1%40proxel.se
      1974853d
    • Tom Lane's avatar
      Stabilize NOTIFY behavior by transmitting notifies before ReadyForQuery. · 79002697
      Tom Lane authored
      This patch ensures that, if any notify messages were received during
      a just-finished transaction, they get sent to the frontend just before
      not just after the ReadyForQuery message.  With libpq and other client
      libraries that act similarly, this guarantees that the client will see
      the notify messages as available as soon as it thinks the transaction
      is done.
      
      This probably makes no difference in practice, since in realistic
      use-cases the application would have to cope with asynchronous
      arrival of notify events anyhow.  However, it makes it a lot easier
      to build cross-session-notify test cases with stable behavior.
      I'm a bit surprised now that we've not seen any buildfarm instability
      with the test cases added by commit b10f40bf.  Tests that I intend
      to add in an upcoming bug fix are definitely unstable without this.
      
      Back-patch to 9.6, which is as far back as we can do NOTIFY testing
      with the isolationtester infrastructure.
      
      Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
      79002697
    • Tom Lane's avatar
      Stabilize the results of pg_notification_queue_usage(). · 8b7ae5a8
      Tom Lane authored
      This function wasn't touched in commit 51004c71, but that turns out
      to be a bad idea, because its results now include any dead space
      that exists in the NOTIFY queue on account of our being lazy about
      advancing the queue tail.  Notably, the isolation tests now fail
      if run twice without a server restart between, because async-notify's
      first test of the function will already show a positive value.
      It seems likely that end users would be equally unhappy about the
      result's instability.  To fix, just make the function call
      asyncQueueAdvanceTail before computing its result.  That should end
      in producing the same value as before, and it's hard to believe that
      there's any practical use-case where pg_notification_queue_usage()
      is called so often as to create a performance degradation, especially
      compared to what we did before.
      
      Out of paranoia, also mark this function parallel-restricted (it
      was volatile, but parallel-safe by default, before).  Although the
      code seems to work fine when run in a parallel worker, that's outside
      the design scope of async.c, and it's a bit scary to have intentional
      side-effects happening in a parallel worker.  There seems no plausible
      use-case where it'd be important to try to parallelize this, so let's
      not take any risk of introducing new bugs.
      
      In passing, re-pgindent async.c and run reformat-dat-files on
      pg_proc.dat, just because I'm a neatnik.
      
      Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us
      8b7ae5a8
    • Tom Lane's avatar
      Remove a couple of unnecessary if-tests. · 91da65f4
      Tom Lane authored
      Commit abd9ca37 replaced a couple of while-loops in fmtfloat()
      with calls to dopr_outchmulti, but I (tgl) failed to notice that
      the new if-tests guarding those calls were really unnecessary,
      because they're inside a larger if-block checking the same thing.
      
      Ranier Vilela
      
      Discussion: https://postgr.es/m/MN2PR18MB2927850AB00CF39CC370D107E34B0@MN2PR18MB2927.namprd18.prod.outlook.com
      91da65f4
  2. 23 Nov, 2019 3 commits
  3. 22 Nov, 2019 6 commits
    • Tom Lane's avatar
      Make psql redisplay the query buffer after \e. · d1c866e5
      Tom Lane authored
      Up to now, whatever you'd edited was put back into the query buffer
      but not redisplayed, which is less than user-friendly.  But we can
      improve that just by printing the text along with a prompt, if we
      enforce that the editing result ends with a newline (which it
      typically would anyway).  You then continue typing more lines if
      you want, or you can type ";" or do \g or \r or another \e.
      
      This is intentionally divorced from readline's processing,
      for simplicity and so that it works the same with or without
      readline enabled.  We discussed possibly integrating things
      more closely with readline; but that seems difficult, uncertainly
      portable across different readline and libedit versions, and
      of limited real benefit anyway.  Let's try the simple way and
      see if it's good enough.
      
      Patch by me, thanks to Fabien Coelho and Laurenz Albe for review
      
      Discussion: https://postgr.es/m/13192.1572318028@sss.pgh.pa.us
      d1c866e5
    • Tom Lane's avatar
      Avoid taking a new snapshot for an immutable simple expression in plpgsql. · 73b06cf8
      Tom Lane authored
      We already used this optimization if the plpgsql function is read-only.
      But it seems okay to do it even in a read-write function, if the
      expression contains only immutable functions/operators.  There would
      only be a change of behavior if an "immutable" called function depends
      on seeing database updates made during the current plpgsql function.
      That's enough of a violation of the promise of immutability that anyone
      who complains won't have much of a case.
      
      The benefits are significant --- for simple cases like
      
        while i < 10000000
        loop
          i := i + 1;
        end loop;
      
      I see net performance improvements around 45%.  Of course, real-world
      cases won't get that much faster, but it ought to be noticeable.
      At the very least, this removes much of the performance penalty that
      used to exist for forgetting to mark a plpgsql function non-volatile.
      
      Konstantin Knizhnik, reviewed by Pavel Stehule, cosmetic changes by me
      
      Discussion: https://postgr.es/m/ed9da20e-01aa-d04b-d085-e6c16b14b9d7@postgrespro.ru
      73b06cf8
    • Tom Lane's avatar
      Add test coverage for "unchanged toast column" replication code path. · f67b1737
      Tom Lane authored
      It seems pretty unacceptable to have no regression test coverage
      for this aspect of the logical replication protocol, especially
      given the bugs we've found in related code.
      
      Discussion: https://postgr.es/m/16129-a0c0f48e71741e5f@postgresql.org
      f67b1737
    • Tom Lane's avatar
      Fix bogus tuple-slot management in logical replication UPDATE handling. · 4d9ceb00
      Tom Lane authored
      slot_modify_cstrings seriously abused the TupleTableSlot API by relying
      on a slot's underlying data to stay valid across ExecClearTuple.  Since
      this abuse was also quite undocumented, it's little surprise that the
      case got broken during the v12 slot rewrites.  As reported in bug #16129
      from Ondřej Jirman, this could lead to crashes or data corruption when
      a logical replication subscriber processes a row update.  Problems would
      only arise if the subscriber's table contained columns of pass-by-ref
      types that were not being copied from the publisher.
      
      Fix by explicitly copying the datum/isnull arrays from the source slot
      that the old row was in already.  This ends up being about the same
      thing that happened pre-v12, but hopefully in a less opaque and
      fragile way.
      
      We might've caught the problem sooner if there were any test cases
      dealing with updates involving non-replicated or dropped columns.
      Now there are.
      
      Back-patch to v10 where this code came in.  Even though the failure
      does not manifest before v12, IMO this code is too fragile to leave
      as-is.  In any case we certainly want the additional test coverage.
      
      Patch by me; thanks to Tomas Vondra for initial investigation.
      
      Discussion: https://postgr.es/m/16129-a0c0f48e71741e5f@postgresql.org
      4d9ceb00
    • Michael Paquier's avatar
      Add .gitignore to src/tutorial/ · 8959a5e0
      Michael Paquier authored
      A set of SQL files are generated for the tutorial when compiling the
      code, so let's avoid any risk to have them added in the tree in the
      future.
      8959a5e0
    • Michael Paquier's avatar
      Remove traces of version-0 calling convention in src/tutorial/ · a9d5157a
      Michael Paquier authored
      Support has been removed as of 5ded4bd2, but code related to the tutorial
      still used it.  Functions using version-1 are already present for some
      time in the tutorial, and the documentation mentions them, so just
      replace the old version with the new one.
      
      Reported-by: Pavel Stehule
      Analyzed-by: Euler Taveira
      Author: Michael Paquier
      Reviewed-by: Tom Lane, Pavel Stehule
      Discussion: https://postgr.es/m/CAFj8pRCgC2uDzrw-vvanXu6Z3ofyviEOQPEpH6_aL4OCe7JRag@mail.gmail.com
      a9d5157a
  4. 21 Nov, 2019 8 commits
  5. 20 Nov, 2019 9 commits
  6. 19 Nov, 2019 7 commits
    • Tom Lane's avatar
      Fix corner-case failure in match_pattern_prefix(). · b3c265d7
      Tom Lane authored
      The planner's optimization code for LIKE and regex operators could
      error out with a complaint like "no = operator for opfamily NNN"
      if someone created a binary-compatible index (for example, a
      bpchar_ops index on a text column) on the LIKE's left argument.
      
      This is a consequence of careless refactoring in commit 74dfe58a.
      The old code in match_special_index_operator only accepted specific
      combinations of the pattern operator and the index opclass, thereby
      indirectly guaranteeing that the opclass would have a comparison
      operator with the same LHS input type as the pattern operator.
      While moving the logic out to a planner support function, I simplified
      that test in a way that no longer guarantees that.  Really though we'd
      like an altogether weaker dependency on the opclass, so rather than
      put back exactly the old code, just allow lookup failure.  I have in
      mind now to rewrite this logic completely, but this is the minimum
      change needed to fix the bug in v12.
      
      Per report from Manuel Rigger.  Back-patch to v12 where the mistake
      came in.
      
      Discussion: https://postgr.es/m/CA+u7OA7nnGYy8rY0vdTe811NuA+Frr9nbcBO9u2Z+JxqNaud+g@mail.gmail.com
      b3c265d7
    • Alexander Korotkov's avatar
      Fix page modification outside of critical section in GIN · b1071408
      Alexander Korotkov authored
      By oversight 52ac6cd2 makes ginDeletePage() sets pd_prune_xid of page to be
      deleted before entering the critical section.  It appears that only versions 11
      and later were affected by this oversight.
      
      Backpatch-through: 11
      b1071408
    • Alexander Korotkov's avatar
      Revise GIN README · 32ca32d0
      Alexander Korotkov authored
      We find GIN concurrency bugs from time to time.  One of the problems here is
      that concurrency of GIN isn't well-documented in README.  So, it might be even
      hard to distinguish design bugs from implementation bugs.
      
      This commit revised concurrency section in GIN README providing more details.
      Some examples are illustrated in ASCII art.
      
      Also, this commit add the explanation of how is tuple layout in internal GIN
      B-tree page different in comparison with nbtree.
      
      Discussion: https://postgr.es/m/CAPpHfduXR_ywyaVN4%2BOYEGaw%3DcPLzWX6RxYLBncKw8de9vOkqw%40mail.gmail.com
      Author: Alexander Korotkov
      Reviewed-by: Peter Geoghegan
      Backpatch-through: 9.4
      32ca32d0
    • Alexander Korotkov's avatar
      Fix traversing to the deleted GIN page via downlink · d5ad7a09
      Alexander Korotkov authored
      Current GIN code appears to don't handle traversing to the deleted page via
      downlink.  This commit fixes that by stepping right from the delete page like
      we do in nbtree.
      
      This commit also fixes setting 'deleted' flag to the GIN pages.  Now other page
      flags are not erased once page is deleted.  That helps to keep our assertions
      true if we arrive deleted page via downlink.
      
      Discussion: https://postgr.es/m/CAPpHfdvMvsw-NcE5bRS7R1BbvA4BxoDnVVjkXC5W0Czvy9LVrg%40mail.gmail.com
      Author: Alexander Korotkov
      Reviewed-by: Peter Geoghegan
      Backpatch-through: 9.4
      d5ad7a09
    • Alexander Korotkov's avatar
      Fix deadlock between ginDeletePage() and ginStepRight() · e1464119
      Alexander Korotkov authored
      When ginDeletePage() is about to delete page it locks its left sibling to revise
      the rightlink.  So, it locks pages in right to left manner.  Int he same time
      ginStepRight() locks pages in left to right manner, and that could cause a
      deadlock.
      
      This commit makes ginScanToDelete() keep exclusive lock on left siblings of
      currently investigated path.  That elimites need to relock left sibling in
      ginDeletePage().  Thus, deadlock with ginStepRight() can't happen anymore.
      
      Reported-by: Chen Huajun
      Discussion: https://postgr.es/m/5c332bd1.87b6.16d7c17aa98.Coremail.chjischj%40163.com
      Author: Alexander Korotkov
      Reviewed-by: Peter Geoghegan
      Backpatch-through: 10
      e1464119
    • Tom Lane's avatar
      Doc: clarify use of RECURSIVE in WITH. · 5b805886
      Tom Lane authored
      Apparently some people misinterpreted the syntax as being that
      RECURSIVE is a prefix of individual WITH queries.  It's a modifier
      for the WITH clause as a whole, so state that more clearly.
      
      Discussion: https://postgr.es/m/ca53c6ce-a0c6-b14a-a8e3-162f0b2cc119@a-kretschmer.de
      5b805886
    • Tom Lane's avatar
      Doc: clarify behavior of ALTER DEFAULT PRIVILEGES ... IN SCHEMA. · 787b3fd3
      Tom Lane authored
      The existing text stated that "Default privileges that are specified
      per-schema are added to whatever the global default privileges are for
      the particular object type".  However, that bare-bones observation is
      not quite clear enough, as demonstrated by the complaint in bug #16124.
      Flesh it out by stating explicitly that you can't revoke built-in
      default privileges this way, and by providing an example to drive
      the point home.
      
      Back-patch to all supported branches, since it's been like this
      from the beginning.
      
      Discussion: https://postgr.es/m/16124-423d8ee4358421bc@postgresql.org
      787b3fd3