1. 07 Jul, 2021 2 commits
    • Tom Lane's avatar
      Fix crash in postgres_fdw for provably-empty remote UPDATE/DELETE. · 30a35bca
      Tom Lane authored
      In 86dc9005, I'd written find_modifytable_subplan with the assumption
      that if the immediate child of a ModifyTable is a Result, it must be
      a projecting Result with a subplan.  However, if the UPDATE or DELETE
      has a provably-constant-false WHERE clause, that's not so: we'll
      generate a dummy subplan with a childless Result.  Add the missing
      null-check so we don't crash on such cases.
      
      Per report from Alexander Pyhalov.
      
      Discussion: https://postgr.es/m/b9a6f53549456b2f3e2fd150dcd79d72@postgrespro.ru
      30a35bca
    • Fujii Masao's avatar
      postgres_fdw: Tighten up allowed values for batch_size, fetch_size options. · 4173477b
      Fujii Masao authored
      Previously the values such as '100$%$#$#', '9,223,372,' were accepted and
      treated as valid integers for postgres_fdw options batch_size and fetch_size.
      Whereas this is not the case with fdw_startup_cost and fdw_tuple_cost options
      for which an error is thrown. This was because endptr was not used
      while converting strings to integers using strtol.
      
      This commit changes the logic so that it uses parse_int function
      instead of strtol as it serves the purpose by returning false in case
      if it is unable to convert the string to integer. Note that
      this function also rounds off the values such as '100.456' to 100 and
      '100.567' or '100.678' to 101.
      
      While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost options.
      
      Since parse_int and parse_real are being used for reloptions and GUCs,
      it is more appropriate to use in postgres_fdw rather than using strtol
      and strtod directly.
      
      Back-patch to v14.
      
      Author: Bharath Rupireddy
      Reviewed-by: Ashutosh Bapat, Tom Lane, Kyotaro Horiguchi, Fujii Masao
      Discussion: https://postgr.es/m/CALj2ACVMO6wY5Pc4oe1OCgUOAtdjHuFsBDw8R5uoYR86eWFQDA@mail.gmail.com
      4173477b
  2. 06 Jul, 2021 1 commit
    • Tom Lane's avatar
      Avoid doing catalog lookups in postgres_fdw's conversion_error_callback. · 86d49142
      Tom Lane authored
      As in 50371df26, this is a bad idea since the callback can't really
      know what error is being thrown and thus whether or not it is safe
      to attempt catalog accesses.  Rather than pushing said accesses into
      the mainline code where they'd usually be a waste of cycles, we can
      look at the query's rangetable instead.
      
      This change does mean that we'll be printing query aliases (if any
      were used) rather than the table or column's true name.  But that
      doesn't seem like a bad thing: it's certainly a more useful definition
      in self-join cases, for instance.  In any case, it seems unlikely that
      any applications would be depending on this detail, so it seems safe
      to change.
      
      Patch by me.  Original complaint by Andres Freund; Bharath Rupireddy
      noted the connection to conversion_error_callback.
      
      Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de
      86d49142
  3. 08 Jun, 2021 1 commit
  4. 05 Jun, 2021 1 commit
    • Tom Lane's avatar
      Fix postgres_fdw failure with whole-row Vars of type RECORD. · f61db909
      Tom Lane authored
      Commit 86dc9005 expects that FDWs can cope with whole-row Vars for
      their tables, even if the Vars are marked with vartype RECORDOID.
      Previously, whole-row Vars generated by the planner had vartype equal
      to the relevant table's rowtype OID.  (The point behind this change is
      to enable sharing of resjunk columns across inheritance child tables.)
      
      It turns out that postgres_fdw fails to cope with this, though through
      bad fortune none of its test cases exposed that.  Things mostly work,
      but when we try to read back a value of such a Var, the expected
      rowtype is not available to record_in().  Fortunately, it's not
      difficult to hack up the tupdesc that controls this process to
      substitute the foreign table's rowtype for RECORDOID.  Thus we can
      solve the runtime problem while still sharing the resjunk column
      with other tables.
      
      Per report from Alexander Pyhalov.
      
      Discussion: https://postgr.es/m/7817fb9ebd6661cdf9b67dec6e129a78@postgrespro.ru
      f61db909
  5. 13 May, 2021 1 commit
  6. 12 May, 2021 2 commits
    • Tom Lane's avatar
      Initial pgindent and pgperltidy run for v14. · def5b065
      Tom Lane authored
      Also "make reformat-dat-files".
      
      The only change worthy of note is that pgindent messed up the formatting
      of launcher.c's struct LogicalRepWorkerId, which led me to notice that
      that struct wasn't used at all anymore, so I just took it out.
      def5b065
    • Etsuro Fujita's avatar
      Fix EXPLAIN ANALYZE for async-capable nodes. · a363bc6d
      Etsuro Fujita authored
      EXPLAIN ANALYZE for an async-capable ForeignScan node associated with
      postgres_fdw is done just by using instrumentation for ExecProcNode()
      called from the node's callbacks, causing the following problems:
      
      1) If the remote table to scan is empty, the node is incorrectly
         considered as "never executed" by the command even if the node is
         executed, as ExecProcNode() isn't called from the node's callbacks at
         all in that case.
      2) The command fails to collect timings for things other than
         ExecProcNode() done in the node, such as creating a cursor for the
         node's remote query.
      
      To fix these problems, add instrumentation for async-capable nodes, and
      modify postgres_fdw accordingly.
      
      My oversight in commit 27e1f145.
      
      While at it, update a comment for the AsyncRequest struct in execnodes.h
      and the documentation for the ForeignAsyncRequest API in fdwhandler.sgml
      to match the code in ExecAsyncAppendResponse() in nodeAppend.c, and fix
      typos in comments in nodeAppend.c.
      
      Per report from Andrey Lepikhov, though I didn't use his patch.
      
      Reviewed-by: Andrey Lepikhov
      Discussion: https://postgr.es/m/2eb662bb-105d-fc20-7412-2f027cc3ca72%40postgrespro.ru
      a363bc6d
  7. 07 May, 2021 1 commit
  8. 27 Apr, 2021 1 commit
    • Fujii Masao's avatar
      Don't pass "ONLY" options specified in TRUNCATE to foreign data wrapper. · 8e9ea08b
      Fujii Masao authored
      Commit 8ff1c946 allowed TRUNCATE command to truncate foreign tables.
      Previously the information about "ONLY" options specified in TRUNCATE
      command were passed to the foreign data wrapper. Then postgres_fdw
      constructed the TRUNCATE command to issue the remote server and
      included "ONLY" options in it based on the passed information.
      
      On the other hand, "ONLY" options specified in SELECT, UPDATE or DELETE
      have no effect when accessing or modifying the remote table, i.e.,
      are not passed to the foreign data wrapper. So it's inconsistent to
      make only TRUNCATE command pass the "ONLY" options to the foreign data
      wrapper. Therefore this commit changes the TRUNCATE command so that
      it doesn't pass the "ONLY" options to the foreign data wrapper,
      for the consistency with other statements. Also this commit changes
      postgres_fdw so that it always doesn't include "ONLY" options in
      the TRUNCATE command that it constructs.
      
      Author: Fujii Masao
      Reviewed-by: Bharath Rupireddy, Kyotaro Horiguchi, Justin Pryzby, Zhihong Yu
      Discussion: https://postgr.es/m/551ed8c1-f531-818b-664a-2cecdab99cd8@oss.nttdata.com
      8e9ea08b
  9. 23 Apr, 2021 1 commit
  10. 08 Apr, 2021 1 commit
    • Fujii Masao's avatar
      Allow TRUNCATE command to truncate foreign tables. · 8ff1c946
      Fujii Masao authored
      This commit introduces new foreign data wrapper API for TRUNCATE.
      It extends TRUNCATE command so that it accepts foreign tables as
      the targets to truncate and invokes that API. Also it extends postgres_fdw
      so that it can issue TRUNCATE command to foreign servers, by adding
      new routine for that TRUNCATE API.
      
      The information about options specified in TRUNCATE command, e.g.,
      ONLY, CACADE, etc is passed to FDW via API. The list of foreign tables to
      truncate is also passed to FDW. FDW truncates the foreign data sources
      that the passed foreign tables specify, based on those information.
      For example, postgres_fdw constructs TRUNCATE command using them
      and issues it to the foreign server.
      
      For performance, TRUNCATE command invokes the FDW routine for
      TRUNCATE once per foreign server that foreign tables to truncate belong to.
      
      Author: Kazutaka Onishi, Kohei KaiGai, slightly modified by Fujii Masao
      Reviewed-by: Bharath Rupireddy, Michael Paquier, Zhihong Yu, Alvaro Herrera, Stephen Frost, Ashutosh Bapat, Amit Langote, Daniel Gustafsson, Ibrar Ahmed, Fujii Masao
      Discussion: https://postgr.es/m/CAOP8fzb_gkReLput7OvOK+8NHgw-RKqNv59vem7=524krQTcWA@mail.gmail.com
      Discussion: https://postgr.es/m/CAJuF6cMWDDqU-vn_knZgma+2GMaout68YUgn1uyDnexRhqqM5Q@mail.gmail.com
      8ff1c946
  11. 06 Apr, 2021 1 commit
    • Fujii Masao's avatar
      postgres_fdw: Allow partitions specified in LIMIT TO to be imported. · a3740c48
      Fujii Masao authored
      Commit f49bcd4e disallowed postgres_fdw to import table partitions.
      Because all data can be accessed through the partitioned table which
      is the root of the partitioning hierarchy, importing only partitioned
      table should allow access to all the data without creating extra objects.
      This is a reasonable default when importing a whole schema. But there
      may be the case where users want to explicitly import one of
      a partitioned tables' partitions.
      
      For that use case, this commit allows postgres_fdw to import tables or
      foreign tables which are partitions of some other table only when they
      are explicitly specified in LIMIT TO clause.  It doesn't change
      the behavior that any partitions not specified in LIMIT TO are
      automatically excluded in IMPORT FOREIGN SCHEMA command.
      
      Author: Matthias van de Meent
      Reviewed-by: Bernd Helmle, Amit Langote, Michael Paquier, Fujii Masao
      Discussion: https://postgr.es/m/CAEze2Whwg4i=mzApMe+PXxCEfgoZmHGqdqQFW7J4bmj_5p6t1A@mail.gmail.com
      a3740c48
  12. 31 Mar, 2021 3 commits
    • Tom Lane's avatar
      Silence compiler warning in non-assert builds. · 8998e3ca
      Tom Lane authored
      Per buildfarm.
      8998e3ca
    • Tom Lane's avatar
      Rework planning and execution of UPDATE and DELETE. · 86dc9005
      Tom Lane authored
      This patch makes two closely related sets of changes:
      
      1. For UPDATE, the subplan of the ModifyTable node now only delivers
      the new values of the changed columns (i.e., the expressions computed
      in the query's SET clause) plus row identity information such as CTID.
      ModifyTable must re-fetch the original tuple to merge in the old
      values of any unchanged columns.  The core advantage of this is that
      the changed columns are uniform across all tables of an inherited or
      partitioned target relation, whereas the other columns might not be.
      A secondary advantage, when the UPDATE involves joins, is that less
      data needs to pass through the plan tree.  The disadvantage of course
      is an extra fetch of each tuple to be updated.  However, that seems to
      be very nearly free in context; even worst-case tests don't show it to
      add more than a couple percent to the total query cost.  At some point
      it might be interesting to combine the re-fetch with the tuple access
      that ModifyTable must do anyway to mark the old tuple dead; but that
      would require a good deal of refactoring and it seems it wouldn't buy
      all that much, so this patch doesn't attempt it.
      
      2. For inherited UPDATE/DELETE, instead of generating a separate
      subplan for each target relation, we now generate a single subplan
      that is just exactly like a SELECT's plan, then stick ModifyTable
      on top of that.  To let ModifyTable know which target relation a
      given incoming row refers to, a tableoid junk column is added to
      the row identity information.  This gets rid of the horrid hack
      that was inheritance_planner(), eliminating O(N^2) planning cost
      and memory consumption in cases where there were many unprunable
      target relations.
      
      Point 2 of course requires point 1, so that there is a uniform
      definition of the non-junk columns to be returned by the subplan.
      We can't insist on uniform definition of the row identity junk
      columns however, if we want to keep the ability to have both
      plain and foreign tables in a partitioning hierarchy.  Since
      it wouldn't scale very far to have every child table have its
      own row identity column, this patch includes provisions to merge
      similar row identity columns into one column of the subplan result.
      In particular, we can merge the whole-row Vars typically used as
      row identity by FDWs into one column by pretending they are type
      RECORD.  (It's still okay for the actual composite Datums to be
      labeled with the table's rowtype OID, though.)
      
      There is more that can be done to file down residual inefficiencies
      in this patch, but it seems to be committable now.
      
      FDW authors should note several API changes:
      
      * The argument list for AddForeignUpdateTargets() has changed, and so
      has the method it must use for adding junk columns to the query.  Call
      add_row_identity_var() instead of manipulating the parse tree directly.
      You might want to reconsider exactly what you're adding, too.
      
      * PlanDirectModify() must now work a little harder to find the
      ForeignScan plan node; if the foreign table is part of a partitioning
      hierarchy then the ForeignScan might not be the direct child of
      ModifyTable.  See postgres_fdw for sample code.
      
      * To check whether a relation is a target relation, it's no
      longer sufficient to compare its relid to root->parse->resultRelation.
      Instead, check it against all_result_relids or leaf_result_relids,
      as appropriate.
      
      Amit Langote and Tom Lane
      
      Discussion: https://postgr.es/m/CA+HiwqHpHdqdDn48yCEhynnniahH78rwcrv1rEX65-fsZGBOLQ@mail.gmail.com
      86dc9005
    • Etsuro Fujita's avatar
      Add support for asynchronous execution. · 27e1f145
      Etsuro Fujita authored
      This implements asynchronous execution, which runs multiple parts of a
      non-parallel-aware Append concurrently rather than serially to improve
      performance when possible.  Currently, the only node type that can be
      run concurrently is a ForeignScan that is an immediate child of such an
      Append.  In the case where such ForeignScans access data on different
      remote servers, this would run those ForeignScans concurrently, and
      overlap the remote operations to be performed simultaneously, so it'll
      improve the performance especially when the operations involve
      time-consuming ones such as remote join and remote aggregation.
      
      We may extend this to other node types such as joins or aggregates over
      ForeignScans in the future.
      
      This also adds the support for postgres_fdw, which is enabled by the
      table-level/server-level option "async_capable".  The default is false.
      
      Robert Haas, Kyotaro Horiguchi, Thomas Munro, and myself.  This commit
      is mostly based on the patch proposed by Robert Haas, but also uses
      stuff from the patch proposed by Kyotaro Horiguchi and from the patch
      proposed by Thomas Munro.  Reviewed by Kyotaro Horiguchi, Konstantin
      Knizhnik, Andrey Lepikhov, Movead Li, Thomas Munro, Justin Pryzby, and
      others.
      
      Discussion: https://postgr.es/m/CA%2BTgmoaXQEt4tZ03FtQhnzeDEMzBck%2BLrni0UWHVVgOTnA6C1w%40mail.gmail.com
      Discussion: https://postgr.es/m/CA%2BhUKGLBRyu0rHrDCMC4%3DRn3252gogyp1SjOgG8SEKKZv%3DFwfQ%40mail.gmail.com
      Discussion: https://postgr.es/m/20200228.170650.667613673625155850.horikyota.ntt%40gmail.com
      27e1f145
  13. 30 Mar, 2021 2 commits
    • David Rowley's avatar
      Allow estimate_num_groups() to pass back further details about the estimation · ed934d4f
      David Rowley authored
      Here we add a new output parameter to estimate_num_groups() to allow it to
      inform the caller of additional, possibly useful information about the
      estimation.
      
      The new output parameter is a struct that currently contains just a single
      field with a set of flags.  This was done rather than having the flags as
      an output parameter to allow future fields to be added without having to
      change the signature of the function at a later date when we want to pass
      back further information that might not be suitable to store in the flags
      field.
      
      It seems reasonable that one day in the future that the planner would want
      to know more about the estimation. For example, how many individual sets
      of statistics was the estimation generated from?  The planner may want to
      take that into account if we ever want to consider risks as well as costs
      when generating plans.
      
      For now, there's only 1 flag we set in the flags field.  This is to
      indicate if the estimation fell back on using the hard-coded constants in
      any part of the estimation. Callers may like to change their behavior if
      this is set, and this gives them the ability to do so.  Callers may pass
      the flag pointer as NULL if they have no interest in obtaining any
      additional information about the estimate.
      
      We're not adding any actual usages of these flags here.  Some follow-up
      commits will make use of this feature.  Additionally, we're also not
      making any changes to add support for clauselist_selectivity() and
      clauselist_selectivity_ext().  However, if this is required in the future
      then the same struct being added here should be fine to use as a new
      output argument for those functions too.
      
      Author: David Rowley
      Discussion: https://postgr.es/m/CAApHDvqQqpk=1W-G_ds7A9CsXX3BggWj_7okinzkLVhDubQzjA@mail.gmail.com
      ed934d4f
    • Etsuro Fujita's avatar
      Update obsolete comment. · bc2797eb
      Etsuro Fujita authored
      Back-patch to all supported branches.
      
      Author: Etsuro Fujita
      Discussion: https://postgr.es/m/CAPmGK17DwzaSf%2BB71dhL2apXdtG-OmD6u2AL9Cq2ZmAR0%2BzapQ%40mail.gmail.com
      bc2797eb
  14. 17 Feb, 2021 1 commit
    • Tomas Vondra's avatar
      Fix tuple routing to initialize batching only for inserts · 927f453a
      Tomas Vondra authored
      A cross-partition update on a partitioned table is implemented as a
      delete followed by an insert. With foreign partitions, this was however
      causing issues, because the FDW and core may disagree on when to enable
      batching.  postgres_fdw was only allowing batching for plain inserts
      (CMD_INSERT) while core was trying to batch the insert component of the
      cross-partition update.  Fix by restricting core to apply batching only
      to plain CMD_INSERT queries.
      
      It's possible to allow batching for cross-partition updates, but that
      will require more extensive changes, so better to leave that for a
      separate patch.
      
      Author: Amit Langote
      Reviewed-by: Tomas Vondra, Takayuki Tsunakawa
      Discussion: https://postgr.es/m/20200628151002.7x5laxwpgvkyiu3q@development
      927f453a
  15. 08 Feb, 2021 1 commit
    • Heikki Linnakangas's avatar
      Fix permission checks on constraint violation errors on partitions. · 6214e2b2
      Heikki Linnakangas authored
      If a cross-partition UPDATE violates a constraint on the target partition,
      and the columns in the new partition are in different physical order than
      in the parent, the error message can reveal columns that the user does not
      have SELECT permission on. A similar bug was fixed earlier in commit
      804b6b6d.
      
      The cause of the bug is that the callers of the
      ExecBuildSlotValueDescription() function got confused when constructing
      the list of modified columns. If the tuple was routed from a parent, we
      converted the tuple to the parent's format, but the list of modified
      columns was grabbed directly from the child's RTE entry.
      
      ExecUpdateLockMode() had a similar issue. That lead to confusion on which
      columns are key columns, leading to wrong tuple lock being taken on tables
      referenced by foreign keys, when a row is updated with INSERT ON CONFLICT
      UPDATE. A new isolation test is added for that corner case.
      
      With this patch, the ri_RangeTableIndex field is no longer set for
      partitions that don't have an entry in the range table. Previously, it was
      set to the RTE entry of the parent relation, but that was confusing.
      
      NOTE: This modifies the ResultRelInfo struct, replacing the
      ri_PartitionRoot field with ri_RootResultRelInfo. That's a bit risky to
      backpatch, because it breaks any extensions accessing the field. The
      change that ri_RangeTableIndex is not set for partitions could potentially
      break extensions, too. The ResultRelInfos are visible to FDWs at least,
      and this patch required small changes to postgres_fdw. Nevertheless, this
      seem like the least bad option. I don't think these fields widely used in
      extensions; I don't think there are FDWs out there that uses the FDW
      "direct update" API, other than postgres_fdw. If there is, you will get a
      compilation error, so hopefully it is caught quickly.
      
      Backpatch to 11, where support for both cross-partition UPDATEs, and unique
      indexes on partitioned tables, were added.
      
      Reviewed-by: Amit Langote
      Security: CVE-2021-3393
      6214e2b2
  16. 05 Feb, 2021 1 commit
    • Etsuro Fujita's avatar
      postgres_fdw: Fix assertion in estimate_path_cost_size(). · 5e7fa189
      Etsuro Fujita authored
      Commit 08d2d58a added an assertion assuming that the retrieved_rows
      estimate for a foreign relation, which is re-used to cost pre-sorted
      foreign paths with local stats, is set to at least one row in
      estimate_path_cost_size(), which isn't correct because if the relation
      is a foreign table with tuples=0, the estimate would be set to 0 there
      when not using remote estimates.
      
      Per bug #16807 from Alexander Lakhin.  Back-patch to v13 where the
      aforementioned commit went in.
      
      Author: Etsuro Fujita
      Reviewed-by: Kyotaro Horiguchi
      Discussion: https://postgr.es/m/16807-9fe4e08fbaa5c7ce%40postgresql.org
      5e7fa189
  17. 26 Jan, 2021 1 commit
  18. 21 Jan, 2021 1 commit
    • Tom Lane's avatar
      Fix pull_varnos' miscomputation of relids set for a PlaceHolderVar. · 55dc86ec
      Tom Lane authored
      Previously, pull_varnos() took the relids of a PlaceHolderVar as being
      equal to the relids in its contents, but that fails to account for the
      possibility that we have to postpone evaluation of the PHV due to outer
      joins.  This could result in a malformed plan.  The known cases end up
      triggering the "failed to assign all NestLoopParams to plan nodes"
      sanity check in createplan.c, but other symptoms may be possible.
      
      The right value to use is the join level we actually intend to evaluate
      the PHV at.  We can get that from the ph_eval_at field of the associated
      PlaceHolderInfo.  However, there are some places that call pull_varnos()
      before the PlaceHolderInfos have been created; in that case, fall back
      to the conservative assumption that the PHV will be evaluated at its
      syntactic level.  (In principle this might result in missing some legal
      optimization, but I'm not aware of any cases where it's an issue in
      practice.)  Things are also a bit ticklish for calls occurring during
      deconstruct_jointree(), but AFAICS the ph_eval_at fields should have
      reached their final values by the time we need them.
      
      The main problem in making this work is that pull_varnos() has no
      way to get at the PlaceHolderInfos.  We can fix that easily, if a
      bit tediously, in HEAD by passing it the planner "root" pointer.
      In the back branches that'd cause an unacceptable API/ABI break for
      extensions, so leave the existing entry points alone and add new ones
      with the additional parameter.  (If an old entry point is called and
      encounters a PHV, it'll fall back to using the syntactic level,
      again possibly missing some valid optimization.)
      
      Back-patch to v12.  The computation is surely also wrong before that,
      but it appears that we cannot reach a bad plan thanks to join order
      restrictions imposed on the subquery that the PlaceHolderVar came from.
      The error only became reachable when commit 4be058fe allowed trivial
      subqueries to be collapsed out completely, eliminating their join order
      restrictions.
      
      Per report from Stephan Springl.
      
      Discussion: https://postgr.es/m/171041.1610849523@sss.pgh.pa.us
      55dc86ec
  19. 20 Jan, 2021 1 commit
    • Tomas Vondra's avatar
      Implement support for bulk inserts in postgres_fdw · b663a413
      Tomas Vondra authored
      Extends the FDW API to allow batching inserts into foreign tables. That
      is usually much more efficient than inserting individual rows, due to
      high latency for each round-trip to the foreign server.
      
      It was possible to implement something similar in the regular FDW API,
      but it was inconvenient and there were issues with reporting the number
      of actually inserted rows etc. This extends the FDW API with two new
      functions:
      
      * GetForeignModifyBatchSize - allows the FDW picking optimal batch size
      
      * ExecForeignBatchInsert - inserts a batch of rows at once
      
      Currently, only INSERT queries support batching. Support for DELETE and
      UPDATE may be added in the future.
      
      This also implements batching for postgres_fdw. The batch size may be
      specified using "batch_size" option both at the server and table level.
      
      The initial patch version was written by me, but it was rewritten and
      improved in many ways by Takayuki Tsunakawa.
      
      Author: Takayuki Tsunakawa
      Reviewed-by: Tomas Vondra, Amit Langote
      Discussion: https://postgr.es/m/20200628151002.7x5laxwpgvkyiu3q@development
      b663a413
  20. 02 Jan, 2021 1 commit
  21. 24 Nov, 2020 1 commit
  22. 15 Oct, 2020 1 commit
    • David Rowley's avatar
      Fixup some appendStringInfo and appendPQExpBuffer calls · 110d8172
      David Rowley authored
      A number of places were using appendStringInfo() when they could have been
      using appendStringInfoString() instead.  While there's no functionality
      change there, it's just more efficient to use appendStringInfoString()
      when no formatting is required.  Likewise for some
      appendStringInfoString() calls which were just appending a single char.
      We can just use appendStringInfoChar() for that.
      
      Additionally, many places were using appendPQExpBuffer() when they could
      have used appendPQExpBufferStr(). Change those too.
      
      Patch by Zhijie Hou, but further searching by me found significantly more
      places that deserved the same treatment.
      
      Author: Zhijie Hou, David Rowley
      Discussion: https://postgr.es/m/cb172cf4361e4c7ba7167429070979d4@G08CNEXMBPEKD05.g08.fujitsu.local
      110d8172
  23. 14 Oct, 2020 1 commit
  24. 30 Aug, 2020 1 commit
    • Tom Lane's avatar
      Redefine pg_class.reltuples to be -1 before the first VACUUM or ANALYZE. · 3d351d91
      Tom Lane authored
      Historically, we've considered the state with relpages and reltuples
      both zero as indicating that we do not know the table's tuple density.
      This is problematic because it's impossible to distinguish "never yet
      vacuumed" from "vacuumed and seen to be empty".  In particular, a user
      cannot use VACUUM or ANALYZE to override the planner's normal heuristic
      that an empty table should not be believed to be empty because it is
      probably about to get populated.  That heuristic is a good safety
      measure, so I don't care to abandon it, but there should be a way to
      override it if the table is indeed intended to stay empty.
      
      Hence, represent the initial state of ignorance by setting reltuples
      to -1 (relpages is still set to zero), and apply the minimum-ten-pages
      heuristic only when reltuples is still -1.  If the table is empty,
      VACUUM or ANALYZE (but not CREATE INDEX) will override that to
      reltuples = relpages = 0, and then we'll plan on that basis.
      
      This requires a bunch of fiddly little changes, but we can get rid of
      some ugly kluges that were formerly needed to maintain the old definition.
      
      One notable point is that FDWs' GetForeignRelSize methods will see
      baserel->tuples = -1 when no ANALYZE has been done on the foreign table.
      That seems like a net improvement, since those methods were formerly
      also in the dark about what baserel->tuples = 0 really meant.  Still,
      it is an API change.
      
      I bumped catversion because code predating this change would get confused
      by seeing reltuples = -1.
      
      Discussion: https://postgr.es/m/F02298E0-6EF4-49A1-BCB6-C484794D9ACC@thebuild.com
      3d351d91
  25. 07 Apr, 2020 1 commit
    • Tomas Vondra's avatar
      Consider Incremental Sort paths at additional places · ba3e76cc
      Tomas Vondra authored
      Commit d2d8a229 introduced Incremental Sort, but it was considered
      only in create_ordered_paths() as an alternative to regular Sort. There
      are many other places that require sorted input and might benefit from
      considering Incremental Sort too.
      
      This patch modifies a number of those places, but not all. The concern
      is that just adding Incremental Sort to any place that already adds
      Sort may increase the number of paths considered, negatively affecting
      planning time, without any benefit. So we've taken a more conservative
      approach, based on analysis of which places do affect a set of queries
      that did seem practical. This means some less common queries may not
      benefit from Incremental Sort yet.
      
      Author: Tomas Vondra
      Reviewed-by: James Coleman
      Discussion: https://postgr.es/m/CAPpHfds1waRZ=NOmueYq0sx1ZSCnt+5QJvizT8ndT2=etZEeAQ@mail.gmail.com
      ba3e76cc
  26. 01 Jan, 2020 1 commit
  27. 03 Dec, 2019 1 commit
  28. 02 Dec, 2019 1 commit
    • Tom Lane's avatar
      Make postgres_fdw's "Relations" output agree with the rest of EXPLAIN. · 4526951d
      Tom Lane authored
      The relation aliases shown in the "Relations" line for a foreign scan
      didn't always agree with those used in the rest of EXPLAIN's output.
      The regression test result changes appearing here provide examples.
      
      It's really impossible for postgres_fdw to duplicate EXPLAIN's alias
      assignment logic during postgresGetForeignRelSize(), because of the
      de-duplication that EXPLAIN does on a global basis --- and anyway,
      trying to duplicate that would be unmaintainable.  Instead, just put
      numeric rangetable indexes into the string, and convert those to
      table names/aliases in postgresExplainForeignScan, which does have
      access to the results of ruleutils.c's alias assignment logic.
      Aside from being more reliable, this shifts some work from planning
      to EXPLAIN, which is a good tradeoff for performance.  (I also
      changed from using StringInfo to using psprintf, which makes the
      code slightly simpler and reduces its memory consumption.)
      
      A kluge required by this solution is that we have to reverse-engineer
      the rtoffset applied by setrefs.c.  If that logic ever fails
      (presumably because the member tables of a join got offset by
      different amounts), we'll need some more cooperation with setrefs.c
      to keep things straight.  But for now, there's no need for that.
      
      Arguably this is a back-patchable bug fix, but since this is a mostly
      cosmetic issue and there have been no field complaints, I'll refrain
      for now.
      
      Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us
      4526951d
  29. 01 Nov, 2019 1 commit
  30. 24 Oct, 2019 1 commit
  31. 12 Aug, 2019 1 commit
    • 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
  32. 05 Aug, 2019 1 commit
  33. 03 Jul, 2019 1 commit
  34. 14 Jun, 2019 1 commit
    • Etsuro Fujita's avatar
      postgres_fdw: Fix costing of pre-sorted foreign paths with local stats. · 08d2d58a
      Etsuro Fujita authored
      Commit aa09cd24 modified estimate_path_cost_size() so that it reuses
      cached costs of a basic foreign path for a given foreign-base/join
      relation when costing pre-sorted foreign paths for that relation, but it
      incorrectly re-computed retrieved_rows, an estimated number of rows
      fetched from the remote side, which is needed for costing both the basic
      and pre-sorted foreign paths.  To fix, handle retrieved_rows the same way
      as the cached costs: store in that relation's fpinfo the retrieved_rows
      estimate computed for costing the basic foreign path, and reuse it when
      costing the pre-sorted foreign paths.  Also, reuse the rows/width
      estimates stored in that relation's fpinfo when costing the pre-sorted
      foreign paths, to make the code consistent.
      
      In commit ffab494a, to extend the costing mentioned above to the
      foreign-grouping case, I made a change to add_foreign_grouping_paths() to
      store in a given foreign-grouped relation's RelOptInfo the rows estimate
      for that relation for reuse, but this patch makes that change unnecessary
      since we already store the row estimate in that relation's fpinfo, which
      this patch reuses when costing a foreign path for that relation with the
      sortClause ordering; remove that change.
      
      In passing, fix thinko in commit 7012b132: in estimate_path_cost_size(),
      the width estimate for a given foreign-grouped relation to be stored in
      that relation's fpinfo was reset incorrectly when costing a basic foreign
      path for that relation with local stats.
      
      Apply the patch to HEAD only to avoid destabilizing existing plan choices.
      
      Author: Etsuro Fujita
      Discussion: https://postgr.es/m/CAPmGK17jaJLPDEkgnP2VmkOg=5wT8YQ1CqssU8JRpZ_NSE+dqQ@mail.gmail.com
      08d2d58a
  35. 13 Jun, 2019 1 commit
    • Etsuro Fujita's avatar
      postgres_fdw: Account for triggers in non-direct remote UPDATE planning. · 8b6da83d
      Etsuro Fujita authored
      Previously, in postgresPlanForeignModify, we planned an UPDATE operation
      on a foreign table so that we transmit only columns that were explicitly
      targets of the UPDATE, so as to avoid unnecessary data transmission, but
      if there were BEFORE ROW UPDATE triggers on the foreign table, those
      triggers might change values for non-target columns, in which case we
      would miss sending changed values for those columns.  Prevent optimizing
      away transmitting all columns if there are BEFORE ROW UPDATE triggers on
      the foreign table.
      
      This is an oversight in commit 7cbe57c3 which added triggers on foreign
      tables, so apply the patch all the way back to 9.4 where that came in.
      
      Author: Shohei Mochizuki
      Reviewed-by: Amit Langote
      Discussion: https://postgr.es/m/201905270152.x4R1q3qi014550@toshiba.co.jp
      8b6da83d