1. 11 Jul, 2018 8 commits
    • Tom Lane's avatar
      Mark built-in btree comparison functions as leakproof where it's safe. · 39a96512
      Tom Lane authored
      Generally, if the comparison operators for a datatype or pair of datatypes
      are leakproof, the corresponding btree comparison support function can be
      considered so as well.  But we had not originally worried about marking
      support functions as leakproof, reasoning that they'd not likely be used in
      queries so the marking wouldn't matter.  It turns out there's at least one
      place where it does matter: calc_arraycontsel() finds the target datatype's
      default btree comparison function and tries to use that to estimate
      selectivity, but it will be blocked in some cases if the function isn't
      leakproof.  This leads to unnecessarily poor selectivity estimates and bad
      plans, as seen in bug #15251.
      
      Hence, run around and apply proleakproof markings where the corresponding
      btree comparison operators are leakproof.  (I did eyeball each function
      to verify that it wasn't doing anything surprising, too.)
      
      This isn't a full solution to bug #15251, and it's not back-patchable
      because of the need for a catversion bump.  A more useful response probably
      is to consider whether we can check permissions on the parent table instead
      of the child.  However, this change will help in some cases where that
      won't, and it's easy enough to do in HEAD, so let's do so.
      
      Discussion: https://postgr.es/m/3876.1531261875@sss.pgh.pa.us
      39a96512
    • Tom Lane's avatar
      Fix create_scan_plan's handling of sortgrouprefs for physical tlists. · 57cd2b6e
      Tom Lane authored
      We should only run apply_pathtarget_labeling_to_tlist if CP_LABEL_TLIST
      was specified, because only in that case has use_physical_tlist checked
      that the labeling will succeed; otherwise we may get an "ORDER/GROUP BY
      expression not found in targetlist" error.  (This subsumes the previous
      test about gating_clauses, because we reset "flags" to zero earlier
      if there are gating clauses to apply.)
      
      The only known case in which a failure can occur is with a ProjectSet
      path directly atop a table scan path, although it seems likely that there
      are other cases or will be such in future.  This means that the failure
      is currently only visible in the v10 branch: 9.6 didn't have ProjectSet,
      while in v11 and HEAD, apply_scanjoin_target_to_paths for some weird
      reason is using create_projection_path not apply_projection_to_path,
      masking the problem because there's a ProjectionPath in between.
      
      Nonetheless this code is clearly wrong on its own terms, so back-patch
      to 9.6 where this logic was introduced.
      
      Per report from Regina Obe.
      
      Discussion: https://postgr.es/m/001501d40f88$75186950$5f493bf0$@pcorp.us
      57cd2b6e
    • Tom Lane's avatar
      Fix bugs with degenerate window ORDER BY clauses in GROUPS/RANGE mode. · ff4f8891
      Tom Lane authored
      nodeWindowAgg.c failed to cope with the possibility that no ordering
      columns are defined in the window frame for GROUPS mode or RANGE OFFSET
      mode, leading to assertion failures or odd errors, as reported by Masahiko
      Sawada and Lukas Eder.  In RANGE OFFSET mode, an ordering column is really
      required, so add an Assert about that.  In GROUPS mode, the code would
      work, except that the node initialization code wasn't in sync with the
      execution code about when to set up tuplestore read pointers and spare
      slots.  Fix the latter for consistency's sake (even though I think the
      changes described below make the out-of-sync cases unreachable for now).
      
      Per SQL spec, a single ordering column is required for RANGE OFFSET mode,
      and at least one ordering column is required for GROUPS mode.  The parser
      enforced the former but not the latter; add a check for that.
      
      We were able to reach the no-ordering-column cases even with fully spec
      compliant queries, though, because the planner would drop partitioning
      and ordering columns from the generated plan if they were redundant with
      earlier columns according to the redundant-pathkey logic, for instance
      "PARTITION BY x ORDER BY y" in the presence of a "WHERE x=y" qual.
      While in principle that's an optimization that could save some pointless
      comparisons at runtime, it seems unlikely to be meaningful in the real
      world.  I think this behavior was not so much an intentional optimization
      as a side-effect of an ancient decision to construct the plan node's
      ordering-column info by reverse-engineering the PathKeys of the input
      path.  If we give up redundant-column removal then it takes very little
      code to generate the plan node info directly from the WindowClause,
      ensuring that we have the expected number of ordering columns in all
      cases.  (If anyone does complain about this, the planner could perhaps
      be taught to remove redundant columns only when it's safe to do so,
      ie *not* in RANGE OFFSET mode.  But I doubt anyone ever will.)
      
      With these changes, the WindowAggPath.winpathkeys field is not used for
      anything anymore, so remove it.
      
      The test cases added here are not actually very interesting given the
      removal of the redundant-column-removal logic, but they would represent
      important corner cases if anyone ever tries to put that back.
      
      Tom Lane and Masahiko Sawada.  Back-patch to v11 where RANGE OFFSET
      and GROUPS modes were added.
      
      Discussion: https://postgr.es/m/CAD21AoDrWqycq-w_+Bx1cjc+YUhZ11XTj9rfxNiNDojjBx8Fjw@mail.gmail.com
      Discussion: https://postgr.es/m/153086788677.17476.8002640580496698831@wrigleys.postgresql.org
      ff4f8891
    • Alexander Korotkov's avatar
      Fix more wrong paths in header comments · edf59c40
      Alexander Korotkov authored
      It appears that there are more files, whose header comment paths are
      wrong.  So, fix those paths.  No backpatching per proposal of Tom Lane.
      
      Discussion: https://postgr.es/m/CAPpHfdsJyYbOj59MOQL%2B4XxdcomLSLfLqBtAvwR%2BpsCqj3ELdQ%40mail.gmail.com
      edf59c40
    • Alvaro Herrera's avatar
      Rethink how to get float.h in old Windows API for isnan/isinf · f2c58706
      Alvaro Herrera authored
      We include <float.h> in every place that needs isnan(), because MSVC
      used to require it.  However, since MSVC 2013 that's no longer necessary
      (cf. commit cec8394b), so we can retire the inclusion to a
      version-specific stanza in win32_port.h, where it doesn't need to
      pollute random .c files.  The header is of course still needed in a few
      places for other reasons.
      
      I (Álvaro) removed float.h from a few more files than in Emre's original
      patch.  This doesn't break the build in my system, but we'll see what
      the buildfarm has to say about it all.
      
      Author: Emre Hasegeli
      Discussion: https://postgr.es/m/CAE2gYzyc0+5uG+Cd9-BSL7NKC8LSHLNg1Aq2=8ubjnUwut4_iw@mail.gmail.com
      f2c58706
    • Alexander Korotkov's avatar
      Fix wrong file path in header comment · a01d0fa1
      Alexander Korotkov authored
      Header comment of shm_mq.c was mistakenly specifying path to shm_mq.h.
      It was introduced in ec9037df.  So, theoretically it could be
      backpatched to 9.4, but it doesn't seem to worth it.
      a01d0fa1
    • Thomas Munro's avatar
      Use signals for postmaster death on FreeBSD. · f98b8476
      Thomas Munro authored
      Use FreeBSD 11.2's new support for detecting parent process death to
      make PostmasterIsAlive() very cheap, as was done for Linux in an
      earlier commit.
      
      Author: Thomas Munro
      Discussion: https://postgr.es/m/7261eb39-0369-f2f4-1bb5-62f3b6083b5e@iki.fi
      f98b8476
    • Thomas Munro's avatar
      Use signals for postmaster death on Linux. · 9f095299
      Thomas Munro authored
      Linux provides a way to ask for a signal when your parent process dies.
      Use that to make PostmasterIsAlive() very cheap.
      
      Based on a suggestion from Andres Freund.
      
      Author: Thomas Munro, Heikki Linnakangas
      Reviewed-By: Michael Paquier
      Discussion: https://postgr.es/m/7261eb39-0369-f2f4-1bb5-62f3b6083b5e%40iki.fi
      Discussion: https://postgr.es/m/20180411002643.6buofht4ranhei7k%40alap3.anarazel.de
      9f095299
  2. 10 Jul, 2018 5 commits
  3. 09 Jul, 2018 9 commits
  4. 08 Jul, 2018 3 commits
    • Jeff Davis's avatar
      Fix WITH CHECK OPTION on views referencing postgres_fdw tables. · a45adc74
      Jeff Davis authored
      If a view references a foreign table, and the foreign table has a
      BEFORE INSERT trigger, then it's possible for a tuple inserted or
      updated through the view to be changed such that it violates the
      view's WITH CHECK OPTION constraint.
      
      Before this commit, postgres_fdw handled this case inconsistently. A
      RETURNING clause on the INSERT or UPDATE statement targeting the view
      would cause the finally-inserted tuple to be read back, and the WITH
      CHECK OPTION violation would throw an error. But without a RETURNING
      clause, postgres_fdw would not read the final tuple back, and WITH
      CHECK OPTION would not throw an error for the violation (or may throw
      an error when there is no real violation). AFTER ROW triggers on the
      foreign table had a similar effect as a RETURNING clause on the INSERT
      or UPDATE statement.
      
      To fix, this commit retrieves the attributes needed to enforce the
      WITH CHECK OPTION constraint along with the attributes needed for the
      RETURNING clause (if any) from the remote side. Thus, the WITH CHECK
      OPTION constraint is always evaluated against the final tuple after
      any triggers on the remote side.
      
      This fix may be considered inconsistent with CHECK constraints
      declared on foreign tables, which are not enforced locally at all
      (because the constraint is on a remote object). The discussion
      concluded that this difference is reasonable, because the WITH CHECK
      OPTION is a constraint on the local view (not any remote object);
      therefore it only makes sense to enforce its WITH CHECK OPTION
      constraint locally.
      
      Author: Etsuro Fujita
      Reviewed-by: Arthur Zakirov, Stephen Frost
      Discussion: https://www.postgresql.org/message-id/7eb58fab-fd3b-781b-ac33-f7cfec96021f%40lab.ntt.co.jp
      a45adc74
    • Peter Geoghegan's avatar
      Correct obsolete unique index insertion comment. · e915fed2
      Peter Geoghegan authored
      Commit bc292937 failed to update a comment about unique index
      checking.  _bt_insertonpg() is no longer responsible for finding an
      insertion location while preventing conflicting insertions.
      e915fed2
    • Michael Paquier's avatar
      Use access() to check file existence in GetNewRelFileNode() · 677da8c1
      Michael Paquier authored
      Previous code used BasicOpenFile() and close() just to check for a file
      collision, while there is no need to hold open a file descriptor but
      that's an overkill here.
      
      Author: Paul Guo
      Reviewed-by: Peter Eisentraut, Michael Paquier
      Discussion: https://postgr.es/m/CABQrizcUtiHaquxK=d4etBX8GF9kbZB50Nt1gO9_aN-e9SptyQ@mail.gmail.com
      677da8c1
  5. 07 Jul, 2018 1 commit
  6. 06 Jul, 2018 6 commits
  7. 05 Jul, 2018 7 commits
    • Alvaro Herrera's avatar
      logical decoding: beware of an unset specinsert change · 3ca966c0
      Alvaro Herrera authored
      Coverity complains that there is no protection in the code (at least in
      non-assertion-enabled builds) against speculative insertion failing to
      follow the expected protocol.  Add an elog(ERROR) for the case.
      3ca966c0
    • Peter Eisentraut's avatar
      doc: Reword old inheritance partitioning documentation · 0c06534b
      Peter Eisentraut authored
      Prefer to use phrases like "child" instead of "partition" when
      describing the legacy inheritance-based partitioning.  The word
      "partition" now has a fixed meaning for the built-in partitioning, so
      keeping it out of the documentation of the old method makes things
      clearer.
      
      Author: Justin Pryzby <pryzby@telsasoft.com>
      0c06534b
    • Peter Eisentraut's avatar
      doc: Fix typos · 17411e0f
      Peter Eisentraut authored
      Author: Justin Pryzby <pryzby@telsasoft.com>
      17411e0f
    • Alvaro Herrera's avatar
      Reduce cost of test_decoding's new oldest_xmin test · 8d1c1ca7
      Alvaro Herrera authored
      Change a whole-database VACUUM into doing just pg_attribute, which is
      the portion that verifies what we want it to do.  The original
      formulation wastes a lot of CPU time, which leads the test to fail when
      runtime exceeds isolationtester timeout when it's super-slow, such as
      under CLOBBER_CACHE_ALWAYS.  Per buildfarm member friarbird.
      
      It turns out that the previous shape of the test doesn't always detect
      the condition it is supposed to detect (on unpatched reorderbuffer
      code): the reason is that there is a good chance of encountering a
      xl_running_xacts record (logged every 15 seconds) before the checkpoint
      -- and because we advance the xmin when we receive that WAL record, and
      we *don't* advance the xmin twice consecutively without receiving a
      client message in between, that means the xmin is not advanced enough
      for the tuple to be pruned from pg_attribute by VACUUM.  So the test
      would spuriously pass.
      
      The reason this test deficiency wasn't detected earlier is that HOT
      pruning removes the tuple anyway, even if vacuum leaves it in place, so
      the test correctly fails (detecting the coding mistake), but for the
      wrong reason.
      
      To fix this mess, run the s0_get_changes step twice before vacuum
      instead of once: this seems to cause the xmin to be advanced reliably,
      wreaking havoc with more certainty.
      
      Author: Arseny Sher
      Discussion: https://postgr.es/m/87h8lkuxoa.fsf@ars-thinkpad
      8d1c1ca7
    • Peter Eisentraut's avatar
      Fix typo · f61988d1
      Peter Eisentraut authored
      f61988d1
    • Michael Paquier's avatar
      Prevent references to invalid relation pages after fresh promotion · 3c64dcb1
      Michael Paquier authored
      If a standby crashes after promotion before having completed its first
      post-recovery checkpoint, then the minimal recovery point which marks
      the LSN position where the cluster is able to reach consistency may be
      set to a position older than the first end-of-recovery checkpoint while
      all the WAL available should be replayed.  This leads to the instance
      thinking that it contains inconsistent pages, causing a PANIC and a hard
      instance crash even if all the WAL available has not been replayed for
      certain sets of records replayed.  When in crash recovery,
      minRecoveryPoint is expected to always be set to InvalidXLogRecPtr,
      which forces the recovery to replay all the WAL available, so this
      commit makes sure that the local copy of minRecoveryPoint from the
      control file is initialized properly and stays as it is while crash
      recovery is performed.  Once switching to archive recovery or if crash
      recovery finishes, then the local copy minRecoveryPoint can be safely
      updated.
      
      Pavan Deolasee has reported and diagnosed the failure in the first
      place, and the base fix idea to rely on the local copy of
      minRecoveryPoint comes from Kyotaro Horiguchi, which has been expanded
      into a full-fledged patch by me.  The test included in this commit has
      been written by Álvaro Herrera and Pavan Deolasee, which I have modified
      to make it faster and more reliable with sleep phases.
      
      Backpatch down to all supported versions where the bug appears, aka 9.3
      which is where the end-of-recovery checkpoint is not run by the startup
      process anymore.  The test gets easily supported down to 10, still it
      has been tested on all branches.
      
      Reported-by: Pavan Deolasee
      Diagnosed-by: Pavan Deolasee
      Reviewed-by: Pavan Deolasee, Kyotaro Horiguchi
      Author: Michael Paquier, Kyotaro Horiguchi, Pavan Deolasee, Álvaro
      Herrera
      Discussion: https://postgr.es/m/CABOikdPOewjNL=05K5CbNMxnNtXnQjhTx2F--4p4ruorCjukbA@mail.gmail.com
      3c64dcb1
    • Andres Freund's avatar
      Use context with correct lifetime in hypothetical_dense_rank_final. · 249126e7
      Andres Freund authored
      The query lifetime expression context created in
      hypothetical_dense_rank_final() was buggily allocated in the calling
      memory context. I (Andres) broke that in bf6c614a.
      
      Reported-By: Rajkumar Raghuwanshi
      Author: Amit Langote
      Discussion:  https://postgr.es/m/CAKcux6kmzWmur5HhA_aU6gYVFu0RLQdgJJ+aC9SLdcOvBSrpfA@mail.gmail.com
      Backpatch: 11-
      249126e7
  8. 04 Jul, 2018 1 commit
    • Andres Freund's avatar
      Check for interrupts inside the nbtree page deletion code. · 3a01f68e
      Andres Freund authored
      When deleting pages the nbtree code has to walk through siblings of a
      tree node. When those sibling links are corrupted that can lead to
      endless loops - which are currently not interruptible.  This is
      especially problematic if autovacuum is repeatedly blocked on such
      indexes, as it can be hard to get out of that situation without
      resorting to single user mode.
      
      Thus add interrupt checks to appropriate places in such
      loops. Unfortunately in one of the cases it's it's not easy to do so.
      
      Between 9.3 and 9.4 the page deletion (and page split) code changed
      significantly. Before it was significantly less robust against
      interruptions. Therefore don't backpatch to 9.3.
      
      Author: Andres Freund
      Discussion: https://postgr.es/m/20180627191629.wkunw2qbibnvlz53@alap3.anarazel.de
      Backpatch: 9.4-
      3a01f68e