Skip to content

P
Postgres FD Implementation
Switch branch/tag
  1. 27 Jan, 2021 3 commits
  3. 26 Jan, 2021 8 commits
    • Tom Lane's avatar
      Rethink recently-added SPI interfaces. · d5a83d79
      Tom Lane authored 
      SPI_execute_with_receiver and SPI_cursor_parse_open_with_paramlist are
new in v14 (cf. commit 2f48ede0).  Before they can get out the door,
let's change their APIs to follow the practice recently established by
SPI_prepare_extended etc: shove all optional arguments into a struct
that callers are supposed to pre-zero.  The hope is to allow future
addition of more options without either API breakage or a continuing
proliferation of new SPI entry points.  With that in mind, choose
slightly more generic names for them: SPI_execute_extended and
SPI_cursor_parse_open respectively.

Discussion: https://postgr.es/m/CAFj8pRCLPdDAETvR7Po7gC5y_ibkn_-bOzbeJb39WHms01194Q@mail.gmail.com
      d5a83d79
    • Tom Lane's avatar
      Suppress compiler warnings from commit ee895a65. · 7292fd8f
      Tom Lane authored 
      For obscure reasons, some buildfarm members are now generating
complaints about plpgsql_call_handler's "retval" variable possibly
being used uninitialized.  It seems no less safe than it was before
that commit, but these complaints are (mostly?) new.  I trust that
initializing the variable where it's declared will be enough to
shut that up.

I also notice that some compilers are warning about setjmp clobber
of the same variable, which is maybe a bit more defensible.  Mark
it volatile to silence that.

Also, rearrange the logic to give procedure_resowner a single
point of initialization, in hopes of silencing some setjmp-clobber
warnings about that.  (Marking it volatile would serve too, but
its sibling variables are depending on single assignment, so let's
stick with that method.)

Discussion: https://postgr.es/m/E1l4F1z-0000cN-Lx@gemulon.postgresql.org
      7292fd8f
    • Tom Lane's avatar
      Code review for psql's helpSQL() function. · f76a8500
      Tom Lane authored 
      The loops to identify word boundaries could access past the end of
the input string.  Likely that would never result in an actual
crash, but it makes valgrind unhappy.

The logic to try different numbers of words didn't work when the
input has two words but we only have a match to the first, eg
"\h with select".  (We must "continue" the pass loop, not "break".)

The logic to compute nl_count was bizarrely managed, and in at
least two code paths could end up calling PageOutput with
nl_count = 0, resulting in failing to paginate output that should
have been fed to the pager.  Also, in v12 and up, the nl_count
calculation hadn't been updated to account for the addition of a URL.

The PQExpBuffer holding the command syntax details wasn't freed,
resulting in a session-lifespan memory leak.

While here, improve some comments, choose a more descriptive name
for a variable, fix inconsistent datatype choice for another variable.

Per bug #16837 from Alexander Lakhin.  This code is very old,
so back-patch to all supported branches.

Kyotaro Horiguchi and Tom Lane

Discussion: https://postgr.es/m/16837-479bcd56040c71b3@postgresql.org
      f76a8500
    • Michael Paquier's avatar
      Fix memory leak when deallocating prepared statement in postgres_fdw · 7b4c6604
      Michael Paquier authored 
      The leak is minor, so no backpatch is done.  Oversight in 21734d2f.

Reported-by: Tom Lane
      7b4c6604
    • Fujii Masao's avatar
      postgres_fdw: Fix test failure with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS · 0c3fc09f
      Fujii Masao authored 
      The roles created by regression test should have names starting with
"regress_", and the test introduced in commit 411ae649 did not do that.

Per buildfarm member longfin.

Discussion: https://postgr.es/m/73fc5ae4-3c54-1262-4533-f8c547de2e60@oss.nttdata.com
      0c3fc09f
    • Fujii Masao's avatar
      postgres_fdw: Stabilize regression test for postgres_fdw_disconnect_all(). · 6adc5376
      Fujii Masao authored 
      The regression test added in commit 411ae649 caused buildfarm failures.
The cause of them was that the order of warning messages output in the test
was not stable. To fix this, this commit sets client_min_messages to ERROR
temporarily when performing the test generating those warnings.

Per buildfarm failures.

Discussion: https://postgr.es/m/2147113.1611644754@sss.pgh.pa.us
      6adc5376
    • Fujii Masao's avatar
      postgres_fdw: Add functions to discard cached connections. · 411ae649
      Fujii Masao authored 
      This commit introduces two new functions postgres_fdw_disconnect()
and postgres_fdw_disconnect_all(). The former function discards
the cached connections to the specified foreign server. The latter discards
all the cached connections. If the connection is used in the current
transaction, it's not closed and a warning message is emitted.

For example, these functions are useful when users want to explicitly
close the foreign server connections that are no longer necessary and
then to prevent them from eating up the foreign servers connections
capacity.

Author: Bharath Rupireddy, tweaked a bit by Fujii Masao
Reviewed-by: Alexey Kondratov, Zhijie Hou, Zhihong Yu, Fujii Masao
Discussion: https://postgr.es/m/CALj2ACVvrp5=AVp2PupEm+nAC8S4buqR3fJMmaCoc7ftT0aD2A@mail.gmail.com
      411ae649
    • Tom Lane's avatar
      Improve performance of repeated CALLs within plpgsql procedures. · ee895a65
      Tom Lane authored 
      This patch essentially is cleaning up technical debt left behind
by the original implementation of plpgsql procedures, particularly
commit d92bc83c.  That patch (or more precisely, follow-on patches
fixing its worst bugs) forced us to re-plan CALL and DO statements
each time through, if we're in a non-atomic context.  That wasn't
for any fundamental reason, but just because use of a saved plan
requires having a ResourceOwner to hold a reference count for the
plan, and we had no suitable resowner at hand, nor would the
available APIs support using one if we did.  While it's not that
expensive to create a "plan" for CALL/DO, the cycles do add up
in repeated executions.

This patch therefore makes the following API changes:

* GetCachedPlan/ReleaseCachedPlan are modified to let the caller
specify which resowner to use to pin the plan, rather than forcing
use of CurrentResourceOwner.

* spi.c gains a "SPI_execute_plan_extended" entry point that lets
callers say which resowner to use to pin the plan.  This borrows the
idea of an options struct from the recently added SPI_prepare_extended,
hopefully allowing future options to be added without more API breaks.
This supersedes SPI_execute_plan_with_paramlist (which I've marked
deprecated) as well as SPI_execute_plan_with_receiver (which is new
in v14, so I just took it out altogether).

* I also took the opportunity to remove the crude hack of letting
plpgsql reach into SPI private data structures to mark SPI plans as
"no_snapshot".  It's better to treat that as an option of
SPI_prepare_extended.

Now, when running a non-atomic procedure or DO block that contains
any CALL or DO commands, plpgsql creates a ResourceOwner that
will be used to pin the plans of the CALL/DO commands.  (In an
atomic context, we just use CurrentResourceOwner, as before.)
Having done this, we can just save CALL/DO plans normally,
whether or not they are used across transaction boundaries.
This seems to be good for something like 2X speedup of a CALL
of a trivial procedure with a few simple argument expressions.
By restricting the creation of an extra ResourceOwner like this,
there's essentially zero penalty in cases that can't benefit.

Pavel Stehule, with some further hacking by me

Discussion: https://postgr.es/m/CAFj8pRCLPdDAETvR7Po7gC5y_ibkn_-bOzbeJb39WHms01194Q@mail.gmail.com
      ee895a65
  5. 25 Jan, 2021 8 commits
  7. 24 Jan, 2021 6 commits
  9. 23 Jan, 2021 4 commits
  11. 22 Jan, 2021 8 commits
  13. 21 Jan, 2021 3 commits
    • Tom Lane's avatar
      Improve new wording of libpq's connection failure messages. · 27a48e5a
      Tom Lane authored 
      "connection to server so-and-so failed:" seems clearer than the
previous wording "could not connect to so-and-so:" (introduced by
52a10224), because the latter suggests a network-level connection
failure.  We're now prefixing this string to all types of connection
failures, for instance authentication failures; so we need wording
that doesn't imply a low-level error.

Per discussion with Robert Haas.

Discussion: https://postgr.es/m/CA+TgmobssJ6rS22dspWnu-oDxXevGmhMD8VcRBjmj-b9UDqRjw@mail.gmail.com
      27a48e5a
    • 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
    • Tomas Vondra's avatar
      Fix initialization of FDW batching in ExecInitModifyTable · 920f853d
      Tomas Vondra authored 
      ExecInitModifyTable has to initialize batching for all result relations,
not just the first one. Furthermore, when junk filters were necessary,
the pointer pointed past the mtstate->resultRelInfo array.

Per reports from multiple non-x86 animals (florican, locust, ...).

Discussion: https://postgr.es/m/20200628151002.7x5laxwpgvkyiu3q@development
      920f853d