Re: [HACKERS] a raft of parallelism-related bug fixes

[Andrew Dunstan]

On 10/17/2015 06:17 PM, Robert Haas wrote:



However, I'm pretty sure that we don't want to switch the *entire*
buildfarm to using lots of unnecessary parallelism.  What we might be
able to do is have some critters that people spin up for this precise
purpose.  Just like we currently have CLOBBER_CACHE_ALWAYS buildfarm
members, we could have GRATUITOUSLY_PARALLEL buildfarm members.  If
Andrew is willing to add buildfarm support for that option and a few
people are willing to run critters in that mode, I will be happy -
more than happy, really - to put the test code into committable form,
guarded by a #define, and away we go.





If all that is required is a #define, like CLOBBER_CACHE_ALWAYS, then no 
special buildfarm support is required - you would just add that to the 
animal's config file, more or less like this:


 config_env =>
 {
 CPPFLAGS => '-DGRATUITOUSLY_PARALLEL',
 },

I try to make things easy :-)


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Oct 17, 2015 at 4:54 PM, Robert Haas  wrote:
>
> On Sat, Oct 17, 2015 at 2:44 AM, Amit Kapila 
wrote:
> > I am not denying from that fact, the point I wanted to convey here is
that
> > the logic guarded by "params != estate->paramLI" in plpgsql_param_fetch
> > is only needed if cursors are in use otherwise we won't need them even
> > for parallel query.
>
> Well, I think what Noah and are trying to explain is that this is not
> true.  The problem is that, even if there are no cursors anywhere in
> the picture, there might be some variable in the param list that is
> not used by the parallel query but which, if evaluated, leads to an
> error.
>

I understand what Noah wants to say, it's just that I am not able to see
how that is possible by looking at current code.  The code is not straight
forward in this area, so there is a good chance that I might be missing
something.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] a raft of parallelism-related bug fixes

[Stephen Frost]

* Noah Misch (n...@leadboat.com) wrote:
> On Sat, Oct 17, 2015 at 06:17:37PM -0400, Robert Haas wrote:
> > people are willing to run critters in that mode, I will be happy -
> > more than happy, really - to put the test code into committable form,
> > guarded by a #define, and away we go.
> 
> I would make one such animal.

We're also looking at what animals it makes sense to run as part of
pginfra and I expect we'd be able to include an animal for these tests
also (though Stefan is the one really driving that effort).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] a raft of parallelism-related bug fixes

[Noah Misch]

On Sat, Oct 17, 2015 at 06:17:37PM -0400, Robert Haas wrote:
> One idea that I think would provide
> *excellent* test coverage is to take the test code included on this
> thread and run it on the buildfarm.  The idea of the code is to
> basically run the regression test suite with every parallel-eligible
> query forced to unnecessarily use parallelism.  Turning that and
> running 'make check' found, directly or indirectly, all of these bugs.
> Doing that on the whole buildfarm would probably find more.
> 
> However, I'm pretty sure that we don't want to switch the *entire*
> buildfarm to using lots of unnecessary parallelism.  What we might be
> able to do is have some critters that people spin up for this precise
> purpose.  Just like we currently have CLOBBER_CACHE_ALWAYS buildfarm
> members, we could have GRATUITOUSLY_PARALLEL buildfarm members.  If
> Andrew is willing to add buildfarm support for that option and a few

What, if anything, would this mode require beyond adding a #define?  If
nothing, it won't require specific support in the buildfarm script.
CLOBBER_CACHE_ALWAYS has no specific support.

> people are willing to run critters in that mode, I will be happy -
> more than happy, really - to put the test code into committable form,
> guarded by a #define, and away we go.

I would make one such animal.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Oct 17, 2015 at 2:41 AM, Robert Haas  wrote:
>
> On Fri, Oct 16, 2015 at 2:29 AM, Amit Kapila 
wrote:
> >> Yeah, but I think the scenario is legitimate.  When a query gets run
> >> from within PL/pgsql, parallelism is an option, at least as we have
> >> the code today.  So if a Gather were present, and the query used a
> >> parameter, then you could have this issue.  For example:
> >>
> >> SELECT * FROM bigtable WHERE unindexed_column = some_plpgsql_variable;
> >>
> >
> > I don't think for such statements the control flow will set up an
unshared
> > param list.  I have tried couple of such statements [1] and found that
> > always such parameters are set up by setup_param_list().  I think there
> > are only two possibilities which could lead to setting up of unshared
> > params:
> >
> > 1. Usage of cursors - This is already prohibited for parallel-mode.
> > 2. Usage of read-write-param - This only happens for expressions like
> > x := array_append(x, foo) (Refer exec_check_rw_parameter()).  Read-write
> > params are not used for SQL statements. So this also won't be used for
> > parallel-mode
> >
> > There is a chance that I might be missing some case where unshared
> > params will be required for parallel-mode (as of today), but if not then
> > I think we can live without current changes.
>
> *shrug*
>
> The gather-test stuff isn't failing for no reason.  Either PL/pgsql
> shouldn't be passing CURSOR_OPT_PARALLEL_OK, or having a parallel plan
> get generated there should work.  There's not a third option.
>

Agreed and on looking at code, I think in below code, if we pass
parallelOK as true for the cases where Portal is non-null, such a
problem could happen.


static int

exec_run_select(PLpgSQL_execstate *estate,

PLpgSQL_expr *expr, long maxtuples, Portal *portalP,

bool parallelOK)

{

ParamListInfo paramLI;

int rc;


/*

* On the first call for this expression generate the plan

*/

if (expr->plan == NULL)

exec_prepare_plan(estate, expr, parallelOK ?

CURSOR_OPT_PARALLEL_OK : 0);


/*

* If a portal was requested, put the query into the portal

*/

if (portalP != NULL)

{

/*

* Set up short-lived ParamListInfo

*/

paramLI = setup_unshared_param_list(estate, expr);


*portalP = SPI_cursor_open_with_paramlist(NULL, expr->plan,

  paramLI,

  estate->readonly_func);



and one such case is

exec_stmt_return_query()
{
..

if (stmt->query != NULL)

{

/* static query */

exec_run_select(estate, stmt->query, 0, , true);
..
}


In this function we are using controlled fetch mechanism (count as 50) to
fetch the tuples which we initially thought of not supporting for
parallelism,
as such a mechanism is not built for parallel workers and that is the
reason we want to prohibit cases where ever cursor is getting created.

Do we want to support parallelism for this case on the basis that this API
will eventually fetch all the tuples by using controlled fetch mechanism?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Oct 17, 2015 at 12:06 PM, Noah Misch  wrote:
>
> On Sat, Oct 17, 2015 at 11:00:57AM +0530, Amit Kapila wrote:
> > On Sat, Oct 17, 2015 at 6:15 AM, Noah Misch  wrote:
> > > On Thu, Oct 15, 2015 at 04:30:01PM +0530, Amit Kapila wrote:
> > > > On Mon, Oct 12, 2015 at 9:16 PM, Robert Haas 
wrote:
> > > > > plpgsql_param_fetch() assumes that it can detect whether it's
being
> > > > > called from copyParamList() by checking whether params !=
> > > > > estate->paramLI.  I don't know why this works, but I do know that
this
> > > > > test fails to detect the case where it's being called from
> > > > > SerializeParamList(), which causes failures in exec_eval_datum()
as
> > > > > predicted.  Calls from SerializeParamList() need the same
treatment as
> > > > > calls from copyParamList() because it, too, will try to evaluate
every
> > > > > parameter in the list.
> > > >
> > > > From what I understood by looking at code in this area, I think the
> > check
> > > > params != estate->paramLI and code under it is required for
parameters
> > > > that are setup by setup_unshared_param_list().  Now unshared params
> > > > are only created for Cursors and expressions that are passing a R/W
> > > > object pointer; for cursors we explicitly prohibit the parallel
> > > > plan generation
> > > > and I am not sure if it makes sense to generate parallel plans for
> > > > expressions
> > > > involving R/W object pointer, if we don't generate parallel plan
where
> > > > expressions involve such parameters, then SerializeParamList()
should
> > not
> > > > be affected by the check mentioned by you.
> > >
> > > The trouble comes from the opposite direction.  A
setup_unshared_param_list()
> > > list is fine under today's code, but a shared param list needs more
help.  To
> > > say it another way, parallel queries that use the shared
estate->paramLI need,
> > > among other help, the logic now guarded by "params !=
estate->paramLI".
> > >
> >
> > Why would a parallel query need such a logic, that logic is needed
mainly
> > for cursor with params and we don't want a parallelize such cases?
>
> This is not about mixing cursors with parallelism.  Cursors get special
> treatment because each cursor copies its param list.  Parallel query also
> copies (more precisely, serializes) its param list.  You need certain
logic
> for every param list subject to being copied.
>

I am not denying from that fact, the point I wanted to convey here is that
the logic guarded by "params != estate->paramLI" in plpgsql_param_fetch
is only needed if cursors are in use otherwise we won't need them even
for parallel query.



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Sat, Oct 17, 2015 at 11:45 AM, Amit Kapila 
wrote:
>
> Agreed and on looking at code, I think in below code, if we pass
> parallelOK as true for the cases where Portal is non-null, such a
> problem could happen.
>
>
> static int
>
> exec_run_select(PLpgSQL_execstate *estate,
>
> PLpgSQL_expr *expr, long maxtuples, Portal *portalP,
>
> bool parallelOK)
>
> {
>
> ParamListInfo paramLI;
>
> int rc;
>
>
> /*
>
> * On the first call for this expression generate the plan
>
> */
>
> if (expr->plan == NULL)
>
> exec_prepare_plan(estate, expr, parallelOK ?
>
> CURSOR_OPT_PARALLEL_OK : 0);
>
>
> /*
>
> * If a portal was requested, put the query into the portal
>
> */
>
> if (portalP != NULL)
>
> {
>
> /*
>
> * Set up short-lived ParamListInfo
>
> */
>
> paramLI = setup_unshared_param_list(estate, expr);
>
>
> *portalP = SPI_cursor_open_with_paramlist(NULL, expr->plan,
>
>   paramLI,
>
>   estate->readonly_func);
>
>
>
>
> and one such case is
>
> exec_stmt_return_query()
> {
> ..
>
> if (stmt->query != NULL)
>
> {
>
> /* static query */
>
> exec_run_select(estate, stmt->query, 0, , true);
>
> ..
> }
>
>
> In this function we are using controlled fetch mechanism (count as 50) to
> fetch the tuples which we initially thought of not supporting for
parallelism,
> as such a mechanism is not built for parallel workers and that is the
> reason we want to prohibit cases where ever cursor is getting created.
>

Here, I wanted to say that is one of the reasons for prohibiting cursors
for parallelism, certainly there are others like Backward scan.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Parallel Seq Scan

[Noah Misch]

On Sat, Oct 17, 2015 at 11:00:57AM +0530, Amit Kapila wrote:
> On Sat, Oct 17, 2015 at 6:15 AM, Noah Misch  wrote:
> > On Thu, Oct 15, 2015 at 04:30:01PM +0530, Amit Kapila wrote:
> > > On Mon, Oct 12, 2015 at 9:16 PM, Robert Haas  
> > > wrote:
> > > > plpgsql_param_fetch() assumes that it can detect whether it's being
> > > > called from copyParamList() by checking whether params !=
> > > > estate->paramLI.  I don't know why this works, but I do know that this
> > > > test fails to detect the case where it's being called from
> > > > SerializeParamList(), which causes failures in exec_eval_datum() as
> > > > predicted.  Calls from SerializeParamList() need the same treatment as
> > > > calls from copyParamList() because it, too, will try to evaluate every
> > > > parameter in the list.
> > >
> > > From what I understood by looking at code in this area, I think the
> check
> > > params != estate->paramLI and code under it is required for parameters
> > > that are setup by setup_unshared_param_list().  Now unshared params
> > > are only created for Cursors and expressions that are passing a R/W
> > > object pointer; for cursors we explicitly prohibit the parallel
> > > plan generation
> > > and I am not sure if it makes sense to generate parallel plans for
> > > expressions
> > > involving R/W object pointer, if we don't generate parallel plan where
> > > expressions involve such parameters, then SerializeParamList() should
> not
> > > be affected by the check mentioned by you.
> >
> > The trouble comes from the opposite direction.  A 
> > setup_unshared_param_list()
> > list is fine under today's code, but a shared param list needs more help.  
> > To
> > say it another way, parallel queries that use the shared estate->paramLI 
> > need,
> > among other help, the logic now guarded by "params != estate->paramLI".
> >
> 
> Why would a parallel query need such a logic, that logic is needed mainly
> for cursor with params and we don't want a parallelize such cases?

This is not about mixing cursors with parallelism.  Cursors get special
treatment because each cursor copies its param list.  Parallel query also
copies (more precisely, serializes) its param list.  You need certain logic
for every param list subject to being copied.  If PostgreSQL had no concept of
cursors, we'd be writing that same logic from scratch for parallel query.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] SQL function to report log message

[Jim Nasby]

On 10/15/15 11:51 PM, Pavel Stehule wrote:

I don't think so ignoring NULL in RAISE statement is good idea (it is
not safe). We can replace NULL by some string (like "NULL") by default.
I am thinking about other possibilities.


What I was trying to say is that if the argument to a USING option is 
NULL then RAISE should skip over it, as if it hadn't been applied at 
all. Similar to how the code currently tests for \0.



1. some RAISE statement flag - but there was strong disagreement when I
did it last time
2. some plpgsql GUC variables like plpgsq.raise_ignore_null
3. accept a function from this patch

Now, I am thinking so @3 is good option. It can be really useful as last
rescue for other PL without possibility to raise rich PostgreSQL
exception - currently PLPythonu, partially PLPerl (where are more
issues), probably in others.


I agree, assuming the patch exposes all the stuff you can do with USING 
in plpgsql.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] SQL function to report log message

[Pavel Stehule]

2015-10-17 18:42 GMT+02:00 Jim Nasby :

> On 10/15/15 11:51 PM, Pavel Stehule wrote:
>
>> I don't think so ignoring NULL in RAISE statement is good idea (it is
>> not safe). We can replace NULL by some string (like "NULL") by default.
>> I am thinking about other possibilities.
>>
>
> What I was trying to say is that if the argument to a USING option is NULL
> then RAISE should skip over it, as if it hadn't been applied at all.
> Similar to how the code currently tests for \0.
>

I understand, but I don't prefer this behave. The NULL is strange value and
should be signalized.


>
> 1. some RAISE statement flag - but there was strong disagreement when I
>> did it last time
>> 2. some plpgsql GUC variables like plpgsq.raise_ignore_null
>> 3. accept a function from this patch
>>
>> Now, I am thinking so @3 is good option. It can be really useful as last
>> rescue for other PL without possibility to raise rich PostgreSQL
>> exception - currently PLPythonu, partially PLPerl (where are more
>> issues), probably in others.
>>
>
> I agree, assuming the patch exposes all the stuff you can do with USING in
> plpgsql.
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

Re: [HACKERS] WIP: lookbehind constraints for our regexp engine

[Thom Brown]

On Oct 17, 2015 12:53 AM, "Tom Lane"  wrote:
>
> I've occasionally heard complaints that our regex engine only has
> lookahead constraints not lookbehind constraints, while Perl's for example
> does have those.  While I was fooling around with the other issues in that
> code, I learned enough to realize that it would not be that hard to
> implement such things, and the attached proof-of-concept patch does so
> (using Perl-compatible syntax, in case you're wondering).
>
> However, I don't feel like this is done to the point of being committable,
> because its performance leaves something to be desired in a lot of cases.
> The difficulty is that because the engine can only match in a forward
> direction, in the worst case you have to check a lookbehind constraint by
> trying to match starting from the string start to see if a match exists
> ending at the current-location where you're testing the constraint.  That
> makes it, worst case, O(N^2) to search a string of length N.  Now, you can
> improve on that greatly if you can determine that possible matches don't
> extend all the way back to the string start.  In the attached patch I use
> the "cold start pointer" returned by shortest() to decide that characters
> before the coldstart point no longer have to be rechecked.  That helps
> some, but it's not good enough because there are too many cases where the
> engine doesn't move the coldstart point forward very aggressively.
>
> It might be that that behavior itself can be improved on, which would
> be nice because there are also non-lookbehind-related scenarios where
> smarter coldstart detection would help performance.  But I have no very
> good ideas about how to do it.
>
> Another idea I've been toying with is to add logic that tries to determine
> the maximum possible match length for a regex.  If we can determine that
> for the lookbehind sub-regex, then we'd know we have to back up at most
> that far before applying the match check.  This seems promising because
> a lot of other engines don't even support variable-length lookbehind
> patterns at all, and so we'd be assured of good performance in all the
> cases that competitors can handle at all.
>
> Anyway, I'm not planning to do much more work on this right now, but
> I thought I'd throw it out there just to let people know that this seems
> within reach.  I'm curious to know how many people care, and how much.

+1

I'm one of those who wanted lookbehind, and it would give us complete
lookaround so very much welcome.

Thom

Re: [HACKERS] Parallel Seq Scan

[Robert Haas]

On Sat, Oct 17, 2015 at 2:44 AM, Amit Kapila  wrote:
> I am not denying from that fact, the point I wanted to convey here is that
> the logic guarded by "params != estate->paramLI" in plpgsql_param_fetch
> is only needed if cursors are in use otherwise we won't need them even
> for parallel query.

Well, I think what Noah and are trying to explain is that this is not
true.  The problem is that, even if there are no cursors anywhere in
the picture, there might be some variable in the param list that is
not used by the parallel query but which, if evaluated, leads to an
error.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Amit Kapila]

On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas  wrote:
>
> On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila 
wrote:
> > Another some what related point is currently we are using random()
> > function to ensure a unique name for dsm and it seems to me that
> > it is always going to generate same number on first invocation (at least
> > thats what happening on windows) due to which you are seeing the
> > error.  Another options could be to append current pid or data directory
> > path as we are doing in win32_shmem.c.  I think this could be an
> > optimization which can be done in addition to the fix attached (we can
> > do this as a separate patch as well, if we agreed to do anything).
>
> Maybe we need to be using PostmasterRandom() rather than random() for
> the control segment name.
>

+1.  Though I think it is better to investigate the actual cause before
doing
this.

> But regardless, this patch isn't the right fix.
> dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the
> event of a segment-already-exists condition, without ereporting.
>

Thats right, but in this case, it seems from what is reported, that we are
hitting Access Denied error which on retry seems to disappear (which
ideally shouldn't happen).  It's not clear to me why that is happening.

By reading code, it appears that previously when we get such errors
for main shared memory, we replaced the usage of 'Global\PostreSQL:..'
to 'Global/PostgreSQL:.. (refer GetSharedMemName()), but in case of
dsm we have already taken care of same, so not sure what else could
be reason for Access Denied error.

Dmitry, can we try to see what is the exact value of GetLastError()
when it fails (one way is to check the logs at DEBUG5 level,
_dosmaperr() will print that value or you can modify the code to see it.)


Which Windows version you are using?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Parallel Seq Scan

[Robert Haas]

On Sat, Oct 17, 2015 at 2:15 AM, Amit Kapila  wrote:
> Agreed and on looking at code, I think in below code, if we pass
> parallelOK as true for the cases where Portal is non-null, such a
> problem could happen.
>
> and one such case is
>
> exec_stmt_return_query()
> {
> ..
>
> if (stmt->query != NULL)
>
> {
>
> /* static query */
>
> exec_run_select(estate, stmt->query, 0, , true);
>
> ..
> }
>
>
> In this function we are using controlled fetch mechanism (count as 50) to
> fetch the tuples which we initially thought of not supporting for
> parallelism,
> as such a mechanism is not built for parallel workers and that is the
> reason we want to prohibit cases where ever cursor is getting created.
>
> Do we want to support parallelism for this case on the basis that this API
> will eventually fetch all the tuples by using controlled fetch mechanism?

That was my idea when I made that change, but I think it's not going
to work out well given the way the rest of the code works.  Possibly
that should be reverted for now, but maybe only after testing it.

It's worth noting that, as of commit
bfc78d7196eb28cd4e3d6c24f7e607bacecf1129, the consequences of invoking
the executor with a fetch count have been greatly reduced.
Previously, the assumption was that doing that was broken, and if you
did it you got to keep both pieces.  But that commit rejiggered things
so that your parallel plan just gets run serially in that case.  That
might not be great from a performance perspective, but it beats
undefined behavior by a wide margin.  So I suspect that there are some
decisions about where to pass CURSOR_OPT_PARALLEL_OK that need to be
revisited in the light of that change.  I haven't had time to do that
yet, but we should do it as soon as we get time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Andres Freund]

On 2015-10-17 12:49:17 +0100, Simon Riggs wrote:
> Agreed, but I don't like the idea of hardcoding something so horribly
> specific into the server.

What's that specific about accepting the value for 'disabled' for a
feature that's not supported anymore?

> I'd rather the driver added "driver=npgsql" as an additional parameter in
> the StartupMessage. We can then make the server run some driver specific
> logic, rather than hardcoding something that could cause breakage
> elsewhere. This mechanism would then be extensible to all drivers.

How could this cause breakage alsewhere?


Having to backpatch a new parameter to all supported versions seems far
more invasive than adding a guc that can only be set to one value.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] a raft of parallelism-related bug fixes

[Simon Riggs]

On 12 October 2015 at 18:04, Robert Haas  wrote:


> My recent commit of the Gather executor node has made it relatively
> simple to write code that does an end-to-end test of all of the
> parallelism-relate commits which have thus far gone into the tree.
>

I've been wanting to help here for a while, but time remains limited for
next month or so.

>From reading this my understanding is that there isn't a test suite
included with this commit?

I've tried to review the Gather node commit and I note that the commit
message contains a longer description of the functionality in that patch
than any comments in the patch as a whole. No design comments, no README,
no file header comments. For such a major feature that isn't acceptable - I
would reject a patch from others on that basis alone (and have done so). We
must keep the level of comments high if we are to encourage wider
participation in the project.

So reviewing patch 13 isn't possible without prior knowledge.

Hoping we'll be able to find some time on this at PGConf.eu; thanks for
coming over.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Simon Riggs]

On 17 October 2015 at 14:39, Tom Lane  wrote:

> Andres Freund  writes:
> > Having to backpatch a new parameter to all supported versions seems far
> > more invasive than adding a guc that can only be set to one value.
>
> Indeed.  It is completely stupid to do this in any other way except
> by reinstating ssl_renegotiation_limit as an ordinary GUC variable
> whose min and max are both zero.
>

Agreed, my suggestion requires we can set that GUC, but we can set
not-in-file also.


> Quite aside from the implementation effort of inventing some
> single-purpose kluge to do it another way, that solution would also
> cover the complaints we're doubtless gonna get that "SET
> ssl_renegotiation_limit = 0" doesn't work anymore.
>

Agreed, single purpose kluge is a bad thing.

Rough patch for the extensible, backpatchable, non-invasive proposal
attached.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


startup_option_driver.v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] checkpoint_segments upgrade recommendation?

[Michael Paquier]

On Sat, Oct 17, 2015 at 4:45 AM, Robert Haas  wrote:

> On Fri, Oct 16, 2015 at 2:52 PM, Peter Eisentraut  wrote:
> > The release notes say that checkpoint_segments has been replaced by
> > max_wal_size and min_wal_size, but there is no indication on how to
> > convert between the old and new settings.  I think a lot of people will
> > have checkpoint_segments delicately tuned, so we should at least give
> > them a hint on how to carry that forward in spirit.
>
> Yeah, it would be nice to have some guidance about that.  But do we
> know what the guidance should be?
>

I think that we should just suggest a reverse formula of the maximum soft
limit of checkpoint_segments for max_wal_size in the release notes of 9.5,
basically:
(3 * your_old_checkpoint_segments + 1) * 16MB = max_wal_size
I am not sure it is worth mentioning that one needs to be be sure to keep
some extra room to handle potential spikes because that's not a hard limit,
but people who have already played with pg_xlog on a different partition
are already aware of that after tuning checkpoint_segments.
min_wal_size is a new parameter though, I don't think it matters much to
hint users for the transfer to the new configuration...
Regards,
-- 
Michael

Re: [HACKERS] remaining open items

[Simon Riggs]

On 16 October 2015 at 20:17, Robert Haas  wrote:

>> - DDL deparsing testing module should have detected that transforms
> >> were not supported, but it failed to notice that.  There's no thread
> >> linked to this one either, but the description of the issue seems
> >> clear enough.  Alvaro, any chance that you can, as the comment says,
> >> whack it until it does?
> >
> > I've been looking at this on and off, without anything useful to share
> > yet.  One approach that was suggested (which I don't like much, but I
> > admit is a possible approach) is that we just need to remain vigilant
> > that all features that add DDL properly test the event trigger side of
> > it, just as we remain vigilant about pg_dump support without having any
> > explicit test that it works.
>
> I really, really hope that's not where we end up.  Just shoot me now.


I share your pain, but the latter appears to be the only actionable
proposal at present.

A test suite was originally written, but not committed.
Why do we need to fix DDL when pg_dump remains annoying in the same way?
Why do we need to fix it now? Surely this will break things in later
releases, but not in 9.5, since we aren't ever going to add DDL to 9.5
again.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Simon Riggs]

On 17 October 2015 at 13:27, Andres Freund  wrote:

> On 2015-10-17 12:49:17 +0100, Simon Riggs wrote:
> > Agreed, but I don't like the idea of hardcoding something so horribly
> > specific into the server.
>
> What's that specific about accepting the value for 'disabled' for a
> feature that's not supported anymore?
>

Because we don't do that in any other non-supported feature.

Do I really need to explain why a specific, hardcoded kluge is a bad idea?


> > I'd rather the driver added "driver=npgsql" as an additional parameter in
> > the StartupMessage. We can then make the server run some driver specific
> > logic, rather than hardcoding something that could cause breakage
> > elsewhere. This mechanism would then be extensible to all drivers.
>
> How could this cause breakage alsewhere?
>

Because we are adding code for use by one specific driver, but doing
nothing to ensure it runs only for that driver. We'll forget why we did
this and it could cause breakage elsewhere.


> Having to backpatch a new parameter to all supported versions seems far
> more invasive than adding a guc that can only be set to one value.
>

I doubt it, since as I pointed out the protocol already supports it. The
suggested method is principled and extensible.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Tom Lane]

Andres Freund  writes:
> Having to backpatch a new parameter to all supported versions seems far
> more invasive than adding a guc that can only be set to one value.

Indeed.  It is completely stupid to do this in any other way except
by reinstating ssl_renegotiation_limit as an ordinary GUC variable
whose min and max are both zero.

Quite aside from the implementation effort of inventing some
single-purpose kluge to do it another way, that solution would also
cover the complaints we're doubtless gonna get that "SET
ssl_renegotiation_limit = 0" doesn't work anymore.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: lookbehind constraints for our regexp engine

[David G. Johnston]

On Friday, October 16, 2015, Tom Lane  wrote:

>
> Anyway, I'm not planning to do much more work on this right now, but
> I thought I'd throw it out there just to let people know that this seems
> within reach.  I'm curious to know how many people care, and how much.
>
>
+1

It's hard to quantify how much but look behind is a tool that I find
occasion to use.  My most recent need involved ensuring that the current
position after processing two variable length matches is exactly n
characters from the start of the string.

I don't have any immediate projects that would be refactored into a more
pure SQL variation for having this feature so on that the strength is
fairly low.  But if you are going to do it then now, while it is fresh,
would be the best time.

David J.

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Simon Riggs]

On 16 October 2015 at 14:41, Shay Rojansky  wrote:

>
>> If not, the only solution I can see is for PostgreSQL to not protest if
>> it sees the
>> parameter in the startup packet.
>>
>
> Yeah, that's the ideal solution here as far as I'm concerned.
>

Agreed, but I don't like the idea of hardcoding something so horribly
specific into the server.

It seems reasonable for us to have behaviour that varies according to the
driver software that is trying to connect.

I'd rather the driver added "driver=npgsql" as an additional parameter in
the StartupMessage. We can then make the server run some driver specific
logic, rather than hardcoding something that could cause breakage
elsewhere. This mechanism would then be extensible to all drivers.

The StartupMessage already allows a variable number of parameters, so we
don't need to change the protocol. We can backpatch a simple fix to all
supported versions.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] remaining open items

[Tom Lane]

Simon Riggs  writes:
> On 16 October 2015 at 20:17, Robert Haas  wrote:
 - DDL deparsing testing module should have detected that transforms
 were not supported, but it failed to notice that.  There's no thread
 linked to this one either, but the description of the issue seems
 clear enough.  Alvaro, any chance that you can, as the comment says,
 whack it until it does?

>>> I've been looking at this on and off, without anything useful to share
>>> yet.  One approach that was suggested (which I don't like much, but I
>>> admit is a possible approach) is that we just need to remain vigilant
>>> that all features that add DDL properly test the event trigger side of
>>> it, just as we remain vigilant about pg_dump support without having any
>>> explicit test that it works.

>> I really, really hope that's not where we end up.  Just shoot me now.

> I share your pain, but the latter appears to be the only actionable
> proposal at present.

I had the idea that the test suite would consist of "run the standard
regression tests with a DDL deparsing hook installed, and see if it fails
anywhere".  This would not prove that the deparsing logic is right,
certainly, but it would at least catch errors of omission for any DDL
tested in the regression tests, which we could hope is pretty much
everything.

> Why do we need to fix DDL when pg_dump remains annoying in the same way?

It is not true that we have no test coverage for pg_dump: the pg_upgrade
test exercises pg_dump too.  The difficulty with pg_dump is that this
approach only tests dumping of objects that are left behind at the end of
the regression tests, and we get too many submissions from neatniks who
think that test cases ought to delete all the objects they create.  But
that is just a matter of needing to formulate test cases with this in
mind.

> Why do we need to fix it now? Surely this will break things in later
> releases, but not in 9.5, since we aren't ever going to add DDL to 9.5
> again.

This is a fair point.  We still need a test methodology here, but it's
not clear why it needs to be regarded as a 9.5 blocker.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Tom Lane]

Amit Kapila  writes:
> On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas  wrote:
>> Maybe we need to be using PostmasterRandom() rather than random() for
>> the control segment name.

> +1.  Though I think it is better to investigate the actual cause before
> doing this.

BackendRun() deliberately prevents that from working.  And it also sets
srandom() to a new value for each subprocess, so that AFAICS this idea
would be a net negative.  If you are seeing duplicate key values getting
selected, the problem is elsewhere.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] proposal: PL/Pythonu - function ereport

[Pavel Stehule]

but the implementation is pretty ugly :( - I didn't write C extensions for
> Python before, and the extending exception class with some methods isn't
> well supported and well documented.
>

here is new patch

cleaned, all unwanted artefacts removed. I am not sure if used way for
method registration is 100% valid, but I didn't find any related
documentation.

Regards

Pavel


>
> Any help is welcome
>
> Regards
>
> Pavel
>
>
>>
>>
>> --
>>  Craig Ringer   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index 015bbad..bf468e1
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 1205,1210 
--- 1205,1235 
  approximately the same functionality
 

+ 
+   
+Raising Errors
+ 
+
+ The plpy module provides several possibilities to
+ to raise a exception:
+
+ 
+
+ 
+  SPIError([ message [, detail [, hint [, sqlstate  [, schema  [, table  [, column  [, datatype  [, constraint ])
+  
+
+ The constructor of SPIError exception (class) supports keyword parameters. 
+ 
+ DO $$
+   raise plpy.SPIError('custom message', hint = 'It is test only');
+ $$ LANGUAGE plpythonu;
+ 
+
+  
+ 
+
+   
   
  
   
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
new file mode 100644
index 1f52af7..ac985c6
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*** EXCEPTION WHEN SQLSTATE 'SILLY' THEN
*** 422,424 
--- 422,473 
  	-- NOOP
  END
  $$ LANGUAGE plpgsql;
+ /* possibility to set all accessable fields in custom exception
+  */
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 4, in 
+ hint = 'This is hint text.')
+ PL/Python anonymous code block
+ \set VERBOSITY verbose
+ DO $$
+ raise plpy.SPIError('This is message text.',
+ detail = 'This is detail text',
+ hint = 'This is hint text.',
+ sqlstate = 'SILLY',
+ schema = 'any info about schema',
+ table = 'any info about table',
+ column = 'any info about column',
+ datatype = 'any info about datatype',
+ constraint = 'any info about constraint')
+ $$ LANGUAGE plpythonu;
+ ERROR:  SILLY: plpy.SPIError: This is message text.
+ DETAIL:  This is detail text
+ HINT:  This is hint text.
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 10, in 
+ constraint = 'any info about constraint')
+ PL/Python anonymous code block
+ SCHEMA NAME:  any info about schema
+ TABLE NAME:  any info about table
+ COLUMN NAME:  any info about column
+ DATATYPE NAME:  any info about datatype
+ CONSTRAINT NAME:  any info about constraint
+ LOCATION:  PLy_elog, plpy_elog.c:122
+ \set VERBOSITY default
+ DO $$
+ raise plpy.SPIError(detail = 'This is detail text')
+ $$ LANGUAGE plpythonu;
+ ERROR:  plpy.SPIError: 
+ DETAIL:  This is detail text
+ CONTEXT:  Traceback (most recent call last):
+   PL/Python anonymous code block, line 2, in 
+ raise plpy.SPIError(detail = 'This is detail text')
+ PL/Python anonymous code block
diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c
new file mode 100644
index 15406d6..a835af9
*** a/src/pl/plpython/plpy_elog.c
--- b/src/pl/plpython/plpy_elog.c
*** PyObject   *PLy_exc_spi_error = NULL;
*** 23,29 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position);
  static char *get_source_line(const char *src, int lineno);
  
  
--- 23,32 
  
  static void PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth);
  static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail,
! 	   char **hint, char **query, int *position,
! 	   char **schema_name, char **table_name, char **column_name,
! 	   char **datatype_name, char **constraint_name);
! 
  static char *get_source_line(const char *src, int lineno);
  
  
*** PLy_elog(int elevel, const char *fmt,...
*** 51,62 
  	char	   *hint = NULL;
  	char	   *query = NULL;
  	int			position = 0;
  
  	PyErr_Fetch(, , );
  	if (exc != NULL)
  	{
  		if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! 			PLy_get_spi_error_data(val, , , , , );
  		else 

Re: [HACKERS] dblink: add polymorphic functions.

[Joe Conway]

On 07/30/2015 09:51 AM, Tom Lane wrote:
> Joe Conway  writes:
>> What about just TYPE then?
> 
>> SELECT x::TYPE(some_expression) FROM ...
>> SELECT CAST (x AS TYPE(some_expression)) FROM ...

> The main limitation of this patch is that it won't work for call sites
> that pass pstate == NULL to LookupTypeName.  There are a fair number
> of them, some of which wouldn't care because they could never invoke
> this notation anyway, but for others we'd need to do some work to cons
> up a suitable pstate.

Sorry it took so long for me to get back to this, but any reason the
attached won't work?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 5b0d568..a1aba26 100644
*** a/src/backend/parser/parse_agg.c
--- b/src/backend/parser/parse_agg.c
*** check_agglevels_and_constraints(ParseSta
*** 485,490 
--- 485,493 
  err = _("grouping operations are not allowed in trigger WHEN conditions");
  
  			break;
+ 		case EXPR_KIND_TYPE:
+ 			/* okay */
+ 			break;
  
  			/*
  			 * There is intentionally no default: case here, so that the
*** transformWindowFuncCall(ParseState *psta
*** 842,847 
--- 845,853 
  		case EXPR_KIND_TRIGGER_WHEN:
  			err = _("window functions are not allowed in trigger WHEN conditions");
  			break;
+ 		case EXPR_KIND_TYPE:
+ 			/* okay */
+ 			break;
  
  			/*
  			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index fa77ef1..9f76ee6 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*** transformSubLink(ParseState *pstate, Sub
*** 1690,1695 
--- 1690,1696 
  		case EXPR_KIND_OFFSET:
  		case EXPR_KIND_RETURNING:
  		case EXPR_KIND_VALUES:
+ 		case EXPR_KIND_TYPE:
  			/* okay */
  			break;
  		case EXPR_KIND_CHECK_CONSTRAINT:
*** ParseExprKindName(ParseExprKind exprKind
*** 3225,3230 
--- 3226,3233 
  			return "EXECUTE";
  		case EXPR_KIND_TRIGGER_WHEN:
  			return "WHEN";
+ 		case EXPR_KIND_TYPE:
+ 			return "TYPE";
  
  			/*
  			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index 6616639..19a2684 100644
*** a/src/backend/parser/parse_type.c
--- b/src/backend/parser/parse_type.c
***
*** 19,25 
--- 19,27 
  #include "catalog/pg_type.h"
  #include "lib/stringinfo.h"
  #include "nodes/makefuncs.h"
+ #include "nodes/nodeFuncs.h"
  #include "parser/parser.h"
+ #include "parser/parse_expr.h"
  #include "parser/parse_type.h"
  #include "utils/array.h"
  #include "utils/builtins.h"
*** static int32 typenameTypeMod(ParseState
*** 52,58 
   * found but is a shell, and there is typmod decoration, an error will be
   * thrown --- this is intentional.
   *
!  * pstate is only used for error location info, and may be NULL.
   */
  Type
  LookupTypeName(ParseState *pstate, const TypeName *typeName,
--- 54,60 
   * found but is a shell, and there is typmod decoration, an error will be
   * thrown --- this is intentional.
   *
!  * In most cases pstate is only used for error location info, and may be NULL.
   */
  Type
  LookupTypeName(ParseState *pstate, const TypeName *typeName,
*** LookupTypeName(ParseState *pstate, const
*** 60,66 
  {
  	Oid			typoid;
  	HeapTuple	tup;
! 	int32		typmod;
  
  	if (typeName->names == NIL)
  	{
--- 62,68 
  {
  	Oid			typoid;
  	HeapTuple	tup;
! 	int32		typmod = -2;
  
  	if (typeName->names == NIL)
  	{
*** LookupTypeName(ParseState *pstate, const
*** 143,148 
--- 145,172 
  			format_type_be(typoid;
  		}
  	}
+ 	else if (list_length(typeName->typmods) == 1 &&
+ 			 list_length(typeName->names) == 1 &&
+ 			 strcmp(strVal(linitial(typeName->names)), "type") == 0)
+ 	{
+ 		/* TYPE(expression) notation */
+ 		Node	   *typexpr = (Node *) linitial(typeName->typmods);
+ 
+ 		/* If needed, create a dummy ParseState for transformExpr */
+ 		if (pstate == NULL)
+ 			pstate = make_parsestate(NULL);
+ 
+ 		typexpr = transformExpr(pstate, typexpr, EXPR_KIND_TYPE);
+ 
+ 		/* We needn't bother assigning collations to the expr */
+ 		/* We use the expression's type/typmod and then throw the expr away */
+ 		typoid = exprType(typexpr);
+ 		typmod = exprTypmod(typexpr);
+ 
+ 		/* If an array reference, return the array type instead */
+ 		if (typeName->arrayBounds != NIL)
+ 			typoid = get_array_type(typoid);
+ 	}
  	else
  	{
  		/* Normal reference to a type name */
*** LookupTypeName(ParseState *pstate, const
*** 192,198 
  	if (!HeapTupleIsValid(tup)) /* should not happen */
  		elog(ERROR, "cache lookup failed for type %u", typoid);
  
! 	typmod = typenameTypeMod(pstate, typeName, 

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Robert Haas]

On Oct 17, 2015, at 10:57 AM, Andres Freund  wrote:
>> Rough patch for the extensible, backpatchable, non-invasive proposal
>> attached.
> 
> This just doesn't make any sense. This way npgsql setting that flag can't be 
> released before a new set of backbranch releases are in widespread use. 
> Otherwise it'll just error out in all those, not just in 9.5 as it's now the 
> case. It breaks compatibility with all unsupported versions of postgres 
> because those will never learn to ignore this driver argument. Without any 
> need. 

Quite so.  Simon's proposal would leave a swath of devastation of exactly the 
type we're trying to fix, but on an epic scale.


...Robert

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Dangling Client Backend Process

[Alvaro Herrera]

Andres Freund wrote:
> On 2015-10-14 17:33:01 +0900, Kyotaro HORIGUCHI wrote:
> > If I recall correctly, he concerned about killing the backends
> > running transactions which could be saved. I have a sympathy with
> > the opinion.
> 
> I still don't. Leaving backends alive after postmaster has died prevents
> the auto-restart mechanism to from working from there on.  Which means
> that we'll potentially continue happily after another backend has
> PANICed and potentially corrupted shared memory. Which isn't all that
> unlikely if postmaster isn't around anymore.

I agree.  When postmaster terminates without waiting for all backends to
go away, things are going horribly wrong -- either a DBA has done
something stupid, or the system is misbehaving.  As Andres says, if
another backend dies at that point, things are even worse -- the dying
backend could have been holding a critical lwlock, for instance, or it
could have corrupted shared buffers on its way out.  It is not sensible
to leave the rest of the backends in the system still trying to run just
because there is no one there to kill them.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Allow ssl_renegotiation_limit in PG 9.5

[Andres Freund]

On October 17, 2015 4:18:50 PM GMT+02:00, Simon Riggs  
wrote:
>On 17 October 2015 at 14:39, Tom Lane  wrote:
>
>> Andres Freund  writes:
>> > Having to backpatch a new parameter to all supported versions seems
>far
>> > more invasive than adding a guc that can only be set to one value.
>>
>> Indeed.  It is completely stupid to do this in any other way except
>> by reinstating ssl_renegotiation_limit as an ordinary GUC variable
>> whose min and max are both zero.
>>
>
>Agreed, my suggestion requires we can set that GUC, but we can set
>not-in-file also.
>
>
>> Quite aside from the implementation effort of inventing some
>> single-purpose kluge to do it another way, that solution would also
>> cover the complaints we're doubtless gonna get that "SET
>> ssl_renegotiation_limit = 0" doesn't work anymore.
>>
>
>Agreed, single purpose kluge is a bad thing.
>
>Rough patch for the extensible, backpatchable, non-invasive proposal
>attached.

This just doesn't make any sense. This way npgsql setting that flag can't be 
released before a new set of backbranch releases are in widespread use. 
Otherwise it'll just error out in all those, not just in 9.5 as it's now the 
case. It breaks compatibility with all unsupported versions of postgres because 
those will never learn to ignore this driver argument. Without any need. 

Ffs all were talking about is continuing to accept a guc in 9.5+, which had 
been accepted for 10+years. 

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Proposal: SET ROLE hook

[Jim Nasby]

On 10/16/15 12:51 PM, Andres Freund wrote:

>Hmmm, do you mean essentially skip the "GRANT postgres to joe" and use a
>SECURITY DEFINER C function that does the set role to postgres under the
>covers with "GRANT EXECUTE on FUNCTION elevate_user() to joe"?

Yes.


>Being able to use something like that on existing versions would be
>very nice, but it feels kind of grotty.

Hm. To me it doesn't feel too bad - security definer functions are there
to allow to do things that users would normally not be allowed to do...


Tons of environments can't use C functions, and ignoring that it still 
feels ugly.


How much harder would it be to add SET ROLE to event triggers? (Could be 
done in conjunction with the hook; they serve different purposes.)

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] buildfarm failures on crake and sittella

[Andrew Dunstan]

On 10/16/2015 02:19 PM, Andrew Dunstan wrote:



On 10/16/2015 11:13 AM, Robert Haas wrote:

Andrew,

The FileTextArrayFDW-build failure on crake, and the RedisFDW-build
failure on sittella, are expected results of my commit
5043193b78919a1bd144563aadc2f7f726549913.  If those FDWs do not push
quals down, they just need to be updated to pass an additional NIL
argument to make_foreignscan().  If they do, they need to pass a list
of the pushed-down quals in that new argument, as described in the
above-mentioned commit.

Can you either update those FDWs or disable those builds for now so
the BF is happy?



I expect to get to it tomorrow.





I have done this and everything seems to be working. In the RedisFDW 
case, it does process certain quals (those in the form "key" = 
), but it has been doing the same thing in redisGetForeignPlan 
as filefdw does in fileGetForeignPlan, so I added the same fix i.e. 
passing back NIL as the extra parameter. I hope that's correct. If not, 
maybe we've been doing things a bit wrong for a while :-)


cheers

andrew



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] plpython is broken for recursive use

[Andrew Dunstan]

On 10/16/2015 10:03 PM, Tom Lane wrote:

I wrote:

This seems like a very Rube-Goldbergian way of setting up a local
namespace for the user-defined code.  I think perhaps what we should do
is:
1. Compile the user-supplied code directly into a code object, without
wrapping it in a "def".  (Hence, PLy_procedure_munge_source goes away
entirely, which would be nice.)  Forget about generating a code object
containing a call, too.

After further study, it appears this approach won't work because it
breaks "yield" --- AFAICT, Python only allows "yield" inside a "def".

At this point I think what we need is to find a way of passing the
function parameters honestly, that is, as actual parameters in the
manufactured call.  I've not looked into how that might be done.



+1 if it can be done

I haven't looked very closely at plpython for a long time, but anything 
else seems ugly.


cheers

andrew


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Improve the concurency of vacuum full table and select statement on the same relation

[Jim Nasby]

On 10/16/15 10:04 AM, Robert Haas wrote:

On Thu, Oct 15, 2015 at 8:28 PM, Jim Nasby  wrote:

It's just how the authors of pg_repack decided to handle it. It seems pretty
reasonable, since you probably don't want an errant DDL statement to cause
the rollback of hours or days of pg_repack work.

Ultimately, I don't think you'll find many people interested in working on
this, because the whole goal is to never need VACUUM FULL or pg_repack. If
you're clustering just for the sake of clustering, that has it's own set of
difficulties that should be addressed.


I think the topic of online table reorganization is a pretty important
one, actually.  That is a need that we have had for a long time,
creates serious operational problems for users, and it's also a need
that is not going away.  I think the chances of eliminating that need
completely, even if we rearchitected or heap storage, are nil.


Yeah, which is why I made the comment about CLUSTER.


I think the bigger issue is that it's a very hard problem to solve.


ISTM nothing can be done here until there's some way to influence how 
pages get pulled from the FSM (IE: pull from one of the first X pages 
with free space). Maybe having some way to expose that would be enough 
of a start.



pg_repack is one approach, but I've heard more than one person say
that, as C-3PO said about the asteroid, it may not be entirely stable.


I looked at it recently, and it seems to be under active development. 
But I agree it'd be better if we could handle this internally.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [DOCS] max_worker_processes on the standby

[Petr Jelinek]

On 2015-10-02 22:02, Robert Haas wrote:

On Fri, Oct 2, 2015 at 2:59 PM, Alvaro Herrera  wrote:

Robert Haas wrote:

The standby can have the feature enabled even though the master has it
disabled?  That seems like it can only lead to heartache.


Can you elaborate?


Sort of.  Our rule up until now has always been that the standby is an
exact copy of the master.  I suspect deviating from that behavior will
introduce bugs.  I suspect having the standby make data changes that
aren't WAL-logged will introduce bugs; not to be unkind, but that
certainly seems like a lesson to take from what happened with
multixacts.



I agree with that sentiment.

Attached patch adds variable to the shmem which is used for module 
activation tracking - set to true in ActiveCommitTs() and false in 
DeactivateCommitTs(). All the checks inside the commit_ts code were 
changed to use this new variable. I also removed the static variable 
Alvaro added in previous commit because it's not needed anymore.


The patch also does full cleanup of the shmem state in 
DeactivateCommitTs() so that standby does not have stale last committed 
transaction info after enable/disable/enable cycle on primary


I also removed no longer used do_wal parameters in couple of functions.

--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 24b8291..8af8dbe 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -80,11 +80,17 @@ static SlruCtlData CommitTsCtlData;
 /*
  * We keep a cache of the last value set in shared memory.  This is protected
  * by CommitTsLock.
+ *
+ * This is also good place to keep the activation status.  We need to keep
+ * the activation status separate from the GUC bellow because the standby needs
+ * to activate the module if the primary has it active independently of what
+ * track_commit_timestamp setting is on standby.
  */
 typedef struct CommitTimestampShared
 {
 	TransactionId xidLastCommit;
 	CommitTimestampEntry dataLastCommit;
+	bool	commitTsActive;
 } CommitTimestampShared;
 
 CommitTimestampShared *commitTsShared;
@@ -93,14 +99,6 @@ CommitTimestampShared *commitTsShared;
 /* GUC variable */
 bool		track_commit_timestamp;
 
-/*
- * When this is set, commit_ts is force-enabled during recovery.  This is so
- * that a standby can replay WAL records coming from a master with the setting
- * enabled.  (Note that this doesn't enable SQL access to the data; it's
- * effectively write-only until the GUC itself is enabled.)
- */
-static bool		enable_during_recovery;
-
 static void SetXidCommitTsInPage(TransactionId xid, int nsubxids,
 	 TransactionId *subxids, TimestampTz ts,
 	 RepOriginId nodeid, int pageno);
@@ -109,7 +107,7 @@ static void TransactionIdSetCommitTs(TransactionId xid, TimestampTz ts,
 static int	ZeroCommitTsPage(int pageno, bool writeXlog);
 static bool CommitTsPagePrecedes(int page1, int page2);
 static void ActivateCommitTs(void);
-static void DeactivateCommitTs(bool do_wal);
+static void DeactivateCommitTs(void);
 static void WriteZeroPageXlogRec(int pageno);
 static void WriteTruncateXlogRec(int pageno);
 static void WriteSetTimestampXlogRec(TransactionId mainxid, int nsubxids,
@@ -148,11 +146,8 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
 	TransactionId headxid;
 	TransactionId newestXact;
 
-	/*
-	 * No-op if the module is not enabled, but allow writes in a standby
-	 * during recovery.
-	 */
-	if (!track_commit_timestamp && !enable_during_recovery)
+	/* No-op if the module is not active. */
+	if (!commitTsShared->commitTsActive)
 		return;
 
 	/*
@@ -284,7 +279,7 @@ TransactionIdGetCommitTsData(TransactionId xid, TimestampTz *ts,
 	TransactionId newestCommitTs;
 
 	/* Error if module not enabled */
-	if (!track_commit_timestamp)
+	if (!commitTsShared->commitTsActive)
 		ereport(ERROR,
 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
  errmsg("could not get commit timestamp data"),
@@ -367,7 +362,7 @@ GetLatestCommitTsData(TimestampTz *ts, RepOriginId *nodeid)
 	TransactionId xid;
 
 	/* Error if module not enabled */
-	if (!track_commit_timestamp)
+	if (!commitTsShared->commitTsActive)
 		ereport(ERROR,
 (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
  errmsg("could not get commit timestamp data"),
@@ -493,6 +488,7 @@ CommitTsShmemInit(void)
 		commitTsShared->xidLastCommit = InvalidTransactionId;
 		TIMESTAMP_NOBEGIN(commitTsShared->dataLastCommit.time);
 		commitTsShared->dataLastCommit.nodeid = InvalidRepOriginId;
+		commitTsShared->commitTsActive = false;
 	}
 	else
 		Assert(found);
@@ -566,7 +562,7 @@ CompleteCommitTsInitialization(void)
 	 * any leftover data.
 	 */
 	if (!track_commit_timestamp)
-		DeactivateCommitTs(true);
+		DeactivateCommitTs();
 }
 
 /*
@@ -588,11 +584,11 @@ 

Re: [HACKERS] a raft of parallelism-related bug fixes

[Robert Haas]

On Sat, Oct 17, 2015 at 9:16 AM, Simon Riggs  wrote:
> From reading this my understanding is that there isn't a test suite included
> with this commit?

Right.  The patches on the thread contain code that can be used for
testing, but the committed code does not itself include test coverage.
I welcome thoughts on how we could perform automated testing of this
code.  I think at least part of the answer is that I need to press on
toward getting the rest of Amit's parallel sequential scan patch
committed, because then this becomes a user-visible feature and I
expect that to make it much easier to find whatever bugs remain.  A
big part of the difficulty in testing this up until now is that I've
been building towards, hey, we have parallel query.  Until we actually
do, you need to write C code to test this, which raises the bar
considerably.

Now, that does not mean we shouldn't test this in other ways, and of
course I have, and so have Amit and other people from the community -
of late, Noah Misch and Haribabu Kommi have found several bugs through
code inspection and testing, which included some of the same ones that
I was busy finding and fixing using the test code attached to this
thread.  That's one of the reasons why I wanted to press forward with
getting the fixes for those bugs committed. It's just a waste of
everybody's time if we re-finding known bugs for which fixes already
exist.

But the question of how to test this in the buildfarm is a good one,
and I don't have a complete answer.  Once the rest of this goes in,
which I hope will be soon, we can EXPLAIN or EXPLAIN ANALYZE or just
straight up run parallel queries in the regression test suite and see
that they behave as expected.  But I don't expect that to provide
terribly good test coverage.  One idea that I think would provide
*excellent* test coverage is to take the test code included on this
thread and run it on the buildfarm.  The idea of the code is to
basically run the regression test suite with every parallel-eligible
query forced to unnecessarily use parallelism.  Turning that and
running 'make check' found, directly or indirectly, all of these bugs.
Doing that on the whole buildfarm would probably find more.

However, I'm pretty sure that we don't want to switch the *entire*
buildfarm to using lots of unnecessary parallelism.  What we might be
able to do is have some critters that people spin up for this precise
purpose.  Just like we currently have CLOBBER_CACHE_ALWAYS buildfarm
members, we could have GRATUITOUSLY_PARALLEL buildfarm members.  If
Andrew is willing to add buildfarm support for that option and a few
people are willing to run critters in that mode, I will be happy -
more than happy, really - to put the test code into committable form,
guarded by a #define, and away we go.

Of course, other ideas for testing are also welcome.

> I've tried to review the Gather node commit and I note that the commit
> message contains a longer description of the functionality in that patch
> than any comments in the patch as a whole. No design comments, no README, no
> file header comments. For such a major feature that isn't acceptable - I
> would reject a patch from others on that basis alone (and have done so). We
> must keep the level of comments high if we are to encourage wider
> participation in the project.

It's good to have your perspective on how this can be improved, and
I'm definitely willing to write more documentation.  Any lack in that
area is probably due to being too close to the subject area, having
spent several years on parallelism in general, and 200+ emails on
parallel sequential scan in particular.  Your point about the lack of
a good header file comment for execParallel.c is a good one, and I'll
rectify that next week.

It's worth noting, though, that the executor files in general don't
contain great gobs of comments, and the executor README even has this
vintage 2001 comment:

XXX a great deal more documentation needs to be written here...

Well, yeah.  It's taken me a long time to understand how the executor
actually works, and there are parts of it - particularly related to
EvalPlanQual - that I still don't fully understand.  So some of the
lack of comments in, for example, nodeGather.c is because it copies
the style of other executor nodes, like nodeSeqscan.c.  It's not
exactly clear to me what more to document there.  You either
understand what a rescan node is, in which case the code for each
node's rescan method tends to be fairly self-evident, or you don't -
but that clearly shouldn't be re-explained in every file.  So I guess
what I'm saying is I could use some advice on what kinds things would
be most useful to document, and where to put that documentation.

Right now, the best explanation of how parallelism works is in
src/backend/access/transam/README.parallel -- but, as you rightly
point out, that doesn't cover the executor bits.  Should we have SGML
documentation under "VII. 

Re: [HACKERS] buildfarm failures on crake and sittella

[Robert Haas]

On Sat, Oct 17, 2015 at 12:26 PM, Andrew Dunstan  wrote:
> I have done this and everything seems to be working. In the RedisFDW case,
> it does process certain quals (those in the form "key" = ), but it
> has been doing the same thing in redisGetForeignPlan as filefdw does in
> fileGetForeignPlan, so I added the same fix i.e. passing back NIL as the
> extra parameter. I hope that's correct. If not, maybe we've been doing
> things a bit wrong for a while :-)

Thanks for getting to this so quickly.  If you're processing the quals
remotely, but not remove them from the list of quals to be checked
locally by the executor, you're fine.  If you are checking them
remotely *instead of* locally, then you need to make some changes that
should look similar to what the patch changed in postgres_fdw.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers