Skip to content

Provide FDW query time profiles #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

pramsey
Copy link

@pramsey pramsey commented Jan 8, 2016

Profile info is written to debug, so to see them you have to turn on display:

contrib_regression=# set client_min_messages = debug1;

And to compare to overall times, you have to enable overall timing:

contrib_regression=# \timing

And then you can see and compare:

contrib_regression=# SELECT count(c3) FROM ft1 t1 WHERE t1.c1 === t1.c2;
DEBUG:  FDW Query Duration: 5.853 ms
 count 
-------
     6
(1 row)

Time: 7.189 ms

@javisantana
Copy link

and the connection time? could we log it?

@pramsey
Copy link
Author

pramsey commented Jan 8, 2016

Sorry, connection time? Time between requesting and getting the connection? (Note that connections are cached, so it's a one-time overhead.)

@javisantana
Copy link

In a shared env where pgbouncer will end sessions all the time, would the connection be kept as well if the user mapping match?

@pramsey
Copy link
Author

pramsey commented Jan 8, 2016

Connections are cached per server/user combo, so same server could have many connections from different users.

@javisantana
Copy link

So when the connection is closed? in a machine with 10k users that could be a pretty interesting problem :)

@pramsey
Copy link
Author

pramsey commented Jan 8, 2016

Connections are closed when the backend shuts down. So they last the length of a particular client session. Could be quite long lived if the backend is being managed by pgbouncer.

@zenitraM
Copy link

But we could minimize that effect connecting to the foreign server using PGbouncer

rafatower pushed a commit that referenced this pull request Nov 23, 2016
When a subtransaction is aborted in plpython because of an SPI
exception, it tries to find a matching python exception in a hash
`PLy_spi_exceptions` and to make python vm raise it.

That hash is generated during module initialization, but the exception
objects are not marked to prevent the garbage collector from collecting
them, which can lead to a segmentation fault when processing any SPI
exception.

PoC to reproduce the issue:

```sql
CREATE OR REPLACE FUNCTION server_crashes()
RETURNS VOID
AS $$
    import gc
    gc.collect()
    plan = plpy.prepare('SELECT raises_an_spi_exception();', [])
    plpy.execute(plan)
$$ LANGUAGE plpythonu;

CREATE OR REPLACE FUNCTION raises_an_spi_exception()
RETURNS VOID
AS $$
DECLARE
  sql TEXT;
BEGIN
  sql = format('%I', NULL); -- NullValueNotAllowed
END
$$ LANGUAGE plpgsql;

SELECT server_crashes(); -- segfault here
```

Stacktrace of the problem (using PostgreSQL `REL9_5_STABLE` and python
`2.7.3-0ubuntu3.8` on a Ubuntu 12.04):

```
 Program received signal SIGSEGV, Segmentation fault.
 0x00007f3155c7670b in PyObject_Call (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Objects/abstract.c:2525
 2525    ../Objects/abstract.c: No such file or directory.
 (gdb) bt
 #0  0x00007f3155c7670b in PyObject_Call (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Objects/abstract.c:2525
 #1  0x00007f3155d81ab1 in PyEval_CallObjectWithKeywords (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Python/ceval.c:3890
 #2  0x00007f3155c766ed in PyObject_CallObject (o=0x7f31b7db2a30, a=0x7f31b87d17d0) at ../Objects/abstract.c:2517
 #3  0x00007f31561e112b in PLy_spi_exception_set (edata=0x7f31b8743d78, excclass=0x7f31b7db2a30) at plpy_spi.c:547
 #4  PLy_spi_subtransaction_abort (oldcontext=<optimized out>, oldowner=<optimized out>) at plpy_spi.c:527
 #5  0x00007f31561e2185 in PLy_spi_execute_plan (ob=0x7f31b87d0cd8, list=0x7f31b7c530d8, limit=0) at plpy_spi.c:307
 #6  0x00007f31561e22d4 in PLy_spi_execute (self=<optimized out>, args=0x7f31b87a6d08) at plpy_spi.c:180
 #7  0x00007f3155cda4d6 in PyCFunction_Call (func=0x7f31b7d29600, arg=0x7f31b87a6d08, kw=0x0) at ../Objects/methodobject.c:81
 #8  0x00007f3155d82383 in call_function (pp_stack=0x7fff9207e710, oparg=2) at ../Python/ceval.c:4021
 #9  0x00007f3155d7cda4 in PyEval_EvalFrameEx (f=0x7f31b8805be0, throwflag=0) at ../Python/ceval.c:2666
 #10 0x00007f3155d82898 in fast_function (func=0x7f31b88b5ed0, pp_stack=0x7fff9207ea70, n=0, na=0, nk=0) at ../Python/ceval.c:4107
 #11 0x00007f3155d82584 in call_function (pp_stack=0x7fff9207ea70, oparg=0) at ../Python/ceval.c:4042
 #12 0x00007f3155d7cda4 in PyEval_EvalFrameEx (f=0x7f31b8805a00, throwflag=0) at ../Python/ceval.c:2666
 #13 0x00007f3155d7f8a9 in PyEval_EvalCodeEx (co=0x7f31b88aa460, globals=0x7f31b8727ea0, locals=0x7f31b8727ea0, args=0x0, argcount=0, kws=0x0, kwcount=0, defs=0x0, defcount=0, closure=0x0) at ../Python/ceval.c:3253
 #14 0x00007f3155d74ff4 in PyEval_EvalCode (co=0x7f31b88aa460, globals=0x7f31b8727ea0, locals=0x7f31b8727ea0) at ../Python/ceval.c:667
 #15 0x00007f31561dc476 in PLy_procedure_call (kargs=kargs@entry=0x7f31561e5690 "args", vargs=<optimized out>, proc=0x7f31b873b2d0, proc=0x7f31b873b2d0) at plpy_exec.c:801
 #16 0x00007f31561dd9c6 in PLy_exec_function (fcinfo=fcinfo@entry=0x7f31b7c1f870, proc=0x7f31b873b2d0) at plpy_exec.c:61#17 0x00007f31561de9f9 in plpython_call_handler (fcinfo=0x7f31b7c1f870) at plpy_main.c:291
```
@Algunenano
Copy link

Closing this for now

@Algunenano Algunenano closed this Nov 30, 2018
@rafatower
Copy link

This was once in production, via debian patches. Fairly easy to bring back, if needed.

Algunenano pushed a commit that referenced this pull request Nov 30, 2018
refresh_by_match_merge() has some issues in the way it builds a SQL
query to construct the "diff" table:

1. It doesn't require the selected unique index(es) to be indimmediate.
2. It doesn't pay attention to the particular equality semantics enforced
by a given index, but just assumes that they must be those of the column
datatype's default btree opclass.
3. It doesn't check that the indexes are btrees.
4. It's insufficiently careful to ensure that the parser will pick the
intended operator when parsing the query.  (This would have been a
security bug before CVE-2018-1058.)
5. It's not careful about indexes on system columns.

The way to fix #4 is to make use of the existing code in ri_triggers.c
for generating an arbitrary binary operator clause.  I chose to move
that to ruleutils.c, since that seems a more reasonable place to be
exporting such functionality from than ri_triggers.c.

While #1, #3, and #5 are just latent given existing feature restrictions,
and #2 doesn't arise in the core system for lack of alternate opclasses
with different equality behaviors, #4 seems like an issue worth
back-patching.  That's the bulk of the change anyway, so just back-patch
the whole thing to 9.4 where this code was introduced.

Discussion: https://postgr.es/m/13836.1521413227@sss.pgh.pa.us
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants