1. 15 Aug, 2019 3 commits
    • Tom Lane's avatar
      Fix plpgsql to re-look-up composite type names at need. · fe9b7b2f
      Tom Lane authored
      Commit 4b93f579 rearranged things in plpgsql to make it cope better with
      composite types changing underneath it intra-session.  However, I failed to
      consider the case of a composite type being dropped and recreated entirely.
      In my defense, the previous coding didn't consider that possibility at all
      either --- but it would accidentally work so long as you didn't change the
      type's field list, because the built-at-compile-time list of component
      variables would then still match the type's new definition.  The new
      coding, however, occasionally tries to re-look-up the type by OID, and
      then fails to find the dropped type.
      
      To fix this, we need to save the TypeName struct, and then redo the type
      OID lookup from that.  Of course that's expensive, so we don't want to do
      it every time we need the type OID.  This can be fixed in the same way that
      4b93f579 dealt with changes to composite types' definitions: keep an eye
      on the type's typcache entry to see if its tupledesc has been invalidated.
      (Perhaps, at some point, this mechanism should be generalized so it can
      work for non-composite types too; but for now, plpgsql only tries to
      cope with intra-session redefinitions of composites.)
      
      I'm slightly hesitant to back-patch this into v11, because it changes
      the contents of struct PLpgSQL_type as well as the signature of
      plpgsql_build_datatype(), so in principle it could break code that is
      poking into the innards of plpgsql.  However, the only popular extension
      of that ilk is pldebugger, and it doesn't seem to be affected.  Since
      this is a regression for people who were relying on the old behavior,
      it seems worth taking the small risk of causing compatibility issues.
      
      Per bug #15913 from Daniel Fiori.  Back-patch to v11 where 4b93f579
      came in.
      
      Discussion: https://postgr.es/m/15913-a7e112e16dedcffc@postgresql.org
      fe9b7b2f
    • Tom Lane's avatar
      Use a hash table to de-duplicate NOTIFY events faster. · bb5ae8f6
      Tom Lane authored
      Previously, async.c got rid of duplicate notifications by scanning
      the list of pending events to compare each one to the proposed new
      event.  This works okay for very small numbers of distinct events,
      but degrades as O(N^2) for many events.  We can improve matters by
      using a hash table to probe for duplicates.  So as not to add a
      lot of overhead for the simple cases that the code did handle well
      before, create the hash table only once a (sub)transaction has
      queued more than 16 distinct notify events.
      
      A downside is that we now have to do per-event work to propagate
      a successful subtransaction's notify events up to its parent.
      (But this isn't significant unless the subtransaction had many
      events, in which case the O(N^2) behavior would have been in
      play already, so we still come out ahead.)
      
      We can make some lemonade out of this lemon, though: since we must
      examine each event anyway, it's now possible to de-duplicate events
      fully, rather than skipping that for events merged up from
      subtransactions.  Hence, remove the old weasel wording in notify.sgml
      about whether de-duplication happens or not, and adjust the test
      case in async-notify.spec that exhibited the old behavior.
      
      While at it, rearrange the definition of struct Notification to make
      it more compact and require just one palloc per event, rather than
      two or three.  This saves space when there are a lot of events,
      in fact more than enough to buy back the space needed for the hash
      table.
      
      Patch by me, based on discussions around a different patch
      submitted by Filip Rembiałkowski.
      
      Discussion: https://postgr.es/m/17822.1564186806@sss.pgh.pa.us
      bb5ae8f6
    • Tom Lane's avatar
      Doc: improve documentation about postgresql.auto.conf. · 45aaaa42
      Tom Lane authored
      Clarify what external tools can do to this file, and add a bit
      of detail about what ALTER SYSTEM itself does.
      
      Discussion: https://postgr.es/m/aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com
      45aaaa42
  2. 14 Aug, 2019 5 commits
  3. 13 Aug, 2019 6 commits
  4. 12 Aug, 2019 5 commits
    • Peter Geoghegan's avatar
      amcheck: Skip unlogged relations during recovery. · 6754fe65
      Peter Geoghegan authored
      contrib/amcheck failed to consider the possibility that unlogged
      relations will not have any main relation fork files when running in hot
      standby mode.  This led to low-level "can't happen" errors that complain
      about the absence of a relfilenode file.
      
      To fix, simply skip verification of unlogged index relations during
      recovery.  In passing, add a direct check for the presence of a main
      fork just before verification proper begins, so that we cleanly verify
      the presence of the main relation fork file.
      
      Author: Andrey Borodin, Peter Geoghegan
      Reported-By: Andrey Borodin
      Diagnosed-By: Andrey Borodin
      Discussion: https://postgr.es/m/DA9B33AC-53CB-4643-96D4-7A0BBC037FA1@yandex-team.ru
      Backpatch: 10-, where amcheck was introduced.
      6754fe65
    • Tom Lane's avatar
      Fix planner's test for case-foldable characters in ILIKE with ICU. · 03c811a4
      Tom Lane authored
      As coded, the ICU-collation path in pattern_char_isalpha() failed
      to consider regular ASCII letters to be case-varying.  This led to
      like_fixed_prefix treating too much of an ILIKE pattern as being a
      fixed prefix, so that indexscans derived from an ILIKE clause might
      miss entries that they should find.
      
      Per bug #15892 from James Inform.  This is an oversight in the original
      ICU patch (commit eccfef81), so back-patch to v10 where that came in.
      
      Discussion: https://postgr.es/m/15892-e5d2bea3e8a04a1b@postgresql.org
      03c811a4
    • Tom Lane's avatar
      Remove EState.es_range_table_array. · 3c926587
      Tom Lane authored
      Now that list_nth is O(1), there's no good reason to maintain a
      separate array of RTE pointers rather than indexing into
      estate->es_range_table.  Deleting the array doesn't save all that
      much either; but just on cleanliness grounds, it's better not to
      have duplicate representations of the identical information.
      
      Discussion: https://postgr.es/m/14960.1565384592@sss.pgh.pa.us
      3c926587
    • Tom Lane's avatar
      Rationalize use of list_concat + list_copy combinations. · 5ee190f8
      Tom Lane authored
      In the wake of commit 1cff1b95, the result of list_concat no longer
      shares the ListCells of the second input.  Therefore, we can replace
      "list_concat(x, list_copy(y))" with just "list_concat(x, y)".
      
      To improve call sites that were list_copy'ing the first argument,
      or both arguments, invent "list_concat_copy()" which produces a new
      list sharing no ListCells with either input.  (This is a bit faster
      than "list_concat(list_copy(x), y)" because it makes the result list
      the right size to start with.)
      
      In call sites that were not list_copy'ing the second argument, the new
      semantics mean that we are usually leaking the second List's storage,
      since typically there is no remaining pointer to it.  We considered
      inventing another list_copy variant that would list_free the second
      input, but concluded that for most call sites it isn't worth worrying
      about, given the relative compactness of the new List representation.
      (Note that in cases where such leakage would happen, the old code
      already leaked the second List's header; so we're only discussing
      the size of the leak not whether there is one.  I did adjust two or
      three places that had been troubling to free that header so that
      they manually free the whole second List.)
      
      Patch by me; thanks to David Rowley for review.
      
      Discussion: https://postgr.es/m/11587.1550975080@sss.pgh.pa.us
      5ee190f8
    • Alexander Korotkov's avatar
      Fix string comparison in jsonpath · 251c8e39
      Alexander Korotkov authored
      Take into account pg_server_to_any() may return input string "as is".
      
      Reported-by: Andrew Dunstan, Thomas Munro
      Discussion: https://postgr.es/m/0ed83a33-d900-466a-880a-70ef456c721f%402ndQuadrant.com
      Author: Alexander Korotkov, Thomas Munro
      Backpatch-through: 12
      251c8e39
  5. 11 Aug, 2019 2 commits
    • Tom Lane's avatar
      Partially revert "Insert temporary debugging output in regression tests." · b43f7c11
      Tom Lane authored
      This reverts much of commit f03a9ca4,
      but leaves the relpages/reltuples probe in select_parallel.sql.
      The pg_stat_all_tables probes are unstable enough to be annoying,
      and it no longer seems likely that they will teach us anything more
      about the underlying problem.  I'd still like some more confirmation
      though that the observed plan instability is caused by VACUUM leaving
      relpages/reltuples as zero for one of these tables.
      
      Discussion: https://postgr.es/m/CA+hUKG+0CxrKRWRMf5ymN3gm+BECHna2B-q1w8onKBep4HasUw@mail.gmail.com
      b43f7c11
    • Alexander Korotkov's avatar
      Adjust string comparison in jsonpath · d54ceb9e
      Alexander Korotkov authored
      We have implemented jsonpath string comparison using default database locale.
      However, standard requires us to compare Unicode codepoints.  This commit
      implements that, but for performance reasons we still use per-byte comparison
      for "==" operator.  Thus, for consistency other comparison operators do per-byte
      comparison if Unicode codepoints appear to be equal.
      
      In some edge cases, when same Unicode codepoints have different binary
      representations in database encoding, we diverge standard to achieve better
      performance of "==" operator.  In future to implement strict standard
      conformance, we can do normalization of input JSON strings.
      
      Original patch was written by Nikita Glukhov, rewritten by me.
      
      Reported-by: Markus Winand
      Discussion: https://postgr.es/m/8B7FA3B4-328D-43D7-95A8-37B8891B8C78%40winand.at
      Author: Nikita Glukhov, Alexander Korotkov
      Backpatch-through: 12
      d54ceb9e
  6. 10 Aug, 2019 2 commits
    • Tom Lane's avatar
      Fix "ANALYZE t, t" inside a transaction block. · cabe0f29
      Tom Lane authored
      This failed with either "tuple already updated by self" or "duplicate
      key value violates unique constraint", depending on whether the table
      had previously been analyzed or not.  The reason is that ANALYZE tried
      to insert or update the same pg_statistic rows twice, and there was no
      CommandCounterIncrement between.  So add one.  The same case works fine
      outside a transaction block, because then there's a whole transaction
      boundary between, as a consequence of the way VACUUM works.
      
      This issue has been latent all along, but the problem was unreachable
      before commit 11d8d72c added the ability to specify multiple tables
      in ANALYZE.  We could, perhaps, alternatively fix it by adding code to
      de-duplicate the list of VacuumRelations --- but that would add a
      lot of overhead to work around dumb commands, so it's not attractive.
      
      Per bug #15946 from Yaroslav Schekin.  Back-patch to v11.
      
      (Note: in v11 I also back-patched the test added by commit 23224563;
      otherwise the problem doesn't manifest in the test I added, because
      "vactst" is empty when the tests for multiple ANALYZE targets are
      reached.  That seems like not a very good thing anyway, so I did this
      rather than rethinking the choice of test case.)
      
      Discussion: https://postgr.es/m/15946-5c7570a2884a26cf@postgresql.org
      cabe0f29
    • Peter Geoghegan's avatar
      Rename tuplesort.c's SortTuple.tupindex field. · d8cd68c8
      Peter Geoghegan authored
      Rename the "tupindex" field from tuplesort.c's SortTuple struct to
      "srctape", since it can only ever be used to store a source/input tape
      number when merging external sort runs.  This has been the case since
      commit 8b304b8b, which removed replacement selection sort from
      tuplesort.c.
      d8cd68c8
  7. 09 Aug, 2019 3 commits
  8. 08 Aug, 2019 4 commits
  9. 07 Aug, 2019 9 commits
    • Tom Lane's avatar
      Doc: document permissions required for ANALYZE. · e94cd0a8
      Tom Lane authored
      VACUUM's reference page had this text, but ANALYZE's didn't.  That's
      a clear oversight given that section 5.7 explicitly delegates the
      responsibility to define permissions requirements to the individual
      commands' man pages.
      
      Per gripe from Isaac Morland.  Back-patch to all supported branches.
      
      Discussion: https://postgr.es/m/CAMsGm5fp3oBUs-2iRfii0iEO=fZuJALVyM2zJLhNTjG34gpAVQ@mail.gmail.com
      e94cd0a8
    • Alvaro Herrera's avatar
      Remove unnecessary #include <limits.h> · e1f4c481
      Alvaro Herrera authored
      This include was probably copied from tuplestore.c, but it's not needed.
      
      Extracted from a larger patch submitted by vignesh C <vignesh21@gmail.com>
      
      Discussion: https://postgr.es/m/CALDaNm1B9naPDTm3ox1m_yZvOm3KA5S4kZQSWWAeLHAQ=3gV1Q@mail.gmail.com
      e1f4c481
    • Alvaro Herrera's avatar
    • Alvaro Herrera's avatar
      Apply constraint exclusion more generally in partitioning · 4e85642d
      Alvaro Herrera authored
      We were applying constraint exclusion on the partition constraint when
      generating pruning steps for a clause, but only for the rather
      restricted situation of them being boolean OR operators; however it is
      possible to have differently shaped clauses that also benefit from
      constraint exclusion.  This applies particularly to the default
      partition since their constraints are in essence a long list of OR'ed
      subclauses ... but it applies to other cases too.  So in certain cases
      we're scanning partitions that we don't need to.
      
      Remove the specialized code in OR clauses, and add a generally
      applicable test of the clause refuting the partition constraint; mark
      the whole pruning operation as contradictory if it hits.
      
      This has the unwanted side-effect of testing some (most? all?)
      constraints more than once if constraint_exclusion=on.  That seems
      unavoidable as far as I can tell without some additional work, but
      that's not the recommended setting for that parameter anyway.
      However, because this imposes additional processing cost for all
      queries using partitioned tables, I decided not to backpatch this
      change.
      
      Author: Amit Langote, Yuzuko Hosoya, Álvaro Herrera
      Reviewers: Shawn Wang, Thibaut Madeleine, Yoshikazu Imai, Kyotaro
      Horiguchi; they were also uncredited reviewers for commit 489247b0.
      Discussion: https://postgr.es/m/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf@lab.ntt.co.jp
      4e85642d
    • Alexander Korotkov's avatar
      Fix some typos in jsonpath documentation · 1f33f211
      Alexander Korotkov authored
      Discussion: https://postgr.es/m/8B7FA3B4-328D-43D7-95A8-37B8891B8C78%40winand.at
      Author: Markus Winand
      Backpatch-through: 12
      1f33f211
    • Etsuro Fujita's avatar
      Fix typos in comments. · 68343b4a
      Etsuro Fujita authored
      68343b4a
    • Heikki Linnakangas's avatar
      Fix predicate-locking of HOT updated rows. · 1169fcf1
      Heikki Linnakangas authored
      In serializable mode, heap_hot_search_buffer() incorrectly acquired a
      predicate lock on the root tuple, not the returned tuple that satisfied
      the visibility checks. As explained in README-SSI, the predicate lock does
      not need to be copied or extended to other tuple versions, but for that to
      work, the correct, visible, tuple version must be locked in the first
      place.
      
      The original SSI commit had this bug in it, but it was fixed back in 2013,
      in commit 81fbbfe3. But unfortunately, it was reintroduced a few months
      later in commit b89e1510. Wising up from that, add a regression test
      to cover this, so that it doesn't get reintroduced again. Also, move the
      code that sets 't_self', so that it happens at the same time that the
      other HeapTuple fields are set, to make it more clear that all the code in
      the loop operate on the "current" tuple in the chain, not the root tuple.
      
      Bug spotted by Andres Freund, analysis and original fix by Thomas Munro,
      test case and some additional changes to the fix by Heikki Linnakangas.
      Backpatch to all supported versions (9.4).
      
      Discussion: https://www.postgresql.org/message-id/20190731210630.nqhszuktygwftjty%40alap3.anarazel.de
      1169fcf1
    • Michael Paquier's avatar
      Fix some incorrect parsing of time with time zone strings · 64579be6
      Michael Paquier authored
      When parsing a timetz string with a dynamic timezone abbreviation or a
      timezone not specified, it was possible to generate incorrect timestamps
      based on a date which uses some non-initialized variables if the input
      string did not specify fully a date to parse.  This is already checked
      when a full timezone spec is included in the input string, but the two
      other cases mentioned above missed the same checks.
      
      This gets fixed by generating an error as this input is invalid, or in
      short when a date is not fully specified.
      
      Valgrind was complaining about this problem.
      
      Bug: #15910
      Author: Alexander Lakhin
      Discussion: https://postgr.es/m/15910-2eba5106b9aa0c61@postgresql.org
      Backpatch-through: 9.4
      64579be6
    • Michael Paquier's avatar
      Adjust tuple data lookup logic in multi-insert logical decoding · 75c1921c
      Michael Paquier authored
      As of now, logical decoding of a multi-insert has been scanning all
      xl_multi_insert_tuple entries only if XLH_INSERT_CONTAINS_NEW_TUPLE was
      getting set in the record.  This is not an issue on HEAD as multi-insert
      records are not used for system catalogs, but the logical decoding logic
      includes all the code necessary to handle that properly, except that the
      code missed to iterate correctly over all xl_multi_insert_tuple entries
      when the flag is not set.  Hence, when trying to use multi-insert for
      system catalogs, an assertion would be triggered.
      
      An upcoming patch is going to make use of multi-insert for system
      catalogs, and this fixes the logic to make sure that all entries are
      scanned correctly without softening the existing assertions.
      
      Reported-by: Daniel Gustafsson
      Author: Michael Paquier
      Reviewed-by: Daniel Gustafsson
      Discussion: https://postgr.es/m/CBFFD532-C033-49EB-9A5A-F67EAEE9EB0B@yesql.se
      75c1921c
  10. 06 Aug, 2019 1 commit
    • Tom Lane's avatar
      Fix intarray's GiST opclasses to not fail for empty arrays with <@. · efc77cf5
      Tom Lane authored
      contrib/intarray considers "arraycol <@ constant-array" to be indexable,
      but its GiST opclass code fails to reliably find index entries for empty
      array values (which of course should trivially match such queries).
      This is because the test condition to see whether we should descend
      through a non-leaf node is wrong.
      
      Unfortunately, empty array entries could be anywhere in the index,
      as these index opclasses are currently designed.  So there's no way
      to fix this except by lobotomizing <@ indexscans to scan the whole
      index ... which is what this patch does.  That's pretty unfortunate:
      the performance is now actually worse than a seqscan, in most cases.
      We'd be better off to remove <@ from the GiST opclasses entirely,
      and perhaps a future non-back-patchable patch will do so.
      
      In the meantime, applications whose performance is adversely impacted
      have a couple of options.  They could switch to a GIN index, which
      doesn't have this bug, or they could replace "arraycol <@ constant-array"
      with "arraycol <@ constant-array AND arraycol && constant-array".
      That will provide about the same performance as before, and it will find
      all non-empty subsets of the given constant-array, which is all that
      could reliably be expected of the query before.
      
      While at it, add some more regression test cases to improve code
      coverage of contrib/intarray.
      
      In passing, adjust resize_intArrayType so that when it's returning an
      empty array, it uses construct_empty_array for that rather than
      cowboy hacking on the input array.  While the hack produces an array
      that looks valid for most purposes, it isn't bitwise equal to empty
      arrays produced by other code paths, which could have subtle odd
      effects.  I don't think this code path is performance-critical
      enough to justify such shortcuts.  (Back-patch this part only as far
      as v11; before commit 01783ac3 we were not careful about this in
      other intarray code paths either.)
      
      Back-patch the <@ fixes to all supported versions, since this was
      broken from day one.
      
      Patch by me; thanks to Alexander Korotkov for review.
      
      Discussion: https://postgr.es/m/458.1565114141@sss.pgh.pa.us
      efc77cf5